Spring Framework
  1. Spring Framework
  2. SPR-6690

unexpected behaviour with AntPatternComparator

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Cannot Reproduce
    • Affects Version/s: 3.0 GA
    • Fix Version/s: None
    • Component/s: None
    • Labels:
      None
    • Last commented by a User:
      true

      Description

      Hi guys,

      the issue only starts from upgrade from 2.5.6 to 3.0.0.

      let's say we have two urlMapping in a SimpleUrlHandlerMapping:

      <property name="urlMap">
         <map>
          <entry key="/**/login.*"><ref local="controller1"/></entry>   
          <entry key="/**/endUser/action/login.*"><ref local="controller2"/></entry>
         </map>
      </property>

      when using the url:
      *http://www.domain.com/context/web/endUser/action/login.html*
      in spring 2.5.6 the AbstractUrlHandletMapping returns the second match as best match:

      String urlPath - org.springframework.web.servlet.handler.AbstractUrlHandlerMapping.lookupHandler(String, HttpServletRequest)

      This is ok, because the count of wildcasts for both entries are equal and the second entry matchs more charachters than the first one.

      This behaviour is changed in spring 3.0.0. A new introduced AntPatternComparator in 3.0.0 is used for this propose:

      org.springframework.util.AntPathMatcher.AntPatternComparator

      In this case the comparator sorts the shorter url mapping (/**/login.*) higher than the longer one (/**/endUser/action/login.*). This results in matching controller1 when using the url:
      *http://www.domain.com/context/web/endUser/action/login.html*
      Which is not correct in my view.

      I presume that a best match is the match with the most matched characters in url path.

        Activity

        Hide
        Arjen Poutsma added a comment -

        Are you sure you're running 3.0 GA and not an earlier milestone or RC? I fixed late in the 3.0 cycle, and cannot reproduce it anymore. The following test, taken from AntPatchMatcherTests, succeeds for me:

        comparator = pathMatcher.getPatternComparator("/web/endUser/action/login.html");
        paths.add("/**/login.*");
        paths.add("/**/endUser/action/login.*");
        Collections.sort(paths, comparator);
        assertEquals("/**/endUser/action/login.*", paths.get(0));
        assertEquals("/**/login.*", paths.get(1));

        In other words, the longer pattern gets sorted before the shorter one.

        Show
        Arjen Poutsma added a comment - Are you sure you're running 3.0 GA and not an earlier milestone or RC? I fixed late in the 3.0 cycle, and cannot reproduce it anymore. The following test, taken from AntPatchMatcherTests, succeeds for me: comparator = pathMatcher.getPatternComparator("/web/endUser/action/login.html"); paths.add("/**/login.*"); paths.add("/**/endUser/action/login.*"); Collections.sort(paths, comparator); assertEquals("/**/endUser/action/login.*", paths.get(0)); assertEquals("/**/login.*", paths.get(1)); In other words, the longer pattern gets sorted before the shorter one.
        Hide
        Nabil Ben Said added a comment -

        Thanks Arjen!

        You are right, but I made also a mistake in the issue description.
        Please replace the second urlMapping entry by the following one in the unit test and you will see how the test fails:

        <entry key="/**/endUser/action/*"><ref local="controller2"/></entry>

        improved unit test:

        comparator = pathMatcher.getPatternComparator("/web/endUser/action/login.html");
        paths.add("/**/login.*");
        paths.add("/**/endUser/action/*");
        Collections.sort(paths, comparator);
        assertEquals("/**/endUser/action/*", paths.get(0));
        assertEquals("/**/login.*", paths.get(1));

        Show
        Nabil Ben Said added a comment - Thanks Arjen! You are right, but I made also a mistake in the issue description. Please replace the second urlMapping entry by the following one in the unit test and you will see how the test fails: <entry key="/**/endUser/action/*"><ref local="controller2"/></entry> improved unit test: comparator = pathMatcher.getPatternComparator("/web/endUser/action/login.html"); paths.add("/**/login.*"); paths.add("/**/endUser/action/*"); Collections.sort(paths, comparator); assertEquals("/**/endUser/action/*", paths.get(0)); assertEquals("/**/login.*", paths.get(1));
        Hide
        Nabil Ben Said added a comment -

        I am using 3.0.0RELEASE.

        Show
        Nabil Ben Said added a comment - I am using 3.0.0RELEASE.
        Hide
        Arjen Poutsma added a comment -

        I see and can reproduce it now.

        Some background info: as part of 3.0, we did change the way path patterns are compared. It used to be a pretty simple length-based comparison, but with the introduction of URI templates, pattern length is not saying much anymore.

        The 3.0 comparison only considers pattern length when the amount of wildcards (*'s as well as URI templates) in the patterns is equal (as can be seen in the AntPathMatcher comparator). When checking the amount of patterns, we ignore trailing .*'s, to make sure that mappings such as /

        {var}

        actually match requests such as /400.html, assigning the value 400 to 'var'. In short: while this is indeed a breaking change, it's there for good reasons.

        Now that I've explained the background for this breaking change, I see two options going forward:

        • I can either add a 'ignoreTrailingWildcards' property to the AntPathMatcher, defaulting it to true, but you could set it to false. This would require you to set up a custom AntPatchMatcher, and inject it into the DefaultAnnotationHandlerMapping and AnnotationMethodHandlerAdapter.
        • You could change your mappings, and make them more explicit. For instance, you could do something like:

        <property name="urlMap">
           <map>
            <entry key="/**/login.*"><ref local="controller1"/></entry>   
            <entry key="/**/endUser/action/login.*"><ref local="controller2"/></entry>
            <entry key="/**/endUser/action/*"><ref local="controller2"/></entry>
           </map>
        </property>

        I am not a big fan of adding (confusing) new properties, so I hope you might consider differentiating your mappings more clearly.

        Show
        Arjen Poutsma added a comment - I see and can reproduce it now. Some background info: as part of 3.0, we did change the way path patterns are compared. It used to be a pretty simple length-based comparison, but with the introduction of URI templates, pattern length is not saying much anymore. The 3.0 comparison only considers pattern length when the amount of wildcards (*'s as well as URI templates) in the patterns is equal (as can be seen in the AntPathMatcher comparator). When checking the amount of patterns, we ignore trailing .*'s, to make sure that mappings such as / {var} actually match requests such as /400.html, assigning the value 400 to 'var'. In short: while this is indeed a breaking change, it's there for good reasons. Now that I've explained the background for this breaking change, I see two options going forward: I can either add a 'ignoreTrailingWildcards' property to the AntPathMatcher, defaulting it to true, but you could set it to false. This would require you to set up a custom AntPatchMatcher, and inject it into the DefaultAnnotationHandlerMapping and AnnotationMethodHandlerAdapter. You could change your mappings, and make them more explicit. For instance, you could do something like: < property name = "urlMap" > < map > < entry key = "/**/login.*" >< ref local = "controller1" /></ entry > < entry key = "/**/endUser/action/login.*" >< ref local = "controller2" /></ entry > < entry key = "/**/endUser/action/*" >< ref local = "controller2" /></ entry > </ map > </ property > I am not a big fan of adding (confusing) new properties, so I hope you might consider differentiating your mappings more clearly.
        Hide
        Nabil Ben Said added a comment -

        Thanks again. I will make my mappings more explicit.
        And perhaps you can ignore trailing wildcards by default in case no URI templates are defined.

        Show
        Nabil Ben Said added a comment - Thanks again. I will make my mappings more explicit. And perhaps you can ignore trailing wildcards by default in case no URI templates are defined.

          People

          • Assignee:
            Arjen Poutsma
            Reporter:
            Nabil Ben Said
            Last updater:
            Trevor Marshall
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:
              Days since last comment:
              5 years, 14 weeks, 4 days ago