SDK-5763: [Android Core SDK] Add DisplayUnitCache interface + setter on CleverTapAPI (#999)#1012
Open
CTLalit wants to merge 2 commits into
Open
SDK-5763: [Android Core SDK] Add DisplayUnitCache interface + setter on CleverTapAPI (#999)#1012CTLalit wants to merge 2 commits into
CTLalit wants to merge 2 commits into
Conversation
…on CleverTapAPI (#999) * feat(SDK-5763): add DisplayUnitCache interface + setter on CleverTapAPI Extracts the public surface of CTDisplayUnitController into a new DisplayUnitCache interface and exposes CleverTapAPI.setDisplayUnitCache() so external SDKs (e.g. Native Display) can install their own cache implementation. Default behaviour preserved when the setter is not called — the SDK lazily installs CTDisplayUnitController on first adUnit_notifs response. - displayunits/DisplayUnitCache.java: new public interface - displayunits/CTDisplayUnitController.java: implements DisplayUnitCache - ControllerManager.java: holds DisplayUnitCache reference; new getDisplayUnitCache()/setDisplayUnitCache(); deprecated old getters - CleverTapAPI.java: setDisplayUnitCache() public; lookup methods route via interface - AnalyticsManager, DisplayUnitResponse, LoginController: route via getDisplayUnitCache() - AnalyticsManagerTest: mock points updated to displayUnitCache * SDK-5763: add pushDisplayUnitElementClickedEventForID(unitID, elementID, additionalProperties) Adds a dedicated Core SDK method for tracking element-level clicks within a Native Display unit, separate from the existing unit-level pushDisplayUnitClickedEventForID. The new method: - Carries the campaign's wzrk_* fields from the cached unit JSON (same enrichment as the unit-level method). - Adds wzrk_element_id = elementID to evtData from the dedicated parameter. - Merges additionalProperties into evtData after wzrk_* enrichment. Keys starting with wzrk_ are stripped defensively — the prefix is reserved for server-controlled attribution. The existing pushDisplayUnitClickedEventForID(unitId) stays untouched for unit-level clicks (graceful degradation for older ND SDK clients). Why a new method (vs an overload of the existing one): - Method name encodes the semantic — readers/dashboard query authors don't need tribal knowledge to interpret "click + additionalProperties". - ND SDK PR #32 wires its reflective probe to this new signature; old Core SDK callers continue to call the legacy single-arg method unchanged. No silent attribution semantics drift. Tests in AnalyticsManagerTest cover: - Happy path: elementID lands as wzrk_element_id; additionalProperties merge as first-class evtData fields. - wzrk_* keys in additionalProperties are filtered out. - coreMetaData.setWzrkParams (which feeds wzrk_ref batch headers) receives only wzrk_* keys — caller-supplied non-wzrk extras don't ride along on subsequent batched events. - displayUnitCache null short-circuit preserved. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * SDK-5763: type updateDisplayUnits with parsed models, hoist parsing to response Refactor the DisplayUnitCache interface so updateDisplayUnits takes List<CleverTapDisplayUnit> instead of JSONArray and is void. The cache contract is now pure-model — JSON parsing lives in DisplayUnitResponse, not in cache implementations. - DisplayUnitCache: parameter is List<CleverTapDisplayUnit>; return removed (caller already has the list). - CTDisplayUnitController: resets and populates the items map from the supplied list, skipping null/empty-id entries. - DisplayUnitResponse: new parseDisplayUnitsFromJson(JSONArray) filters malformed entries (error field) and per-item JSONException, then hands the model list to the cache and notifies the listener. - CTDisplayUnitControllerTest: rewritten against the list-based API. Host-installed DisplayUnitCache implementations no longer need to know about the SDK's JSON shape; they only deal with model objects. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * SDK-5763: address review — TOCTOU + cross-thread visibility for display-unit cache - ControllerManager.displayUnitCache is now volatile so writes from setDisplayUnitCache() (any thread, including a public-API caller outside the synchronized init block in DisplayUnitResponse) are safely visible to subsequent reads. - CleverTapAPI.getAllDisplayUnits() and getDisplayUnitForId() now capture the cache reference into a local variable once, then null-check and dereference that local. Closes the TOCTOU window where a concurrent setDisplayUnitCache(null) could let the second getDisplayUnitCache() return null after the guard had passed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * SDK-5763: import DisplayUnitCache and use the short name throughout CleverTapAPI Adds the DisplayUnitCache import in CleverTapAPI.java, switches the setDisplayUnitCache(...) parameter to the short name, and replaces the fully-qualified DisplayUnitCache references inside getAllDisplayUnits() and getDisplayUnitForId() (introduced when the import wasn't yet present) with the short name as well. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * chore: fixes test case. --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> (cherry picked from commit e3090b2)
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
Contributor
WalkthroughIntroduce a DisplayUnitCache interface and wire it through ControllerManager and CleverTapAPI; update CTDisplayUnitController to implement the cache with List-based updates; refactor response parsing to populate the cache; add pushDisplayUnitElementClickedEventForID to AnalyticsManager/CleverTapAPI with wzrk_* merging/filtering; update tests. ChangesDisplay Unit Cache Abstraction and Element Click Events
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
clevertap-core/src/main/java/com/clevertap/android/sdk/response/DisplayUnitResponse.java (1)
84-89:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEmpty payload bypasses cache reset and callback notification.
Line 85 exits early for
messages.length() == 0, so Line 103 and Line 104 never execute. That keeps stale display units in cache and skips the expectednullcallback on an empty server payload.Suggested fix
private void parseDisplayUnits(JSONArray messages) { - if (messages == null || messages.length() == 0) { + if (messages == null) { logger.verbose(config.getAccountId(), - Constants.FEATURE_DISPLAY_UNIT + "Can't parse Display Units, jsonArray is either empty or null"); + Constants.FEATURE_DISPLAY_UNIT + "Can't parse Display Units, jsonArray is null"); + callbackManager.notifyDisplayUnitsLoaded(null); return; } @@ ArrayList<CleverTapDisplayUnit> displayUnits = parseDisplayUnitsFromJson(messages); cache.updateDisplayUnits(displayUnits); callbackManager.notifyDisplayUnitsLoaded(displayUnits.isEmpty() ? null : displayUnits); }Also applies to: 102-105
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@clevertap-core/src/main/java/com/clevertap/android/sdk/response/DisplayUnitResponse.java` around lines 84 - 89, The early return in parseDisplayUnits prevents the existing "reset cache and notify with null" logic from running when messages.length() == 0; change the guard so only messages == null returns early, and for an empty JSONArray ensure you execute the existing cache-reset and callback-notify code (the same logic currently found after the return) so stale display units are cleared and the listener is invoked with null; keep the null check but move/execute the empty-array handling path to call the reset/cache-clear and notify routines currently located later in parseDisplayUnits.
🧹 Nitpick comments (1)
clevertap-core/src/main/java/com/clevertap/android/sdk/displayunits/DisplayUnitCache.java (1)
29-30: 💤 Low valueConsider returning
Listinstead ofArrayListfor interface flexibility.The interface accepts
List<CleverTapDisplayUnit>inupdateDisplayUnitsbut returns the concreteArrayList<CleverTapDisplayUnit>fromgetAllDisplayUnits. ReturningListwould give implementors more flexibility (e.g., returning an immutable list or a different list implementation) and is the standard Java practice for interface design.If
ArrayListis required for backward compatibility with existing SDK consumers, this is acceptable—but worth documenting that constraint.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@clevertap-core/src/main/java/com/clevertap/android/sdk/displayunits/DisplayUnitCache.java` around lines 29 - 30, Change the DisplayUnitCache#getAllDisplayUnits signature to return the interface type List<CleverTapDisplayUnit> (preserving the `@Nullable` annotation) instead of the concrete ArrayList to match the flexibility of updateDisplayUnits and standard Java practice; update any implementing classes and usages that expect ArrayList to accept List (or adapt via new ArrayList<>(list) only where a concrete ArrayList is strictly required) and add a short comment if ArrayList compatibility must be preserved for legacy consumers.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@clevertap-core/src/main/java/com/clevertap/android/sdk/AnalyticsManager.java`:
- Around line 208-210: The code currently does a null-check on
controllerManager.getDisplayUnitCache() and then calls it again, risking a
TOCTOU race; fix by capturing the cache once into a local variable (e.g.,
DisplayUnitCache cache = controllerManager.getDisplayUnitCache()), check that
local for null, and then call cache.getDisplayUnitForID(unitID) and use that
local cache for any subsequent accesses; apply this local-capture pattern in
pushDisplayUnitClickedEventForID, pushDisplayUnitElementClickedEventForID, and
pushDisplayUnitViewedEventForID so all calls to getDisplayUnitForID and other
cache methods use the single captured variable.
---
Outside diff comments:
In
`@clevertap-core/src/main/java/com/clevertap/android/sdk/response/DisplayUnitResponse.java`:
- Around line 84-89: The early return in parseDisplayUnits prevents the existing
"reset cache and notify with null" logic from running when messages.length() ==
0; change the guard so only messages == null returns early, and for an empty
JSONArray ensure you execute the existing cache-reset and callback-notify code
(the same logic currently found after the return) so stale display units are
cleared and the listener is invoked with null; keep the null check but
move/execute the empty-array handling path to call the reset/cache-clear and
notify routines currently located later in parseDisplayUnits.
---
Nitpick comments:
In
`@clevertap-core/src/main/java/com/clevertap/android/sdk/displayunits/DisplayUnitCache.java`:
- Around line 29-30: Change the DisplayUnitCache#getAllDisplayUnits signature to
return the interface type List<CleverTapDisplayUnit> (preserving the `@Nullable`
annotation) instead of the concrete ArrayList to match the flexibility of
updateDisplayUnits and standard Java practice; update any implementing classes
and usages that expect ArrayList to accept List (or adapt via new
ArrayList<>(list) only where a concrete ArrayList is strictly required) and add
a short comment if ArrayList compatibility must be preserved for legacy
consumers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: cf06ff26-6975-405b-b596-b9bf8c9a0966
📒 Files selected for processing (11)
clevertap-core/src/main/java/com/clevertap/android/sdk/AnalyticsManager.javaclevertap-core/src/main/java/com/clevertap/android/sdk/BaseAnalyticsManager.javaclevertap-core/src/main/java/com/clevertap/android/sdk/CleverTapAPI.javaclevertap-core/src/main/java/com/clevertap/android/sdk/ControllerManager.javaclevertap-core/src/main/java/com/clevertap/android/sdk/displayunits/CTDisplayUnitController.javaclevertap-core/src/main/java/com/clevertap/android/sdk/displayunits/DisplayUnitCache.javaclevertap-core/src/main/java/com/clevertap/android/sdk/login/LoginController.javaclevertap-core/src/main/java/com/clevertap/android/sdk/response/DisplayUnitResponse.javaclevertap-core/src/test/java/com/clevertap/android/sdk/AnalyticsManagerTest.ktclevertap-core/src/test/java/com/clevertap/android/sdk/MockAnalyticsManager.ktclevertap-core/src/test/java/com/clevertap/android/sdk/displayunits/CTDisplayUnitControllerTest.kt
Comment on lines
+208
to
210
| if (controllerManager.getDisplayUnitCache() != null) { | ||
| CleverTapDisplayUnit displayUnit = controllerManager.getDisplayUnitCache() | ||
| .getDisplayUnitForID(unitID); |
Contributor
There was a problem hiding this comment.
Avoid TOCTOU on display-unit cache reads (Line 208, Line 261, Line 352).
You null-check controllerManager.getDisplayUnitCache() and then call the getter again. If cache changes between reads, this can throw and silently drop the event.
Suggested fix
- if (controllerManager.getDisplayUnitCache() != null) {
- CleverTapDisplayUnit displayUnit = controllerManager.getDisplayUnitCache()
+ final com.clevertap.android.sdk.displayunits.DisplayUnitCache displayUnitCache =
+ controllerManager.getDisplayUnitCache();
+ if (displayUnitCache != null) {
+ CleverTapDisplayUnit displayUnit = displayUnitCache
.getDisplayUnitForID(unitID);
if (displayUnit != null) {
...
}
}Apply the same local-capture pattern in:
pushDisplayUnitClickedEventForIDpushDisplayUnitElementClickedEventForIDpushDisplayUnitViewedEventForID
Also applies to: 258-263, 351-353
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@clevertap-core/src/main/java/com/clevertap/android/sdk/AnalyticsManager.java`
around lines 208 - 210, The code currently does a null-check on
controllerManager.getDisplayUnitCache() and then calls it again, risking a
TOCTOU race; fix by capturing the cache once into a local variable (e.g.,
DisplayUnitCache cache = controllerManager.getDisplayUnitCache()), check that
local for null, and then call cache.getDisplayUnitForID(unitID) and use that
local cache for any subsequent accesses; apply this local-capture pattern in
pushDisplayUnitClickedEventForID, pushDisplayUnitElementClickedEventForID, and
pushDisplayUnitViewedEventForID so all calls to getDisplayUnitForID and other
cache methods use the single captured variable.
…cached wzrk_* on top Flip the merge order in pushDisplayUnitElementClickedEventForID so that server-controlled attribution still wins over same-named caller keys (the original security intent) WITHOUT silently dropping novel caller wzrk_* keys that aren't in the cached unit. New order of evtData assembly (later layers win on key collision): 1. additionalProperties, merged verbatim (no wzrk_ filter). 2. wzrk_element_id from the dedicated argument. 3. Cached unit wzrk_* fields layered on top. Result: - A caller-supplied wzrk_id never overwrites the cached wzrk_id (same guarantee as before). - Novel caller wzrk_* keys (e.g. an integration that needs to mark a campaign-relevant field the server didn't ship) now pass through instead of being silently dropped + logged. Tests: - AnalyticsManagerTest: replaced `strips wzrk_ keys from additionalProperties` with `cached wzrk_ wins over caller wzrk_ but novel wzrk_ keys pass through`, covering both halves of the new contract. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
clevertap-core/src/test/java/com/clevertap/android/sdk/AnalyticsManagerTest.kt (1)
40-42:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winRemove conflicting
assertEquals/assertFalseimports inAnalyticsManagerTest.kt.
AnalyticsManagerTest.ktimports bothorg.junit.Assert.assertEquals/assertFalseandkotlin.test.assertEquals/assertFalse, but the code callsassertEquals(...)andassertFalse(...)unqualified—this creates an ambiguous reference and breaks compilation. Keep one set or alias one pair.Suggested fix
import org.junit.Assert.assertEquals import org.junit.Assert.assertFalse import org.junit.Assert.assertNotNull import org.junit.Assert.assertTrue ... -import kotlin.test.assertEquals -import kotlin.test.assertFalseAlso applies to: 50-51
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@clevertap-core/src/test/java/com/clevertap/android/sdk/AnalyticsManagerTest.kt` around lines 40 - 42, The test imports two conflicting assertion sets causing ambiguous references; open AnalyticsManagerTest.kt and remove one pair of conflicting imports (either drop org.junit.Assert.assertEquals and org.junit.Assert.assertFalse or drop kotlin.test.assertEquals and kotlin.test.assertFalse) so that the unqualified calls to assertEquals(...) and assertFalse(...) resolve, or alternatively fully-qualify the calls (e.g., kotlin.test.assertEquals) wherever used; ensure only one assertion import pair remains (affecting the imports around assertEquals/assertFalse).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@clevertap-core/src/test/java/com/clevertap/android/sdk/AnalyticsManagerTest.kt`:
- Around line 40-42: The test imports two conflicting assertion sets causing
ambiguous references; open AnalyticsManagerTest.kt and remove one pair of
conflicting imports (either drop org.junit.Assert.assertEquals and
org.junit.Assert.assertFalse or drop kotlin.test.assertEquals and
kotlin.test.assertFalse) so that the unqualified calls to assertEquals(...) and
assertFalse(...) resolve, or alternatively fully-qualify the calls (e.g.,
kotlin.test.assertEquals) wherever used; ensure only one assertion import pair
remains (affecting the imports around assertEquals/assertFalse).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 700b081b-b0db-42d7-af67-a212f80f078e
📒 Files selected for processing (3)
clevertap-core/src/main/java/com/clevertap/android/sdk/AnalyticsManager.javaclevertap-core/src/main/java/com/clevertap/android/sdk/CleverTapAPI.javaclevertap-core/src/test/java/com/clevertap/android/sdk/AnalyticsManagerTest.kt
Code Coverage Debug
Files
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Behavior Changes
Deprecations