-
Notifications
You must be signed in to change notification settings - Fork 25k
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
Make typeless APIs usable with indices whose type name is different from _doc
#35790
Changes from 1 commit
d376d9d
718cf42
29ede50
f0a5d50
5f5c802
3bfb13f
e8a6b2b
13e9539
981db1e
10bb764
9619fe4
9bc3cc5
a7d6130
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,40 @@ | ||
--- | ||
"DELETE with typeless API on an index that has types": | ||
|
||
- skip: | ||
version: " - 6.99.99" | ||
reason: Typeless APIs were introduced in 7.0.0 | ||
|
||
- do: | ||
indices.create: # not using include_type_name: false on purpose | ||
index: index | ||
body: | ||
mappings: | ||
not_doc: | ||
properties: | ||
foo: | ||
type: "keyword" | ||
|
||
- do: | ||
index: | ||
index: index | ||
type: not_doc | ||
id: 1 | ||
body: { foo: bar } | ||
|
||
- do: | ||
catch: bad_request | ||
delete: | ||
index: index | ||
type: some_random_type | ||
id: 1 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. maybe add some assert for the error message that comes back too? |
||
|
||
- do: | ||
delete: | ||
index: index | ||
id: 1 | ||
|
||
- match: { _index: "index" } | ||
- match: { _type: "_doc" } | ||
- match: { _id: "1"} | ||
- match: { _version: 2} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do we also need to check whether the doc gets deleted when specifying not_doc as a type? Or maybe that is already covered by existing tests? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. indeed this is tested by |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,56 @@ | ||
--- | ||
"Explain with typeless API on an index that has types": | ||
|
||
- skip: | ||
version: " - 6.99.99" | ||
reason: Typeless APIs were introduced in 7.0.0 | ||
|
||
- do: | ||
indices.create: # not using include_type_name: false on purpose | ||
index: index | ||
body: | ||
mappings: | ||
not_doc: | ||
properties: | ||
foo: | ||
type: "keyword" | ||
|
||
- do: | ||
index: | ||
index: index | ||
type: not_doc | ||
id: 1 | ||
body: { foo: bar } | ||
|
||
- do: | ||
indices.refresh: {} | ||
|
||
- do: | ||
catch: missing | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this makes more sense to me compared to the 400 above. Yet I think - besides which error code is better - the two scenarios (explain and delete) should return the same error code when referring to a doc belonging to a type that's not there? |
||
explain: | ||
index: index | ||
type: some_random_type | ||
id: 1 | ||
body: | ||
query: | ||
match_all: {} | ||
|
||
- match: { _index: "index" } | ||
- match: { _type: "some_random_type" } | ||
- match: { _id: "1"} | ||
- match: { matched: false} | ||
|
||
- do: | ||
explain: | ||
index: index | ||
type: _doc #todo: make _explain typeless and remove this | ||
id: 1 | ||
body: | ||
query: | ||
match_all: {} | ||
|
||
- match: { _index: "index" } | ||
- match: { _type: "_doc" } | ||
- match: { _id: "1"} | ||
- is_true: matched | ||
- match: { explanation.value: 1 } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do we also need to check whether the doc is found when specifying not_doc as a type? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we have one already |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,46 @@ | ||
--- | ||
"GET with typeless API on an index that has types": | ||
|
||
- skip: | ||
version: " - 6.99.99" | ||
reason: Typeless APIs were introduced in 7.0.0 | ||
|
||
- do: | ||
indices.create: # not using include_type_name: false on purpose | ||
index: index | ||
body: | ||
mappings: | ||
not_doc: | ||
properties: | ||
foo: | ||
type: "keyword" | ||
|
||
- do: | ||
index: | ||
index: index | ||
type: not_doc | ||
id: 1 | ||
body: { foo: bar } | ||
|
||
- do: | ||
catch: missing | ||
get: | ||
index: index | ||
type: some_random_type | ||
id: 1 | ||
|
||
- match: { _index: "index" } | ||
- match: { _type: "some_random_type" } | ||
- match: { _id: "1"} | ||
- match: { found: false} | ||
|
||
- do: | ||
get: | ||
index: index | ||
id: 1 | ||
|
||
- match: { _index: "index" } | ||
- match: { _type: "_doc" } | ||
- match: { _id: "1"} | ||
- match: { _version: 1} | ||
- match: { _source: { foo: bar }} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do we also need to check whether the doc comes back when specifying not_doc as a type? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I didn't add a test for it because this is already tested by |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,62 @@ | ||
--- | ||
"Index with typeless API on an index that has types": | ||
|
||
- skip: | ||
version: " - 6.99.99" | ||
reason: Typeless APIs were introduced in 7.0.0 | ||
|
||
- do: | ||
indices.create: # not using include_type_name: false on purpose | ||
index: index | ||
body: | ||
mappings: | ||
not_doc: | ||
properties: | ||
foo: | ||
type: "keyword" | ||
|
||
- do: | ||
index: | ||
index: index | ||
id: 1 | ||
body: { foo: bar } | ||
|
||
- match: { _index: "index" } | ||
- match: { _type: "_doc" } | ||
- match: { _id: "1"} | ||
- match: { _version: 1} | ||
|
||
- do: | ||
get: # not using typeless API on purpose | ||
index: index | ||
type: not_doc | ||
id: 1 | ||
|
||
- match: { _index: "index" } | ||
- match: { _type: "not_doc" } # the important bit to check | ||
- match: { _id: "1"} | ||
- match: { _version: 1} | ||
- match: { _source: { foo: bar }} | ||
|
||
|
||
- do: | ||
index: | ||
index: index | ||
body: { foo: bar } | ||
|
||
- match: { _index: "index" } | ||
- match: { _type: "_doc" } | ||
- match: { _version: 1} | ||
- set: { _id: id } | ||
|
||
- do: | ||
get: # using typeful API on purpose | ||
index: index | ||
type: not_doc | ||
id: '$id' | ||
|
||
- match: { _index: "index" } | ||
- match: { _type: "not_doc" } # the important bit to check | ||
- match: { _id: $id} | ||
- match: { _version: 1} | ||
- match: { _source: { foo: bar }} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
--- | ||
"GET mapping with typeless API on an index that has types": | ||
|
||
- skip: | ||
version: " - 6.99.99" | ||
reason: include_type_name was introduced in 7.0.0 | ||
|
||
- do: | ||
indices.create: # not using include_type_name: false on purpose | ||
index: index | ||
body: | ||
mappings: | ||
not_doc: | ||
properties: | ||
foo: | ||
type: "keyword" | ||
|
||
- do: | ||
indices.get_mapping: | ||
include_type_name: false | ||
index: index | ||
|
||
- match: { index.mappings.properties.foo.type: "keyword" } |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,43 @@ | ||
--- | ||
"PUT mapping with typeless API on an index that has types": | ||
|
||
- skip: | ||
version: " - 6.99.99" | ||
reason: include_type_name was introduced in 7.0.0 | ||
|
||
- do: | ||
indices.create: # not using include_type_name: false on purpose | ||
index: index | ||
body: | ||
mappings: | ||
not_doc: | ||
properties: | ||
foo: | ||
type: "keyword" | ||
|
||
- do: | ||
indices.put_mapping: | ||
include_type_name: false | ||
index: index | ||
body: | ||
properties: | ||
bar: | ||
type: "long" | ||
|
||
- do: | ||
indices.get_mapping: | ||
include_type_name: false | ||
index: index | ||
|
||
- match: { index.mappings.properties.foo.type: "keyword" } | ||
- match: { index.mappings.properties.bar.type: "long" } | ||
|
||
- do: | ||
catch: bad_request | ||
indices.put_mapping: | ||
index: index | ||
body: | ||
some_other_type: | ||
properties: | ||
bar: | ||
type: "long" |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,45 @@ | ||
--- | ||
"Term vectors with typeless API on an index that has types": | ||
|
||
- skip: | ||
version: " - 6.99.99" | ||
reason: Typeless APIs were introduced in 7.0.0 | ||
|
||
- do: | ||
indices.create: # not using include_type_name: false on purpose | ||
index: index | ||
body: | ||
mappings: | ||
not_doc: | ||
properties: | ||
foo: | ||
type: "text" | ||
term_vector: "with_positions" | ||
|
||
- do: | ||
index: | ||
index: index | ||
type: not_doc | ||
id: 1 | ||
body: { foo: bar } | ||
|
||
- do: | ||
indices.refresh: {} | ||
|
||
- do: | ||
termvectors: | ||
index: index | ||
type: _doc # todo: remove when termvectors support typeless API | ||
id: 1 | ||
|
||
- is_true: found | ||
- match: {_type: _doc} | ||
- match: {term_vectors.foo.terms.bar.term_freq: 1} | ||
|
||
- do: | ||
termvectors: | ||
index: index | ||
type: some_random_type | ||
id: 1 | ||
|
||
- is_false: found | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do we also need to check what happens when providing not_doc as a type? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we have one already |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,39 @@ | ||
--- | ||
"Update with typeless API on an index that has types": | ||
|
||
- skip: | ||
version: " - 6.99.99" | ||
reason: Typeless APIs were introduced in 7.0.0 | ||
|
||
- do: | ||
indices.create: # not using include_type_name: false on purpose | ||
index: index | ||
body: | ||
mappings: | ||
not_doc: | ||
properties: | ||
foo: | ||
type: "keyword" | ||
|
||
- do: | ||
index: | ||
index: index | ||
type: not_doc | ||
id: 1 | ||
body: { foo: bar } | ||
|
||
- do: | ||
update: | ||
index: index | ||
id: 1 | ||
body: | ||
doc: | ||
foo: baz | ||
|
||
- do: | ||
get: | ||
index: index | ||
type: not_doc | ||
id: 1 | ||
|
||
- match: { _source.foo: baz } |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -35,6 +35,8 @@ | |
import org.elasticsearch.index.IndexService; | ||
import org.elasticsearch.index.engine.Engine; | ||
import org.elasticsearch.index.get.GetResult; | ||
import org.elasticsearch.index.mapper.IdFieldMapper; | ||
import org.elasticsearch.index.mapper.Uid; | ||
import org.elasticsearch.index.shard.IndexShard; | ||
import org.elasticsearch.index.shard.ShardId; | ||
import org.elasticsearch.search.SearchService; | ||
|
@@ -109,10 +111,8 @@ protected ExplainResponse shardOperation(ExplainRequest request, ShardId shardId | |
SearchContext context = searchService.createSearchContext(shardSearchLocalRequest, SearchService.NO_TIMEOUT); | ||
Engine.GetResult result = null; | ||
try { | ||
Term uidTerm = context.mapperService().createUidTerm(request.type(), request.id()); | ||
if (uidTerm == null) { | ||
return new ExplainResponse(shardId.getIndexName(), request.type(), request.id(), false); | ||
} | ||
// No need to check the type, IndexShard#get does it for is | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. for is ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. for us :) thanks for catching I'll fix |
||
Term uidTerm = new Term(IdFieldMapper.NAME, Uid.encodeId(request.id())); | ||
result = context.indexShard().get(new Engine.Get(false, false, request.type(), request.id(), uidTerm)); | ||
if (!result.exists()) { | ||
return new ExplainResponse(shardId.getIndexName(), request.type(), request.id(), false); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I get where this comes from, yet the error is a bit weird, specifically because it mentions mapping update while we are deleting a document, which may confuse users:
Not sure whether this was previously discussed and/or can be improved. I was even wondering if this should have been a 404 - doc not found or 400 - request is wrong as the type is not there...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a bit unrelated to this PR, the behavior was already like this before. I agree it's not great but at the same time it's a bit tricky given that DELETE calls are write calls and that there are expectations regarding versioning if you delete a non-existing document and create a new one soon after with the same id.