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

Use optype CREATE for single auto-id index requests #47353

Merged

Conversation

ywelsch
Copy link
Contributor

@ywelsch ywelsch commented Oct 1, 2019

Changes auto-id index requests to use optype CREATE. According to our docs (https://www.elastic.co/guide/en/elasticsearch/reference/current/docs-index_.html#_create_document_ids_automatically):

If you don’t specify a document ID when using POST, the op_type is automatically set to create and the index operation generates a unique ID for the document.

This is unfortunately not true, as we are currently using index as default optype. To make these auto-id index requests compatible with the new "create-doc" index privilege (which is based on the optype), the default optype is changed to create, just as it is already documented.

@ywelsch ywelsch added >bug :Distributed Indexing/CRUD A catch all label for issues around indexing, updating and getting a doc by id. Not search. v8.0.0 v7.5.0 labels Oct 1, 2019
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-distributed

@ywelsch
Copy link
Contributor Author

ywelsch commented Oct 1, 2019

@elasticmachine run elasticsearch-ci/1
run elasticsearch-ci/2
both unrelated failures

Copy link
Contributor

@henningandersen henningandersen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@Override
public RestChannelConsumer prepareRequest(RestRequest request, final NodeClient client) throws IOException {
assert request.params().get("id") == null : "non-null id: " + request.params().get("id");
if (request.params().get("op_type") == null && clusterService.state().nodes().getMinNodeVersion().onOrAfter(Version.CURRENT)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rest-api-spec (index.json) says default for op_type is index, I think we should change that to be create.

I wonder if we should have an end-goal of removing op_type support here as a breaking change in 8.0?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is that this API spec mixes auto-id which non-auto-id. In one case, the default op_type is create, in the other case it is index. I have moved the explanation of the default into the description (as we have for many other parameters).

@ywelsch ywelsch merged commit e1012ef into elastic:master Oct 2, 2019
ywelsch added a commit to ywelsch/elasticsearch that referenced this pull request Oct 2, 2019
Changes auto-id index requests to use optype CREATE, making it compliant with our docs.
This will also make these auto-id index requests compatible with the new "create-doc" index
privilege (which is based on the optype), the default optype is changed to create, just as it is
already documented.
ywelsch added a commit that referenced this pull request Oct 2, 2019
Changes auto-id index requests to use optype CREATE, making it compliant with our docs.
This will also make these auto-id index requests compatible with the new "create-doc" index
privilege (which is based on the optype), the default optype is changed to create, just as it is
already documented.
ywelsch added a commit that referenced this pull request Oct 2, 2019
ywelsch added a commit that referenced this pull request Oct 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>bug :Distributed Indexing/CRUD A catch all label for issues around indexing, updating and getting a doc by id. Not search. v7.5.0 v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants