Replace strict parsing mode with response headers assertions #22130
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
javanna
force-pushed
the
enhancement/remove_strict_parsing_mode
branch
from
cb4c1fd
to
4d7ae2e
Compare
nik9000
approved these changes
Dec 13, 2016
Agreed, I think we could check the responses through java api or REST client depending on the test, but that can come later, if we need it. At least now we check for warnings in all the unit tests, which is already much more than what we did until now. Thanks for your review ;) |
Yeah! That is much nicer. And we check the wording. Which I see you fixed in a bunch of places because it was funky. Nice! |
jpountz
approved these changes
Dec 14, 2016
| import static org.hamcrest.CoreMatchers.containsString; | ||
| import static org.hamcrest.Matchers.hasToString; |
There was a problem hiding this comment.
Can you avoid reorganizinge the imports?
There was a problem hiding this comment.
I was reverting this bit, funnily enough this same import order change came in through another PR: b9600c7#diff-41da0c9cbefe797a78bab64005bf2b94 . :)
| @@ -118,7 +112,7 @@ public void testToQuery() throws IOException { | |||
|
|
|||
| public void testExceptionOnMissingTypes() throws IOException { | |||
| assumeTrue("test runs only when at least a type is registered", getCurrentTypes().length == 0); | |||
| QueryShardException e = expectThrows(QueryShardException.class, () -> super.testToQuery()); | |||
| QueryShardException e = expectThrows(QueryShardException.class, super::testToQuery); | |||
javanna
force-pushed
the
enhancement/remove_strict_parsing_mode
branch
4 times, most recently
from
8d5c772
to
ddd8613
Compare
We return deprecation warnings as response headers, besides logging them. Strict parsing mode stayed around, but was only used in query tests, though we also introduced checks for deprecation warnings there that don't need strict parsing anymore (see elastic#20993). We can then safely remove support for strict parsing mode. The final goal is to remove the ParseFieldMatcher class, but there are many many users of it. This commit prepares the field for the removal, by deprecating ParseFieldMatcher and making it effectively not needed. Strict parsing is removed from ParseFieldMatcher, and strict parsing is replaced in tests where needed with deprecation warnings checks. Note that the setting to enable strict parsing was never ported to the new settings infra hance it cannot be set in production. It is really only used in our own tests. Relates to elastic#19552
We are currenlty checking that no deprecation warnings are emitted in our query tests. That can be moved to ESTestCase (disabled in ESIntegTestCase) as it allows us to easily catch where our tests use deprecated features and assert on the expected warnings.
javanna
force-pushed
the
enhancement/remove_strict_parsing_mode
branch
from
ddd8613
to
26867c1
Compare
javanna
added a commit
to javanna/elasticsearch
that referenced
this pull request
Dec 30, 2016
… directly Relates to elastic#19552 Relates to elastic#22130
javanna
added a commit
to javanna/elasticsearch
that referenced
this pull request
Dec 30, 2016
…Field directly Relates to elastic#19552 Relates to elastic#22130
javanna
added a commit
to javanna/elasticsearch
that referenced
this pull request
Dec 30, 2016
…Field directly Relates to elastic#19552 Relates to elastic#22130
javanna
added a commit
to javanna/elasticsearch
that referenced
this pull request
Dec 30, 2016
… directly Relates to elastic#19552 Relates to elastic#22130
javanna
added a commit
to javanna/elasticsearch
that referenced
this pull request
Dec 30, 2016
…Field directly Relates to elastic#19552 Relates to elastic#22130
javanna
added a commit
to javanna/elasticsearch
that referenced
this pull request
Dec 30, 2016
…Field directly Relates to elastic#19552 Relates to elastic#22130
javanna
added a commit
to javanna/elasticsearch
that referenced
this pull request
Dec 30, 2016
… directly Relates to elastic#19552 Relates to elastic#22130
javanna
added a commit
to javanna/elasticsearch
that referenced
this pull request
Dec 31, 2016
… directly Relates to elastic#19552 Relates to elastic#22130
This was referenced Dec 31, 2016
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is the first step towards removing
ParseFieldMatcher. The only reason to have this class is strict parsing, which is only used in our test infra. Because it depends on a setting (although this setting was never settable by users), we were moved all of theParseFieldchecks toParseFieldMatcherwhen it was introduced (see #11859), which was quite an intrusive change in our parsing code. We now have a deprecation logger, and we return warning headers whenever some deprecated feature is used throughout the execution of a request. The functionality of strict parsing can either be moved to clients, or re-implemented without the need forParseFieldMatcher, by rather leveraging our current infrastructure based onThreadContextandDeprecationLogger. Another disadvantage ofParseFieldMatcheris that it's completely decoupled from the deprecation logger, hence it became hard to keep the two in sync. Also, we ended up creating many copies ofParseFieldMatcherin our codebase that cause confusion.This PR deprecates
ParseFieldMatcherand makes it not useful as it removes support for strict parsing. All tests that used to rely on strict parsing are now moved to checking warning headers, introduced first with #20993 in query tests. Even more, we can now move the warnings check fromAbstractQueryTestCasetoESTestCaseand test more closely deprecation warnings, as well as easily catch which tests use deprecated features. This can only be leveraged in unit tests or single node test (where the only node runs in the same jvm as the test).Once this change goes in, we can go and step by step remove all the usages of
ParseFieldMatcher, which will be a mechanical change (a big one), and finally remove the class.Relates to #19552