UserDefaults Crash Fix #5917
UserDefaults Crash Fix #5917
Conversation
JZDesign
changed the title
| static func cacheKey(for request: URLRequest) -> String? { | ||
| return request.url?.absoluteString | ||
| static func cacheKey(for request: URLRequest) -> CacheKey? { | ||
| return (request.url?.absoluteString.asData.md5String).map(ETagManager.CacheKey.init) |
There was a problem hiding this comment.
Using ://some-text/some-path in the file system is not a good idea
ajpallares
left a comment
ajpallares
left a comment
There was a problem hiding this comment.
I think the implementation looks mostly good. However, I have some comments about improving the code and also about tests, which I think have some issues because of the MockSimpleCache's implementation
| var workingCacheDirectory: URL? | ||
| var workingDocsDirectory: URL? |
There was a problem hiding this comment.
The cache sets the working directory once when used by the consumer. This is used to test that the clear happens at the working directory.
| func testETagIsEmptyIfThereIsNoETagSavedForThatRequestWithDifferentURL() throws { | ||
| let request1 = URLRequest(url: Self.testURL) | ||
| let request2 = URLRequest(url: Self.testURL2) | ||
|
|
||
| try self.mockStoredETagResponse(for: request1) | ||
| // Request2 will also try to load, stub a failure for it | ||
| mockCache.stubLoadFile(with: .failure(SampleError())) | ||
|
|
||
| let header = self.eTagManager.eTagHeader(for: request2, withSignatureVerification: false) | ||
| expect(header[ETagManager.eTagRequestHeader.rawValue]) == "" | ||
| } |
There was a problem hiding this comment.
I don't think these tests are very useful now using the MockSimpleCache. Probably the issue is in the MockSimpleCache implementation, which is not associating the stored data to keys, but storing data by index.
- If we change the order of the mocks, the test fails
// Request2 will also try to load, stub a failure for it
mockCache.stubLoadFile(with: .failure(SampleError()))
try self.mockStoredETagResponse(for: request1)Even more "wrong", if we only check in the test for the eTag header of request1, the test fails
func testETagIsEmptyIfThereIsNoETagSavedForThatRequestWithDifferentURL() throws {
let request1 = URLRequest(url: Self.testURL)
let request2 = URLRequest(url: Self.testURL2)
try self.mockStoredETagResponse(for: request1)
// Request2 will also try to load, stub a failure for it
mockCache.stubLoadFile(with: .failure(SampleError()))
let header1 = self.eTagManager.eTagHeader(for: request1, withSignatureVerification: false)
XCTAssertEqual(header1[ETagManager.eTagRequestHeader.rawValue], Self.testETag)
}
There was a problem hiding this comment.
I was too involved in other things to give this much attention yesterday. I started in on this today. Again, great feedback, thank you.
I've created a tailored mock for this use case. One of the reasons I had avoided it was the md5 conversions and needing to have the exact right storage URL to look it up. But you're right, these tests would have been easily breakable with the index based version.
… just in case it's needed later
|
@RCGitBot please test |
Checklist
purchases-androidand hybridsMotivation
resolves #5912
Description
Replaces the user defaults cache with a file cache to prevent crashes on watchOS (and probably TVOS)