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

Update API: Setting ctx.op to random string results in noop #43514

Open
spinscale opened this issue Jun 22, 2019 · 8 comments
Open

Update API: Setting ctx.op to random string results in noop #43514

spinscale opened this issue Jun 22, 2019 · 8 comments
Labels
:Distributed/CRUD A catch all label for issues around indexing, updating and getting a doc by id. Not search. help wanted adoptme Team:Distributed Meta label for distributed team

Comments

@spinscale
Copy link
Contributor

Elasticsearch version (bin/elasticsearch --version): 7.1.1

Description of the problem including expected versus actual behavior:

When using a script in the Update API, setting ctx.op to any string will result in a noop. IMO this should throw an exception, if it's anything else than the suggested ones.

This leniency is actually documented with a TODO, but I do not know the history behind that (that todo is from early 2017). Also a warn message is logged.

Steps to reproduce:

DELETE test

PUT test/_doc/1
{
  "key" : "value"
}

POST test/_update/1
{
  "script": {
    "source": "ctx._source.foo = 'bar'; ctx.op ='anything'",
    "lang": "painless"
  }
}

# document unchanged
GET test/_doc/1
@dnhatn dnhatn added the :Distributed/CRUD A catch all label for issues around indexing, updating and getting a doc by id. Not search. label Jun 23, 2019
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-distributed

@dnhatn dnhatn added the help wanted adoptme label Jun 24, 2019
@panwarab
Copy link

panwarab commented Jun 28, 2019

can I take this up? @spinscale @dnhatn

@dnhatn
Copy link
Member

dnhatn commented Jun 28, 2019

@abhiroj Yes

@panwarab
Copy link

panwarab commented Jun 28, 2019

cool! how should I get started to reproduce it @dnhatn can you point me to the Class

@henningandersen
Copy link
Contributor

@abhiroj I think resolving this takes following steps:

1.reproduce it using the example above against an elasticsearch node.
2. write a test that provokes the same, either as an ESIntegTestCase that does such an update against a test node and/or a ESTestCase that runs against UpdateHelper more directly.
3. remove the leniency and figuring out what kind of error reporting to do.
4. run the full suite of tests
5. repeat step 1 and check that things look good when running against a real node too.
6. open a PR with your changes.

@zacharymorn
Copy link
Contributor

It’s still present in the latest code base

# Request 
 curl -XPOST  'http://localhost:9200/test/_update/1' -H 'Content-Type: application/json' -u elastic-admin:elastic-password -d '
{
  "script": {
    "source": "ctx._source.foo = '\''bar'\''; ctx.op = '\''anything'\''",
    "lang": "painless"
  }
}'
# Response (“noop” for the result field)
{"_index":"test","_id":"1","_version":1,"result":"noop","_shards":{"total":0,"successful":0,"failed":0},"_seq_no":0,"_primary_term":1}

@abhiroj , are you currently working on this? If not I can take this up.

@zacharymorn
Copy link
Contributor

@spinscale I've create a WIP PR for this, please let me know if this aligns with what you are considering.

@zacharymorn
Copy link
Contributor

@spinscale Any update on this?

@rjernst rjernst added the Team:Distributed Meta label for distributed team label May 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Distributed/CRUD A catch all label for issues around indexing, updating and getting a doc by id. Not search. help wanted adoptme Team:Distributed Meta label for distributed team
Projects
None yet
Development

No branches or pull requests

7 participants