Add parsing from xContent to SearchResponse #22533

cbuescher · 2017-01-10T16:22:23Z

In preparation to be able to parse SearchResponse from its rest representation
for the java rest client, this adds fromXContent to SearchResponse. Most of the
information in the original object is preserved when parsing it back. However,
the exceptions in the "failure" section won't be identical to the original ones
on the server side since they are parsed back to a generic
ElasticsearchException on the receiving side. Also the "aggregations", "suggest"
and "profile" section parsing is currently skipped and will be added by
subsequent PRs.

In preparation to be able to parse SearchResponse from its rest representation for the java rest client, this adds fromXContent to SearchResponse. Most of the information in the original object is preserved when parsing it back. However, the exceptions in the "failure" section won't be identical to the original ones on the server side since they are parsed back to a generic ElasticsearchException on the receiving side. Also the "aggregations", "suggest" and "profile" section parsing is currently skipped and will be added by subsequent PRs.

javanna

left some comments, looks good though. I would start a new feature branch based on this given that the parsing will be completed only once we have also done profile, suggest and aggs sections which will take a while.

javanna · 2017-01-12T12:40:43Z

core/src/main/java/org/elasticsearch/action/search/SearchResponse.java

+                if (Fields._SCROLL_ID.equals(currentFieldName)) {
+                    scrollId = parser.text();
+                } else if (Fields.TOOK.equals(currentFieldName)) {
+                    tookInMillis = parser.intValue();


isn't took a long?

right, changes that

javanna · 2017-01-12T12:43:38Z

core/src/main/java/org/elasticsearch/action/search/SearchResponse.java

+                    timedOut = parser.booleanValue();
+                } else if (Fields.TERMINATED_EARLY.equals(currentFieldName)) {
+                    terminatedEarly = parser.booleanValue();
+                }


move the else to this line?

javanna · 2017-01-12T12:50:20Z

core/src/main/java/org/elasticsearch/action/search/SearchResponse.java

        return Strings.toString(this);
    }
+
+    public static class InternalSearchResponse {


just wondering, do we even need this class? Shall we rather collapse all of its fields to SearchResponse ?

I think this is still needed as its own class because it is used as an "incomplete" search response in e.g. SeachPhaseController#merge() and in some other places where some information we need for the SearchResponse ctor is not available yet. I also wanted to keep the amount of changes small, so I'd prefer to leave it for now. This would be another refactoring that is not necessary for parsing.

that's fine.

javanna · 2017-01-12T12:54:11Z

core/src/main/java/org/elasticsearch/action/search/SearchResponse.java

        return Strings.toString(this);
    }
+
+    public static class InternalSearchResponse {


shouldn't this still implement Streamable if we keep it?

It could, it still has the read/write methods. However, they should not be used anywhere else than in SearchResponse, so I'd go one step in the other direction and even reduce the visibility of write/read.

ok because although the class can be created separately, it should always be used as part of the SearchResponse when serializing?

I just checked where InternalSearchResponse is used, all places that I found where this is instantiated it is later wrapped in a SearchResponse in the same method, so I think it's never sent across the wire independently.

javanna · 2017-01-12T12:55:54Z

core/src/main/java/org/elasticsearch/rest/action/RestActions.java

                                   response.getShardFailures());
    }

+    public static final class ShardsFields {


do we need this inner class? I think we tend to be removing those when we find them rather than adding new ones

sure, I will remove it

javanna · 2017-01-12T13:02:46Z

core/src/test/java/org/elasticsearch/action/search/SearchResponseTests.java

+
+        // the "_shard/total/failures" section makes if impossible to directly compare xContent, because
+        // the failures in the parsed SearchResponse are wrapped in an extra ElasticSearchException on the client side.
+        // Because of this we compare the "top level" fields for equality and the subsections xContent equivalence independently


just an idea, but maybe we could instead generate random response without exceptions and check that like we usually do, and have specific tests for exceptions? Otherwise exceptions affect the way we test the rest of the response which doesn't sound optimal.

Good point, I will change this. However, I'd like the createTestItem() method to still be able to output an object with set "failures", so I will make this controllable from the outside. In our "normal" fromXContent() test we can use xContent comparison again then, In another test with "failures" at the moment the only thing we can really check is that we can parse the response without throwing an error I think. Other more detailed tests are covered by ShardSearchFailureTests already I think.

javanna · 2017-01-12T13:03:36Z

core/src/test/java/org/elasticsearch/action/search/SearchResponseTests.java

+        SearchResponse response = new SearchResponse(
+                new InternalSearchResponse(new InternalSearchHits(hits, 100, 1.5f), null, null, null, false, null), null, 0, 0, 0,
+                new ShardSearchFailure[0]);
+        BytesReference xContent = toXContent(response, XContentType.JSON);


you could call Strings.toString instead I think

Good to know.

javanna · 2017-01-12T13:04:36Z

core/src/test/java/org/elasticsearch/action/search/ShardSearchFailureTests.java

+
+    public void testFromXContent() throws IOException {
+        ShardSearchFailure response = createTestItem();
+        XContentType xcontentType = XContentType.JSON; //randomFrom(XContentType.values());


javanna · 2017-01-12T13:05:45Z

core/src/test/java/org/elasticsearch/action/search/ShardSearchFailureTests.java

+        assertEquals(response.shardId(), parsed.shardId());
+
+        // we cannot compare the cause, because it will be wrapped in an outer ElasticSearchException
+        // best effort: try to check that the original message appears somewhere in the renderes xContent


s/renderes/rendered . Instead, we could build what we expect given the exception? Isn't it just wrapping what we have into an ElasticsearchException with the same message?

javanna · 2017-01-12T13:07:40Z

test/framework/src/main/java/org/elasticsearch/test/hamcrest/ElasticsearchAssertions.java

+            assertMapEquals((Map<String, Object>) expected, (Map<String, Object>) actual, path);
        } else if (expected instanceof List) {
-            assertListEquals((List<Object>) expected, (List<Object>) actual);
+            assertListEquals((List<Object>) expected, (List<Object>) actual, path);


do we still need these changes?

I added these path arguments to be able to better debug errors in map-comparisons. When we compare xContent and some nested map has different size, we currently only get the expected and actual size as output. This is very useful to print in the test error message, so I'd like to leave it.

I don't follow, there is a new argument to these methods that always gets passed in as "" ? or what am I missing?

I got it, ok!

In assertToXContentEquivalent() we start with an empty path. Whenever assertMapEquals() finds an expected key, we append that to the path. Whenever an expected value or a map size differs, we additionally print the collected path information up until then. Its basically a recursion, "path" being the collector. the assertListEquals() method also needs the argument because we need to pass it on for other nested objects inside.

tlrx

I like it, thanks for doing it @cbuescher. I think it makes sense to have a dedicated branch until every pieces are merged in, though you could already add ShardSearchFailure changes in its own pull request.

tlrx · 2017-01-12T13:23:17Z

core/src/main/java/org/elasticsearch/action/search/SearchResponse.java

+                            } else {
+                                throwUnknownField(currentFieldName, parser.getTokenLocation());
+                            }
+                        }


I think it needs another else block with throwUnknownField(currentFieldName, parser.getTokenLocation()) here

Right, on that level its more like an unexpected token, we have another helper for that which I will use.

tlrx · 2017-01-12T13:25:15Z

core/src/main/java/org/elasticsearch/action/search/SearchResponse.java

+            }
+        }
+        return new SearchResponse(new InternalSearchResponse(hits, null, null, null, timedOut, terminatedEarly),
+                scrollId, totalShards, successfulShards, tookInMillis, failures.toArray(new ShardSearchFailure[0]));


Can we use failures.toArray(new ShardSearchFailure[failures.size()]) ?

Sure, good point.

tlrx · 2017-01-12T13:28:32Z

core/src/main/java/org/elasticsearch/action/search/ShardSearchFailure.java

+    private static final String REASON_FIELD = "reason";
+    private static final String NODE_FIELD = "node";
+    private static final String INDEX_FIELD = "index";
+    private static final String SHARD_FIELD = "shard";


I think in other classes we didn't add a _FIELD suffix

I just added the _FIELD suffix to other classes in this PR, I think it better communicates that this is a field name.

tlrx · 2017-01-12T13:29:40Z

core/src/main/java/org/elasticsearch/action/search/ShardSearchFailure.java

+                    throwUnknownField(currentFieldName, parser.getTokenLocation());
+                }
+            } else {
+                throw new ParsingException(parser.getTokenLocation(),


XContentParserUtils.throwUnknownToken() instead?

Yes, forgot about that for a second

tlrx · 2017-01-12T13:31:28Z

core/src/main/java/org/elasticsearch/rest/action/RestActions.java

-        builder.field("total", total);
-        builder.field("successful", successful);
-        builder.field("failed", failed);
+        builder.startObject(ShardsFields._SHARDS);


I think it would make sense, yes.

tlrx · 2017-01-12T13:33:46Z

core/src/main/java/org/elasticsearch/snapshots/SnapshotsService.java

 import com.carrotsearch.hppc.cursors.ObjectCursor;
 import com.carrotsearch.hppc.cursors.ObjectObjectCursor;
+
 import org.apache.logging.log4j.message.ParameterizedMessage;


That would be nice to revert this change (it "pollutes" a bit the Git history)

makes sense, thanks for catching, didn't see that

tlrx · 2017-01-12T13:35:19Z

core/src/test/java/org/elasticsearch/action/search/SearchResponseTests.java

+                    + "\"max_score\":1.5,"
+                    + "\"hits\":[{\"_type\":\"type\",\"_id\":\"id1\",\"_score\":2.0}]"
+                    + "}"
+                + "}", xContent.utf8ToString());


Thanks for the indentation ;)

tlrx · 2017-01-12T13:36:24Z

test/framework/src/main/java/org/elasticsearch/test/hamcrest/ElasticsearchAssertions.java

            try (XContentParser expectedParser = xContentType.xContent().createParser(NamedXContentRegistry.EMPTY, expected)) {
                Map<String, Object> expectedMap = expectedParser.map();
-                assertMapEquals(expectedMap, actualMap);
+                assertMapEquals(expectedMap, actualMap, "");


Do we need this empty path?

As mentioned in another comment, I added these path arguments to be able to better debug errors in map-comparisons when we compare xContent. This empty path is used as initial value, assertMapEquals() will be called recursively later.

javanna

left a couple of comments, LGTM otherwise, no need for another review round on my end

javanna · 2017-01-12T15:28:00Z

core/src/main/java/org/elasticsearch/action/search/SearchResponse.java

+        Boolean terminatedEarly = null;
+        long tookInMillis = 0;
+        int successfulShards = 0;
+        int totalShards = 0;


shall we use -1 as default values, just to make sure not to confuse them with actual values that get returned?

javanna · 2017-01-12T15:29:15Z

core/src/main/java/org/elasticsearch/action/search/SearchResponse.java

+        String currentFieldName = null;
+        InternalSearchHits hits = null;
+        boolean timedOut = false;
+        Boolean terminatedEarly = null;


shall timedOut also be a Boolean? maybe not because it's always returned while terminatedEarly is not? trying to understand the difference between the two

yes, thats it. "timedOut" is always set, always rendered out, so it should always be parsed. "terminatedEarly" can also be "null" in the original object, it only rendered when its present, so its okay if we don't parse it.

javanna · 2017-01-12T15:32:33Z

core/src/test/java/org/elasticsearch/action/search/ShardSearchFailureTests.java

+        // we cannot compare the cause, because it will be wrapped in an outer ElasticSearchException
+        // best effort: try to check that the original message appears somewhere in the rendered xContent
+        String originalMsg = response.getCause().getMessage();
+        assertTrue(parsed.getCause().getMessage().contains(originalMsg));


Instead, can we build what we expect given the exception? Isn't it just wrapping what we have into an ElasticsearchException with the same message? Or does it get too complicated? I think Tanguy did something similar in some other test.

I will look into it but won't spend too much time on it at this point.

Building the "expected" value (either the object or its xContent rendering) is really difficult and will break easiliy without using ElasticsearchException#fromXContent(), which unfortunately is what we want to test here. The way this wraps the original exception and rewrites the message in it is hard to mock without actually repeating it, which defies the usefulness of a test IMHO. Still looking but I'm a bit clueless at this point.

It can be complicated to rebuild the object, how about doing something like:

assertEquals(parsed.getCause().getMessage(), "Elasticsearch exception [type=parse_exception, reason=" + originalMsg +"]");

Yes, thats another slightly different option, that works when we don't test for nested causes which I think we don't have to here. Will change this, although it doesn't add much to the current version.

cbuescher · 2017-01-19T18:55:40Z

@javanna @tlrx since we agreed to put this on a a separate branch I opened #22699 than can be merged to master already. I will rebase this PR as well for now until we open the feature branch

cbuescher · 2017-04-26T14:11:53Z

Closing this since it is quiet stale now.

cbuescher added :Java High Level REST Client >enhancement v6.0.0-alpha1 labels Jan 10, 2017

cbuescher requested a review from javanna January 10, 2017 16:22

javanna requested changes Jan 12, 2017

View reviewed changes

tlrx requested changes Jan 12, 2017

View reviewed changes

Addressing review comments

6bbbd2e

cbuescher force-pushed the addParsing-searchResponse branch from 893ed52 to 6bbbd2e Compare January 12, 2017 15:16

javanna approved these changes Jan 12, 2017

View reviewed changes

Addressing some more review comments

f62fa7e

cbuescher closed this Apr 26, 2017

javanna mentioned this pull request May 2, 2017

Introduce SearchResponseSections base class #24442

Merged

clintongormley removed the v6.0.0-alpha1 label May 8, 2017

javanna mentioned this pull request May 16, 2017

Add fromXContent method to SearchResponse #24720

Merged

Add parsing from xContent to SearchResponse #22533

Add parsing from xContent to SearchResponse #22533

Uh oh!

Conversation

cbuescher commented Jan 10, 2017

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!

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!

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

cbuescher Jan 12, 2017 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cbuescher Jan 12, 2017 •

edited

Loading