Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ETagManager.httpResultFromCacheOrBackend: return response headers #2666

Merged
merged 3 commits into from
Jun 19, 2023
Merged

ETagManager.httpResultFromCacheOrBackend: return response headers #2666

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1995ca7
`ETagManager.httpResultFromCacheOrBackend`: return response headers
NachoSoto Jun 16, 2023
25510ab
Added comment
NachoSoto Jun 16, 2023
d6ac1be
Updated test
NachoSoto Jun 16, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 9 additions & 7 deletions Sources/Networking/HTTPClient/ETagManager.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,8 @@ class ETagManager {
.compactMapValues { $0 }
}

/// - Returns: `response` if a cached response couldn't be fetched,
/// or the cached `HTTPResponse`, always including the headers in `response`.
func httpResultFromCacheOrBackend(with response: HTTPResponse<Data?>,
request: URLRequest,
retried: Bool) -> HTTPResponse<Data>? {
Expand All @@ -99,7 +101,8 @@ class ETagManager {
let newResponse = storedResponse.withUpdatedValidationTime()

self.storeIfPossible(newResponse, for: request)
return newResponse.asResponse(withRequestDate: response.requestDate)
return newResponse.asResponse(withRequestDate: response.requestDate,
headers: response.responseHeaders)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will use the cached response but with the 304 response headers... I think this could be confusing...

I was thinking, we could maybe add additional properties to HTTPResponse to have the original headers and new headers (only on 304 responses). It would still be a bit confusing, but clearer that there are 2 sets of headers.

Another option would be to rename the property in HTTPResponse to latestHeaders or something and add some documentation... Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will use the cached response but with the 304 response headers... I think this could be confusing...

I mean this method is httpResultFromCacheOrBackend, it returns the cached value if getting a 304.
Another way to think about it is, this method returns the 304 response + the "missing body".

I'll add a comment mentioning that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done: 25510ab
And also updated a test to reflect this when using the mock: d6ac1be

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I think that helps 👍

}
if retried {
Logger.warn(
Expand Down Expand Up @@ -148,10 +151,6 @@ private extension ETagManager {
}
}

func storedHTTPResponse(for request: URLRequest, withRequestDate requestDate: Date?) -> HTTPResponse<Data>? {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This wasn't used.

return self.storedETagAndResponse(for: request)?.asResponse(withRequestDate: requestDate)
}

func storeStatusCodeAndResponseIfNoError(for request: URLRequest,
response: HTTPResponse<Data?>,
eTag: String) {
Expand Down Expand Up @@ -246,10 +245,13 @@ extension ETagManager.Response {
return try? JSONEncoder.default.encode(self)
}

fileprivate func asResponse(withRequestDate requestDate: Date?) -> HTTPResponse<Data> {
fileprivate func asResponse(
withRequestDate requestDate: Date?,
headers: HTTPClient.ResponseHeaders
) -> HTTPResponse<Data> {
return HTTPResponse(
statusCode: self.statusCode,
responseHeaders: [:],
responseHeaders: headers,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the core of the change.

body: self.data,
requestDate: requestDate,
verificationResult: self.verificationResult
Expand Down
25 changes: 15 additions & 10 deletions Tests/UnitTests/Networking/ETagManagerTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,17 +45,22 @@ class ETagManagerTests: TestCase {
let request = URLRequest(url: Self.testURL)
let cachedResponse = self.mockStoredETagResponse(for: Self.testURL, statusCode: .success, eTag: eTag)

let response = self.eTagManager.httpResultFromCacheOrBackend(
with: self.responseForTest(url: Self.testURL,
body: nil,
eTag: eTag,
statusCode: .notModified),
request: request,
retried: false
let response = try XCTUnwrap(
self.eTagManager.httpResultFromCacheOrBackend(
with: self.responseForTest(url: Self.testURL,
body: nil,
eTag: eTag,
statusCode: .notModified),
request: request,
retried: false
)
)
expect(response).toNot(beNil())
expect(response?.statusCode) == .success
expect(response?.body) == cachedResponse

expect(response.statusCode) == .success
expect(response.body) == cachedResponse
expect(response.responseHeaders).toNot(beEmpty())
expect(Set(response.responseHeaders.keys.compactMap { $0 as? String }))
== Set(self.getHeaders(eTag: eTag).keys)
}

func testValidationTimeIsUpdatedWhenUsingStoredResponse() throws {
Expand Down
10 changes: 8 additions & 2 deletions Tests/UnitTests/Networking/HTTPClientTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1069,11 +1069,16 @@ final class HTTPClientTests: BaseHTTPClientTests {
let eTag = "tag"
let requestDate = Date().addingTimeInterval(-1000000)

let headers: [String: String] = [
HTTPClient.ResponseHeader.contentType.rawValue: "application/json",
HTTPClient.ResponseHeader.signature.rawValue: UUID().uuidString
]

self.eTagManager.stubResponseEtag(eTag)
self.eTagManager.shouldReturnResultFromBackend = false
self.eTagManager.stubbedHTTPResultFromCacheOrBackendResult = .init(
statusCode: .success,
responseHeaders: [:],
responseHeaders: headers,
body: mockedCachedResponse,
requestDate: requestDate,
verificationResult: .notRequested
Expand All @@ -1084,7 +1089,7 @@ final class HTTPClientTests: BaseHTTPClientTests {

return .init(data: Data(),
statusCode: .notModified,
headers: nil)
headers: headers)
}

let response: HTTPResponse<Data>.Result? = waitUntilValue { completion in
Expand All @@ -1098,6 +1103,7 @@ final class HTTPClientTests: BaseHTTPClientTests {
expect(response?.value?.body) == mockedCachedResponse
expect(response?.value?.requestDate) == requestDate
expect(response?.value?.verificationResult) == .notRequested
expect(response?.value?.responseHeaders).to(haveCount(headers.count))

expect(self.eTagManager.invokedETagHeaderParametersList).to(haveCount(1))
expect(self.eTagManager.invokedETagHeaderParameters?.withSignatureVerification) == false
Expand Down