Remove QueryParseContext from parsing QueryBuilders#25448
Remove QueryParseContext from parsing QueryBuilders#25448cbuescher merged 2 commits intoelastic:masterfrom
Conversation
Currently QueryParseContext is only a thin wrapper around an XContentParser that adds little functionality of its own. I provides helpers for long deprecated field names which can be removed and two helper methods that can be made static and moved to other classes. This is a first step in helping to remove QueryParseContext entirely.
| @@ -42,75 +34,4 @@ public QueryParseContext(XContentParser parser) { | |||
| public XContentParser parser() { | |||
| return this.parser; | |||
| } | |||
There was a problem hiding this comment.
I'm leaving this stub of the class here for now, otherwise the PR would have gotten very big. I have a follow up almost ready that removes the whole class, but that requires removal from all kinds of method signatures, aggregations etc...
nik9000
left a comment
There was a problem hiding this comment.
I think now is a good time for this!
| while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) { | ||
| if (token == XContentParser.Token.FIELD_NAME) { | ||
| currentFieldName = parser.currentName(); | ||
| } else if (parseContext.isDeprecatedSetting(currentFieldName)) { |
There was a problem hiding this comment.
This breaks 1.x compatibility, which seems fine to me! Probably worth adding a note to the migration docs just in case.
There was a problem hiding this comment.
I will add a short note about those two fields.
| * Creates a new {@link QueryBuilder} from the query held by the {@link QueryParseContext} | ||
| * in {@link org.elasticsearch.common.xcontent.XContent} format | ||
| * Creates a new {@link QueryBuilder} from the query held by the | ||
| * {@link QueryParseContext} in |
There was a problem hiding this comment.
This is not longer correct - more like "Creates a new QueryBuilder from the query represented by the xcontent in the parser." or something.
| * the input parse context. The state on the parser contained in | ||
| * this context will be changed as a side effect of this method | ||
| * call | ||
| * @param parser |
There was a problem hiding this comment.
At this point I don't know that @param adds anything either.
| } | ||
| } | ||
|
|
||
| public void testParseInnerQueryBuilder() throws IOException { |
There was a problem hiding this comment.
Probably worth moving this to AbstractQueryBuilder.
There was a problem hiding this comment.
Already part of a follow-up PR. I can do it now if you prefer, but the test is split there anyway.
There was a problem hiding this comment.
Part of the followup is fine with me!
|
Very happy to have this coming in! Glad to see the end of this tunnel. |
|
@nik9000 thanks for the review, after this context is gone there might be possibilities for more cleanups down the road. |
|
Awesome that this can go!!! |
* master: (129 commits) Add doc note regarding explicit publish host Fix typo in name of test Add additional test for sequence-number recovery WrapperQueryBuilder should also rewrite the parsed query. Remove dead code and stale Javadoc Update defaults in documentation (#25483) [DOCS] Add docs-dir to Painless (#25482) Add concurrent deprecation logger test [DOCS] Update shared attributes for Elasticsearch (#25479) Use LRU set to reduce repeat deprecation messages Add NioTransport threads to thread name checks (#25477) Add shortcut for AbstractQueryBuilder.parseInnerQueryBuilder to QueryShardContext Prevent channel enqueue after selector close (#25478) Fix Java 9 compilation issue Remove unregistered `transport.netty.*` settings (#25476) Handle ping correctly in NioTransport (#25462) Tests: Remove platform specific assertion in NioSocketChannelTests Remove QueryParseContext from parsing QueryBuilders (#25448) Promote replica on the highest version node (#25277) test: added not null assertion ...
Currently QueryParseContext is only a thin wrapper around an XContentParser that
adds little functionality of its own. It only provides helpers for long deprecated
field names which can be removed, and two helper methods that can be made static
and moved to other classes. This is a first step in helping to remove
QueryParseContext.