Add deprecation logging message for 'fuzzy' query #20993

cbuescher · 2016-10-18T13:54:28Z

The fuzzy query is deprecated from 5.0 on. Similar to IndicesQueryBuilder we should log a deprecation warning whenever this query is used.

Relates to #15760

cbuescher · 2016-10-18T13:55:19Z

This should also go to 5.x and maybe 5.0, since the query is deprecated from 5.0 on.

javanna

left one comment

javanna · 2016-10-18T15:29:56Z

core/src/main/java/org/elasticsearch/index/query/FuzzyQueryBuilder.java

@@ -151,6 +155,7 @@ public FuzzyQueryBuilder(String fieldName, boolean value) {
     * @param value The value of the term
     */
    public FuzzyQueryBuilder(String fieldName, Object value) {
+        DEPRECATION_LOGGER.deprecated("{} query is deprecated. Instead use the [match] query with fuzziness parameter", NAME);


add a test for this? If I remember correctly the deprecation logger is hooked into the response header warnings, hence testable from the outside without looking at logs.

I already ran into a failing docs test (the CONSOLE tests Nik added also barf if they receive a warning header that they don't expect). Would that be enough or do you think in terms of IT tests? I just pushed a change that checks for those headers in the docs test. Do you know if there's a way of checking this in the regular REST (yaml) tests?

Do you know if there's a way of checking this in the regular REST (yaml) tests?

You can look at the code generated by the docs tests and do what it does. You should add a feature section to the test so the clients don't choke on it.

Personally I'm fine with just asserting it in the docs tests. The build still fails if we remove it or change the message in some crazy way and that is all I'm looking for.

@javanna I looked into adding rest tests that check the warnings like @nik9000 suggested, I could add a test similar to the one that the docs-tests generate but there seems to be no other rest test for "fuzzy" at the moment and it feels strange to introduce tests for a deprecated query. As mentioned above, the docs tests already check the warning headers. Are you okay with leaving it like this?

I am on the fence. I think we should have a test for this, but I understand your objections. I am not sure we should rely on docs tests to verify this though. Docs tests are there to ensure that docs are correct, and we shouldn't be tempted to use them to verify that the code is correct. Ideally we should have a failing test besides docs if we remove the deprecation warning. Maybe add a unit test to fuzzy query builder tests that verifies that the deprecation log goes somewhere (in the ThreadContext). Is that feasible?

I'll see whats possible for the unit test, but not sure if the logger is accessible there. The other option would be yaml tests for fuzzy query (basically what the docs test produced, like @nik9000 suggested. I didn't do this yet because it would duplicate what the docs tests already do and also fuzzy is already on the way out (this whole PR started because I want to remove fuzzy on master on another branch that I have). I'll try to find a way to access the logger in the unit test.

I'm quite happy to use docs tests to verify that the code is correct in lots of cases, mostly the cases where you are going to document the feature anyway and there isn't a lot of value having all the clients test it. Then you get docs and you get tests. I don't think you should add useless docs just because they add tests, but I think we could do better to document more things are if that lets us skip adding yaml tests for them then great, incentive to do the right thing.

I like the yaml tests (and junit tests with rest client) to do more complex edge case stuff that you aren't going to document.

cbuescher · 2016-10-24T13:16:33Z

@javanna I added a test to the FuzzyQueryBuilder unit test, but I'm not too sure if this is the best way to test this. If you have any other idea how this can be done in our unit tests please let me know.

javanna · 2016-10-24T13:59:23Z

I added a test to the FuzzyQueryBuilder unit test, but I'm not too sure if this is the best way to test this. If you have any other idea how this can be done in our unit tests please let me know.

I would have done it exactly that way, this is how I envision we will test deprecations warnings from now on. What is wrong with that? :) Much better than having to write an integration test for it.

cbuescher · 2016-10-24T14:06:07Z

Thanks @javanna , I just wasn't sure setting the ThreadLocal on the logger in the test is the right way to go, since it might be used by other tests elsewhere (thats why I added those comments). So I assume this is okay then, should I merge this?

javanna · 2016-10-24T14:40:53Z

core/src/test/java/org/elasticsearch/index/query/FuzzyQueryBuilderTests.java

@@ -181,4 +187,16 @@ public void testParseFailsWithMultipleFields() throws IOException {
        e = expectThrows(ParsingException.class, () -> parseQuery(shortJson));
        assertEquals("[fuzzy] query doesn't support multiple fields, found [message1] and [message2]", e.getMessage());
    }
+
+    public void testDeprecationLoggingHeaders() throws IOException {
+        try (ThreadContext threadContext = new ThreadContext(Settings.EMPTY)) {


Maybe add the threadContext creation in the base class? and set it / remove it in the before /after test ?

I am on the fence. If other tests are executed concurrently, then adding the ThreadContext to the DeprecationLogger in the base class will mean that it potentially receives deprecation warnings from elsewhere. I was hoping to make this as local as possible so I'm almost sure that we test the following contructor call in isolation. So I'd rather add the context here, and even remove it from the Deprecation logger after this test is done. I'll push an update along that line.

JUnit doesn't execute tests concurrently in the same JVM. It doesn't even reuse the same instance for the test when it starts the next test, it builds a new one. So do whatever you think is the cleanest thing for you. I haven't read the code so I can't be sure, just this comment.

My two cents is that if the thread context is a thing you need for other tests it might be worth pulling out. You could pull it out and call verifyNoMoreInteractions on the warning logger somehow so all queries that make deprecated logging must be matched in their unit tests.

No tests will ever be executed concurrently. Removing the thread context is enough, I suspect that this is how we will test deprecation warnings once we remove strict parsing, which we currently use only for testing.

cbuescher · 2016-10-26T15:03:22Z

@javanna I decided to go the route suggested by @nik9000 and add the deprecation warning checks to all query test (and the QueryParseContextTests). I pushed a separate commit since this affects tests other than for fuzzy, so the scope got a big bigger than the original intention of this PR. I can also open a separate PR for this if you like. Basically we add a new ThreadContext to the DeprecationLogger in @Before and check that it is empty in @After in AbstractQueryTestCase. Each test that triggers some warning headers needs to check them explicitely (which will also reset the logger to a new ThreadContext if successful). I hope this is along the lines you envisioned it.

Edit: just realized that (at)Before and (at)After are users, sorry for the noise.

javanna

I left a few comments but I like the latest changes. This is totally going the direction I wanted it to go, thanks! You are already making changes that I had made when I first started working on #19552. With this new way of checking deprecation warnings, we almost don't need strict parsing anymore in our tests.

javanna · 2016-11-01T13:42:15Z

core/src/test/java/org/elasticsearch/index/query/BoolQueryBuilderTests.java

+        // we should have deprecation warning headers regardless of throwing and exception
+        checkWarningHeaders("query malformed, empty clause found at [1:27]",
+                "query malformed, empty clause found at [1:46]",
+                "query malformed, empty clause found at [1:100]");


nice! comment should be "throwing an exception" I guess.

javanna · 2016-11-01T13:42:37Z

core/src/test/java/org/elasticsearch/index/query/FuzzyQueryBuilderTests.java

+     * all tests create deprecation warnings through the builders constructor, those are
+     * checked once here after test it run
+     */
+    @After void checkWarningHeaders() {


missing line break after annotation?

javanna · 2016-11-01T13:44:02Z

core/src/test/java/org/elasticsearch/index/query/FuzzyQueryBuilderTests.java

@@ -38,6 +39,14 @@

 public class FuzzyQueryBuilderTests extends AbstractQueryTestCase<FuzzyQueryBuilder> {

+    /**
+     * all tests create deprecation warnings through the builders constructor, those are
+     * checked once here after test it run


what do you mean by "the builders constructor" ? Should it be "after each test is run" ?

I mean new FuzzyQueryBuilder(), will rephrase that.

javanna · 2016-11-01T13:44:53Z

core/src/test/java/org/elasticsearch/index/query/IndicesQueryBuilderTests.java


 import java.io.IOException;

 public class IndicesQueryBuilderTests extends AbstractQueryTestCase<IndicesQueryBuilder> {

+    /**
+     * all tests create deprecation warnings through the builders constructor, those are
+     * checked once here after test it run


same comments as above in FuzzyQueryBuilderTests

javanna · 2016-11-01T13:45:50Z

core/src/test/java/org/elasticsearch/index/query/QueryParseContextTests.java

+
+    @After
+    public void teardown() throws IOException {
+        this.threadContext.close();


shouldn't we do DeprecationLogger.removeThreadContext first?

javanna · 2016-11-01T13:46:55Z

...lang-mustache/src/test/java/org/elasticsearch/script/mustache/TemplateQueryBuilderTests.java

@@ -56,6 +57,14 @@
     */
    private QueryBuilder templateBase;

+    /**
+     * all tests create deprecation warnings through the builders constructor, those are
+     * checked once here after test it run


same comments as above

javanna · 2016-11-01T13:49:02Z

test/framework/src/main/java/org/elasticsearch/test/AbstractQueryTestCase.java

+            assertThat(warnings, hasItem(equalTo(msg)));
+        }
+        // "clear" current warning headers by setting a new ThreadContext
+        DeprecationLogger.removeThreadContext(this.threadContext);


I think that we are leaking a thread local here? We should close the current threadContext before overriding it.

javanna · 2016-11-01T13:51:33Z

test/framework/src/main/java/org/elasticsearch/test/AbstractQueryTestCase.java

+    /**
+     * used to check warning headers of the deprecation logger
+     */
+    protected ThreadContext threadContext;


does this need to be protected? I think it is safer if it is private and the only way to access it is via checkWarningHeaders

javanna · 2016-11-01T13:53:07Z

test/framework/src/main/java/org/elasticsearch/test/AbstractQueryTestCase.java

+        // "clear" current warning headers by setting a new ThreadContext
+        DeprecationLogger.removeThreadContext(this.threadContext);
+        this.threadContext = new ThreadContext(Settings.EMPTY);
+        DeprecationLogger.setThreadContext(this.threadContext);


It would be nice to find another way to do this other than replacing the thread context. I had a look but didn't find other ways though :) I guess you have tried as well.

We would need to be able to reset the warnings-map somehow, but the way ThreadContext currently works it won't let you modify its internal maps, and I think thats there for a reason. I agree this is not super nice but since its test code I think its okay.

agreed that's ok

javanna · 2016-11-01T13:54:06Z

test/framework/src/main/java/org/elasticsearch/test/AbstractQueryTestCase.java

+    @After
+    public void teardown() throws IOException {
+        final List<String> warnings = threadContext.getResponseHeaders().get(DeprecationLogger.DEPRECATION_HEADER);
+        if (warnings != null) {


do we expect null or empty when there are no warnings? seems like both are ok but two invariants are not needed and only one should be the right one?

I think null is what you get back from the response headers map if there are no "Warning" entries.

cbuescher · 2016-11-01T17:44:35Z

@javanna thanks, I rebased and added a commit trying to address your comments.

javanna

left one comment on using assumptions, LGTM otherwise

javanna · 2016-11-01T21:08:36Z

core/src/test/java/org/elasticsearch/index/query/FuzzyQueryBuilderTests.java

@@ -107,7 +113,12 @@ public void testUnsupportedFuzzinessForStringType() throws IOException {
    }

    public void testToQueryWithStringField() throws IOException {
-        assumeTrue("test runs only when at least a type is registered", getCurrentTypes().length > 0);
+        try {
+            assumeTrue("test runs only when at least a type is registered", getCurrentTypes().length > 0);


shall we move to using @ConditionalIgnore here. If I understood correctly in that case before and after wouldn't be executed for ignored methods, hence you wouldn't have to keep track of whether the test ran yourself

I didn't know that one, great suggestion. I will try if this works.

me neither, I did some googling :)

Unfortunately @ConditionalIgnore doesn't seem to be part of junits standard set of annotations yet (seems to be targeted for junit 5). Rather than build our own or import one of the custom solutions I found, I'd keep the current solution. This whole test class is going away with the deprecated query builder anyway soon (will stay on the 5.x branch for it's lifetime however).

I added comments for this.

javanna

LGTM

cbuescher · 2016-11-02T11:10:09Z

@javanna thanks, I'll rebase an squash a bit, then merge this to master and 5.x

This query is deprecated from 5.0 on. Similar to IndicesQueryBuilder we should log a deprecation warning whenever this query is used. Relates to elastic#15760

…ContextTests This adds checks for expected warning headers to the query builder test infrastructure. Tests that are adding deprecation warnings to the response headers need to check those, otherwise the abstract base class for the test class will complain at teardown.

cbuescher · 2016-11-02T15:51:19Z

Merged to 5.x with 56cffc6 and 5806bb6

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 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 #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 #19552

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

cbuescher added review >deprecation v6.0.0-alpha1 labels Oct 18, 2016

cbuescher force-pushed the add-fuzzy-query-deprecationLogging branch from 01b13cb to c653a6c Compare October 18, 2016 13:57

javanna requested changes Oct 18, 2016

View reviewed changes

cbuescher force-pushed the add-fuzzy-query-deprecationLogging branch from c653a6c to 5d9d827 Compare October 18, 2016 17:08

javanna reviewed Oct 24, 2016

View reviewed changes

cbuescher force-pushed the add-fuzzy-query-deprecationLogging branch 2 times, most recently from 42b0b0d to ac47823 Compare October 26, 2016 14:56

javanna requested changes Nov 1, 2016

View reviewed changes

cbuescher force-pushed the add-fuzzy-query-deprecationLogging branch from ac47823 to de524de Compare November 1, 2016 17:43

javanna requested changes Nov 1, 2016

View reviewed changes

cbuescher added the v5.1.1 label Nov 2, 2016

javanna approved these changes Nov 2, 2016

View reviewed changes

cbuescher force-pushed the add-fuzzy-query-deprecationLogging branch from df45662 to a04d570 Compare November 2, 2016 11:33

cbuescher added 2 commits November 2, 2016 12:33

Add deprecation logging message for 'fuzzy' query

8286d72

This query is deprecated from 5.0 on. Similar to IndicesQueryBuilder we should log a deprecation warning whenever this query is used. Relates to elastic#15760

cbuescher force-pushed the add-fuzzy-query-deprecationLogging branch from a04d570 to 2136ff7 Compare November 2, 2016 11:33

cbuescher merged commit b3370de into elastic:master Nov 2, 2016

cbuescher removed the review label Nov 2, 2016

javanna mentioned this pull request Dec 12, 2016

Replace strict parsing mode with response headers assertions #22130

Merged

clintongormley added :Search/Search Search-related issues that do not fall into other categories and removed :Query DSL labels Feb 14, 2018

Add deprecation logging message for 'fuzzy' query #20993

Add deprecation logging message for 'fuzzy' query #20993

Uh oh!

Conversation

cbuescher commented Oct 18, 2016

Uh oh!

cbuescher commented Oct 18, 2016

Uh oh!

javanna 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!

cbuescher commented Oct 24, 2016

Uh oh!

javanna commented Oct 24, 2016

Uh oh!

cbuescher commented Oct 24, 2016

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!

cbuescher commented Oct 26, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

javanna 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!

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!

cbuescher commented Nov 1, 2016

Uh oh!

javanna left a comment

Choose a reason for hiding this comment

Uh oh!

cbuescher commented Oct 26, 2016 •

edited

Loading