Make typeless APIs usable with indices whose type name is different from `_doc` #35790

jpountz · 2018-11-21T14:50:44Z

This commit makes document, update, explain, termvectors and mapping
typeless APIs work on indices that have a type whose name is not _doc.
Unfortunately, this needs to be a bit of a hack since I didn't want calls with
random type names to see documents with the type name that the user had chosen
upon type creation.

The explain and termvectors do not support being called without a type for
now so the test is just using _doc as a type for now, we will need to fix
tests later but this shouldn't require further changes server-side since passing
_doc as a type name is what typeless APIs do internally anyway.

Relates #35190

…rom `_doc`. This commit makes `document`, `update`, `explain`, `termvectors` and `mapping` typeless APIs work on indices that have a type whose name is not `_doc`. Unfortunately, this needs to be a bit of a hack since I didn't want calls with random type names to see documents with the type name that the user had chosen upon type creation. The `explain` and `termvectors` do not support being called without a type for now so the test is just using `_doc` as a type for now, we will need to fix tests later but this shouldn't require further changes server-side since passing `_doc` as a type name is what typeless APIs do internally anyway. Relates elastic#35190

elasticmachine · 2018-11-21T14:50:47Z

Pinging @elastic/es-search

jtibshirani

Thanks @jpountz for working on this! Some comments:

I noticed that when there is an existing mapping type, a typeless 'put mapping' call can result in an assertion error around mapping versions. Here is a short reproduction: https://gist.github.com/jtibshirani/66bb9719a6398fe05d966bf2aba4c68a
I think we also need to account for the types mismatch when checking whether routing is required. This applies to a few of the APIs, but here is an example call for explain requests: https://github.com/elastic/elasticsearch/blob/master/server/src/main/java/org/elasticsearch/action/explain/TransportExplainAction.java#L86
Maybe we could add tests for 'bulk' requests and mtermvectors for completeness?

I also thought it would be good if someone from the distributed team could take a look at the PR, as these changes are a bit subtle, and there may be other special cases to consider.

jtibshirani · 2018-11-22T00:43:12Z

server/src/test/java/org/elasticsearch/index/shard/IndexShardTests.java

@@ -3497,4 +3500,57 @@ public void testResetEngine() throws Exception {
    public Settings threadPoolSettings() {
        return Settings.builder().put(super.threadPoolSettings()).put("thread_pool.estimated_time_interval", "5ms").build();
    }
+
+    public void testDeleteOnWrongType() throws IOException {


Small comment: maybe this test and the next should have consistent names (like testTypelessDelete and testTypelessGet)?

jtibshirani · 2018-11-22T02:00:54Z

server/src/main/java/org/elasticsearch/search/DefaultSearchContext.java

@@ -302,6 +302,11 @@ public Query buildFilteredQuery(Query query) {

    private Query createTypeFilter(String[] types) {
        if (types != null && types.length >= 1) {
+            if (Arrays.asList(types).contains(MapperService.SINGLE_MAPPING_NAME)) {


I think this was added to deal with 'explain' requests, but for me this makes the overall search behavior a bit unintuitive. If a user explicitly adds a (deprecated) type filter on _doc in a search, I think they would expect the filter to be applied instead of being ignored. From a user's perspective the search APIs differ from the document CRUD APIs, where the _doc is instead seen as part of the endpoint (and not an explicit filter).

Good point.

jpountz · 2018-11-23T17:49:04Z

server/src/main/java/org/elasticsearch/cluster/metadata/IndexMetaData.java

+     * has no mappings.
+     * @deprecated Use {@link #mapping()} instead now that indices have a single type
+     */
+    @Deprecated
    public ImmutableOpenMap<String, MappingMetaData> getMappings() {


Note that we should remove this one too in the future but decided to keep it for a follow-up PR as this PR would otherwise be much larger.

jpountz · 2018-11-23T18:17:48Z

Thanks for the comments @jtibshirani, I pushed more changes to address the issues that you found.

jtibshirani

This looks good to me, just a couple small comments. Do you think someone else should look it over as well, who's very familiar with some of these core classes? This PR also reminded me that we need to remove support for the _default_ mapping type.

jtibshirani · 2018-11-26T19:41:09Z

server/src/main/java/org/elasticsearch/cluster/metadata/IndexMetaData.java

+     * has no mappings.
+     * @deprecated Use {@link #mapping()} instead now that indices have a single type
+     */
+    @Deprecated
    public ImmutableOpenMap<String, MappingMetaData> getMappings() {


jtibshirani · 2018-11-26T19:56:16Z

rest-api-spec/src/main/resources/rest-api-spec/test/mtermvectors/30_mix_typeless_typeful.yml

@@ -0,0 +1,33 @@
+---
+"mtermvectors without types on an index that has types":


Small comment: inconsistent spacing in this test (extra newline in mtermvectors.body.docs, spaces before :).

jtibshirani · 2018-11-26T20:04:15Z

server/src/main/java/org/elasticsearch/action/explain/TransportExplainAction.java

@@ -104,15 +107,19 @@ protected void asyncShardOperation(ExplainRequest request, ShardId shardId,

    @Override
    protected ExplainResponse shardOperation(ExplainRequest request, ShardId shardId) throws IOException {
+        String[] types;
+        if (MapperService.SINGLE_MAPPING_NAME.equals(request.type())) { // typeless explain call
+            types = null;


Small comment, but I think an empty array would be more consistent with the default value of types in ShardSearchRequest, QueryShardContext, etc.?

jpountz · 2018-11-27T09:08:36Z

The _default_ mapping is already disallowed on 7.x indices:

elasticsearch/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java

Lines 414 to 416 in be75b40

    
           if (indexSettings.getIndexVersionCreated().onOrAfter(Version.V_7_0_0_alpha1)) { 
        
               throw new IllegalArgumentException("The [default] mapping cannot be updated on index [" + index().getName() + 
        
                       "]: defaults mappings are not useful anymore now that indices can have at most one type.");

jpountz · 2018-11-27T09:33:28Z

@javanna Would you mind having a look at this change?

jtibshirani

Thanks @jpountz, I had missed the _default_ mapping removal.

jtibshirani · 2018-11-29T22:45:50Z

@javanna just a friendly ping on this (I have a couple PRs that are dependent on this change).

javanna

I left some questions, nothing major though I think.

javanna · 2018-11-30T15:25:00Z

rest-api-spec/src/main/resources/rest-api-spec/test/delete/70_mix_typeless_typeful.yml

+          body:   { foo: bar }
+
+ - do:
+      catch:   bad_request


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:

Rejecting mapping update to [index] as the final mapping would have more than 1 type: [not_doc, some_random_type]

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...

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.

javanna · 2018-11-30T15:27:04Z

rest-api-spec/src/main/resources/rest-api-spec/test/explain/40_mix_typeless_typeful.yml

+      indices.refresh: {}
+
+ - do:
+      catch:  missing


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?

javanna · 2018-11-30T15:28:43Z

rest-api-spec/src/main/resources/rest-api-spec/test/delete/70_mix_typeless_typeful.yml

+      delete:
+          index:  index
+          type:   some_random_type
+          id:     1


maybe add some assert for the error message that comes back too?

javanna · 2018-11-30T15:33:35Z

rest-api-spec/src/main/resources/rest-api-spec/test/get/100_mix_typeless_typeful.yml

+ - match:   { _type:    "_doc" }
+ - match:   { _id:      "1"}
+ - match:   { _version: 1}
+ - match:   { _source: { foo: bar }}


do we also need to check whether the doc comes back when specifying not_doc as a type?

I didn't add a test for it because this is already tested by get/11_basic_with_types.yml?

javanna · 2018-11-30T15:34:01Z

rest-api-spec/src/main/resources/rest-api-spec/test/delete/70_mix_typeless_typeful.yml

+ - match:   { _index:   "index" }
+ - match:   { _type:    "_doc" }
+ - match:   { _id:      "1"}
+ - match:   { _version: 2}


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?

indeed this is tested by delete/13_basic_with_types.yml

javanna · 2018-11-30T15:34:20Z

rest-api-spec/src/main/resources/rest-api-spec/test/explain/40_mix_typeless_typeful.yml

+ - match:   { _type:    "_doc" }
+ - match:   { _id:      "1"}
+ - is_true: matched
+ - match:   { explanation.value: 1 }


do we also need to check whether the doc is found when specifying not_doc as a type?

we have one already

javanna · 2018-11-30T15:40:28Z

rest-api-spec/src/main/resources/rest-api-spec/test/termvectors/50_mix_typeless_typeful.yml

+          type:  some_random_type
+          id:    1
+
+ - is_false: found 


do we also need to check what happens when providing not_doc as a type?

we have one already

javanna · 2018-11-30T15:43:45Z

server/src/main/java/org/elasticsearch/action/explain/TransportExplainAction.java

-            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


for us :) thanks for catching I'll fix

markharwood · 2018-12-03T18:15:20Z

If I have a mapping with custom type my_type and I index or update using index/_doc/id endpoint shouldn't the response show "_type":"my_type"? It currently returns "_type":"_doc" and I thought we were asking users to think of _doc in the url as a verb rather than a doc type. A search still returns the "_type":"my_type" which contradicts the index/update response.

jpountz · 2018-12-04T10:57:58Z

@markharwood This is done consciously: by always returning _doc as a value for the _type field, we hope that users will stop reading its value, which is in-turn expected to decrease chances of breakage once we entirely remove it from responses in the future.

jpountz · 2018-12-04T11:01:35Z

@elasticmachine test this please

markharwood · 2018-12-04T11:12:39Z

by always returning _doc as a value for the _type field,

Are search responses planning to do this too then?

jpountz · 2018-12-04T14:56:30Z

@markharwood I'm not 100% sure about this one, the _search API is indeed more subtle. The work that @nik9000 is doing to allow us to change the format based on the version that the client is expecting would hopefully help here.

- deprecate type-based constructors of IndexRequest - update tests to use typeless IndexRequest constructors - no yaml tests as they have been already added in elastic#35790 Relates to elastic#35190

* Deprecate types in index API - deprecate type-based constructors of IndexRequest - update tests to use typeless IndexRequest constructors - no yaml tests as they have been already added in #35790 Relates to #35190

We introduce a typeless API in elastic#35790 where we translate the default docType "_doc" to the user-defined docType. However, we still use docType from the source rather the translated type (i.e, type of the Mapper) when parsing a document. This leads to a situation where we have two translog operations for the same document with different types: - prvOp [Index{id='9LCpwGcBkJN7eZxaB54L', type='_doc', seqNo=1, primaryTerm=1, version=1, autoGeneratedIdTimestamp=1545125562123}] - newOp [Index{id='9LCpwGcBkJN7eZxaB54L', type='not_doc', seqNo=1, primaryTerm=1, version=1, autoGeneratedIdTimestamp=-1}] Closes elastic#36769

We introduce a typeless API in #35790 where we translate the default docType "_doc" to the user-defined docType. However, we do not rewrite the SourceToParse with the resolved docType. This leads to a situation where we have two translog operations for the same document with different types: - prvOp [Index{id='9LCpwGcBkJN7eZxaB54L', type='_doc', seqNo=1, primaryTerm=1, version=1, autoGeneratedIdTimestamp=1545125562123}] - newOp [Index{id='9LCpwGcBkJN7eZxaB54L', type='not_doc', seqNo=1, primaryTerm=1, version=1, autoGeneratedIdTimestamp=-1}] Closes #36769

jpountz added >feature :Search Foundations/Mapping Index mappings, including merging and defining field types v7.0.0 labels Nov 21, 2018

jpountz requested a review from jtibshirani November 21, 2018 14:50

jtibshirani reviewed Nov 22, 2018

View reviewed changes

jpountz added 3 commits November 22, 2018 18:01

iter

718cf42

Merge branch 'master' into enhancement/typeless_apis_work_with_any_type

29ede50

iter

f0a5d50

jpountz commented Nov 23, 2018

View reviewed changes

Add tests.

5f5c802

Merge branch 'master' into enhancement/typeless_apis_work_with_any_type

3bfb13f

jtibshirani reviewed Nov 26, 2018

View reviewed changes

jpountz added 2 commits November 27, 2018 10:15

Merge branch 'master' into enhancement/typeless_apis_work_with_any_type

e8a6b2b

iter

13e9539

jpountz requested a review from javanna November 27, 2018 09:33

jtibshirani approved these changes Nov 27, 2018

View reviewed changes

jpountz added 2 commits November 28, 2018 12:24

Merge branch 'master' into enhancement/typeless_apis_work_with_any_type

981db1e

iter

10bb764

javanna reviewed Nov 30, 2018

View reviewed changes

Review feedback.

9619fe4

This was referenced Dec 3, 2018

Implementation tracking for 7.0 types deprecation. #35190

Closed

Deprecate types in update requests. #36181

Merged

markharwood mentioned this pull request Dec 4, 2018

Deprecate types in termvector and mtermvector requests. #36182

Merged

jpountz added 2 commits December 4, 2018 14:29

Merge branch 'master' into enhancement/typeless_apis_work_with_any_type

9bc3cc5

iter

a7d6130

jpountz merged commit d24b40f into elastic:master Dec 4, 2018

jpountz deleted the enhancement/typeless_apis_work_with_any_type branch December 4, 2018 18:22

mayya-sharipova mentioned this pull request Dec 12, 2018

Deprecate types in index API #36575

Merged

jtibshirani added the backport pending label Dec 18, 2018

jtibshirani mentioned this pull request Dec 19, 2018

Typeless index call can fail against an index with a custom type. #36811

Closed

dnhatn mentioned this pull request Dec 21, 2018

Rewrite SourceToParse with resolved docType #36921

Merged

jtibshirani mentioned this pull request Jan 7, 2019

Deprecate reference to _type in lookup queries #37016

Merged

jtibshirani removed the backport pending label Jan 15, 2019

jtibshirani mentioned this pull request Jan 15, 2019

Solidify the approach to allowing typeless requests on an index with a custom type. #37450

Closed

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

		@@ -0,0 +1,33 @@
		---
		"mtermvectors without types on an index that has types":

Make typeless APIs usable with indices whose type name is different from _doc #35790

Make typeless APIs usable with indices whose type name is different from _doc #35790

Conversation

jpountz commented Nov 21, 2018

elasticmachine commented Nov 21, 2018

jtibshirani left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtibshirani Nov 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpountz commented Nov 23, 2018

jtibshirani left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpountz commented Nov 27, 2018

jpountz commented Nov 27, 2018

jtibshirani left a comment

Choose a reason for hiding this comment

jtibshirani commented Nov 29, 2018

javanna left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markharwood commented Dec 3, 2018

jpountz commented Dec 4, 2018

jpountz commented Dec 4, 2018

markharwood commented Dec 4, 2018

jpountz commented Dec 4, 2018

Make typeless APIs usable with indices whose type name is different from `_doc` #35790

Make typeless APIs usable with indices whose type name is different from `_doc` #35790

jtibshirani Nov 22, 2018 •

edited

Loading