Skip to content

Commit ba1597a

Browse files
louis-launchdarklylouis-launchdarkly
committed
chore: Address code review comments with type guards
1 parent ded3e63 commit ba1597a

File tree

4 files changed

+113
-113
lines changed

4 files changed

+113
-113
lines changed

‎lib/sdk/server-ai/src/main/java/com/launchdarkly/sdk/server/ai/LDAiClient.java‎

Lines changed: 88 additions & 85 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
11
package com.launchdarkly.sdk.server.ai;
22

3+
import static java.util.Arrays.binarySearch;
4+
35
import java.util.ArrayList;
46
import java.util.HashMap;
57
import java.util.List;
@@ -43,113 +45,114 @@ public LDAiClient(LDClientInterface client) {
4345

4446
/**
4547
* Method to convert the JSON variable into the AiConfig object
48+
*
49+
* If the parsing failed, the code will log an error and
50+
* return a well formed but with nullable value nulled and disabled AIConfig
51+
*
52+
* Doing all the error checks, so if somehow LD backend return incorrect value types, there is logging
53+
* This also opens up the possibility of allowing customer to build this using a JSON string in the future
4654
*
4755
* @param value
4856
* @param key
4957
*/
5058
protected AiConfig parseAiConfig(LDValue value, String key) {
51-
AiConfig result = new AiConfig();
52-
53-
try {
54-
// Verify the whole value is a JSON object
55-
if (value == null || value.getType() != LDValueType.OBJECT) {
56-
throw new AiConfigParseException("Input to parseAiConfig must be a JSON object");
57-
}
58-
59-
// Convert the _meta JSON object into Meta
60-
LDValue valueMeta = value.get("_ldMeta");
61-
if (valueMeta == LDValue.ofNull() || valueMeta.getType() != LDValueType.OBJECT) {
62-
throw new AiConfigParseException("_ldMeta must be a JSON object");
63-
}
59+
boolean enabled = false;
6460

65-
// Create Meta using constructor
66-
Meta meta = new Meta(
67-
ldValueNullCheck(valueMeta.get("variationKey")).stringValue(),
68-
Optional.of(valueMeta.get("version").intValue()),
69-
ldValueNullCheck(valueMeta.get("enabled")).booleanValue()
70-
);
71-
result.setMeta(meta);
72-
73-
// Convert the optional messages from an JSON array of JSON objects into Message
74-
// Q: Does it even make sense to have 0 messages?
75-
LDValue valueMessages = value.get("messages");
76-
if (valueMeta == LDValue.ofNull() || valueMessages.getType() != LDValueType.ARRAY) {
77-
throw new AiConfigParseException("messages must be a JSON array");
78-
}
79-
80-
List<Message> messages = new ArrayList<Message>();
81-
valueMessages.valuesAs(new Message.MessageConverter());
82-
for (Message message : valueMessages.valuesAs(new Message.MessageConverter())) {
83-
messages.add(message);
84-
}
85-
result.setMessages(messages);
61+
// Verify the whole value is a JSON object
62+
if(!checkValueWithFailureLogging(value, LDValueType.OBJECT, logger, "Input to parseAiConfig must be a JSON object")) {
63+
return new AiConfig(enabled, null, null, null, null);
64+
}
8665

87-
// Convert the optional model from an JSON object of with parameters and custom
88-
// into Model
89-
LDValue valueModel = value.get("model");
90-
if (valueModel == LDValue.ofNull() || valueModel.getType() != LDValueType.OBJECT) {
91-
throw new AiConfigParseException("model must be a JSON object");
92-
}
66+
// Convert the _meta JSON object into Meta
67+
LDValue valueMeta = value.get("_ldMeta");
68+
if (!checkValueWithFailureLogging(valueMeta, LDValueType.OBJECT, logger, "_ldMeta must be a JSON object")) {
69+
// Q: If we can't read _meta, enabled by spec would be defaulted to false. Does it even matter the rest of the values?
70+
return new AiConfig(enabled, null, null, null, null);
71+
}
9372

94-
// Prepare parameters and custom maps for Model
95-
String modelName = ldValueNullCheck(valueModel.get("name")).stringValue();
96-
HashMap<String, LDValue> parameters = null;
97-
HashMap<String, LDValue> custom = null;
73+
// The booleanValue will get false if that value something that we are not expecting, which is good
74+
enabled = valueMeta.get("enabled").booleanValue();
9875

99-
LDValue valueParameters = valueModel.get("parameters");
100-
if (valueParameters.getType() != LDValueType.NULL) {
101-
if (valueParameters.getType() != LDValueType.OBJECT) {
102-
throw new AiConfigParseException("non-null parameters must be a JSON object");
76+
String variationKey = null;
77+
if (checkValueWithFailureLogging(valueMeta.get("variationKey"), LDValueType.STRING, logger, "variationKey should be a string")) {
78+
variationKey = valueMeta.get("variationKey").stringValue();
79+
}
80+
// Create Meta using constructor
81+
Meta meta = new Meta(
82+
variationKey,
83+
Optional.of(valueMeta.get("version").intValue())
84+
);
85+
86+
// Convert the optional model from an JSON object of with parameters and custom
87+
// into Model
88+
Model model = null;
89+
90+
LDValue valueModel = value.get("model");
91+
if (checkValueWithFailureLogging(valueModel, LDValueType.OBJECT, logger, "model if exists must be a JSON object")) {
92+
if (checkValueWithFailureLogging(valueModel.get("name"), LDValueType.STRING, logger, "model name must be a string and is required")) {
93+
String modelName = valueModel.get("name").stringValue();
94+
95+
// Prepare parameters and custom maps for Model
96+
HashMap<String, LDValue> parameters = null;
97+
HashMap<String, LDValue> custom = null;
98+
99+
LDValue valueParameters = valueModel.get("parameters");
100+
if (checkValueWithFailureLogging(valueParameters, LDValueType.OBJECT, logger, "non-null parameters must be a JSON object")) {
101+
parameters = new HashMap<>();
102+
for (String k : valueParameters.keys()) {
103+
parameters.put(k, valueParameters.get(k));
104+
}
103105
}
104-
105-
parameters = new HashMap<>();
106-
for (String k : valueParameters.keys()) {
107-
parameters.put(k, valueParameters.get(k));
106+
107+
LDValue valueCustom = valueModel.get("custom");
108+
if (checkValueWithFailureLogging(valueCustom, LDValueType.OBJECT, logger, "non-null custom must be a JSON object")) {
109+
110+
custom = new HashMap<>();
111+
for (String k : valueCustom.keys()) {
112+
custom.put(k, valueCustom.get(k));
113+
}
108114
}
115+
116+
model = new Model(modelName, parameters, custom);
109117
}
118+
}
110119

111-
LDValue valueCustom = valueModel.get("custom");
112-
if (valueCustom.getType() != LDValueType.NULL) {
113-
if (valueCustom.getType() != LDValueType.OBJECT) {
114-
throw new AiConfigParseException("non-null custom must be a JSON object");
115-
}
120+
// Convert the optional messages from an JSON array of JSON objects into Message
121+
// Q: Does it even make sense to have 0 messages?
122+
List<Message> messages = null;
116123

117-
custom = new HashMap<>();
118-
for (String k : valueCustom.keys()) {
119-
custom.put(k, valueCustom.get(k));
120-
}
124+
LDValue valueMessages = value.get("messages");
125+
if (checkValueWithFailureLogging(valueMessages, LDValueType.ARRAY, logger, "messages if exists must be a JSON array")) {
126+
messages = new ArrayList<Message>();
127+
valueMessages.valuesAs(new Message.MessageConverter());
128+
for (Message message : valueMessages.valuesAs(new Message.MessageConverter())) {
129+
messages.add(message);
121130
}
122-
123-
// Create Model using constructor
124-
Model model = new Model(modelName, parameters, custom);
125-
result.setModel(model);
126-
127-
// Convert the optional provider from an JSON object of with name into Provider
128-
LDValue valueProvider = value.get("provider");
129-
if (valueProvider.getType() != LDValueType.NULL) {
130-
if (valueProvider.getType() != LDValueType.OBJECT) {
131-
throw new AiConfigParseException("non-null provider must be a JSON object");
132-
}
131+
}
133132

134-
Provider provider = new Provider(ldValueNullCheck(valueProvider.get("name")).stringValue());
135-
result.setProvider(provider);
136-
} else {
137-
// Provider is optional - we can just set null and proceed
138-
result.setProvider(null);
133+
// Convert the optional provider from an JSON object of with name into Provider
134+
LDValue valueProvider = value.get("provider");
135+
String providerName = null;
136+
137+
if(checkValueWithFailureLogging(valueProvider, LDValueType.OBJECT, logger, "non-null provider must be a JSON object")) {
138+
if(checkValueWithFailureLogging(valueProvider.get("name"), LDValueType.STRING, logger, "provider name must be a String")) {
139+
providerName = valueProvider.get("name").stringValue();
139140
}
140-
} catch (AiConfigParseException e) {
141-
// logger.error(e.getMessage());
142-
return null;
143141
}
144142

145-
return result;
143+
Provider provider = new Provider(providerName);
144+
145+
return new AiConfig(enabled, meta, model, messages, provider);
146146
}
147147

148-
protected <T> T ldValueNullCheck(T ldValue) throws AiConfigParseException {
149-
if (ldValue == LDValue.ofNull()) {
150-
throw new AiConfigParseException("Unexpected Null value for non-optional field");
148+
protected boolean checkValueWithFailureLogging(LDValue ldValue, LDValueType expectedType, LDLogger logger, String message) {
149+
if (ldValue.getType() != expectedType) {
150+
if (logger != null) {
151+
logger.error(message);
152+
}
153+
return false;
151154
}
152-
return ldValue;
155+
return true;
153156
}
154157

155158
class AiConfigParseException extends Exception {

‎lib/sdk/server-ai/src/main/java/com/launchdarkly/sdk/server/ai/datamodel/AiConfig.java‎

Lines changed: 18 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -3,43 +3,41 @@
33
import java.util.List;
44

55
public final class AiConfig {
6-
private List<Message> messages;
6+
private final boolean enabled;
77

8-
private Meta meta;
8+
private final Meta meta;
9+
10+
private final Model model;
11+
12+
private final List<Message> messages;
913

10-
private Model model;
14+
private final Provider provider;
1115

12-
private Provider provider;
16+
public AiConfig(boolean enabled, Meta meta, Model model, List<Message> messages, Provider provider) {
17+
this.enabled = enabled;
18+
this.meta = meta;
19+
this.model = model;
20+
this.messages = messages;
21+
this.provider = provider;
22+
}
1323

14-
public List<Message> getMessages() {
15-
return messages;
24+
public boolean isEnabled() {
25+
return enabled;
1626
}
1727

18-
public void setMessages(List<Message> messages) {
19-
this.messages = messages;
28+
public List<Message> getMessages() {
29+
return messages;
2030
}
2131

2232
public Meta getMeta() {
2333
return meta;
2434
}
2535

26-
public void setMeta(Meta meta) {
27-
this.meta = meta;
28-
}
29-
3036
public Model getModel() {
3137
return model;
3238
}
3339

34-
public void setModel(Model model) {
35-
this.model = model;
36-
}
37-
3840
public Provider getProvider() {
3941
return provider;
4042
}
41-
42-
public void setProvider(Provider provider) {
43-
this.provider = provider;
44-
}
4543
}

‎lib/sdk/server-ai/src/main/java/com/launchdarkly/sdk/server/ai/datamodel/Meta.java‎

Lines changed: 6 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -16,19 +16,18 @@ public final class Meta {
1616
/**
1717
* If the config is enabled.
1818
*/
19-
private final boolean enabled;
19+
// private final boolean enabled;
2020

2121
/**
2222
* Constructor for Meta with all required fields.
2323
*
2424
* @param variationKey the variation key
2525
* @param version the version
26-
* @param enabled if the config is enabled
2726
*/
28-
public Meta(String variationKey, Optional<Integer> version, boolean enabled) {
27+
public Meta(String variationKey, Optional<Integer> version) {
2928
this.variationKey = variationKey;
3029
this.version = version != null ? version : Optional.empty();
31-
this.enabled = enabled;
30+
// this.enabled = enabled;
3231
}
3332

3433
public String getVariationKey() {
@@ -39,7 +38,7 @@ public Optional<Integer> getVersion() {
3938
return version;
4039
}
4140

42-
public boolean isEnabled() {
43-
return enabled;
44-
}
41+
// public boolean isEnabled() {
42+
// return enabled;
43+
// }
4544
}

‎lib/sdk/server-ai/src/test/java/com/launchdarkly/sdk/server/ai/LDAiClientTest.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ public class LDAiClientTest {
1313
public void testParseAiConfig() {
1414
String rawJson = "{\n" + //
1515
" \"_ldMeta\": {\n" + //
16-
" \"variationKey\" : 1234,\n" + //
16+
" \"variationKey\" : \"1234\",\n" + //
1717
" \"enabled\": true,\n" + //
1818
" \"version\": 1\n" + //
1919
" },\n" + //

0 commit comments

Comments
 (0)