WIP media type registry with singleton instance

[pgomulka]

• Have you signed the contributor license agreement?
• Have you followed the contributor guidelines?
• If submitting code, have you built your formula locally prior to submission with gradle check?
• If submitting code, is your pull request against master? Unless there is a good reason otherwise, we prefer pull requests against master and will backport as needed.
• If submitting code, have you checked that your submission is for an OS and architecture that we support?
• If you are submitting this code for a class then read our policy for that.

[pgomulka]

this looks like it works..
do plugins' classloader inherit classloader from main server?
but I am not super sure about the design of this too. It is a static common instance. Probably not ideal.

But on the other hand I don't want to try injecting this from Node class..

[jakelandis]

I am not a big fan of the singleton, nor a static approach. Globals should generally be avoided.

do plugins' classloader inherit classloader from main server?

yeah, they are children and can see the main loader, but not other plugins

Taking a step back ... we really trying to take a string and find the associated mime type and all the associated parameters. However, not all mime types are valid for all APIs ... for example, we don't want to support application/txt for _search, and need to avoid registering application/txt such that a request coming through for _search would be valid. Since we are heading to a more strict support, i believe that strict support needs to be tightly coupled to the Route ie. /_search vs. /_sql. Therefor, I think the concept of a registry needs to be associated with the route and the backing state probably in BaseRestHandler or RestControler.

        return List.of(
            new Route(GET, "/_search", STANDARD_MEDIA_TYPES, COMPAT_MEDIA_TYPES),

and

        return List.of(
            new Route(GET, Protocol.SQL_QUERY_REST_ENDPOINT, TextFormat),

We can probably default the standard and compat media types to avoid too much copy,pasta but it would allow per route to opt-in/out of compatibility. There might be some mimatch in the supported accept vs. supported content-type's ...but I would not worry about that for now.

This requires moving where the header -> ParsedMedia type happens, for that I am not sure ... it can happen really early in the processing , even before handleRequest. maybe in dispacthRequest (request.addParsedMediaType(mediaParser.parse(handler.getSupportedMediaTypes())))?

I think by "registering" the supported media types that is tightly coupled to the Route will help in the long term with stricter parsing.

[pgomulka]

I think mapping of media types with API endpoints is something in addition to a "media type registry"
Correlating allowed media types with routes would be a bigger refactoring. It would fit into Make media type parsing strict #63080

The compatible api plugin PR (which requires a media type registry) and Make media type parsing strict I feel are separate. It is a question if this should be worked on first?

On the registry and where it should live:
Problem with making it an instance within RestController or BaseRestHandler would require to pass that registry instance to many other places (wherever XContentType/TextFormat are created)

I agree that making it static is not great, but XContentType is an enum and its creation methods are static (fromFormat or fromMediaType) . Because of this, registry will have to be accessed from a static context anyway.
XContentType can access a static field (like in this PR - a static MediaTypeParser field on XcontentType access a static field on MediaTypeRegistry)
or from a Node class will set a static field on a XContentType.

WDYT?

[jaymode]

My take is that we should not introduce a global static like this and to keep the concerns separate.

For registration of media types, we can have a registry but this should never be passed to plugins; instead we should pull from plugins. This may mean there are inefficiencies but I think we will be much happier with the code.

For plugins, we pull additional media types:

interface ActionPlugin {
     default List<MediaTypeDefintiion> getAdditionalMediaTypes() {
         return List.of(); // sql would do csv, etc
     }
}

For rest handlers we define supported media types:

interface RestHandler {
    // may want to split this into two methods; eg allowedContentMediaTypes and allowedAcceptMediaTypes since we accept ndjson but don't output it and it may be the same with SQL?
    default Set<MediaType> supportedMediaTypes() {
        return XContentType.mediaTypes(); // this would include the regular and vendor specific along with parameters (required/optional)
    }
}

For parsing, we maintain a MediaTypeParser that is built from the values pulled from plugins and XContentType and put the parsed values (MediaType) on the request for both content type and accept (output type). Our code will now need to map from MediaType to XContentType or TextFormat. Those mappings should be handled within XContentType and TextFormat with a fromMediaType(MediaType) method.

In terms of other changes that may make sense with this suggestion, I would consider changing the compatible plugin API to use the parsed values for media type rather than the raw strings.

Once we start enforcing what we accept for these values including parameters, then the RestController can check this against the set of values defined by the handler.

I think there are some holes in my idea, but if you like it I am happy to chat more about it.

[pgomulka]

I think I see what you mean.
In your idea I think we still miss the part where we share the instance of MediaTypeRegistry with other "clients" (XContentType or TextFormat or CompatibleVersionPlugin).

The idea looks similar to what we do with DiscoveryNode.additionalRoles
This mean that we still have to maintain a static global variable where we will store all of the additional media types. (may be encapsulated within MediaTypeRegistry). It is easy to set this on XContentType in a Node class with sth like XContentType.setMediaTypeRegistry (invoked very early in the Node constructor)
That means that XContentType would have to have a static variable to store a MediaTypeRegistry

TextFormat would have to reach to the same instance of media type registry (XContentTYpe.getMediaTypeRgistry ?).
The same applies to CompatibleVersionPlugin where we don't define a new media types, but we want to have access to all defined media types (including additional ones from other plugins like sql)

[pgomulka]

CompatibleVersionPlugin has to be able to parse all the media types, including those defined in other plugins.
For instance.
plain/text media type is defined in SQL plugin, we can pull it from it from that plugin on startup using getAdditionalMediaTypes.
To allow CompatibleVersionPlugin to perform all the validations it is doing now, it has to be able to parse media types defined by SQL

Why you wanted to avoid sharing MediaTypeRegistry ?

[jaymode]

Why you wanted to avoid sharing MediaTypeRegistry ?

Mainly to avoid having one more class passed around everywhere.

CompatibleVersionPlugin has to be able to parse all the media types, including those defined in other plugins.

I understand that CompatibleVersionPlugin needs the media types; could we not pass it the ParsedMediaType objects and have it operate off of that? This way we keep our media type parsing out of this as well and only the output object is consumed by the CompatibleVersionPlugin?

[pgomulka]

I will give it a go and create a draft pr
to me it looks it very similar. Instead of passing down MediaTypeRegistry to a plugin we would have to pass it down to RestController.

[pgomulka]

@jaymode I updated current PR with the approach where we pull the supported media types.
I no longer set them on a plugin with just a setter. I have changed the plugin to expect a media type registry.
This is not visible as I have hidden that when creating a lambda that is passed under CompatibleVersion interface
Let me know what you think

This of course does not addresses @jakelandis points on per route parsing.

[jaymode]

I opened #27 as a idea of what I think we can do to just use the parsed values