chore: Convert JSON LDValue into the AiConfig Object#61
chore: Convert JSON LDValue into the AiConfig Object#61louis-launchdarkly wants to merge 12 commits intolc/SDK-1192/AI-config-feature-branchfrom
Conversation
|
I just realized I also need to copy the .github file from another project in this mono-repo to have the test run in CI. I am running the test locally via |
lib/sdk/server-ai/src/main/java/com/launchdarkly/sdk/server/ai/datamodel/AiConfig.java
Outdated
Show resolved
Hide resolved
lib/sdk/server-ai/src/main/java/com/launchdarkly/sdk/server/ai/LDAiClient.java
Show resolved
Hide resolved
lib/sdk/server-ai/src/main/java/com/launchdarkly/sdk/server/ai/LDAiClient.java
Outdated
Show resolved
Hide resolved
lib/sdk/server-ai/src/main/java/com/launchdarkly/sdk/server/ai/LDAiClient.java
Outdated
Show resolved
Hide resolved
lib/sdk/server-ai/src/main/java/com/launchdarkly/sdk/server/ai/LDAiClient.java
Outdated
Show resolved
Hide resolved
lib/sdk/server-ai/src/main/java/com/launchdarkly/sdk/server/ai/LDAiClient.java
Outdated
Show resolved
Hide resolved
f442874 to
877d4e2
Compare
Co-Authored-By: lchan@launchdarkly.com <lchan@launchdarkly.com>
Co-Authored-By: lchan@launchdarkly.com <lchan@launchdarkly.com>
|
I think the only optional change I may make in this PR is to use the builder pattern for the AiConfig. The changes from Devin and me addressed Ryan's feedback, added more comprehensive unit tests and is ready for review. I will do the other methods (Actual AiConfig Feature Flag evaluation, LDAIConfigTracker) in separate PRs. |
There was a problem hiding this comment.
Add nullable and nonnull annotations on fields to help with consumption.
import javax.annotation.Nonnull;
import javax.annotation.Nullable;
lib/sdk/server-ai/src/test/java/com/launchdarkly/sdk/server/ai/LDAiClientTest.java
Show resolved
Hide resolved
|
|
||
| protected <T> T ldValueNullCheck(T ldValue) throws AiConfigParseException { | ||
| if (ldValue == LDValue.ofNull()) { | ||
| throw new AiConfigParseException("Unexpected Null value for non-optional field"); |
There was a problem hiding this comment.
Probably helpful to add field name.
…ructors package-private Co-Authored-By: lchan@launchdarkly.com <lchan@launchdarkly.com>
|
Also, I realized using Ai is not to spec - in the future PR I will update everything to say AI instead. |
Generally speaking it would be using whatever is idiomatic to the language. Some languages have convenient standards. Like two letter initialisms are uppercase, but anything else follows the normal standard. So in C# Java isn't as concrete, but we should be consistent.
Generally we call this out in the spec. |
|
bugbot review |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 5 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
| if (checkValueWithFailureLogging(valueMessages, LDValueType.ARRAY, logger, | ||
| "messages if exists must be a JSON array")) { | ||
| messages = new ArrayList<Message>(); | ||
| valueMessages.valuesAs(new Message.MessageConverter()); |
There was a problem hiding this comment.
Discarded valuesAs result is dead code
Low Severity
The first call to valueMessages.valuesAs(new Message.MessageConverter()) on this line creates a new MessageConverter and iterates the array, but the result is completely discarded. The actual conversion happens on the very next line in the for-each loop. This is dead code that wastes an allocation and iteration.
|
|
||
| LDValue valueModel = value.get("model"); | ||
| if (checkValueWithFailureLogging(valueModel, LDValueType.OBJECT, logger, | ||
| "model if exists must be a JSON object")) { |
There was a problem hiding this comment.
Optional fields log errors when simply absent
Medium Severity
When optional fields like model, messages, or provider are absent from the JSON, LDValue.get() returns LDValue.ofNull() (type NULL). checkValueWithFailureLogging then logs an error because NULL != OBJECT. This causes spurious error logs in production for perfectly valid configurations that omit optional fields. Tests don't catch this because the logger is always null.
Additional Locations (2)
|
|
||
| meta = Meta.builder() | ||
| .variationKey(variationKey) | ||
| .version(Optional.of(valueMeta.get("version").intValue())) |
There was a problem hiding this comment.
Version is never Optional.empty() when absent
Medium Severity
valueMeta.get("version").intValue() returns 0 when the version key is absent (since LDValue.ofNull().intValue() returns 0). Wrapping this in Optional.of(...) means the version is always present — it's Optional.of(0) instead of Optional.empty() when the field is missing. The Optional<Integer> type on Meta.version is thus misleading and never empty during parsing.
| default: | ||
| return null; | ||
| } | ||
| } |
There was a problem hiding this comment.
Role.getRole(null) throws NullPointerException in switch
Medium Severity
Role.getRole uses a switch on a String parameter, which throws NullPointerException when null is passed. In MessageConverter.toType, if a message JSON is missing the role field, ldValue.get("role").stringValue() returns null, which is then passed to Role.getRole(null), causing a crash during message parsing.
Additional Locations (1)
| this.model = model; | ||
| this.messages = messages; | ||
| this.provider = provider; | ||
| } |
There was a problem hiding this comment.
Messages list not defensively copied, breaking immutability
High Severity
The AiConfig constructor stores the messages list by direct reference without a defensive copy or Collections.unmodifiableList() wrapper. This breaks the immutability guarantee that the PR explicitly requires (per reviewer discussion). In contrast, Model correctly wraps its maps with Collections.unmodifiableMap(new HashMap<>(...)). Callers can mutate the list after building AiConfig, or via getMessages(), leading to the exact race condition the reviewer warned about — writing a default value for an event while the customer mutates the config.


Convert JSON LDValue into the AiConfig Object
For ease of discussion, implement only the conversion logic from LDValue (that we will get back from the Feature Flag SDK) to the AiConfig DataModel.
This uses the exception-based short circuiting (similar to https://github.com/launchdarkly/dotnet-core/blob/6774539af1a2f87b96ae3e647fdcdc5663c791ab/pkgs/sdk/server-ai/src/LdAiClient.cs#L180) and LDValue reading logic similar to https://github.com/launchdarkly/python-server-sdk-ai/blob/main/ldai/client.py#L134
Updates
Link to Devin run
https://app.devin.ai/sessions/f1057ea9206643789c93d45aa8c5c500
Requested by: lchan@launchdarkly.com
Note
Medium Risk
Introduces new JSON parsing and null-handling behavior that will affect how AI configs are interpreted at runtime; errors could lead to silently missing fields or disabled configs. CI workflow addition is low risk.
Overview
Adds a dedicated GitHub Actions workflow to build/test the
lib/sdk/server-aiworkspace on pushes/PRs.Implements
LDAiClient.parseAiConfigto parse anLDValueJSON object into a new immutableAiConfigdatamodel (Meta,Model,Message,Provider), including type-checking with error logging and null/disabled fallbacks for malformed inputs. UpdatesRolestring serialization/parsing and adds comprehensive JUnit coverage for valid/invalid JSON cases.Written by Cursor Bugbot for commit 5244939. This will update automatically on new commits. Configure here.