Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add fromxcontent methods to delete response #22680

Merged
merged 3 commits into from Jan 20, 2017

Conversation

Projects
None yet
3 participants
@dadoonet
Copy link
Member

commented Jan 18, 2017

This commit adds the parsing fromXContent() methods to the IndexResponse class.

It's a pale copy of what has been done in #22229.

There are a lot of code in common between IndexResponse and DeleteResponse including in tests.
Do we want to keep it as it is or avoid code duplication?

@tlrx
Copy link
Member

left a comment

This looks good, it's indeed very similar to IndexResponse/IndexResponseTests.

I'd like to merge #22586 before this go in, so that we can mutualize some code in the tests. There will be a light modification in the ConstructingObjectParser as well but nothing big.

core/src/main/java/org/elasticsearch/action/delete/DeleteResponse.java Outdated
* 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.
*/

This comment has been minimized.

Copy link
@tlrx

tlrx Jan 19, 2017

Member

You can remove this comment

core/src/test/java/org/elasticsearch/action/delete/DeleteResponseTests.java Outdated
}

private static void assertDeleteResponse(DeleteResponse expected, Map<String, Object> actual) {
assertEquals(expected.getIndex(), actual.get("_index"));

This comment has been minimized.

Copy link
@tlrx

tlrx Jan 19, 2017

Member

I'm about to change this in #22586 so that DeleteResponse/IndexResponse/UpdateResponse don't have to repeat all these checks. There will be a assertDocWriteResponse() method, and here we only have to check for UpdateResponse specific fields.

core/src/test/java/org/elasticsearch/action/delete/DeleteResponseTests.java Outdated
String id = randomAsciiOfLength(5);
long seqNo = randomIntBetween(-2, 5);
long version = (long) randomIntBetween(0, 5);
boolean created = randomBoolean();

This comment has been minimized.

Copy link
@tlrx

tlrx Jan 19, 2017

Member

found?

Add fromxcontent methods to delete response
This commit adds the parsing fromXContent() methods to the IndexResponse class.

It's a pale copy of what has been done in #22229.

@dadoonet dadoonet force-pushed the dadoonet:pr/delete-from-xcontent branch to 0315dcc Jan 19, 2017

@dadoonet

This comment has been minimized.

Copy link
Member Author

commented Jan 19, 2017

@tlrx updated based on new changes and your comments

@tlrx
Copy link
Member

left a comment

Thanks @dadoonet! This is getting very close, I just left a last comment.

// Print the parsed object out and test that the output is the same as the original output
BytesReference parsedDeleteResponseBytes = toXContent(parsedDeleteResponse, xContentType);
try (XContentParser parser = createParser(xContentType.xContent(), parsedDeleteResponseBytes)) {
assertDocWriteResponse(deleteResponse, parser.map());

This comment has been minimized.

Copy link
@tlrx

tlrx Jan 19, 2017

Member

You still need to check the parsed value of the found field, right?

This comment has been minimized.

Copy link
@dadoonet

dadoonet Jan 19, 2017

Author Member

ha right!

Also test found field
And optimize imports
@dadoonet

This comment has been minimized.

Copy link
Member Author

commented Jan 19, 2017

@tlrx updated with your comments.

@tlrx

tlrx approved these changes Jan 19, 2017

@dadoonet dadoonet merged commit 5be8bd7 into elastic:master Jan 20, 2017

2 checks passed

CLA Commit author is a member of Elasticsearch
Details
elasticsearch-ci Build finished.
Details

@dadoonet dadoonet deleted the dadoonet:pr/delete-from-xcontent branch Jan 20, 2017

@dadoonet

This comment has been minimized.

Copy link
Member Author

commented Jan 20, 2017

Thank you Tanguy!

dadoonet added a commit to dadoonet/elasticsearch that referenced this pull request Jan 20, 2017

Add fromxcontent methods to delete response
This commit adds the parsing fromXContent() methods to the DeleteResponse class. The method is based on a ObjectParser because it is easier to use when parsing parent abstract classes like DocWriteResponse.

It also changes the ReplicationResponse.ShardInfo so that it now implements ToXContentObject. This way, the ShardInfo.fromXContent() method can be used by the DeleteResponse's ObjectParser.

Backport of elastic#22680 in 5.x branch

@dadoonet dadoonet removed the v5.3.0 label Jan 20, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.