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.

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

ETags support #509

Merged
merged 44 commits into from Jul 9, 2021
Merged

ETags support #509

merged 44 commits into from Jul 9, 2021

Conversation

vegaro
Copy link
Contributor

@vegaro vegaro commented May 22, 2021

When sending an empty X-RevenueCat-ETag header the backend sends back an etag:

X-RevenueCat-ETag: ba2d888b10b51c8a953e0d2a591fc569

If sending a future request with that etag and (assuming nothing has changed), we'll get back a 304 NOT MODIFIED with no body.

This etag support has been added to all calls. We assume two different requests with the same url can possibly have the same response and we ignore the body we send in the request. This means that two different requests, with same URL but different body, will send the same etag. This only really matters for /receipts, since getOfferings and getPurchaserInfo don't have a body.

In the case of /receipt, the first call posting a token will return a PurchaserInfo including that token. Posting another token will return a PurchaserInfo including both tokens. Future calls re-posting either token will return the same PurchaserInfo including both tokens. This is the reason why we think identifying etags by just URL (not taking into account the body of the request) is good enough.

I believe I hit a weird scenario with /subscribers never returning 304. I believe it's a backend issue. What it looks like to me is that for an already existing user, the purchaser info call is never 304. If I generate a new user, I get 304s. So I am wondering if the memcached version has something to do with it

TODO:

  • Test an extension to make sure the user defaults with custom suite name has the expected behavior.

@vegaro vegaro marked this pull request as ready for review May 26, 2021 01:31

@objc public override init() {
// TODO: do they persist after reinstalling
let bundleID: String = Bundle.main.bundleIdentifier ?? ""
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I ended up doing this @aboedo but I am not sure it's correct, and it wasn't very clear in the docs either.

Copy link
Member

Choose a reason for hiding this comment

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

I think this should be fine.
However, it's worth running a quick test with an app extension, to make absolutely sure that we don't get unexpected behavior, since UserDefaults with custom suite names are shared between apps and extensions.
The bundle id should prevent it, though.

Copy link
Member

Choose a reason for hiding this comment

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

the type here can be omitted, since it can easily be inferred as a String

@vegaro vegaro added the HOLD label May 26, 2021
@vegaro vegaro requested a review from a team May 27, 2021 20:05
@vegaro
Copy link
Contributor Author

vegaro commented May 27, 2021

Added the HOLD label to not merge this until the backend issue is figured out

Examples/LegacySwiftExample/SwiftExample/AppDelegate.swift Outdated Show resolved Hide resolved
Purchases/Networking/RCHTTPClient.m Outdated Show resolved Hide resolved
Purchases/Networking/RCHTTPClient.m Outdated Show resolved Hide resolved

@objc public override init() {
// TODO: do they persist after reinstalling
let bundleID: String = Bundle.main.bundleIdentifier ?? ""
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be fine.
However, it's worth running a quick test with an app extension, to make absolutely sure that we don't get unexpected behavior, since UserDefaults with custom suite names are shared between apps and extensions.
The bundle id should prevent it, though.

private var userDefaults: UserDefaults

@objc public override init() {
// TODO: do they persist after reinstalling
Copy link
Member

Choose a reason for hiding this comment

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

they will, sadly.
if we want to clean up our storage after a reinstall, we could just check if there's anything in device cache, and if so, tell etagManager to clean up, or clean up after the user id is updated

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How can we detect a reinstall? Anything you have in mind?

PurchasesCoreSwift/Networking/ETagManager.swift Outdated Show resolved Hide resolved
PurchasesCoreSwift/Networking/ETagManager.swift Outdated Show resolved Hide resolved
PurchasesCoreSwift/Networking/ETagManager.swift Outdated Show resolved Hide resolved
PurchasesCoreSwift/Networking/ETagManager.swift Outdated Show resolved Hide resolved
PurchasesCoreSwift/Networking/ETagManager.swift Outdated Show resolved Hide resolved
@vegaro vegaro changed the base branch from develop to main June 9, 2021 17:19
@vegaro
Copy link
Contributor Author

vegaro commented Jun 18, 2021

@aboedo can you do another pass here? I made changes and asked some questions. thanks!

@vegaro vegaro requested a review from aboedo July 2, 2021 18:41
@vegaro
Copy link
Contributor Author

vegaro commented Jul 2, 2021

thanks @taquitos , I believe everything is addressed @aboedo

@vegaro vegaro removed the HOLD label Jul 2, 2021
Copy link
Member

@aboedo aboedo left a comment

Choose a reason for hiding this comment

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

a few style comments are unaddressed, other than that it looks great 🎉
The backend issues are solved, right? i.e.: this should be ready to 🚢 ?

PurchasesCoreSwift/Networking/ETagAndResponseWrapper.swift Outdated Show resolved Hide resolved
PurchasesCoreSwift/Networking/ETagManager.swift Outdated Show resolved Hide resolved
Copy link
Member

@aboedo aboedo left a comment

Choose a reason for hiding this comment

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

looks great! 🚢

Purchases/Networking/RCHTTPClient.m Outdated Show resolved Hide resolved
Purchases/Networking/RCHTTPClient.m Outdated Show resolved Hide resolved
@vegaro
Copy link
Contributor Author

vegaro commented Jul 9, 2021

ok, I am extremely happy to announce that I fixed all comments, tested all cases (200s, 500s, 304s, retries, errors when finding in cache, and app extensions) and it looks like we are golden @aboedo

@taquitos
Copy link
Contributor

taquitos commented Jul 9, 2021

Nice

👏👏👏

Copy link
Member

@aboedo aboedo left a comment

Choose a reason for hiding this comment

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

looks awesome! great work on this!

typedef void (^RetryRequestBlock)();

- (instancetype)initWithSystemInfo:(RCSystemInfo *)systemInfo
eTagManager:(RCETagManager *)eTagManager{
Copy link
Member

Choose a reason for hiding this comment

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

nitpick: missing space before {

@vegaro vegaro merged commit 125da78 into main Jul 9, 2021
@vegaro vegaro deleted the etags-support branch July 9, 2021 19:25
aboedo added a commit that referenced this pull request Jul 21, 2021
* Create CODE_OF_CONDUCT.md (#589)

Resolves #588

* Update issue templates (#537)

* Update issue templates

Looks like GitHub wants this named differently 🤷‍♂️

* Delete migration_task.md

* Update custom.md

* Update README.md (#636)

Just removing the outdated image on the readme

* ETags support (#509)

* identity v3: public-facing changes (#453)

* re-added the public-facing changes for identity v3

* updated nullability for errors in login

* temporarily removed version from PurchasesCoreSwift in Purchases.podspec

* updated sample app to use identity v3

* improved copy in RCPurchases.h

* Integration tests: Identity v3 test cases (#506)

* added extra test case for storekit tests

* added more test cases

* added a few more test cases

* slight cleanup

* added another, particularly gnarly test case

* updated test case name

* added another test case

* one more test case

* refactors and cleanups, made chained calls more reliable for one test case

* fixed name, added debug information to appUserIDs

* fixed issue with test clearing transactions when it shouldn't

* formatting

* code cleanup

* update call to replace api key and proxy url to catch issues if proxy url is empty

* updated syntax since lanes are executed in a block

* updated xcode and simulator versions for storekitTests

* updated syntax for setting log levels

* fixed fastlane changes that broke replace_in for storekit_tests

* update fastlane

* improvements from PR comments

* restricted storekit_tests to release tags and branches

* removed signing for storekittest app since it only runs in simulator

* removed storekitTestCertificate.cer since it's not needed, re-added tests just in case

* restricted storekit_tests to release branches and tags again now that they passed

* Bump addressable in /IntegrationTests/CocoapodsIntegration (#647)

Bumps [addressable](https://github.com/sporkmonger/addressable) from 2.7.0 to 2.8.0.
- [Release notes](https://github.com/sporkmonger/addressable/releases)
- [Changelog](https://github.com/sporkmonger/addressable/blob/main/CHANGELOG.md)
- [Commits](sporkmonger/addressable@addressable-2.7.0...addressable-2.8.0)

---
updated-dependencies:
- dependency-name: addressable
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* remove unused typedef to fix build warning (#649)

* Release: 3.12.0 (#648)

* looks like in some rebase createAlias got un-deprecated, re-deprecated it

* updated changelog and version number to 3.12.0

* added missing PR for identity v3 in changelog

* added install swiflint step to more ci jobs

* fix typo in circleci config.yml

* updated the name of the swiftlint step in xcode so it's more obvious at first glance when it fails

* fixed an issue where a few targets wouldn't compile correctly when doing carthage archive because they were pointing to unreleased versions of Purchases.

* re-added the old carthage script for archives

* added `vendor` folder to excluded directories for swiftlint

* Preparing for next version (#656)

Co-authored-by: Andy Boedo <andresboedo@gmail.com>

* added swiftlint installation to all places that were missing it (#657)

* Release: 3.12.1 (#659)

* renamed eTagInResponse -> maybeEtagInResponse to resolve conflict in some versions of Xcode

* updated version number to 3.12.1 and changelog

* added `scan_derived_data` to excluded directories for swiftlint

* Preparing for next version (#661)

Co-authored-by: Distiller <distiller@static.38.39.178.68.cyberlynk.net>

* Fix: Calling `setDebugLogsEnabled(false)` enables debug logs when it should not (#663)

* fixed a bug where setting debugLogsEnabled = false would still set them to true

* cleanup: replaced if / else with ternary operator

* reverted changes in SystemInfo initialization

* format fixes

* removed rchttpRequest

* updated call in `buildTvWatchAndMacOS`: install-gems -> install-dependencies

* undo accidental whitespace

* deleted empty RCSystemInfo.m file

Co-authored-by: Joshua Liebowitz <taquitos@users.noreply.github.com>
Co-authored-by: Corey Rabazinski <6013553+CoreyRab@users.noreply.github.com>
Co-authored-by: Cesar de la Vega <cesarvegaro@gmail.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: revenuecat-ops <60164957+revenuecat-ops@users.noreply.github.com>
Co-authored-by: Distiller <distiller@static.38.39.178.68.cyberlynk.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants