Workaround bug in android 4 for JSON objects with List<String> by aboedo · Pull Request #942 · RevenueCat/purchases-android

aboedo · 2023-04-04T21:27:40Z

Fixes https://linear.app/revenuecat/issue/SDK-3052/400-from-post-receipts-because-product-did-not-have-quotes
https://revenuecats.atlassian.net/browse/CSDK-681

Works around the following bug in android 4:
https://stackoverflow.com/q/37317669
http://fupeg.blogspot.com/2011/07/android-json-bug.html

Essentially, this bug means that when you get a list of product_ids, instead of sending a list of strings, it sends a list with a string in it.

Before	After
"product_ids": "[item1, item2]"	"product_ids": ["item1, "item2"]

codecov · 2023-04-05T08:25:27Z

Codecov Report

Merging #942 (1344e6a) into main (82e7c20) will decrease coverage by 0.63%.
The diff coverage is 33.33%.

❗ Current head 1344e6a differs from pull request most recent head 5eea593. Consider uploading reports for the commit 5eea593 to get more accurate results

@@            Coverage Diff             @@
##             main     #942      +/-   ##
==========================================
- Coverage   85.07%   84.45%   -0.63%     
==========================================
  Files         165      160       -5     
  Lines        5824     5520     -304     
  Branches      801      774      -27     
==========================================
- Hits         4955     4662     -293     
- Misses        550      561      +11     
+ Partials      319      297      -22

Impacted Files	Coverage Δ
...java/com/revenuecat/purchases/common/HTTPClient.kt	`85.50% <33.33%> (-2.38%)`	⬇️

... and 25 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

vegaro · 2023-04-05T08:25:51Z

Btw, I found the similar issue we had in the past #144

tonidero · 2023-04-05T10:31:20Z

This would need to be removed I think?

could you clarify? this was added to fix a separate but similar issue (the one that César posted, #144)

tonidero · 2023-04-05T10:32:12Z

We should add a comment explaining this.

aboedo · 2023-04-06T15:38:07Z

            .filterNotNullValues()
    }

-    private fun Map<String, Any?>.convert(): JSONObject {


moved to its own class to make testing easier and more focused

aboedo · 2023-04-06T15:38:30Z

+        return JSONObject(inputMap)
+    }
+
+    /** To avoid Java type erasure, we use a Kotlin inline function with a reified parameter


this bit was moved from HTTPClient

aboedo · 2023-04-06T15:43:58Z

+        if (inputMap == null) {
+            return JSONObject()
+        }


todo: make sure that this matches the previous behavior exactly.

aboedo · 2023-04-06T15:47:16Z

-        val jsonBody = body?.convert()
+        val jsonBody = body?.let { mapConverter.convertToJSON(body) }


just double-checking, android folks: this should be equivalent, right?

I would do

val jsonBody = body?.let { mapConverter.convertToJSON(it) }

But yes. The difference is that with it, you are not using smart casting from kotlin. But it's the same.

aboedo · 2023-04-06T15:49:48Z

could you clarify? this was added to fix a separate but similar issue (the one that César posted, #144)

aboedo · 2023-04-06T15:51:38Z

I won't merge until I've managed to reproduce with an instrumented test, but it should be ready to review in the meantime.

aboedo · 2023-04-06T15:51:52Z

+     * (i.e.: "[\"value1\", \"value2\"]" instead of "[value1, value2]")
+     */
+    @Test
+    fun `test map conversion fixes wrong treatment of arrays of strings in JSON library`() {


the old code fails this test

tonidero

LGTM!

tonidero · 2023-04-06T16:14:17Z

+     */
+    internal fun convertToJSON(inputMap: Map<String, Any?>): JSONObject {
+        val mapWithoutInnerMaps = inputMap.mapValues { (_, value) ->
+            value.tryCast<Map<String, Any?>>(ifSuccess = { convertToJSON(this) })


Unless I'm reading this wrong, this line doesn't do anything. It will convert to JSON but we are ignoring the return value I think? We moved this line to the else inside the when, so I think this line can be removed if I'm not wrong.

yes, I was thinking the same

oops, leftover from refactor, thanks

tonidero · 2023-04-06T16:19:57Z

+            .put("key1", "value1")
+            .put("key2", JSONArray(listOf("value2", "value3")))
+            .put("key3", JSONObject().put("nestedKey", "nestedValue"))
+            .put("key4", JSONArray(listOf("value4", "value5")))


Hmm should this be a JSONObject with a JSONArray value?

ohh oops this isn't nested 😅 Great catch!

updated, lmk what you think!

tonidero · 2023-04-06T16:20:45Z

+     */
+    @Test
+    fun `test map conversion fixes wrong treatment of arrays of strings in JSON library`() {
+        val mapConverterMock = spyk<MapConverter>()


Maybe rename to mapConverterSpy? Since it's not technically a mock

ohh spyk nice, I've never used this

I'm not technically using it as a spy either, more like a partial mock, since I do modify the return value for some circumstances. Perhaps renaming it to that would be clearer?

trying it this way, let me know what you think

vegaro · 2023-04-06T17:00:29Z

+     */
+    @Test
+    fun `test map conversion fixes wrong treatment of arrays of strings in JSON library`() {
+        val mapConverterMock = spyk<MapConverter>()


ohh spyk nice, I've never used this

vegaro · 2023-04-06T17:15:16Z

+     */
+    internal fun convertToJSON(inputMap: Map<String, Any?>): JSONObject {
+        val mapWithoutInnerMaps = inputMap.mapValues { (_, value) ->
+            value.tryCast<Map<String, Any?>>(ifSuccess = { convertToJSON(this) })


yes, I was thinking the same

vegaro · 2023-04-06T17:17:54Z

-        val jsonBody = body?.convert()
+        val jsonBody = body?.let { mapConverter.convertToJSON(body) }


I would do

val jsonBody = body?.let { mapConverter.convertToJSON(it) }

But yes. The difference is that with it, you are not using smart casting from kotlin. But it's the same.

tonidero

LGTM!

tonidero · 2023-04-12T07:27:32Z

Ah linter failed.

…s in a map would get treated as a single string instead of a JSONArray

aboedo · 2023-04-12T13:06:21Z

We might wanna separate that linter step so it's clearer that it's not a test failure but rather linting, very different priorities there.
The linter actually failed on an entirely separate part of the codebase, though - I suppose it was failing on master on the commit this was based upon. I'm rebasing to see if that makes a difference.

vegaro · 2023-04-12T14:16:17Z

@aboedo I extracted detektAll into its own job in #965

aboedo · 2023-04-12T19:12:28Z

@vegaro awesome thanks!! 🙌🙌🙌

NachoSoto · 2023-05-19T15:40:32Z

Is this ready to be merged?

aboedo · 2023-05-19T20:41:34Z

oops forgot about this, yeah, it's ready to go

@swehner

**This is an automatic release.** ### New Features * Support DEFERRED mode (#985) via swehner (@swehner) * Add completion callback to syncPurchases API (#1002) via Toni Rico (@tonidero) ### Bugfixes * Workaround bug in android 4 for JSON objects with List<String> (#942) via Andy Boedo (@aboedo) ### Dependency Updates * Bump fastlane-plugin-revenuecat_internal from `fe45299` to `13773d2` (#1015) via dependabot[bot] (@dependabot[bot]) ### Other Changes * Bump dokka to 1.8.10 to support Gradle 8 (#1009) via Toni Rico (@tonidero) * Disable offline entitlements temporarily (#1023) via Toni Rico (@tonidero) * Fix integration tests in CI (#1019) via Toni Rico (@tonidero) * Add offline entitlements integration tests (#1006) via Toni Rico (@tonidero) * Disable offline entitlements in observer mode (#1014) via Toni Rico (@tonidero) * Extracts setup and teardown to BasePurchasesTest (#1011) via Cesar de la Vega (@vegaro) * Support forcing server errors for tests (#1008) via Toni Rico (@tonidero) --------- Co-authored-by: revenuecat-ops <ops@revenuecat.com> Co-authored-by: Toni Rico <antonio.rico.diez@revenuecat.com>

aboedo self-assigned this Apr 4, 2023

aboedo added the pr:fix A bug fix label Apr 4, 2023

tonidero reviewed Apr 5, 2023

View reviewed changes

aboedo force-pushed the andy/sdk-3052-400-from-post-receipts-because-product branch 2 times, most recently from 2a95f56 to dd17b2b Compare April 6, 2023 14:49

aboedo commented Apr 6, 2023

View reviewed changes

aboedo marked this pull request as ready for review April 6, 2023 15:50

aboedo requested review from tonidero and vegaro April 6, 2023 15:50

aboedo commented Apr 6, 2023

View reviewed changes

tonidero approved these changes Apr 6, 2023

View reviewed changes

vegaro approved these changes Apr 6, 2023

View reviewed changes

tonidero approved these changes Apr 12, 2023

View reviewed changes

aboedo added 13 commits April 12, 2023 10:03

introduces a workaround for a bug in android 4 where a list of string…

977db1e

…s in a map would get treated as a single string instead of a JSONArray

extracted relevant logic to a MapConverter class

5e58860

added tests

e10882d

added docstrings

1bbb188

removed useless comment

110ccff

cleanup and fixed mocking

5ebcc69

update test naming for consistency

394eefa

clear up test name

596b296

clarified variable names

7ab958e

fix bad rename

4a58388

updated types and behavior to match previous behavior for null body

b3ca3d0

some clean up from PR comments

83cc5c0

updated test case to include an actual nested array

c2a7709

aboedo force-pushed the andy/sdk-3052-400-from-post-receipts-because-product branch from 57b8d2f to c2a7709 Compare April 12, 2023 13:03

removed unused imports

5eea593

vegaro mentioned this pull request Apr 12, 2023

Extract detekt job from test job #965

Merged

aboedo merged commit c21e0c7 into main May 19, 2023

aboedo deleted the andy/sdk-3052-400-from-post-receipts-because-product branch May 19, 2023 20:42

This was referenced May 24, 2023

[AUTOMATIC] Release/6.3.0 #1022

Closed

[AUTOMATIC] Release/6.3.0 #1025

Merged

		val jsonBody = body?.convert()
		val jsonBody = body?.let { mapConverter.convertToJSON(body) }

Uh oh!

Conversation

aboedo commented Apr 4, 2023

Uh oh!

codecov Bot commented Apr 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

vegaro commented Apr 5, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aboedo commented Apr 6, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tonidero left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aboedo Apr 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tonidero left a comment

Choose a reason for hiding this comment

Uh oh!

tonidero commented Apr 12, 2023

Uh oh!

aboedo commented Apr 12, 2023

Uh oh!

vegaro commented Apr 12, 2023

Uh oh!

aboedo commented Apr 12, 2023

Uh oh!

NachoSoto commented May 19, 2023

Uh oh!

aboedo commented May 19, 2023

Uh oh!

Reviewers

Assignees

codecov Bot commented Apr 5, 2023 •

edited

Loading

aboedo Apr 11, 2023 •

edited

Loading