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

The enabled setting is not updatable (and probably shouldn't) #33566

Closed
ywelsch opened this issue Sep 10, 2018 · 11 comments
Closed

The enabled setting is not updatable (and probably shouldn't) #33566

ywelsch opened this issue Sep 10, 2018 · 11 comments
Labels
>bug help wanted adoptme :Search Foundations/Mapping Index mappings, including merging and defining field types Team:Search Foundations Meta label for the Search Foundations team in Elasticsearch

Comments

@ywelsch
Copy link
Contributor

ywelsch commented Sep 10, 2018

According to the docs , the enabled setting for mappings should be updatable (see TIP at the end), which it actually isn't:

PUT my_index 
{
  "mappings": {
    "doc": { 
      "properties": {
        "bla": {
          "properties": {
            "title":    { "type": "text"  }
          }
        }
      }
    }
  }
}

GET /my_index/_mapping/doc

PUT /my_index/_mapping/doc
{
  "properties": {
    "bla": {
      "enabled" : false
    }
  }
}

GET /my_index/_mapping/doc

-> returns same initial mapping :sadtrombone:

While implementing a fix, I noticed that we probably do not want to change and fix this, as this might lead to a surprising effects if the setting is changed during indexing (A field might be indexed on the primary but not on the replica (or vice versa)). Similarly, it might interfere in funny ways with reindexing.

I suggest updating the docs to say that this field is non-updateable, but wanted to raise it first for discussion with @elastic/es-search-aggs.

@ywelsch ywelsch added >bug :Search Foundations/Mapping Index mappings, including merging and defining field types team-discuss labels Sep 10, 2018
@colings86
Copy link
Contributor

Additionally we should probably fail the request to update the mappings in this case too!

@jpountz
Copy link
Contributor

jpountz commented Sep 12, 2018

+1 to disallow updating this field

Relates to #28070

@jpountz
Copy link
Contributor

jpountz commented Sep 18, 2018

We discussed this issue and agreed to disallow updates to the enabled property via mapping updates both ways (false -> true and true -> false). Such mapping updates should raise an error and we should fix our docs.

@jpountz jpountz added help wanted adoptme and removed team-discuss labels Sep 18, 2018
@cbismuth
Copy link
Contributor

I'll have a look on this one.

@cbismuth
Copy link
Contributor

cbismuth commented Sep 20, 2018

Here is a complete cURL recreation script in this Gist.
Bug successfully reproduced against up-to-date master branch.

I'm now debugging to see how o.e.r.a.a.i.RestPutMappingAction is implemented, I'll let you know.

@cbismuth
Copy link
Contributor

While implementing a fix [...]

Sorry @ywelsch, I missed the line where you were saying you has already started to work on this. Feel free to let me know what you want to do. I can push my changes to share with you what I already have.

Anyway, if the enabled field shouldn't be updatable, should it be totally removed?

@cbismuth
Copy link
Contributor

I looks like an exception should be raised in the conditional block below.

} else if (propNode.size() == 1 && propNode.get("enabled") != null) {

@ywelsch
Copy link
Contributor Author

ywelsch commented Sep 20, 2018

Sorry @ywelsch, I missed the line where you were saying you has already started to work on this. Feel free to let me know what you want to do.

The "fix" I was working on was to allow dynamically changing "enable", until I noticed that this is not what we want. I have not done any subsequent work on this, so you are free to proceed on this.

@cbismuth
Copy link
Contributor

cbismuth commented Sep 21, 2018

I've opened a PR, could please tell me if this design could fit? See here #33933.

If you agree on this, I'll then implement test cases and check for existing test failures.

cbuescher pushed a commit that referenced this issue Oct 2, 2018
This commit adds a check for "enabled" attribute change for types when
a RestPutMappingAction is received. A MappingException is thrown when
such a change is detected.  Change are prevented in both ways: "false -> true"
and "true -> false".

Closes #33566
kcm pushed a commit that referenced this issue Oct 30, 2018
This commit adds a check for "enabled" attribute change for types when
a RestPutMappingAction is received. A MappingException is thrown when
such a change is detected.  Change are prevented in both ways: "false -> true" 
and "true -> false".

Closes #33566
henningandersen added a commit to henningandersen/elasticsearch that referenced this issue Jun 27, 2019
Removed the invalid tip that enabled can be updated for existing fields
and clarified instead that it cannot.

Related to elastic#33566 and elastic#33933
henningandersen added a commit that referenced this issue Jun 28, 2019
Removed the invalid tip that enabled can be updated for existing fields
and clarified instead that it cannot.

Related to #33566 and #33933
henningandersen added a commit that referenced this issue Jun 28, 2019
Removed the invalid tip that enabled can be updated for existing fields
and clarified instead that it cannot.

Related to #33566 and #33933
henningandersen added a commit that referenced this issue Jun 28, 2019
Removed the invalid tip that enabled can be updated for existing fields
and clarified instead that it cannot.

Related to #33566 and #33933
henningandersen added a commit that referenced this issue Jun 28, 2019
Removed the invalid tip that enabled can be updated for existing fields
and clarified instead that it cannot.

Related to #33566 and #33933
@BMomani
Copy link

BMomani commented Apr 23, 2024

what is the current status of this
the documentation say
The enabled setting can be updated on existing fields using the PUT mapping API.
is that correct or not?

@javanna
Copy link
Member

javanna commented Jun 18, 2024

@BMomani the docs you linked are for 6.5. We have updated more recent docs to reflect code changes, see ##43701 . It is correct that you cannot update the enabled flag.

@javanna javanna added the Team:Search Foundations Meta label for the Search Foundations team in Elasticsearch label Jul 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>bug help wanted adoptme :Search Foundations/Mapping Index mappings, including merging and defining field types Team:Search Foundations Meta label for the Search Foundations team in Elasticsearch
Projects
None yet
Development

No branches or pull requests

6 participants