Introduce XContentParser#namedObject #22003

nik9000 · 2016-12-06T17:38:55Z

Introduces XContentParser#namedObject which works a little like StreamInput#readNamedWriteable: on startup components register parsers under names and a superclass. At runtime we look up the parser and call it to parse the object.

Right now the parsers take a context object they use to help with the parsing but I hope to be able to eliminate the need for this context as most what it is used for at this point is to move around parser registries which should be replaced by this method eventually. I make no effort to do so in this PR because it is big enough already. This is meant to the a start down a road that allows us to remove classes like QueryParseContext, AggregatorParsers, IndicesQueriesRegistry, and ParseFieldRegistry.

The goal here is to reduce the amount of plumbing required to allow parsing pluggable things. With this you don't have to pass registries all over the place. Instead you must pass a super registry to fewer places and use it to wrap the reader. This is the same tradeoff that we use for NamedWriteable and it allows much, much simpler binary serialization. We think we want that same thing for xcontent serialization.

The only parsing actually converted to this method is parsing ScoreFunctions inside of FunctionScoreQuery. I chose this because it is relatively self contained.

nik9000 · 2016-12-06T17:39:29Z

core/src/main/java/org/elasticsearch/node/Node.java

                .flatMap(Function.identity()).collect(Collectors.toList());
            final NamedWriteableRegistry namedWriteableRegistry = new NamedWriteableRegistry(namedWriteables);
+            NamedXContentRegistry xContentRegistry = new NamedXContentRegistry(Stream.of(
+                    searchModule.getNamedXContents().stream()


I use this funny looking construct to match with the constructs above. It'll eventually have more than one module it has to stream, just like above.

nik9000 · 2016-12-06T17:39:59Z

core/src/main/java/org/elasticsearch/node/Node.java

                    b.bind(IndicesQueriesRegistry.class).toInstance(searchModule.getQueryParserRegistry());
                    b.bind(SearchRequestParsers.class).toInstance(searchModule.getSearchRequestParsers());
                    b.bind(SearchExtRegistry.class).toInstance(searchModule.getSearchExtRegistry());
+                    b.bind(NamedXContentRegistry.class).toInstance(xContentRegistry);


I hate binding another thing but in subsequent PRs I'll use this to remove a bunch of things.

nik9000 · 2016-12-06T17:41:43Z

I think at least @javanna, @rjernst, @martijnvg and maybe others might have an interest in this.

rjernst · 2016-12-06T19:09:00Z

I love the concept; it has been a pain to deal with those registries like QueryParserRegistry when trying to deguice the rest side of things. I left some comments about wrapping, as I think it makes things confusing and will be used wrong (hence all the need for the extra checks you have on passing wrapped vs unwrapped parsers).

nik9000 · 2016-12-06T19:29:11Z

I left some comments about wrapping

They didn't stick. Do you want to make the registry required for building the XContentParser or something like that?

rjernst · 2016-12-06T19:01:17Z

core/src/main/java/org/elasticsearch/common/xcontent/NamedXContentRegistry.java

+     */
+    public NamedXContentRegistry(List<Entry> entries) {
+        Map<Class<?>, Map<String, Entry>> registry = new HashMap<>();
+        for (Entry entry : entries) {


Can you make this algorithm the same as that in the NamedWriteableRegistry? I think it is cleaner (does not require replacing the inner maps to make them unmodifiable.

rjernst · 2016-12-06T19:05:51Z

core/src/main/java/org/elasticsearch/common/xcontent/NamedXContentRegistry.java

+     * Wrap an {@link XContentParser} in one that implements {@link XContentParser#namedObject(Class, String, ParseFieldMatcherSupplier)}
+     * against this registry.
+     */
+    public XContentParser wrap(XContentParser parser) {


Why isn't this inside xcontentfactory.creatParser? In fact why do we need a wrapper at all? Can't this just be native methods on XContentParser?

nik9000 · 2016-12-06T20:16:27Z

I talked with @rjernst over another channel. I'm going to implement the suggestion to use NamedWriteableRegistry's algorithm for building the map of maps. I'm going to try and make the registry a required parameter for building the XContentParser. That'll balloon this PR significantly but this is the right time for it. I'll try and take some shortcuts, mostly moving where the parsers are built so we don't have thousands of spots where we build the parsers. That also means I'll remove all the methods called wrap.

To get #22003 in cleanly we need to centralize as much `XContentParser` creation as possible into `RestRequest`. That'll mean we have to plumb the `NamedXContentRegistry` into fewer places. This removes `RestAction.hasBody`, `RestAction.guessBodyContentType`, and `RestActions.getRestContent`, moving callers over to `RestRequest.hasContentOrSourceParam`, `RestRequest.contentOrSourceParam`, and `RestRequest.contentOrSourceParamParser` and `RestRequest.withContentOrSourceParamParserOrNull`. The idea is to use `withContentOrSourceParamParserOrNull` if you need to handle requests without any sort of body content and to use `contentOrSourceParamParser` otherwise. I believe the vast majority of this PR to be purely mechanical but I know I've made the following behavioral change (I'll add more if I think of more): * If you make a request to an endpoint that requires a request body and has cut over to the new APIs instead of getting `Failed to derive xcontent` you'll get `Body required`. * Template parsing is now non-strict by default. This is important because we need to be able to deprecate things without requests failing.

nik9000 · 2016-12-16T15:45:49Z

OK! I've got the code compiling. I've taken a bunch of shortcuts, used NamedXContentRegistry.EMPTY in a bunch of places where it probably can't be forever. I'm now working to get the tests passing. Once they pass I'll update.

nik9000 · 2016-12-16T18:55:15Z

Everything is passing locally!

imotov

Left some comments

imotov · 2016-12-18T19:18:44Z

core/src/main/java/org/elasticsearch/common/xcontent/NamedXContentRegistry.java

When parsing stored metadata I will need a way to distinguish between unknown custom element and an error parsing such element. The former typically comes when elasticsearch starts after a plugin with custom metadata was removed from the cluster and therefore can be ignored and the latter means that something got very wrong and we cannot ignore it. I think this warrants some other exception.

Unfortunately we have to be lenient when reading cluster state with custom metadata...

(that is why we have all these lookupPrototypeSafe(...) and lookupPrototype(...) static methods on ClusterState, Metadata and IndexMetadata)

imotov · 2016-12-18T19:24:35Z

core/src/main/java/org/elasticsearch/action/admin/indices/create/CreateIndexRequest.java

NamedXContentRegistry.EMPTY is currently used in two use cases. 1) when we use XContentParser as a lexer to convert XContent from one format to another or convert it into map and 2) where XContentParser is feeding into a parser, which doesn't used named objects at the moment. As we discussed yesterday, it would be great to distinguish between these two use cases by moving all uses in the first category into helper methods.

nik9000 · 2016-12-19T14:58:29Z

@martijnvg and @imotov: for the unknown custom prototype stuff, would you prefer a new subclass of ParsingException or another version of namedObject that returns Optional and is empty when it can't find the right parser? I kind of prefer the former because that means I only need one method.

nik9000 · 2016-12-19T15:01:09Z

@imotov I pushed some commits that add an explanation comment for every non-test usage of NamedXContent.EMPTY and removes many of the usages.

imotov · 2016-12-19T15:19:32Z

@nik9000 @martijnvg the reason I asked to add another exception is because I think that a missing parser is still an error, but we react to this error in a different way comparing to the corrupted xcontent stream that we cannot parse. Returning Optional would work for me, but I think it wouldn't be semantically correct since in my mind it would indicate that object doesn't exist (rather than object exits but we cannot read it)

nik9000 · 2016-12-19T15:20:18Z

I'm happy to make a new exception!

nik9000 · 2016-12-19T18:51:52Z

@imotov, can you have another look?

imotov

Left a couple of minor comments. Otherwise LGTM.

imotov · 2016-12-19T22:58:11Z

core/src/main/java/org/elasticsearch/search/internal/AliasFilter.java

I am fairly confused about what's going on here. Could you add a comment or explain here?

imotov · 2016-12-19T23:04:53Z

core/src/test/java/org/elasticsearch/common/xcontent/BaseXContentTestCase.java

Yah. I'll rebase before merging and use it.

imotov · 2016-12-19T23:08:02Z

core/src/main/java/org/elasticsearch/repositories/blobstore/BlobStoreFormat.java

Actually, I will need the registry here, but for this PR, it's probably OK :)

imotov · 2016-12-19T23:19:19Z

core/src/main/java/org/elasticsearch/repositories/blobstore/BlobStoreRepository.java

EMPTY here is good though.

imotov · 2016-12-19T23:26:07Z

core/src/test/java/org/elasticsearch/indices/cluster/ClusterStateChanges.java

I think one of these nulls should be xContentRegistry. I will actually need it there.

imotov · 2016-12-19T23:27:10Z

core/src/test/java/org/elasticsearch/indices/cluster/ClusterStateChanges.java

and most likely here

imotov · 2016-12-20T00:44:55Z

core/src/main/java/org/elasticsearch/rest/action/search/RestMultiSearchAction.java

xContentRegistry is not used in this method

Introduces `XContentParser#namedObject which works a little like `StreamInput#readNamedWriteable`: on startup components register parsers under names and a superclass. At runtime we look up the parser and call it to parse the object. Right now the parsers take a context object they use to help with the parsing but I hope to be able to eliminate the need for this context as most what it is used for at this point is to move around parser registries which should be replaced by this method eventually. I make no effort to do so in this PR because it is big enough already. This is meant to the a start down a road that allows us to remove classes like `QueryParseContext`, `AggregatorParsers`, `IndicesQueriesRegistry`, and `ParseFieldRegistry`. The goal here is to reduce the amount of plumbing required to allow parsing pluggable things. With this you don't have to pass registries all over the place. Instead you must pass a super registry to fewer places and use it to wrap the reader. This is the same tradeoff that we use for NamedWriteable and it allows much, much simpler binary serialization. We think we want that same thing for xcontent serialization. The only parsing actually converted to this method is parsing `ScoreFunctions` inside of `FunctionScoreQuery`. I chose this because it is relatively self contained.

nik9000 · 2016-12-20T16:06:57Z

Thanks for reviewing @imotov! I've merged to:

master: a04dcfb
5.x: 08167d2

Introduces `XContentParser#namedObject which works a little like `StreamInput#readNamedWriteable`: on startup components register parsers under names and a superclass. At runtime we look up the parser and call it to parse the object. Right now the parsers take a context object they use to help with the parsing but I hope to be able to eliminate the need for this context as most what it is used for at this point is to move around parser registries which should be replaced by this method eventually. I make no effort to do so in this PR because it is big enough already. This is meant to the a start down a road that allows us to remove classes like `QueryParseContext`, `AggregatorParsers`, `IndicesQueriesRegistry`, and `ParseFieldRegistry`. The goal here is to reduce the amount of plumbing required to allow parsing pluggable things. With this you don't have to pass registries all over the place. Instead you must pass a super registry to fewer places and use it to wrap the reader. This is the same tradeoff that we use for NamedWriteable and it allows much, much simpler binary serialization. We think we want that same thing for xcontent serialization. The only parsing actually converted to this method is parsing `ScoreFunctions` inside of `FunctionScoreQuery`. I chose this because it is relatively self contained.

Removes `AggregatorParsers`, replacing all of its functionality with `XContentParser#namedObject`. This is the third bit of payoff from elastic#22003, one less thing to pass around the entire application.

Removes `AggregatorParsers`, replacing all of its functionality with `XContentParser#namedObject`. This is the third bit of payoff from #22003, one less thing to pass around the entire application.

nik9000 added >non-issue v5.2.0 v6.0.0-alpha1 WIP labels Dec 6, 2016

nik9000 commented Dec 6, 2016

View reviewed changes

nik9000 added review and removed WIP labels Dec 6, 2016

rjernst reviewed Dec 6, 2016

View reviewed changes

nik9000 added WIP and removed review labels Dec 6, 2016

clintongormley added the :Internal label Dec 7, 2016

nik9000 mentioned this pull request Dec 8, 2016

Begin centralizing XContentParser creation into RestRequest #22041

Merged

This was referenced Dec 10, 2016

Consolidate the last easy parser construction #22095

Merged

Start to centralize creation of XContentParser in tests #22096

Merged

Continue consolidating XContentParser construction in tests #22145

Merged

nik9000 force-pushed the named_xcontent branch from 9f046f9 to 55b24bb Compare December 16, 2016 15:31

nik9000 changed the title ~~Introduce XContentParser#namedXObject~~ Introduce XContentParser#namedObject Dec 16, 2016

nik9000 force-pushed the named_xcontent branch from 1f067ab to be1dad4 Compare December 16, 2016 17:17

nik9000 requested a review from imotov December 16, 2016 17:22

nik9000 added review and removed >non-issue WIP labels Dec 16, 2016

imotov suggested changes Dec 19, 2016

View reviewed changes

imotov approved these changes Dec 20, 2016

View reviewed changes

nik9000 added 2 commits December 20, 2016 10:01

Simplify request handling of registry

c7d8860

nik9000 force-pushed the named_xcontent branch from 09e2aec to c7d8860 Compare December 20, 2016 15:01

Fix warnings assertion

4a2e5ef

nik9000 merged commit a04dcfb into elastic:master Dec 20, 2016

nik9000 added >breaking-java and removed review labels Dec 20, 2016

This was referenced Dec 20, 2016

Replace IndicesQueriesRegistry #22289

Merged

Remove much ceremony from parsing client yaml test suites #22311

Merged

nik9000 mentioned this pull request Dec 31, 2016

Replace AggregatorParsers with namedObject #22397

Merged

Introduce XContentParser#namedObject #22003

Introduce XContentParser#namedObject #22003

Uh oh!

Conversation

nik9000 commented Dec 6, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nik9000 commented Dec 6, 2016

Uh oh!

rjernst commented Dec 6, 2016

Uh oh!

nik9000 commented Dec 6, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nik9000 commented Dec 6, 2016

Uh oh!

nik9000 commented Dec 16, 2016

Uh oh!

nik9000 commented Dec 16, 2016

Uh oh!

imotov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

martijnvg Dec 19, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nik9000 commented Dec 19, 2016

Uh oh!

nik9000 commented Dec 19, 2016

Uh oh!

imotov commented Dec 19, 2016

Uh oh!

nik9000 commented Dec 19, 2016

Uh oh!

nik9000 commented Dec 19, 2016

Uh oh!

imotov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nik9000 commented Dec 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

nik9000 commented Dec 6, 2016 •

edited

Loading

martijnvg Dec 19, 2016 •

edited

Loading

nik9000 commented Dec 20, 2016 •

edited

Loading