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

validate different wildcards only if request method is the same #1583

Merged
merged 3 commits into from Mar 14, 2018

Conversation

@kovetskiy
Copy link

@kovetskiy kovetskiy commented Mar 12, 2018

Hi. I noticed that goa design doesn't allow to use different wildcards at the same position for different methods. It's reasonable only if we check wildcard positions in URLs with the same HTTP method. But it's not reasonable to make this check if http methods are different.
My error was:

resource "decisions" action "list": route "/api/v1/decisions/:entity/:entityID/" conflicts with route "/api/v1/decisions/:decisionID" of decisions action delete. Make sure wildcards at the same positions have the same name. Conflicting wildcards are "entity" from route GET "/:entity/:entityID/" of resource "decisions" action "list" and "decisionID" from route DELETE "/:decisionID" of resource "decisions" action "delete".

So DELETE "/:decisionID" was conflicting with GET "/:entity/:entityID/" but there are no really possible conflicts because http methods are different and mux can handle both without any problem.

I made a patch and tested it, works just fine.

@kovetskiy
Copy link
Author

@kovetskiy kovetskiy commented Mar 12, 2018

@raphael
Copy link
Member

@raphael raphael commented Mar 12, 2018

This is great, thank you. Could I convince you to add a test that would have failed before the fix but doesn't with the PR applied? Thank you!

@kovetskiy kovetskiy force-pushed the kovetskiy:master branch 2 times, most recently from b7f5801 to 6060c2f Mar 14, 2018
@kovetskiy kovetskiy force-pushed the kovetskiy:master branch from 6060c2f to 65aa2ce Mar 14, 2018
@kovetskiy
Copy link
Author

@kovetskiy kovetskiy commented Mar 14, 2018

@raphael I have added test that fails without my fixes and passes with my fixes.

@raphael
Copy link
Member

@raphael raphael commented Mar 14, 2018

Looks great, thank you!

@raphael
Copy link
Member

@raphael raphael commented Mar 14, 2018

One last thing I promise :) could you backport the fix to the v1 branch? Simply cherry pick the merge commit and create a new PR targeting the v1 branch. Thank you!

@raphael raphael merged commit 64d207b into goadesign:master Mar 14, 2018
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
tchssk added a commit to tchssk/goa that referenced this pull request Mar 29, 2018
…esign#1583)

* test routes validation with different http methods

* validate different wildcards only if request method is the same

* reduce design/validation code complexity
raphael added a commit that referenced this pull request Mar 29, 2018
… (#1639)

* test routes validation with different http methods

* validate different wildcards only if request method is the same

* reduce design/validation code complexity
@jiekang jiekang mentioned this pull request Jul 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants