Skip to content

Commit 0e87606

Browse files
author
Max Hniebergall
committed
Clean up TODOs
1 parent 8892f03 commit 0e87606

File tree

9 files changed

+227
-143
lines changed

9 files changed

+227
-143
lines changed

‎x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/inference/action/UpdateInferenceModelAction.java‎

Lines changed: 77 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
import org.elasticsearch.common.io.stream.StreamInput;
1717
import org.elasticsearch.common.io.stream.StreamOutput;
1818
import org.elasticsearch.common.xcontent.XContentHelper;
19+
import org.elasticsearch.core.Nullable;
1920
import org.elasticsearch.core.TimeValue;
2021
import org.elasticsearch.inference.ModelConfigurations;
2122
import org.elasticsearch.inference.TaskType;
@@ -27,6 +28,7 @@
2728
import org.elasticsearch.xpack.core.ml.utils.MlStrings;
2829

2930
import java.io.IOException;
31+
import java.util.Collections;
3032
import java.util.HashMap;
3133
import java.util.Map;
3234
import java.util.Objects;
@@ -43,14 +45,19 @@ public UpdateInferenceModelAction() {
4345
super(NAME);
4446
}
4547

46-
public record Settings(Map<String, Object> serviceSettings, Map<String, Object> taskSettings) {}
48+
public record Settings(
49+
@Nullable Map<String, Object> serviceSettings,
50+
@Nullable Map<String, Object> taskSettings,
51+
@Nullable TaskType taskType
52+
) {}
4753

4854
public static class Request extends AcknowledgedRequest<Request> {
4955

5056
private final String inferenceEntityId;
5157
private final BytesReference content;
5258
private final XContentType contentType;
5359
private final TaskType taskType;
60+
private Settings settings;
5461

5562
public Request(String inferenceEntityId, BytesReference content, XContentType contentType, TaskType taskType, TimeValue timeout) {
5663
super(timeout, DEFAULT_ACK_TIMEOUT);
@@ -97,72 +104,91 @@ public BytesReference getContent() {
97104
* The map is validated such that only allowed fields are present.
98105
* If any fields in the body are not on the allow list, this function will throw an exception.
99106
*/
100-
public Settings getContentAsMaps() {
101-
Map<String, Object> unvalidatedMap = XContentHelper.convertToMap(content, false, contentType).v2();
102-
Map<String, Object> serviceSettings = new HashMap<>();
103-
Map<String, Object> taskSettings = new HashMap<>();
104-
105-
// TODO either of these maps can be null, we need to handle that
107+
public Settings getContentAsSettings() {
108+
if (settings == null) { // settings is deterministic on content, so we only need to compute it once
109+
Map<String, Object> unvalidatedMap = XContentHelper.convertToMap(content, false, contentType).v2();
110+
Map<String, Object> serviceSettings = new HashMap<>();
111+
Map<String, Object> taskSettings = new HashMap<>();
112+
TaskType taskType = null;
113+
114+
if (unvalidatedMap.isEmpty()) {
115+
throw new ElasticsearchStatusException("Request body is empty", RestStatus.BAD_REQUEST);
116+
}
106117

107-
if (unvalidatedMap.isEmpty()) {
108-
throw new ElasticsearchStatusException("Request body is empty", RestStatus.BAD_REQUEST);
109-
}
118+
if (unvalidatedMap.containsKey("task_type")) {
119+
if (unvalidatedMap.get("task_type") instanceof String taskTypeString) {
120+
taskType = TaskType.fromStringOrStatusException(taskTypeString);
121+
} else {
122+
throw new ElasticsearchStatusException(
123+
"Failed to parse [task_type] in update request [{}]",
124+
RestStatus.INTERNAL_SERVER_ERROR,
125+
unvalidatedMap.toString()
126+
);
127+
}
128+
unvalidatedMap.remove("task_type");
129+
}
110130

111-
if (unvalidatedMap.containsKey(SERVICE_SETTINGS)) {
112-
if (unvalidatedMap.get(SERVICE_SETTINGS) instanceof Map<?, ?> tempMap) {
113-
for (Map.Entry<?, ?> entry : (tempMap).entrySet()) {
114-
if (entry.getKey() instanceof String key && entry.getValue() instanceof Object value) {
115-
serviceSettings.put(key, value);
116-
} else {
117-
throw new ElasticsearchStatusException(
118-
"Failed to parse update request [{}]",
119-
RestStatus.INTERNAL_SERVER_ERROR,
120-
unvalidatedMap.toString()
121-
);
131+
if (unvalidatedMap.containsKey(SERVICE_SETTINGS)) {
132+
if (unvalidatedMap.get(SERVICE_SETTINGS) instanceof Map<?, ?> tempMap) {
133+
for (Map.Entry<?, ?> entry : (tempMap).entrySet()) {
134+
if (entry.getKey() instanceof String key && entry.getValue() instanceof Object value) {
135+
serviceSettings.put(key, value);
136+
} else {
137+
throw new ElasticsearchStatusException(
138+
"Failed to parse update request [{}]",
139+
RestStatus.INTERNAL_SERVER_ERROR,
140+
unvalidatedMap.toString()
141+
);
142+
}
122143
}
144+
unvalidatedMap.remove(SERVICE_SETTINGS);
145+
} else {
146+
throw new ElasticsearchStatusException(
147+
"Unable to parse service settings in the request [{}]",
148+
RestStatus.BAD_REQUEST,
149+
unvalidatedMap.toString()
150+
);
123151
}
124-
unvalidatedMap.remove(SERVICE_SETTINGS);
125-
} else {
126-
throw new ElasticsearchStatusException(
127-
"Unable to parse service settings in the request [{}]",
128-
RestStatus.BAD_REQUEST,
129-
unvalidatedMap.toString()
130-
);
131152
}
132-
}
133153

134-
if (unvalidatedMap.containsKey(TASK_SETTINGS)) {
135-
if (unvalidatedMap.get(TASK_SETTINGS) instanceof Map<?, ?> tempMap) {
136-
for (Map.Entry<?, ?> entry : (tempMap).entrySet()) {
137-
if (entry.getKey() instanceof String key && entry.getValue() instanceof Object value) {
138-
serviceSettings.put(key, value);
139-
} else {
140-
throw new ElasticsearchStatusException(
141-
"Failed to parse update request [{}]",
142-
RestStatus.INTERNAL_SERVER_ERROR,
143-
unvalidatedMap.toString()
144-
);
154+
if (unvalidatedMap.containsKey(TASK_SETTINGS)) {
155+
if (unvalidatedMap.get(TASK_SETTINGS) instanceof Map<?, ?> tempMap) {
156+
for (Map.Entry<?, ?> entry : (tempMap).entrySet()) {
157+
if (entry.getKey() instanceof String key && entry.getValue() instanceof Object value) {
158+
taskSettings.put(key, value);
159+
} else {
160+
throw new ElasticsearchStatusException(
161+
"Failed to parse update request [{}]",
162+
RestStatus.INTERNAL_SERVER_ERROR,
163+
unvalidatedMap.toString()
164+
);
165+
}
145166
}
167+
unvalidatedMap.remove(TASK_SETTINGS);
168+
} else {
169+
throw new ElasticsearchStatusException(
170+
"Unable to parse task settings in the request [{}]",
171+
RestStatus.BAD_REQUEST,
172+
unvalidatedMap.toString()
173+
);
146174
}
147-
unvalidatedMap.remove(TASK_SETTINGS);
148-
} else {
175+
}
176+
177+
if (unvalidatedMap.isEmpty() == false) {
149178
throw new ElasticsearchStatusException(
150-
"Unable to parse task settings in the request [{}]",
179+
"Request contained fields which cannot be updated, remove these fields and try again [{}]",
151180
RestStatus.BAD_REQUEST,
152181
unvalidatedMap.toString()
153182
);
154183
}
155-
}
156184

157-
if (unvalidatedMap.isEmpty() == false) {
158-
throw new ElasticsearchStatusException(
159-
"Request contained fields which cannot be updated, remove these fields and try again [{}]",
160-
RestStatus.BAD_REQUEST,
161-
unvalidatedMap.toString()
185+
this.settings = new Settings(
186+
serviceSettings.isEmpty() == false ? Collections.unmodifiableMap(serviceSettings) : null,
187+
taskSettings.isEmpty() == false ? Collections.unmodifiableMap(taskSettings) : null,
188+
taskType
162189
);
163190
}
164-
165-
return new Settings(serviceSettings, taskSettings);
191+
return this.settings;
166192
}
167193

168194
public XContentType getContentType() {

‎x-pack/plugin/inference/src/main/java/org/elasticsearch/xpack/inference/action/TransportPutInferenceModelAction.java‎

Lines changed: 2 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,7 @@
4141
import org.elasticsearch.xpack.core.ml.utils.ExceptionsHelper;
4242
import org.elasticsearch.xpack.inference.InferencePlugin;
4343
import org.elasticsearch.xpack.inference.registry.ModelRegistry;
44+
import org.elasticsearch.xpack.inference.services.ServiceUtils;
4445
import org.elasticsearch.xpack.inference.services.elasticsearch.ElasticsearchInternalService;
4546

4647
import java.io.IOException;
@@ -100,7 +101,7 @@ protected void masterOperation(
100101
ActionListener<PutInferenceModelAction.Response> listener
101102
) throws Exception {
102103
var requestAsMap = requestToMap(request);
103-
var resolvedTaskType = resolveTaskType(request.getTaskType(), (String) requestAsMap.remove(TaskType.NAME));
104+
var resolvedTaskType = ServiceUtils.resolveTaskType(request.getTaskType(), (String) requestAsMap.remove(TaskType.NAME));
104105

105106
String serviceName = (String) requestAsMap.remove(ModelConfigurations.SERVICE);
106107
if (serviceName == null) {
@@ -227,37 +228,4 @@ protected ClusterBlockException checkBlock(PutInferenceModelAction.Request reque
227228
return state.blocks().globalBlockedException(ClusterBlockLevel.METADATA_WRITE);
228229
}
229230

230-
/**
231-
* task_type can be specified as either a URL parameter or in the
232-
* request body. Resolve which to use or throw if the settings are
233-
* inconsistent
234-
* @param urlTaskType Taken from the URL parameter. ANY means not specified.
235-
* @param bodyTaskType Taken from the request body. Maybe null
236-
* @return The resolved task type
237-
*/
238-
static TaskType resolveTaskType(TaskType urlTaskType, String bodyTaskType) {
239-
if (bodyTaskType == null) {
240-
if (urlTaskType == TaskType.ANY) {
241-
throw new ElasticsearchStatusException("model is missing required setting [task_type]", RestStatus.BAD_REQUEST);
242-
} else {
243-
return urlTaskType;
244-
}
245-
}
246-
247-
TaskType parsedBodyTask = TaskType.fromStringOrStatusException(bodyTaskType);
248-
if (parsedBodyTask == TaskType.ANY) {
249-
throw new ElasticsearchStatusException("task_type [any] is not valid type for inference", RestStatus.BAD_REQUEST);
250-
}
251-
252-
if (parsedBodyTask.isAnyOrSame(urlTaskType) == false) {
253-
throw new ElasticsearchStatusException(
254-
"Cannot resolve conflicting task_type parameter in the request URL [{}] and the request body [{}]",
255-
RestStatus.BAD_REQUEST,
256-
urlTaskType.toString(),
257-
bodyTaskType
258-
);
259-
}
260-
261-
return parsedBodyTask;
262-
}
263231
}

0 commit comments

Comments
 (0)