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

Added validation for upsert request #24282

Merged
merged 4 commits into from Apr 28, 2017

Conversation

Projects
None yet
4 participants
@kunal642
Copy link
Contributor

commented Apr 24, 2017

related to #16671

@elasticmachine

This comment has been minimized.

Copy link

commented Apr 24, 2017

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@kunal642 kunal642 changed the title added validation for upserd request Added validation for upserd request Apr 24, 2017

@kunal642 kunal642 force-pushed the kunal642:issue#16671 branch to 8d7076f Apr 24, 2017

@kunal642

This comment has been minimized.

Copy link
Contributor Author

commented Apr 25, 2017

Can someone please review this

updateRequest.version(1L);
updateRequest.upsert(new IndexRequest("index","type", "1"));
assertThat(updateRequest.validate().validationErrors(), contains("index is missing",
"can't provide both upsert request and a version","type is missing","id is missing","script or doc is missing"));

This comment has been minimized.

Copy link
@nik9000

nik9000 Apr 25, 2017

Contributor

Can you set the missing things so they don't throw as well?

@nik9000

This comment has been minimized.

Copy link
Contributor

commented Apr 25, 2017

I think it is worth also writing a test for bulk requests. They can attempt to set the version on an upsert request.

@kunal642

This comment has been minimized.

Copy link
Contributor Author

commented Apr 25, 2017

@nik9000 the requested changes have been done

updateRequest.version(1L);
updateRequest.doc("{}", XContentType.JSON);
updateRequest.upsert(new IndexRequest("index","type", "id").source("{}", XContentType.JSON));
BulkRequest bulkRequest = new BulkRequest();

This comment has been minimized.

Copy link
@nik9000

nik9000 Apr 25, 2017

Contributor

I meant a test that uses the bulk request's overrides in BulkRequest#add. That way we are testing a way of "sneaking" the version into the request that is unique to bulk requests.

@kunal642

This comment has been minimized.

Copy link
Contributor Author

commented Apr 26, 2017

@nik9000 changed the test case

@nik9000

This comment has been minimized.

Copy link
Contributor

commented Apr 26, 2017

@nik9000 changed the test case

Thanks! I've spent all day merging things and haven't gotten around to looking at this yet. I promise I'll get to it soon!

@nik9000
Copy link
Contributor

left a comment

Actually, I did get a chance to look at it. That is perfect. I'll have a look at merging tomorrow.

@nik9000
Copy link
Contributor

left a comment

I hate to do this after approving the change but running gradle check caught this:
https://github.com/elastic/elasticsearch/blob/master/core/src/test/java/org/elasticsearch/update/UpdateIT.java#L535-L543

That test is now out of date. Can you remove it entirely with your PR?

You might have to merge master into your PR if you ran tests locally and didn't catch it. It is what I did before running the tests.

@kunal642

This comment has been minimized.

Copy link
Contributor Author

commented Apr 27, 2017

@nik9000 i dont know how i missed it
Ill remove the marked test case

@kunal642

This comment has been minimized.

Copy link
Contributor Author

commented Apr 27, 2017

@nik9000 i tried running gradle test and all the test suites related to plugins are failing.

@nik9000

This comment has been minimized.

Copy link
Contributor

commented Apr 27, 2017

@kunal642 to be honest I never got that far. gradle check stopped me in core before I got to plugins.

@kunal642

This comment has been minimized.

Copy link
Contributor Author

commented Apr 27, 2017

@nik9000 should i push??

@nik9000

This comment has been minimized.

Copy link
Contributor

commented Apr 27, 2017

@nik9000 should i push??

Sure! If you push what you have I can have a look at the failure and advise from there.

@kunal642

This comment has been minimized.

Copy link
Contributor Author

commented Apr 27, 2017

@nik9000 please have a look

@nik9000

This comment has been minimized.

Copy link
Contributor

commented Apr 27, 2017

So far all the tests seem fine to me....

@kunal642

This comment has been minimized.

Copy link
Contributor Author

commented Apr 27, 2017

Thats great
I guess there is some problem in my repo

When i normally run the test cases they give me jar hell exception
In debug mode they run fine

Gradle check stalls the test cases for seconds and gives timeout exception after a while

@nik9000 nik9000 merged commit a5bd201 into elastic:master Apr 28, 2017

1 check passed

CLA Commit author has signed the CLA
Details

@clintongormley clintongormley changed the title Added validation for upserd request Added validation for upsert request May 2, 2017

@elasticmachine

This comment has been minimized.

Copy link

commented May 2, 2017

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@nik9000

This comment has been minimized.

Copy link
Contributor

commented May 2, 2017

@elasticmachine, this PR is merged.

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.