SDK-5763: [Android Core SDK] Add DisplayUnitCache interface + setter on CleverTapAPI#999
Merged
piyush-kukadiya merged 6 commits intoMay 19, 2026
Merged
Conversation
✅ 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
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughReplace direct CTDisplayUnitController usage with a host-installable DisplayUnitCache abstraction, make the SDK hold a nullable cache in ControllerManager, add CleverTapAPI#setDisplayUnitCache and element-click APIs, adapt server parsing/reset/event pushers to use the cache, and update tests and mocks to exercise the new flows. ChangesDisplay Unit Cache Migration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
clevertap-core/src/main/java/com/clevertap/android/sdk/login/LoginController.java (1)
293-298:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCapture the cache once before resetting it.
Line 293 calls the getter twice, so a concurrent setter can swap the cache between the null-check and
reset(). That can skip the logout reset or throw if the second lookup returns null.🔧 Proposed fix
+import com.clevertap.android.sdk.displayunits.DisplayUnitCache; ... - if (controllerManager.getDisplayUnitCache() != null) { - controllerManager.getDisplayUnitCache().reset(); + DisplayUnitCache cache = controllerManager.getDisplayUnitCache(); + if (cache != null) { + cache.reset(); } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clevertap-core/src/main/java/com/clevertap/android/sdk/login/LoginController.java` around lines 293 - 298, In LoginController, avoid calling controllerManager.getDisplayUnitCache() twice: capture its result into a local variable (e.g., displayUnitCache) before the null check, then call displayUnitCache.reset() if non-null; otherwise call config.getLogger().verbose with the Constants.FEATURE_DISPLAY_UNIT message. This prevents a race where the cache can be swapped between the null-check and reset, which could skip reset or throw a NPE.clevertap-core/src/main/java/com/clevertap/android/sdk/AnalyticsManager.java (1)
204-216:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReuse the same cache reference in both event paths.
Lines 204-206 and 236-238 repeat the same TOCTOU pattern. If the cache is swapped or cleared between the null-check and
getDisplayUnitForID(), the event can be dropped or resolved against a different instance.🔧 Proposed fix
+import com.clevertap.android.sdk.displayunits.DisplayUnitCache; ... - if (controllerManager.getDisplayUnitCache() != null) { - CleverTapDisplayUnit displayUnit = controllerManager.getDisplayUnitCache() - .getDisplayUnitForID(unitID); + DisplayUnitCache cache = controllerManager.getDisplayUnitCache(); + if (cache != null) { + CleverTapDisplayUnit displayUnit = cache.getDisplayUnitForID(unitID); if (displayUnit != null) {Apply the same pattern in
pushDisplayUnitViewedEventForID(...).Also applies to: 236-244
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clevertap-core/src/main/java/com/clevertap/android/sdk/AnalyticsManager.java` around lines 204 - 216, The code performs a TOCTOU check on controllerManager.getDisplayUnitCache(): first checking for null and then calling getDisplayUnitForID(), which can race if the cache is swapped; to fix, capture controllerManager.getDisplayUnitCache() into a local variable (e.g., displayUnitCache) at the start of pushDisplayUnitViewedEventForID(...) and use that local reference for both the null check and the subsequent getDisplayUnitForID() call (and similarly for the other repeated block around lines 236-244), ensuring the same cache instance is used for the entire logic path before queuing the event and calling coreMetaData.setWzrkParams().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@clevertap-core/src/main/java/com/clevertap/android/sdk/CleverTapAPI.java`:
- Around line 1475-1476: In CleverTapAPI, avoid the TOCTOU by capturing the
display unit cache once into a local variable before the null-check and then
using that variable for getAllDisplayUnits(); specifically, in the blocks that
call coreState.getControllerManager().getDisplayUnitCache() (the occurrences
around the current lines and the similar code near line 1794), assign the result
to a local variable (e.g., displayUnitCache) and use that for the null-check and
subsequent getAllDisplayUnits() call so the same instance is used throughout the
method.
In
`@clevertap-core/src/main/java/com/clevertap/android/sdk/ControllerManager.java`:
- Line 31: The shared field displayUnitCache in ControllerManager is written by
setDisplayUnitCache() and read by getDisplayUnitCache() without synchronization,
causing visibility races; fix it by making the field volatile (or alternatively
synchronizing both setDisplayUnitCache() and getDisplayUnitCache()) so writes
are immediately visible to readers like those in CleverTapAPI and after
DisplayUnitResponse initialization, ensuring consistent cross-thread visibility
of the DisplayUnitCache reference.
In
`@clevertap-core/src/main/java/com/clevertap/android/sdk/response/DisplayUnitResponse.java`:
- Around line 87-94: The code re-reads controllerManager.getDisplayUnitCache()
after leaving the synchronized(displayUnitControllerLock) block, allowing a
concurrent setDisplayUnitCache() to swap the cache and potentially drop or
misapply the parsed payload; fix it by capturing the cache instance inside the
synchronized block (use the same DisplayUnitCache reference created or returned
within the synchronized section) and then call
cache.updateDisplayUnits(messages) on that captured instance (referencing
displayUnitControllerLock, controllerManager.getDisplayUnitCache(),
controllerManager.setDisplayUnitCache(), DisplayUnitCache cache, and
updateDisplayUnits(messages)) so the lazy-install and update operate on the
exact same cache object.
---
Outside diff comments:
In
`@clevertap-core/src/main/java/com/clevertap/android/sdk/AnalyticsManager.java`:
- Around line 204-216: The code performs a TOCTOU check on
controllerManager.getDisplayUnitCache(): first checking for null and then
calling getDisplayUnitForID(), which can race if the cache is swapped; to fix,
capture controllerManager.getDisplayUnitCache() into a local variable (e.g.,
displayUnitCache) at the start of pushDisplayUnitViewedEventForID(...) and use
that local reference for both the null check and the subsequent
getDisplayUnitForID() call (and similarly for the other repeated block around
lines 236-244), ensuring the same cache instance is used for the entire logic
path before queuing the event and calling coreMetaData.setWzrkParams().
In
`@clevertap-core/src/main/java/com/clevertap/android/sdk/login/LoginController.java`:
- Around line 293-298: In LoginController, avoid calling
controllerManager.getDisplayUnitCache() twice: capture its result into a local
variable (e.g., displayUnitCache) before the null check, then call
displayUnitCache.reset() if non-null; otherwise call config.getLogger().verbose
with the Constants.FEATURE_DISPLAY_UNIT message. This prevents a race where the
cache can be swapped between the null-check and reset, which could skip reset or
throw a NPE.
🪄 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: ea70abc3-382a-4df7-a5bf-b527d041e093
📒 Files selected for processing (8)
clevertap-core/src/main/java/com/clevertap/android/sdk/AnalyticsManager.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.kt
Comment on lines
+87
to
+94
| synchronized (displayUnitControllerLock) {// lock to avoid multiple instance creation for controller | ||
| if (controllerManager.getCTDisplayUnitController() == null) { | ||
| controllerManager.setCTDisplayUnitController(new CTDisplayUnitController()); | ||
| if (controllerManager.getDisplayUnitCache() == null) { | ||
| controllerManager.setDisplayUnitCache(new CTDisplayUnitController()); | ||
| } | ||
| } | ||
| ArrayList<CleverTapDisplayUnit> displayUnits = controllerManager.getCTDisplayUnitController() | ||
| .updateDisplayUnits(messages); | ||
| DisplayUnitCache cache = controllerManager.getDisplayUnitCache(); | ||
| ArrayList<CleverTapDisplayUnit> displayUnits = cache != null | ||
| ? cache.updateDisplayUnits(messages) : null; |
Contributor
There was a problem hiding this comment.
Keep the lazy-install and update on the same cache instance.
Line 92 re-reads the cache after the synchronized install path, so a concurrent setDisplayUnitCache() call can replace the controller between installation and updateDisplayUnits(). That can drop the parsed payload or apply it to the wrong cache instance.
🔧 Proposed fix
- synchronized (displayUnitControllerLock) {// lock to avoid multiple instance creation for controller
- if (controllerManager.getDisplayUnitCache() == null) {
- controllerManager.setDisplayUnitCache(new CTDisplayUnitController());
- }
- }
- DisplayUnitCache cache = controllerManager.getDisplayUnitCache();
+ DisplayUnitCache cache;
+ synchronized (displayUnitControllerLock) { // lock to avoid multiple instance creation for controller
+ cache = controllerManager.getDisplayUnitCache();
+ if (cache == null) {
+ cache = new CTDisplayUnitController();
+ controllerManager.setDisplayUnitCache(cache);
+ }
+ }
ArrayList<CleverTapDisplayUnit> displayUnits = cache != null
? cache.updateDisplayUnits(messages) : null;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@clevertap-core/src/main/java/com/clevertap/android/sdk/response/DisplayUnitResponse.java`
around lines 87 - 94, The code re-reads controllerManager.getDisplayUnitCache()
after leaving the synchronized(displayUnitControllerLock) block, allowing a
concurrent setDisplayUnitCache() to swap the cache and potentially drop or
misapply the parsed payload; fix it by capturing the cache instance inside the
synchronized block (use the same DisplayUnitCache reference created or returned
within the synchronized section) and then call
cache.updateDisplayUnits(messages) on that captured instance (referencing
displayUnitControllerLock, controllerManager.getDisplayUnitCache(),
controllerManager.setDisplayUnitCache(), DisplayUnitCache cache, and
updateDisplayUnits(messages)) so the lazy-install and update operate on the
exact same cache object.
Code Coverage Debug
Files
|
CTLalit
force-pushed
the
task/SDK-5763-display-unit-cache-interface
branch
from
1325170 to
6a50498
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
clevertap-core/src/test/java/com/clevertap/android/sdk/displayunits/CTDisplayUnitControllerTest.kt (1)
63-79: ⚡ Quick winAdd one test for blank/invalid unit IDs being ignored.
The suite covers null/empty-list and reset paths well, but it doesn’t assert the new filtering behavior for invalid IDs. Please add a case with a blank-id unit and verify it is not retrievable from cache.
🤖 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/displayunits/CTDisplayUnitControllerTest.kt` around lines 63 - 79, Add a new unit test in CTDisplayUnitControllerTest that calls ctDisplayUnitController.updateDisplayUnits with a list containing a CleverTapDisplayUnit (or mock from mockDisplayUnits) whose id is blank/empty, then assert that ctDisplayUnitController.allDisplayUnits does not include that unit and that ctDisplayUnitController.getDisplayUnitForID("") (or the blank id used) returns null; place this alongside existing tests (e.g., test_updateDisplayUnits_validList_populatesCache and test_updateDisplayUnits_replacesPreviousCache) to verify blank/invalid IDs are ignored by updateDisplayUnits and not stored in the cache.
🤖 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.
Nitpick comments:
In
`@clevertap-core/src/test/java/com/clevertap/android/sdk/displayunits/CTDisplayUnitControllerTest.kt`:
- Around line 63-79: Add a new unit test in CTDisplayUnitControllerTest that
calls ctDisplayUnitController.updateDisplayUnits with a list containing a
CleverTapDisplayUnit (or mock from mockDisplayUnits) whose id is blank/empty,
then assert that ctDisplayUnitController.allDisplayUnits does not include that
unit and that ctDisplayUnitController.getDisplayUnitForID("") (or the blank id
used) returns null; place this alongside existing tests (e.g.,
test_updateDisplayUnits_validList_populatesCache and
test_updateDisplayUnits_replacesPreviousCache) to verify blank/invalid IDs are
ignored by updateDisplayUnits and not stored in the cache.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ce0b5982-f9ff-4a51-bf55-0f8285fb1fba
📒 Files selected for processing (4)
clevertap-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/response/DisplayUnitResponse.javaclevertap-core/src/test/java/com/clevertap/android/sdk/displayunits/CTDisplayUnitControllerTest.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- clevertap-core/src/main/java/com/clevertap/android/sdk/displayunits/DisplayUnitCache.java
deeksha-rgb
approved these changes
May 19, 2026
CTLalit
force-pushed
the
task/SDK-5763-display-unit-cache-interface
branch
from
f26bea0 to
69b43ff
Compare
piyush-kukadiya
approved these changes
May 19, 2026
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
…ID, 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) <[email protected]>
…o 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) <[email protected]>
…ay-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) <[email protected]>
…leverTapAPI 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) <[email protected]>
piyush-kukadiya
force-pushed
the
task/SDK-5763-display-unit-cache-interface
branch
from
935ef49 to
3cda547
Compare
This was referenced May 19, 2026
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
4 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.
Jira
SDK-5763 — child of epic SDK-5256 (Native Display v2).
Companion PRs: iOS Core SDK SDK-5764 and Native Display SDK SDK-5765.
Summary
Extracts
CTDisplayUnitController's read+update surface into a publicDisplayUnitCacheinterface and exposesCleverTapAPI.setDisplayUnitCache(...)so external SDKs (Native Display) can install their own cache implementation.Why: ND SDK's manual JSON mode feeds units the Core SDK never received from its server pipeline, so
pushDisplayUnit*EventForID's mandatory cache lookup misses and analytics are unattributed.pushEvent("Notification Viewed", ...)is not a workaround — those names are inDEFAULT_RESTRICTED_EVENT_NAMESand dropped at validation. Letting ND SDK plug its own cache implementation lets attribution events resolve through the bridge's existing cache.Changes
displayunits/DisplayUnitCache.javagetAllDisplayUnits,getDisplayUnitForID,updateDisplayUnits,reset)displayunits/CTDisplayUnitController.javaimplements DisplayUnitCache(no behaviour change)ControllerManager.javaDisplayUnitCachereference;getDisplayUnitCache()/setDisplayUnitCache(); deprecatedgetCTDisplayUnitController()/setCTDisplayUnitController()retained as adaptersCleverTapAPI.javasetDisplayUnitCache(DisplayUnitCache);getDisplayUnitForId/getAllDisplayUnitsroute via interfaceAnalyticsManager.javapushDisplayUnit*EventForIDresolve viagetDisplayUnitCache()response/DisplayUnitResponse.javalogin/LoginController.javatest/.../AnalyticsManagerTest.ktdisplayUnitCacheBackwards compatibility
CTDisplayUnitControlleris lazy-installed on firstadUnit_notifsresponse (same lazy-init point as before, just routed via the new accessor).getCTDisplayUnitController()returns the cast default impl when present, null when a custom impl is installed.DisplayUnitListenercontinues to fire only for server-pipeline activity.Test plan
:clevertap-core:compileDebugJavaWithJavacclean:clevertap-core:testDebugUnitTestsuite passesSummary by CodeRabbit
New Features
Improvements
Tests
Deprecations