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

[elasticsearch-api] Mismatch between documentation and behaviour in Elasticsearch::API::Actions#create #491

Closed
janko opened this issue Jan 27, 2018 · 3 comments

Comments

@janko
Copy link

janko commented Jan 27, 2018

The documentation for Elasticsearch::API::Actions#create says for the :id parameter:

Document ID (optional, will be auto-generated if missing)

However, the :id parameter is actually required:

def create(arguments={})
raise ArgumentError, "Required argument 'id' missing" unless arguments[:id]
index arguments.update :op_type => 'create'
end

So either the documentation needs to be corrected or the :id shouldn't be required. I'm hoping for the latter, as id shouldn't be required by Elasticsearch when creating documents directly.

@karmi
Copy link
Contributor

karmi commented Jan 27, 2018

Right, there's some kind of mismatch. I can see that I've added that specific verification in the last commit, and the Elasticsearch's REST API specification lists the parameter as required:true... I'll check what's up with the requirement.

@janko janko changed the title [elasticsearch-api] Inconsistency between documentation and behaviour in Elasticsearch::API::Actions#create [elasticsearch-api] Mismatch between documentation and behaviour in Elasticsearch::API::Actions#create Jan 27, 2018
@karmi karmi closed this as completed in b1a8482 Jan 28, 2018
@karmi
Copy link
Contributor

karmi commented Jan 28, 2018

I've fixed the method in the attached commit and verified it manually against ES 6.x and master. Many thanks for bringing it to my attention, @janko-m!


Verification curl localhost:9250 | jq '.version.number'

"7.0.0-alpha1"

curl -H 'Content-Type: application/json' -X POST localhost:9250/test/doc?op_type=create -d '{"title":"test"}'

{"error":# {"root_cause":[# {"type":"action_request_validation_exception","reason":"Validation Failed: 1: an id must be provided if version type or value are set;"}],"type":"action_request_validation_exception","reason":"Validation Failed: 1: an id must be provided if version type or value are set;"},"status":400}

curl -H 'Content-Type: application/json' -X POST localhost:9250/test/doc/1?op_type=create -d '{"title":"test"}'

{"_index":"test","_type":"doc","_id":"1","_version":1,"result":"created","_shards":# {"total":2,"successful":2,"failed":0},"_seq_no":0,"_primary_term":1}

curl -H 'Content-Type: application/json' -X PUT localhost:9250/test/doc/1?op_type=create -d '{"title":"test"}'

{"error":# {"root_cause":[# {"type":"version_conflict_engine_exception","reason":"[doc][1]: version conflict, document already exists (current version [1])","index_uuid":"u_hk8__eTiC3fSjIarlIiQ","shard":"4","index":"test"}],"type":"version_conflict_engine_exception","reason":"[doc][1]: version conflict, document already exists (current version [1])","index_uuid":"u_hk8__eTiC3fSjIarlIiQ","shard":"4","index":"test"},"status":409}

curl -H 'Content-Type: application/json' -X POST localhost:9250/test/doc -d '{"title":"test"}'

{"_index":"test","_type":"doc","_id":"qGZgN2EByDMEGVYQkNI4","_version":1,"result":"created","_shards":# {"total":2,"successful":2,"failed":0},"_seq_no":0,"_primary_term":1}

curl localhost:9200 | jq '.version.number'

"6.1.2"

curl -H 'Content-Type: application/json' -X POST localhost:9200/test/doc -d '{"title":"test"}'

{"_index":"test","_type":"doc","_id":"kYZpN2EB3Vk5YTrSK5Pn","_version":1,"result":"created","_shards":# {"total":2,"successful":1,"failed":0},"_seq_no":1,"_primary_term":1}

curl -H 'Content-Type: application/json' -X POST localhost:9200/test/doc?op_type=create -d {"title":"test"}

{"error":# {"root_cause":[# {"type":"action_request_validation_exception","reason":"Validation Failed: 1: an id must be provided if version type or value are set;"}],"type":"action_request_validation_exception","reason":"Validation Failed: 1: an id must be provided if version type or value are set;"},"status":400}

curl -H 'Content-Type: application/json' -X PUT localhost:9200/test/doc/1?op_type=create -d '{"title":"test"}'

{"_index":"test","_type":"doc","_id":"1","_version":3,"result":"created","_shards":{"total":2,"successful":2,"failed":0},"_seq_no":2,"_primary_term":1}

curl -H 'Content-Type: application/json' -X PUT localhost:9200/test/doc/1?op_type=create -d '{"title":"test"}'

{"error":{"root_cause":[{"type":"version_conflict_engine_exception","reason":"[doc][1]: version conflict, document already exists (current version [3])","index_uuid":"u_hk8__eTiC3fSjIarlIiQ","shard":"4","index":"test"}],"type":"version_conflict_engine_exception","reason":"[doc][1]: version conflict, document already exists (current version [3])","index_uuid":"u_hk8__eTiC3fSjIarlIiQ","shard":"4","index":"test"},"status":409}

curl -H 'Content-Type: application/json' -X POST localhost:9250/test/doc -d '{"title":"test"}'

{"_index":"test","_type":"doc","_id":"s_iiPGEBLenwa_nplLPj","_version":1,"result":"created","_shards":{"total":2,"successful":2,"failed":0},"_seq_no":0,"_primary_term":1}

karmi pushed a commit that referenced this issue Jan 28, 2018
Previously, the "Create" API has required the `:id` argument, although it is _not_ required
[https://www.elastic.co/guide/en/elasticsearch/reference/current/docs-index_.html#_automatic_id_generation].

The REST API specs list it as required, though: <https://github.com/elastic/elasticsearch/blob/b47b399f000fce42eac0e00b9fdcf969122660ab/rest-api-spec/src/main/resources/rest-api-spec/api/create.json#L11>

This patch changes the logic of calling the `index` API method: when the `:id` is present,
it adds the `op_type` parameter, if it's missing, it simply calls the `index` method with parameters "as is".

Closes #491

(cherry picked from commit b1a8482)
@karmi
Copy link
Contributor

karmi commented Jan 28, 2018

I've merged the fix into the master and 6.x and released as part of 6.0.1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants