Feat/eit/sdk 4341 #798
Feat/eit/sdk 4341 #798
Conversation
## Walkthrough
This update introduces a new encryption-in-transit feature and a major refactor of the network layer in the CleverTap Android SDK. It adds new classes for key management, encryption, ARP/IJ storage, and network state, and replaces the legacy `NetworkManager.java` with a new Kotlin implementation. Multiple classes are updated to support these changes, and comprehensive unit tests are added.
## Changes
| File(s) / Group | Change Summary |
|-------------------------------------------------------------------------------------------------|---------------|
| `CleverTapFactory.kt`, `DBManager.kt`, `CleverTapAPI.java`, `SessionManager.java` | Refactored core initialization and session logic to support new encryption and network components; updated method calls and dependencies. |
| `CleverTapInstanceConfig.java`, `ManifestInfo.java`, `CoreMetaData.java` | Added encryption-in-transit configuration fields and accessors; updated JSON and parcel serialization; added relaxNetwork flag. |
| `Constants.java` | Removed obsolete header and namespace constants. |
| `cryption/AESGCMCrypt.kt`, `cryption/CTKeyGenerator.kt`, `cryption/CryptFactory.kt`,<br>`cryption/CryptRepository.kt`, `cryption/CryptExtensions.kt` | Refactored cryptography classes to use injected key generator and repository; removed direct Android key management from crypt classes; added Base64 utilities. |
| `network/ArpRepo.kt`, `network/IJRepo.kt`, `network/NetworkEncryptionManager.kt`,<br>`network/NetworkManager.kt`, `network/NetworkRepo.kt`, `network/QueueHeaderBuilder.kt` | Introduced new Kotlin classes for ARP/IJ storage, network encryption, network state, and header building; replaced legacy Java network manager. |
| `network/api/CtApi.kt`, `network/api/CtApiWrapper.kt`, `network/api/EncryptedResponseBody.kt`,<br>`network/api/EncryptedSendQueueRequestBody.kt`, `network/api/EncryptionResult.kt` | Updated network API to support encryption, new header/query constants, and new request/response body structures; added encryption result types. |
| `response/ARPResponse.java`, `response/MetadataResponse.java`, `response/PushAmpResponse.java`,<br>`response/CleverTapResponseDecorator.java` | Refactored response classes to use new ARP/IJ repositories and updated method signatures; minor formatting and logic updates. |
| `utils/CTJsonConverter.java` | Replaced DB-dependent push ID conversion with a direct method. |
| `sample/src/main/AndroidManifest.xml`, `sample/src/main/java/com/clevertap/demo/MyApplication.kt` | Added encryption-in-transit manifest flag and demo in-app button listener. |
| `gradle-scripts/commons.gradle` | Increased JVM heap for unit tests. |
| `test/java/com/clevertap/android/sdk/`, `test/java/com/clevertap/android/sdk/network/`,<br>`test/java/com/clevertap/android/sdk/network/api/`, `test/java/com/clevertap/android/sdk/network/http/` | Added/updated tests for new network, encryption, ARP/IJ, and header builder classes; migrated to MockK; expanded test coverage; removed legacy dependencies. |
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant App
participant CleverTapFactory
participant CTKeyGenerator
participant CryptFactory
participant NetworkEncryptionManager
participant NetworkManager
participant NetworkRepo
participant ArpRepo
participant IJRepo
participant QueueHeaderBuilder
participant CtApi
App->>CleverTapFactory: Initialize CleverTap
CleverTapFactory->>CTKeyGenerator: Create/generate key
CleverTapFactory->>CryptFactory: Inject CTKeyGenerator
CleverTapFactory->>NetworkEncryptionManager: Inject CTKeyGenerator, CryptFactory
CleverTapFactory->>IJRepo: Create IJRepo
CleverTapFactory->>ArpRepo: Create ArpRepo
CleverTapFactory->>NetworkRepo: Create NetworkRepo
CleverTapFactory->>QueueHeaderBuilder: Inject dependencies
CleverTapFactory->>NetworkManager: Inject encryptionManager, arpRepo, ijRepo, networkRepo, queueHeaderBuilder
App->>NetworkManager: flushDBQueue / sendQueue
NetworkManager->>QueueHeaderBuilder: buildHeader()
NetworkManager->>CtApi: sendQueue(headers, body, isEncrypted)
CtApi-->>NetworkManager: Response (possibly encrypted)
NetworkManager->>NetworkEncryptionManager: decryptResponse (if encrypted)
NetworkEncryptionManager-->>NetworkManager: Decrypted payload or failure
NetworkManager->>ResponseProcessors: processResponse()Possibly related PRs
Suggested labels
Suggested reviewers
Learnt from: CTLalit |
- exposes the encryption in transit flag in config.
- Extracts out the CTKeyGenerator class which is responsible for key generation - Moves extensions to correct class - adds methods to crypt repo for key persistence - Make data classes for request and response bodies - introduces NetworkEncryptionManager which is abstraction of feature encrypt/decrypt in network layer of SDK.
- passes correct params to refactored classes.
- adds check for encryption on network and relevant helper classes - adds param in CtApi so that is can be passed in the header - exposes session level enc key - fixes test cases
- adds check for encryption on network and relevant helper classes - adds param in CtApi so that is can be passed in the header - exposes session level enc key - fixes test cases - introduces constants in the network manager class for header fields.
- ide warnings and safe fixes
- decrypts the response received at SDK if required - header value is read to check if response is encrypted
- the keys used for parsing were incorrect.
- failed due to using Mockito mocking with Mockk verify methods. - fixes incorrect json string which was failing the tests.
- handles the prefs edits for the respective things in their own repo. - breaks down dependencies - lighter and more testable NetworkManager.kt
- fixes duplication in testing code - bubbles up logger for better test case logging
- fixes test for 200ok case with null body.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (6)
clevertap-core/src/main/java/com/clevertap/android/sdk/network/NetworkManager.kt (6)
111-119:mNetworkHeadersListenersis not thread-safe – replace withCopyOnWriteArrayList.
addNetworkHeadersListener()/removeNetworkHeadersListener()may be called from the main thread while callbacks are iterated on a worker thread. Using a plainArrayListrisksConcurrentModificationException.
830-832: Consider logging decryption failure details for debugging.When decryption fails, the method returns
falsewithout logging the failure reason, which could make troubleshooting difficult.
893-894: Improve exception handling for JSON parsing.The JSONException is silently swallowed with only a comment, which could hide important parsing errors.
🧰 Tools
🪛 detekt (1.23.8)
[warning] 893-893: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
921-928: Improve exception handling in push notification processing.Multiple exception types are caught but not properly logged, making debugging difficult. The
e.printStackTrace()should be replaced with proper logging.🧰 Tools
🪛 detekt (1.23.8)
[warning] 921-921: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
293-453: Break down thegetQueueHeadermethod to improve maintainability.This 160-line method violates the Single Responsibility Principle by handling device info, app fields, identities, notifications, and referrer data all in one place.
1019-1031: Modernize network connectivity check and improve error handling.The current implementation uses deprecated
activeNetworkInfoAPI and the optimistic fallback approach might mask real connectivity issues.
🧹 Nitpick comments (1)
clevertap-core/src/main/java/com/clevertap/android/sdk/network/NetworkManager.kt (1)
699-706: Document encryption fallback behavior for security transparency.The method silently falls back to unencrypted requests when encryption fails (lines 699-706). For security-sensitive applications, this behavior should be clearly documented and potentially made configurable.
Consider adding debug logging when fallback occurs:
} else { + logger.verbose(config.accountId, "Encryption failed, falling back to unencrypted request: ${encryptionResult}") logger.verbose("Normal Request cause encryption failed = $body")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
clevertap-core/src/main/java/com/clevertap/android/sdk/network/NetworkManager.kt(1 hunks)clevertap-core/src/main/java/com/clevertap/android/sdk/network/api/CtApi.kt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- clevertap-core/src/main/java/com/clevertap/android/sdk/network/api/CtApi.kt
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: CTLalit
PR: CleverTap/clevertap-android-sdk#798
File: clevertap-core/src/main/java/com/clevertap/android/sdk/network/NetworkManager.kt:152-152
Timestamp: 2025-05-30T08:21:16.938Z
Learning: CTLalit prefers to avoid modifying legacy code when working on specific features, keeping the scope focused to reduce risk and maintain clear boundaries for changes.
clevertap-core/src/main/java/com/clevertap/android/sdk/network/NetworkManager.kt (1)
Learnt from: CTLalit
PR: CleverTap/clevertap-android-sdk#798
File: clevertap-core/src/main/java/com/clevertap/android/sdk/network/NetworkManager.kt:558-558
Timestamp: 2025-05-30T08:41:45.282Z
Learning: In the CleverTap Android SDK, debug logging that includes request bodies is controlled by build configuration and only enabled in development builds, not in production. This build-time control already addresses potential sensitive data exposure concerns.
🧬 Code Graph Analysis (1)
clevertap-core/src/main/java/com/clevertap/android/sdk/network/NetworkManager.kt (11)
clevertap-core/src/main/java/com/clevertap/android/sdk/response/ARPResponse.java (1)
ARPResponse(19-139)clevertap-core/src/main/java/com/clevertap/android/sdk/response/ConsoleResponse.java (1)
ConsoleResponse(10-56)clevertap-core/src/main/java/com/clevertap/android/sdk/response/InboxResponse.java (1)
InboxResponse(14-80)clevertap-core/src/main/java/com/clevertap/android/sdk/response/PushAmpResponse.java (1)
PushAmpResponse(19-132)clevertap-core/src/main/java/com/clevertap/android/sdk/response/FetchVariablesResponse.java (1)
FetchVariablesResponse(14-82)clevertap-core/src/main/java/com/clevertap/android/sdk/response/DisplayUnitResponse.java (1)
DisplayUnitResponse(15-96)clevertap-core/src/main/java/com/clevertap/android/sdk/response/FeatureFlagResponse.java (1)
FeatureFlagResponse(12-69)clevertap-core/src/main/java/com/clevertap/android/sdk/response/ProductConfigResponse.java (1)
ProductConfigResponse(13-87)clevertap-core/src/main/java/com/clevertap/android/sdk/network/api/CtApi.kt (2)
sendQueue(66-82)performHandshakeForDomain(84-104)clevertap-core/src/main/java/com/clevertap/android/sdk/network/NetworkHeadersListener.kt (1)
fromEventGroup(29-36)clevertap-core/src/main/java/com/clevertap/android/sdk/inapp/evaluation/EvaluationManager.kt (1)
fromBoolean(37-39)
🪛 detekt (1.23.8)
clevertap-core/src/main/java/com/clevertap/android/sdk/network/NetworkManager.kt
[warning] 152-152: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
[warning] 893-893: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
[warning] 921-921: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
clevertap-core/src/main/java/com/clevertap/android/sdk/network/NetworkManager.kt (4)
111-119:mNetworkHeadersListenersis not thread-safe – replace withCopyOnWriteArrayList.
addNetworkHeadersListener()/removeNetworkHeadersListener()may be called from the main thread while callbacks are iterated on a worker thread (notifyNetworkHeaderListeners). Using a plainArrayListrisksConcurrentModificationException.- private val mNetworkHeadersListeners: MutableList<NetworkHeadersListener> = ArrayList() + private val mNetworkHeadersListeners = CopyOnWriteArrayList<NetworkHeadersListener>()
885-886: 🛠️ Refactor suggestionImprove exception handling for JSON parsing.
The JSONException is silently swallowed with only a comment, which could hide important parsing errors.
} catch (jsonException: JSONException) { - //skip + logger.verbose(config.accountId, "Failed to parse event in queue", jsonException)🧰 Tools
🪛 detekt (1.23.8)
[warning] 885-885: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
913-920: 🛠️ Refactor suggestionImprove exception handling in push notification processing.
Multiple exception types are caught but not properly logged, making debugging difficult. The
e.printStackTrace()should be replaced with proper logging.} catch (e: JSONException) { - logger.verbose( - config.accountId, - "Encountered an exception while parsing the push notification viewed event queue" - ) + logger.verbose( + config.accountId, + "Exception while parsing push notification viewed event queue", + e + ) } catch (e: Exception) { - e.printStackTrace() + logger.verbose( + config.accountId, + "Unexpected exception while processing push notification queue", + e + )🧰 Tools
🪛 detekt (1.23.8)
[warning] 913-913: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
152-157: 🛠️ Refactor suggestionImprove exception handling to preserve original exception context.
The exception is caught and swallowed without logging the original exception details, which could make debugging difficult.
} catch (e: Exception) { - config.logger.verbose( - config.accountId, - "met with exception while notifying listeners for PushImpressionSentToServer event" - ) + config.logger.verbose( + config.accountId, + "Exception while notifying listeners for PushImpressionSentToServer event", + e + )🧰 Tools
🪛 detekt (1.23.8)
[warning] 152-152: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
🧹 Nitpick comments (3)
clevertap-core/src/main/java/com/clevertap/android/sdk/network/NetworkManager.kt (3)
293-453: Break down thegetQueueHeadermethod to improve maintainability.This 160-line method violates the Single Responsibility Principle by handling device info, app fields, identities, notifications, and referrer data all in one place.
Consider decomposing into smaller, focused methods:
private fun getQueueHeader(context: Context, caller: String?): JSONObject? { return try { val header = JSONObject() - // Add caller if available - if (caller != null) { - header.put(Constants.D_SRC, caller) - } - // ... 150+ lines of header construction + + addBasicFields(header, caller) + addDeviceAndAppFields(header) + addIdentityFields(header) + addNotificationFields(header) + addReferrerFields(header) + addMetricsFields(header, context) + header } catch (e: JSONException) { logger.verbose(config.accountId, "Failed to attach header", e) null } } +private fun addBasicFields(header: JSONObject, caller: String?) { /* ... */ } +private fun addDeviceAndAppFields(header: JSONObject) { /* ... */ } +private fun addIdentityFields(header: JSONObject) { /* ... */ } +private fun addNotificationFields(header: JSONObject) { /* ... */ } +private fun addReferrerFields(header: JSONObject) { /* ... */ } +private fun addMetricsFields(header: JSONObject, context: Context) { /* ... */ }
822-824: Consider logging encryption failures for debugging.When decryption fails, the method returns
falsewithout logging the failure reason, which could make troubleshooting difficult.is EncryptionFailure -> { + logger.verbose(config.accountId, "Failed to decrypt response: ${decryptResponse.error}") return false // todo lp check if this should be considered as nw failure? }
1011-1023: Modernize network connectivity check and improve error handling.The current implementation uses deprecated
activeNetworkInfoAPI and the optimistic fallback approach might mask real connectivity issues.Consider updating to use modern connectivity APIs:
@JvmStatic fun isNetworkOnline(context: Context): Boolean { return try { val cm = context.getSystemService(Context.CONNECTIVITY_SERVICE) as? ConnectivityManager ?: return false // be more conservative when service unavailable - @SuppressLint("MissingPermission") val netInfo = cm.activeNetworkInfo - return netInfo != null && netInfo.isConnected + if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.M) { + val network = cm.activeNetwork ?: return false + val capabilities = cm.getNetworkCapabilities(network) ?: return false + return capabilities.hasCapability(NetworkCapabilities.NET_CAPABILITY_INTERNET) + } else { + @SuppressLint("MissingPermission") + val netInfo = cm.activeNetworkInfo + return netInfo != null && netInfo.isConnected + } } catch (e: SecurityException) { + logger.verbose(config.accountId, "ACCESS_NETWORK_STATE permission required", e) + return false // be conservative when permission missing } catch (ignore: Exception) { - // lets be optimistic, if we are truly offline we handle the exception - return true + logger.verbose(config.accountId, "Failed to check network connectivity", ignore) + return false // be conservative on unexpected errors } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
clevertap-core/src/main/java/com/clevertap/android/sdk/network/NetworkManager.kt(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: CTLalit
PR: CleverTap/clevertap-android-sdk#798
File: clevertap-core/src/main/java/com/clevertap/android/sdk/network/NetworkManager.kt:152-152
Timestamp: 2025-05-30T08:21:16.938Z
Learning: CTLalit prefers to avoid modifying legacy code when working on specific features, keeping the scope focused to reduce risk and maintain clear boundaries for changes.
clevertap-core/src/main/java/com/clevertap/android/sdk/network/NetworkManager.kt (1)
Learnt from: CTLalit
PR: CleverTap/clevertap-android-sdk#798
File: clevertap-core/src/main/java/com/clevertap/android/sdk/network/NetworkManager.kt:558-558
Timestamp: 2025-05-30T08:41:45.282Z
Learning: In the CleverTap Android SDK, debug logging that includes request bodies is controlled by build configuration and only enabled in development builds, not in production. This build-time control already addresses potential sensitive data exposure concerns.
🧬 Code Graph Analysis (1)
clevertap-core/src/main/java/com/clevertap/android/sdk/network/NetworkManager.kt (10)
clevertap-core/src/main/java/com/clevertap/android/sdk/response/ARPResponse.java (1)
ARPResponse(19-139)clevertap-core/src/main/java/com/clevertap/android/sdk/response/ConsoleResponse.java (1)
ConsoleResponse(10-56)clevertap-core/src/main/java/com/clevertap/android/sdk/response/InboxResponse.java (1)
InboxResponse(14-80)clevertap-core/src/main/java/com/clevertap/android/sdk/response/PushAmpResponse.java (1)
PushAmpResponse(19-132)clevertap-core/src/main/java/com/clevertap/android/sdk/response/DisplayUnitResponse.java (1)
DisplayUnitResponse(15-96)clevertap-core/src/main/java/com/clevertap/android/sdk/response/FeatureFlagResponse.java (1)
FeatureFlagResponse(12-69)clevertap-core/src/main/java/com/clevertap/android/sdk/response/ProductConfigResponse.java (1)
ProductConfigResponse(13-87)clevertap-core/src/main/java/com/clevertap/android/sdk/network/api/CtApi.kt (2)
sendQueue(66-82)performHandshakeForDomain(84-104)clevertap-core/src/main/java/com/clevertap/android/sdk/network/NetworkHeadersListener.kt (1)
fromEventGroup(29-36)clevertap-core/src/main/java/com/clevertap/android/sdk/inapp/evaluation/EvaluationManager.kt (1)
fromBoolean(37-39)
🪛 detekt (1.23.8)
clevertap-core/src/main/java/com/clevertap/android/sdk/network/NetworkManager.kt
[warning] 152-152: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
[warning] 885-885: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
[warning] 913-913: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
🔇 Additional comments (2)
clevertap-core/src/main/java/com/clevertap/android/sdk/network/NetworkManager.kt (2)
63-103: Well-structured constructor with proper dependency injection.The constructor design effectively uses dependency injection and clearly separates concerns. The initialization of response processors in the
cleverTapResponseslist is comprehensive and properly structured.
63-1025: Excellent architectural design and Kotlin modernization.This NetworkManager implementation successfully:
• Modernizes the codebase: Clean Kotlin rewrite with proper language idioms and null safety
• Integrates new features: Seamless integration with encryption (NetworkEncryptionManager), repositories (IJRepo,ArpRepo), and updated API components
• Maintains extensibility: Well-designed response processing chain and proper dependency injection
• Follows best practices: Clear separation of concerns and comprehensive error handling patternsThe class effectively replaces the legacy Java implementation while adding substantial new functionality like encryption in transit.
🧰 Tools
🪛 detekt (1.23.8)
[warning] 152-152: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
[warning] 885-885: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
[warning] 913-913: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
- handles json validation and correct json - simplifies code slightly
…ers in a readable class, Code coverage for header object - Introduces a new queue header builder class - Fixes retry counter ignoring variables response success - adds code coverage for header object which was missing
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
clevertap-core/src/main/java/com/clevertap/android/sdk/network/NetworkManager.kt (3)
109-117:mNetworkHeadersListenersis not thread-safe – replace withCopyOnWriteArrayList.
addNetworkHeadersListener()/removeNetworkHeadersListener()may be called from the main thread while callbacks are iterated on a worker thread (notifyNetworkHeaderListeners). Using a plainArrayListrisksConcurrentModificationException.- private val mNetworkHeadersListeners: MutableList<NetworkHeadersListener> = ArrayList() + private val mNetworkHeadersListeners = CopyOnWriteArrayList<NetworkHeadersListener>()
515-541: Add configurable encryption fallback and document behavior.The current implementation silently falls back to unencrypted requests whenever
encryptResponsereturnsEncryptionFailure. For security-sensitive use cases, this behavior should be both documented and made optional.• Document the fallback in your public docs (e.g., README.md or SDK reference), clearly stating that on encryption failure the SDK will retry unencrypted.
• Expose a new flag inCleverTapInstanceConfig(for example,strictEncryptionMode) to disable the automatic fallback and instead surface an error or drop the request.
• UpdateNetworkManagerto respect this flag and add tests covering both fallback-enabled and strict-mode scenarios.
656-668: Consider logging encryption failures for debugging.When decryption fails, the method returns
falsewithout logging the failure reason, which could make troubleshooting difficult.is EncryptionFailure -> { + logger.verbose(config.accountId, "Failed to decrypt response: ${decryptResponse.error}") return false // todo lp check if this should be considered as nw failure? }clevertap-core/src/test/java/com/clevertap/android/sdk/network/NetworkManagerTest.kt (2)
72-75: Reset MockK interactions intearDown()to avoid cross-test leakageThe same
networkEncryptionManagermock instance is shared across all tests, yet its recorded interactions are never cleared.
Tests that verify call counts (verify(exactly = 0) { … }) can therefore fail or give false-positives depending on previous test execution order.@After fun tearDown() { closeable?.close() + // Ensures mocks don't carry interaction history into the next test run + clearAllMocks() // <-- import io.mockk.clearAllMocks }
536-540: Replacespykwith a relaxedmockkforCleverTapResponse
spyk<CleverTapResponse>()tries to create a spy of the real class, calling the original (often abstract / open) methods.
IfCleverTapResponseis an interface or an abstract class with un-implemented members, the spy will throw at runtime whenprocessResponseis invoked bysendQueue.Switching to a relaxed mock avoids this risk while still letting you
verifythe interaction.-for (i in 1..10) { - val spyProcessor = spyk<CleverTapResponse>() - responses.add(spyProcessor) -} +for (i in 1..10) { + val processor = mockk<CleverTapResponse>(relaxed = true) + responses.add(processor) +}
🧹 Nitpick comments (6)
clevertap-core/src/main/java/com/clevertap/android/sdk/utils/CTJsonConverter.java (1)
181-188: Consider using consistent logging pattern.The method uses
Logger.v()directly while other methods in the class use injectedILoggerinstances passed as parameters. For consistency with the existing codebase patterns, consider either:
- Adding an
ILoggerparameter to this method, or- Using a consistent logging approach across the utility class
The implementation logic is correct and handles the conversion properly.
clevertap-core/src/main/java/com/clevertap/android/sdk/network/QueueHeaderBuilder.kt (3)
15-28: Consider reducing constructor dependencies.The constructor takes 11 dependencies, which suggests high coupling and potential violation of the Single Responsibility Principle. Consider:
- Grouping related dependencies into dedicated service classes
- Using a builder pattern for QueueHeaderBuilder itself
- Creating focused sub-builders for different header sections
This would improve testability and maintainability.
120-123: Document side effect in header building.The method modifies
coreMetaData.isBgPingas a side effect during header construction. Consider documenting this behavior or moving the state update to a separate method to maintain clearer separation of concerns.+// Note: Resets background ping flag after including it in header if (coreMetaData.isBgPing) { header.put("bk", 1) coreMetaData.isBgPing = false }
139-141: Document side effect in session flag handling.Similar to the background ping flag, this method has a side effect of resetting the first request session flag. Consider adding documentation or refactoring to separate state management from header building.
+// Note: Resets first request flag after including it in header header.put("frs", coreMetaData.isFirstRequestInSession) coreMetaData.isFirstRequestInSession = falseclevertap-core/src/test/java/com/clevertap/android/sdk/network/QueueHeaderBuilderTest.kt (2)
102-185: Consider splitting the comprehensive test.While this test provides excellent coverage of all header fields, it's quite large (80+ lines). Consider splitting it into focused tests for different header sections (e.g., config fields, metadata fields, etc.) for better maintainability and clearer failure diagnostics.
The current implementation is thorough and correct, but smaller, focused tests would be easier to debug when they fail.
167-168: Verify identity set assertions.The assertions for
ct_pifield check for the presence of "Email" and "Identity" strings, but these seem like implementation details. Consider verifying the actual structure or format rather than string containment, as the identity set format might change.-assertThat(header.optString("ct_pi")).contains("Email") -assertThat(header.optString("ct_pi")).contains("Identity") +// Consider verifying the actual JSON structure instead +assertThat(header.optString("ct_pi")).isNotEmpty()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
clevertap-core/src/main/java/com/clevertap/android/sdk/CleverTapAPI.java(1 hunks)clevertap-core/src/main/java/com/clevertap/android/sdk/CleverTapFactory.kt(6 hunks)clevertap-core/src/main/java/com/clevertap/android/sdk/network/NetworkManager.kt(1 hunks)clevertap-core/src/main/java/com/clevertap/android/sdk/network/QueueHeaderBuilder.kt(1 hunks)clevertap-core/src/main/java/com/clevertap/android/sdk/response/PushAmpResponse.java(2 hunks)clevertap-core/src/main/java/com/clevertap/android/sdk/utils/CTJsonConverter.java(1 hunks)clevertap-core/src/test/java/com/clevertap/android/sdk/network/NetworkManagerTest.kt(8 hunks)clevertap-core/src/test/java/com/clevertap/android/sdk/network/QueueHeaderBuilderTest.kt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- clevertap-core/src/main/java/com/clevertap/android/sdk/CleverTapFactory.kt
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: CTLalit
PR: CleverTap/clevertap-android-sdk#798
File: clevertap-core/src/main/java/com/clevertap/android/sdk/network/NetworkManager.kt:152-152
Timestamp: 2025-05-30T08:21:16.962Z
Learning: CTLalit prefers to avoid modifying legacy code when working on specific features, keeping the scope focused to reduce risk and maintain clear boundaries for changes.
clevertap-core/src/main/java/com/clevertap/android/sdk/network/NetworkManager.kt (1)
Learnt from: CTLalit
PR: CleverTap/clevertap-android-sdk#798
File: clevertap-core/src/main/java/com/clevertap/android/sdk/network/NetworkManager.kt:558-558
Timestamp: 2025-05-30T08:41:45.328Z
Learning: In the CleverTap Android SDK, debug logging that includes request bodies is controlled by build configuration and only enabled in development builds, not in production. This build-time control already addresses potential sensitive data exposure concerns.
🧬 Code Graph Analysis (2)
clevertap-core/src/main/java/com/clevertap/android/sdk/response/PushAmpResponse.java (1)
clevertap-core/src/main/java/com/clevertap/android/sdk/db/DBAdapter.kt (1)
fetchPushNotificationIds(113-137)
clevertap-core/src/test/java/com/clevertap/android/sdk/network/NetworkManagerTest.kt (2)
test_shared/src/main/java/com/clevertap/android/shared/test/BaseTestCase.kt (1)
getSampleJsonArrayOfJsonObjects(58-67)clevertap-core/src/test/java/com/clevertap/android/sdk/network/IJRepoTest.kt (1)
assertFalse(654-656)
🪛 detekt (1.23.8)
clevertap-core/src/main/java/com/clevertap/android/sdk/network/NetworkManager.kt
[warning] 150-150: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
[warning] 716-716: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
[warning] 744-744: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
🔇 Additional comments (18)
clevertap-core/src/main/java/com/clevertap/android/sdk/CleverTapAPI.java (1)
1188-1188: LGTM! Clean API update following NetworkManager refactoring.The removal of the
contextparameter from thedefineTemplatesmethod call correctly aligns with the new Kotlin NetworkManager API. This change improves the API design by reducing parameter coupling and suggests the context is now handled internally by the NetworkManager implementation.clevertap-core/src/main/java/com/clevertap/android/sdk/response/PushAmpResponse.java (2)
3-3: LGTM: Import statement updated correctly.The import has been properly updated to use the new
pushIdsToJSONArraymethod fromCTJsonConverter.
79-80:✅ Verification successful
Verify null handling for push notification IDs.
The change correctly uses the new
fetchPushNotificationIds()method and converts the result usingpushIdsToJSONArray. However, based on the relevant code snippet fromDBAdapter.kt, thefetchPushNotificationIds()method returnsArray<String?>(nullable strings), but thepushIdsToJSONArraymethod expectsString[].Please verify that null values are properly handled in this conversion. Run the following script to check the method signatures:
🏁 Script executed:
#!/bin/bash # Description: Verify the method signatures for push ID handling # Expected: fetchPushNotificationIds returns Array<String?>, pushIdsToJSONArray expects String[] echo "Checking fetchPushNotificationIds signature:" ast-grep --pattern 'fun fetchPushNotificationIds(): $RETURN_TYPE' echo "Checking pushIdsToJSONArray signature:" ast-grep --pattern 'public static JSONArray pushIdsToJSONArray($PARAM_TYPE)'Length of output: 332
To pinpoint the exact signatures, let’s locate their definitions in the codebase:
🏁 Script executed:
#!/bin/bash # Search for the Kotlin declaration of fetchPushNotificationIds echo "=== fetchPushNotificationIds definition ===" rg -n "fun fetchPushNotificationIds" -C3 # Search for the Java declaration of pushIdsToJSONArray echo "=== pushIdsToJSONArray definition ===" rg -n "pushIdsToJSONArray" -C3Length of output: 4476
Null-safe handling of push IDs is confirmed
fetchPushNotificationIds()returns a non-nullString[](elements may be null), andpushIdsToJSONArrayexplicitly guards against null/empty values viaStrings.isNullOrEmpty(pushId)before adding to theJSONArray. No further changes are needed.clevertap-core/src/main/java/com/clevertap/android/sdk/network/QueueHeaderBuilder.kt (2)
29-60: LGTM: Well-structured header building orchestration.The
buildHeadermethod provides good error handling and clear orchestration of header field addition. The early return for missing credentials is appropriate.
184-189: Verify InAppFCManager null handling.The null-safe call and logging are good, but ensure that the
getInAppsCount(context)method can handle the context parameter safely and doesn't throw exceptions that could break header construction.clevertap-core/src/test/java/com/clevertap/android/sdk/network/QueueHeaderBuilderTest.kt (4)
18-46: LGTM: Well-structured test setup with comprehensive mocking.Good use of mockk for relaxed mocking and proper dependency injection setup for the builder instance.
47-55: LGTM: Clear test for caller inclusion.The test properly verifies that the caller parameter is included in the header when provided.
57-65: LGTM: Proper static mocking for debug level.Good use of
mockStaticwith proper resource management usinguseblock to test the debug flag inclusion.
67-74: LGTM: Important edge case for null credentials.This test correctly verifies that the builder returns null when essential configuration is missing, which prevents malformed requests.
clevertap-core/src/main/java/com/clevertap/android/sdk/network/NetworkManager.kt (4)
60-101: Well-designed dependency injection architecture.The constructor properly injects all dependencies including the new encryption components (
NetworkEncryptionManager,IJRepo,ArpRepo,QueueHeaderBuilder), making the class testable and following SOLID principles.
126-181: Solid queue flushing implementation with proper batch processing.The
flushDBQueuemethod correctly handles:
- Batch processing with 50 items per batch
- Proper cursor management for pagination
- Push notification impression handling
- Network error callbacks and batch listeners
🧰 Tools
🪛 detekt (1.23.8)
[warning] 150-150: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
288-290: Good refactoring – header construction now properly delegated.Using
QueueHeaderBuilderfor header construction improves modularity and testability compared to the previous monolithic approach mentioned in past reviews.
350-362: Clean implementation of mute SDK functionality.The
shouldMuteSdkmethod properly handles the mute header with appropriate null safety and trimming, returning early when SDK should be muted.clevertap-core/src/test/java/com/clevertap/android/sdk/network/NetworkManagerTest.kt (5)
160-194: Excellent test coverage for domain change scenarios.The
test_sendQueue_domainChanges_returnFalsetest properly:
- Sets up domain change conditions
- Verifies the method returns false as expected
- Checks that the domain was actually updated
- Cleans up state for other tests
372-431: Comprehensive encryption test coverage.The encryption-related test methods provide excellent coverage:
- Testing successful decryption with proper header
- Testing decryption failure scenarios
- Testing cases where encryption header is absent or false
- Proper verification of
NetworkEncryptionManagerinteractions
269-297: Well-structured test for full response flag behavior.The test correctly verifies that when special events (
APP_LAUNCHED_EVENT,WZRK_FETCH) are present, all response processors receive theisFullResponseflag set to true.
506-563: Good dependency injection in test setup.The
provideNetworkManagermethod properly injects all new dependencies includingIJRepo,ArpRepo,NetworkEncryptionManager, andQueueHeaderBuilder, matching the production constructor signature.
10-11: Good addition of test utilities.The imports of
StorageHelperandTestLoggersupport the enhanced test scenarios for storage manipulation and logging verification.
- fixes deps in network manager class - the api response handing deps are passed in response handling list
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (11)
clevertap-core/src/main/java/com/clevertap/android/sdk/network/NetworkManager.kt (7)
69-77: Thread safety issue with listener collection.The
mNetworkHeadersListenersArrayList is not thread-safe and can causeConcurrentModificationExceptionwhen listeners are added/removed from different threads while being iterated.- private val mNetworkHeadersListeners: MutableList<NetworkHeadersListener> = ArrayList() + private val mNetworkHeadersListeners = CopyOnWriteArrayList<NetworkHeadersListener>()
475-501: Encryption fallback behavior may need configuration option.The current implementation silently falls back to unencrypted requests when encryption fails. For security-sensitive applications, this behavior should be configurable to either fail fast or continue unencrypted.
Consider adding a
strictEncryptionModeconfiguration flag to control this behavior.
614-627: Add logging for decryption failures.When decryption fails, the method returns
falsewithout logging the failure reason, making troubleshooting difficult.is EncryptionFailure -> { + logger.verbose(config.accountId, "Failed to decrypt response: ${decryptResponse.error}") return false }
675-677: Improve JSON exception handling.The JSONException is silently ignored, which could hide important parsing errors that would aid debugging.
} catch (jsonException: JSONException) { - //skip + logger.verbose(config.accountId, "Failed to parse event in queue", jsonException)🧰 Tools
🪛 detekt (1.23.8)
[warning] 675-675: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
703-710: Improve exception handling in push notification processing.Multiple exception types are caught but not properly logged. The
e.printStackTrace()should be replaced with proper logging.} catch (e: JSONException) { - logger.verbose( - config.accountId, - "Encountered an exception while parsing the push notification viewed event queue" - ) + logger.verbose( + config.accountId, + "Exception while parsing push notification viewed event queue", + e + ) } catch (e: Exception) { - e.printStackTrace() + logger.verbose( + config.accountId, + "Unexpected exception while processing push notification queue", + e + )🧰 Tools
🪛 detekt (1.23.8)
[warning] 703-703: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
801-813: Modernize network connectivity check.The current implementation uses deprecated
activeNetworkInfoAPI and the optimistic fallback approach might mask real connectivity issues.Consider updating to use modern
ConnectivityManagerAPIs for Android M+ and implement more conservative error handling.
69-77:⚠️ Potential issue
mNetworkHeadersListenersis not thread-safe – replace withCopyOnWriteArrayList.
addNetworkHeadersListener()/removeNetworkHeadersListener()may be called from the main thread while callbacks are iterated on a worker thread (notifyNetworkHeaderListeners). Using a plainArrayListrisksConcurrentModificationException.- private val mNetworkHeadersListeners: MutableList<NetworkHeadersListener> = ArrayList() + private val mNetworkHeadersListeners = CopyOnWriteArrayList<NetworkHeadersListener>()clevertap-core/src/test/java/com/clevertap/android/sdk/network/NetworkManagerTest.kt (4)
68-71: Add MockK cleanup to prevent cross-test interference.The tearDown method doesn't clear MockK interactions, which can cause test failures due to interaction history carrying over between tests.
@After fun tearDown() { closeable?.close() + clearAllMocks() }
520-524: Replace spyk with relaxed mockk for CleverTapResponse.Using
spyk<CleverTapResponse>()may cause runtime issues ifCleverTapResponsehas abstract/unimplemented methods. A relaxed mock is safer for testing.-for (i in 1..10) { - val spyProcessor = spyk<CleverTapResponse>() - responses.add(spyProcessor) -} +for (i in 1..10) { + val processor = mockk<CleverTapResponse>(relaxed = true) + responses.add(processor) +}
68-71:⚠️ Potential issueReset MockK interactions in
tearDown()to avoid cross-test leakage.The same
networkEncryptionManagermock instance is shared across all tests, yet its recorded interactions are never cleared. Tests that verify call counts (verify(exactly = 0) { … }) can therefore fail or give false-positives depending on previous test execution order.@After fun tearDown() { closeable?.close() + // Ensures mocks don't carry interaction history into the next test run + clearAllMocks() // <-- import io.mockk.clearAllMocks }
520-524: 🛠️ Refactor suggestionReplace
spykwith a relaxedmockkforCleverTapResponse.
spyk<CleverTapResponse>()tries to create a spy of the real class, calling the original (often abstract / open) methods. IfCleverTapResponseis an interface or an abstract class with un-implemented members, the spy will throw at runtime whenprocessResponseis invoked bysendQueue.-for (i in 1..10) { - val spyProcessor = spyk<CleverTapResponse>() - responses.add(spyProcessor) -} +for (i in 1..10) { + val processor = mockk<CleverTapResponse>(relaxed = true) + responses.add(processor) +}
🧹 Nitpick comments (1)
clevertap-core/src/main/java/com/clevertap/android/sdk/CleverTapFactory.kt (1)
308-345: Consider breaking down QueueHeaderBuilder instantiation to improve readability.The QueueHeaderBuilder constructor has grown quite large with 12+ parameters including lambda functions. This impacts readability and maintainability.
Consider extracting the lambda functions into separate methods:
+ private fun createFirstRequestTsProvider() = { + StorageHelper.getIntFromPrefs( + context, + config, + Constants.KEY_FIRST_TS, + 0 + ) + } + + private fun createLastRequestTsProvider() = { + StorageHelper.getIntFromPrefs( + context, + config, + Constants.KEY_LAST_TS, + 0 + ) + } val queueHeaderBuilder = QueueHeaderBuilder( context = context, config = config, coreMetaData = coreMetaData, controllerManager = controllerManager, deviceInfo = deviceInfo, arpRepo = arpRepo, ijRepo = ijRepo, databaseManager = baseDatabaseManager, validationResultStack = validationResultStack, - firstRequestTs = { - StorageHelper.getIntFromPrefs( - context, - config, - Constants.KEY_FIRST_TS, - 0 - ) - }, - lastRequestTs = { - StorageHelper.getIntFromPrefs( - context, - config, - Constants.KEY_LAST_TS, - 0 - ) - }, + firstRequestTs = createFirstRequestTsProvider(), + lastRequestTs = createLastRequestTsProvider(), logger = config.logger )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
clevertap-core/src/main/java/com/clevertap/android/sdk/CleverTapFactory.kt(6 hunks)clevertap-core/src/main/java/com/clevertap/android/sdk/network/NetworkManager.kt(1 hunks)clevertap-core/src/main/java/com/clevertap/android/sdk/response/ARPResponse.java(4 hunks)clevertap-core/src/test/java/com/clevertap/android/sdk/network/NetworkManagerTest.kt(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- clevertap-core/src/main/java/com/clevertap/android/sdk/response/ARPResponse.java
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: CTLalit
PR: CleverTap/clevertap-android-sdk#798
File: clevertap-core/src/main/java/com/clevertap/android/sdk/network/NetworkManager.kt:152-152
Timestamp: 2025-05-30T08:21:16.962Z
Learning: CTLalit prefers to avoid modifying legacy code when working on specific features, keeping the scope focused to reduce risk and maintain clear boundaries for changes.
clevertap-core/src/main/java/com/clevertap/android/sdk/network/NetworkManager.kt (1)
Learnt from: CTLalit
PR: CleverTap/clevertap-android-sdk#798
File: clevertap-core/src/main/java/com/clevertap/android/sdk/network/NetworkManager.kt:558-558
Timestamp: 2025-05-30T08:41:45.328Z
Learning: In the CleverTap Android SDK, debug logging that includes request bodies is controlled by build configuration and only enabled in development builds, not in production. This build-time control already addresses potential sensitive data exposure concerns.
🧬 Code Graph Analysis (1)
clevertap-core/src/test/java/com/clevertap/android/sdk/network/NetworkManagerTest.kt (3)
test_shared/src/main/java/com/clevertap/android/shared/test/BaseTestCase.kt (1)
getSampleJsonArrayOfJsonObjects(58-67)clevertap-core/src/test/java/com/clevertap/android/sdk/network/IJRepoTest.kt (1)
assertFalse(654-656)clevertap-core/src/main/java/com/clevertap/android/sdk/ControllerManager.java (1)
ControllerManager(23-243)
🪛 detekt (1.23.8)
clevertap-core/src/main/java/com/clevertap/android/sdk/network/NetworkManager.kt
[warning] 110-110: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
[warning] 675-675: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
[warning] 703-703: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
🔇 Additional comments (13)
clevertap-core/src/main/java/com/clevertap/android/sdk/CleverTapFactory.kt (6)
112-123: Good architectural improvement with IJRepo integration.The introduction of
IJRepofor managing IJ data and its injection intoDBManagerimproves separation of concerns and follows the repository pattern consistently implemented across the codebase.
130-134: Excellent security architecture improvement.Centralizing cryptographic key management in
CTKeyGeneratorimproves security by providing a dedicated, well-defined interface for key operations. This removes key management logic fromAESGCMCryptand aligns with security best practices.
308-345: NetworkEncryptionManager and QueueHeaderBuilder integration looks solid.The
NetworkEncryptionManagersetup correctly integrates encryption capabilities. WhileQueueHeaderBuilderhas many dependencies, this is necessary for comprehensive header construction and represents good dependency injection practices.
372-384: Well-structured NetworkManager constructor with named parameters.The use of named parameters significantly improves readability and maintainability. The integration of new dependencies (
encryptionManager,arpResponse,queueHeaderBuilder) and expandedcleverTapResponseslist properly supports the new encryption and modularization features.
112-123: LGTM! Clean integration of IJRepo into the dependency chain.The IJRepo is properly instantiated and passed to DBManager, establishing a clear separation of concerns for IJ data management.
348-370:✅ Verification successful
Verify the expanded CleverTapResponse list includes all necessary processors.
The list has grown significantly with many new response types. Ensure that all required response processors are included and properly ordered if order matters.
🏁 Script executed:
#!/bin/bash # Description: Verify all response types are properly implemented and used # Expected: Find all CleverTapResponse implementations and their usage # Search for all CleverTapResponse implementations rg -A 3 "class.*Response.*implements|extends.*CleverTapResponse|: CleverTapResponse" --type kotlin --type java # Search for any missing response processors that might be referenced elsewhere rg "Response\(" --type kotlin --type java | grep -E "(process|handle)"Length of output: 14902
CleverTapResponse list is complete and correctly ordered
All 11 concrete CleverTapResponse implementations are present in the
cleverTapResponseslist:
- InAppResponse
- MetadataResponse
- ARPResponse
- ConsoleResponse
- InboxResponse
- PushAmpResponse
- FetchVariablesResponse
- DisplayUnitResponse
- FeatureFlagResponse
- ProductConfigResponse
- GeofenceResponse
No missing processors or ordering issues detected.
clevertap-core/src/test/java/com/clevertap/android/sdk/network/NetworkManagerTest.kt (4)
364-449: Excellent encryption test coverage.The encryption-related tests comprehensively cover successful decryption, failed decryption, and non-encrypted scenarios. The use of MockK's
every/verifypatterns and exact call count verification demonstrates good testing practices.
152-190: Excellent test coverage for domain change scenarios.This test thoroughly validates the domain change detection and handling logic, including proper state management and cleanup.
264-293: LGTM! Comprehensive test forisFullResponseflag behavior.The test properly validates that response processors receive the correct
isFullResponseflag when App Launched events are present, ensuring proper downstream processing.
368-398: Well-structured encryption test with proper mock verification.The test effectively validates the encryption/decryption flow and verifies that decrypted data is properly passed to response processors.
clevertap-core/src/main/java/com/clevertap/android/sdk/network/NetworkManager.kt (3)
47-61: Constructor parameter count is high but well-organized with dependency injection.The constructor has 10+ parameters which is complex but follows a clear dependency injection pattern. The named parameters make it readable and the dependencies are well-typed.
248-250: LGTM! Clean delegation to QueueHeaderBuilder.The extraction of header building logic into QueueHeaderBuilder improves separation of concerns and testability.
614-627: LGTM! Clean encryption/decryption integration with proper error handling.The encryption header detection and decryption logic is well-implemented with appropriate error handling for decryption failures.
- removes network shared prefs ops and adds them to a new NetworkRepo.kt class - NetworkManager.kt and QueueHeaderBuilder.kt will access data from repo - todo pending in other classes and move constants
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
clevertap-core/src/main/java/com/clevertap/android/sdk/network/NetworkManager.kt (3)
558-568: Consider logging encryption failures for debugging.When decryption fails, the method returns
falsewithout logging the failure reason, which could make troubleshooting difficult.is EncryptionFailure -> { + logger.verbose(config.accountId, "Failed to decrypt response: ${decryptResponse.error}") return false // todo lp check if this should be considered as nw failure? }
69-77:⚠️ Potential issue
mNetworkHeadersListenersis not thread-safe – replace withCopyOnWriteArrayList.
addNetworkHeadersListener()/removeNetworkHeadersListener()may be called from the main thread while callbacks are iterated on a worker thread (notifyNetworkHeaderListeners). Using a plainArrayListrisksConcurrentModificationException.- private val mNetworkHeadersListeners: MutableList<NetworkHeadersListener> = ArrayList() + private val mNetworkHeadersListeners = CopyOnWriteArrayList<NetworkHeadersListener>()
416-442: 🛠️ Refactor suggestionDocument encryption fallback behavior and consider making it configurable.
The implementation silently falls back to unencrypted requests when encryption fails (lines 433-435). This behavior should be documented and potentially made configurable for security-sensitive applications.
Consider adding a configuration flag to control fallback behavior:
private fun sendQueueApi(body: SendQueueRequestBody): Response { if (config.isEncryptionInTransitEnabled && coreMetaData.isRelaxNetwork.not()) { val encryptionResult = encryptionManager.encryptResponse(body.toString()) val sessionEncryptionKey = encryptionManager.sessionEncryptionKey() if (encryptionResult is EncryptionSuccess) { // ... encrypted send logic } else { logger.verbose("Encryption failed, falling back to unencrypted request") if (config.strictEncryptionMode) { throw SecurityException("Encryption required but failed") } } } return ctApiWrapper.ctApi.sendQueue( isViewedEvent = false, body = body.toString(), isEncrypted = false ) }clevertap-core/src/test/java/com/clevertap/android/sdk/network/NetworkManagerTest.kt (2)
68-71: 🛠️ Refactor suggestionReset MockK interactions in
tearDown()to avoid cross-test leakageThe same
networkEncryptionManagermock instance is shared across all tests, yet its recorded interactions are never cleared. Tests that verify call counts (verify(exactly = 0) { … }) can therefore fail or give false-positives depending on previous test execution order.@After fun tearDown() { closeable?.close() + // Ensures mocks don't carry interaction history into the next test run + clearAllMocks() // <-- import io.mockk.clearAllMocks }
514-518: 🛠️ Refactor suggestionReplace
spykwith a relaxedmockkforCleverTapResponse
spyk<CleverTapResponse>()tries to create a spy of the real class, calling the original (often abstract / open) methods. IfCleverTapResponseis an interface or an abstract class with un-implemented members, the spy will throw at runtime whenprocessResponseis invoked bysendQueue.Switching to a relaxed mock avoids this risk while still letting you
verifythe interaction.-for (i in 1..10) { - val spyProcessor = spyk<CleverTapResponse>() - responses.add(spyProcessor) -} +for (i in 1..10) { + val processor = mockk<CleverTapResponse>(relaxed = true) + responses.add(processor) +}
🧹 Nitpick comments (1)
clevertap-core/src/main/java/com/clevertap/android/sdk/network/http/NetworkRepo.kt (1)
81-87: Consider adding a getter method for spiky domain.The class provides
setSpikyDomain()but no correspondinggetSpikyDomain()method, which breaks the typical getter/setter pattern established for other properties.Add the missing getter method:
fun setSpikyDomain(spikyDomainName: String) { StorageHelper.putString( context, StorageHelper.storageKeyWithSuffix(config.accountId, Constants.SPIKY_KEY_DOMAIN_NAME), spikyDomainName ) } +fun getSpikyDomain(): String? { + return StorageHelper.getStringFromPrefs( + context, + config, + Constants.SPIKY_KEY_DOMAIN_NAME, + null + ) +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
clevertap-core/src/main/java/com/clevertap/android/sdk/CleverTapFactory.kt(6 hunks)clevertap-core/src/main/java/com/clevertap/android/sdk/network/NetworkManager.kt(1 hunks)clevertap-core/src/main/java/com/clevertap/android/sdk/network/http/NetworkRepo.kt(1 hunks)clevertap-core/src/test/java/com/clevertap/android/sdk/network/NetworkManagerTest.kt(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- clevertap-core/src/main/java/com/clevertap/android/sdk/CleverTapFactory.kt
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: CTLalit
PR: CleverTap/clevertap-android-sdk#798
File: clevertap-core/src/main/java/com/clevertap/android/sdk/network/NetworkManager.kt:152-152
Timestamp: 2025-05-30T08:21:16.962Z
Learning: CTLalit prefers to avoid modifying legacy code when working on specific features, keeping the scope focused to reduce risk and maintain clear boundaries for changes.
clevertap-core/src/main/java/com/clevertap/android/sdk/network/NetworkManager.kt (1)
Learnt from: CTLalit
PR: CleverTap/clevertap-android-sdk#798
File: clevertap-core/src/main/java/com/clevertap/android/sdk/network/NetworkManager.kt:558-558
Timestamp: 2025-05-30T08:41:45.328Z
Learning: In the CleverTap Android SDK, debug logging that includes request bodies is controlled by build configuration and only enabled in development builds, not in production. This build-time control already addresses potential sensitive data exposure concerns.
🧬 Code Graph Analysis (1)
clevertap-core/src/test/java/com/clevertap/android/sdk/network/NetworkManagerTest.kt (2)
test_shared/src/main/java/com/clevertap/android/shared/test/BaseTestCase.kt (1)
getSampleJsonArrayOfJsonObjects(58-67)clevertap-core/src/main/java/com/clevertap/android/sdk/ControllerManager.java (1)
ControllerManager(23-243)
🪛 detekt (1.23.8)
clevertap-core/src/main/java/com/clevertap/android/sdk/network/NetworkManager.kt
[warning] 110-110: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
[warning] 616-616: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
[warning] 644-644: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
🔇 Additional comments (2)
clevertap-core/src/test/java/com/clevertap/android/sdk/network/NetworkManagerTest.kt (2)
369-398: Excellent test coverage for encryption scenarios.The new encryption-related test methods provide comprehensive coverage of encryption/decryption scenarios including success, failure, and header-based control flow. The test setup properly mocks the
NetworkEncryptionManagerand verifies both the encryption calls and the data flow to response processors.
264-327: Well-structured tests for response processing flags.The tests effectively verify that the
isFullResponseflag is correctly set based on the presence ofAPP_LAUNCHED_EVENTandWZRK_FETCHevents in the queue. The verification of processor interactions ensures the contract betweenNetworkManagerandCleverTapResponseimplementations is maintained.
| fun getFirstRequestTs() : Int { | ||
| return StorageHelper.getIntFromPrefs( | ||
| context, | ||
| config, | ||
| Constants.KEY_FIRST_TS, | ||
| 0 | ||
| ) | ||
| } | ||
| fun setFirstRequestTs(firstRequestTs: Int) { | ||
| StorageHelper.putInt( | ||
| context, | ||
| StorageHelper.storageKeyWithSuffix(config.accountId, Constants.KEY_FIRST_TS), | ||
| firstRequestTs | ||
| ) | ||
| } |
There was a problem hiding this comment.
Fix inconsistent StorageHelper usage pattern.
The getter uses StorageHelper.getIntFromPrefs(context, config, key, default) while the setter uses StorageHelper.putInt(context, StorageHelper.storageKeyWithSuffix(config.accountId, key), value). This inconsistency could lead to reading/writing different preference keys.
Apply this fix to make the patterns consistent:
fun getFirstRequestTs() : Int {
return StorageHelper.getIntFromPrefs(
context,
config,
- Constants.KEY_FIRST_TS,
+ StorageHelper.storageKeyWithSuffix(config.accountId, Constants.KEY_FIRST_TS),
0
)
}Or alternatively, modify the setter to use the same pattern as the getter.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fun getFirstRequestTs() : Int { | |
| return StorageHelper.getIntFromPrefs( | |
| context, | |
| config, | |
| Constants.KEY_FIRST_TS, | |
| 0 | |
| ) | |
| } | |
| fun setFirstRequestTs(firstRequestTs: Int) { | |
| StorageHelper.putInt( | |
| context, | |
| StorageHelper.storageKeyWithSuffix(config.accountId, Constants.KEY_FIRST_TS), | |
| firstRequestTs | |
| ) | |
| } | |
| fun getFirstRequestTs() : Int { | |
| return StorageHelper.getIntFromPrefs( | |
| context, | |
| config, | |
| StorageHelper.storageKeyWithSuffix(config.accountId, Constants.KEY_FIRST_TS), | |
| 0 | |
| ) | |
| } |
🤖 Prompt for AI Agents
In
clevertap-core/src/main/java/com/clevertap/android/sdk/network/http/NetworkRepo.kt
between lines 14 and 28, the getter and setter for firstRequestTs use
inconsistent StorageHelper methods that may access different preference keys. To
fix this, modify the setter to use StorageHelper.putInt with the same key
parameter as the getter, avoiding the use of storageKeyWithSuffix, so both
methods read and write to the same preference key consistently.
| val randomGen = SecureRandom() | ||
| val randomDelay = (randomGen.nextInt(10) + 1) * 1000 | ||
| val delayBy = currentDelay + randomDelay | ||
| if (delayBy < Constants.MAX_DELAY_FREQUENCY) { | ||
| config.logger.debug( | ||
| config.accountId, | ||
| "Setting delay frequency to $currentDelay" | ||
| ) | ||
| return delayBy | ||
| } else { | ||
| return Constants.PUSH_DELAY_MS | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix incorrect fallback delay value.
The logic incorrectly returns Constants.PUSH_DELAY_MS instead of the calculated delayBy when the delay exceeds the maximum threshold. This negates the backoff calculation.
Apply this fix:
if (delayBy < Constants.MAX_DELAY_FREQUENCY) {
config.logger.debug(
config.accountId,
- "Setting delay frequency to $currentDelay"
+ "Setting delay frequency to $delayBy"
)
return delayBy
} else {
- return Constants.PUSH_DELAY_MS
+ return Constants.MAX_DELAY_FREQUENCY
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| val randomGen = SecureRandom() | |
| val randomDelay = (randomGen.nextInt(10) + 1) * 1000 | |
| val delayBy = currentDelay + randomDelay | |
| if (delayBy < Constants.MAX_DELAY_FREQUENCY) { | |
| config.logger.debug( | |
| config.accountId, | |
| "Setting delay frequency to $currentDelay" | |
| ) | |
| return delayBy | |
| } else { | |
| return Constants.PUSH_DELAY_MS | |
| } | |
| } | |
| val randomGen = SecureRandom() | |
| val randomDelay = (randomGen.nextInt(10) + 1) * 1000 | |
| val delayBy = currentDelay + randomDelay | |
| if (delayBy < Constants.MAX_DELAY_FREQUENCY) { | |
| config.logger.debug( | |
| config.accountId, | |
| "Setting delay frequency to $delayBy" | |
| ) | |
| return delayBy | |
| } else { | |
| return Constants.MAX_DELAY_FREQUENCY | |
| } | |
| } |
🤖 Prompt for AI Agents
In
clevertap-core/src/main/java/com/clevertap/android/sdk/network/http/NetworkRepo.kt
between lines 110 and 122, the fallback logic incorrectly returns
Constants.PUSH_DELAY_MS when the calculated delay exceeds the maximum allowed
delay, which bypasses the intended backoff calculation. To fix this, modify the
else branch to return Constants.MAX_DELAY_FREQUENCY instead of
Constants.PUSH_DELAY_MS, ensuring the delay does not exceed the maximum
threshold but still respects the backoff logic.
There was a problem hiding this comment.
fixed this in new commit.
There was a problem hiding this comment.
@CTLalit Thank you for fixing the delay frequency logic! Great to see the fallback now properly returns MAX_DELAY_FREQUENCY and the log message shows the correct delayBy value. This ensures the backoff calculation works as intended.
🐰✨
╭─────╮
│ 🎉 │
╰─────╯
＼(^o^)／
🧠 Learnings used
Learnt from: CTLalit
PR: CleverTap/clevertap-android-sdk#798
File: clevertap-core/src/main/java/com/clevertap/android/sdk/network/NetworkManager.kt:152-152
Timestamp: 2025-05-30T08:21:16.962Z
Learning: CTLalit prefers to avoid modifying legacy code when working on specific features, keeping the scope focused to reduce risk and maintain clear boundaries for changes.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
- adds test cases to network repo class - covers domains, mute, delay frequency
- uses repo instead of direct access to sp - adds missing spiky domain tests
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (14)
clevertap-core/src/main/java/com/clevertap/android/sdk/network/NetworkManager.kt (10)
110-115: Poor exception handling in legacy code section.The exception is caught but not properly logged with exception details, making debugging difficult. However, based on previous feedback, this appears to be legacy code that should remain unchanged to maintain focused scope.
644-651: Inconsistent exception handling in push notification processing.The exception handling here uses different patterns - JSONException gets a generic verbose log while the generic Exception uses
printStackTrace(). This inconsistency makes debugging difficult.
558-561: Missing logging for decryption failures.When decryption fails, the method returns
falsewithout logging the failure reason, making troubleshooting difficult.
616-618: JSONException silently swallowed.The JSONException is caught but only has a
//skipcomment, which hides important parsing errors.
718-730: Deprecated network connectivity API usage.The method uses the deprecated
activeNetworkInfoAPI and optimistically returnstrueon exceptions, which can mask real connectivity issues.
69-77:⚠️ Potential issueThread-safety issue with
mNetworkHeadersListenerscollection.The
ArrayListused formNetworkHeadersListenersis not thread-safe. This can causeConcurrentModificationExceptionwhen listeners are added/removed while being iterated innotifyNetworkHeaderListeners.Replace with thread-safe collection:
- private val mNetworkHeadersListeners: MutableList<NetworkHeadersListener> = ArrayList() + private val mNetworkHeadersListeners = CopyOnWriteArrayList<NetworkHeadersListener>()
416-442: 🛠️ Refactor suggestionSilent fallback to unencrypted requests on encryption failure.
When encryption fails, the method silently falls back to sending unencrypted requests without documenting this behavior or making it configurable. This could be a security concern for applications requiring strict encryption.
Consider adding configuration to control this fallback behavior and document it clearly in the SDK documentation.
69-77: Thread safety issue with NetworkHeadersListenersThe
mNetworkHeadersListenersusesArrayListwhich is not thread-safe. This can causeConcurrentModificationExceptionwhenaddNetworkHeadersListener()/removeNetworkHeadersListener()are called from the main thread whilenotifyNetworkHeaderListenersiterates on a worker thread.
110-115: 🛠️ Refactor suggestionImprove exception handling to preserve stack trace
The exception is caught but only a generic message is logged without the actual exception details, making debugging difficult.
Consider logging the exception details:
} catch (e: Exception) { config.logger.verbose( config.accountId, - "met with exception while notifying listeners for PushImpressionSentToServer event" + "Exception while notifying listeners for PushImpressionSentToServer event", + e ) }
649-651: 🛠️ Refactor suggestionReplace printStackTrace with proper logging
The generic exception handling uses
printStackTrace()which doesn't integrate with the logging infrastructure and may not be captured in production environments.Replace with consistent logging:
} catch (e: Exception) { - e.printStackTrace() + logger.verbose( + config.accountId, + "Unexpected exception while processing push notification queue", + e + )clevertap-core/src/test/java/com/clevertap/android/sdk/network/NetworkManagerTest.kt (4)
514-518: Consider using relaxedmockkinstead ofspykfor CleverTapResponse.Using
spyk<CleverTapResponse>()may cause runtime issues ifCleverTapResponseis an interface or abstract class. A relaxedmockkwould be safer while still allowing verification.
68-71: Missing MockK interaction clearing in tearDown.The tearDown method should clear MockK interactions to prevent cross-test contamination, especially when using
verify(exactly = 0)assertions.Add
clearAllMocks()to prevent interaction history from affecting subsequent tests.
68-71: Add MockK cleanup to prevent cross-test contaminationThe tearDown method handles Mockito cleanup but doesn't clear MockK interactions, which can cause test failures due to interaction history from previous tests.
514-518: Replace spy usage with relaxed mock for CleverTapResponseUsing
spyk<CleverTapResponse>()can cause runtime exceptions ifCleverTapResponseis an interface or abstract class with unimplemented members.
🧹 Nitpick comments (5)
clevertap-core/src/main/java/com/clevertap/android/sdk/network/api/CtApiWrapper.kt (1)
18-18: Fix KDoc parameter description.The KDoc parameter description still refers to "The application context" but the parameter is now
networkRepoof typeNetworkRepo.- * @param networkRepo The application context. + * @param networkRepo The network repository for managing network-related persistent state.clevertap-core/src/test/java/com/clevertap/android/sdk/network/http/NetworkRepoTest.kt (1)
116-148: Good coverage for mute state management.The mute state tests verify both true and false scenarios. However, the tests could be more robust by directly verifying the stored values rather than just checking for successful completion.
Consider adding helper methods or additional assertions to verify the actual stored mute state values:
// Example of more robust verification @Test fun `setMuted with true should store non-zero timestamp`() { // Given networkRepo = NetworkRepo(appCtx, config) val beforeTime = (System.currentTimeMillis() / 1000).toInt() // When networkRepo.setMuted(true) // Then - Verify a non-zero value was stored by checking behavior // Could add a getMutedTimestamp method for direct verification val afterTime = (System.currentTimeMillis() / 1000).toInt() assertTrue("Mute operation should complete successfully", true) }clevertap-core/src/main/java/com/clevertap/android/sdk/network/NetworkRepo.kt (1)
102-134: Review delay calculation logic and logging accuracy.The delay calculation logic is sound, but there's a logging inconsistency on line 127 where it logs
currentDelaybut should logdelayBy(the calculated delay that's actually returned).config.logger.debug( config.accountId, - "Setting delay frequency to $currentDelay" + "Setting delay frequency to $delayBy" )Also, consider adding a log statement when falling back to
PUSH_DELAY_MSdue to exceedingMAX_DELAY_FREQUENCY:} else { + config.logger.debug( + config.accountId, + "Calculated delay $delayBy exceeds max delay, falling back to ${Constants.PUSH_DELAY_MS}" + ) return Constants.PUSH_DELAY_MS }clevertap-core/src/main/java/com/clevertap/android/sdk/network/NetworkManager.kt (2)
557-561: Add logging for decryption failuresWhen decryption fails, the method returns
falsewithout logging the specific failure reason, which could make troubleshooting difficult.Consider adding debug logging:
is EncryptionFailure -> { + logger.verbose(config.accountId, "Failed to decrypt response: ${decryptResponse.error}") return false }
616-617: Improve exception handling for JSON parsingThe JSONException is silently ignored with only a comment, which could hide important parsing errors and make debugging difficult.
Consider adding proper logging:
} catch (jsonException: JSONException) { - //skip + logger.verbose(config.accountId, "Failed to parse event in queue", jsonException)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
clevertap-core/src/main/java/com/clevertap/android/sdk/CleverTapFactory.kt(6 hunks)clevertap-core/src/main/java/com/clevertap/android/sdk/network/NetworkManager.kt(1 hunks)clevertap-core/src/main/java/com/clevertap/android/sdk/network/NetworkRepo.kt(1 hunks)clevertap-core/src/main/java/com/clevertap/android/sdk/network/api/CtApiWrapper.kt(4 hunks)clevertap-core/src/test/java/com/clevertap/android/sdk/network/NetworkManagerTest.kt(6 hunks)clevertap-core/src/test/java/com/clevertap/android/sdk/network/api/CtApiWrapperTest.kt(3 hunks)clevertap-core/src/test/java/com/clevertap/android/sdk/network/http/NetworkRepoTest.kt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- clevertap-core/src/main/java/com/clevertap/android/sdk/CleverTapFactory.kt
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: CTLalit
PR: CleverTap/clevertap-android-sdk#798
File: clevertap-core/src/main/java/com/clevertap/android/sdk/network/NetworkManager.kt:152-152
Timestamp: 2025-05-30T08:21:16.962Z
Learning: CTLalit prefers to avoid modifying legacy code when working on specific features, keeping the scope focused to reduce risk and maintain clear boundaries for changes.
clevertap-core/src/main/java/com/clevertap/android/sdk/network/NetworkManager.kt (1)
Learnt from: CTLalit
PR: CleverTap/clevertap-android-sdk#798
File: clevertap-core/src/main/java/com/clevertap/android/sdk/network/NetworkManager.kt:558-558
Timestamp: 2025-05-30T08:41:45.328Z
Learning: In the CleverTap Android SDK, debug logging that includes request bodies is controlled by build configuration and only enabled in development builds, not in production. This build-time control already addresses potential sensitive data exposure concerns.
🔇 Additional comments (30)
clevertap-core/src/test/java/com/clevertap/android/sdk/network/api/CtApiWrapperTest.kt (2)
6-6: LGTM! Clean refactoring to support NetworkRepo abstraction.The test class has been properly updated to use the new
NetworkRepoabstraction instead of directContextusage. The import, field declaration, and initialization are all correctly implemented.Also applies to: 21-21, 29-30
52-54: Consistent constructor updates across all test methods.All test methods that create
CtApiWrapperinstances have been consistently updated to usenetworkRepoinstead ofapplication. This maintains test consistency across the entire class.clevertap-core/src/main/java/com/clevertap/android/sdk/network/api/CtApiWrapper.kt (2)
23-23: Excellent refactoring to use NetworkRepo abstraction.The constructor and method signatures have been properly updated to use
NetworkRepoinstead ofContext, which provides better separation of concerns and encapsulation of network-related state management.Also applies to: 31-31, 49-49
62-63: Clean abstraction for domain retrieval.The refactoring replaces direct SharedPreferences access with clean
NetworkRepomethod calls for retrieving cached domains. This improves maintainability and testability.clevertap-core/src/test/java/com/clevertap/android/sdk/network/http/NetworkRepoTest.kt (4)
1-27: Excellent test class setup with comprehensive coverage.The test class is well-structured with proper Robolectric setup and realistic CleverTap configuration instantiation. The setup method correctly creates real config instances that will use actual SharedPreferences through Robolectric.
28-115: Thorough testing of timestamp storage and retrieval.The test methods for first and last request timestamps cover both positive cases (storing and retrieving values) and edge cases (default values when no data is stored). The test structure with clear given-when-then sections enhances readability.
266-404: Comprehensive delay frequency calculation testing.The test coverage for
getMinDelayFrequencyis excellent, covering:
- Retry counts below 10
- Null account regions
- Calculated delays with mocked random generation
- Boundary conditions and edge cases
- Default random delay validation
The use of mocked random delay generation allows for deterministic testing of the delay logic.
406-446: Valuable integration and isolation tests.The integration test verifies that different storage operations work independently, and the test with different config instances ensures proper data isolation between different CleverTap accounts.
clevertap-core/src/main/java/com/clevertap/android/sdk/network/NetworkRepo.kt (4)
9-16: Well-designed constructor with dependency injection.The constructor design is excellent, accepting all necessary dependencies and providing a sensible default for the random delay generator using
SecureRandom. The default random delay range of 1-10 seconds is appropriate for network retry scenarios.
51-66: Consistent storage approach for mute state.The mute state methods correctly use the manual
storageKeyWithSuffixapproach consistently in both the true and false branches. This is implemented correctly.
18-32:⚠️ Potential issueInconsistent storage key usage between get and set methods.
The
getFirstRequestTs()method usesStorageHelper.getIntFromPrefs()which internally callsstorageKeyWithSuffix, butsetFirstRequestTs()manually callsStorageHelper.storageKeyWithSuffix(). This inconsistency could lead to different storage keys being used.fun setFirstRequestTs(firstRequestTs: Int) { StorageHelper.putInt( context, - StorageHelper.storageKeyWithSuffix(config.accountId, Constants.KEY_FIRST_TS), + Constants.KEY_FIRST_TS, firstRequestTs ) }Or alternatively, ensure both methods use the same approach consistently.
Likely an incorrect or invalid review comment.
68-83:✅ Verification successful
Domain storage methods need consistency check.
The domain methods may have the same storage key inconsistency issue. Verify that
getDomain()andsetDomain()use compatible storage key generation.
🏁 Script executed:
#!/bin/bash # Description: Check StorageHelper implementation to verify if getIntFromPrefs and getStringFromPrefs internally use storageKeyWithSuffix echo "Checking StorageHelper implementation for key generation consistency..." # Search for StorageHelper methods to understand key handling ast-grep --pattern $'class StorageHelper { $$$ getIntFromPrefs($$$) { $$$ } $$$ }' ast-grep --pattern $'class StorageHelper { $$$ getStringFromPrefs($$$) { $$$ } $$$ }' # Also check for storageKeyWithSuffix usage rg -A 5 -B 2 "storageKeyWithSuffix"Length of output: 67234
No storage key mismatch for domain methods
BothsetDomain()andgetDomain()use the same key format (KEY_DOMAIN_NAME:<accountId>).setDomainexplicitly callsstorageKeyWithSuffix(accountId, KEY_DOMAIN_NAME), andgetDomain’sgetStringFromPrefsinternally appliesstorageKeyWithSuffix(config, rawKey). No changes needed.clevertap-core/src/main/java/com/clevertap/android/sdk/network/NetworkManager.kt (11)
46-61: Constructor complexity is high but may be justified for this central component.The constructor has 12 dependencies which indicates this class has significant responsibilities. While this violates typical dependency injection guidelines, it may be acceptable for a central network manager that coordinates multiple subsystems.
Consider documenting the rationale for this design choice and ensure each dependency serves a distinct purpose.
295-295: Request body logging is appropriately controlled.The debug logging includes the entire request body, but based on the build configuration, this is only enabled in development builds, not production. This appropriately addresses potential sensitive data exposure concerns.
189-191: Excellent refactoring of header construction.The previous large
getQueueHeadermethod has been properly refactored to use theQueueHeaderBuilderpattern, significantly improving maintainability and single responsibility adherence.
274-341: Complex but well-structuredsendQueuemethod.The method is quite long but handles the core network sending logic with proper error handling, response processing, and cleanup. The structure is clear with distinct sections for validation, API calls, and response handling.
592-602: Appropriate error response handling.The method properly handles different HTTP error codes with specific logic for code 419 (relaxing network constraints) and includes proper logging.
555-568: Well-implemented encrypted response handling.The encryption/decryption integration properly checks for the encryption header, handles both success and failure cases, and maintains the response processing flow.
673-699: Proper domain management implementation.The domain setting methods properly update both the repository and API wrapper caches, and include appropriate callback notifications for domain changes.
701-715: Appropriate SDK muting implementation.The muting logic properly clears queues when muted and uses async execution for database operations to avoid blocking the caller thread.
194-222: Handshake implementation looks solidThe handshake logic properly handles success/failure scenarios, domain changes, and mute commands with appropriate error handling and logging.
718-730: Network connectivity check uses deprecated APIThe method uses the deprecated
activeNetworkInfoAPI and returns optimisticallytrueon exceptions, which could mask real connectivity issues.Note: Based on previous feedback preferences to avoid modifying legacy-style code in feature-focused PRs, this may be intentionally left unchanged to maintain scope boundaries.
46-732: Well-architected network layer implementationThe new Kotlin
NetworkManagersuccessfully implements the encryption-in-transit feature with a clean dependency injection pattern. The integration withNetworkEncryptionManager,NetworkRepo, and other new components follows solid architectural principles.Key strengths:
- Clear separation of concerns between encryption, storage, and network operations
- Comprehensive error handling and logging throughout
- Proper integration with the new repository pattern
- Support for different event types and encryption modes
Areas noted for future enhancement:
- Thread safety for listener collections
- Exception logging improvements
- Encryption fallback configurability
clevertap-core/src/test/java/com/clevertap/android/sdk/network/NetworkManagerTest.kt (7)
52-71: Improved test setup with proper mock lifecycle management.The test setup properly manages MockitoAnnotations lifecycle and integrates MockK for the new encryption manager dependency.
16-19: Excellent use of defined constants over hardcoded strings.The tests now use properly defined header constants from
CtApi.Companioninstead of hardcoded strings, improving maintainability and reducing magic strings.Also applies to: 83-84, 91-92
152-475: Comprehensive test coverage for new NetworkManager functionality.The new test cases thoroughly cover:
- Domain change handling
- Mute header processing
- Encryption/decryption scenarios
- Special event detection (App Launched, wzrk_fetch)
- Success and failure paths
The tests are well-structured with clear arrange-act-assert patterns and descriptive names.
368-449: Excellent encryption functionality testing.The encryption test cases thoroughly cover:
- Successful response decryption
- Decryption failure handling
- Missing encryption headers
- False encryption headers
The MockK usage is correct with proper
every/verifypatterns.
521-537: Proper NetworkManager constructor usage with all dependencies.The constructor call correctly provides all required dependencies including the new ones (encryption manager, network repo, queue header builder) with clear named parameters for readability.
368-449: Excellent encryption test coverageThe encryption-related test cases comprehensively cover the new encryption functionality including:
- Successful decryption with proper processor interaction
- Decryption failure scenarios
- Header-based encryption detection logic
- Both enabled and disabled encryption states
The MockK usage is proper and the test assertions verify the correct behavior flows.
152-223: Comprehensive domain and mute handling testsThe test cases thoroughly verify domain change detection and mute header processing functionality with proper setup/teardown of SharedPreferences state. Good coverage of both return values and side effects.
clevertap-core/src/main/java/com/clevertap/android/sdk/network/NetworkRepo.kt
Outdated
Show resolved
Hide resolved
clevertap-core/src/main/java/com/clevertap/android/sdk/network/NetworkRepo.kt
Outdated
Show resolved
Hide resolved
| private fun sendQueueApi(body: SendQueueRequestBody): Response { | ||
| if (config.isEncryptionInTransitEnabled && coreMetaData.isRelaxNetwork.not()) { | ||
| val encryptionResult = encryptionManager.encryptResponse(body.toString()) | ||
| val sessionEncryptionKey = encryptionManager.sessionEncryptionKey() | ||
|
|
||
| if (encryptionResult is EncryptionSuccess) { | ||
| val bodyEnc = EncryptedSendQueueRequestBody( | ||
| encryptedPayload = encryptionResult.data, | ||
| key = sessionEncryptionKey, | ||
| iv = encryptionResult.iv | ||
| ).toJsonString() | ||
| logger.verbose("Encrypted Request = $bodyEnc") | ||
| return ctApiWrapper.ctApi.sendQueue( | ||
| isViewedEvent = false, | ||
| body = bodyEnc, | ||
| isEncrypted = true | ||
| ) | ||
| } else { | ||
| logger.verbose("Normal Request cause encryption failed = $body") | ||
| } | ||
| } | ||
| return ctApiWrapper.ctApi.sendQueue( | ||
| isViewedEvent = false, | ||
| body = body.toString(), | ||
| isEncrypted = false | ||
| ) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Document encryption fallback behavior and consider configurability
The method silently falls back to unencrypted requests when encryption fails (lines 434-435). This behavior should be documented and potentially made configurable for security-sensitive applications.
Consider adding configuration to control fallback behavior and ensure it's documented:
private fun sendQueueApi(body: SendQueueRequestBody): Response {
if (config.isEncryptionInTransitEnabled && coreMetaData.isRelaxNetwork.not()) {
val encryptionResult = encryptionManager.encryptResponse(body.toString())
if (encryptionResult is EncryptionSuccess) {
// ... encrypted send logic
} else {
logger.verbose(config.accountId, "Encryption failed, falling back to unencrypted request")
// Consider: if (config.strictEncryptionMode) throw SecurityException("Encryption required but failed")
}
}
return ctApiWrapper.ctApi.sendQueue(/* ... */)
}🤖 Prompt for AI Agents
In
clevertap-core/src/main/java/com/clevertap/android/sdk/network/NetworkManager.kt
between lines 416 and 442, the sendQueueApi method silently falls back to
sending unencrypted requests if encryption fails. To fix this, add documentation
explaining this fallback behavior and introduce a configuration option (e.g.,
strictEncryptionMode) to control whether to allow fallback or throw an exception
on encryption failure. Update the code to check this config and either proceed
with unencrypted requests or raise a SecurityException accordingly, and log the
fallback event with relevant details.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
clevertap-core/src/test/java/com/clevertap/android/sdk/network/NetworkManagerTest.kt (2)
67-71: Reset MockK interactions intearDown()to avoid cross-test leakageThe same
networkEncryptionManagermock instance is shared across all tests, yet its recorded interactions are never cleared.
Tests that verify call counts (verify(exactly = 0) { … }) can therefore fail or give false-positives depending on previous test execution order.
541-545: Replacespykwith a relaxedmockkforCleverTapResponse
spyk<CleverTapResponse>()tries to create a spy of the real class, calling the original (often abstract / open) methods.
IfCleverTapResponseis an interface or an abstract class with un-implemented members, the spy will throw at runtime whenprocessResponseis invoked bysendQueue.Switching to a relaxed mock avoids this risk while still letting you
verifythe interaction.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
clevertap-core/src/test/java/com/clevertap/android/sdk/network/NetworkManagerTest.kt(6 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: CTLalit
PR: CleverTap/clevertap-android-sdk#798
File: clevertap-core/src/main/java/com/clevertap/android/sdk/network/NetworkManager.kt:152-152
Timestamp: 2025-05-30T08:21:16.962Z
Learning: CTLalit prefers to avoid modifying legacy code when working on specific features, keeping the scope focused to reduce risk and maintain clear boundaries for changes.
🧬 Code Graph Analysis (1)
clevertap-core/src/test/java/com/clevertap/android/sdk/network/NetworkManagerTest.kt (3)
test_shared/src/main/java/com/clevertap/android/shared/test/BaseTestCase.kt (1)
getSampleJsonArrayOfJsonObjects(58-67)clevertap-core/src/test/java/com/clevertap/android/sdk/network/IJRepoTest.kt (1)
assertFalse(654-656)clevertap-core/src/main/java/com/clevertap/android/sdk/ControllerManager.java (1)
ControllerManager(23-243)
🔇 Additional comments (2)
clevertap-core/src/test/java/com/clevertap/android/sdk/network/NetworkManagerTest.kt (2)
367-473: Excellent encryption test coverage!The encryption-related tests comprehensively cover all scenarios:
- Successful decryption
- Failed decryption
- Missing encryption header
- False encryption header
The tests properly verify mock interactions and assert expected behavior.
475-501: Good coverage of specific HTTP error codesThe tests for 402 (Payment Required) and 419 (Authentication Timeout) properly verify that sendQueue returns false for these error conditions.
clevertap-core/src/test/java/com/clevertap/android/sdk/network/NetworkManagerTest.kt
Outdated
Show resolved
Hide resolved
… network repo prefs - fixes prefs since the get the set methods are misleading
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
clevertap-core/src/test/java/com/clevertap/android/sdk/network/NetworkManagerTest.kt (3)
67-70: Reset MockK interactions intearDown()to avoid cross-test leakage.The same
networkEncryptionManagermock instance is shared across all tests, yet its recorded interactions are never cleared. Tests that verify call counts (verify(exactly = 0) { … }) can therefore fail or give false-positives depending on previous test execution order.@After fun tearDown() { closeable?.close() + // Ensures mocks don't carry interaction history into the next test run + clearAllMocks() // <-- import io.mockk.clearAllMocks }
184-188: Fix incorrect storage key construction.The storage key construction is inconsistent with line 167. It should use the full config object, not just the accountId.
Apply this diff to fix the storage key:
- StorageHelper.putString( - appCtx, - StorageHelper.storageKeyWithSuffix(cleverTapInstanceConfig.accountId, NetworkRepo.KEY_DOMAIN_NAME), - originalDomain - ) + StorageHelper.putString( + appCtx, + StorageHelper.storageKeyWithSuffix(cleverTapInstanceConfig, NetworkRepo.KEY_DOMAIN_NAME), + originalDomain + )
541-545: Replacespykwith a relaxedmockkforCleverTapResponse.
spyk<CleverTapResponse>()tries to create a spy of the real class, calling the original (often abstract / open) methods. IfCleverTapResponseis an interface or an abstract class with un-implemented members, the spy will throw at runtime whenprocessResponseis invoked bysendQueue.Switching to a relaxed mock avoids this risk while still letting you
verifythe interaction.-for (i in 1..10) { - val spyProcessor = spyk<CleverTapResponse>() - responses.add(spyProcessor) -} +for (i in 1..10) { + val processor = mockk<CleverTapResponse>(relaxed = true) + responses.add(processor) +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
clevertap-core/src/main/java/com/clevertap/android/sdk/Constants.java(0 hunks)clevertap-core/src/main/java/com/clevertap/android/sdk/network/NetworkRepo.kt(1 hunks)clevertap-core/src/test/java/com/clevertap/android/sdk/network/NetworkManagerTest.kt(6 hunks)clevertap-core/src/test/java/com/clevertap/android/sdk/network/http/NetworkRepoTest.kt(1 hunks)
💤 Files with no reviewable changes (1)
- clevertap-core/src/main/java/com/clevertap/android/sdk/Constants.java
🚧 Files skipped from review as they are similar to previous changes (1)
- clevertap-core/src/main/java/com/clevertap/android/sdk/network/NetworkRepo.kt
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: CTLalit
PR: CleverTap/clevertap-android-sdk#798
File: clevertap-core/src/main/java/com/clevertap/android/sdk/network/NetworkManager.kt:152-152
Timestamp: 2025-05-30T08:21:16.962Z
Learning: CTLalit prefers to avoid modifying legacy code when working on specific features, keeping the scope focused to reduce risk and maintain clear boundaries for changes.
🔇 Additional comments (6)
clevertap-core/src/test/java/com/clevertap/android/sdk/network/http/NetworkRepoTest.kt (2)
413-433: Good coverage of randomness testing.The test effectively verifies that the default random delay generator produces varied results within the expected range. The approach of testing multiple calls and checking for uniqueness is a solid way to validate randomness behavior.
461-475: Excellent test for config isolation.This test correctly verifies that different CleverTap instance configurations maintain separate storage, which is crucial for multi-account scenarios. The test design and assertions are appropriate.
clevertap-core/src/test/java/com/clevertap/android/sdk/network/NetworkManagerTest.kt (4)
264-292: Good test coverage for App Launched event handling.The test correctly verifies that when an App Launched event is present in the queue, all response processors receive the
isFullResponse=trueflag. The test structure and assertions are appropriate for this scenario.
368-397: Comprehensive encryption testing with proper mocking.This test effectively verifies the encryption flow by mocking the
NetworkEncryptionManagerto return successful decryption and then validating that:
- Decryption is called when the encryption header is present
- The decrypted response is passed to processors
- The method returns true for successful processing
403-425: Good negative test case for encryption failure.This test correctly handles the scenario where decryption fails, ensuring that
sendQueuereturns false and that the decryption attempt was made. This is important for robust error handling.
476-501: Good coverage of specific HTTP error codes.The tests for HTTP 402 and 419 error codes are valuable additions that ensure proper handling of these specific error scenarios. The test structure is consistent and the assertions are correct.
| @Test | ||
| fun `setMuted with true should store current timestamp`() { | ||
| // Given | ||
| networkRepo = NetworkRepo(appCtx, config) | ||
| val beforeTime = (System.currentTimeMillis() / 1000).toInt() | ||
|
|
||
| // When | ||
| networkRepo.setMuted(true) | ||
| val afterTime = (System.currentTimeMillis() / 1000).toInt() | ||
|
|
||
| // Then - Verify by checking that a non-zero timestamp was stored | ||
| // We can't directly access the muted value, but we can verify the behavior | ||
| // by calling setMuted(false) and then setMuted(true) again | ||
| networkRepo.setMuted(false) | ||
| networkRepo.setMuted(true) | ||
|
|
||
| // The test passes if no exception is thrown and the method completes successfully | ||
| assertTrue("setMuted(true) should complete without error", true) | ||
| } | ||
|
|
||
| @Test | ||
| fun `setMuted with false should store zero`() { | ||
| // Given | ||
| networkRepo = NetworkRepo(appCtx, config) | ||
|
|
||
| // When | ||
| networkRepo.setMuted(false) | ||
|
|
||
| // Then - The test passes if no exception is thrown | ||
| assertTrue("setMuted(false) should complete without error", true) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve mute state testing to verify actual behavior.
The current mute tests don't verify the actual muted state or timestamp values. They only check that the methods complete without exceptions, which provides limited value.
Consider enhancing the tests to verify the actual stored values:
@Test
fun `setMuted with true should store current timestamp`() {
// Given
networkRepo = NetworkRepo(appCtx, config)
val beforeTime = (System.currentTimeMillis() / 1000).toInt()
// When
networkRepo.setMuted(true)
val afterTime = (System.currentTimeMillis() / 1000).toInt()
// Then - Verify by checking that a non-zero timestamp was stored
- // We can't directly access the muted value, but we can verify the behavior
- // by calling setMuted(false) and then setMuted(true) again
- networkRepo.setMuted(false)
- networkRepo.setMuted(true)
-
- // The test passes if no exception is thrown and the method completes successfully
- assertTrue("setMuted(true) should complete without error", true)
+ // Verify the muted timestamp is stored in the expected range
+ val storedMutedTime = StorageHelper.getIntFromPrefs(
+ appCtx,
+ config,
+ Constants.KEY_MUTED,
+ 0
+ )
+ assertTrue("Muted timestamp should be between beforeTime and afterTime",
+ storedMutedTime >= beforeTime && storedMutedTime <= afterTime)
}
@Test
fun `setMuted with false should store zero`() {
// Given
networkRepo = NetworkRepo(appCtx, config)
+ // First set muted to true
+ networkRepo.setMuted(true)
// When
networkRepo.setMuted(false)
- // Then - The test passes if no exception is thrown
- assertTrue("setMuted(false) should complete without error", true)
+ // Then - Verify muted timestamp is cleared
+ val storedMutedTime = StorageHelper.getIntFromPrefs(
+ appCtx,
+ config,
+ Constants.KEY_MUTED,
+ 0
+ )
+ assertEquals("setMuted(false) should store 0", 0, storedMutedTime)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @Test | |
| fun `setMuted with true should store current timestamp`() { | |
| // Given | |
| networkRepo = NetworkRepo(appCtx, config) | |
| val beforeTime = (System.currentTimeMillis() / 1000).toInt() | |
| // When | |
| networkRepo.setMuted(true) | |
| val afterTime = (System.currentTimeMillis() / 1000).toInt() | |
| // Then - Verify by checking that a non-zero timestamp was stored | |
| // We can't directly access the muted value, but we can verify the behavior | |
| // by calling setMuted(false) and then setMuted(true) again | |
| networkRepo.setMuted(false) | |
| networkRepo.setMuted(true) | |
| // The test passes if no exception is thrown and the method completes successfully | |
| assertTrue("setMuted(true) should complete without error", true) | |
| } | |
| @Test | |
| fun `setMuted with false should store zero`() { | |
| // Given | |
| networkRepo = NetworkRepo(appCtx, config) | |
| // When | |
| networkRepo.setMuted(false) | |
| // Then - The test passes if no exception is thrown | |
| assertTrue("setMuted(false) should complete without error", true) | |
| } | |
| @Test | |
| fun `setMuted with true should store current timestamp`() { | |
| // Given | |
| networkRepo = NetworkRepo(appCtx, config) | |
| val beforeTime = (System.currentTimeMillis() / 1000).toInt() | |
| // When | |
| networkRepo.setMuted(true) | |
| val afterTime = (System.currentTimeMillis() / 1000).toInt() | |
| // Then - Verify by checking that a non-zero timestamp was stored | |
| // Verify the muted timestamp is stored in the expected range | |
| val storedMutedTime = StorageHelper.getIntFromPrefs( | |
| appCtx, | |
| config, | |
| Constants.KEY_MUTED, | |
| 0 | |
| ) | |
| assertTrue( | |
| "Muted timestamp should be between beforeTime and afterTime", | |
| storedMutedTime >= beforeTime && storedMutedTime <= afterTime | |
| ) | |
| } | |
| @Test | |
| fun `setMuted with false should store zero`() { | |
| // Given | |
| networkRepo = NetworkRepo(appCtx, config) | |
| // First set muted to true | |
| networkRepo.setMuted(true) | |
| // When | |
| networkRepo.setMuted(false) | |
| // Then - Verify muted timestamp is cleared | |
| val storedMutedTime = StorageHelper.getIntFromPrefs( | |
| appCtx, | |
| config, | |
| Constants.KEY_MUTED, | |
| 0 | |
| ) | |
| assertEquals("setMuted(false) should store 0", 0, storedMutedTime) | |
| } |
🤖 Prompt for AI Agents
In
clevertap-core/src/test/java/com/clevertap/android/sdk/network/http/NetworkRepoTest.kt
between lines 146 and 176, the tests for setMuted(true) and setMuted(false) only
check for method completion without errors but do not verify the actual muted
state or timestamp values. To fix this, enhance the tests by accessing the
stored muted state or timestamp after calling setMuted and assert that the
timestamp is correctly set to a non-zero value when muted is true and zero when
muted is false. This will ensure the tests validate the actual behavior rather
than just method execution.
feat(SDK-4709): adds coverage for CleverTapInstanceConfig - uses string numeric values similar to other flags in the manifest. - adds unit test coverage for CleverTapInstanceConfig
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
clevertap-core/src/test/java/com/clevertap/android/sdk/CleverTapFixtures.kt (1)
9-35: Consider simplifying the documentation for better readability.The documentation is comprehensive but quite verbose. Consider condensing it while retaining essential information.
/** * Creates a ManifestInfo instance with default parameters for testing. - * - * @param accountId Account ID (default: Constant.ACC_ID) - * @param accountToken Account token (default: Constant.ACC_TOKEN) - * @param accountRegion Account region (default: null) + * + * @param accountId Account ID (default: Constant.ACC_ID) + * @param accountToken Account token (default: Constant.ACC_TOKEN) + * @param accountRegion Account region (default: null) * @param proxyDomain Proxy domain (default: "") * @param spikyProxyDomain Spiky proxy domain (default: "") - * @param handshakeDomain Handshake domain (default: "") - * @param useADID Use Google Ad ID (default: false) - * @param appLaunchedDisabled App launched disabled (default: false) - * @param notificationIcon Notification icon (default: "notification icon") - * @param excludedActivitiesForInApps Excluded activities for in-apps (default: null) - * @param sslPinning SSL pinning enabled (default: false) - * @param backgroundSync Background sync enabled (default: false) - * @param useCustomID Use custom ID (default: false) - * @param fcmSenderId FCM sender ID (default: "fcm:sender:id") - * @param packageName Package name (default: "some.app.package") - * @param beta Beta mode enabled (default: false) - * @param intentServiceName Intent service name (default: "serviceName") - * @param devDefaultPushChannelId Default push channel ID (default: "push-channel-id") - * @param profileKeys Profile keys array (default: emptyArray()) - * @param encryptionLevel Encryption level (default: 0) - * @param provider1 Provider 1 configuration (default: HMS configuration) - * @param provider2 Provider 2 configuration (default: null) - * @param encryptionInTransit Encryption in transit (default: "0") + * @param encryptionInTransit Encryption in transit flag (default: "0") */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
clevertap-core/src/main/java/com/clevertap/android/sdk/CleverTapInstanceConfig.java(9 hunks)clevertap-core/src/main/java/com/clevertap/android/sdk/ManifestInfo.java(6 hunks)clevertap-core/src/test/java/com/clevertap/android/sdk/CleverTapFixtures.kt(1 hunks)clevertap-core/src/test/java/com/clevertap/android/sdk/CleverTapInstanceConfigTest.kt(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- clevertap-core/src/main/java/com/clevertap/android/sdk/ManifestInfo.java
- clevertap-core/src/main/java/com/clevertap/android/sdk/CleverTapInstanceConfig.java
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: CTLalit
PR: CleverTap/clevertap-android-sdk#798
File: clevertap-core/src/main/java/com/clevertap/android/sdk/network/NetworkManager.kt:152-152
Timestamp: 2025-05-30T08:21:16.962Z
Learning: CTLalit prefers to avoid modifying legacy code when working on specific features, keeping the scope focused to reduce risk and maintain clear boundaries for changes.
🔇 Additional comments (7)
clevertap-core/src/test/java/com/clevertap/android/sdk/CleverTapFixtures.kt (2)
36-86: Well-designed factory method for test fixtures.The factory method provides excellent flexibility for testing by allowing selective parameter overrides while maintaining sensible defaults. The inclusion of the new
encryptionInTransitparameter with a default value of "0" aligns well with the encryption-in-transit feature introduction.
88-88: Good backward compatibility maintenance.The update preserves the existing API while leveraging the new factory method, ensuring that existing tests continue to work without modification.
clevertap-core/src/test/java/com/clevertap/android/sdk/CleverTapInstanceConfigTest.kt (5)
8-9: LGTM! Appropriate test imports added.The addition of
assertFalseandassertTrueimports is necessary for the new encryption-in-transit test methods.
73-84: Well-implemented test for encryption enabled scenario.The test correctly verifies that when
encryptionInTransitis set to "1", theisEncryptionInTransitEnabledproperty returnstrue. Good use of descriptive test naming and clear assertion.
86-96: Good test coverage for encryption disabled scenario.The test properly validates that when
encryptionInTransitis set to "0", the encryption feature is disabled. This ensures the boolean conversion logic works correctly for the "false" case.
98-108: Excellent defensive testing for invalid input.This test ensures robust behavior by verifying that invalid string values for
encryptionInTransitdefault to disabled state. This is important for preventing unexpected behavior with malformed configuration values.
110-155: Comprehensive serialization test with good property coverage.This test thoroughly validates that all CleverTapInstanceConfig properties, including the new
encryptionInTransitfield, are preserved through JSON serialization/deserialization cycles. The test covers all boolean flags and key properties, which is essential for ensuring configuration persistence works correctly.The TODO comment on lines 151-152 indicates a known issue with identity keys serialization that's outside the scope of this encryption feature.
- adds test cases to check headers - breaks down sendqueue to 2 sdk methods
- robust network testing
https://wizrocket.atlassian.net/browse/SDK-4341
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Tests
Chores
Style