fix: only log unfetched products once - #3929
Conversation
|
@cursor review |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| dispatch { | ||
| getProductsOfTypes( | ||
| productIds, | ||
| productIds - storeProducts.map { it.purchasingData.productId }.toSet(), |
There was a problem hiding this comment.
This is a behavioral change: follow-up queries only query for un-fetched products. This is probably more efficient on the store side, since they'll be fetching fewer products, but it also prevents us from logging "unfetched" log messages for logs that were fetched in a previous query
There was a problem hiding this comment.
Indeed! I think this makes sense even if we change what I mentioned in my previous comment. I guess it could be a problem if the ids could be duplicated for different types... But I don't think that's possible?
There was a problem hiding this comment.
Yeah, I double checked and it's not possible for the Galaxy, Amazon, or Play Stores, so it shouldn't be a problem today, but could theoretically be possible in the future for other stores? I doubt it though, since by definition, a product identifier should uniquely identify a product
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3929 +/- ##
==========================================
+ Coverage 81.22% 81.26% +0.04%
==========================================
Files 435 435
Lines 17849 17868 +19
Branches 2790 2790
==========================================
+ Hits 14498 14521 +23
+ Misses 2328 2327 -1
+ Partials 1023 1020 -3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 6c0e968. Configure here.
tonidero
left a comment
tonidero
left a comment
There was a problem hiding this comment.
I'm just wondering if there is a cleaner way... Wdyt? Happy to chat about it of course! Also, great improvement 🙌
| productType = it, | ||
| productType = productType, | ||
| productIds = productIds, | ||
| logUnfetchedProducts = isFinalQuery, |
There was a problem hiding this comment.
Hmm so I'm wondering if it would be cleaner to just log missing products after all the calls are done, where we then know if there are any missing products after fetching all types... Then, we don't need to be passing the logging logic to each call... Wdyt?
There was a problem hiding this comment.
@tonidero Yeah, I like the idea! It doesn't feel great to have params in our functions just for logging logic. It would also allow us to standardize the logging logic across stores, which is a big plus. The reason I didn't go with that approach is that could lose store-specific logging info, like in this log from the Play Store:
Product not found: [redacted product ID]- Product Type: subs, Reason: PRODUCT_NOT_FOUND, Serialized doc ID:
Looking at it though, that reason may not actually be that helpful, and the Galaxy/Amazon stores don't actually include any store-specific info. If you're in favor, I'll refactor this to remove the bool params and centralize the logging outside of the stores' billing wrappers
There was a problem hiding this comment.
Yeah... I think that's fine TBH... maybe if there is a reason other than PRODUCT_NOT_FOUND it could be useful? In case there is some useful reason why it doesn't show I guess... But yeah, I feel it's cleaner to move most of the logic outside...
There was a problem hiding this comment.
Cool, thanks! I will refactor this to remove the bool params and place the logging outside of the BillingWrappers :)
| dispatch { | ||
| getProductsOfTypes( | ||
| productIds, | ||
| productIds - storeProducts.map { it.purchasingData.productId }.toSet(), |
There was a problem hiding this comment.
Indeed! I think this makes sense even if we change what I mentioned in my previous comment. I guess it could be a problem if the ids could be duplicated for different types... But I don't think that's possible?
2 participants
Motivation
When fetching products, the SDK queries the stores twice: once for subscription products, and again for one-time purchases. The SDK logs warning messages when stores don't return all expected products. This is a good log to keep, but when querying for a one-time product, the SDK will log the "unfetched product" log when the subscription product query returns nothing, even when the one-time purchase query will be successful.
This creates confusion for humans/agents reading through the SDK logs, as the product isn't truly unfetched yet, it just hasn't been fetched yet.
Description
This PR updates how the SDK generates "product not fetched" logs so that it only logs it once per product request. Since the logs contain store-specific information, they must be generated in each stores'
BillingWrapperimplementation. So, to accomodate this, we've added alogUnfetchedProductsparam toBillingAbstract.queryProductDetailsAsync(), andqueryProductDetailsAsync()consumers specify whether or not they'd like the unfetched logs to be generated.Testing
Added unit tests to confirm whether the
logUnfetchedProductsparam works as intended, and to check that thequeryProductDetailsAsync()consumers are passing in the proper values.Note
Low Risk
Behavior change is limited to logging timing and slightly more efficient product-type chaining; product fetch semantics and billing APIs stay the same aside from an optional parameter defaulting to true.
Overview
Adds a
logUnfetchedProductsflag throughBillingAbstract.queryProductDetailsAsync()and each store billing wrapper (Google, Amazon, Galaxy, simulated). Store handlers only emit “missing/unfetched product” warnings when that flag is true, so multi-step fetches do not warn prematurely.PurchasesOrchestrator.getProductsOfTypesnow passeslogUnfetchedProducts = falseon intermediate product-type queries andtrueonly on the last query in the chain. It also stops querying the next product type when there are no remaining IDs, and only queries INAPP for IDs not already returned from SUBS. DiagnosticsnotFoundtracking uses the remaining requested ID set at completion instead of recomputing from collected products.OfferingsFactoryuses the same pattern: SUBS query with logging disabled, INAPP follow-up with logging enabled when needed.Reviewed by Cursor Bugbot for commit 01df168. Bugbot is set up for automated code reviews on this repo. Configure here.