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

Fix JSON Kotlin Extension Functions #2004

Merged
merged 2 commits into from Feb 28, 2024
Merged

Fix JSON Kotlin Extension Functions #2004

merged 2 commits into from Feb 28, 2024

Conversation

jkasten2
Copy link
Member

@jkasten2 jkasten2 commented Feb 24, 2024

Description

One Line Summary

Fixes our JSON Kotlin extension functions and added tests to ensure our JSON implementation in tests stay accurate.

Details

This PR sloves 3 issues with JSONArray / JSONArray:

  1. Tests were running with a different implementation than Android's
    • Swapping out org.json:json for com.tdunning:json fixed this.
      • Yes this is confusing, but org.json:json is NOT the same library as Android's org.json namespace!
  2. Our JSONObject.toMap() Kotlin extension function didn't support nested JSON.
    • It isn't clear if anything in our SDK was affected by this today, but can bite us in the future.
  3. Could not use JSONObject Kotlin extension functions defined in src in tests using Robolectric
    • Was only an issue with org.json:json for some reason, so swapping com.tdunning:json also fixed this issue.

This PR is based on some of the attempted work from PR #2003.

Motivation

JSONObject and JSONArray are two APIs we use a lot in this SDK. It is important they are working fully and we are testing with the same implementation in tests.

Scope

Effects code that uses JSONObject.toMap(). And tests that used org.json:json.

History

Fun fact, Google rolled their own org.json for Android. They probably did this due to the silly "The Software shall be used for Good, not Evil." line in the orignal org.json licence.

  • The original org.json is now public domain.

Testing

Unit testing

New JSONObjectEnvTest.kt and JSONObjectExtensionsTest.kt test files were added to cover the behavior changes and ensure test env does not regress.

Manual testing

Tested on an Android 14 emulator ensuring the device tag updates are sent.

Affected code checklist

  • Notifications
    • Display
    • Open
    • Push Processing
    • Confirm Deliveries
  • Outcomes
  • Sessions
  • In-App Messaging
  • REST API requests
  • Public API changes

Checklist

Overview

  • I have filled out all REQUIRED sections above
  • PR does one thing
  • Any Public API changes are explained in the PR details and conform to existing APIs

Testing

  • I have included test coverage for these changes, or explained why they are not needed
  • All automated tests pass, or I explained why that is not possible
  • I have personally tested this on my device, or explained why that is not possible

Final pass

  • Code is as readable as possible.
  • I have reviewed this PR myself, ensuring it meets each checklist item

This change is Reviewable

org.json:json is a different implementation than Android AOSP, swapped it
out with com.tdunning:json which seems to be identical to AOSP. This is
important as we want the test to behave as close as possible to a real
Android device.

Added some tests to ensure if we ever change JSON libraries again
we don't run into hard to debug problems later.
Also added JSONArray.toList() and a few more JSON tests
@jkasten2 jkasten2 changed the title [WIP] Fix JSON Kotlin Extension Functions Fix JSON Kotlin Extension Functions Feb 24, 2024
Copy link
Contributor

@nan-li nan-li left a comment

Choose a reason for hiding this comment

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

How did you know to make the changes in this PR?
Was something failing, or did you experience some strange behavior during testing?

Base automatically changed from tests/clean-up-dup-mocks-and-helpers to main February 28, 2024 18:12
@jkasten2
Copy link
Member Author

jkasten2 commented Feb 28, 2024

The first thing that got me started in looking into this was I tried to use JSONObject.toMap() in a test to make it easier to read the test. However it would fail when running the test with it not being able to find it .toMap()

I then uncovered these problems:

  1. The org.json:json library causing our Kotlin Extension methods on JSONObject to not be found, but only if the test uses Robolectric
    • I attempted to fix this by not using org.json:json when Robolectric is used (in PR [Abandoned][Tests] Allow JSONObject Kotlin extension functions to work in tests #2003). However I discovered that some tests failed due to behavior differences in the implementation of some JSONObject methods, specifically JSONObject.getString will throw in org.json:json if you attempt this on a key that is really a JSONObject, but AOSP's implementation it works by just stringifying it.
  2. Our JSONObject.toMap() wasn't implemented correctly, as it didn't supported nested JSONObject and JSONArray like other libraries do.

@jkasten2 jkasten2 merged commit caa994e into main Feb 28, 2024
2 checks passed
@jkasten2 jkasten2 deleted the fix/json-ext-functions branch February 28, 2024 18:31
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

2 participants