REOPENED -PathVariable mappings are greedy over hard coded mappings [SPR-5924]

[spring-projects-issues]

Eric Bottard opened SPR-5924 and commented

Hard coded request mapping values should take precedence over path variables. Wild card patterns in a path are currently inferior to explicit values. Path variables should be applied after explicit paths and before wild cards.

For example:

@RequestMapping(value = "/resources/new/", method = RequestMethod.GET)
is currently trumped by
@RequestMapping(value = "/resources/{resourceName}/", method = RequestMethod.GET)

@RequestMapping(value = "/resources/new/", method = RequestMethod.GET)
currently trumps
@RequestMapping(value = "/resources/*/", method = RequestMethod.GET)

@RequestMapping(value = "/resources/new/", method = RequestMethod.GET)
should trump
@RequestMapping(value = "/resources/{resourceName}/", method = RequestMethod.GET)
should trump
@RequestMapping(value = "/resources/*/", method = RequestMethod.GET)

---

Affects: 3.0 M1

Issue Links:

• @RequestMapping best match gives wrong Controller method when using wildcards [SPR-5362] #10035 @RequestMapping best match gives wrong Controller method when using wildcards ("is duplicated by")
• PathVariable mappings are greedy over hard coded mappings [SPR-5367] #10040 PathVariable mappings are greedy over hard coded mappings
• @Controller method order effects @RequestMapping behavior in ways not expected [SPR-5731] #10401 @Controller method order effects @RequestMapping behavior in ways not expected

Referenced from: commits 8073efd, abfc479

[spring-projects-issues]

Eric Bottard commented

This is issue #10040, which I could not reopen properly, sorry if this is not the worrect way to do this.

Anyway, it seems that the correction you commited (https://fisheye.springsource.org/changelog/spring-framework/?cs=563) for this does not cover all cases, and indeed the ROO created controllers I get in my project don't work.

When there is "/some/form" and "/some/{id}", nowhere in the RequestMappingInfoComparator do you favor the "/form" path over the one with @PathVariables. The behaviour is thus undefined.

Please review and correct me if I'm wrong.

Also, the fact that the comparator may eventually return 0 is a code smell in my opinion, because the developper has no idea of the fact that there is some ambiguity.

[spring-projects-issues]

Arjen Poutsma commented

You cannot reopen that issue, because you are not the reporter of it. So creating a new issue like you did is the correct this to do :).

The trick is that the RequestMappingInfoComparator delegates to the PathComparator (created via AntPathMatcher.getPatternComparator()) to do the pattern comparison. And that pattern comparator does put patterns without uri templates before those without (see the tests). Only when the patterns are considered equal, then the RMIC kicks in (checking params, headers, HTTP methods, etc).

Can you give me a test case or code snippet which shows the problems you are having?

[spring-projects-issues]

Eric Bottard commented

Hmmm, since you made a typo in your previous comment, I'm not sure what you meant (2x 'without') :
And that pattern comparator does put patterns without uri templates before those without
I assume you meant without / with, which is what is implied by this copied bug description.

Anyway, I think I found the source of the problem. Seems like the ugly copy/paste beast stroke twice in AntPathMatcher :

int bracketCount1 = StringUtils.countOccurrencesOf(pattern1, "{");
int bracketCount2 = StringUtils.countOccurrencesOf(pattern1, "{"); <----- here, should be pattern1
if (bracketCount1 < bracketCount2) {
return -1;
}
else if (bracketCount1 < bracketCount2) { <----- here, should be ">"
return 1;
}

I'm using Spring 3.0.0M3

A testcase is easy to reproduce : just create some automatic Roo controller (my command is of class "Question" if that may have any impact on method ordering)

[spring-projects-issues]

Eric Bottard commented

Damn, I got fooled myself.
Please read "here, should be pattern2"

[spring-projects-issues]

Arjen Poutsma commented

Ohhh, cr*p. Fixing that typo right now.

Thanks for spotting this! Four eyes see more than two :).

Can you try a recent snapshot (as of tonight) and try again?

[spring-projects-issues]

Arjen Poutsma commented

One more thing: not sure about that second part of your fix, the

else if (bracketCount1 < bracketCount2) { <----- here, should be ">" return 1; }

part.

The idea is that patterns with two brackets (uri templates) end up before those with one, because they are more specific.

[spring-projects-issues]

Eric Bottard commented

Copy/Paste is the root of all evil I tell you.

Trying a recent snapshot is cumbersome for me, but I copy/pasted the class and made the change myself, making sure my version of the matcher got picked. And sure enough, the problem goes away...

Glad you fixed the issue so quicky, thank you.

[spring-projects-issues]

Eric Bottard commented

One more thing: not sure about that second part of your fix, the

else if (bracketCount1 < bracketCount2) { <----- here, should be ">" return 1; }

part.

The idea is that patterns with two brackets (uri templates) end up before those with one, because they are more specific.

Sure enough, but here again, there seems to be a copy/paste issue : the test is exactly the same as 2 lines before (ie you forgot to switch bC1 and bC2)

[spring-projects-issues]

Arjen Poutsma commented

Jeez. Good thing I'm going on a holiday tomorrow! ;)

Fixed that too. Thanks!