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

BUG: use a single trie for all methods #46482

Closed
suikammd opened this issue Sep 9, 2019 · 5 comments · Fixed by #46487
Closed

BUG: use a single trie for all methods #46482

suikammd opened this issue Sep 9, 2019 · 5 comments · Fixed by #46487
Assignees
Labels
:Core/Infra/REST API REST infrastructure and utilities >non-issue

Comments

@suikammd
Copy link

suikammd commented Sep 9, 2019

ES 6.x version use a single trie for all methods which results to covering the already registered handler params. ex
I have two methods : POST /events/{id}, GET /events/{name}, after registering the POST becomes /events/{name}.
check insertOrUpdate in https://github.com/elastic/elasticsearch/blob/5436581f696ce27513266090965afc824df76175/server/src/main/java/org/elasticsearch/common/path/PathTrie.java
ES 5.x use different trie for different methods, more reasonable?

@henningandersen
Copy link
Contributor

Hi @suikammd ,

you are right, this looks suspect. It can be worked around by using the same wildcard name (which would typically also be desirable?).

I think a check that the namedWildcard is identical should be established (and throw if not), let me know if you have comments to this?

@henningandersen henningandersen self-assigned this Sep 9, 2019
henningandersen added a commit to henningandersen/elasticsearch that referenced this issue Sep 9, 2019
Registering two different http methods on the same path using different
wildcard names would result in the last wildcard name being active only.
Now throw an exception instead.

Closes elastic#46482
@suikammd
Copy link
Author

suikammd commented Sep 9, 2019

yes, you can avoid these problems by checking the wildcard name conflicts.
I just think the practice in es 5.x is more elegant and reasonable, different methods different tries.

@henningandersen
Copy link
Contributor

Hi @suikammd ,

it relates to #25459 and #24437, where options and error handling was changed to suggest the valid HTTP methods. It also cements that the same resource should be the same resource and use same parameter naming. I think that is a good thing, since using different names could be confusing. The parameter names are documented in the rest-specs (.json) and are likely used in client side code generation.

Unless there are compelling reasons to allow different wildcard-names for the same path, I think this should be left as is (but with the proposed check).

@henningandersen henningandersen added :Core/Infra/REST API REST infrastructure and utilities >non-issue labels Sep 9, 2019
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

@suikammd
Copy link
Author

suikammd commented Sep 9, 2019

thanks, I understand. The current practice seems more restful...

henningandersen added a commit that referenced this issue Sep 9, 2019
Registering two different http methods on the same path using different
wildcard names would result in the last wildcard name being active only.
Now throw an exception instead.

Closes #46482
henningandersen added a commit that referenced this issue Sep 9, 2019
Registering two different http methods on the same path using different
wildcard names would result in the last wildcard name being active only.
Now throw an exception instead.

Closes #46482
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 >non-issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants