Empty SKU list bug

[beylmk]

Addresses this ticket: https://spectrum.chat/revenuecat/general/android-crash-sku-must-be-set~7faa56e8-2ec4-4c8d-b37a-2d525c41d8f6?authed=true

BillingClient throws an exception if an empty string sku is passed to querySkuDetailsAsync

• filter out empty skus from the list
• don't bother querying if list is empty
• unit tests

Also pulled in a unit test fix from the 4.1.0 release -- pulling sha256() out of the verify block

[beylmk]

@vegaro @aboedo
I tested and found that passing an empty sku list to the BillingClient always returns empty sku details (i.e. the query doesn't just return all of the correct type in that case)... but still, wanted to double-check whether you think there could be any unwanted side-effects with skipping this call?

if not, i think it might help users not hit their quota?

[aboedo]

I think it should be fine, and like you said, would actually reduce quota usage (probably not by much, since hopefully we're not calling this often with an empty list).

One thing that I found confusing, though, is that it seems from the crash logs that the line that's actually crashing is 143?

This PR might solve it anyway, but I'm curious because we were doing

skuDetailsList?.takeUnless { it.isEmpty() }?.forEach {

but it was crashing on it.sku, right? Am I misinterpreting? Could the billing library have been sending back a non-empty list, where calling .sku on its items would crash?

[beylmk]

great question...it.sku calls optString(), so that should just return an empty string if not found. once I realized that, I guess I rationalized that we can’t trust line numbers in crash reports, since our users could be on a different version with different code OR perhaps that the async nature of the callback means the line that the crash reports on might not be exact? @vegaro any other thoughts on why that would've happened?

re-testing by passing an empty sku now shows the error occuring on the querySkuDetailsAsync line.

[vegaro]

Line 143 was doing the following in older versions of the SDK (just checked in 4.0.2).

                    querySkuDetailsAsync(params) { billingResult, skuDetailsList ->

I am not sure which version they are using, but that would match the exception happening inside the code of the billing client, particularly inside the implementation of querySkuDetailsAsync.

It's weird that you're getting an empty response if calling with an empty list though... Have you tried with a null list? I think you would have to call the function in Java to bypass the nullability checks from Kotlin. Writing a test in Java that calls BillingWrapper.querySkuDetailsAsync could be a good way of achieving that.

[vegaro]

Can you clarify this @beylmk :

re-testing by passing an empty sku now shows the error occuring on the querySkuDetailsAsync line.

Do you mean you are now able to reproduce it by passing an empty list?

[beylmk]

no, passing an empty list doesn't throw anything. i pass a list containing at least one empty sku, i.e. listOf("")

[beylmk]

why would it be weird to get an empty response with an empty list? is that not expected? i'll test out a null list 👍

[beylmk]

@vegaro SkuDetailsParams.build() will throw an IllegalArgumentException if we pass a null list

[vegaro]

I meant that it is weird because that is what it looks like the stacktrace suggests. But who knows...

Maybe we are missing something? This was reported in Unity so maybe that gives us some clues

[aboedo]

looks great, left a question

[aboedo]

I think it should be fine, and like you said, would actually reduce quota usage (probably not by much, since hopefully we're not calling this often with an empty list).

One thing that I found confusing, though, is that it seems from the crash logs that the line that's actually crashing is 143?

This PR might solve it anyway, but I'm curious because we were doing

skuDetailsList?.takeUnless { it.isEmpty() }?.forEach {

but it was crashing on it.sku, right? Am I misinterpreting? Could the billing library have been sending back a non-empty list, where calling .sku on its items would crash?

[vegaro]

Thanks!