Add validation unit tests for empty and malformed queries (#33095) #33862

cbismuth · 2018-09-19T14:36:21Z

This pull request adds unit tests to leverage REST query validation action logic. It checks for ParsingException when malformed or empty queries are sent.

javanna

I left a couple of small comments, looks good overall. thanks @cbismuth for your contribution.

javanna · 2018-09-19T14:49:28Z

.../src/test/java/org/elasticsearch/rest/action/admin/indices/RestValidateQueryActionTests.java

+
+        expectParsingException(content);
+    }
+


maybe also add a test for a query that is valid?

Yes, I'll add it.

javanna · 2018-09-19T14:49:41Z

.../src/test/java/org/elasticsearch/rest/action/admin/indices/RestValidateQueryActionTests.java

+        final RestRequest request = new FakeRestRequest
+            .Builder(xContentRegistry())
+            .withPath("index1/type1/_validate/query")
+            .withParams(new HashMap<>())


use Collections.emptyMap() instead?

javanna · 2018-09-19T14:49:58Z

.../src/test/java/org/elasticsearch/rest/action/admin/indices/RestValidateQueryActionTests.java

+        expectParsingException(content);
+    }
+
+    private void expectParsingException(String content) {


could this be static?

Finally no, as xContentRegistry() method is inherited from ESTestCase parent test class.

javanna · 2018-09-19T14:51:38Z

.../src/test/java/org/elasticsearch/rest/action/admin/indices/RestValidateQueryActionTests.java

+        final ValidateQueryRequest validateQueryRequest = new ValidateQueryRequest("index1");
+
+        expectThrows(ParsingException.class,
+            () -> request.withContentOrSourceParamParserOrNull(parser -> validateQueryRequest.query(RestActions.getQueryContent(parser))));


I wonder if we want to take this one step further and check that the rest action not only calls this code, but also catches the exception. What do you think?

I would be better, I'm working on it, thank you @javanna.

cbismuth · 2018-09-19T20:47:35Z

@javanna I've updated test cases to maximize coverage.

I had to disable o.e.c.IndicesAdminClient#validateQuery() validation API in tests, as it would require an integration test to get a live IndicesAdminClient instance. Do you think we should develop such a test?

javanna

I have left two suggestions on how to improve the tests, LGTM otherwise. Thanks @cbismuth !

javanna · 2018-09-20T11:20:25Z

.../src/test/java/org/elasticsearch/rest/action/admin/indices/RestValidateQueryActionTests.java

+    public void setUp() throws Exception {
+        super.setUp();
+
+        doNothing().when(action).validateQuery(eq(client), any(ValidateQueryRequest.class), any(RestChannel.class));


what do you think of the following lines instead of adding the validateQuery method to the rest action?

TestThreadPool threadPool = new TestThreadPool(RestValidateQueryActionTests.class.getName()); NodeClient client = new NodeClient(settings, threadPool); TaskManager taskManager = new TaskManager(Settings.EMPTY, threadPool, Collections.emptySet()); TransportAction transportAction = new TransportAction(Settings.EMPTY, ValidateQueryAction.NAME, new ActionFilters(Collections.emptySet()), taskManager) { @Override protected void doExecute(Task task, ActionRequest request, ActionListener listener) { } }; Map<Action, TransportAction> actions = new HashMap<>(); actions.put(ValidateQueryAction.INSTANCE, transportAction); client.initialize(actions, () -> "local", null);

I would do this from a static beforeClass method given that it's the same config for all methods. We need to remember to close the thread pool also at the end.

I really do prefer your solution, as it doesn't require to modify production code for testing purpose only. Moreover, it's a great pattern to remember! Thank you @javanna 👍

javanna · 2018-09-20T11:20:58Z

.../src/test/java/org/elasticsearch/rest/action/admin/indices/RestValidateQueryActionTests.java

+        doNothing().when(action).validateQuery(eq(client), any(ValidateQueryRequest.class), any(RestChannel.class));
+    }
+
+    public void testRestValidateQueryAction_validQuery() throws Exception {


I think that we don't need to repeat the action name in every method name, leave testValidQuery for this one? Same for the other methods.

Totally 👍

cbismuth · 2018-09-20T11:56:53Z

PR updated with suggested changes, thank you @javanna 👍

javanna

left a couple more comments, thanks again @cbismuth

javanna · 2018-09-21T08:56:03Z

.../src/test/java/org/elasticsearch/rest/action/admin/indices/RestValidateQueryActionTests.java

+    private static final TestThreadPool threadPool = new TestThreadPool(RestValidateQueryActionTests.class.getName());
+    private static final NodeClient client = new NodeClient(settings, threadPool);
+
+    private final UsageService usageService = new UsageService(settings);


given that it's always the same instances used, maybe we could declare these three static too and initialize them only once.

Totally, thank you.

javanna · 2018-09-21T09:00:01Z

.../src/test/java/org/elasticsearch/rest/action/admin/indices/RestValidateQueryActionTests.java

+public class RestValidateQueryActionTests extends AbstractSearchTestCase {
+
+    private static final Settings settings = Settings.EMPTY;
+    private static final TestThreadPool threadPool = new TestThreadPool(RestValidateQueryActionTests.class.getName());


It would be wise to close the threadpool in an afterclass method, also assign all of the static variables to null. We have checks for threads and objects that are left behind which might trip if we are not careful.

javanna · 2018-09-21T09:00:09Z

.../src/test/java/org/elasticsearch/rest/action/admin/indices/RestValidateQueryActionTests.java

+
+    private final UsageService usageService = new UsageService(settings);
+    private final RestController controller = new RestController(settings, emptySet(), null, client, null, usageService);
+    private final RestValidateQueryAction action = spy(new RestValidateQueryAction(settings, controller));


I think that you don't need to spy on this anymore.

Yes, you're right.

cbismuth · 2018-09-21T09:22:36Z

You're welcome @javanna, PR updated, thank you for your detailed review, I'm learning a lot 👍

javanna · 2018-09-21T09:32:58Z

.../src/test/java/org/elasticsearch/rest/action/admin/indices/RestValidateQueryActionTests.java

+
+    @AfterClass
+    public static void terminateTestThreadPool() throws InterruptedException {
+        terminate(threadPool);


can you assign all of the static variables to null as part of this method?

javanna · 2018-09-21T09:34:15Z

.../src/test/java/org/elasticsearch/rest/action/admin/indices/RestValidateQueryActionTests.java

+
+public class RestValidateQueryActionTests extends AbstractSearchTestCase {
+
+    private static final Settings settings = Settings.EMPTY;


maybe we can remove this given that we always use empty and use empty directly instead?

I extracted it to not cause hard line break at 140 char line width and please Checkstyle. But I can break lines, no worries.

EDIT no need to break lines.

But you're right non-empty settings would probably be different for each allocated object.

cbismuth · 2018-09-21T09:56:48Z

PR updated, thank you @javanna.

javanna

LGTM thanks @cbismuth I will get this in as soon as I get a green build.

javanna · 2018-09-21T11:19:44Z

test this please

cbismuth · 2018-09-21T13:50:37Z

Great! Thank you very much for your time and comments @javanna, they will help me a lot to make new contributions.

javanna · 2018-09-21T15:16:31Z

test this please

javanna · 2018-09-24T09:22:00Z

thanks @cbismuth !

elasticmachine · 2018-09-24T09:22:16Z

Pinging @elastic/es-search-aggs

cbismuth · 2018-09-24T09:37:48Z

Thank you @javanna 😉

Relates to #33095

Add validation unit tests for empty and malformed queries (#33095)

5f096c6

cbismuth mentioned this pull request Sep 19, 2018

Validate API with empty query returns HTTP code 400 instead of 200 and "valid": false #33095

Closed

Fix unit test name (#33095)

3b0b35f

javanna requested changes Sep 19, 2018

View reviewed changes

Test validation from the action handler and check a valid query (#33095)

b9397e4

javanna requested changes Sep 20, 2018

View reviewed changes

Stub ValidateQueryAction instead of RestValidateQueryAction (#33095)

11e3a06

javanna requested changes Sep 21, 2018

View reviewed changes

Improve test implementation with @javanna code review (#33095)

f50b184

javanna reviewed Sep 21, 2018

View reviewed changes

Allow test static references to be garbage collected (#33095)

8d465e4

javanna approved these changes Sep 21, 2018

View reviewed changes

javanna merged commit 47ed6c7 into elastic:master Sep 24, 2018

javanna added the :Search/Search Search-related issues that do not fall into other categories label Sep 24, 2018

javanna added >enhancement >test Issues or PRs that are addressing/adding tests v7.0.0 v6.5.0 backport pending and removed >enhancement labels Sep 24, 2018

javanna pushed a commit that referenced this pull request Sep 25, 2018

[TEST] Add validate query tests for empty and malformed queries (#33862)

dfdd7b9

Relates to #33095

javanna removed the backport pending label Sep 25, 2018

cbismuth deleted the 33095_add_rest_validate_query_action_tests branch October 25, 2018 17:39

kcm pushed a commit that referenced this pull request Oct 30, 2018

[TEST] Add validate query tests for empty and malformed queries (#33862)

2c8f7ba

Relates to #33095

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019


		public class RestValidateQueryActionTests extends AbstractSearchTestCase {

		private static final Settings settings = Settings.EMPTY;

Add validation unit tests for empty and malformed queries (#33095) #33862

Add validation unit tests for empty and malformed queries (#33095) #33862

Conversation

cbismuth commented Sep 19, 2018

javanna left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cbismuth commented Sep 19, 2018 • edited Loading

javanna left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cbismuth commented Sep 20, 2018

javanna left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cbismuth commented Sep 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cbismuth Sep 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cbismuth commented Sep 21, 2018

javanna left a comment

Choose a reason for hiding this comment

javanna commented Sep 21, 2018

cbismuth commented Sep 21, 2018

javanna commented Sep 21, 2018

javanna commented Sep 24, 2018

elasticmachine commented Sep 24, 2018

cbismuth commented Sep 24, 2018

cbismuth commented Sep 19, 2018 •

edited

Loading

cbismuth Sep 21, 2018 •

edited

Loading