Skip to content

fix: support null handling in nested structures#508

Merged
Shahroz16 merged 6 commits intomainfrom
mbl-955-fix-nested-null-parsing
Apr 10, 2025
Merged

fix: support null handling in nested structures#508
Shahroz16 merged 6 commits intomainfrom
mbl-955-fix-nested-null-parsing

Conversation

@Shahroz16
Copy link
Contributor

@Shahroz16 Shahroz16 commented Apr 8, 2025

closes: MBL-955

Changes

  • Added sanitizeNullsForJson() extension to recursively replace all null values with JsonNull in CustomAttributes for Map and List before forwarding to analytics
  • Added unit tests to validate the behavior of new extension
  • Added tests on CustomerIO.instance to validate end-to-end behavior
  • Updated Datapipeline method to cater Map<String,Any?> so they can cater cases of both, CustomeAttribute and mapof type <string, any?>

Sample Code

Crash on main before this fix:

CustomerIO.instance().track(
    "Add to Cart",
    mapOf(
        "item" to mapOf(
            "id" to "100",
            "name" to "Mobile",
            "discount" to null,
        ),
    )
)

@Shahroz16 Shahroz16 requested a review from a team as a code owner April 8, 2025 09:38
@github-actions
Copy link

github-actions bot commented Apr 8, 2025

Sample app builds 📱

Below you will find the list of the latest versions of the sample apps. It's recommended to always download the latest builds of the sample apps to accurately test the pull request.


@codecov
Copy link

codecov bot commented Apr 8, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 52.45%. Comparing base (26b4621) to head (5adacc4).
Report is 5 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff              @@
##               main     #508      +/-   ##
============================================
+ Coverage     52.02%   52.45%   +0.43%     
- Complexity      318      319       +1     
============================================
  Files           105      104       -1     
  Lines          2862     2888      +26     
  Branches        381      384       +3     
============================================
+ Hits           1489     1515      +26     
  Misses         1259     1259              
  Partials        114      114              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@github-actions
Copy link

github-actions bot commented Apr 8, 2025

Build available to test
Version: mbl-955-fix-nested-null-parsing-SNAPSHOT
Repository: https://s01.oss.sonatype.org/content/repositories/snapshots/

@github-actions
Copy link

github-actions bot commented Apr 8, 2025

  • java_layout: mbl-955-fix-nested-null-parsing (1744105183)

@github-actions
Copy link

github-actions bot commented Apr 8, 2025

  • kotlin_compose: mbl-955-fix-nested-null-parsing (1744105171)

Copy link
Contributor

@mrehan27 mrehan27 left a comment

Choose a reason for hiding this comment

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

Looks good 👍🏻

We can roll it out after testing the changes on wrapper SDKs.

@Shahroz16 Shahroz16 changed the title fix: support null handling in nested structures fix: support null handling in nested structures Apr 8, 2025
Copy link
Contributor

@mahmoud-elmorabea mahmoud-elmorabea left a comment

Choose a reason for hiding this comment

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

Approach looks solid! 💯

Just wanted to double check on two main concerns:

  • I think we might have missed at least one place using CustomAttributes, maybe it's worth doing a search for it to make sure we covered all. I only found one though
  • I cross tested with iOS quickly and it seems like iOS omits the value all together rather than sending it null, but it's not consistent, have we cross checked behavior with iOS?

@mahmoud-elmorabea mahmoud-elmorabea requested a review from a team April 9, 2025 13:11
@Shahroz16
Copy link
Contributor Author

  • I cross tested with iOS quickly and it seems like iOS omits the value all together rather than sending it null, but it's not consistent, have we cross checked behavior with iOS?

Thats a good callout, I agree with consistency but I also don't want to introduce a breaking change in terms of behavior. For example, currently null in the parent are treated as JsonNull and not skipped.

If we add logic of skipping nulls, then even that current behavior is going to be changed and if anyone has any segments/events driven based on that current behavior, it could break for them

@github-actions
Copy link

  • java_layout: mbl-955-fix-nested-null-parsing (1744293869)

@github-actions
Copy link

  • kotlin_compose: mbl-955-fix-nested-null-parsing (1744293885)

@Shahroz16 Shahroz16 merged commit bf6cb1b into main Apr 10, 2025
34 of 35 checks passed
@Shahroz16 Shahroz16 deleted the mbl-955-fix-nested-null-parsing branch April 10, 2025 14:37
github-actions bot pushed a commit that referenced this pull request Apr 10, 2025
## [4.5.6](4.5.5...4.5.6) (2025-04-10)

### Bug Fixes

* support null handling in nested structures ([#508](#508)) ([bf6cb1b](bf6cb1b))
github-actions bot pushed a commit that referenced this pull request Apr 10, 2025
## [4.5.6](4.5.5...4.5.6) (2025-04-10)

### Bug Fixes

* support null handling in nested structures ([#508](#508)) ([bf6cb1b](bf6cb1b))
github-actions bot pushed a commit that referenced this pull request Apr 11, 2025
## [4.5.6](4.5.5...4.5.6) (2025-04-11)

### Bug Fixes

* support null handling in nested structures ([#508](#508)) ([bf6cb1b](bf6cb1b))
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.

3 participants