[Test] Extend parsing checks for DocWriteResponses #25257

tlrx · 2017-06-15T15:26:01Z

This commit changes the parsing logic of DocWriteResponse, ReplicationResponse and GetResult so that it skips any unknown additional fields (for forward compatibility reasons). This affects the IndexResponse, UpdateResponse, DeleteResponse and GetResponse objects.

javanna

LGTM besides the suggestions I left

javanna · 2017-06-15T15:31:53Z

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

nit: shall we be more accurate in verifying that the current token is a start_array? anyways skipChildren won't have any effect otherwise.

Sure, it will avoid to execute a noop since the verification will be done anyway by Jackson

javanna · 2017-06-15T15:32:18Z

core/src/main/java/org/elasticsearch/action/support/replication/ReplicationResponse.java

same as above: shall we be more accurate in checking what the current token is.

javanna · 2017-06-15T15:32:42Z

core/src/main/java/org/elasticsearch/action/support/replication/ReplicationResponse.java

and here too, same as above.

javanna · 2017-06-15T15:35:16Z

core/src/main/java/org/elasticsearch/index/get/GetField.java

I wonder if we are making this parser too lenient with this change. it's ok to skip what we don't know about, but maybe we can avoid returning null as the array with at least a value is required?

I agree, this is too lenient and also breaks the current GET behavior: the high level client ignores the 404 response code and tries to parse the response body again. If the parsing is too lenient then a potential "error" is just skipped and that's wrong. I'm looking at this.

I updated the code to make it throw a ParsingException when the found field has not been parsed.

javanna · 2017-06-15T15:35:53Z

core/src/main/java/org/elasticsearch/index/get/GetResult.java

I wish we can avoid having this null check, related to comment above

cbuescher

LGTM, I left two minor suggestions about comments in the tests

cbuescher · 2017-06-16T08:55:14Z

core/src/test/java/org/elasticsearch/action/get/GetResponseTests.java

+1 to the great comment. I still had to look up why we cannot insert into the root GetResponse and realized its the metadata which can be any values. Can you add this little piece of info to the comment?

Sure, I rephrased that

cbuescher · 2017-06-16T09:06:24Z

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

Nit: maybe also explain the last exception rult, why inserting into the nested GetResult doesn't work here.

tlrx · 2017-06-16T09:59:54Z

Thanks @cbuescher @javanna for your comments. I updated the code.

javanna

left a few more comments, nothing major though

javanna · 2017-06-16T10:08:02Z

client/rest-high-level/src/main/java/org/elasticsearch/client/RestHighLevelClient.java

thanks for adding this comment

javanna · 2017-06-16T10:12:50Z

core/src/main/java/org/elasticsearch/index/get/GetField.java

maybe this is too much? we do expect each field under fields to contain an array of values I'd say. without this change parseStoredFieldsValue would throw exception if it finds something that it doesn't support, which is probably ok?

I also thought looking below that we are not testing this at the moment, maybe I am wrong.

Ok, I'm reverting this change. The parseStoredFieldsValue() is strict but not tested and it looks like it does not handle correctly the null value that can be printed out by MappedTypeField sub classes. I'm not sure it's a real case be we should test that too. I'll add a test for parseStoredFieldsValue() as a follow up.

sounds good

javanna · 2017-06-16T10:14:11Z

core/src/main/java/org/elasticsearch/index/get/GetResult.java

_index, _type and _id are required too right? Shall we check them too?

also add a comment on why we need to throw this exception?

Yes we can add them all. I added a comment.

I'm wrong, at this stage and when parsing an UpdateResponse the index/type/id/version fields can be missing because they are printed out by the UpdateResponse itself...

I removed the checks from this method and add them in the GetResponse.fromXContent() where I think they should be located. This way we keep the same behavior (ie throwing an exception when fields are missing) but that would impact only users of the GetResponse.fromXContent() method.

which makes me wonder...shouldn't we do the same in the update api with 404? I'm afraid so... cause if the doc is missing we do return 404?

For the Update API, a 404 is returned with an exception in the response body. The high level rest client doesn't ignore 404s but it's okay because the response body will always be an exception.

The Get API is a bit different, when the document doesn't exists it returns a 404 but with a response body that is not always an exception. So the client has to try to parse that doc, fails, and then parses it as an exception and got the right result.

ok phew sounds good :)

javanna · 2017-06-16T10:25:21Z

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

the path.endsWith("get") could be the path.endsWith(".get") ?

I changed this to path.contains("get") which is equivalent to
path.contains("get.fields") || path.contains("get._source") || path.endsWith("get")

tlrx · 2017-06-16T14:21:40Z

@javanna I updated again the code, moving few things around. Hopefully we're good now.

javanna · 2017-06-16T15:02:45Z

core/src/main/java/org/elasticsearch/action/get/GetResponse.java

this is the case when we get a 404 back, which can be parsed as a normal get response with found set to false, or as an elasticsearch exception. The caller of this method needs a way to figure out whether we got back a valid get response, which can be done by catching ParsingException.

Maybe some of this should be in the javadocs of this method.

javanna · 2017-06-16T15:04:03Z

core/src/main/java/org/elasticsearch/action/get/GetResponse.java

funny that I would have instead done

if (getResult.getIndex() == null && getResult.getType() == null && getResult.getId() == null)

as this seems to be a stronger condition :) what do you think?

javanna · 2017-06-16T15:05:37Z

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

is there a reason why we don't shuffle stuff here?

We don't want to shuffle here (though we could) because we just want to check that the parsed update response, when printed out as xcontent and parsed back as a map, has the exact same output as what we expect. I hoped the comment was clear enough...

javanna

left a few very minors, LGTM though

This commit changes the parsing logic of DocWriteResponse, ReplicationResponse and GetResult so that it skips any unknown additional fields (for forward compatibility reasons). This affects the IndexResponse, UpdateResponse, DeleteResponse and GetResponse objects.

tlrx · 2017-06-19T09:02:37Z

@elasticmachine retest this please

This commit changes the parsing logic of DocWriteResponse, ReplicationResponse and GetResult so that it skips any unknown additional fields (for forward compatibility reasons). This affects the IndexResponse, UpdateResponse,DeleteResponse and GetResponse objects.

tlrx added :Java High Level REST Client >test Issues or PRs that are addressing/adding tests v5.6.0 v6.0.0 labels Jun 15, 2017

tlrx requested review from cbuescher and javanna June 15, 2017 15:26

javanna approved these changes Jun 15, 2017

View reviewed changes

cbuescher approved these changes Jun 16, 2017

View reviewed changes

javanna reviewed Jun 16, 2017

View reviewed changes

javanna approved these changes Jun 16, 2017

View reviewed changes

tlrx added 5 commits June 19, 2017 10:14

Apply feedback

7580af5

boo

7162844

Move checks in GetResponse

6c8a786

Last feedback

1aec207

tlrx force-pushed the relax-parsing-logic-docwriteresponses branch from 15fd2c7 to 1aec207 Compare June 19, 2017 08:14

tlrx merged commit e4f4886 into elastic:master Jun 19, 2017

tlrx mentioned this pull request Jun 19, 2017

[Test] Add unit test for XContentParserUtilsTests.parseStoredFieldsValue #25288

Merged

tlrx deleted the relax-parsing-logic-docwriteresponses branch June 19, 2017 12:16

clintongormley added v6.0.0-beta1 and removed v6.0.0 labels Jul 25, 2017

[Test] Extend parsing checks for DocWriteResponses #25257

[Test] Extend parsing checks for DocWriteResponses #25257

Uh oh!

Conversation

tlrx commented Jun 15, 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!

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

tlrx commented Jun 16, 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!

tlrx Jun 16, 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!

tlrx commented Jun 16, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

javanna Jun 16, 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

tlrx Jun 16, 2017 •

edited

Loading

javanna Jun 16, 2017 •

edited

Loading