Collect config for XContentParser #79814

nik9000 · 2021-10-26T14:20:16Z

This shifts the internal API for building an XContentParser to make
maintenance easier.

To build an XContentParser you need three things:

An XContentType that knows whether you are reading json, smile,
yaml, or cbor
Configuration information
A place to get the bytes

Years ago, there wasn't any configuration. But we've since added stuff
like NamedXContentRegistry and DeprecationHandler and
RestApiVersion and, more lately, filtering configuration. Each time we
added one we modified the method signature to add it. Each "place to get
the bytes" also needs its own method. All of which are implemented by
each XContentType. All together we had dozens of these methods for
different ways of building parser. All because of the combinatorial
explosion of config x type x source.

This tames the combinatorial explosion somewhat by bundling all of the
configuration into an immutable XContentParserConfiguration. Sadly, it
has to add createParser methods that consume the configuration objects.
Luckily, we can remove some of the old methods. We could remove all of
them, but that'd make this change huge. Instead we deprecate the old
methods and point them to the new ones.

This shifts the internal API for building an `XContentParser` to make maintenance easier. To build an `XContentParser` you need three things: 1. An `XContentType` that knows whether you are reading json, smile, yaml, or cbor 2. Configuration information 3. A place to get the bytes Years ago, there wasn't any configration. But we've since added stuff like `NamedXContentRegistry` and `DeprecationHandler` and `RestApiVersion` and, more lately, filtering configuration. Each time we added one we modified the method signature to add it. Each "place to get the bytes" also needs its own method. All of which are implemented by each `XContentType`. All together we had dozens of these methods for different ways of building parser. All because of the combinatorial explosion of `config x type x source`. This tames the combinatorial explosion somewhat by bundling all of the configuration into an immutable `XContentParserConfiguration`. Sadly, it has to add `createParser` methods that consume the configuration objects. Luckilly, we can remove some of the old methods. We *could* remove all of them, but that'd make this change *huge*. Instead we deprecate the old methods and point them to the new ones.

elasticmachine · 2021-10-26T14:20:19Z

Pinging @elastic/es-core-infra (Team:Core/Infra)

pgomulka

LGTM - great work :)
the only real concern I have is with the PLAIN name

btw we have a similar issue with XContentBuilder constructors :)

pgomulka · 2021-10-27T07:34:00Z

server/src/main/java/org/elasticsearch/action/search/MultiSearchRequest.java

@@ -207,7 +215,7 @@ public static void readMultiLineFormat(BytesReference data,
                break;
            }
            // support first line with \n
-            if (restApiVersion == RestApiVersion.V_7 && nextMarker == 0) {
+            if (config.restApiVersion() == RestApiVersion.V_7 && nextMarker == 0) {


might be worth calling it parserConfig as it is in other places. Not because of consistency, but because I think it reads better

pgomulka · 2021-10-27T07:43:35Z

server/src/main/java/org/elasticsearch/rest/RestRequest.java

    public RestApiVersion getRestApiVersion() {
-        return restApiVersion;
+        return parserConfig.restApiVersion();


just a thought..
restApiVersion is used for both XContentParser and XContentBuilder
we create a builder with

new XContentBuilder(XContentFactory.xContent(responseContentType), unclosableOutputStream, includes, excludes, responseMediaType, request.getRestApiVersion());

in AbstractRestChannel. I wonder if it would be confusing that the version is taken from a request which takes it from parserConfig? Obviously we now know that it only wrapps parameters..

I wonder if maybe for RestRequest it would make sense to keep the restApiVersion field together with parserConfig. Or would it be more complex ?
I am a little biased, because when working on RestApiCompatibility I got used to thinking that RestRequest keeps the version.
How does this read to you?

tldr ;] I am not convinced myself about this, but might be worth taking a second look at this class

I think it's ok to keep it separate. It'd be worried if they didn't line up but it'd be fine to add an assertion I think.

pgomulka · 2021-10-27T07:44:30Z

libs/x-content/src/main/java/org/elasticsearch/xcontent/XContentParserConfiguration.java

+ * Configuration for {@link XContentParser}.
+ */
+public class XContentParserConfiguration {
+    public static final XContentParserConfiguration PLAIN = new XContentParserConfiguration(


how about DEFAULT? or EMPTY?
PLAIN reminds me of plaint/text and I think it would be better away from that

That's a pretty compelling reason not to call it PLAIN. I think EMPTY is pretty good though. It sort of implies at least a chunk of the configuration it encodes.

…_parser_config

nik9000 · 2021-10-27T17:39:45Z

@elasticmachine update branch

In elastic#79814 I deprecated many variants of `XContent#createParser` in favor of some new variants that bundle many of the arguments. This removes one of the deprecated variants by converting all call sites to the new variant.

In elastic#79814 I deprecated many variants of XContent#createParser in favor of some new variants that bundle many of the arguments. This replaces the variants used by the search execution context with the new variants.

In #79814 I deprecated many variants of `XContent#createParser` in favor of some new variants that bundle many of the arguments. This removes one of the deprecated variants by converting all call sites to the new variant.

In elastic#79814 I deprecated many variants of XContent#createParser in favor of some new variants that bundle many of the arguments. This replaces the variants used by the search execution context with the new variants.

This removes one of the `createParser` methods that I deprecated in elastic#79814, migrating callers to one of the new methods that it created.

This removes one of the `createParser` methods that I deprecated in #79814, migrating callers to one of the new methods that it created.

This removes one of the `createParser` methods that I deprecated in elastic#79814, migrating callers to one of the new methods that it created.

This removes one of the `createParser` methods that I deprecated in #79814, migrating callers to one of the new methods that it created.

This removes many calls to the last remaining `createParser` method that I deprecated in elastic#79814, migrating callers to one of the new methods that it created.

This removes many calls to the last remaining `createParser` method that I deprecated in #79814, migrating callers to one of the new methods that it created.

nik9000 added >non-issue :Core/Infra/Core Core issues without another label v8.0.0 labels Oct 26, 2021

nik9000 requested a review from pgomulka October 26, 2021 14:20

elasticmachine added the Team:Core/Infra Meta label for core/infra team label Oct 26, 2021

Merge branch 'master' into xcontent_parser_config

796bf5e

pgomulka approved these changes Oct 27, 2021

View reviewed changes

nik9000 added 13 commits October 27, 2021 10:10

Merge branch 'master' into xcontent_parser_config

4fe5bc9

Rename

53d4c87

Restore field

cd8e874

More cleanup

d8d82c4

Merge commit '4e09186b3d3a20e97e44713d4200b22bc3cec332' into xcontent…

b9201d6

…_parser_config

format

90aae4d

Merge commit '12ad399c488f0cc60e19b5e1b29c6d569cb4351a' into xcontent…

0b749af

…_parser_config

Merge branch 'master' into xcontent_parser_config

857e774

Sneaky

b48653b

Better?

ceeed9e

Another

86ccf08

More

37a9181

Merge branch 'master' into xcontent_parser_config

2d79c7c

Merge branch 'master' into xcontent_parser_config

2c4f5ed

nik9000 added v8.1.0 auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) and removed v8.0.0 labels Oct 27, 2021

elasticsearchmachine merged commit a8258bd into elastic:master Oct 27, 2021

nik9000 mentioned this pull request Oct 27, 2021

Remove createParser variant #79954

Merged

nik9000 mentioned this pull request Nov 1, 2021

Switch SearchExecutionContext to new createParser #80161

Closed

nik9000 added a commit to nik9000/elasticsearch that referenced this pull request Feb 23, 2022

Remove deprecated xcontent method

0d0712f

This removes one of the `createParser` methods that I deprecated in elastic#79814, migrating callers to one of the new methods that it created.

nik9000 mentioned this pull request Feb 23, 2022

Remove deprecated xcontent method #84314

Merged

nik9000 added a commit that referenced this pull request Feb 24, 2022

Remove deprecated xcontent method (#84314)

6c70578

This removes one of the `createParser` methods that I deprecated in #79814, migrating callers to one of the new methods that it created.

nik9000 mentioned this pull request Feb 24, 2022

Remove deprecated xcontent method #84372

Merged

elasticsearchmachine pushed a commit that referenced this pull request Feb 28, 2022

Remove deprecated xcontent method (#84314) (#84372)

3c66958

This removes one of the `createParser` methods that I deprecated in #79814, migrating callers to one of the new methods that it created.

nik9000 mentioned this pull request Mar 7, 2022

Remove calls to deprecated xcontent method #84733

Merged

ywangd mentioned this pull request Mar 9, 2022

NPE when auditing REST requests with filtered fields #84784

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Collect config for XContentParser #79814

Collect config for XContentParser #79814

Uh oh!

nik9000 commented Oct 26, 2021

Uh oh!

elasticmachine commented Oct 26, 2021

Uh oh!

pgomulka left a comment

Uh oh!

pgomulka Oct 27, 2021

Uh oh!

nik9000 Oct 27, 2021

Uh oh!

pgomulka Oct 27, 2021

Uh oh!

nik9000 Oct 27, 2021

Uh oh!

pgomulka Oct 27, 2021

Uh oh!

nik9000 Oct 27, 2021

Uh oh!

nik9000 commented Oct 27, 2021

Uh oh!

Uh oh!

Collect config for XContentParser #79814

Collect config for XContentParser #79814

Uh oh!

Conversation

nik9000 commented Oct 26, 2021

Uh oh!

elasticmachine commented Oct 26, 2021

Uh oh!

pgomulka left a comment

Choose a reason for hiding this comment

Uh oh!

pgomulka Oct 27, 2021

Choose a reason for hiding this comment

Uh oh!

nik9000 Oct 27, 2021

Choose a reason for hiding this comment

Uh oh!

pgomulka Oct 27, 2021

Choose a reason for hiding this comment

Uh oh!

nik9000 Oct 27, 2021

Choose a reason for hiding this comment

Uh oh!

pgomulka Oct 27, 2021

Choose a reason for hiding this comment

Uh oh!

nik9000 Oct 27, 2021

Choose a reason for hiding this comment

Uh oh!

nik9000 commented Oct 27, 2021

Uh oh!

Uh oh!