diff --git a/grails-doc/src/en/guide/theWebLayer/urlmappings/embeddedVariables.adoc b/grails-doc/src/en/guide/theWebLayer/urlmappings/embeddedVariables.adoc index b6d9a3e3435..e909341ea94 100644 --- a/grails-doc/src/en/guide/theWebLayer/urlmappings/embeddedVariables.adoc +++ b/grails-doc/src/en/guide/theWebLayer/urlmappings/embeddedVariables.adoc @@ -72,6 +72,8 @@ static mappings = { Here the name of the controller, action and id are implicitly obtained from the variables `controller`, `action` and `id` embedded within the URL. +These names come from the URL and from nowhere else. A request parameter named `controller`, `action` or `namespace` does not stand in for a token the URL leaves out: given the mapping above, `/product?action=delete` routes to `ProductController`'s default action, not to `delete`. The parameter is still bound, and is still readable as `params.action`; it just has no say in which action runs. To route on a request parameter, ask for it explicitly with a closure, as shown at the end of this section. + ===== Wildcard Artefact Validation By default, when URL mappings use wildcard variables like `$controller`, `$action`, or `$namespace`, Grails validates the captured values against registered controller artefacts. If a captured value does not correspond to a real controller, action, or namespace, the mapping is skipped and the next mapping is tried. diff --git a/grails-doc/src/en/guide/upgrading/upgrading80x.adoc b/grails-doc/src/en/guide/upgrading/upgrading80x.adoc index fd3bff35dd9..ec44d132bde 100644 --- a/grails-doc/src/en/guide/upgrading/upgrading80x.adoc +++ b/grails-doc/src/en/guide/upgrading/upgrading80x.adoc @@ -2917,3 +2917,65 @@ job is disabled with `jobEnabled = false`, or the class is not a job artefact of An application that caught `NullPointerException` around these calls has to catch those instead. See <>. + +==== 52. URL Mapping Names Come From the URI, Not From Request Parameters + +A mapping that captures its controller, action or namespace from the URL — `"/$controller/$action?/$id?"`, +the default mapping every application starts with — used to resolve those names by reading the request's +parameters during dispatch. The values were normally the ones the URI supplied, because the match wrote its +captures over the request parameters first. But a token the URI did *not* supply left whatever the request +carried under that name in place, and the request's own value decided the route. + +A request for `/article?action=gallery` therefore reached `ArticleController.gallery()` instead of the +controller's default action: the optional `$action` token captured nothing, and the query string filled the +gap. The same held for `controller` and `namespace`, and for a form body as well as a query string. + +Captured names now come from the match itself. A token the URI does not supply is simply absent, and a +request parameter of the same name has no say in routing: + +[source,groovy] +.grails-app/controllers/UrlMappings.groovy +---- +static mappings = { + "/$controller/$action?/$id?"() +} +---- + +[cols="1,1,1", options="header"] +|=== +| Request +| Grails 7 +| Grails 8 + +| `/article/gallery` +| `ArticleController.gallery()` +| `ArticleController.gallery()` + +| `/article?action=gallery` +| `ArticleController.gallery()` +| the default action of `ArticleController` +|=== + +`params.action` still holds `gallery` in both versions — only the routing decision changed. + +Choosing an action from a request parameter is still supported, by asking for it explicitly with a closure: + +[source,groovy] +.grails-app/controllers/UrlMappings.groovy +---- +static mappings = { + "/$controller" { + action = { params.goHere } + } +} +---- + +A closure supplied by the mapping keeps reading request state, exactly as before, and mappings that use one +are unaffected by this change. + +Two consequences beyond the routing fix. Request parameters are now built once per request rather than once +per candidate mapping, since candidates no longer have to be configured onto the request to be identified. +And `grails.web.mapping.UrlMappingInfo` gains a +`isNameResolutionRequestDependent()` method for callers that resolve names themselves; it is a default +method that reports `true`, so an implementation outside the framework keeps its current behaviour without +changes. diff --git a/grails-test-suite-web/src/test/groovy/org/grails/web/mapping/UrlMappingParameterTests.groovy b/grails-test-suite-web/src/test/groovy/org/grails/web/mapping/UrlMappingParameterTests.groovy index 096f28c24b2..1226a210906 100644 --- a/grails-test-suite-web/src/test/groovy/org/grails/web/mapping/UrlMappingParameterTests.groovy +++ b/grails-test-suite-web/src/test/groovy/org/grails/web/mapping/UrlMappingParameterTests.groovy @@ -45,12 +45,12 @@ class UrlMappingParameterTests extends Specification implements UrlMappingsUnitT } void testNotEqual() { - when: + when: 'a request parameter shadows the name the URI captures' webRequest.params.controller = 'foo' def info = urlMappingsHolder.match('/showSomething/bad') - then:'url should not have matched' - info.controllerName == 'foo' + then: 'the notEqual constraint rejects the blog mapping and the URI falls through to the default mapping, whose controller is the one the URI captured rather than the request parameter' + info.controllerName == 'showSomething' when: info = urlMappingsHolder.match('/showSomething/good') diff --git a/grails-web-url-mappings/src/main/groovy/grails/web/mapping/UrlMappingInfo.java b/grails-web-url-mappings/src/main/groovy/grails/web/mapping/UrlMappingInfo.java index 00c9c5ad908..03feb956c1c 100644 --- a/grails-web-url-mappings/src/main/groovy/grails/web/mapping/UrlMappingInfo.java +++ b/grails-web-url-mappings/src/main/groovy/grails/web/mapping/UrlMappingInfo.java @@ -147,4 +147,25 @@ public interface UrlMappingInfo { default boolean hasWildcardCaptures() { return false; } + + /** + * Whether resolving {@link #getControllerName()}, {@link #getActionName()}, + * {@link #getNamespace()} or {@link #getViewName()} depends on the state of the current request, + * and so requires {@link #configure(GrailsWebRequest)} to have run first. + * + *

A mapping such as "/$controller/$action?" captures its names from the URI and can + * answer them from the match alone. A mapping that computes a name with a closure of its own - for + * example "/$controller" { action = { params.goHere } } - reads whatever that closure + * reaches for, typically the parameters of the current request, and a caller that wants the name + * has to configure the request before asking for it.

+ * + *

Implementations that cannot tell should leave this as the default, which asks callers to + * configure the request and preserves the behaviour of every release before this method existed.

+ * + * @return true if the names can only be resolved once the request has been configured + * @since 8.0 + */ + default boolean isNameResolutionRequestDependent() { + return true; + } } diff --git a/grails-web-url-mappings/src/main/groovy/org/grails/web/mapping/AbstractUrlMappingInfo.java b/grails-web-url-mappings/src/main/groovy/org/grails/web/mapping/AbstractUrlMappingInfo.java index e510c1ee66f..a4bb762b455 100644 --- a/grails-web-url-mappings/src/main/groovy/org/grails/web/mapping/AbstractUrlMappingInfo.java +++ b/grails-web-url-mappings/src/main/groovy/org/grails/web/mapping/AbstractUrlMappingInfo.java @@ -120,6 +120,9 @@ protected String evaluateNameForValue(Object value) { if (value instanceof CharSequence) { return value.toString().trim(); } + else if (value instanceof RuntimeConstraintEvaluator) { + return evaluateCapturedName((RuntimeConstraintEvaluator) value); + } else { GrailsWebRequest webRequest = (GrailsWebRequest) RequestContextHolder.getRequestAttributes(); return evaluateNameForValue(value, webRequest); @@ -131,6 +134,10 @@ protected String evaluateNameForValue(Object value, GrailsWebRequest webRequest) return null; } + if (value instanceof RuntimeConstraintEvaluator) { + return evaluateCapturedName((RuntimeConstraintEvaluator) value); + } + String name; if (value instanceof Closure) { Closure callable = (Closure) value; @@ -150,6 +157,20 @@ else if (value instanceof Map) { return name != null ? name.trim() : null; } + /** + * Resolves a name the mapping captured from the URI - the {@code $controller} token of + * {@code "/$controller/$action?"}, for example - from this instance's own parameters, so that the + * answer depends only on the mapping and the URI that matched it and not on what the current + * request happens to carry. + * + * @param evaluator The evaluator held by the mapping for the token + * @return The captured value, or null if the URI did not supply one + */ + private String evaluateCapturedName(RuntimeConstraintEvaluator evaluator) { + Object value = params.get(evaluator.getConstraintName()); + return value != null ? value.toString().trim() : null; + } + /** * The redirect information should be a String or a Map. If it * is a String that string is the URI to redirect to. If it is diff --git a/grails-web-url-mappings/src/main/groovy/org/grails/web/mapping/DefaultUrlMappingInfo.java b/grails-web-url-mappings/src/main/groovy/org/grails/web/mapping/DefaultUrlMappingInfo.java index 5b6b24ff432..959f92f75bc 100644 --- a/grails-web-url-mappings/src/main/groovy/org/grails/web/mapping/DefaultUrlMappingInfo.java +++ b/grails-web-url-mappings/src/main/groovy/org/grails/web/mapping/DefaultUrlMappingInfo.java @@ -212,6 +212,25 @@ public boolean hasWildcardCaptures() { namespace instanceof Closure; } + @Override + public boolean isNameResolutionRequestDependent() { + return isRequestDependent(controllerName) || isRequestDependent(actionName) || + isRequestDependent(namespace) || isRequestDependent(viewName); + } + + /** + * A name captured from the URI is held by this instance, so resolving it needs nothing from the + * request. A name computed by a closure the mapping supplied reads whatever that closure reaches + * for, which is typically the parameters of the current request. A name selected by HTTP method + * reads the method from the request directly, which configuring it does not affect. + * + * @param name The controller, action, namespace or view name held by this instance + * @return true if resolving the name needs the request to have been configured + */ + private static boolean isRequestDependent(Object name) { + return name instanceof Closure && !(name instanceof RuntimeConstraintEvaluator); + } + public String getViewName() { return evaluateNameForValue(viewName); } diff --git a/grails-web-url-mappings/src/main/groovy/org/grails/web/mapping/RegexUrlMapping.java b/grails-web-url-mappings/src/main/groovy/org/grails/web/mapping/RegexUrlMapping.java index d2283f2204a..ba4f3d8b38c 100644 --- a/grails-web-url-mappings/src/main/groovy/org/grails/web/mapping/RegexUrlMapping.java +++ b/grails-web-url-mappings/src/main/groovy/org/grails/web/mapping/RegexUrlMapping.java @@ -34,8 +34,6 @@ import java.util.regex.Pattern; import java.util.regex.PatternSyntaxException; -import groovy.lang.Closure; - import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -732,7 +730,11 @@ else if (viewName != null && controllerName == null) { /** * This method will look for a constraint for the given name and return a closure that when executed will - * attempt to evaluate its value from the bound request parameters at runtime. + * attempt to evaluate its value at runtime. + * + *

The mapping is shared by every request it matches, so the returned evaluator holds only the name + * of the token to resolve. The {@link UrlMappingInfo} produced by a match resolves it against the values + * that match captured.

* * @param name The name of the constrained property * @param constraints The array of current ConstrainedProperty instances @@ -743,15 +745,7 @@ private Object createRuntimeConstraintEvaluator(final String name, ConstrainedPr for (ConstrainedProperty constraint : constraints) { if (constraint.getPropertyName().equals(name)) { - return new Closure(this) { - private static final long serialVersionUID = -2404119898659287216L; - - @Override - public Object call(Object... objects) { - GrailsWebRequest webRequest = (GrailsWebRequest) RequestContextHolder.currentRequestAttributes(); - return webRequest.getParams().get(name); - } - }; + return new RuntimeConstraintEvaluator(this, name); } } return null; diff --git a/grails-web-url-mappings/src/main/groovy/org/grails/web/mapping/RuntimeConstraintEvaluator.java b/grails-web-url-mappings/src/main/groovy/org/grails/web/mapping/RuntimeConstraintEvaluator.java new file mode 100644 index 00000000000..e92005c131a --- /dev/null +++ b/grails-web-url-mappings/src/main/groovy/org/grails/web/mapping/RuntimeConstraintEvaluator.java @@ -0,0 +1,67 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.grails.web.mapping; + +import groovy.lang.Closure; + +import org.springframework.web.context.request.RequestContextHolder; + +import org.grails.web.servlet.mvc.GrailsWebRequest; + +/** + * Resolves the value a URL mapping captured for one of its named tokens - the {@code $controller}, + * {@code $action} or {@code $namespace} segments of a mapping such as + * {@code "/$controller/$action?/$id?"}. + * + *

A {@link RegexUrlMapping} is built once and shared by every request it matches, so an evaluator + * held by the mapping cannot hold the captured value itself. {@link AbstractUrlMappingInfo}, which is + * created per match and does hold it, recognises this type and resolves the name straight from its own + * parameters. That keeps {@link grails.web.mapping.UrlMappingInfo#getControllerName()} and its siblings + * a function of the mapping and the matched URI, rather than of whatever happens to be bound to the + * current thread.

+ * + *

Calling the closure directly falls back to the parameters of the current request, which is all a + * shared instance can do on its own.

+ * + * @since 8.0 + */ +class RuntimeConstraintEvaluator extends Closure { + + private static final long serialVersionUID = -2404119898659287216L; + + private final String constraintName; + + RuntimeConstraintEvaluator(Object owner, String constraintName) { + super(owner); + this.constraintName = constraintName; + } + + /** + * @return the name of the constrained property this evaluator resolves + */ + String getConstraintName() { + return constraintName; + } + + @Override + public Object call(Object... args) { + GrailsWebRequest webRequest = (GrailsWebRequest) RequestContextHolder.currentRequestAttributes(); + return webRequest.getParams().get(constraintName); + } +} diff --git a/grails-web-url-mappings/src/main/groovy/org/grails/web/mapping/mvc/AbstractGrailsControllerUrlMappings.groovy b/grails-web-url-mappings/src/main/groovy/org/grails/web/mapping/mvc/AbstractGrailsControllerUrlMappings.groovy index 211d3000322..1f6f3572e9d 100644 --- a/grails-web-url-mappings/src/main/groovy/org/grails/web/mapping/mvc/AbstractGrailsControllerUrlMappings.groovy +++ b/grails-web-url-mappings/src/main/groovy/org/grails/web/mapping/mvc/AbstractGrailsControllerUrlMappings.groovy @@ -216,7 +216,11 @@ abstract class AbstractGrailsControllerUrlMappings implements UrlMappings { matches.add(info) continue } - if (webRequest != null) { + // Names captured from the URI resolve from the match itself, so the candidate can be + // identified without touching the request. Only a mapping that computes a name from + // request state needs its parameters built first, and only that mapping pays for it - + // the winner's parameters are built once, by UrlMappingsHandlerMapping. + if (webRequest != null && info.isNameResolutionRequestDependent()) { webRequest.resetParams() info.configure(webRequest) } diff --git a/grails-web-url-mappings/src/test/groovy/org/grails/web/mapping/mvc/UrlMappingNameResolutionSpec.groovy b/grails-web-url-mappings/src/test/groovy/org/grails/web/mapping/mvc/UrlMappingNameResolutionSpec.groovy new file mode 100644 index 00000000000..f6939460ec0 --- /dev/null +++ b/grails-web-url-mappings/src/test/groovy/org/grails/web/mapping/mvc/UrlMappingNameResolutionSpec.groovy @@ -0,0 +1,178 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.grails.web.mapping.mvc + +import org.springframework.mock.web.MockHttpServletRequest + +import grails.core.DefaultGrailsApplication +import grails.util.GrailsWebMockUtil +import grails.web.Controller +import grails.web.mapping.AbstractUrlMappingsSpec +import grails.web.mapping.UrlMapping +import grails.web.mapping.UrlMappingInfo +import grails.web.mapping.UrlMappings +import org.grails.web.servlet.mvc.GrailsWebRequest + +/** + * The controller, action and namespace a mapping captures from the URI are a function of the mapping + * and the URI that matched it: they resolve from the match itself, whether or not the request has been + * configured, and a request parameter of the same name does not stand in for a token the URI did not + * supply. A mapping that computes a name with a closure of its own keeps reading request state, which + * is the documented way to route on something other than the URI. + */ +class UrlMappingNameResolutionSpec extends AbstractUrlMappingsSpec { + + void 'resolves controller, action and id captured by a dynamic mapping'() { + when: 'a request matches the default dynamic mapping' + def mappingInfo = dispatch('/article/show/42', { + "/$controller/$action?/$id?"() + }, ArticleController) + + then: 'every name comes from the segment of the URI that captured it' + mappingInfo != null + with(mappingInfo) { + controllerName == 'article' + actionName == 'show' + id == '42' + } + } + + void 'resolves a namespace captured by a dynamic mapping'() { + when: 'a request matches a mapping that captures the namespace' + def mappingInfo = dispatch('/admin/bulletin/list', { + "/$namespace/$controller/$action?"() + }, BulletinController) + + then: 'the namespaced controller is selected' + mappingInfo != null + with(mappingInfo) { + namespace == 'admin' + controllerName == 'bulletin' + actionName == 'list' + } + } + + void 'resolves captured names without configuring the request'() { + given: 'a bound request that has not been configured for any match' + UrlMappings mappingsHolder = createUrlMappingsHolder({ + "/$controller/$action?/$id?"() + }, ArticleController) + GrailsWebMockUtil.bindMockWebRequest() + + when: 'the candidates for a URI are collected' + UrlMappingInfo[] mappingInfos = mappingsHolder.matchAll('/article/show/42', 'GET', UrlMapping.ANY_VERSION) + + then: 'the names are answerable from the match alone' + mappingInfos.length == 1 + with(mappingInfos.first()) { + controllerName == 'article' + actionName == 'show' + !nameResolutionRequestDependent + } + } + + void 'a request parameter does not supply a name the URI did not capture'() { + when: 'a request carries an action parameter and the matched URI has no segment for it' + def mappingInfo = dispatch('/article', [action: 'gallery'], { + "/$controller/$action?"() + }, ArticleController) + + then: 'the mapping routes to the controller with no action, leaving the default action to run' + mappingInfo != null + with(mappingInfo) { + controllerName == 'article' + actionName != 'gallery' + !actionName + } + } + + void 'a name closure reads a parameter the URI captures'() { + when: 'a mapping computes its action from a token the URI captures' + def mappingInfo = dispatch('/articles/gallery', { + "/articles/$section"(controller: 'article') { + action = { params.section } + } + }, ArticleController) + + then: 'the closure sees the captured value and the action resolves through it' + mappingInfo != null + with(mappingInfo) { + controllerName == 'article' + actionName == 'gallery' + nameResolutionRequestDependent + } + } + + void 'a name closure reads a request parameter'() { + when: 'a mapping computes its action from a request parameter, as the guide documents' + def mappingInfo = dispatch('/article', [goHere: 'gallery'], { + "/$controller" { + action = { params.goHere } + } + }, ArticleController) + + then: 'the action comes from the request parameter' + mappingInfo != null + with(mappingInfo) { + controllerName == 'article' + actionName == 'gallery' + } + } + + private UrlMappings createUrlMappingsHolder(Closure mappings, Class... controllerClasses) { + def grailsApplication = new DefaultGrailsApplication(controllerClasses).tap { + initialise() + } + new GrailsControllerUrlMappings(grailsApplication, getUrlMappingsHolder(mappings)) + } + + private UrlMappingInfo dispatch(String requestURI, Closure mappings, Class... controllerClasses) { + dispatch(requestURI, [:], mappings, controllerClasses) + } + + private UrlMappingInfo dispatch(String requestURI, Map parameters, Closure mappings, Class... controllerClasses) { + UrlMappings mappingsHolder = createUrlMappingsHolder(mappings, controllerClasses) + GrailsWebRequest webRequest = GrailsWebMockUtil.bindMockWebRequest() + MockHttpServletRequest request = webRequest.request as MockHttpServletRequest + request.requestURI = requestURI + request.method = 'GET' + parameters.each { String name, String value -> request.addParameter(name, value) } + new UrlMappingsHandlerMapping(mappingsHolder).getHandler(request)?.handler as UrlMappingInfo + } +} + +@Controller +class ArticleController { + + def index() {} + + def gallery() {} + + def show() {} +} + +@Controller +class BulletinController { + + static namespace = 'admin' + + def index() {} + + def list() {} +}