Skip to content

[Inference API] Add custom headers for Azure OpenAI Service#142969

Merged
jonathan-buttner merged 39 commits intoelastic:mainfrom
jonathan-buttner:ia-azure-headers
Mar 3, 2026
Merged

[Inference API] Add custom headers for Azure OpenAI Service#142969
jonathan-buttner merged 39 commits intoelastic:mainfrom
jonathan-buttner:ia-azure-headers

Conversation

@jonathan-buttner
Copy link
Copy Markdown
Contributor

@jonathan-buttner jonathan-buttner commented Feb 24, 2026

This PR adds custom headers support for the Azure OpenAI service.

Notable changes:

  • This mostly follows the same approach from the PR here: [ML] Adding custom headers support openai text embeddings #134960
  • Refactors the completion and embedding task settings to leverage a base class that does most of the work
  • This only supports a single header key. The http spec allows for multiple but we had already done a single key in the previous PR so I stuck with that
  • I took a shot at using ConstructingObjectParser instead of parsing from a map to start that transition

Testing

  1. Create a deployment in Azure
  2. Create endpoint with a header that would overwrite the api key
PUT _inference/text_embedding/azure
{
    "service": "azureopenai",
    "service_settings": {
        "api_key": "<api key>",
        "resource_name": "jonathan-buttner-test",
        "deployment_id": "jon-text-embedding",
        "api_version": "2023-05-15"
    },
    "task_settings": {
        "headers": {
            "api-key": "jon-test"
        }
    }
}

The creation request should fail because the api key is invalid when overwritten by the custom header.

Updating

To unset user, specify "user": null. To unset headers either send "headers": null or "headers": {}

PUT _inference/text_embedding/azure/_update
{
    "task_settings": {
        "user": null,
        "headers": null
    }
}

@elasticsearchmachine
Copy link
Copy Markdown
Collaborator

Hi @jonathan-buttner, I've created a changelog YAML for you.

};

private final String user;
public static final AzureOpenAiCompletionTaskSettings EMPTY = FACTORY.emptySettings();
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is doing some weird stuff to avoid the empty instance and factory depending on each other during initialization.

More information here: https://github.com/elastic/elasticsearch/pull/142969/changes#diff-a84378b3ed8b308ba6b3492e327a46706de1f8dbca5ed50db2134aec40381b4fR79-R83

            // Ideally we'd be able to pass the empty instance in via the Factory constructor, but since the empty instance relies on the
            // factory to be created, we have to lazily create it here. The empty instance will call the AzureOpenAiTaskSettings
            // constructor with the factory. If we don't do it this way we end up getting an NPE in the constructor because the factory
            // hasn't finished initialization yet.

* Base class for Azure OpenAI task settings (embeddings and completion). Holds optional user and optional
* custom HTTP headers via {@link Headers}.
*/
public abstract class AzureOpenAiTaskSettings<T extends AzureOpenAiTaskSettings<T>> implements TaskSettings {
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This generally follows the same pattern that we started in OpenAiTaskSettings except this class leverages a ConstructingObjectParser to parse out the fields. Since we'd like to move in that direction, hopefully this will make that easier in the future.

);

constructingObjectParser.declareString(optionalConstructorArg(), new ParseField(AzureOpenAiServiceFields.USER));
Headers.initParser(constructingObjectParser);
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Apply the header parsing logic to the constructing object parser.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this is where we would call parser.declareObject(optionalConstructorArg(), Headers.PARSER, new ParseField("headers") or something

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah I was struggling with that. The issue I see is that headers is only a map so there isn't another level for parsing. For example if we had this:

{
  "headers: {
    "some_other_field": {
      ...
    }
  }
}

Then the headers class could have a parser that explicitly looks for some_other_field. But headers just wants it as a map. Like imagine headers was a string with some special parsing logic around it. How would we leverage a parser that was wrapping a string.

In the examples I've seen we typically declare another parser if it encapsulates a whole object.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ah, yes, of course. Makes sense 👍

Factory<T> factory
) {
if (map.isEmpty()) {
return factory.emptySettings();
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Another option I looked into was to use an abstract method on the AzureOpenAiTaskSettings class to handle "creating" the right type of an empty instance. The problem is that we're in a static context here and to avoid having to construct a new object entirely we need to do it in a static method.

OpenAiTaskSettings doesn't leverage empty instances and requires an object to be created each time.


try {
try (
var xContent = XContentBuilder.builder(JsonXContent.jsonXContent).map(map);
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This converts a Map to json and then parses it.

}

@Override
public T updatedTaskSettings(Map<String, Object> newSettings) {
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This doesn't try to return an empty instance. To return an empty instance we'd need to cast to T I think. I don't think it's worth it because in the flow of a request including task settings the caller will check if the task settings are empty and return the same model. This will also be called in the update api flow but I don't think that needs to be very memory conscious.

If you have other ideas I'm open to improving this though.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm fine with this as it is, but one option would be to have an abstract emptyInstance() method on this class and then add

        if (userToUse.isUndefined() && headersToUse.value().isUndefined()) {
            return emptyInstance();
        }

at the end of this method.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good idea, we actually have this via the factory already 🙌 so I'll add the if block.

*
* @param map the settings received from a request
* @return a {@link AzureOpenAiEmbeddingsRequestTaskSettings}
* @return a {@link AzureAiStudioEmbeddingsRequestTaskSettings}
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This was referencing the wrong class

@elasticsearchmachine
Copy link
Copy Markdown
Collaborator

Pinging @elastic/search-inference-team (Team:Search - Inference)

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR adds support for specifying custom HTTP headers in Azure OpenAI inference task settings, refactoring Azure OpenAI completion/embeddings task settings to share a common base parser/serialization implementation and improving parse-error unwrapping.

Changes:

  • Introduces a reusable Headers parser/value object and wires it into Azure OpenAI task settings and request construction.
  • Refactors Azure OpenAI completion/embeddings task settings to extend a new AzureOpenAiTaskSettings base (incl. transport version gating).
  • Updates/adds tests and transport version definitions, plus a changelog entry.

Reviewed changes

Copilot reviewed 35 out of 35 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
x-pack/plugin/inference/src/test/java/org/elasticsearch/xpack/inference/services/openai/OpenAiTaskSettingsTests.java Updates assertions for revised map-type error messages.
x-pack/plugin/inference/src/test/java/org/elasticsearch/xpack/inference/services/custom/CustomTaskSettingsTests.java Updates expected error message type rendering for invalid parameter values.
x-pack/plugin/inference/src/test/java/org/elasticsearch/xpack/inference/services/custom/CustomServiceSettingsTests.java Updates header-validation error message assertions.
x-pack/plugin/inference/src/test/java/org/elasticsearch/xpack/inference/services/azureopenai/embeddings/AzureOpenAiEmbeddingsTaskSettingsTests.java Refactors embeddings task settings tests to inherit shared Azure OpenAI task settings test base.
x-pack/plugin/inference/src/test/java/org/elasticsearch/xpack/inference/services/azureopenai/embeddings/AzureOpenAiEmbeddingsRequestTaskSettingsTests.java Removes request-only task settings tests (request settings now handled via base parsing/updating).
x-pack/plugin/inference/src/test/java/org/elasticsearch/xpack/inference/services/azureopenai/embeddings/AzureOpenAiEmbeddingsModelTests.java Updates model construction to include headers-aware task settings.
x-pack/plugin/inference/src/test/java/org/elasticsearch/xpack/inference/services/azureopenai/completion/AzureOpenAiCompletionTaskSettingsTests.java Refactors completion task settings tests to inherit shared Azure OpenAI task settings test base.
x-pack/plugin/inference/src/test/java/org/elasticsearch/xpack/inference/services/azureopenai/completion/AzureOpenAiCompletionRequestTaskSettingsTests.java Removes request-only task settings tests (now handled via base parsing/updating).
x-pack/plugin/inference/src/test/java/org/elasticsearch/xpack/inference/services/azureopenai/completion/AzureOpenAiCompletionModelTests.java Updates model construction to include headers-aware task settings.
x-pack/plugin/inference/src/test/java/org/elasticsearch/xpack/inference/services/azureopenai/action/AzureOpenAiActionCreatorTests.java Switches helper import to new shared Azure OpenAI task settings test utilities.
x-pack/plugin/inference/src/test/java/org/elasticsearch/xpack/inference/services/azureopenai/AzureOpenAiTaskSettingsTests.java Adds shared test base covering user + headers parsing, updating, and BWC behavior.
x-pack/plugin/inference/src/test/java/org/elasticsearch/xpack/inference/services/azureopenai/AzureOpenAiServiceTests.java Updates expectations for request config parsing errors (now can surface XContentParseException).
x-pack/plugin/inference/src/test/java/org/elasticsearch/xpack/inference/services/ServiceUtilsTests.java Updates error message assertions for type-name based reporting.
x-pack/plugin/inference/src/test/java/org/elasticsearch/xpack/inference/common/parser/ParseExceptionUtilsTests.java Adds tests for unwrapping XContentParseException chains.
x-pack/plugin/inference/src/test/java/org/elasticsearch/xpack/inference/common/parser/HeadersTests.java Adds tests for parsing/serialization/validation of headers.
x-pack/plugin/inference/src/main/java/org/elasticsearch/xpack/inference/services/azureopenai/request/AzureOpenAiRequest.java Refactors Azure OpenAI request creation into a shared base and applies custom headers to outgoing requests.
x-pack/plugin/inference/src/main/java/org/elasticsearch/xpack/inference/services/azureopenai/request/AzureOpenAiEmbeddingsRequest.java Migrates embeddings request to extend AzureOpenAiRequest and delegates shared HTTP request creation.
x-pack/plugin/inference/src/main/java/org/elasticsearch/xpack/inference/services/azureopenai/request/AzureOpenAiCompletionRequest.java Migrates completion request to extend AzureOpenAiRequest and delegates shared HTTP request creation.
x-pack/plugin/inference/src/main/java/org/elasticsearch/xpack/inference/services/azureopenai/request/AzureOpenAiChatCompletionRequest.java Migrates chat completion request to extend AzureOpenAiRequest and delegates shared HTTP request creation.
x-pack/plugin/inference/src/main/java/org/elasticsearch/xpack/inference/services/azureopenai/embeddings/AzureOpenAiEmbeddingsTaskSettings.java Refactors embeddings task settings to extend shared AzureOpenAiTaskSettings and parse with context.
x-pack/plugin/inference/src/main/java/org/elasticsearch/xpack/inference/services/azureopenai/embeddings/AzureOpenAiEmbeddingsRequestTaskSettings.java Removes request-only task settings record (superseded by shared updating/parsing).
x-pack/plugin/inference/src/main/java/org/elasticsearch/xpack/inference/services/azureopenai/embeddings/AzureOpenAiEmbeddingsModel.java Switches task settings parsing/updating to context-aware AzureOpenAiTaskSettings base behavior.
x-pack/plugin/inference/src/main/java/org/elasticsearch/xpack/inference/services/azureopenai/completion/AzureOpenAiCompletionTaskSettings.java Refactors completion task settings to extend shared AzureOpenAiTaskSettings and parse with context.
x-pack/plugin/inference/src/main/java/org/elasticsearch/xpack/inference/services/azureopenai/completion/AzureOpenAiCompletionRequestTaskSettings.java Removes request-only task settings record (superseded by shared updating/parsing).
x-pack/plugin/inference/src/main/java/org/elasticsearch/xpack/inference/services/azureopenai/completion/AzureOpenAiCompletionModel.java Switches task settings parsing/updating to context-aware AzureOpenAiTaskSettings base behavior.
x-pack/plugin/inference/src/main/java/org/elasticsearch/xpack/inference/services/azureopenai/AzureOpenAiTaskSettings.java Adds shared base for Azure OpenAI task settings (user + headers parsing, updating, BWC transport support).
x-pack/plugin/inference/src/main/java/org/elasticsearch/xpack/inference/services/azureopenai/AzureOpenAiService.java Removes task-settings “empty map” post-parse check (unknown fields now enforced by parser in request context).
x-pack/plugin/inference/src/main/java/org/elasticsearch/xpack/inference/services/azureopenai/AzureOpenAiModel.java Reintroduces typed accessor override for secret settings on the base model.
x-pack/plugin/inference/src/main/java/org/elasticsearch/xpack/inference/services/azureaistudio/embeddings/AzureAiStudioEmbeddingsRequestTaskSettings.java Fixes incorrect Javadoc reference after Azure OpenAI request-task-settings removal.
x-pack/plugin/inference/src/main/java/org/elasticsearch/xpack/inference/services/ServiceUtils.java Changes map validation error messages to report value type via class name.
x-pack/plugin/inference/src/main/java/org/elasticsearch/xpack/inference/common/parser/ParseExceptionUtils.java Adds helper to unwrap XContentParseException to a more actionable runtime exception.
x-pack/plugin/inference/src/main/java/org/elasticsearch/xpack/inference/common/parser/Headers.java Adds Headers record to parse/validate/serialize custom HTTP headers.
server/src/main/resources/transport/upper_bounds/9.4.csv Bumps the 9.4 transport version upper bound to include new referable version.
server/src/main/resources/transport/definitions/referable/inference_azure_openai_task_settings_headers.csv Adds referable transport version definition for Azure OpenAI task settings headers.
docs/changelog/142969.yaml Adds changelog entry for Azure OpenAI custom headers support.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

jonathan-buttner and others added 3 commits February 24, 2026 15:43
…inference/services/azureopenai/AzureOpenAiService.java

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…inference/services/azureopenai/request/AzureOpenAiRequest.java

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
this.isDefined = isDefined;
}

public boolean isAbsent() {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could we rename this to isUndefined()? Something bugs me about isPresent() XOR inAbsent() not always being true (when isNull() is true) :-)

this(read(in));
}

private static StatefulValue<Map<String, String>> read(StreamInput in) throws IOException {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do we need this method? Could we not inline this in the Headers(StreamInput) constructor?

} catch (IOException e) {
throw new IllegalArgumentException("Failed to parse Azure OpenAI task settings", e);
} catch (XContentParseException e) {
throw unwrapXContentParseException(e);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why do we want to unwrap here?

Copy link
Copy Markdown
Contributor Author

@jonathan-buttner jonathan-buttner Feb 27, 2026

Choose a reason for hiding this comment

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

If we don't unwrap we'll get XContentParseException getCause -> XContentParseException getCause -> ValidationException

That'll look like this (from HeadersTests::testParse_ThrowsWhenValueNotString):

assertThat(exception.getMessage(), containsString("[headers_parser] failed to parse field [headers]"));
        assertThat(exception.getCause().getMessage(), containsString("Failed to build [headers_parser] after last required field arrived"));
        assertThat(
            exception.getCause().getCause().getMessage(),
            containsString(
                "Map field [root.headers] has an entry that is not valid, [key => 1]. Value type of [Integer] is not one of [String].;"
            )
        );

The response in Elasticsearch will remove the top one but it'll still return the top exception as "Failed to build... after last required field arrived". I didn't think that was very helpful so I figured unwrapping might be better here so that the first exception is the validation one. That's also inline with how things worked previously when parsing using maps instead of ConstructingObjectParser.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The theory behind this is that when parsing an object that may have multiple fields, sub-fields, etc., the XContentParseException takes care of showing the exact location where the error happened. Here is an example output when parsing fails deep in a ML anomaly detection job:

image

Are we certain we don't want to leverage that in our inference parsing going forwards?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ah interesting, ok I'll remove the unwrapping then 👍

"inference_azure_openai_task_settings_headers"
);

protected record Settings(StatefulValue<String> user, Headers headers) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do we make user stateful because we want to be able to set it to null?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah my reasoning was that I thought it'd be a weird experience for a customer to be able to unset headers but not user. So I figured it'd probably be good to do it for both.

I think we should probably move in the direction of having any optional field allowed to be unset.

Copy link
Copy Markdown
Contributor

@DonalEvans DonalEvans left a comment

Choose a reason for hiding this comment

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

I think it's possible I was just confused about how we want things to work with the comments around how we persist null/empty user values, so apologies if that's the case.

@@ -288,6 +288,19 @@ public void declareStringOrNull(BiConsumer<Value, String> consumer, ParseField f
);
}

/**
* Declare a field of type {@code T} parsed from string and converted to {@code T} using provided function,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think this comment needs to be adjusted, since no function is provided and the type is always String


public static <Value, Context> void initParser(ConstructingObjectParser<Value, Context> parser) {
parser.declareObjectOrNull(optionalConstructorArg(), (p, c) -> p.mapOrdered(), null, HEADERS);
parser.declareObjectOrNull(optionalConstructorArg(), (p, c) -> {
var orderedMap = p.mapOrdered();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is there a reason we want to use an ordered map here rather than just a map?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Naa I'll switch it to a map.

var stringsMap = doValidation((Map<String, Object>) arg, validationException);

if (stringsMap.isEmpty()) {
return UNDEFINED_INSTANCE;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is this correct? I think to get here, the user would have had to specify headers as an empty map in the task settings, which to me feels like it's defined but empty rather than being undefined. If I was a user who wanted to clear the headers I'd previously set, I could see specifying an empty map for the headers field being one way of doing that, with another being setting the field to null explicitly.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah fair point. I'll switch this to return the NULL_INSTANCE and leave a comment describing that we're considering this the same situation as when a user wants to remove the headers.

Comment on lines +61 to +63
if (validationException.validationErrors().isEmpty() == false) {
throw validationException;
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This can be simplified to validationException.throwIfValidationErrorsExist();

import java.util.Map;
import java.util.Objects;

import static org.elasticsearch.xcontent.ConstructingObjectParser.optionalConstructorArg;
import static org.elasticsearch.xpack.inference.services.ServiceUtils.removeNullValues;
import static org.elasticsearch.xpack.inference.services.ServiceUtils.validateMapStringValues;

public record Headers(Map<String, String> headersMap) implements ToXContentFragment, Writeable {
public record Headers(StatefulValue<Map<String, String>> value) implements ToXContentFragment, Writeable {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nitpick, but this might be better named "mapValue" or even just "map", since "value" is a bit vague. I'm thinking along the same lines as how we might typically name an AtomicReference, where instead of calling them all "reference" the name usually refers to what they're a reference to.

* With {@code counter++ >= 10}, 10 getCause() steps are allowed; a chain of 11
* XContentParseExceptions causes the 11th (index 10) to be returned when the limit is hit.
*/
public void testUnwrap_StopsAtTenLevels_ReturnsExceptionAtLimit() {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ideally this test would have a chain of more than 11 exceptions and confirm that we stop before getting to the last one. As is, this test would still pass even if we never stopped unwrapping exceptions. I think that this should give the coverage we want:

    public void testUnwrap_StopsAtTenLevels_ReturnsExceptionAtLimit() {
        XContentParseException toReturn = null;
        var chain = new XContentParseException[MAX_UNWRAP_DEPTH + 5];

        for (var i = chain.length - 1; i > 0; i--) {
            chain[i - 1] = new XContentParseException(null, "level " + i, chain[i]);
            if (i == MAX_UNWRAP_DEPTH) {
                toReturn = chain[i];
            }
        }
        var result = ParseExceptionUtils.unwrapXContentParseException(chain[0]);
        assertThat(result, sameInstance(toReturn));
        // Confirm there were more nested exceptions that we didn't try to unwrap
        assertThat(result.getCause(), notNullValue());
    }

Copy link
Copy Markdown
Contributor Author

@jonathan-buttner jonathan-buttner Mar 2, 2026

Choose a reason for hiding this comment

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

Good point, I removed this class and tests based on Dimitris recommendation because we're no longer unwrapping 👍

}

public static <T> StatefulValue<T> of(T value) {
return new StatefulValue<>(Objects.requireNonNull(value), true);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Very minor nitpick, but if we want to more closely align with the behaviour of Optional, we could throw new NoSuchElementException("No value present") here instead of an NPE.

Comment on lines +112 to +113
var value = randomAlphaOfLength(10);
assertEquals(StatefulValue.of(value), StatefulValue.of(value));
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

To make this test stronger, I think we should be passing in two different instances of equal values here:

var map = new HashMap<>(Map.of(randomAlphaOfLength(10), randomAlphaOfLength(10)));
var equalMap = new HashMap<>(map);
assertEquals(StatefulValue.of(map), StatefulValue.of(equalMap));

this makes sure that we're actually properly testing the equality of the wrapped objects instead of just doing ==.

The same comment applies to the hashCode() test below this one. It might even be better to have one test that covers both equals() and hashCode() since then it's more obvious that we're first establishing that two values are equal, then that they also have the same hashcode.

Comment on lines +48 to +50
private String randomUser() {
return randomBoolean() ? null : randomAlphaOfLength(15);
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

For full coverage, it would be good to have the chance of returning an empty String for user as well as null. I think adding that will help surface a bug in the toXContent() implementation.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

"user": "" will return a validation error for the parsing logic. I've tried to test explicitly with an empty string where applicable (when in persistent context).

containsString(
"Map field [task_settings.headers] has an entry that is not valid, [key => 1]. Value type of [Integer] is not one of [String]."
"Map field [headers] has an entry that is not " + "valid, [key => 1]. Value type of [Integer] is not one of [String].;"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We can join these two strings.

private static final AzureOpenAiTaskSettings.Factory<AzureOpenAiCompletionTaskSettings> FACTORY = new Factory<>() {
@Override
public AzureOpenAiCompletionTaskSettings create(CommonSettings commonSettings) {
return new AzureOpenAiCompletionTaskSettings(commonSettings);
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I decided to not check for isEmpty() here because it was causing a bug where commonSettings had user.nullInstance() and headers.nullInstance() it'd return true and we'd return the empty instance even though we want the null state. We could do a check for something like user.isUndefined() && headers.isUndefined() but I'm not sure it's worth it.

Comment on lines +206 to 208
if (user.isPresent() && user.get().isEmpty() == false) {
builder.field(AzureOpenAiServiceFields.USER, user.get());
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Okay, that should be fine then

Comment on lines 84 to 94
if (newSettings.user().isPresent()) {
newSettingsMap.put(AzureOpenAiServiceFields.USER, newSettings.user().get());
} else if (newSettings.user().isNull()) {
newSettingsMap.put(AzureOpenAiServiceFields.USER, null);
}

if (newSettings.headers().isPresent()) {
newSettingsMap.put(Headers.HEADERS_FIELD, new HashMap<>(newSettings.headers().value().get()));
newSettingsMap.put(Headers.HEADERS_FIELD, new HashMap<>(newSettings.headers().mapValue().get()));
} else if (newSettings.headers().isNull()) {
newSettingsMap.put(Headers.HEADERS_FIELD, null);
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This can be simplified to:

if (newSettings.user().isUndefined() == false) {
    newSettingsMap.put(AzureOpenAiServiceFields.USER, newSettings.user().orElse(null));
}

if (newSettings.headers().mapValue().isUndefined() == false) {
    newSettingsMap.put(Headers.HEADERS_FIELD, newSettings.headers().mapValue().orElse(null));
}

@jonathan-buttner jonathan-buttner enabled auto-merge (squash) March 3, 2026 14:08
@jonathan-buttner jonathan-buttner merged commit cf8bc46 into elastic:main Mar 3, 2026
34 of 35 checks passed
@jonathan-buttner jonathan-buttner deleted the ia-azure-headers branch March 3, 2026 15:59
GalLalouche pushed a commit to GalLalouche/elasticsearch that referenced this pull request Mar 3, 2026
…142969)

* Adding headers file

* Using base class for task settings

* Refactoring base class to allow for returning empty instance

* Fixing test failures

* Fixing test

* Update docs/changelog/142969.yaml

* [CI] Auto commit changes from spotless

* clean up and consolidating request header logic

* Returning validation exception instead of xcontent when possible

* Adding unwrapper utils tests

* Update x-pack/plugin/inference/src/main/java/org/elasticsearch/xpack/inference/services/azureopenai/AzureOpenAiService.java

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

* Update x-pack/plugin/inference/src/main/java/org/elasticsearch/xpack/inference/services/azureopenai/request/AzureOpenAiRequest.java

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

* [CI] Auto commit changes from spotless

* Address chatgpt feedback

* Adding test for unwrap exception

* Working refactor

* Removing duplicate method

* Adding tests for statefulvalue

* Using a single model reference

* Fixing test and renaming

* Fixing tests and refactoring validation

* [CI] Auto commit changes from spotless

* Removing exception unwrap logic

* Addressing feedback

* [CI] Auto commit changes from spotless

* Allowing null in update api

* Using orElse

---------

Co-authored-by: elasticsearchmachine <infra-root+elasticsearchmachine@elastic.co>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
szybia added a commit to szybia/elasticsearch that referenced this pull request Mar 3, 2026
…locations

* upstream/main: (51 commits)
  ESQL: Remaining serialization tests (elastic#143470)
  Eagerly release resources in `TransportAwaitClusterStateVersionAppliedAction` (elastic#143477)
  Stop and relocate sliced reindex on shutdown (elastic#143183)
  Documentation for query_vector base64 parameter (elastic#142675)
  ES|QL: Fix LIMIT after all columns are dropped (elastic#143463)
  Update docs-build.yml (elastic#142958)
  Fix KnnIndexTester to work with byte vectors (elastic#143493)
  Fix IndexInputUtils.withSlice to produce native-safe MemorySegments on Java 21 (elastic#143479)
  CPS fix: include only relevant projects in the search response metadata (elastic#143367)
  apm-data: explicit map of timestamp.us to long (elastic#143173)
  [Inference API] Add custom headers for Azure OpenAI Service (elastic#142969)
  ESQL: Add name IDs to golden tests and fix synthetic names (elastic#143450)
  Add getUnavailableShards to BaseBroadcastResponse (elastic#143406)
  Add description to reindex API without sensitive info (elastic#143112)
  SQL: fix CLI tests (elastic#143451)
  ES|QL: Add note of future removal of FORK implicit LIMIT (elastic#143457)
  [Test] Randomly disable doc values skippers in time-series indices (elastic#143389)
  Improve pattern text downgrade license test (elastic#143102)
  [Transform] Stop transforms at the end of tests (elastic#139783)
  Mute org.elasticsearch.compute.lucene.read.ValueSourceReaderTypeConversionTests testLoadAll elastic#143471
  ...
shmuelhanoch pushed a commit to shmuelhanoch/elasticsearch that referenced this pull request Mar 4, 2026
…142969)

* Adding headers file

* Using base class for task settings

* Refactoring base class to allow for returning empty instance

* Fixing test failures

* Fixing test

* Update docs/changelog/142969.yaml

* [CI] Auto commit changes from spotless

* clean up and consolidating request header logic

* Returning validation exception instead of xcontent when possible

* Adding unwrapper utils tests

* Update x-pack/plugin/inference/src/main/java/org/elasticsearch/xpack/inference/services/azureopenai/AzureOpenAiService.java

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

* Update x-pack/plugin/inference/src/main/java/org/elasticsearch/xpack/inference/services/azureopenai/request/AzureOpenAiRequest.java

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

* [CI] Auto commit changes from spotless

* Address chatgpt feedback

* Adding test for unwrap exception

* Working refactor

* Removing duplicate method

* Adding tests for statefulvalue

* Using a single model reference

* Fixing test and renaming

* Fixing tests and refactoring validation

* [CI] Auto commit changes from spotless

* Removing exception unwrap logic

* Addressing feedback

* [CI] Auto commit changes from spotless

* Allowing null in update api

* Using orElse

---------

Co-authored-by: elasticsearchmachine <infra-root+elasticsearchmachine@elastic.co>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants