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

url params parsing should not be lenient #14719

Closed
rjernst opened this issue Nov 12, 2015 · 11 comments
Closed

url params parsing should not be lenient #14719

rjernst opened this issue Nov 12, 2015 · 11 comments
Assignees
Labels
:Core/Infra/REST API REST infrastructure and utilities >enhancement

Comments

@rjernst
Copy link
Member

rjernst commented Nov 12, 2015

If you use eg the _analyze api, and mispell the tokenizer, it will happily go on using the standard analyzer (if you have filters specified, it will ignore them as well).

We should remove all leniency from url param parsing, it does not help anyone, but only causes confusion. Rest actions should remove parameters as they are parsed, and fail when there are extras left (eg Unknown parameter "tokenzier" for _analyze)

@dadoonet
Copy link
Member

++

@clintongormley clintongormley added >enhancement help wanted adoptme :Core/Infra/REST API REST infrastructure and utilities labels Nov 17, 2015
@martinstuga
Copy link
Contributor

I would be happy to implement this change.

May I go ahead?

@rjernst
Copy link
Member Author

rjernst commented Nov 28, 2015

@martinstuga Please do! Note that it is a large task. It means handling every rest handler (although doing in a series of smaller PRs would be just fine and easier to review).

@martinstuga
Copy link
Contributor

This validation must be implemented on every single RestHandler?

I think that we can do it on a generic and less error prone way by checking if all passed params are used in the handleRequest method. Sounds good?

@rjernst
Copy link
Member Author

rjernst commented Nov 28, 2015

Yes, what I mean is, right now I believe each reat handler just does a get on the params they use. They will need to do a remove, and then have a check at the end (in shared code) that there are no params left.

@martinstuga
Copy link
Contributor

I think there is a way to do everything with shared code. I'll implement it and then you check if it's correct.
Ok?

@rjernst
Copy link
Member Author

rjernst commented Nov 28, 2015

Sure, please do.

@martinstuga
Copy link
Contributor

I have a few questions, in the case there're wrong parameters, which behaviour should we implement?

Throw an Exception?
Send a warning message in the response?

In order to keep backward compatibility, don't you think we should add an parameter to enable/disable this feature?

@nik9000
Copy link
Member

nik9000 commented Nov 30, 2015

Throw an Exception?

Yes. Make sure the exception is translated into a 400 level error.

In order to keep backward compatibility, don't you think we should add an parameter to enable/disable this feature?

Do it in master and add it to the breaking changes list I think. I'm not personally a huge fan of silently ignoring parameters even in 2.x but I think it'd be quite uncontroversial to do it in 3.0.

@martinstuga
Copy link
Contributor

I've already created a PR just with the validation do GET rest requests.
Please, validate if everything is OK and let me know.

Thanks.

martinstuga added a commit to martinstuga/elasticsearch that referenced this issue Dec 8, 2015
First commit to allow the team to check if this approach is OK.
@martinstuga
Copy link
Contributor

Please, anybody can give me some feedback about the PR #15462?

I'm waiting for this feedback to go ahead with the other rest services (PUT, DELETE,...)

Thanks.

martinstuga added a commit to martinstuga/elasticsearch that referenced this issue Dec 24, 2015
martinstuga added a commit to martinstuga/elasticsearch that referenced this issue Dec 24, 2015
First commit to allow the team to check if this approach is OK.
martinstuga added a commit to martinstuga/elasticsearch that referenced this issue Dec 24, 2015
martinstuga added a commit to martinstuga/elasticsearch that referenced this issue Dec 24, 2015
First commit to allow the team to check if this approach is OK.
@jasontedor jasontedor self-assigned this Sep 21, 2016
@jasontedor jasontedor removed the help wanted adoptme label Sep 21, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Core/Infra/REST API REST infrastructure and utilities >enhancement
Projects
None yet
Development

No branches or pull requests

6 participants