Trusted Entitlements: update handling of 304 responses#2698
Merged
Conversation
f3654ae to
90a95d8
Compare
tonidero
reviewed
Jun 23, 2023
38c9d99 to
fd49f15
Compare
90a95d8 to
12283d5
Compare
fd49f15 to
5b3a59d
Compare
12283d5 to
5741e29
Compare
5b3a59d to
83757b0
Compare
4875780 to
6d5da7b
Compare
250270c to
b0782ab
Compare
6d5da7b to
e9e2d8e
Compare
tonidero
reviewed
Jun 27, 2023
b0782ab to
a11ae88
Compare
e9e2d8e to
4d7e631
Compare
88a3120 to
62010bf
Compare
b4e4adc to
dd41618
Compare
NachoSoto
commented
Jun 29, 2023
Contributor
Author
There was a problem hiding this comment.
This is removed from HTTPResponse now, so it no longer defaults to .notRequested.
NachoSoto
commented
Jun 29, 2023
Contributor
Author
There was a problem hiding this comment.
A nice refactor here: this is now defined on the protocol HTTPResponseType so it's available for both response types.
Also no longer accepting any String, enforcing that we use this only with HTTPClient.ResponseHeaders and not HTTPClient.RequestHeaders
1265476 to
c101849
Compare
NachoSoto
commented
Jun 29, 2023
62010bf to
e654254
Compare
fb1b4c9 to
7181dba
Compare
Contributor
Author
|
This is ready again 🎉 |
tonidero
reviewed
Jun 30, 2023
tonidero
approved these changes
Jun 30, 2023
tonidero
left a comment
Contributor
There was a problem hiding this comment.
Everything makes sense!
|
|
||
| // MARK: - VerifiedHTTPResponse | ||
|
|
||
| struct VerifiedHTTPResponse<Body: HTTPResponseBody>: HTTPResponseType { |
Contributor
There was a problem hiding this comment.
In Android, I'm still keeping only the HTTPResponse object (in android that's HTTPResult. In Android, that's created within the ETagManager, (which I admit, it's a weird responsibility to have there), but we don't need to default to NOT_REQUESTED.
Contributor
Author
There was a problem hiding this comment.
I kept this refactor cause it made things cleaner on iOS.
9d3ee66 to
92b1d1c
Compare
7181dba to
ba53303
Compare
6d66630 to
4d8fd34
Compare
Base automatically changed from
nacho/sdk-3164-ios-dont-parse-nonce-for-offerings-and-entitlement-mapping
to
main
June 30, 2023 18:50
- `ETagManager` no longer knows anything about signatures - `HTTPClient` loads cached response from `ETagManager` first before verifying signature with the cached body - Created new `VerifiedHTTPResponse`, which has the `VerificationResult`, extracted from `HTTPResponse` - Removed `VerificationResult.from(cache:response:)` - Updated `MockETagManager` to reflect behavior changed in #2666 - Removed `ETagManager` tests about verification since they're no longer relevant - Removed several `HTTPClient` tests that were checking a behavior that was impossible (for example, no verification result despite it being enabled), or that checked behavior based on the cached `VerificationResult`
ba53303 to
1876819
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changes:
ETagManagerno longer stores the verification modeVerifiedHTTPResponse, which has theVerificationResult, extracted fromHTTPResponseVerificationResult.from(cache:response:)SignatureVerificationHTTPClientTeststo test using the realETagManager, so they become more "end to end" unit tests. A lot of them were relying on implementation details of the mock instead of the realETagManager.HTTPClienttests that were checking a behavior that was impossible (for example, no verification result despite it being enabled), or that checked behavior based on the cachedVerificationResultDepends on #2679 and #2697.