-
Notifications
You must be signed in to change notification settings - Fork 950
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
"collection" url mapping inside a "group" behaves the same as it is not in a group. Fixes #10844 #10922
Conversation
…ot in a group. Fixes #10844
Does the original test pass with your changes? It's better to create a new test that covers the behavior or just edit the test just enough to get it to pass. |
} | ||
} | ||
} | ||
} | ||
|
||
expect: | ||
linkGenerator.link(controller:'bar', action:'baz', params:[barId:1]) == 'http://localhost/g/bars/1/baz' | ||
linkGenerator.link(controller: 'employee1', action: 'search1', method: 'GET', params: [userId: 1]) == 'http://localhost/api1/employees/search?userId=1' | ||
linkGenerator.link(controller: 'employee2', action: 'search2', method: 'GET', params: [userId: 1]) == 'http://localhost/api2/employees/search?userId=1' | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps add a test with employeeId
to verify it puts it in the URL?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's not possible. According to http://docs.grails.org/3.3.2/guide/theWebLayer.html#restfulMappings in Nested Resources
section, that mapping is not created automatically.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I give it a second thought and also read the documentation twice and I've push a commit.
No, the original test doesn't pass because it was a bug and this change is a breaking change, so I though it was better to change the failing test with the new behavior and submit the PR to |
|
Fixes #10844