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

Regression: UrlPathHelper.getLookupPathForRequest(…) returns different results for URIs containing matrix parameters [SPR-13455] #18035

Closed
spring-projects-issues opened this issue Sep 9, 2015 · 5 comments
Assignees
Labels
type: bug
Milestone

Comments

@spring-projects-issues
Copy link
Collaborator

@spring-projects-issues spring-projects-issues commented Sep 9, 2015

Oliver Drotbohm opened SPR-13455 and commented

On Spring 4.1.7, calling UrlPathHelper.getLookupPathForRequest(…) on /books/;test/1 returns /books/1, i.e. it eliminates double slashes that result from the removal of the matrix parameter.

Spring 4.2.1 returns /books//1 for the same operation, which might cause issues for downstream code not expecting the double slashes.

A brief debugging session unveiled getPathWithinServletMapping(…) to not clean up the path correctly anymore. 4.2 has introduced new code (for sanitization), which results in the the servletPath not being returned anymore. The newly introduced code calls getRemainingPath(…) which results in an empty string, which lets getLookupPathForRequest(…) fall back to getPathWithinApplication(…) which it didn't have to before.


Affects: 4.2.1

Issue Links:

  • DATAREST-674 Guard against regression in UrlPathHelper in Spring 4.2

Referenced from: commits spring-projects/spring-data-rest@76380eb, spring-projects/spring-data-rest@465bcdc, spring-projects/spring-data-rest@619511d

0 votes, 7 watchers

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Sep 9, 2015

Rossen Stoyanchev commented

Pasting example test from chat:

@Test
public void test() throws UnsupportedEncodingException {
	UrlPathHelperhelper = new UrlPathHelper();
	helper.setRemoveSemicolonContent(true);
	MockHttpServletRequest request = new MockHttpServletRequest();
	request.setRequestURI("/books/;test/1");
	assertEquals("/books/1", helper.getRequestUri(request));
}

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Oct 6, 2015

Brian Clozel commented

Hi Oliver Drotbohm, I'm trying to tackle this issue and I'm probably missing some background here.

You issue description seems to point to this commit, although I can't seem to reproduce the exact code execution path you're describing.

It seems you've discussed this issue with Rossen and came up with another way to reproduce this - but I don't see any link between this example in the comments section and the issue described here.

In the meantime, I've prepared a commit that fixes the latter, but this may be rather naïve, not knowing what behavior changed nor how to reproduce this issue in the first place.

Thanks!

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Oct 7, 2015

Oliver Drotbohm commented

I've looked into the original poster's test cases and am a bit puzzled. He's running integration tests against Tomcat and I can see the following intermediate values in getPathWithinServletMapping(…) on Spring 4.1.7 (Boot 1.2.5):

  • pathWithinApp -> /books//1
  • servletPath -> /books/1
  • path -> null

All other variables turn out to be null and thus, servletPath is returned. If I try the same test with a MockMvcServletRequest (like in the example above), servletPath path is an empty string, thus, path becomes /books//1 and is thus immediately returned as is, which makes the tests fail on 4.1.7 even (but due to the fact the "real" request is behaving differently than the mock).

On Spring 4.2 (Boot 1.3) however, the integration test results in an empty String being returned from getPathWithinServletMapping(…):

  • pathWithinApp -> /books//1
  • servletPath -> /books/1
  • sanitizedPathWithinApp -> /books/1

getRemainingPath(…) then returns an empty String which is why the next if-clause returns exactly that from the method. getLookupPathFromRequest(…) then falls back to getPathWithinApplication(…) which doesn't apply any double-slash removal.

So if you tweak the test case more towards what happens on Tomcat, if fails on Spring 4.2.1 but succeeds on 4.1.7:

UrlPathHelper helper = new UrlPathHelper();
MockHttpServletRequest request = new MockHttpServletRequest();
request.setServletPath("/books/1");
request.setRequestURI("/books/;test/1");

assertThat(helper.getLookupPathForRequest(request), is("/books/1"));

Does that help?

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Oct 8, 2015

Brian Clozel commented

Thanks, that definitely helped.
This is fixed now in master.

Rossen Stoyanchev, I think you wanted to take a look at this. I ended up removing duplicate slashes in the lookup path, as it also makes its behavior more consistent and simple (see previous bug reports like #16979).

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Oct 8, 2015

Oliver Drotbohm commented

Using the latest snapshots the test case works for me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug
Projects
None yet
Development

No branches or pull requests

2 participants