UrlMappings - use non-greedy matching for URL paths. Fixes #9849

Previously Grails would use greedy matches between paths to math up until each slash. This causes problem with overlapping URL mappings (see #9849). This changes makes paths non-greedy and groups for the format extension mapping into a single optional group, thus fixing the problem.

Author: graemerocher

grails-web-url-mappings/src/main/groovy/org/grails/web/mapping/RegexUrlMapping.java

@@ -74,6 +74,7 @@
 @SuppressWarnings("rawtypes")
 public class RegexUrlMapping extends AbstractUrlMapping {
 
+    public static final String FORMAT_PARAMETER = "format";
     private Pattern[] patterns;
     private Map<Integer, List<Pattern>> patternByTokenCount = new HashMap<Integer, List<Pattern>>();
     private UrlMappingData urlData;
@@ -251,9 +252,9 @@ protected Pattern convertToRegex(String url) {
 
             // Now replace "*" with "[^/]" and "**" with ".*".
             pattern = "^" + urlRoot
-                    .replace("(\\.(*))", "\\.?([^/]+)?")
-                    .replaceAll("([^\\*])\\*([^\\*])", "$1[^/]+$2")
-                    .replaceAll("([^\\*])\\*$", "$1[^/]+")
+                    .replace("(\\.(*))", "(\\.[^/]+)?")
+                    .replaceAll("([^\\*])\\*([^\\*])", "$1[^/]+?$2")
+                    .replaceAll("([^\\*])\\*$", "$1[^/]+?")
                     .replaceAll("\\*\\*", ".*");
 
             if("/(*)(\\.(*))".equals(urlEnd)) {
@@ -263,12 +264,12 @@ protected Pattern convertToRegex(String url) {
                 pattern += "/([^/]+)\\.([^/.]+)?";
             } else {
                 pattern += urlEnd
-                        .replace("(\\.(*))", "\\.?([^/]+)?")
-                        .replaceAll("([^\\*])\\*([^\\*])", "$1[^/]+$2")
-                        .replaceAll("([^\\*])\\*$", "$1[^/]+")
+                        .replace("(\\.(*))", "(\\.[^/]+)?")
+                        .replaceAll("([^\\*])\\*([^\\*])", "$1[^/]+?$2")
+                        .replaceAll("([^\\*])\\*$", "$1[^/]+?")
                         .replaceAll("\\*\\*", ".*")
-                        .replaceAll("\\(\\[\\^\\/\\]\\+\\)\\\\\\.", "([^/.]+)\\\\.")
-                        .replaceAll("\\(\\[\\^\\/\\]\\+\\)\\?\\\\\\.", "([^/.]+)\\?\\\\.")
+                        .replaceAll("\\(\\[\\^\\/\\]\\+\\)\\\\\\.", "([^/.]+?)\\\\.")
+                        .replaceAll("\\(\\[\\^\\/\\]\\+\\)\\?\\\\\\.", "([^/.]+?)\\?\\\\.")
                 ;
             }
             pattern += "/??$";
@@ -634,7 +635,13 @@ private UrlMappingInfo createUrlMappingInfo(String uri, Matcher m) {
                     return null;
                 }
 
-                params.put(cp.getPropertyName(), lastGroup);
+                String propertyName = cp.getPropertyName();
+                if(lastGroup != null) {
+                    if(FORMAT_PARAMETER.equals(propertyName) && lastGroup.startsWith(".")) {
+                        lastGroup = lastGroup.substring(1);
+                    }
+                }
+                params.put(propertyName, lastGroup);
                 break;
             }
             else {
@@ -651,7 +658,11 @@ private UrlMappingInfo createUrlMappingInfo(String uri, Matcher m) {
                         return null;
                     }
 
-                    params.put(cp.getPropertyName(), lastGroup);
+                    String propertyName = cp.getPropertyName();
+                    if(FORMAT_PARAMETER.equals(propertyName) && lastGroup.startsWith(".")) {
+                        lastGroup = lastGroup.substring(1);
+                    }
+                    params.put(propertyName, lastGroup);
                 }
             }
 

grails-web-url-mappings/src/test/groovy/org/codehaus/groovy/grails/web/mapping/RestfulResourceMappingSpec.groovy

@@ -19,6 +19,63 @@ import spock.lang.Specification
  * @author Graeme Rocher
  */
 class RestfulResourceMappingSpec extends Specification{
+    @Issue('https://github.com/grails/grails-core/issues/9849')
+    void "Test conflicting UrlMappings related to a resource mappings"() {
+        given:"A URL mappings definition with a single resource"
+        def urlMappingsHolder = getUrlMappingsHolder {
+            "/api/worksheet"(resources:"worksheet")
+            "/api/work"(resources:"work")
+        }
+
+        when:"The URLs are obtained"
+        def urlMappings = urlMappingsHolder.urlMappings
+
+        then:"There are correct of them in total"
+        urlMappings.size() == 16
+
+        expect:"That the appropriate URLs are matched for the appropriate HTTP methods"
+        urlMappingsHolder.matchAll('/api/work', 'GET')
+        urlMappingsHolder.matchAll('/api/work', 'GET')[0].controllerName == 'work'
+
+        urlMappingsHolder.matchAll('/api/work.xml', 'GET')
+        urlMappingsHolder.matchAll('/api/work.xml', 'GET')[0].controllerName == 'work'
+        urlMappingsHolder.matchAll('/api/work.xml', 'GET')[0].parameters.format == 'xml'
+
+        urlMappingsHolder.matchAll('/api/worksheet', 'GET')
+        urlMappingsHolder.matchAll('/api/worksheet', 'GET')[0].controllerName == 'worksheet'
+
+        urlMappingsHolder.matchAll('/api/worksheet.xml', 'GET')
+        urlMappingsHolder.matchAll('/api/worksheet.xml', 'GET')[0].controllerName == 'worksheet'
+        urlMappingsHolder.matchAll('/api/worksheet.xml', 'GET')[0].parameters.format == 'xml'
+
+    }
+
+    @Issue('https://github.com/grails/grails-core/issues/9877')
+    void "Test conflicting UrlMappings with default url mapping"() {
+        given:"A URL mappings definition with a single resource"
+        def urlMappingsHolder = getUrlMappingsHolder {
+            "/$controller/$action?/$id?(.$format)?"{
+                constraints {
+                    // apply constraints here
+                }
+            }
+
+            "/foo"(resources: 'foo')
+        }
+
+        when:"The URLs are obtained"
+        def urlMappings = urlMappingsHolder.urlMappings
+
+        then:"There are correct of them in total"
+        urlMappings.size() == 9
+
+        expect:"That the appropriate URLs are matched for the appropriate HTTP methods"
+        urlMappingsHolder.matchAll('/foo', 'GET')
+        urlMappingsHolder.matchAll('/foo', 'GET')[0].controllerName == 'foo'
+
+        urlMappingsHolder.matchAll('/fooBar', 'GET')[0].parameters.controller == 'fooBar'
+        urlMappingsHolder.matchAll('/fooWithAnythingAfterIt', 'GET')[0].parameters.controller == 'fooWithAnythingAfterIt'
+    }
 
     void "Test resource members"() {
         given: