Java SDK: All tests passing (294/294 unit, 138/138 cross-SDK)#11
Java SDK: All tests passing (294/294 unit, 138/138 cross-SDK)#11
Conversation
- Add getCustomFieldValue method for accessing custom field values - Fix type coercion for custom field values (json, number, boolean types)
The attrsSeq assignment was happening during a read operation but mutating shared state. This could cause race conditions with concurrent access. The attrsSeq is already properly set in the write path when assignments are created.
- Add HTTP client error scenario tests (timeout, rate limiting, SSL, retries) - Add concurrency and thread safety tests - Add configuration validation tests - Add edge case type conversion tests - Add data provider integration tests Total: ~29 new tests added, all 265 tests pass
Add 29 context tests covering treatment exposure queuing, variable values, peek, refresh, publish, close, and track. Upgrade build infrastructure: Gradle 5.6→8.5, Mockito 3→5, JaCoCo 0.8.6→0.8.12 for Java 21 compatibility. Fix ABSmartly.java case-sensitivity issue and JaCoCo exclusions.
- Fix deadlock in Context.flush() (read lock upgrade to write lock) - Fix AudienceMatcher.evaluate() crash on empty/null audience strings - Fix deserializers to return null instead of throwing IllegalStateException - Fix double error logging in closeAsync() - Add JsonMapperUtils for consistent ObjectMapper configuration - Fix scheduler shutdown, Client HTTPS handling, MatchOperator patterns
WalkthroughThe PR renames public API classes from ABSmartly/ABSmartlyConfig to ABsmartly/ABsmartlyConfig and adds deprecated wrapper classes for backwards compatibility. It introduces a Builder for ABsmartly, refactors Context for safer concurrency and readiness handling, centralises Jackson mapper creation via JsonMapperUtils, adds regex matching timeouts/limits, tightens Client endpoint and response handling, updates Gradle and tooling versions, and expands README and many unit tests and platform examples. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
- Fix InOperator: args are [needle, haystack] not [haystack, needle] - Fix EqualsOperator: handle null==null comparison (returns true) - Update unit tests to match corrected argument ordering
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
README.md (1)
7-7:⚠️ Potential issue | 🟡 MinorJava 1.6 compatibility claim conflicts with code examples using Java 9+ APIs.
Line 7 states the SDK is "compatible with Java versions 1.6 and later", yet multiple documentation examples use
Map.of()(lines 235, 284, 299, 309, 540), which is a Java 9+ API. Either update the compatibility statement to reflect the actual minimum version, or replaceMap.of()calls in examples withHashMap-based alternatives that work on Java 1.6+.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` at line 7, The README claims "compatible with Java versions 1.6 and later" but several examples use the Java 9+ API Map.of(); either update the README's compatibility statement to the correct minimum Java version or change the examples that call Map.of(...) to Java 1.6-compatible constructions (e.g., create a new HashMap, put the key/value pairs, and optionally wrap with Collections.unmodifiableMap where immutability is needed); update every Map.of occurrence referenced in the doc to use the HashMap-based alternative if you choose to keep 1.6 compatibility.core-api/src/main/java/com/absmartly/sdk/ABsmartly.java (1)
73-78:⚠️ Potential issue | 🔴 CriticalThe README documentation contains inaccurate examples that will fail to compile.
The README (lines 60–69) documents
ABsmartly.builder()as the recommended initialisation approach with method chaining (.endpoint(),.apiKey(), etc.). However, theABsmartlyclass only exposes a staticcreate(ABsmartlyConfig config)method; nobuilder()method exists. The only correct initialisation approach is shown in the "Advanced Configuration" section usingABsmartlyConfig.create()andClient.create().Update the README to remove the builder pattern examples or implement the missing
builder()method if intended.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core-api/src/main/java/com/absmartly/sdk/ABsmartly.java` around lines 73 - 78, The README shows a fluent ABsmartly.builder() API but the ABsmartly class only exposes create(ABsmartlyConfig), so implement a static builder() on ABsmartly that returns a nested Builder (e.g., ABsmartly.Builder) which exposes fluent setters like endpoint(String), apiKey(String), and any other documented chainable methods, accumulates values into an ABsmartlyConfig, and has build() (or create()) that constructs the config and calls ABsmartly.create(config); ensure the Builder maps each fluent setter to the corresponding ABsmartlyConfig field and that ABsmartly.create(ABsmartlyConfig) remains the single creation path.
🧹 Nitpick comments (25)
core-api/src/main/java/com/absmartly/sdk/java/time/Clock.java (1)
17-17: Considerprivatevisibility and conventionalstatic finalnaming.
utc_is package-private, so any class in the same package can access the rawSystemClockUTCinstance directly, bypassingsystemUTC(). Making itprivatepreserves encapsulation. Additionally, Java conventions namestatic finalconstants inUPPER_SNAKE_CASE.♻️ Proposed refactor
- static final SystemClockUTC utc_ = new SystemClockUTC(); + private static final SystemClockUTC UTC = new SystemClockUTC();And update the reference in
systemUTC():static public Clock systemUTC() { - return utc_; + return UTC; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core-api/src/main/java/com/absmartly/sdk/java/time/Clock.java` at line 17, The field utc_ is package-private and not following constant naming conventions; change the declaration of utc_ (SystemClockUTC) to be private and rename it to UCT_SYSTEM_CLOCK or SYSTEM_CLOCK_UTC (consistent with project convention) and update any references, especially the systemUTC() method, to use the new private constant name so callers must go through systemUTC() rather than accessing the raw instance directly.core-api/src/main/java/com/absmartly/sdk/internal/Algorithm.java (1)
7-7:@SuppressWarnings("unchecked")scope could be narrowed (optional)The annotation is applied at method level, but the only unchecked operation is the
(R[])cast on line 11. Java allows@SuppressWarningson local variable declarations, so extracting the cast into a helper variable would restrict the suppression to exactly that one expression:♻️ Proposed narrowing refactor
public static <T, R> R[] mapSetToArray(Set<T> set, R[] array, Function<T, R> mapper) { final int size = set.size(); if (array.length < size) { - array = (R[]) java.lang.reflect.Array.newInstance(array.getClass().getComponentType(), size); + `@SuppressWarnings`("unchecked") + final R[] resized = (R[]) java.lang.reflect.Array.newInstance(array.getClass().getComponentType(), size); + array = resized; }The cast itself is safe by construction —
array.getClass().getComponentType()captures the exact runtime component type — so suppressing the warning is correct either way. This is purely a hygiene suggestion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core-api/src/main/java/com/absmartly/sdk/internal/Algorithm.java` at line 7, The class-level/method-level `@SuppressWarnings`("unchecked") should be narrowed: locate the Algorithm class method that currently has `@SuppressWarnings`("unchecked") and extract the (R[]) cast into a local helper variable (e.g., R[] result = (R[]) Array.newInstance(array.getClass().getComponentType(), array.length);) and place the `@SuppressWarnings`("unchecked") directly on that local variable declaration (or immediately around that single cast), then remove the method-level suppression so only the exact cast expression is suppressed.gradle/jacoco.gradle (1)
5-5: Consider pinning to the latest JaCoCo release (0.8.14).Version 0.8.12 is a valid published release (April 2024), but a newer version — 0.8.14 — is already available. Staying on 0.8.12 is not a blocker, but upgrading would bring in the latest bug fixes and Java-version support improvements.
♻️ Suggested version bump
jacoco { - toolVersion = "0.8.12" + toolVersion = "0.8.14" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gradle/jacoco.gradle` at line 5, Update the JaCoCo toolVersion value from "0.8.12" to the newer release "0.8.14" by changing the toolVersion assignment (symbol: toolVersion) in the jacoco Gradle configuration, then run a build/coverage task to verify compatibility and that no Gradle warnings or failures occur after the bump.core-api/src/test/java/com/absmartly/sdk/internal/ConcurrencyTest.java (2)
183-188:ExecutorServicenot fully shut down — considerawaitTerminationand try-finally.Each test calls
executor.shutdown()but neverawaitTermination(). While thedoneLatch.await()ensures tasks have logically completed, the executor's internal threads may still be running briefly. Using try-finally withawaitTerminationis a cleaner pattern and avoids potential thread leaks across tests.Example cleanup pattern
startLatch.countDown(); doneLatch.await(); executor.shutdown(); + assertTrue(executor.awaitTermination(5, TimeUnit.SECONDS), "Executor did not terminate in time");Also applies to: 219-224, 257-263, 302-308, 338-343
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core-api/src/test/java/com/absmartly/sdk/internal/ConcurrencyTest.java` around lines 183 - 188, The tests currently call executor.shutdown() (e.g., in ConcurrencyTest after startLatch.countDown() / doneLatch.await()) but do not await termination or guarantee shutdown on failure; wrap executor lifecycle in a try-finally, call executor.shutdown() in finally, then call executor.awaitTermination with a reasonable timeout and fall back to executor.shutdownNow() if it times out to avoid thread leaks; apply this pattern to the executor usage around the blocks referencing executor, doneLatch, startLatch and the assertions that use threadCount, operationsPerThread, and successCount (also update the other similar test blocks at the locations mentioned).
149-188: UsingConcurrentHashMap(andsynchronizedList) undermines what these concurrency tests validate.All the new concurrent tests use
ConcurrentHashMaporCollections.synchronizedListas the underlying collection. These are inherently thread-safe, so the tests pass even if theConcurrency.getRW/putRW/addRWlocking is completely removed. This means the tests don't actually verify that theReentrantReadWriteLockinConcurrencyprovides the necessary synchronisation.Switching to a plain
HashMap(orArrayList) would let the test genuinely prove thatConcurrency.*RWmethods are required for correctness under contention. This applies to all five new test methods (testConcurrentGetOperations,testConcurrentPutOperations,testConcurrentComputeIfAbsent,testConcurrentReadWriteMixed,testThreadSafeListOperations).Example for this test
- final Map<String, Integer> map = new ConcurrentHashMap<>(); + final Map<String, Integer> map = new HashMap<>();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core-api/src/test/java/com/absmartly/sdk/internal/ConcurrencyTest.java` around lines 149 - 188, The tests currently use thread-safe collections (ConcurrentHashMap / Collections.synchronizedList) so they succeed even if Concurrency's locking is removed; replace those with non-thread-safe collections so the Concurrency.*RW helpers are actually required: in testConcurrentGetOperations (and the other four tests testConcurrentPutOperations, testConcurrentComputeIfAbsent, testConcurrentReadWriteMixed, testThreadSafeListOperations) change the backing map/list instantiations to plain HashMap / ArrayList (e.g., Map<String,Integer> map = new HashMap<>(); List<T> list = new ArrayList<>()) so that calls to Concurrency.getRW, putRW, computeIfAbsent, addRW actually provide the synchronization under contention and the assertions validate the locking behavior.core-api/src/main/java/com/absmartly/sdk/jsonexpr/ExprEvaluator.java (1)
101-105: Appropriate exception refinement inextractVar.Catching
NumberFormatException(fromInteger.parseInt) andIndexOutOfBoundsException(fromList.get) is precise and correct. However, note that the two separatecatchclauses on a single line reduce readability.🔧 Consider multi-catch syntax for clarity (Java 7+)
- } catch (NumberFormatException ignored) {} catch (IndexOutOfBoundsException ignored) {} + } catch (NumberFormatException | IndexOutOfBoundsException ignored) {}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core-api/src/main/java/com/absmartly/sdk/jsonexpr/ExprEvaluator.java` around lines 101 - 105, In extractVar (class ExprEvaluator) the two exception catches for Integer.parseInt and List.get are on a single line; refactor to use Java 7+ multi-catch (catch (NumberFormatException | IndexOutOfBoundsException e)) or separate multi-line catch for readability, and ensure the caught exception variable is either used or named e/ignored consistently to avoid compiler warnings and improve clarity.core-api/src/test/java/com/absmartly/sdk/jsonexpr/ExprEvaluatorTest.java (1)
414-429:stringConvertspecial-number tests areDecimalFormat-dependent.The expected strings
"NaN"and"\u221E"/"-\u221E"are produced byDecimalFormat.format(), which uses the JVM's default locale symbols. On a JVM with a non-standardDecimalFormatSymbolslocale (e.g., set via-Duser.language), the infinity symbol or NaN representation could theoretically differ. This is unlikely to be an issue in practice but worth being aware of if tests run in varied locale environments.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core-api/src/test/java/com/absmartly/sdk/jsonexpr/ExprEvaluatorTest.java` around lines 414 - 429, The test testStringConvertSpecialNumbers relies on locale-dependent DecimalFormat output from ExprEvaluator.stringConvert, so make it locale-independent by setting a known locale for the test (e.g., Locale.ROOT or Locale.US) before calling stringConvert and restoring the original default afterwards; alternatively, change the assertions to compare against a locale-neutral representation (e.g., Double.toString equivalents) or normalize the produced string to a predictable form prior to asserting. Ensure this change is applied inside testStringConvertSpecialNumbers and that original locale is restored after the assertions.gradle/wrapper/gradle-wrapper.properties (1)
1-1: AdddistributionSha256Sumfor distribution integrity verification.Always set the
distributionSha256Sumproperty in yourgradle-wrapper.propertiesfile to verify the integrity of the downloaded Gradle distribution. The Gradle Wrapper allows for verification of the downloaded Gradle distribution via SHA-256 hash sum comparison, which increases security against targeted attacks by preventing a man-in-the-middle attacker from tampering with the downloaded Gradle distribution.🛡️ Proposed fix
distributionUrl=https\://services.gradle.org/distributions/gradle-8.5-all.zip +distributionSha256Sum=17f277867f6914d61b1aa02efab1ba7bb439ad652ca485cd8ca6842fccec6e43 distributionBase=GRADLE_USER_HOMEThe SHA-256 value above is sourced from the official Gradle release checksums page for the
gradle-8.5-all.zipdistribution.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gradle/wrapper/gradle-wrapper.properties` at line 1, Add the distributionSha256Sum property next to the existing distributionUrl entry to enable Gradle Wrapper integrity verification: locate the distributionUrl line (which references gradle-8.5-all.zip) and add a matching distributionSha256Sum=<SHA-256-for-gradle-8.5-all.zip> value sourced from the official Gradle release checksums so the wrapper verifies the downloaded distribution.build.gradle (1)
52-60: Consider migrating eager task creation totasks.register(Gradle 8 deprecations).The eager
task sourcesJar(...)andtask javadocJar(...)patterns, plusjavadoc.destinationDir, are deprecated since Gradle 7 and generate deprecation warnings in Gradle 8.5. They still work, but migrating them avoids warning noise and future compatibility issues.♻️ Proposed refactor
- task sourcesJar(type: Jar, dependsOn: classes) { - archiveClassifier.set('sources') - from sourceSets.main.allSource - } - - task javadocJar(type: Jar, dependsOn: javadoc) { - archiveClassifier.set('javadoc') - from javadoc.destinationDir - } + tasks.register('sourcesJar', Jar) { + dependsOn classes + archiveClassifier.set('sources') + from sourceSets.main.allSource + } + + tasks.register('javadocJar', Jar) { + dependsOn javadoc + archiveClassifier.set('javadoc') + from javadoc.destinationDirectory.get().asFile + }The
artifact sourcesJar/artifact javadocJarreferences in thepublishingblock also need to becomeartifact tasks.named('sourcesJar')/artifact tasks.named('javadocJar')respectively.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@build.gradle` around lines 52 - 60, Replace the eager task declarations for sourcesJar and javadocJar with lazy registrations using tasks.register (referencing sourcesJar and javadocJar) and make javadocJar depend on the lazily-resolved javadoc task (use tasks.named('javadoc') or the javadoc task's destinationDirectory property instead of the deprecated javadoc.destinationDir). Also update the publishing block to reference the artifacts via tasks.named('sourcesJar') and tasks.named('javadocJar') (or tasks.named(...).get()) rather than the old artifact variables so everything is lazily configured and avoids the Gradle 8 deprecation warnings.core-api/src/test/java/com/absmartly/sdk/DefaultHTTPClientTest.java (2)
301-302:setConnectTimeout(100)is a no-op in this test context.The mocked transport never consults the config's timeout value; the
ConnectTimeoutExceptionis injected directly viacallback.failed(...). ThesetConnectTimeout(100)call implies the test is asserting something about the 100 ms limit, but it has no bearing on the test outcome. UsingDefaultHTTPClientConfig.create()(no custom timeout) would be less misleading.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core-api/src/test/java/com/absmartly/sdk/DefaultHTTPClientTest.java` around lines 301 - 302, The test is misleadingly configuring a timeout that the mocked transport ignores; remove the no-op setConnectTimeout(100) and instantiate the client with DefaultHTTPClient.create(DefaultHTTPClientConfig.create()) instead so the test uses the default config. Update the instantiation in DefaultHTTPClientTest where DefaultHTTPClient.create(...) is called (the call that currently uses DefaultHTTPClientConfig.create().setConnectTimeout(100)) to use the plain DefaultHTTPClientConfig.create() to avoid implying the test depends on the timeout.
314-316: Redundantinstanceofassertions afterassertSame.
assertSame(x, thrown.getCause())already guarantees the objects are identical, soassertTrue(thrown.getCause() instanceof T)is always trivially true and adds no signal. This pattern appears intestConnectionTimeout(lines 315–316),testReadTimeout(lines 340–341), andtestSSLCertificateValidation(lines 415–416).♻️ Remove redundant assertions
- assertSame(timeoutException, thrown.getCause()); - assertTrue(thrown.getCause() instanceof ConnectTimeoutException); + assertSame(timeoutException, thrown.getCause());- assertSame(readTimeoutException, thrown.getCause()); - assertTrue(thrown.getCause() instanceof SocketTimeoutException); + assertSame(readTimeoutException, thrown.getCause());- assertSame(sslException, thrown.getCause()); - assertTrue(thrown.getCause() instanceof javax.net.ssl.SSLHandshakeException); + assertSame(sslException, thrown.getCause());Also applies to: 339-341, 414-416
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core-api/src/test/java/com/absmartly/sdk/DefaultHTTPClientTest.java` around lines 314 - 316, Remove the redundant instanceof assertions that follow assertSame in the unit tests: in testConnectionTimeout, testReadTimeout, and testSSLCertificateValidation delete the lines asserting instanceof (e.g., the assertTrue(thrown.getCause() instanceof ConnectTimeoutException) and similar for read/SSL exceptions) since assertSame(timeoutException, thrown.getCause()) already guarantees identity; keep the assertThrows and assertSame checks unchanged.core-api/src/test/java/com/absmartly/sdk/DefaultHTTPClientConfigTest.java (1)
105-114:testDefaultValuesdoes not assert thesecurityProviderdefault.
DefaultHTTPClientConfiginitialisessecurityProvider_tonull. The test covers all other six fields but omits this one.♻️ Proposed addition
void testDefaultValues() { final DefaultHTTPClientConfig config = DefaultHTTPClientConfig.create(); assertEquals(3000, config.getConnectTimeout()); assertEquals(30000, config.getConnectionKeepAlive()); assertEquals(1000, config.getConnectionRequestTimeout()); assertEquals(5, config.getMaxRetries()); assertEquals(333, config.getRetryInterval()); assertEquals(HTTPVersionPolicy.NEGOTIATE, config.getHTTPVersionPolicy()); + assertNull(config.getSecurityProvider()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core-api/src/test/java/com/absmartly/sdk/DefaultHTTPClientConfigTest.java` around lines 105 - 114, The test testDefaultValues in DefaultHTTPClientConfigTest misses asserting the default for securityProvider_; update the test to assert the default value returned by DefaultHTTPClientConfig.create() via getSecurityProvider() (expecting null) so the securityProvider default is covered alongside the other fields (modify testDefaultValues to include this assertion).core-api/src/main/java/com/absmartly/sdk/internal/JsonMapperUtils.java (2)
7-13: Add a private constructor to prevent instantiation of this utility class.♻️ Suggested fix
public class JsonMapperUtils { + private JsonMapperUtils() {} + public static ObjectMapper createStandardObjectMapper() {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core-api/src/main/java/com/absmartly/sdk/internal/JsonMapperUtils.java` around lines 7 - 13, The utility class JsonMapperUtils should not be instantiable; add a private no-argument constructor to the JsonMapperUtils class to prevent creating instances (so only its static method createStandardObjectMapper can be used), and ensure the constructor is declared private and empty to satisfy static-utility-class conventions.
10-10:MapperFeature.USE_STATIC_TYPINGis a serialisation-only feature — it is a no-op for all current consumers.Jackson's own documentation states: "Note that deserialization always uses declared static types since no runtime types are available."
Every current consumer of
createStandardObjectMapper()(DefaultAudienceDeserializer,DefaultContextDataDeserializer,DefaultVariableParser) uses the mapper exclusively for reading/deserialisation. The feature therefore has no effect. OnlyDefaultContextEventSerializer(a writer) would benefit, but it does not use this utility. Consider either removing the flag from this utility or documenting the intent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core-api/src/main/java/com/absmartly/sdk/internal/JsonMapperUtils.java` at line 10, The MapperFeature.USE_STATIC_TYPING flag set in createStandardObjectMapper() is a serialization-only/no-op for current deserializing consumers; remove the objectMapper.enable(MapperFeature.USE_STATIC_TYPING) call from createStandardObjectMapper() (or alternatively add a comment explaining why it's present) and update callers if needed—notably DefaultAudienceDeserializer, DefaultContextDataDeserializer, and DefaultVariableParser use the mapper only for reading so they don't need this flag, while DefaultContextEventSerializer is the only writer that would benefit and does not use this utility.core-api/src/main/java/com/absmartly/sdk/DefaultContextEventSerializer.java (1)
18-19:DefaultContextEventSerializerskipsJsonMapperUtils.createStandardObjectMapper()— only consumer where it would matter.
USE_STATIC_TYPINGis a serialisation-only feature (Jackson docs state deserialization always uses declared static types). All three deserialiser/parser callers ofJsonMapperUtilsare read-only, so the feature has no effect there. This serialiser is the sole class where enablingUSE_STATIC_TYPINGwould actually change behaviour, yet it still constructs a plainnew ObjectMapper().♻️ Suggested alignment
+import com.absmartly.sdk.internal.JsonMapperUtils; ... public DefaultContextEventSerializer() { - final ObjectMapper objectMapper = new ObjectMapper(); + final ObjectMapper objectMapper = JsonMapperUtils.createStandardObjectMapper(); this.writer_ = objectMapper.writerFor(PublishEvent.class); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core-api/src/main/java/com/absmartly/sdk/DefaultContextEventSerializer.java` around lines 18 - 19, DefaultContextEventSerializer constructs a plain new ObjectMapper() so its writer_ (writerFor(PublishEvent.class)) ignores any standard configuration (e.g. USE_STATIC_TYPING) provided by JsonMapperUtils; replace the ObjectMapper creation with JsonMapperUtils.createStandardObjectMapper() when building the ObjectMapper used to initialize writer_ in DefaultContextEventSerializer so the serializer honors the shared standard mapper settings for PublishEvent serialization.example/src/main/java/com/absmartly/sdk/Example.java (1)
24-36: Consider using try-with-resources to guaranteectxandsdkare closed even on exception.As written, any exception between
createContext(...)andctx.close()will leak the context and SDK resources. Even in an example, demonstrating the idiomatic pattern is useful.♻️ Suggested pattern
-final Context ctx = sdk.createContext(contextConfig).waitUntilReady(); -// ... use ctx ... -ctx.close(); -sdk.close(); +try (final ABsmartly sdk = ABsmartly.create(sdkConfig); + final Context ctx = sdk.createContext(contextConfig).waitUntilReady()) { + // ... use ctx ... +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@example/src/main/java/com/absmartly/sdk/Example.java` around lines 24 - 36, Wrap the SDK and Context in a try-with-resources so they are always closed on exception: move or create the SDK instance in a resource declaration (or nest try-with-resources) and obtain the Context via sdk.createContext(...).waitUntilReady() as a resource, then perform ctx.getTreatment(...), ctx.track(...), etc.; remove explicit ctx.close() and sdk.close() calls and rely on try-with-resources to auto-close the Context and SDK (referencing createContext, waitUntilReady, ctx, ctx.getTreatment, ctx.track, ctx.close, sdk.close).core-api/src/test/java/com/absmartly/sdk/ContextTest.java (2)
2511-2523: Near-duplicate of the existingpublishExceptionallytest at line 1641
publishPropagatesClientErrorOnFailureis essentially the same test aspublishExceptionally, with minor differences in goal properties and exception message. Consider removing this test and, if the intent is to give a more focused name, renamepublishExceptionallyinstead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core-api/src/test/java/com/absmartly/sdk/ContextTest.java` around lines 2511 - 2523, The test publishPropagatesClientErrorOnFailure duplicates publishExceptionally; remove the redundant publishPropagatesClientErrorOnFailure test or consolidate by renaming/adjusting publishExceptionally to cover both cases (e.g., change publishExceptionally’s name to publishPropagatesClientError and/or add the slightly different goal/property inputs and exception message there) so only one test verifies that Context.publish propagates the client error (references: test methods publishPropagatesClientErrorOnFailure and publishExceptionally, Context.publish).
2565-2583: Near-duplicate of the existingcloseAsynctest at line 1661
closeAsyncReturnsSameFutureWhenCalledTwicecovers the same core assertion ascloseAsync(lines 1661–1685), which also verifies that twocloseAsync()calls return the same future, plus additionally checks closing/closed state and event handler invocation count. Consider removing this test.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core-api/src/test/java/com/absmartly/sdk/ContextTest.java` around lines 2565 - 2583, The test closeAsyncReturnsSameFutureWhenCalledTwice duplicates assertions already covered in the existing closeAsync test; remove the redundant test method closeAsyncReturnsSameFutureWhenCalledTwice from ContextTest (or consolidate its unique assertions into the existing closeAsync test) so we only keep one test that verifies multiple closeAsync() calls return the same CompletableFuture and the context transitions to closed; update imports/mocks if needed after removal.core-api/src/test/java/com/absmartly/sdk/DefaultContextDataDeserializerTest.java (1)
127-146:testMalformedJsonResponsebundles four distinct scenarios — one failing assertion silences the restIn JUnit 5, a failing
assert*stops the method, so the{}and[]cases will never be reached if the earlier assertions fail. Also, the third case ({}→assertNotNull) is valid JSON and arguably belongs in a separate test (testEmptyObjectJson), distinct from truly malformed inputs. Consider splitting into individual@Testmethods or usingassertAll.♻️ Suggested split (partial example)
- `@Test` - void testMalformedJsonResponse() { - final ContextDataDeserializer deser = new DefaultContextDataDeserializer(); - - final byte[] malformedJson = "{\"experiments\": [".getBytes(); - final ContextData result = deser.deserialize(malformedJson, 0, malformedJson.length); - assertNull(result); - - final byte[] invalidJson = "not a json at all".getBytes(); - final ContextData result2 = deser.deserialize(invalidJson, 0, invalidJson.length); - assertNull(result2); - - final byte[] emptyBraces = "{}".getBytes(); - final ContextData result3 = deser.deserialize(emptyBraces, 0, emptyBraces.length); - assertNotNull(result3); - - final byte[] emptyArray = "[]".getBytes(); - final ContextData result4 = deser.deserialize(emptyArray, 0, emptyArray.length); - assertNull(result4); - } + `@Test` + void testIncompleteJsonReturnsNull() { + final ContextDataDeserializer deser = new DefaultContextDataDeserializer(); + assertNull(deser.deserialize("{\"experiments\": [".getBytes(), 0, 18)); + } + + `@Test` + void testNonJsonInputReturnsNull() { + final ContextDataDeserializer deser = new DefaultContextDataDeserializer(); + assertNull(deser.deserialize("not a json at all".getBytes(), 0, 17)); + } + + `@Test` + void testEmptyObjectDeserializesToContextData() { + final ContextDataDeserializer deser = new DefaultContextDataDeserializer(); + assertNotNull(deser.deserialize("{}".getBytes(), 0, 2)); + } + + `@Test` + void testJsonArrayRootReturnsNull() { + final ContextDataDeserializer deser = new DefaultContextDataDeserializer(); + assertNull(deser.deserialize("[]".getBytes(), 0, 2)); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core-api/src/test/java/com/absmartly/sdk/DefaultContextDataDeserializerTest.java` around lines 127 - 146, The test method testMalformedJsonResponse in DefaultContextDataDeserializerTest mixes four scenarios so a failing assertion aborts the rest; split it into separate `@Test` methods (e.g., testMalformedJson_returnsNull, testNonJson_returnsNull, testEmptyObject_returnsNotNull, testEmptyArray_returnsNull) that each call ContextDataDeserializer.deserialize (or keep reuse of deser/deserialize) and assert the expected outcome, or wrap the assertions in an assertAll if you prefer a single method; ensure each new test references the same ContextDataDeserializer deser and the deserialize(byte[], int, int) invocation so behavior is validated independently.core-api/src/test/java/com/absmartly/sdk/DefaultContextDataProviderTest.java (1)
91-93: Minor: prefer a top-level import over the fully-qualifiedTimeoutExceptionclass name
java.util.concurrent.TimeoutExceptionis referenced twice in this method. Adding an import keeps the code consistent with other types in this file.♻️ Suggested change
Add at the top of the file alongside other imports:
+import java.util.concurrent.TimeoutException;Then simplify the declaration:
- final java.util.concurrent.TimeoutException timeoutException = new java.util.concurrent.TimeoutException( - "Request timed out"); + final TimeoutException timeoutException = new TimeoutException("Request timed out");And update the
instanceofcheck:- assertTrue(actual.getCause() instanceof java.util.concurrent.TimeoutException); + assertTrue(actual.getCause() instanceof TimeoutException);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core-api/src/test/java/com/absmartly/sdk/DefaultContextDataProviderTest.java` around lines 91 - 93, In DefaultContextDataProviderTest, replace the fully-qualified java.util.concurrent.TimeoutException usages with a top-level import: add "import java.util.concurrent.TimeoutException;" to the imports, then change the declaration of timeoutException to use TimeoutException and adjust any instanceof checks (e.g., in assertions against failedFuture's exception) to use TimeoutException instead of the fully-qualified name; this keeps the style consistent with other types in the file.core-api/src/main/java/com/absmartly/sdk/deprecated/ABSmartly.java (1)
7-15: Missing@deprecatedJavadoc tag — callers won't know what to migrate toThe class-level
@Deprecatedannotation suppresses usage but provides no guidance. Adding a@deprecatedJavadoc tag with a@see(or@link) reference to the replacement class is the standard Java convention and improves IDE tooling.♻️ Suggested Javadoc addition
+/** + * `@deprecated` Use {`@link` com.absmartly.sdk.ABsmartly} instead. + */ `@Deprecated` public class ABSmartly extends ABsmartly {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core-api/src/main/java/com/absmartly/sdk/deprecated/ABSmartly.java` around lines 7 - 15, Add a Javadoc block to the deprecated ABSmartly class that includes a `@deprecated` tag and a `@see` (or {`@link`}) reference to the replacement ABsmartly class so callers/IDEs know the migration target; update the class-level comment above public class ABSmartly (which contains the create(...) factory and protected constructor ABSmartly(...)) to include the deprecation guidance and reference.core-api/src/main/java/com/absmartly/sdk/DefaultContextDataDeserializer.java (1)
19-19:ObjectMapper.enable(MapperFeature)is deprecated since Jackson 2.13, andUSE_STATIC_TYPINGhas no effect on deserialisationThe
JsonMapperUtils.createStandardObjectMapper()utility (referenced fromcore-api/src/main/java/com/absmartly/sdk/internal/JsonMapperUtils.java, lines 10–11) uses deprecated Jackson API. Two concerns:
ObjectMapper.enable(MapperFeature...)has been deprecated since Jackson 2.13 — the idiomatic replacement isJsonMapper.builder().USE_STATIC_TYPINGdetermines whether serialisation type detection uses runtime or declared static type. Deserialistion always uses declared static types; enabling this flag has no effect in a deserialiser context.The project uses Jackson 2.13.4.2, so migrate
JsonMapperUtilsto the builder API and remove the no-opUSE_STATIC_TYPINGflag for clarity and to eliminate deprecation warnings.♻️ Suggested update for `JsonMapperUtils.createStandardObjectMapper()`
-import com.fasterxml.jackson.databind.MapperFeature; import com.fasterxml.jackson.databind.DeserializationFeature; -import com.fasterxml.jackson.databind.ObjectMapper; +import com.fasterxml.jackson.databind.json.JsonMapper; public class JsonMapperUtils { public static ObjectMapper createStandardObjectMapper() { - final ObjectMapper objectMapper = new ObjectMapper(); - objectMapper.enable(MapperFeature.USE_STATIC_TYPING); - objectMapper.enable(DeserializationFeature.FAIL_ON_READING_DUP_TREE_KEY); - return objectMapper; + return JsonMapper.builder() + .enable(DeserializationFeature.FAIL_ON_READING_DUP_TREE_KEY) + .build(); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core-api/src/main/java/com/absmartly/sdk/DefaultContextDataDeserializer.java` at line 19, Update JsonMapperUtils.createStandardObjectMapper() to stop using the deprecated ObjectMapper.enable(MapperFeature...) call and instead build the mapper via JsonMapper.builder(), configuring the equivalent features on the builder; remove any enabling of MapperFeature.USE_STATIC_TYPING (it's a no-op for deserialization) and ensure the resulting mapper is functionally equivalent for the project (e.g., register modules, set property naming/visibility, and any other previously-enabled MapperFeatures) so that DefaultContextDataDeserializer (which calls JsonMapperUtils.createStandardObjectMapper()) continues to work without deprecation warnings.core-api/src/main/java/com/absmartly/sdk/Context.java (2)
1131-1137:buildAttributesMapiteratesattributes_without acquiring a lock — relies on caller to holdcontextLock_.All current call sites hold
contextLock_(read or write), so this is safe. Consider adding a brief Javadoc or assertion noting the precondition for future maintainers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core-api/src/main/java/com/absmartly/sdk/Context.java` around lines 1131 - 1137, buildAttributesMap reads the mutable attributes_ collection without synchronizing, relying on callers to hold contextLock_; add an explicit contract to prevent future misuse by either inserting a brief Javadoc on buildAttributesMap stating it must be called while holding contextLock_ and/or add a runtime assertion inside buildAttributesMap that checks Thread.holdsLock(contextLock_) (or equivalent) to fail fast; reference the buildAttributesMap method and the contextLock_ field so maintainers see the precondition.
726-740:audienceMatchesre-evaluates on every cache hit after anysetAttributecall.The
attrsSeqstored on the cachedAssignmentis only set when the assignment is first created (line 870). It is never updated whenaudienceMatchesconfirms the cached value is still correct. This means after anysetAttributecall, every subsequentgetTreatmentwill rebuild the attributes map and re-evaluate audiences, even when the result is unchanged.If this is intentional (simplicity over performance — avoids needing a write lock to update the assignment), it's fine for most workloads. But for contexts with frequent attribute changes and many experiments, consider updating
assignment.attrsSeqinside the write lock path when the cache is rebuilt, to avoid redundant evaluations on subsequent read-lock hits.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core-api/src/main/java/com/absmartly/sdk/Context.java` around lines 726 - 740, The audienceMatches method currently re-evaluates audiences after any setAttribute because Assignment.attrsSeq is only initialized when the assignment is created (see where attrsSeq is set in Assignment) and never updated; change the cache-rebuild/write-lock path that calls buildAttributesMap and audienceMatcher_.evaluate (the code that updates the cached Assignment) so that when a re-evaluation confirms the cached assignment is still valid you also update assignment.attrsSeq to the current attrsSeq_ value; specifically locate the code that rebuilds the assignment in the write-lock path and set assignment.attrsSeq = attrsSeq_ (or the current sequence token) whenever audienceMatches returns that the audience result did not change, so subsequent read-lock hits skip redundant buildAttributesMap and audienceMatcher_.evaluate calls.core-api/src/main/java/com/absmartly/sdk/ABsmartly.java (1)
72-91:close()is not thread-safe — concurrent callers may double-close resources.Both
client_andscheduler_are non-volatile fields with no synchronisation around the null-check-then-act pattern. If two threads callclose()concurrently, both could enter theif (client_ != null)block and invokeclient_.close()twice, potentially causing issues depending on theClientimplementation. Similarly for the scheduler block.This may be acceptable if
close()is only ever called from a single thread (typical for application shutdown), but it's worth noting given the concurrency improvements elsewhere in this PR.🛡️ Suggested fix: synchronise close() or use a closed guard
A simple approach is to use an
AtomicBooleanguard (similar toContext.closed_):+ private final AtomicBoolean closed_ = new AtomicBoolean(false); + `@Override` public void close() throws IOException { + if (!closed_.compareAndSet(false, true)) { + return; + } if (client_ != null) { client_.close(); client_ = null; } // ... scheduler shutdown ... }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core-api/src/main/java/com/absmartly/sdk/ABsmartly.java` around lines 72 - 91, The close() method is not thread-safe: concurrent callers can double-close client_ and scheduler_. Add a final AtomicBoolean closed_ (or reuse Context.closed_) and check if closed_.compareAndSet(false,true) at the start of close(); only proceed to close client_ and shutdown scheduler_ when the CAS succeeds, and make client_ and scheduler_ volatile or only accessed after the guard to avoid races; this ensures close() runs exactly once and prevents double-close races on client_.close() and scheduler_.shutdown()/shutdownNow()/awaitTermination().
| sourceCompatibility = "1.8" | ||
| targetCompatibility = "1.8" |
There was a problem hiding this comment.
Document the Java 6/7 → 8 bytecode break in the release notes.
Bumping sourceCompatibility and targetCompatibility from 1.6 to 1.8 changes the published JAR's minimum required JVM from Java 6 to Java 8. Any downstream consumer still running Java 6 or 7 will fail to load the SDK at class-loading time. This is a binary-breaking change and should be called out explicitly in the changelog or release notes alongside the version bump.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@build.gradle` around lines 38 - 39, The build now sets sourceCompatibility
and targetCompatibility to "1.8", which raises the published JAR's minimum JVM
requirement from Java 6/7 to Java 8; add a clear changelog/release-notes entry
calling out this binary-breaking change so downstream users are warned. Update
your release notes (or CHANGELOG) to state that
sourceCompatibility/targetCompatibility were bumped to 1.8, that older JVMs
(Java 6 and 7) will fail at class-load time, and include migration guidance
(e.g., upgrade JVM to Java 8 or use an earlier SDK release). Ensure the note
references the change to sourceCompatibility and targetCompatibility so it’s
discoverable.
| if (!endpoint.startsWith("https://")) { | ||
| if (endpoint.startsWith("http://")) { | ||
| System.err.println( | ||
| "WARNING: ABSmartly SDK endpoint is not using HTTPS. API keys will be transmitted in plaintext: " | ||
| + endpoint); | ||
| } else { | ||
| throw new IllegalArgumentException("Endpoint must use http:// or https:// protocol: " + endpoint); | ||
| } | ||
| } |
There was a problem hiding this comment.
Replace System.err.println with the SLF4J logger — library code must not write directly to stderr.
System.err.println bypasses any logging framework configured by the SDK consumer, cannot be suppressed or redirected, and is inconsistent with every other class in this codebase. Client has no Logger field; one should be added.
🔧 Proposed fix
Add a logger field alongside the other private final fields at the bottom of the class:
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
...
public class Client implements Closeable {
+ private static final Logger log = LoggerFactory.getLogger(Client.class);Then replace the System.err.println call:
- System.err.println(
- "WARNING: ABSmartly SDK endpoint is not using HTTPS. API keys will be transmitted in plaintext: "
- + endpoint);
+ log.warn("ABsmartly SDK endpoint is not using HTTPS. API keys will be transmitted in plaintext: {}", endpoint);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@core-api/src/main/java/com/absmartly/sdk/Client.java` around lines 34 - 42,
Replace the direct stderr write in Client with an SLF4J logger: add a private
static final Logger (e.g., LoggerFactory.getLogger(Client.class)) as a field in
the Client class and change the System.err.println call to use logger.warn(...)
with the same warning message (including the endpoint). Ensure imports for
org.slf4j.Logger and org.slf4j.LoggerFactory are added and no other behavior is
changed.
| "WARNING: ABSmartly SDK endpoint is not using HTTPS. API keys will be transmitted in plaintext: " | ||
| + endpoint); |
There was a problem hiding this comment.
Branding inconsistency: warning text says "ABSmartly SDK" but the project renames to "ABsmartly".
🔧 Proposed fix
-"WARNING: ABSmartly SDK endpoint is not using HTTPS. API keys will be transmitted in plaintext: "
+"WARNING: ABsmartly SDK endpoint is not using HTTPS. API keys will be transmitted in plaintext: "📝 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.
| "WARNING: ABSmartly SDK endpoint is not using HTTPS. API keys will be transmitted in plaintext: " | |
| + endpoint); | |
| "WARNING: ABsmartly SDK endpoint is not using HTTPS. API keys will be transmitted in plaintext: " | |
| endpoint); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@core-api/src/main/java/com/absmartly/sdk/Client.java` around lines 37 - 38,
Update the branding in the plaintext HTTPS warning emitted by the Client class:
replace the "ABSmartly SDK" token in the string literal currently built in
Client.java with the correct project name "ABsmartly" so the message reads e.g.
"WARNING: ABsmartly SDK endpoint is not using HTTPS. API keys will be
transmitted in plaintext: " + endpoint; locate the string inside the Client
class where the warning is constructed and change the casing to match the
project's branding.
| dataFuture.complete( | ||
| deserializer_.deserialize(content, 0, content.length)); |
There was a problem hiding this comment.
deserializer_.deserialize() can return null — completing the future successfully with null propagates a silent failure.
The deserializers were changed in this PR to return null on bad input rather than throw. If deserialization fails here, dataFuture.complete(null) is called, and any downstream code that dereferences the ContextData result without a null check will throw an NPE. The same guard pattern used for the empty-body case (line 100) should apply here.
🔧 Proposed fix
- dataFuture.complete(
- deserializer_.deserialize(content, 0, content.length));
+ final ContextData data = deserializer_.deserialize(content, 0, content.length);
+ if (data == null) {
+ dataFuture.completeExceptionally(
+ new IllegalStateException("Failed to deserialize context data response"));
+ } else {
+ dataFuture.complete(data);
+ }📝 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.
| dataFuture.complete( | |
| deserializer_.deserialize(content, 0, content.length)); | |
| final ContextData data = deserializer_.deserialize(content, 0, content.length); | |
| if (data == null) { | |
| dataFuture.completeExceptionally( | |
| new IllegalStateException("Failed to deserialize context data response")); | |
| } else { | |
| dataFuture.complete(data); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@core-api/src/main/java/com/absmartly/sdk/Client.java` around lines 103 - 104,
deserializer_.deserialize(...) can return null which currently causes
dataFuture.complete(null) and later NPEs; change the handler so that after
calling deserializer_.deserialize(content, 0, content.length) you check for null
and treat it like the empty-body case: do not complete the future with null but
instead completeExceptionally (or complete with a failed state) and
log/propagate an error so downstream code expecting a non-null ContextData does
not receive null; update the code path around deserializer_.deserialize,
dataFuture.complete and any code that assumes a non-null ContextData to follow
the same guard pattern used for the empty-body branch (the earlier empty-body
check) and ensure ContextData is validated before completing the future.
| final ObjectMapper objectMapper = new ObjectMapper(); | ||
| objectMapper.enable(MapperFeature.USE_STATIC_TYPING); | ||
| objectMapper.enable(DeserializationFeature.FAIL_ON_READING_DUP_TREE_KEY); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find build configuration files
find . -maxdepth 3 -type f \( -name "pom.xml" -o -name "build.gradle*" -o -name "gradle.properties" \) | head -20Repository: absmartly/java-sdk
Length of output: 123
🌐 Web query:
ObjectMapper.enable MapperFeature deprecated Jackson 2.13 builder pattern JsonMapper
💡 Result:
In Jackson 2.13, only the ObjectMapper mutators that take MapperFeature were deprecated:
ObjectMapper.enable(MapperFeature... f)ObjectMapper.disable(MapperFeature... f)ObjectMapper.configure(MapperFeature f, boolean state)
They’re deprecated “Since 2.13” in favor of configuring via the JsonMapper builder: JsonMapper.builder().enable(...) / .disable(...) / .configure(...). [1]
Recommended (new) pattern
import com.fasterxml.jackson.databind.MapperFeature;
import com.fasterxml.jackson.databind.json.JsonMapper;
JsonMapper mapper = JsonMapper.builder()
.enable(MapperFeature.ACCEPT_CASE_INSENSITIVE_PROPERTIES)
.disable(MapperFeature.DEFAULT_VIEW_INCLUSION)
.build();(You can also mix in other feature types on the builder, e.g. SerializationFeature, DeserializationFeature, etc.)
If you already have a JsonMapper instance
JsonMapper provides rebuild() to get a builder from the current mapper, then build() a new configured instance. [2]
JsonMapper mapper2 = existingJsonMapper.rebuild()
.enable(MapperFeature.ACCEPT_CASE_INSENSITIVE_PROPERTIES)
.build();Note
ObjectMapper.enable(...) for non-MapperFeature enums (like SerializationFeature, DeserializationFeature, JsonParser.Feature, JsonGenerator.Feature) is not what this 2.13 deprecation targets—this deprecation is specifically for the MapperFeature overloads. [1]
Sources:
[1] Jackson databind 2.13 ObjectMapper Javadoc (deprecations + replacement with JsonMapper.builder()) https://fasterxml.github.io/jackson-databind/javadoc/2.13/com/fasterxml/jackson/databind/ObjectMapper.html
[2] Jackson databind 2.13 JsonMapper Javadoc (builder() / rebuild()) https://fasterxml.github.io/jackson-databind/javadoc/2.13/com/fasterxml/jackson/databind/json/JsonMapper.html
🏁 Script executed:
# Check the Jackson version in core-api/build.gradle
cat core-api/build.gradle | grep -A 10 -B 10 "jackson"Repository: absmartly/java-sdk
Length of output: 1399
🏁 Script executed:
# Check the root build.gradle for jacksonVersion definition
cat build.gradle | grep -i "jackson"Repository: absmartly/java-sdk
Length of output: 125
🏁 Script executed:
# Also check for gradle.properties or similar version files
cat gradle.properties 2>/dev/null || echo "gradle.properties not found"Repository: absmartly/java-sdk
Length of output: 89
Replace ObjectMapper with JsonMapper builder to avoid deprecated mutation API.
The ObjectMapper.enable(MapperFeature) mutation API was deprecated in Jackson 2.13 (your project uses 2.13.4.2) in favour of the builder pattern. The DeserializationFeature.FAIL_ON_READING_DUP_TREE_KEY call is unaffected—only MapperFeature overloads are deprecated.
Use JsonMapper.builder() instead:
Suggested fix
final ObjectMapper objectMapper = JsonMapper.builder()
.enable(MapperFeature.USE_STATIC_TYPING)
.build()
.enable(DeserializationFeature.FAIL_ON_READING_DUP_TREE_KEY);Or if you prefer to keep ObjectMapper, configure all features on the builder:
final ObjectMapper objectMapper = JsonMapper.builder()
.enable(MapperFeature.USE_STATIC_TYPING)
.enable(DeserializationFeature.FAIL_ON_READING_DUP_TREE_KEY)
.build();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@core-api/src/main/java/com/absmartly/sdk/internal/JsonMapperUtils.java`
around lines 9 - 11, Replace the deprecated ObjectMapper mutation API by
creating the mapper with JsonMapper.builder(): stop calling
objectMapper.enable(MapperFeature.USE_STATIC_TYPING) on an ObjectMapper instance
and instead use JsonMapper.builder().enable(MapperFeature.USE_STATIC_TYPING)
before build(); for DeserializationFeature.FAIL_ON_READING_DUP_TREE_KEY either
enable it on the same builder or call .enable(...) on the returned
ObjectMapper—adjust the code around the existing ObjectMapper variable in
JsonMapperUtils to use JsonMapper.builder() and enable the listed features
there.
| @Test | ||
| void testNegativeConnectTimeout() { | ||
| final DefaultHTTPClientConfig config = DefaultHTTPClientConfig.create() | ||
| .setConnectTimeout(-1); | ||
| assertEquals(-1, config.getConnectTimeout()); | ||
| } | ||
|
|
||
| @Test | ||
| void testNegativeConnectionKeepAlive() { | ||
| final DefaultHTTPClientConfig config = DefaultHTTPClientConfig.create() | ||
| .setConnectionKeepAlive(-1); | ||
| assertEquals(-1, config.getConnectionKeepAlive()); | ||
| } | ||
|
|
||
| @Test | ||
| void testNegativeConnectionRequestTimeout() { | ||
| final DefaultHTTPClientConfig config = DefaultHTTPClientConfig.create() | ||
| .setConnectionRequestTimeout(-1); | ||
| assertEquals(-1, config.getConnectionRequestTimeout()); | ||
| } | ||
|
|
||
| @Test | ||
| void testNegativeRetryInterval() { | ||
| final DefaultHTTPClientConfig config = DefaultHTTPClientConfig.create() | ||
| .setRetryInterval(-1); | ||
| assertEquals(-1, config.getRetryInterval()); | ||
| } | ||
|
|
||
| @Test | ||
| void testZeroMaxRetries() { | ||
| final DefaultHTTPClientConfig config = DefaultHTTPClientConfig.create() | ||
| .setMaxRetries(0); | ||
| assertEquals(0, config.getMaxRetries()); | ||
| } | ||
|
|
||
| @Test | ||
| void testNegativeMaxRetries() { | ||
| final DefaultHTTPClientConfig config = DefaultHTTPClientConfig.create() | ||
| .setMaxRetries(-1); | ||
| assertEquals(-1, config.getMaxRetries()); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "DefaultHTTPClient.java" | head -5Repository: absmartly/java-sdk
Length of output: 127
🏁 Script executed:
rg -n "Timeout\.ofMilliseconds|TimeValue\.ofMilliseconds|getConnectTimeout|getConnectionKeepAlive|getConnectionRequestTimeout|getRetryInterval" \
--type java -A 2 -B 2 | head -100Repository: absmartly/java-sdk
Length of output: 9361
🏁 Script executed:
find . -type f -name "DefaultHTTPClientConfig.java" -o -name "DefaultHTTPClient.java" | xargs wc -lRepository: absmartly/java-sdk
Length of output: 220
🏁 Script executed:
cat -n core-api/src/main/java/com/absmartly/sdk/DefaultHTTPClientConfig.javaRepository: absmartly/java-sdk
Length of output: 2703
🏁 Script executed:
cat -n core-api/src/main/java/com/absmartly/sdk/DefaultHTTPClient.java | head -70Repository: absmartly/java-sdk
Length of output: 3523
🌐 Web query:
Apache HttpClient 5 HC5 Timeout.ofMilliseconds non-negative contract
💡 Result:
In Apache HttpComponents Core / HttpClient 5 (HC5), org.apache.hc.core5.util.Timeout is defined to be non-negative (“a timeout value as a non-negative long time …”).[1]
What Timeout.ofMilliseconds(...) accepts
Timeout.ofMilliseconds(long milliseconds)ultimately callsnew Timeout(duration, unit).[2]- The
Timeoutconstructor enforces the contract withArgs.notNegative(duration, "duration"), so any negative value (e.g.,-1) violates the contract and results in anIllegalArgumentException.[2]
How to represent “no timeout”
- HC5 represents disabled / infinite timeout as
0 MILLISECONDSvia the constantsTimeout.DISABLEDandTimeout.INFINITE(both effectively0 ms).[1][2] - Separately, some APIs that take a more general
TimeValue(notTimeout) may treat non-positive values as “indefinite” (for example,Deadline.calculate(TimeValue)).[3] But that does not changeTimeout’s non-negative requirement.
Sources:
[1] Timeout Javadoc (Core 5.3.x)
[2] Timeout.java source (constructor uses Args.notNegative, ofMilliseconds delegates to it)
[3] Deadline.calculate(TimeValue) Javadoc (non-positive means indefinite for TimeValue)
🌐 Web query:
Apache HttpClient 5 TimeValue.ofMilliseconds negative values behavior
💡 Result:
TimeValue.ofMilliseconds(long millis) in HttpClient/HttpCore 5 accepts negative values (it does not clamp or throw). The API even exposes predefined constants like TimeValue.NEG_ONE_MILLISECOND / NEG_ONE_SECOND alongside ZERO_MILLISECONDS, which indicates negatives are an expected/valid representation of a duration value. [1]
What that means depends on where the TimeValue is used:
- In deadline-based calculations, non-positive time values (0 or negative) are treated as “indefinite timeout without a deadline” (i.e., no deadline is computed). [2]
- In the closely related
Timeouttype (a subclass/related concept in the same util package),DISABLEDandINFINITEare both represented as 0 milliseconds, and the project has an issue discussing/clarifying the semantics difference between “disabled” vs “infinite.” [3][4]
So: TimeValue.ofMilliseconds(-N) produces a negative TimeValue, and some parts of the stack interpret <= 0 as “indefinite / no deadline”, while other timeout APIs may treat 0 as a special value with semantics that vary by component. [1][2][3][4]
Sources: [1] [2] [3] [4]
Negative timeout/retry tests document unsafe values that will cause runtime failures in DefaultHTTPClient.
These tests verify that DefaultHTTPClientConfig stores negative values without throwing—correct for a simple POJO. However, DefaultHTTPClient passes these directly to Apache HC5's Timeout and TimeValue methods, which behave differently:
Timeout.ofMilliseconds(config.getConnectTimeout())(line 59) andTimeout.ofMilliseconds(config.getConnectionRequestTimeout())(line 60) will throwIllegalArgumentExceptionif passed-1, asTimeoutenforces a non-negative contract viaArgs.notNegative().TimeValue.ofMilliseconds(config.getConnectionKeepAlive())(lines 54, 58) accepts-1but interprets it as "indefinite timeout", which is likely unintended.
The tests implicitly document that these negative values are acceptable inputs, when in fact they violate HC5's contract. Consider adding validation to the config setters to reject negative values with IllegalArgumentException, or at minimum add a comment clarifying why negative values are intentionally unvalidated. If validation is added, update these tests to assert the thrown exception rather than the stored value.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@core-api/src/test/java/com/absmartly/sdk/DefaultHTTPClientConfigTest.java`
around lines 63 - 103, The tests allow negative timeout/retry values but Apache
HC5 (used in DefaultHTTPClient via Timeout.ofMilliseconds and
TimeValue.ofMilliseconds) rejects or misinterprets negatives; update
DefaultHTTPClientConfig setter methods (setConnectTimeout,
setConnectionRequestTimeout, setConnectionKeepAlive, setRetryInterval,
setMaxRetries) to validate inputs and throw IllegalArgumentException for
negative values (ensure any special-case semantics are deliberate), and then
update the corresponding tests (testNegativeConnectTimeout,
testNegativeConnectionKeepAlive, testNegativeConnectionRequestTimeout,
testNegativeRetryInterval, testNegativeMaxRetries) to assertThrows
IllegalArgumentException instead of checking the stored negative value so
behavior matches HC5 expectations.
| @Test | ||
| void testRetryOnTransientError503Response() throws ExecutionException, InterruptedException { | ||
| try (final MockedStatic<HttpAsyncClientBuilder> builderStatic = Mockito | ||
| .mockStatic(HttpAsyncClientBuilder.class)) { | ||
| builderStatic.when(HttpAsyncClientBuilder::create).thenReturn(asyncHTTPClientBuilder); | ||
|
|
||
| final DefaultHTTPClient httpClient = DefaultHTTPClient.create( | ||
| DefaultHTTPClientConfig.create().setMaxRetries(3).setRetryInterval(100)); | ||
|
|
||
| when(asyncHTTPClient.execute(any(), any())).thenAnswer(invocation -> { | ||
| final FutureCallback<SimpleHttpResponse> callback = invocation.getArgument(1); | ||
| callback.completed(SimpleHttpResponse.create(503, "Service Unavailable".getBytes(), | ||
| ContentType.TEXT_PLAIN)); | ||
| return null; | ||
| }); | ||
|
|
||
| final CompletableFuture<HTTPClient.Response> responseFuture = httpClient | ||
| .get("https://api.absmartly.com/v1/context", null, null); | ||
| final HTTPClient.Response response = responseFuture.get(); | ||
|
|
||
| assertEquals(503, response.getStatusCode()); | ||
|
|
||
| verify(asyncHTTPClient, Mockito.timeout(5000).times(1)).execute(any(), any()); | ||
| } | ||
| } |
There was a problem hiding this comment.
testRetryOnTransientError503Response does not test retry behaviour at all.
Because the mock is hooked at the CloseableHttpAsyncClient.execute() level, the DefaultHTTPClientRetryStrategy configured via setRetryStrategy(...) on the builder is never consulted — it lives inside Apache's real async client implementation, which is entirely replaced by the mock. The verify(…).times(1) assertion actually proves the opposite of what the test name implies: execute is called exactly once, confirming zero retries occur.
As written, the test only checks that a 503 SimpleHttpResponse completes the CompletableFuture successfully — identical in effect to the existing get() test with a different status code.
To genuinely test retry logic, DefaultHTTPClientRetryStrategy itself should be exercised in a dedicated unit test, or an integration-style test should drive a real (or closer-to-real) HTTP client against a local stub server.
💡 Minimal rename + assertion adjustment to reflect actual coverage
If the intent is only to assert that 503 passes through the future without throwing (not that retries fire), rename accordingly and drop the misleading config options:
- `@Test`
- void testRetryOnTransientError503Response() throws ExecutionException, InterruptedException {
+ `@Test`
+ void get503ResponsePassesThroughFuture() throws ExecutionException, InterruptedException {
try (final MockedStatic<HttpAsyncClientBuilder> builderStatic = Mockito
.mockStatic(HttpAsyncClientBuilder.class)) {
builderStatic.when(HttpAsyncClientBuilder::create).thenReturn(asyncHTTPClientBuilder);
- final DefaultHTTPClient httpClient = DefaultHTTPClient.create(
- DefaultHTTPClientConfig.create().setMaxRetries(3).setRetryInterval(100));
+ final DefaultHTTPClient httpClient = DefaultHTTPClient.create(DefaultHTTPClientConfig.create());
when(asyncHTTPClient.execute(any(), any())).thenAnswer(invocation -> {
final FutureCallback<SimpleHttpResponse> callback = invocation.getArgument(1);
callback.completed(SimpleHttpResponse.create(503, "Service Unavailable".getBytes(),
ContentType.TEXT_PLAIN));
return null;
});
final CompletableFuture<HTTPClient.Response> responseFuture = httpClient
.get("https://api.absmartly.com/v1/context", null, null);
final HTTPClient.Response response = responseFuture.get();
assertEquals(503, response.getStatusCode());
verify(asyncHTTPClient, Mockito.timeout(5000).times(1)).execute(any(), any());
}
}📝 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 | |
| void testRetryOnTransientError503Response() throws ExecutionException, InterruptedException { | |
| try (final MockedStatic<HttpAsyncClientBuilder> builderStatic = Mockito | |
| .mockStatic(HttpAsyncClientBuilder.class)) { | |
| builderStatic.when(HttpAsyncClientBuilder::create).thenReturn(asyncHTTPClientBuilder); | |
| final DefaultHTTPClient httpClient = DefaultHTTPClient.create( | |
| DefaultHTTPClientConfig.create().setMaxRetries(3).setRetryInterval(100)); | |
| when(asyncHTTPClient.execute(any(), any())).thenAnswer(invocation -> { | |
| final FutureCallback<SimpleHttpResponse> callback = invocation.getArgument(1); | |
| callback.completed(SimpleHttpResponse.create(503, "Service Unavailable".getBytes(), | |
| ContentType.TEXT_PLAIN)); | |
| return null; | |
| }); | |
| final CompletableFuture<HTTPClient.Response> responseFuture = httpClient | |
| .get("https://api.absmartly.com/v1/context", null, null); | |
| final HTTPClient.Response response = responseFuture.get(); | |
| assertEquals(503, response.getStatusCode()); | |
| verify(asyncHTTPClient, Mockito.timeout(5000).times(1)).execute(any(), any()); | |
| } | |
| } | |
| `@Test` | |
| void get503ResponsePassesThroughFuture() throws ExecutionException, InterruptedException { | |
| try (final MockedStatic<HttpAsyncClientBuilder> builderStatic = Mockito | |
| .mockStatic(HttpAsyncClientBuilder.class)) { | |
| builderStatic.when(HttpAsyncClientBuilder::create).thenReturn(asyncHTTPClientBuilder); | |
| final DefaultHTTPClient httpClient = DefaultHTTPClient.create(DefaultHTTPClientConfig.create()); | |
| when(asyncHTTPClient.execute(any(), any())).thenAnswer(invocation -> { | |
| final FutureCallback<SimpleHttpResponse> callback = invocation.getArgument(1); | |
| callback.completed(SimpleHttpResponse.create(503, "Service Unavailable".getBytes(), | |
| ContentType.TEXT_PLAIN)); | |
| return null; | |
| }); | |
| final CompletableFuture<HTTPClient.Response> responseFuture = httpClient | |
| .get("https://api.absmartly.com/v1/context", null, null); | |
| final HTTPClient.Response response = responseFuture.get(); | |
| assertEquals(503, response.getStatusCode()); | |
| verify(asyncHTTPClient, Mockito.timeout(5000).times(1)).execute(any(), any()); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@core-api/src/test/java/com/absmartly/sdk/DefaultHTTPClientTest.java` around
lines 369 - 393, The test method testRetryOnTransientError503Response mistakenly
mocks CloseableHttpAsyncClient.execute so the builder-installed
DefaultHTTPClientRetryStrategy is never exercised; either rename the test to
reflect that it only verifies a 503 response is forwarded (e.g.,
testGetReturns503WithoutThrowing) and remove the misleading retry
config/verify(times(1)), or replace it with a true retry test that does not mock
execute at the client boundary but instead exercises
DefaultHTTPClientRetryStrategy directly (unit test the
DefaultHTTPClientRetryStrategy behavior) or runs an integration-style test
against a local stub server to observe multiple execute invocations; update
assertions and test name accordingly and reference
testRetryOnTransientError503Response, DefaultHTTPClient.create /
DefaultHTTPClientConfig.setMaxRetries / setRetryInterval,
DefaultHTTPClientRetryStrategy, and CloseableHttpAsyncClient.execute to locate
the code to change.
| for (int i = 0; i < threadCount; i++) { | ||
| executor.submit(() -> { | ||
| try { | ||
| startLatch.await(); | ||
| final Integer result = Concurrency.computeIfAbsentRW(rwlock, map, "shared_key", computer); | ||
| assertEquals(42, result); | ||
| } catch (InterruptedException e) { | ||
| Thread.currentThread().interrupt(); | ||
| } finally { | ||
| doneLatch.countDown(); | ||
| } |
There was a problem hiding this comment.
Assertion inside worker thread will be silently swallowed on failure.
The assertEquals(42, result) at line 248 executes inside a Runnable submitted to the executor. If this assertion fails, the AssertionError is caught by the executor and never propagated to the test thread. The test still passes because doneLatch.countDown() runs in the finally block regardless.
To surface failures, collect exceptions or use Future.get():
Proposed fix using Future.get()
+ final List<Future<?>> futures = new ArrayList<>();
for (int i = 0; i < threadCount; i++) {
- executor.submit(() -> {
+ futures.add(executor.submit(() -> {
try {
startLatch.await();
final Integer result = Concurrency.computeIfAbsentRW(rwlock, map, "shared_key", computer);
assertEquals(42, result);
} catch (InterruptedException e) {
Thread.currentThread().interrupt();
} finally {
doneLatch.countDown();
}
- });
+ }));
}
startLatch.countDown();
doneLatch.await();
executor.shutdown();
+ for (Future<?> f : futures) {
+ f.get(); // propagates assertion errors from worker threads
+ }
+
assertEquals(1, map.size());
assertEquals(1, computeCount.get());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@core-api/src/test/java/com/absmartly/sdk/internal/ConcurrencyTest.java`
around lines 243 - 253, The assertion inside the Runnable is swallowed by the
executor; change the worker submission in the test to submit Callables (or
capture Futures) so that you can call Future.get() after doneLatch.await() to
rethrow any AssertionError/Exceptions back to the test thread, or alternatively
collect thrown Throwables from each worker into a concurrent collection and
assert emptiness on the main thread; update the submission around
Concurrency.computeIfAbsentRW(rwlock, map, "shared_key", computer) and ensure
doneLatch.countDown() still runs in finally but failures are surfaced by
checking Futures or the exception collection.
| public ModelAndView showProduct(HttpSession session) { | ||
| final ContextConfig contextConfig = ContextConfig.create() | ||
| .setUnit("session_id", session.getId()); | ||
|
|
||
| final Context context = absmartly.createContext(contextConfig) | ||
| .waitUntilReady(); | ||
|
|
||
| final int treatment = context.getTreatment("exp_product_layout"); | ||
|
|
||
| context.close(); | ||
|
|
||
| ModelAndView mav = new ModelAndView(); | ||
| if (treatment == 0) { | ||
| mav.setViewName("product_control"); | ||
| } else { | ||
| mav.setViewName("product_treatment"); | ||
| } | ||
|
|
||
| return mav; | ||
| } | ||
| } |
There was a problem hiding this comment.
Context is not closed if getTreatment throws an exception.
In the Spring Boot example, context.close() (line 465) is called after getTreatment() (line 463), but if getTreatment() throws, the context will leak. Use try-with-resources since Context implements Closeable:
📝 Suggested improvement
- final Context context = absmartly.createContext(contextConfig)
- .waitUntilReady();
-
- final int treatment = context.getTreatment("exp_product_layout");
-
- context.close();
-
- ModelAndView mav = new ModelAndView();
+ ModelAndView mav = new ModelAndView();
+ try (Context context = absmartly.createContext(contextConfig)
+ .waitUntilReady()) {
+ final int treatment = context.getTreatment("exp_product_layout");
+ if (treatment == 0) {
+ mav.setViewName("product_control");
+ } else {
+ mav.setViewName("product_treatment");
+ }
+ }The same pattern applies to the Jakarta EE example (lines 532–537).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` around lines 456 - 476, The Context is not closed if
Context.getTreatment throws in showProduct; change the code that creates Context
(via ContextConfig.create/setUnit and
absmartly.createContext(...).waitUntilReady()) to use a try-with-resources block
around the Context object (since Context implements Closeable) so that
context.close() is guaranteed even on exceptions; update the showProduct method
accordingly and apply the same try-with-resources pattern to the Jakarta EE
example where Context is created and getTreatment is called.
…ctory Allows creating a fully configured SDK instance in a single call instead of manually creating ClientConfig + ABsmartlyConfig + Client.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core-api/src/main/java/com/absmartly/sdk/ABsmartly.java (1)
88-104:⚠️ Potential issue | 🟠 MajorEnsure scheduler cleanup still happens when client close fails.
If
client_.close()throws IOException on line 89, the scheduler shutdown block is skipped entirely, leaving executor threads running. Wrap client close in a try/catch with scheduler shutdown in a finally block, and rethrow the IOException afterwards.Proposed fix
`@Override` public void close() throws IOException { - if (client_ != null) { - client_.close(); - client_ = null; - } - - if (scheduler_ != null) { - scheduler_.shutdown(); - try { - if (!scheduler_.awaitTermination(5000, TimeUnit.MILLISECONDS)) { - scheduler_.shutdownNow(); - } - } catch (InterruptedException e) { - Thread.currentThread().interrupt(); - scheduler_.shutdownNow(); - } - scheduler_ = null; - } + IOException closeError = null; + try { + if (client_ != null) { + client_.close(); + } + } catch (IOException e) { + closeError = e; + } finally { + client_ = null; + if (scheduler_ != null) { + scheduler_.shutdown(); + try { + if (!scheduler_.awaitTermination(5000, TimeUnit.MILLISECONDS)) { + scheduler_.shutdownNow(); + } + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + scheduler_.shutdownNow(); + } + scheduler_ = null; + } + } + if (closeError != null) { + throw closeError; + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core-api/src/main/java/com/absmartly/sdk/ABsmartly.java` around lines 88 - 104, The client shutdown can throw IOException and currently prevents scheduler cleanup; modify the block around client_.close() so client_.close() is invoked in a try block, catch the IOException to capture it, run the scheduler shutdown/awaitTermination/shutdownNow logic in a finally block (using scheduler_.shutdown(), awaitTermination(...), scheduler_.shutdownNow(), Thread.currentThread().interrupt() on InterruptedException as currently implemented), and after the finally rethrow the original IOException (or rethrow if uncaught) so scheduler_ cleanup always runs; reference client_.close(), scheduler_.shutdown(), scheduler_.awaitTermination(...), and scheduler_.shutdownNow().
♻️ Duplicate comments (2)
README.md (2)
437-457:⚠️ Potential issue | 🟠 MajorContext resource leak if
getTreatmentthrows an exception.In the Spring Boot example,
context.close()is called aftergetTreatment(), but ifgetTreatment()throws an exception, the context will not be closed, causing a resource leak. SinceContextimplementsCloseable, use try-with-resources to guarantee cleanup.📝 Suggested fix
`@GetMapping`("/product") public ModelAndView showProduct(HttpSession session) { - final ContextConfig contextConfig = ContextConfig.create() - .setUnit("session_id", session.getId()); - - final Context context = absmartly.createContext(contextConfig) - .waitUntilReady(); - - final int treatment = context.getTreatment("exp_product_layout"); - - context.close(); - ModelAndView mav = new ModelAndView(); - if (treatment == 0) { - mav.setViewName("product_control"); - } else { - mav.setViewName("product_treatment"); + final ContextConfig contextConfig = ContextConfig.create() + .setUnit("session_id", session.getId()); + + try (Context context = absmartly.createContext(contextConfig) + .waitUntilReady()) { + final int treatment = context.getTreatment("exp_product_layout"); + if (treatment == 0) { + mav.setViewName("product_control"); + } else { + mav.setViewName("product_treatment"); + } } return mav;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 437 - 457, The Context is not guaranteed to be closed if context.getTreatment(...) throws; update showProduct to use try-with-resources: create the Context via absmartly.createContext(ContextConfig.create().setUnit("session_id", session.getId())).waitUntilReady() and wrap that Context in a try(...) block so getTreatment("exp_product_layout") executes inside the try and the Context is automatically closed; remove the explicit context.close() call and keep the rest of the logic that sets the view based on the treatment.
507-523:⚠️ Potential issue | 🟠 MajorContext resource leak if
getTreatmentthrows an exception.The Jakarta EE example has the same resource management issue as the Spring Boot example. If
getTreatment()or any operation beforecontext.close()throws an exception, the context will not be closed. Use try-with-resources to ensure proper cleanup.📝 Suggested fix
`@GET` public Response getProduct() { final String sessionId = request.getSession().getId(); final ContextConfig contextConfig = ContextConfig.create() .setUnit("session_id", sessionId); - final Context context = absmartly.createContext(contextConfig) - .waitUntilReady(); - - final int treatment = context.getTreatment("exp_product_layout"); - - context.close(); - - return Response.ok() - .entity(Map.of("treatment", treatment)) - .build(); + try (Context context = absmartly.createContext(contextConfig) + .waitUntilReady()) { + final int treatment = context.getTreatment("exp_product_layout"); + return Response.ok() + .entity(Map.of("treatment", treatment)) + .build(); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 507 - 523, The getProduct method can leak the Context if absmartly.createContext(...).waitUntilReady() or context.getTreatment(...) throws before context.close() is called; wrap the context lifecycle in a try-with-resources (or a try/finally that always calls context.close()) so the Context is closed even on exceptions. Locate the Context creation and usage in getProduct (the Context context = absmartly.createContext(...).waitUntilReady(); and subsequent context.getTreatment("exp_product_layout") and context.close()) and move them inside a try-with-resources block (or add a finally that calls context.close()) to guarantee cleanup.
🧹 Nitpick comments (2)
README.md (1)
189-189: Remove redundant "first" from "first started".The phrase "first started" is redundant. Consider simplifying to "started" for conciseness.
📝 Suggested improvement
-For long-running contexts, the context is usually created once when the application is first started. +For long-running contexts, the context is usually created once when the application is started.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` at line 189, Edit the sentence "For long-running contexts, the context is usually created once when the application is first started." and remove the redundant word "first" so it reads "For long-running contexts, the context is usually created once when the application is started." This is the only change required—replace "first started" with "started" in the README sentence.core-api/src/test/java/com/absmartly/sdk/ABsmartlyTest.java (1)
109-117: Remove duplicate static verifications for the sameContext.create()invocation.You verify
Context.create(...)twice at each location: first withany()matchers, then again with captors. The captor verification already validates the invocation count and argument values; the first call is redundant. This pattern appears at lines 109–111, 171–173, and 264–266.Proposed simplification (apply to all three test methods)
- contextStatic.verify( - () -> Context.create(any(), any(), any(), any(), any(), any(), any(), any(), any()), - Mockito.times(1)); contextStatic.verify( () -> Context.create(clockCaptor.capture(), configCaptor.capture(), schedulerCaptor.capture(), dataFutureCaptor.capture(), dataProviderCaptor.capture(), eventHandlerCaptor.capture(), eventLoggerCaptor.capture(), variableParserCaptor.capture(), audienceMatcherCaptor.capture()), Mockito.times(1));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core-api/src/test/java/com/absmartly/sdk/ABsmartlyTest.java` around lines 109 - 117, Remove the redundant static verifications of Context.create() that use any() matchers: you should keep only the contextStatic.verify(...) call that uses the captors (clockCaptor, configCaptor, schedulerCaptor, dataFutureCaptor, dataProviderCaptor, eventHandlerCaptor, eventLoggerCaptor, variableParserCaptor, audienceMatcherCaptor) since it already asserts invocation count and captures arguments; delete the duplicate contextStatic.verify(...) calls that pass any() for the same Context.create(...) invocation (they appear alongside the captor-based verifies in the same test methods) so each Context.create(...) is verified only once with the captors.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Around line 656-695: The example uses a non-existent ABsmartly.builder() API;
replace the builder invocation that constructs the SDK (the lines that call
ABsmartly.builder().endpoint(...).apiKey(...).application(...).environment(...).build())
with a direct call to the provided factory method ABsmartly.create(endpoint,
apiKey, application, environment) passing the same endpoint, API key,
application and environment strings so the example uses the correct
ABsmartly.create(...) factory.
---
Outside diff comments:
In `@core-api/src/main/java/com/absmartly/sdk/ABsmartly.java`:
- Around line 88-104: The client shutdown can throw IOException and currently
prevents scheduler cleanup; modify the block around client_.close() so
client_.close() is invoked in a try block, catch the IOException to capture it,
run the scheduler shutdown/awaitTermination/shutdownNow logic in a finally block
(using scheduler_.shutdown(), awaitTermination(...), scheduler_.shutdownNow(),
Thread.currentThread().interrupt() on InterruptedException as currently
implemented), and after the finally rethrow the original IOException (or rethrow
if uncaught) so scheduler_ cleanup always runs; reference client_.close(),
scheduler_.shutdown(), scheduler_.awaitTermination(...), and
scheduler_.shutdownNow().
---
Duplicate comments:
In `@README.md`:
- Around line 437-457: The Context is not guaranteed to be closed if
context.getTreatment(...) throws; update showProduct to use try-with-resources:
create the Context via
absmartly.createContext(ContextConfig.create().setUnit("session_id",
session.getId())).waitUntilReady() and wrap that Context in a try(...) block so
getTreatment("exp_product_layout") executes inside the try and the Context is
automatically closed; remove the explicit context.close() call and keep the rest
of the logic that sets the view based on the treatment.
- Around line 507-523: The getProduct method can leak the Context if
absmartly.createContext(...).waitUntilReady() or context.getTreatment(...)
throws before context.close() is called; wrap the context lifecycle in a
try-with-resources (or a try/finally that always calls context.close()) so the
Context is closed even on exceptions. Locate the Context creation and usage in
getProduct (the Context context = absmartly.createContext(...).waitUntilReady();
and subsequent context.getTreatment("exp_product_layout") and context.close())
and move them inside a try-with-resources block (or add a finally that calls
context.close()) to guarantee cleanup.
---
Nitpick comments:
In `@core-api/src/test/java/com/absmartly/sdk/ABsmartlyTest.java`:
- Around line 109-117: Remove the redundant static verifications of
Context.create() that use any() matchers: you should keep only the
contextStatic.verify(...) call that uses the captors (clockCaptor, configCaptor,
schedulerCaptor, dataFutureCaptor, dataProviderCaptor, eventHandlerCaptor,
eventLoggerCaptor, variableParserCaptor, audienceMatcherCaptor) since it already
asserts invocation count and captures arguments; delete the duplicate
contextStatic.verify(...) calls that pass any() for the same Context.create(...)
invocation (they appear alongside the captor-based verifies in the same test
methods) so each Context.create(...) is verified only once with the captors.
In `@README.md`:
- Line 189: Edit the sentence "For long-running contexts, the context is usually
created once when the application is first started." and remove the redundant
word "first" so it reads "For long-running contexts, the context is usually
created once when the application is started." This is the only change
required—replace "first started" with "started" in the README sentence.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
README.mdcore-api/src/main/java/com/absmartly/sdk/ABsmartly.javacore-api/src/test/java/com/absmartly/sdk/ABsmartlyTest.java
| public class CancellableContextExample { | ||
|
|
||
| public static void main(String[] args) throws Exception { | ||
| final ABsmartly sdk = ABsmartly.builder() | ||
| .endpoint("https://your-company.absmartly.io/v1") | ||
| .apiKey("YOUR-API-KEY") | ||
| .application("website") | ||
| .environment("development") | ||
| .build(); | ||
|
|
||
| final ContextConfig contextConfig = ContextConfig.create() | ||
| .setUnit("session_id", "abc123"); | ||
|
|
||
| final Context context = sdk.createContext(contextConfig); | ||
|
|
||
| // Create future for context initialization | ||
| final CompletableFuture<Context> future = context.waitUntilReadyAsync(); | ||
|
|
||
| // Cancel after 1.5 seconds if not ready | ||
| final ScheduledExecutorService scheduler = Executors.newScheduledThreadPool(1); | ||
| scheduler.schedule(() -> { | ||
| if (!future.isDone()) { | ||
| future.cancel(true); | ||
| System.out.println("Context creation cancelled"); | ||
| } | ||
| }, 1500, TimeUnit.MILLISECONDS); | ||
|
|
||
| try { | ||
| final Context readyContext = future.get(); | ||
| System.out.println("Context ready!"); | ||
| readyContext.close(); | ||
| } catch (CancellationException e) { | ||
| System.out.println("Context creation was cancelled"); | ||
| } catch (ExecutionException e) { | ||
| System.out.println("Context creation failed: " + e.getCause()); | ||
| } finally { | ||
| scheduler.shutdown(); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find ABsmartly.java file
fd -t f 'ABsmartly\.java' -x echo {}Repository: absmartly/java-sdk
Length of output: 119
🏁 Script executed:
# Search for builder method in Java files
rg -n 'builder\s*\(' --type=java -C 3Repository: absmartly/java-sdk
Length of output: 44
🏁 Script executed:
# Read the ABsmartly.java file to check actual API
cat ./core-api/src/main/java/com/absmartly/sdk/ABsmartly.javaRepository: absmartly/java-sdk
Length of output: 3652
Update the example to use the correct ABsmartly.create() factory method instead of the non-existent builder() API.
The ABsmartly class does not have a builder() method. The actual public factory methods are:
create(@nonnullABsmartlyConfig config)create(@nonnullString endpoint,@nonnullString apiKey,@nonnullString application,@nonnullString environment)
Replace the builder pattern (lines 659–664) with a direct call to the convenience factory: ABsmartly.create(endpoint, apiKey, application, environment).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` around lines 656 - 695, The example uses a non-existent
ABsmartly.builder() API; replace the builder invocation that constructs the SDK
(the lines that call
ABsmartly.builder().endpoint(...).apiKey(...).application(...).environment(...).build())
with a direct call to the provided factory method ABsmartly.create(endpoint,
apiKey, application, environment) passing the same endpoint, API key,
application and environment strings so the example uses the correct
ABsmartly.create(...) factory.
The positional create(endpoint, apiKey, app, env) required users to know parameter order. Builder pattern provides named parameters: ABsmartly.builder().endpoint(...).apiKey(...).application(...).environment(...).build()
There was a problem hiding this comment.
♻️ Duplicate comments (2)
README.md (2)
442-447:⚠️ Potential issue | 🟡 MinorContext is not closed if
getTreatmentthrows an exception.In the Spring Boot example,
context.close()(line 447) is called aftergetTreatment()(line 445), but ifgetTreatment()throws, the context will leak. Use try-with-resources sinceContextimplementsCloseable.🛡️ Proposed fix using try-with-resources
- final Context context = absmartly.createContext(contextConfig) - .waitUntilReady(); - - final int treatment = context.getTreatment("exp_product_layout"); - - context.close(); - - ModelAndView mav = new ModelAndView(); + ModelAndView mav = new ModelAndView(); + try (Context context = absmartly.createContext(contextConfig) + .waitUntilReady()) { + final int treatment = context.getTreatment("exp_product_layout"); + if (treatment == 0) { + mav.setViewName("product_control"); + } else { + mav.setViewName("product_treatment"); + } + } + return mav;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 442 - 447, The Context may leak if context.getTreatment("exp_product_layout") throws; change the creation and closing to use try-with-resources around the Context returned by absmartly.createContext(contextConfig).waitUntilReady() so it is always closed; locate the block that calls absmartly.createContext(...).waitUntilReady(), the subsequent call to context.getTreatment(...) and the trailing context.close() and replace with a try-with-resources that declares Context context = absmartly.createContext(contextConfig).waitUntilReady() so close happens automatically.
514-519:⚠️ Potential issue | 🟡 MinorSame context leak issue applies to the Jakarta EE example.
The
context.close()call on line 519 won't execute ifgetTreatment()on line 517 throws. Apply the same try-with-resources pattern here.🛡️ Proposed fix using try-with-resources
- final Context context = absmartly.createContext(contextConfig) - .waitUntilReady(); - - final int treatment = context.getTreatment("exp_product_layout"); - - context.close(); - - return Response.ok() - .entity(Map.of("treatment", treatment)) - .build(); + try (Context context = absmartly.createContext(contextConfig) + .waitUntilReady()) { + final int treatment = context.getTreatment("exp_product_layout"); + return Response.ok() + .entity(Map.of("treatment", treatment)) + .build(); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 514 - 519, The Jakarta EE example leaks Context because context.close() is never reached if context.getTreatment("exp_product_layout") throws; change the pattern around absmartly.createContext(contextConfig).waitUntilReady() so Context is acquired in a try-with-resources (or try (Context context = ...) { ... }) block and call getTreatment inside that block, ensuring the Context is always closed; update references to Context, absmartly.createContext, getTreatment, and context.close accordingly.
🧹 Nitpick comments (4)
README.md (1)
190-190: Minor style improvement: "first started" is redundant.Consider simplifying to just "started" for cleaner prose.
📝 Suggested change
-For long-running contexts, the context is usually created once when the application is first started. +For long-running contexts, the context is usually created once when the application is started.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` at line 190, The sentence "For long-running contexts, the context is usually created once when the application is first started." is wordy — replace "first started" with "started" so it reads: "For long-running contexts, the context is usually created once when the application is started." Update that exact line in README.md to simplify the phrasing.core-api/src/main/java/com/absmartly/sdk/ABsmartly.java (2)
58-62: Consider validating for empty strings in addition to null.The Builder validates that required fields are not null, but the
Clientconstructor (as seen inClient.javalines 28-56) validates that these same fields are neither null nor empty. This inconsistency means users could pass empty strings through the Builder, only to get a differentIllegalArgumentExceptionfrom the Client with a less helpful message.♻️ Proposed fix to add empty string validation
public ABsmartly build() { - if (endpoint == null) throw new IllegalArgumentException("endpoint is required"); - if (apiKey == null) throw new IllegalArgumentException("apiKey is required"); - if (application == null) throw new IllegalArgumentException("application is required"); - if (environment == null) throw new IllegalArgumentException("environment is required"); + if (endpoint == null || endpoint.isEmpty()) throw new IllegalArgumentException("endpoint is required"); + if (apiKey == null || apiKey.isEmpty()) throw new IllegalArgumentException("apiKey is required"); + if (application == null || application.isEmpty()) throw new IllegalArgumentException("application is required"); + if (environment == null || environment.isEmpty()) throw new IllegalArgumentException("environment is required");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core-api/src/main/java/com/absmartly/sdk/ABsmartly.java` around lines 58 - 62, In ABsmartly.build(), extend the null checks to also reject empty or all-whitespace strings for endpoint, apiKey, application, and environment (e.g., using isBlank()/trim().isEmpty()) and throw IllegalArgumentException with the same messages currently used ("endpoint is required", "apiKey is required", "application is required", "environment is required"); update the build() method in class ABsmartly (the build() function) to validate each of those four fields accordingly before constructing the Client.
135-138: Minor: Consider nullifyingclient_only after successful close.If
client_.close()throws anIOException, the reference is still nullified, which could mask issues on retry attempts. However, sinceclose()is typically called once during teardown, this is a minor concern.♻️ Proposed safer nullification
if (client_ != null) { - client_.close(); - client_ = null; + try { + client_.close(); + } finally { + client_ = null; + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core-api/src/main/java/com/absmartly/sdk/ABsmartly.java` around lines 135 - 138, In ABsmartly.java, avoid nullifying client_ before confirming close() succeeded: wrap the client_.close() call in a try-catch and only set client_ = null after close() completes successfully (e.g., try { client_.close(); client_ = null; } catch (IOException e) { /* log/handle and keep client_ for potential retry */ }), referencing the client_ field and its close() method so exceptions don’t cause the reference to be lost.core-api/src/test/java/com/absmartly/sdk/ABsmartlyTest.java (1)
333-356: Consider adding tests for scheduler shutdown edge cases.The
close()test verifies thatawaitTerminationis called, but doesn't cover the fallback paths in the implementation:
- When
awaitTerminationreturnsfalse(timeout)- When
awaitTerminationthrowsInterruptedExceptionThese paths invoke
shutdownNow()and should ideally be tested.🧪 Example test for shutdown timeout fallback
`@Test` void closeCallsShutdownNowOnTimeout() throws IOException, InterruptedException { final ScheduledExecutorService scheduler = mock(ScheduledExecutorService.class); when(scheduler.awaitTermination(anyLong(), any())).thenReturn(false); final ABsmartlyConfig config = ABsmartlyConfig.create() .setClient(client) .setScheduler(scheduler); final ABsmartly absmartly = ABsmartly.create(config); absmartly.close(); verify(scheduler, times(1)).shutdown(); verify(scheduler, times(1)).shutdownNow(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core-api/src/test/java/com/absmartly/sdk/ABsmartlyTest.java` around lines 333 - 356, Add unit tests in ABsmartlyTest to cover the scheduler shutdown fallback paths: create a mock ScheduledExecutorService, inject it via ABsmartlyConfig into ABsmartly.create(), then call absmartly.close() while stubbing scheduler.awaitTermination(...) to (a) return false and (b) throw InterruptedException; for the timeout case verify scheduler.shutdown() and scheduler.shutdownNow() are each called, and for the InterruptedException case verify shutdown() and shutdownNow() are called and that the thread interruption is handled/restored as appropriate. Use the existing test pattern (mocking Context.create with MockedStatic<Context>) and reference the same symbols: ABsmartly.create(config), absmartly.close(), and the mock scheduler in your new tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@README.md`:
- Around line 442-447: The Context may leak if
context.getTreatment("exp_product_layout") throws; change the creation and
closing to use try-with-resources around the Context returned by
absmartly.createContext(contextConfig).waitUntilReady() so it is always closed;
locate the block that calls absmartly.createContext(...).waitUntilReady(), the
subsequent call to context.getTreatment(...) and the trailing context.close()
and replace with a try-with-resources that declares Context context =
absmartly.createContext(contextConfig).waitUntilReady() so close happens
automatically.
- Around line 514-519: The Jakarta EE example leaks Context because
context.close() is never reached if context.getTreatment("exp_product_layout")
throws; change the pattern around
absmartly.createContext(contextConfig).waitUntilReady() so Context is acquired
in a try-with-resources (or try (Context context = ...) { ... }) block and call
getTreatment inside that block, ensuring the Context is always closed; update
references to Context, absmartly.createContext, getTreatment, and context.close
accordingly.
---
Nitpick comments:
In `@core-api/src/main/java/com/absmartly/sdk/ABsmartly.java`:
- Around line 58-62: In ABsmartly.build(), extend the null checks to also reject
empty or all-whitespace strings for endpoint, apiKey, application, and
environment (e.g., using isBlank()/trim().isEmpty()) and throw
IllegalArgumentException with the same messages currently used ("endpoint is
required", "apiKey is required", "application is required", "environment is
required"); update the build() method in class ABsmartly (the build() function)
to validate each of those four fields accordingly before constructing the
Client.
- Around line 135-138: In ABsmartly.java, avoid nullifying client_ before
confirming close() succeeded: wrap the client_.close() call in a try-catch and
only set client_ = null after close() completes successfully (e.g., try {
client_.close(); client_ = null; } catch (IOException e) { /* log/handle and
keep client_ for potential retry */ }), referencing the client_ field and its
close() method so exceptions don’t cause the reference to be lost.
In `@core-api/src/test/java/com/absmartly/sdk/ABsmartlyTest.java`:
- Around line 333-356: Add unit tests in ABsmartlyTest to cover the scheduler
shutdown fallback paths: create a mock ScheduledExecutorService, inject it via
ABsmartlyConfig into ABsmartly.create(), then call absmartly.close() while
stubbing scheduler.awaitTermination(...) to (a) return false and (b) throw
InterruptedException; for the timeout case verify scheduler.shutdown() and
scheduler.shutdownNow() are each called, and for the InterruptedException case
verify shutdown() and shutdownNow() are called and that the thread interruption
is handled/restored as appropriate. Use the existing test pattern (mocking
Context.create with MockedStatic<Context>) and reference the same symbols:
ABsmartly.create(config), absmartly.close(), and the mock scheduler in your new
tests.
In `@README.md`:
- Line 190: The sentence "For long-running contexts, the context is usually
created once when the application is first started." is wordy — replace "first
started" with "started" so it reads: "For long-running contexts, the context is
usually created once when the application is started." Update that exact line in
README.md to simplify the phrasing.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
README.mdcore-api/src/main/java/com/absmartly/sdk/ABsmartly.javacore-api/src/test/java/com/absmartly/sdk/ABsmartlyTest.java
1 participant
Summary
Critical Bug Fixes
1. Deadlock in Context.flush()
Context.flush()held a read lock (contextLock_.readLock()) and then calledgetUnitHash()which tried to acquire a write lock viacomputeIfAbsentRW(). SinceReentrantReadWriteLockdoes not allow upgrading from read to write, this caused a permanent deadlock. Fixed by using write lock directly inflush().2. AudienceMatcher crash on empty/null audience
evaluate("")tried to deserialize empty bytes, throwingMismatchedInputException. Added null/empty guard that returns null.3. Deserializer error handling
DefaultAudienceDeserializer,DefaultContextDataDeserializer,DefaultContextEventSerializer, andDefaultVariableParserall threwIllegalStateExceptionon bad input. Changed to return null, matching the SDK specification.4. Double error logging in closeAsync()
logError()was called in bothflush()error handler andcloseAsync()error handler. Removed the redundant call.5. Additional fixes
shutdownNow()fallbackTest Results
Test plan
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores
Tests