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

Rest Controller wildcard registration #46487

Conversation

henningandersen
Copy link
Contributor

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

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
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

Copy link
Member

@dakrone dakrone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM I left one comment about the test

IllegalArgumentException exception = expectThrows(IllegalArgumentException.class,
() -> restController.registerHandler(secondMethod, path + "/{wildcard2}", handler));

assertThat(exception.getMessage(), containsString("wildcard1"));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think since we know the exact exception text that it would be good to check it here, otherwise it's possible a different exception in the future could be thrown but this test wouldn't break.

Something like:

assertThat(exception.getMessage(), equalTo("Trying to use conflicting wildcard names for same path: wildcard1 and wildcard2"))

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 3164595

@henningandersen
Copy link
Contributor Author

Thanks for reviewing @dakrone

@henningandersen
Copy link
Contributor Author

@elasticmachine run elasticsearch-ci/packaging-sample

@henningandersen henningandersen merged commit 39c4b6b into elastic:master Sep 9, 2019
henningandersen added a commit that referenced this pull request 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BUG: use a single trie for all methods
4 participants