Making progress on tests

x-pack/plugin/inference/src/main/java/org/elasticsearch/xpack/inference/services/ServiceUtils.java

@@ -480,7 +480,7 @@ public static Map<String, Object> extractOptionalMap(
     }
 
     /**
-     * Validates that each value in the map is a {@link String} and returns a new map of Map<String, String>.
+     * Validates that each value in the map is a {@link String} and returns a new map of {@code Map<String, String>}.
      */
     public static Map<String, String> validateMapStringValues(
         Map<String, ?> map,

x-pack/plugin/inference/src/main/java/org/elasticsearch/xpack/inference/services/custom/CustomRequestManager.java

@@ -92,6 +92,7 @@ public void execute(
             var request = new CustomRequest(query, input, model);
             execute(new ExecutableInferenceRequest(requestSender, logger, request, handler, hasRequestCompletedFunction, listener));
         } catch (Exception e) {
+            // Intentionally not logging this exception because it could contain sensitive information from the CustomRequest construction
             listener.onFailure(
                 new ElasticsearchStatusException("Failed to construct the custom service request", RestStatus.BAD_REQUEST, e)
             );

x-pack/plugin/inference/src/main/java/org/elasticsearch/xpack/inference/services/custom/CustomSecretSettings.java

@@ -45,7 +45,7 @@ public static CustomSecretSettings fromMap(@Nullable Map<String, Object> map) {
             throw validationException;
         }
 
-        return new CustomSecretSettings(Objects.requireNonNullElse(secureStringMap, new HashMap<>()));
+        return new CustomSecretSettings(secureStringMap);
     }
 
     private final Map<String, SecureString> secretParameters;
@@ -56,7 +56,7 @@ public SecretSettings newSecretSettings(Map<String, Object> newSecrets) {
     }
 
     public CustomSecretSettings(@Nullable Map<String, SecureString> secretParameters) {
-        this.secretParameters = Objects.requireNonNullElse(secretParameters, new HashMap<>());
+        this.secretParameters = Objects.requireNonNullElse(secretParameters, Map.of());
     }
 
     public CustomSecretSettings(StreamInput in) throws IOException {
@@ -71,7 +71,13 @@ public Map<String, SecureString> getSecretParameters() {
     public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
         builder.startObject();
         if (secretParameters.isEmpty() == false) {
-            builder.field(SECRET_PARAMETERS, secretParameters);
+            builder.startObject(SECRET_PARAMETERS);
+            {
+                for (var entry : secretParameters.entrySet()) {
+                    builder.field(entry.getKey(), entry.getValue().toString());
+                }
+            }
+            builder.endObject();
         }
         builder.endObject();
         return builder;

x-pack/plugin/inference/src/main/java/org/elasticsearch/xpack/inference/services/custom/CustomTaskSettings.java

@@ -66,7 +66,7 @@ public static CustomTaskSettings of(CustomTaskSettings originalSettings, CustomT
     private final Map<String, Object> parameters;
 
     public CustomTaskSettings(StreamInput in) throws IOException {
-        parameters = in.readBoolean() ? in.readGenericMap() : null;
+        parameters = in.readGenericMap();
     }
 
     public CustomTaskSettings(Map<String, Object> parameters) {

x-pack/plugin/inference/src/main/java/org/elasticsearch/xpack/inference/services/custom/request/CustomRequest.java

@@ -36,7 +36,7 @@
 
 public class CustomRequest implements Request {
     /**
-     * This regex pattern matches on the string "${<any characters>}"
+     * This regex pattern matches on the string {@code "${<any characters>}"}
      */
     private static final Pattern VARIABLE_PLACEHOLDER_PATTERN = Pattern.compile("\\$\\{.*?\\}");
 
@@ -71,7 +71,7 @@ private static <T> String toJson(T value, String field) {
             builder.value(value);
             return Strings.toString(builder);
         } catch (IOException e) {
-            throw new IllegalStateException(Strings.format("Failed to serialize custom request value as json, field: %s"), e);
+            throw new IllegalStateException(Strings.format("Failed to serialize custom request value as json, field: %s", field), e);
         }
     }
 

x-pack/plugin/inference/src/main/java/org/elasticsearch/xpack/inference/services/custom/response/RerankResponseParser.java

@@ -10,6 +10,7 @@
 import org.elasticsearch.common.ValidationException;
 import org.elasticsearch.common.io.stream.StreamInput;
 import org.elasticsearch.common.io.stream.StreamOutput;
+import org.elasticsearch.core.Nullable;
 import org.elasticsearch.inference.InferenceServiceResults;
 import org.elasticsearch.xcontent.XContentBuilder;
 import org.elasticsearch.xpack.inference.external.http.HttpResult;
@@ -46,10 +47,14 @@ public static RerankResponseParser fromMap(Map<String, Object> responseParserMap
         return new RerankResponseParser(relevanceScore, rerankIndex, documentText);
     }
 
-    public RerankResponseParser(String relevanceScorePath, String rerankIndexPath, String documentTextPath) {
+    public RerankResponseParser(String relevanceScorePath) {
+        this(relevanceScorePath, null, null);
+    }
+
+    public RerankResponseParser(String relevanceScorePath, @Nullable String rerankIndexPath, @Nullable String documentTextPath) {
         this.relevanceScorePath = Objects.requireNonNull(relevanceScorePath);
-        this.rerankIndexPath = Objects.requireNonNull(rerankIndexPath);
-        this.documentTextPath = Objects.requireNonNull(documentTextPath);
+        this.rerankIndexPath = rerankIndexPath;
+        this.documentTextPath = documentTextPath;
     }
 
     public RerankResponseParser(StreamInput in) throws IOException {

x-pack/plugin/inference/src/test/java/org/elasticsearch/xpack/inference/Utils.java

@@ -235,4 +235,8 @@ public static void assertJsonEquals(String actual, String expected) throws IOExc
             assertThat(actualParser.mapOrdered(), equalTo(expectedParser.mapOrdered()));
         }
     }
+
+    public static <K, V> Map<K, V> modifiableMap(Map<K, V> aMap) {
+        return new HashMap<>(aMap);
+    }
 }

x-pack/plugin/inference/src/test/java/org/elasticsearch/xpack/inference/services/ServiceUtilsTests.java

@@ -20,6 +20,7 @@
 import java.util.List;
 import java.util.Map;
 
+import static org.elasticsearch.xpack.inference.Utils.modifiableMap;
 import static org.elasticsearch.xpack.inference.services.ServiceUtils.convertToUri;
 import static org.elasticsearch.xpack.inference.services.ServiceUtils.createUri;
 import static org.elasticsearch.xpack.inference.services.ServiceUtils.extractOptionalEnum;
@@ -919,8 +920,4 @@ public void testValidateInputType_ValidationErrorsWhenInputTypeIsSpecified() {
         ServiceUtils.validateInputTypeIsUnspecifiedOrInternal(InputType.CLUSTERING, validationException);
         assertThat(validationException.validationErrors().size(), is(4));
     }
-
-    private static <K, V> Map<K, V> modifiableMap(Map<K, V> aMap) {
-        return new HashMap<>(aMap);
-    }
 }

x-pack/plugin/inference/src/test/java/org/elasticsearch/xpack/inference/services/custom/CustomModelTests.java

@@ -7,18 +7,18 @@
 
 package org.elasticsearch.xpack.inference.services.custom;
 
-import io.netty.handler.codec.http.HttpMethod;
-
 import org.apache.http.HttpHeaders;
-import org.elasticsearch.common.ValidationException;
+import org.elasticsearch.common.settings.SecureString;
 import org.elasticsearch.core.Nullable;
 import org.elasticsearch.inference.SimilarityMeasure;
 import org.elasticsearch.inference.TaskType;
 import org.elasticsearch.test.ESTestCase;
+import org.elasticsearch.xpack.inference.services.custom.response.ErrorResponseParser;
+import org.elasticsearch.xpack.inference.services.custom.response.ResponseParser;
+import org.elasticsearch.xpack.inference.services.custom.response.TextEmbeddingResponseParser;
 import org.elasticsearch.xpack.inference.services.settings.RateLimitSettings;
 import org.hamcrest.MatcherAssert;
 
-import java.util.HashMap;
 import java.util.Map;
 
 import static org.hamcrest.Matchers.is;
@@ -28,7 +28,7 @@ public class CustomModelTests extends ESTestCase {
     public static String taskSettingsValue = "test_taskSettings_value";
 
     public static String secretSettingsKey = "test_secret_key";
-    public static String secretSettingsValue = "test_secret_value";
+    public static SecureString secretSettingsValue = new SecureString("test_secret_value".toCharArray());
     public static String url = "http://www.abc.com";
     public static String path = "/endpoint";
 
@@ -66,44 +66,30 @@ public static CustomModel createModel(
     }
 
     public static CustomModel getTestModel() {
-        TaskType taskType = TaskType.TEXT_EMBEDDING;
-        Map<String, Object> jsonParserMap = new HashMap<>(
-            Map.of(CustomServiceSettings.TEXT_EMBEDDING_PARSER_EMBEDDINGS, "$.result.embeddings[*].embedding")
-        );
-        return getTestModel(taskType, jsonParserMap);
+        return getTestModel(TaskType.TEXT_EMBEDDING, new TextEmbeddingResponseParser("$.result.embeddings[*].embedding"));
     }
 
-    public static CustomModel getTestModel(TaskType taskType, Map<String, Object> jsonParserMap) {
-        // service settings
+    public static CustomModel getTestModel(TaskType taskType, ResponseParser responseParser) {
         Integer dims = 1536;
         Integer maxInputTokens = 512;
-        String description = "test fromMap";
-        String version = "v1";
-        String serviceType = taskType.toString();
-        String method = HttpMethod.POST.name();
-        String queryString = "?query=${" + taskSettingsKey + "}";
-        Map<String, Object> headers = Map.of(HttpHeaders.AUTHORIZATION, "${" + secretSettingsKey + "}");
+        Map<String, String> headers = Map.of(HttpHeaders.AUTHORIZATION, "${" + secretSettingsKey + "}");
         String requestContentString = "\"input\":\"${input}\"";
 
-        ResponseJsonParser responseJsonParser = new ResponseJsonParser(taskType, jsonParserMap, new ValidationException());
 
         CustomServiceSettings serviceSettings = new CustomServiceSettings(
             SimilarityMeasure.DOT_PRODUCT,
             dims,
             maxInputTokens,
             url,
-            path,
-            method,
-            queryString,
             headers,
-            null,
             requestContentString,
-            responseJsonParser,
-            new RateLimitSettings(10_000)
+            responseParser,
+            new RateLimitSettings(10_000),
+            new ErrorResponseParser("$.error.message")
         );
 
         // task settings
-        CustomTaskSettings taskSettings = new CustomTaskSettings(Map.of(taskSettingsKey, taskSettingsValue), false);
+        CustomTaskSettings taskSettings = new CustomTaskSettings(Map.of(taskSettingsKey, taskSettingsValue));
 
         // secret settings
         CustomSecretSettings secretSettings = new CustomSecretSettings(Map.of(secretSettingsKey, secretSettingsValue));

x-pack/plugin/inference/src/test/java/org/elasticsearch/xpack/inference/services/custom/CustomSecretSettingsTests.java

@@ -7,13 +7,15 @@
 
 package org.elasticsearch.xpack.inference.services.custom;
 
+import org.elasticsearch.TransportVersion;
 import org.elasticsearch.common.Strings;
 import org.elasticsearch.common.io.stream.Writeable;
-import org.elasticsearch.test.AbstractWireSerializingTestCase;
+import org.elasticsearch.common.settings.SecureString;
+import org.elasticsearch.common.xcontent.XContentHelper;
 import org.elasticsearch.xcontent.XContentBuilder;
 import org.elasticsearch.xcontent.XContentFactory;
 import org.elasticsearch.xcontent.XContentType;
-import org.hamcrest.MatcherAssert;
+import org.elasticsearch.xpack.core.ml.AbstractBWCWireSerializationTestCase;
 
 import java.io.IOException;
 import java.util.HashMap;
@@ -22,11 +24,14 @@
 import static org.elasticsearch.core.Tuple.tuple;
 import static org.hamcrest.Matchers.is;
 
-public class CustomSecretSettingsTests extends AbstractWireSerializingTestCase<CustomSecretSettings> {
+public class CustomSecretSettingsTests extends AbstractBWCWireSerializationTestCase<CustomSecretSettings> {
     public static CustomSecretSettings createRandom() {
-        var secretParameters = randomBoolean()
-            ? randomMap(0, 5, () -> tuple(randomAlphaOfLength(5), (Object) randomAlphaOfLength(5)))
-            : null;
+        Map<String, SecureString> secretParameters = randomMap(
+            0,
+            5,
+            () -> tuple(randomAlphaOfLength(5), new SecureString(randomAlphaOfLength(5).toCharArray()))
+        );
+
         return new CustomSecretSettings(secretParameters);
     }
 
@@ -35,20 +40,28 @@ public void testFromMap() {
             Map.of(CustomSecretSettings.SECRET_PARAMETERS, new HashMap<>(Map.of("test_key", "test_value")))
         );
 
-        MatcherAssert.assertThat(
+        assertThat(
             CustomSecretSettings.fromMap(secretParameters),
-            is(new CustomSecretSettings(Map.of("test_key", "test_value")))
+            is(new CustomSecretSettings(Map.of("test_key", new SecureString("test_value".toCharArray()))))
         );
     }
 
     public void testXContent() throws IOException {
-        var entity = new CustomSecretSettings(Map.of("test_key", "test_value"));
+        var entity = new CustomSecretSettings(Map.of("test_key", new SecureString("test_value".toCharArray())));
 
         XContentBuilder builder = XContentFactory.contentBuilder(XContentType.JSON);
         entity.toXContent(builder, null);
         String xContentResult = Strings.toString(builder);
 
-        assertThat(xContentResult, is("{\"secret_parameters\":{\"test_key\":\"test_value\"}}"));
+        var expected = XContentHelper.stripWhitespace("""
+            {
+                "secret_parameters": {
+                    "test_key": "test_value"
+                }
+            }
+            """);
+
+        assertThat(xContentResult, is(expected));
     }
 
     @Override
@@ -63,6 +76,11 @@ protected CustomSecretSettings createTestInstance() {
 
     @Override
     protected CustomSecretSettings mutateInstance(CustomSecretSettings instance) {
-        return null;
+        return randomValueOtherThan(instance, CustomSecretSettingsTests::createRandom);
+    }
+
+    @Override
+    protected CustomSecretSettings mutateInstanceForVersion(CustomSecretSettings instance, TransportVersion version) {
+        return instance;
     }
 }