Add parsing methods for UpdateResponse #22586

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

This commit adds the fromXContent() method to the UpdateResponse class, so that it can be used with the high level rest client.

This commit adds the fromXContent() method to the UpdateResponse class, so that it can be used with the high level rest client.

javanna

left a few small comments, LGTM otherwise

javanna · 2017-01-12T13:21:17Z

core/src/main/java/org/elasticsearch/action/DocWriteResponse.java

@@ -292,8 +292,8 @@ protected static void declareParserFields(ConstructingObjectParser<? extends Doc
        objParser.declareString(constructorArg(), new ParseField(_ID));
        objParser.declareLong(constructorArg(), new ParseField(_VERSION));
        objParser.declareString(constructorArg(), new ParseField(RESULT));
+        objParser.declareObject(constructorArg(), (p, c) -> ShardInfo.fromXContent(p), new ParseField(_SHARDS));


did ShardInfo become a constructor argument?

No - I changed it to optionalConstructorArg() since it can be an argument of the UpdateResponse.

javanna · 2017-01-12T13:22:27Z

core/src/main/java/org/elasticsearch/action/update/UpdateResponse.java

+     * because most fields are parsed by the parent abstract class {@link DocWriteResponse} and it's
+     * not easy to parse part of the fields in the parent class and other fields in the children class
+     * using the usual streamed parsing method.
+     */


I think you can remove this comment ;) no need to explain why you are using ObjectParser I think :)

javanna · 2017-01-12T13:32:26Z

core/src/main/java/org/elasticsearch/action/update/UpdateResponse.java

+            try (XContentBuilder builder = XContentBuilder.builder(xContent)) {
+                // "get" field contains an embedded version of {@link GetResult} and requires
+                // to be parsed as if it was a full XContent object.
+                XContentHelper.copyCurrentStructure(builder.generator(), p);


why are we copying this? because the xContentType can differ between the response and the get section?

same question

because the xContentType can differ between the response and the get section?

No, this has been a crappy hack around the fact that GetResult can be rendered as a ToXContentObject using toXContent() or as a fragment using toXContentEmbedded().

I changed that so that GetResult now has a fromXContentEmbedded() parsing method that can be used in UpdateResponse's object parser.

Note that GetResult.toXContent() delegates all the work to fromXContentEmbedded(). Please let me know what you think about this.

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

core/src/test/java/org/elasticsearch/action/update/UpdateResponseTests.java

+
+        // We can't use equals() to compare the original and the parsed update response
+        // because the random update response can contain shard failures with exceptions,
+        // and those exceptions are not parsed back with the same types.


can we rather generate responses without exceptions and test them like we usually do, and test exceptions separately?

I changed that part to do something slightly different. We can't compare UpdateResponse using equals() because it can have shard failures AND the GetResult object embedded in the update response is a bit different after parsing (it does not have index/type/id/version).

For the shard failures, we already have IndexResponseTests.assertDocWriteResponse() which takes care of checking shard failures, so I reused it.

For the GetResult, I used something similar to the GetResultTests where it generates and returns a Tuple of actual and expected GetResult, and I set to null the index/type/etc values expected for the parsed GetResult.

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

core/src/test/java/org/elasticsearch/action/update/UpdateResponseTests.java

+    }
+
+    private static void assertUpdateResponse(UpdateResponse expected, Map<String, Object> actual) {
+        IndexResponseTests.assertDocWriteResponse(expected, actual);


don't we have to check also the get result part?

Yes, of course, how can I forgot that? It's like running a 100 meters and forget to pass the finish line...

cbuescher

I also took a look but other than the question @javanna already asked I have nothing to add at the moment. Will take another final look if necessary.

tlrx · 2017-01-17T08:08:45Z

@javanna @cbuescher I updated the pull request with your comments. I think I addressed all of them. It would be great if you can have another look.

javanna · 2017-01-17T12:09:33Z

core/src/test/java/org/elasticsearch/action/update/UpdateResponseTests.java

+            assertNull(parser.nextToken());
+        }
+
+        final UpdateResponse updateResponse = tuple.v2();


can you call the variable expected or something along those lines?

Renamed to expectedUpdateResponse

javanna · 2017-01-17T12:18:54Z

core/src/test/java/org/elasticsearch/index/get/GetResultTests.java

@@ -160,4 +213,13 @@ public static GetResult mutateGetResult(GetResult getResult) {
        }
        return Tuple.tuple(fields, expectedFields);
    }
+
+    private static BytesReference toXContentEmbedded(GetResult getResult, XContentType xContentType) throws IOException {


should you rather have an anonymous ToXContent that delegates GetResult#toXContentEmbedded and call XContentHelper#toXContent instead?

javanna

LGTM

cbuescher · 2017-01-17T14:09:25Z

core/src/test/java/org/elasticsearch/action/update/UpdateResponseTests.java

+        long version = actualGetResult.getVersion();
+        DocWriteResponse.Result result = actualGetResult.isExists() ? DocWriteResponse.Result.UPDATED : DocWriteResponse.Result.NOT_FOUND;
+
+        Long seqNo = randomBoolean() ? null : randomNonNegativeLong();


Generating random longs like this tends to generate really large numbers. I added another 50/50 condition in some of my randomized tests that do e.g. a %10000 to get smaller numbers here more often, this uncovered some type conversion issues (integer/long) in the parser. Maybe you could add something like that here too, since smaller numbers are more likely to appear in real responses.

++ let's add it as a common method somewhere so we remember to do it in other places too?

While I think a common methods would be great, I'm not sure this distinction (small vs. large longs) is needed very often. In the parsing tests its good to have some more "smaller" values to catch cases where they might be parsed back as int. I wonder how a common method would be called (randomNonNegativeLongButOftenSmall?), which range to sample the small values from (that might vary between tests) and where to put it. But maybe @tlrx has some suggestion.

Here I changed to Long seqNo = randomFrom(randomNonNegativeLong(), (long) randomIntBetween(0, 10_000), null); and I also added a comment.

I'm for having less trivial random methods, I really prefer to repeat stuff and adds comments where it makes sense.

++ I like that solution here

cbuescher · 2017-01-17T14:22:51Z

core/src/main/java/org/elasticsearch/action/update/UpdateResponse.java

+                });
+
+        DocWriteResponse.declareParserFields(PARSER);
+        PARSER.declareObject(UpdateResponse::setGetResult, (p, c) -> GetResult.fromXContentEmbedded(p), new ParseField(GET));


I think it might be possible to use the index/type/id information from the already constructed UpdateResponse here to set those missing fields for the GetResult. It looks a bit odd but I think it might work, will need to check that in test though. Or can the index/type/id in the get result be different from the values in the overall response?

You can do sth. like this I think instead of directly setting the parsed (incomplete GetResult):

(response, getResult) -> response.setGetResult(new GetResult(response.getIndex(), response.getType(), response.getId(), getResult.getVersion(), getResult.isExists(), getResult.internalSourceRef(), getResult.getFields()))

This way you can use the index/type/id from the parent UpdateResponse together with the rest of the parsed GetResult information. I haven't tested this very thoroughly though because didn't completely understand the test setup yet.

I had the same idea but at that time all the object was parsed in the ConstructingObjectParser. Then I reverted this part and added a comment in the randomUpdateResponse() test method, but now I'm thinking of it again it does not cost a lot and it makes sense. Thanks for raising this point again.

cbuescher · 2017-01-17T14:40:09Z

core/src/main/java/org/elasticsearch/action/update/UpdateResponse.java

+        PARSER.declareObject(UpdateResponse::setGetResult, (p, c) -> GetResult.fromXContentEmbedded(p), new ParseField(GET));
+    }
+
+    public static UpdateResponse fromXContent(XContentParser parser) throws IOException {


nit: IOException is not thrown

cbuescher · 2017-01-17T15:29:57Z

core/src/test/java/org/elasticsearch/action/update/UpdateResponseTests.java

+            // by the toXContentEmbedded method.
+            GetResult expectedGetResult = new GetResult(null, null, null, -1,
+                    getResults.v2().isExists(), getResults.v2().sourceRef(), getResults.v2().getFields());
+            expected.setGetResult(expectedGetResult);


Why is the expected get result only set to the expected UpdateResponse when seqNo is not null? Should this be behind the if block instead?

Good catch, it should be aligned with actualGetResult.isExists() / expectedGetResult.isExists(). I'll change that.

cbuescher · 2017-01-17T16:30:03Z

core/src/test/java/org/elasticsearch/action/update/UpdateResponseTests.java

+
+    public void testToAndFromXContent() throws IOException {
+        final XContentType xContentType = randomFrom(XContentType.values());
+        final Tuple<UpdateResponse, UpdateResponse> tuple = randomUpdateResponse();


There is a problem with this test setup that I just found: the xContentType that is used for parsing here is not necessarily the same as the one that is used int randomUpdateResponse(). So the expected values might be off, e.g. if in randomUpdateResponse() SMILE is used and here xContentType is Yaml.

you are right thanks a lot for catching this

Good catch @cbuescher ! Vielen dank!

cbuescher · 2017-01-17T17:05:34Z

core/src/test/java/org/elasticsearch/action/update/UpdateResponseTests.java

+            ReplicationResponse.ShardInfo shardInfo = RandomObjects.randomShardInfo(random(), true);
+
+            actual = new UpdateResponse(shardInfo, shardId, type, id, seqNo, version, result);
+            actual.setGetResult(actualGetResult);


same here, seems like the getResult should be set regardless of seqNo being present or not, or do I miss something?

You're right, thanks

cbuescher · 2017-01-17T17:29:05Z

@tlrx I left some more comments concerning parsing the GetResult. I think this would simplify the whole test a little bit.

tlrx · 2017-01-19T08:34:16Z

@cbuescher @javanna Thanks! I think I addressed all your comments in the last two commits. Are we good to go?

javanna · 2017-01-19T09:25:10Z

Are we good to go?

++ on my end

cbuescher · 2017-01-19T10:33:13Z

for me also, LGTM

tlrx · 2017-01-19T11:50:23Z

Thanks @cbuescher @javanna !

This commit adds the fromXContent() method to the UpdateResponse class, so that it can be used with the high level rest client.

@Header

* master: (117 commits) Add missing serialization BWC for disk usage estimates Expose disk usage estimates in nodes stats S3 repository: Deprecate specifying credentials through env vars, sys props, and remove profile files (elastic#22567) Fix Eclipse project generation Fix deprecation logging for lenient booleans Remove @Header we no longer need Make lexer abstract [Docs] Remove outdated info about enabling/disabling doc_values (elastic#22694) Move lexer hacks to EnhancedPainlessLexer Fix incorrect args order passed to createAggregator Improve painless's javadocs Add TestWithDependenciesPlugin to build (elastic#22646) Add parsing from xContent to SearchProfileShardResults and nested classes (elastic#22649) Add unit tests for FiltersAggregator (elastic#22678) Don't register search response listener in transport clients unmute FieldStatsIntegrationIT.testGeoPointNotIndexed, fix already pushed Mute FieldStatsIntegrationIT.testGeoPointNotIndexed, for now Painless: Add augmentation to string for base 64 (elastic#22665) Fix NPE on FieldStats with mixed cluster on version pre/post 5.2 (elastic#22688) Add parsing methods for UpdateResponse (elastic#22586) ...

Add parsing methods for UpdateResponse

31960a2

This commit adds the fromXContent() method to the UpdateResponse class, so that it can be used with the high level rest client.

tlrx added :Java High Level REST Client v5.3.0 v6.0.0-alpha1 labels Jan 12, 2017

tlrx requested review from javanna and cbuescher January 12, 2017 13:07

javanna requested changes Jan 12, 2017

View reviewed changes

clintongormley added the >enhancement label Jan 13, 2017

cbuescher reviewed Jan 16, 2017

View reviewed changes

tlrx added 4 commits January 16, 2017 20:35

Remove comment & make shardInfo as optional ctor

ebaf3bf

Update tests

a9d0535

Add from/toXContentEmbedded tests

ed8bb8f

Fix IndexResponseTests

4078042

javanna reviewed Jan 17, 2017

View reviewed changes

javanna approved these changes Jan 17, 2017

View reviewed changes

cbuescher reviewed Jan 17, 2017

View reviewed changes

tlrx added 2 commits January 18, 2017 20:47

Update after Luca & Christoph reviews

97bf4b4

Also generate small numbers for seqNo

8ecee09

tlrx mentioned this pull request Jan 19, 2017

Add fromxcontent methods to delete response #22680

Merged

cbuescher approved these changes Jan 19, 2017

View reviewed changes

tlrx merged commit 833284c into elastic:master Jan 19, 2017

tlrx removed the review label Jan 19, 2017

tlrx added a commit that referenced this pull request Jan 19, 2017

Add parsing methods for UpdateResponse (#22586)

6093c51

This commit adds the fromXContent() method to the UpdateResponse class, so that it can be used with the high level rest client.

tlrx deleted the add-parsing-methods-to-update-response branch January 19, 2017 12:08

DaveCTurner mentioned this pull request Jan 5, 2021

GetResultsTests#testToAndFromXContentEmbedded fails reproducibly #67009

Closed

Add parsing methods for UpdateResponse #22586

Add parsing methods for UpdateResponse #22586

Conversation

tlrx commented Jan 12, 2017

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cbuescher left a comment

Choose a reason for hiding this comment

tlrx commented Jan 17, 2017

javanna Jan 17, 2017 • edited Loading

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

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

cbuescher Jan 17, 2017 • edited Loading

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

cbuescher Jan 17, 2017 • edited Loading

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

cbuescher commented Jan 17, 2017

tlrx commented Jan 19, 2017

javanna commented Jan 19, 2017

cbuescher commented Jan 19, 2017

tlrx commented Jan 19, 2017

javanna Jan 17, 2017 •

edited

Loading

cbuescher Jan 17, 2017 •

edited

Loading

cbuescher Jan 17, 2017 •

edited

Loading