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

Add support for URI templates in redirectedUrl() & forwardedUrl() [SPR-14790] #19356

Closed
spring-projects-issues opened this issue Oct 10, 2016 · 5 comments

Comments

@spring-projects-issues
Copy link
Collaborator

@spring-projects-issues spring-projects-issues commented Oct 10, 2016

Nicolai Ehemann opened SPR-14790 and commented

Status Quo

Currently, redirectedUrl() and forwardedUrl() in MockMvcResultMatchers support constant URLs and pattern matching.

However, I think often the redirect or forward URL is dependent on the URL of the controller method (for example, "/item/{id}" forwarding to "/item/{id}/something"). It would be much more readable not to build the redirectedUrl() arguments by string concatenation, but to provide URL template and vars equivalent to the get() or post() calls.

Proposal

So, the proposal is to replace redirectedUrl(expectedUrl) with redirectedUrl(expectedUrl, vars). The default behavior (when omitting the vars argument) would be unchanged.

The implementation is quite trivial, I would be willing to provide a pull request if the proposal is accepted.


Issue Links:

  • #20389 Regression after URI template support added to MockMvcResultMatchers

Referenced from: commits spring-projects/spring-social@1539512

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Oct 13, 2016

Sam Brannen commented

McNetic,

Good idea. Thanks for the proposal. (y)

No need to submit a PR. I'll take care of it.

Cheers,

Sam

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Oct 13, 2016

Sam Brannen commented

Completed as described in GitHub commit a795fd4:

Add support for URI templates in redirectedUrl() & forwardedUrl()

Prior to this commit, the redirectedUrl() and forwardedUrl() methods in
MockMvcResultMatchers supported fully constructed URLs and URLs
containing Ant-style path patterns. However, URI templates were not
supported for these result matchers even though the request path can be
built using URL templates -- for example, via the get() and post()
methods in MockMvcRequestBuilders.

This commit addresses this shortcoming by allowing users to supply a
URL template instead of a fully constructed URL to redirectedUrl() and
forwardedUrl(). To populate the URL template, template variables may be
supplied to redirectedUrl() and forwardedUrl() as var-args.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Nov 22, 2016

Craig Walls commented

As a consequence of the work done on this issue (or so it appears), I now have a failing test that previously was passing.

See https://github.com/spring-projects/spring-social/blob/master/spring-social-web/src/test/java/org/springframework/social/connect/web/ConnectControllerTest.java#L264-L265

It is (and has been) expecting https://someprovider.com/oauth/authorize
client_id=clientId&response_type=code&redirect_uri=http%3A%2F%2Flocalhost%2Fconnect%2Foauth2Provider&state=STATE, but now the percent-signs are being expanded so that the actual URL being asserted is https://someprovider.com/oauth/authorize?client_id=clientId&response_type=code&redirect_uri=http%253A%252F%252Flocalhost%252Fconnect%252Foauth2Provider&state=STATE. And thus, the test is failing.

Please advise on [1] whether this breaking change was intentional (I would think not, since the issue description says "The default behavior (when omitting the vars argument) would be unchanged."; and [2] what the best way would be for me to get my tests in a green state again.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Nov 22, 2016

Rossen Stoyanchev commented

Craig Walls, this is clearly a regression so could could you open a new ticket as this one is permanently closed with M3 out already.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Jul 31, 2017

Rossen Stoyanchev commented

I've created #20389 to fix the regression.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants