This is a temporary hack that I introduced so that I can run the unit tests locally despite the fact that the Samsung SDK's minSDK != our minSDK requirement.

We should remove this when we properly add in the Samsung SDK as a dependency to our SDK.

There are more error codes used by the Galaxy Store, we'll add them in as needed

I guess there is a small chance of a race condition between this being set, and when we check at the beginning of this method. I wonder if we should maybe only check whether there is an inflight request here?

Again, this probably doesn't apply right now since we're executing requests serially though.

Honestly, I think that all of the synchronization in this class is probably unnecessary since we're executing the requests serially. That said, would it just be safer to make this entire getProductDetails() function synchronized by annotating it with @Synchronized?

Hmm well, I guess it's dangerous to synchronize on a possibly long block of code. Specially considering that may be doing network requests (the actual fetching of products). So I would be against that. I do agree it's probably unnecessary considering we're executing all of this serially, but just concerning that it's not part of the contract, and this could potentially be executed from somewhere else (though it shouldn't)... Still, I guess I wouldn't consider it a blocker

Might be worth removing all the synchronous code, and instead create a single thread to execute all these Galaxy operations. We've created single thread executors for backend requests, and we could follow a similar approach. See:

Would this have any advantages over the SerialRequestExecutor/synchronized model we're using now? I'm not super familiar with the executors, so I asked Codex about it, and it gave me this:

A single-thread ScheduledExecutorService only serializes the submitted runnables. Our requests are async: we enqueue a block, it kicks off work, returns
immediately, and completes later via a callback. That means the executor thread would finish the runnable while the request is still in flight, and the next runnable
would start right away—breaking the one-at-a-time guarantee. SerialRequestExecutor keeps the head request “active” until its finish is called, so even though the work
hops off to other threads, we don’t start the next until the callback signals completion. To drop it, we’d need every request to fully execute and finish within the
executor’s runnable, or refactor to chain scheduling on callback completion, which is what SerialRequestExecutor already does.

Well, this would be mostly a replacement for the synchronized model we have, not for the SerialRequestExecutor, since I assume the galaxy SDK operations are done in another thread, so that wouldn't protect us against doing multiple requests to the galaxy SDK.

And well, as I mentioned, it could potentially simplify the synchronization. But well, considering we have the SerialRequestExecutor, we can probably get away with removing all that, and documenting well that requests are expected to be performed in serial... Not ideal in case we miss that constraint, since we don't have any compile time guarantees... but might be the simplest for now.

Sounds good! Removed all synchronized blocks in 43a226f

I also introduced a new @GalaxySerialOperation annotation that I applied to the getProductDetails() function. This annotation acts similarly to how the @ExperimentalPreviewRevenueCatPurchasesAPI works, where it forces any consumers of the annotated functions to explicitly opt-in to it. This will at least ensure that in the future, any other developer that tries to call these functions must acknowledge that the function must be called serially with other annotated functions. Let me know what you think!

Same here about possible race conditions.

Just wondering, would it be worth caching the StoreProduct itself instead of ProductVo? So we don't need to transform it on every call to get product details. Might be missing a reason why we need to cache the original ProductVo though.

Oooh, that's a good idea! Honestly, the only reason is so that we can re-use the same handleSuccessfulProductsResponse() function, which takes in ProductVos as a param.

I'll move the code around a little so that we can cache the StoreProduct instead :)

Made this optimization in ef18774 🙌

I guess if we have some of the products in the cache, we would still be querying for all of them. Probably a small optimization though, so not a blocker.

Will add this to a list of future optimizations to make!

Just to check, the API would return either a non-success ErrorVo or a list of products, but not both correct? For example, if part of the productIds requested do not exist, or we're passing an incorrect productId format for some of them, would it return the valid products, without an error? Or would it return the valid ones + an error?

In case it's the second case, we probably should log the error but still return a sucess result I think.

I just tested this out! If you request 2 valid product IDs and 1 invalid product IDs, it returns:

• A non-error (success) ErrorVo
• An ArrayList with the 2 valid products 😅

I'll see if we can create an error log for this in a follow-up PR

Created Linear ticket CAT-2148 to track this

I'm wondering if we should also annotate this method with the @GalaxySerialRequest, so if/when we add other operation helpers, it's easier to remember we need to add that annotation there as well.

This is a great idea! Added in a8a0d84

I guess this will fail to work if the aar is not set somehow... we shouldn't get this merged to main, but it's ok to keep it on the integration branch. Hopefully we can figure out a way to integrate it 🙏

Yeah, this is 1000% a temporary thing and should not be merged into main. We'll work with @MarkVillacampa at some point to find a proper way to integrate the Samsung SDK into our SDK 😄