[ML] StartTrainedModelDeployment Request query params override body params

[maxhniebergall]

@jonathan-buttner recently discovered an issue with the change I recently made to disable inference cache in serverless where the cache would not be disabled if the start deployment request contained a body.

This was surprising to me, and upon closer inspection I noticed that there were two parallel paths for parsing the the request, one which used the body and one which used the parameters. This had the effect of ignoring path parameters if a body was included. Additionally, if a body was included, parameters such as cache_size would cause errors if a body (including empty body) was included:

        "type": "illegal_argument_exception",
        "reason": "request [/_ml/trained_models/.elser_model_2/deployment/_start] contains unrecognized parameter: [cache_size]"

Now, if a parameter is provided in both the query params and body, an exception will be thrown if the parameters are different, unless the parameter provided in the body is the same as the default parameter.

[elasticsearchmachine]

Pinging @elastic/ml-core (Team:ML)

[elasticsearchmachine]

Hi @maxhniebergall, I've created a changelog YAML for you.

[jonathan-buttner]

How about we remove the reference to the class name (users won't know what that means). Maybe something like `Star trained model deployment API requests query params now override body params.

[davidkyle]

LGTM

Is this considered a breaking change?

No this is bug fix. The option of using providing the params in the body is not documented but people may be relying on it so let's keep it. If it is not too complicated it would be good to check if the params are in both body and query params and error if they are different.

[jonathan-buttner]

Max and I talked about this offline but I wanted to make sure I don't forget haha.

If I specify a body setting the cache_size in serverless, this will result in false for the block: if (restRequest.hasParam(CACHE_SIZE.getPreferredName())) { and that means it'll get overridden with a cache size of 0 in serverless.

We'll need to add some logic to check if cache_size has been specified in the body and not override it in that scenario.

[maxhniebergall]

@elasticmachine merge upstream

[maxhniebergall]

@elasticmachine merge upstream

[maxhniebergall]

Now, if a parameter is provided in both the query params and body, an exception will be thrown if the parameters are different, unless the parameter provided in the body is the same as the default parameter.

[jonathan-buttner]

Let me know what you think of this: How about we determine which value to use by passing either the already set value from the body (request.get*()) or if that's null then a default value in the restRequest.param*() call?

I think that'd look something like this:

        var queueCap = request.getQueueCapacity();
        request.setQueueCapacity(
                restRequest.paramAsInt(QUEUE_CAPACITY.getPreferredName(), Objects.requireNonNullElse(queueCap, StartTrainedModelDeploymentAction.DEFAULT_QUEUE_CAPACITY))
        );

That way the query parameter will override the body if it is specified, or we'll take the body value, or if it wasn't set we'll either take the default already set when initializing the StartTrainedModelDeploymentAction.Request or we'll take the default passed to Objects.requireNonNullElse.

If I overlooked the logic and we still need this method then here are a few suggestions:
Let's make it static and I think it can be generic so we can remove the need to cast the return value.

[maxhniebergall]

I think the main difference from what you're proposing is that the sameParamInQueryAndBody throws an exception if the body does not match the query param.

Making the function static and generic does make sense though, I can do do that I think.

[maxhniebergall]

@elasticmachine merge upstream