Add support for nested lists to map flattening logic#777
Conversation
…dobe/aepsdk-core-android-1 into feature/disqualification
|
Question for reviewers: Since this was also a customer requested feature, should we also document this in our public docs at https://developer.adobe.com/client-sdks/home/base/mobile-core/rules-engine/technical-details/#matching-and-retrieving-values-by-keys? |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## feature/disqualification #777 +/- ##
==============================================================
+ Coverage 75.37% 75.39% +0.02%
- Complexity 2338 2339 +1
==============================================================
Files 217 218 +1
Lines 10889 10898 +9
Branches 1411 1412 +1
==============================================================
+ Hits 8207 8216 +9
Misses 2051 2051
Partials 631 631
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
@spoorthipujariadobe It's better to add some functional/module tests for this new feature, such as
|
| flattenedMap[expandedKey] = value | ||
| } | ||
| } | ||
| is List<*>, is Array<*> -> { |
There was a problem hiding this comment.
What do you think of defining the List and Array flattening separately on their respective types?:
internal fun Map<String, Any?>.flattening(prefix: String = "", flattenArrays: Boolean = true): Map<String, Any?>
internal fun List<Any?>.flattening(prefix: String = ""): Map<String, Any?>
internal fun Array<out Any?>.flattening(prefix: String = "") = toList().flattening(prefix)I was thinking if we ever need to flatten top level List/Array values, having it defined on their respective types keeps the implementation stable
There was a problem hiding this comment.
That's a good idea. Modified it
| */ | ||
| @JvmSynthetic | ||
| internal fun Map<String, Any?>.flattening(prefix: String = ""): Map<String, Any?> { | ||
| internal fun Map<String, Any?>.flattening(prefix: String = "", shouldFlattenListAndArray: Boolean = true): Map<String, Any?> { |
There was a problem hiding this comment.
small nit: what do you think of renaming to something like: flattenLists?
| } | ||
|
|
||
| @Test | ||
| fun testMapFlatteningWithDotInKey() { |
There was a problem hiding this comment.
This test case implicitly relies on Kotlin's internal implementation of map key ordering logic right? Should we update this test case to simply validate that the flattened key results are equal to the expected a.b?
| ) | ||
| assertEquals(expectedMap, flattenedMap) | ||
| } | ||
|
|
There was a problem hiding this comment.
Could we also add a base case test of empty map that is flattened == empty map?
| } | ||
|
|
||
| @Test | ||
| fun testMapFlatteningWithListFlatteningDisabled() { |
There was a problem hiding this comment.
Could we also add the Array value case (that is, list is tested but would be nice to validate Array is treated the same)
55ea8ff
into
adobe:feature/disqualification
Description
This PR enhances the map flattening logic to add support for nested lists and arrays. The flattening logic in
MapExtensions.kthas been updated to properly index list/array elements in the flattened keys and supports mixed data structures (maps containing lists, lists containing maps).Example
Before:
{rootKey: {key1: [value0, value1]}} -> {rootKey.key1: [value0, value1]}After:
{rootKey: {key1: [value0, value1]}} -> {rootKey.key1.0: value0, rootKey.key1.1: value1}Impact
This change affects how event data map is flattened in:
matchercondition: Since key lookup in nested lists was never supported, this is a net new feature for this case and shouldn't create any backwards compatibility issues.~all_url: Since customers use this in their Launch rules with postback consequence, this change might impact their existing rules. A backwards compatible carve out for the token ~all_url` is needed so that arrays remain unflattened and preserve the existing behavior. This is achieved by implementing a flag in map flattening which controls whether or not to flatten inner arrays.Related Issue
Motivation and Context
With the existing logic, if a map contains a nested list, it is considered as unsupported for flattening and the resulting flattened map has the list added as an object, making it impossible to lookup for an element at a specific index in the list for rules matcher condition.
We need this ability to support content card disqualification when the card is dismissed. The rule condition in this case verifies that the dismiss event happened on the specific card we want to disqualify and writes a disqualify event to event history as a consequence. To verify this card association, we need to look up and match the activity id which is present inside the propositions array in the dismiss event data.
Disqualify rule example:
How Has This Been Tested?
Updated related tests in
MapUtilsTests.javaandLaunchTokenFinderTest.ktto reflect the new flattening behaviorScreenshots (if appropriate):
Types of changes
Checklist: