Skip to content

chore: Convert JSON LDValue into the AiConfig Object #61

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 12 commits into
base: lc/SDK-1192/AI-config-feature-branch
Choose a base branch
from

Conversation

louis-launchdarkly
Copy link

@louis-launchdarkly louis-launchdarkly commented May 8, 2025

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

  • Implemented builder pattern for all classes in the datamodel directory
  • Made all constructors package-private
  • Updated LDAiClient to use the builder pattern
  • Made fields final for immutability
  • All tests are passing

Link to Devin run

https://app.devin.ai/sessions/f1057ea9206643789c93d45aa8c5c500

Requested by: lchan@launchdarkly.com

@louis-launchdarkly louis-launchdarkly requested a review from a team as a code owner May 8, 2025 22:15
Base automatically changed from lc/SDK-1192/setup-new-project to lc/SDK-1192/AI-config-feature-branch May 8, 2025 22:16
@louis-launchdarkly
Copy link
Author

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 ./gradlew build right now.

Copy link
Member

@kinyoklion kinyoklion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestions in comments.

@louis-launchdarkly louis-launchdarkly force-pushed the lc/SDK-1192/setup-ai-config-parsing branch from f442874 to 877d4e2 Compare May 15, 2025 22:30
@louis-launchdarkly
Copy link
Author

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.

tanderson-ld
tanderson-ld previously approved these changes May 20, 2025
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add nullable and nonnull annotations on fields to help with consumption.

import javax.annotation.Nonnull;
import javax.annotation.Nullable;


protected <T> T ldValueNullCheck(T ldValue) throws AiConfigParseException {
if (ldValue == LDValue.ofNull()) {
throw new AiConfigParseException("Unexpected Null value for non-optional field");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably helpful to add field name.

…ructors package-private

Co-Authored-By: lchan@launchdarkly.com <lchan@launchdarkly.com>
@tanderson-ld tanderson-ld dismissed their stale review May 22, 2025 13:58

More commits made.

@tanderson-ld tanderson-ld self-requested a review May 22, 2025 13:58
@louis-launchdarkly
Copy link
Author

Also, I realized using Ai is not to spec - in the future PR I will update everything to say AI instead.

@kinyoklion
Copy link
Member

kinyoklion commented May 28, 2025

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# AI and Html.

Java isn't as concrete, but we should be consistent.
It appears we use Id, so we probably should use Ai.

DiagnosticId
getId

Generally we call this out in the spec.
"adjusted to be language and SDK appropriate"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants