Skip to content

WIP media type registry with singleton instance #24

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
pgomulka wants to merge 7 commits into
base: compat_plugin_inside_rest_request
Choose a base branch
from
Draft

WIP media type registry with singleton instance #24

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
ed14aae
media type registry with singleton instance
pgomulka Oct 7, 2020
1b0c1d2
pull supported media types, but push down registry
pgomulka Oct 12, 2020
7bdf64f
cleanup
pgomulka Oct 12, 2020
ff93fd2
remove mediatypedefinition
pgomulka Oct 13, 2020
f5c60ff
passing a mediatyperegistry to plugin, but keeping the signature the …
pgomulka Oct 15, 2020
5ad227a
do not use global media type registry in sql
pgomulka Oct 15, 2020
58d1103
Merge branch 'compat/pull_supported_media_types' into compat/media_ty…
pgomulka Oct 15, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
64 changes: 33 additions & 31 deletions libs/x-content/src/main/java/org/elasticsearch/common/xcontent/MediaTypeParser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,27 +25,22 @@
import java.util.regex.Pattern;

public class MediaTypeParser<T extends MediaType> {
private final Map<String, T> formatToMediaType;
private final Map<String, T> typeWithSubtypeToMediaType;
private final Map<String, Map<String, Pattern>> parametersMap;

public MediaTypeParser(Map<String, T> formatToMediaType, Map<String, T> typeWithSubtypeToMediaType,
Map<String, Map<String, Pattern>> parametersMap) {
this.formatToMediaType = Map.copyOf(formatToMediaType);
this.typeWithSubtypeToMediaType = Map.copyOf(typeWithSubtypeToMediaType);
this.parametersMap = Map.copyOf(parametersMap);
}
private MediaTypeRegistry mediaTypeRegistry;

public MediaTypeParser(MediaTypeRegistry mediaTypeRegistry) {
this.mediaTypeRegistry = mediaTypeRegistry;
}
@SuppressWarnings("unchecked")
public T fromMediaType(String mediaType) {
ParsedMediaType parsedMediaType = parseMediaType(mediaType);
return parsedMediaType != null ? parsedMediaType.getMediaType() : null;
return parsedMediaType != null ? (T)parsedMediaType.getMediaType() : null;
}

@SuppressWarnings("unchecked")
public T fromFormat(String format) {
if (format == null) {
return null;
}
return formatToMediaType.get(format.toLowerCase(Locale.ROOT));
return (T)mediaTypeRegistry.formatToMediaType(format.toLowerCase(Locale.ROOT));
}

/**
Expand All @@ -65,7 +60,7 @@ public ParsedMediaType parseMediaType(String headerValue) {
String type = typeSubtype[0];
String subtype = typeSubtype[1];
String typeWithSubtype = type + "/" + subtype;
T xContentType = typeWithSubtypeToMediaType.get(typeWithSubtype);
MediaType xContentType = mediaTypeRegistry.typeWithSubtypeToMediaType(typeWithSubtype);
if (xContentType != null) {
Map<String, String> parameters = new HashMap<>();
for (int i = 1; i < split.length; i++) {
Expand All @@ -90,8 +85,8 @@ public ParsedMediaType parseMediaType(String headerValue) {
}

private boolean isValidParameter(String typeWithSubtype, String parameterName, String parameterValue) {
if (parametersMap.containsKey(typeWithSubtype)) {
Map<String, Pattern> parameters = parametersMap.get(typeWithSubtype);
if (mediaTypeRegistry.parametersFor(typeWithSubtype) != null) {
Map<String, Pattern> parameters = mediaTypeRegistry.parametersFor(typeWithSubtype);
if (parameters.containsKey(parameterName)) {
Pattern regex = parameters.get(parameterName);
return regex.matcher(parameterValue).matches();
Expand All @@ -104,19 +99,32 @@ private boolean hasSpaces(String s) {
return s.trim().equals(s) == false;
}

private static final String COMPATIBLE_WITH_PARAMETER_NAME = "compatible-with";

public Byte parseVersion(String mediaType) {
ParsedMediaType parsedMediaType = parseMediaType(mediaType);
if (parsedMediaType != null) {
String version = parsedMediaType
.getParameters()
.get(COMPATIBLE_WITH_PARAMETER_NAME);
return version != null ? Byte.parseByte(version) : null;
}
return null;
}

/**
* A media type object that contains all the information provided on a Content-Type or Accept header
*/
public class ParsedMediaType {
private final Map<String, String> parameters;
private final T mediaType;
private final MediaType mediaType;

public ParsedMediaType(T mediaType, Map<String, String> parameters) {
public ParsedMediaType(MediaType mediaType, Map<String, String> parameters) {
this.parameters = parameters;
this.mediaType = mediaType;
}

public T getMediaType() {
public MediaType getMediaType() {
return mediaType;
}

Expand All @@ -126,11 +134,11 @@ public Map<String, String> getParameters() {
}

public static class Builder<T extends MediaType> {
private final Map<String, T> formatToMediaType = new HashMap<>();
private final Map<String, T> typeWithSubtypeToMediaType = new HashMap<>();
private final Map<String, MediaType> formatToMediaType = new HashMap<>();
private final Map<String, MediaType> typeWithSubtypeToMediaType = new HashMap<>();
private final Map<String, Map<String, Pattern>> parametersMap = new HashMap<>();

public Builder<T> withMediaTypeAndParams(String alternativeMediaType, T mediaType, Map<String, String> paramNameAndValueRegex) {
public Builder<T> withMediaTypeAndParams(String alternativeMediaType, MediaType mediaType, Map<String, String> paramNameAndValueRegex) {
typeWithSubtypeToMediaType.put(alternativeMediaType.toLowerCase(Locale.ROOT), mediaType);
formatToMediaType.put(mediaType.format(), mediaType);

Expand All @@ -146,15 +154,9 @@ public Builder<T> withMediaTypeAndParams(String alternativeMediaType, T mediaTyp
return this;
}

public Builder<T> copyFromMediaTypeParser(MediaTypeParser<? extends T> mediaTypeParser) {
formatToMediaType.putAll(mediaTypeParser.formatToMediaType);
typeWithSubtypeToMediaType.putAll(mediaTypeParser.typeWithSubtypeToMediaType);
parametersMap.putAll(mediaTypeParser.parametersMap);
return this;
}

public MediaTypeParser<T> build() {
return new MediaTypeParser<>(formatToMediaType, typeWithSubtypeToMediaType, parametersMap);
public MediaTypeParser<T> build(MediaTypeRegistry mediaTypeRegistry) {
mediaTypeRegistry.register(formatToMediaType, typeWithSubtypeToMediaType, parametersMap);
return new MediaTypeParser<T>(mediaTypeRegistry);
}
}
}
54 changes: 54 additions & 0 deletions libs/x-content/src/main/java/org/elasticsearch/common/xcontent/MediaTypeRegistry.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
/*
* Licensed to Elasticsearch under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch licenses this file to you under
* the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

package org.elasticsearch.common.xcontent;

import java.util.Map;
import java.util.concurrent.ConcurrentHashMap;
import java.util.regex.Pattern;

public class MediaTypeRegistry {
private static final MediaTypeRegistry INSTANCE = new MediaTypeRegistry();
private Map<String, MediaType> formatToMediaType = new ConcurrentHashMap<>();
private Map<String, MediaType> typeWithSubtypeToMediaType = new ConcurrentHashMap<>();
private Map<String, Map<String, Pattern>> parametersMap= new ConcurrentHashMap<>();

public static MediaTypeRegistry getInstance() {
return INSTANCE;
Copy link
Owner Author

@pgomulka pgomulka Oct 7, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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..

Copy link

@jakelandis jakelandis Oct 7, 2020

Choose a reason for hiding this comment

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

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.

Copy link
Owner Author

@pgomulka pgomulka Oct 8, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link

@jaymode jaymode Oct 8, 2020

Choose a reason for hiding this comment

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

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.

Copy link
Owner Author

@pgomulka pgomulka Oct 12, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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)

Copy link
Owner Author

@pgomulka pgomulka Oct 13, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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 ?

Copy link

@jaymode jaymode Oct 13, 2020

Choose a reason for hiding this comment

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

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?

Copy link
Owner Author

@pgomulka pgomulka Oct 14, 2020

Choose a reason for hiding this comment

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

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.

Copy link
Owner Author

@pgomulka pgomulka Oct 15, 2020

Choose a reason for hiding this comment

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

@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.

Copy link

@jaymode jaymode Oct 16, 2020

Choose a reason for hiding this comment

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

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

}

public <T extends MediaType> void register(Map<String, T> formatToMediaType, Map<String, T> typeWithSubtypeToMediaType, Map<String, Map<String, Pattern>> parametersMap) {
this.formatToMediaType.putAll(formatToMediaType);
this.typeWithSubtypeToMediaType.putAll(typeWithSubtypeToMediaType);
this.parametersMap.putAll(parametersMap);
}


public MediaType formatToMediaType(String format) {
return formatToMediaType.get(format);
}

public MediaType typeWithSubtypeToMediaType(String typeWithSubtype) {
return typeWithSubtypeToMediaType.get(typeWithSubtype);
}

public Map<String, Pattern> parametersFor(String typeWithSubtype) {
return parametersMap.get(typeWithSubtype);
}
}
2 changes: 1 addition & 1 deletion libs/x-content/src/main/java/org/elasticsearch/common/xcontent/XContentType.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,7 @@ public XContent xContent() {
Map.of(COMPATIBLE_WITH_PARAMETER_NAME, VERSION_PATTERN, "charset", "UTF-8"))
.withMediaTypeAndParams("application/vnd.elasticsearch+x-ndjson", JSON,
Map.of(COMPATIBLE_WITH_PARAMETER_NAME, VERSION_PATTERN, "charset", "UTF-8"))
.build();
.build(MediaTypeRegistry.getInstance());

/**
* Accepts a format string, which is most of the time is equivalent to {@link XContentType#subtype()}
Expand Down
28 changes: 25 additions & 3 deletions libs/x-content/src/test/java/org/elasticsearch/common/xcontent/MediaTypeParserTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,12 +29,12 @@
import static org.hamcrest.Matchers.nullValue;

public class MediaTypeParserTests extends ESTestCase {

MediaTypeParser<XContentType> mediaTypeParser = new MediaTypeParser.Builder<XContentType>()
MediaTypeRegistry mediaTypeRegistry = new MediaTypeRegistry();
MediaTypeParser<XContentType> mediaTypeParser = new MediaTypeParser.Builder()
.withMediaTypeAndParams("application/vnd.elasticsearch+json",
XContentType.JSON, Map.of("compatible-with", "\\d+",
"charset", "UTF-8"))
.build();
.build(mediaTypeRegistry);

public void testJsonWithParameters() throws Exception {
String mediaType = "application/vnd.elasticsearch+json";
Expand Down Expand Up @@ -74,4 +74,26 @@ public void testInvalidParameters() {
assertThat(mediaTypeParser.parseMediaType(mediaType + "; key=") ,
is(nullValue()));
}

public void testVersionParsing() {
byte version = (byte) Math.abs(randomByte());
assertThat(mediaTypeParser.parseVersion("application/vnd.elasticsearch+json;compatible-with=" + version),
equalTo(version));
assertThat(mediaTypeParser.parseVersion("application/json"),
nullValue());


assertThat(mediaTypeParser.parseVersion("APPLICATION/VND.ELASTICSEARCH+JSON;COMPATIBLE-WITH=" + version),
equalTo(version));
assertThat(mediaTypeParser.parseVersion("APPLICATION/JSON"),
nullValue());

assertThat(mediaTypeParser.parseVersion("application/json;compatible-with=" + version + ".0"),
is(nullValue()));
}

public void testUnrecognizedParameter() {
assertThat(mediaTypeParser.parseVersion("application/json; sth=123"),
is(nullValue())); }

}
9 changes: 7 additions & 2 deletions ...in/rest-compatibility/src/main/java/org/elasticsearch/compat/CompatibleVersionPlugin.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,18 +8,23 @@
import org.elasticsearch.ElasticsearchStatusException;
import org.elasticsearch.Version;
import org.elasticsearch.common.Nullable;
import org.elasticsearch.common.xcontent.MediaType;
import org.elasticsearch.common.xcontent.MediaTypeParser;
import org.elasticsearch.common.xcontent.MediaTypeRegistry;
import org.elasticsearch.common.xcontent.XContentType;
import org.elasticsearch.plugins.Plugin;
import org.elasticsearch.plugins.RestCompatibilityPlugin;
import org.elasticsearch.rest.RestStatus;

public class CompatibleVersionPlugin extends Plugin implements RestCompatibilityPlugin {

MediaTypeParser<MediaType> mediaTypeParser = new MediaTypeParser.Builder<>()
.build(MediaTypeRegistry.getInstance());
@Override
public Version getCompatibleVersion(@Nullable String acceptHeader, @Nullable String contentTypeHeader, boolean hasContent) {
Byte aVersion = XContentType.parseVersion(acceptHeader);
Byte aVersion = mediaTypeParser.parseVersion(acceptHeader);
byte acceptVersion = aVersion == null ? Version.CURRENT.major : Integer.valueOf(aVersion).byteValue();
Byte cVersion = XContentType.parseVersion(contentTypeHeader);
Byte cVersion = mediaTypeParser.parseVersion(contentTypeHeader);
byte contentTypeVersion = cVersion == null ? Version.CURRENT.major : Integer.valueOf(cVersion).byteValue();

// accept version must be current or prior
Expand Down
11 changes: 9 additions & 2 deletions x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/SqlMediaTypeParser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@

import org.elasticsearch.common.xcontent.MediaType;
import org.elasticsearch.common.xcontent.MediaTypeParser;
import org.elasticsearch.common.xcontent.MediaTypeRegistry;
import org.elasticsearch.common.xcontent.XContentType;
import org.elasticsearch.rest.RestRequest;
import org.elasticsearch.xpack.sql.action.SqlQueryRequest;
Expand All @@ -19,15 +20,21 @@

public class SqlMediaTypeParser {
private static final MediaTypeParser<? extends MediaType> parser = new MediaTypeParser.Builder<>()
.copyFromMediaTypeParser(XContentType.mediaTypeParser)
.withMediaTypeAndParams(TextFormat.PLAIN_TEXT.typeWithSubtype(), TextFormat.PLAIN_TEXT,
Map.of("header", "present|absent", "charset", "utf-8"))
.withMediaTypeAndParams(TextFormat.CSV.typeWithSubtype(), TextFormat.CSV,
Map.of("header", "present|absent", "charset", "utf-8",
"delimiter", ".+"))// more detailed parsing is in TextFormat.CSV#delimiter
.withMediaTypeAndParams(TextFormat.TSV.typeWithSubtype(), TextFormat.TSV,
Map.of("header", "present|absent", "charset", "utf-8"))
.build();
.withMediaTypeAndParams("text/vnd.elasticsearch+plain", TextFormat.PLAIN_TEXT,
Map.of("header", "present|absent", "charset", "utf-8", "compatible-with", "\\d+"))
.withMediaTypeAndParams("text/vnd.elasticsearch+csv", TextFormat.CSV,
Map.of("header", "present|absent", "charset", "utf-8",
"delimiter", ".+", "compatible-with", "\\d+"))// more detailed parsing is in TextFormat.CSV#delimiter
.withMediaTypeAndParams("text/vnd.elasticsearch+tsv", TextFormat.TSV,
Map.of("header", "present|absent", "charset", "utf-8", "compatible-with", "\\d+"))
.build(MediaTypeRegistry.getInstance());

/*
* Since we support {@link TextFormat} <strong>and</strong>
Expand Down