Copilot commented on code in PR #15526:
URL: https://github.com/apache/grails-core/pull/15526#discussion_r2981653610
##########
grails-web-url-mappings/src/main/groovy/org/grails/web/mapping/DefaultUrlMappingEvaluator.java:
##########
@@ -418,6 +418,69 @@ public void group(String uri, Closure<?> mappingsClosure) {
}
}
+ /**
+ * Define a group with default mapping parameters that are inherited
by child mappings.
+ * Child mappings can override any default by specifying the parameter
explicitly.
+ *
+ * <p>Example usage:
+ * <pre>
+ * group "/api", namespace: 'api', controller: 'resource', {
+ * "/list"(action: 'list') // inherits namespace and controller
+ * "/show"(action: 'show') // inherits namespace and controller
+ * }
+ * </pre>
+ *
+ * @param defaults Map of default parameters (controller, action,
namespace, plugin, view)
+ * @param uri The URI prefix for the group
+ * @param mappingsClosure The mappings in the group
+ * @since 7.1
+ */
+ public void group(Map<String, Object> defaults, String uri, Closure<?>
mappingsClosure) {
+ try {
+ var parentResource = new ParentResource(null, uri, true, true);
+ parentResources.push(parentResource);
+ var mappingInfo = pushNewMetaMappingInfo();
+ applyGroupDefaults(mappingInfo, defaults);
+ var builder = new UrlGroupMappingRecursionBuilder(this,
parentResource);
+ mappingsClosure.setDelegate(builder);
+ mappingsClosure.setResolveStrategy(Closure.DELEGATE_FIRST);
+ mappingsClosure.call();
+ } finally {
+ mappingInfoDeque.pop();
+ parentResources.pop();
+ }
+ }
+
+ private void applyGroupDefaults(MetaMappingInfo mappingInfo,
Map<String, Object> defaults) {
+ if (defaults == null || defaults.isEmpty()) {
+ return;
+ }
+ // Merge defaults with any inherited group defaults
+ if (mappingInfo.getGroupDefaults() == null) {
+ mappingInfo.setGroupDefaults(new HashMap<>(defaults));
+ } else {
+ mappingInfo.getGroupDefaults().putAll(defaults);
+ }
+ if (defaults.containsKey(CONTROLLER)) {
+ mappingInfo.setController(defaults.get(CONTROLLER));
+ }
+ if (defaults.containsKey(ACTION)) {
+ mappingInfo.setAction(defaults.get(ACTION));
+ }
+ if (defaults.containsKey(NAMESPACE)) {
+ mappingInfo.setNamespace(defaults.get(NAMESPACE));
+ }
+ if (defaults.containsKey(PLUGIN)) {
+ mappingInfo.setPlugin(defaults.get(PLUGIN));
+ }
+ if (defaults.containsKey(VIEW)) {
+ mappingInfo.setView(defaults.get(VIEW));
+ }
+ if (defaults.containsKey(HTTP_METHOD)) {
+
mappingInfo.setHttpMethod(defaults.get(HTTP_METHOD).toString());
+ }
+ }
Review Comment:
`applyGroupDefaults` currently only updates `MetaMappingInfo`. When URL
mappings are evaluated from a Script (binding != null),
controller/action/namespace defaults are resolved from the script Binding (see
`getVariableFromNamedArgsOrBinding`), not from `MetaMappingInfo`, so these
defaults may be ignored in the common `URLMappings.groovy` path. To make group
defaults effective for Script-based mappings, either (1) push defaults into the
Binding for the duration of the group closure (and restore afterward), or (2)
update the variable resolution logic to fall back to the `defaultValue` when
the Binding doesn't define the key.
##########
grails-web-url-mappings/src/main/groovy/org/grails/web/mapping/DefaultUrlMappingEvaluator.java:
##########
@@ -1282,5 +1351,13 @@ public void group(String uri, Closure<?> mappings) {
}
super.group(uri, mappings);
}
+
+ @Override
+ public void group(Map<String, Object> defaults, String uri, Closure<?>
mappings) {
+ if (parentResource != null) {
+ uri = parentResource.uri.concat(uri);
+ }
+ super.group(defaults, uri, mappings);
+ }
Review Comment:
This override has the same argument-order issue as the parent overload. For
nested groups, Groovy will call `group(String, Map, Closure)` when using `group
"/x", controller:'c', { ... }`, but this method only matches `group(Map,
String, Closure)`, causing nested group defaults to fail at runtime.
```suggestion
}
@Override
public void group(String uri, Map<String, Object> defaults,
Closure<?> mappings) {
if (parentResource != null) {
uri = parentResource.uri.concat(uri);
}
super.group(uri, defaults, mappings);
}
```
##########
grails-web-url-mappings/src/test/groovy/org/grails/web/mapping/GroupDefaultsSpec.groovy:
##########
@@ -0,0 +1,130 @@
+/*
+ * 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 grails.util.GrailsWebMockUtil
+import grails.web.mapping.AbstractUrlMappingsSpec
+import org.springframework.web.context.request.RequestContextHolder
+
+class GroupDefaultsSpec extends AbstractUrlMappingsSpec {
+
+ void "group defaults are inherited by child mappings"() {
+ given:
+ def holder = getUrlMappingsHolder {
+ group "/api", namespace: 'api', controller: 'resource', {
+ "/list"(action: 'list')
+ "/show"(action: 'show')
+ }
+ }
Review Comment:
These tests exercise `DefaultUrlMappingEvaluator.evaluateMappings(Closure)`
(binding == null). Since production `URLMappings.groovy` evaluation typically
goes through the Script/binding path (`evaluateMappings(Resource/Class)`),
consider adding at least one test that evaluates a Script-based mappings
resource/class and verifies group defaults inheritance there as well (otherwise
the feature may pass tests but fail in real apps).
##########
grails-web-url-mappings/src/main/groovy/org/grails/web/mapping/DefaultUrlMappingEvaluator.java:
##########
@@ -418,6 +418,69 @@ public void group(String uri, Closure<?> mappingsClosure) {
}
}
+ /**
+ * Define a group with default mapping parameters that are inherited
by child mappings.
+ * Child mappings can override any default by specifying the parameter
explicitly.
+ *
+ * <p>Example usage:
+ * <pre>
+ * group "/api", namespace: 'api', controller: 'resource', {
+ * "/list"(action: 'list') // inherits namespace and controller
+ * "/show"(action: 'show') // inherits namespace and controller
+ * }
+ * </pre>
+ *
+ * @param defaults Map of default parameters (controller, action,
namespace, plugin, view)
Review Comment:
The Javadoc for `defaults` lists supported keys as `(controller, action,
namespace, plugin, view)`, but the implementation also applies
`method`/`HTTP_METHOD`. Please update the Javadoc so it accurately reflects
supported defaults.
```suggestion
* @param defaults Map of default parameters (controller, action,
namespace, plugin, view, method/HTTP_METHOD)
```
##########
grails-web-url-mappings/src/main/groovy/org/grails/web/mapping/DefaultUrlMappingEvaluator.java:
##########
@@ -418,6 +418,69 @@ public void group(String uri, Closure<?> mappingsClosure) {
}
}
+ /**
+ * Define a group with default mapping parameters that are inherited
by child mappings.
+ * Child mappings can override any default by specifying the parameter
explicitly.
+ *
+ * <p>Example usage:
+ * <pre>
+ * group "/api", namespace: 'api', controller: 'resource', {
+ * "/list"(action: 'list') // inherits namespace and controller
+ * "/show"(action: 'show') // inherits namespace and controller
+ * }
+ * </pre>
+ *
+ * @param defaults Map of default parameters (controller, action,
namespace, plugin, view)
+ * @param uri The URI prefix for the group
+ * @param mappingsClosure The mappings in the group
+ * @since 7.1
+ */
+ public void group(Map<String, Object> defaults, String uri, Closure<?>
mappingsClosure) {
+ try {
+ var parentResource = new ParentResource(null, uri, true, true);
+ parentResources.push(parentResource);
+ var mappingInfo = pushNewMetaMappingInfo();
+ applyGroupDefaults(mappingInfo, defaults);
+ var builder = new UrlGroupMappingRecursionBuilder(this,
parentResource);
+ mappingsClosure.setDelegate(builder);
+ mappingsClosure.setResolveStrategy(Closure.DELEGATE_FIRST);
+ mappingsClosure.call();
Review Comment:
The parameter order of this overload doesn't match the DSL call style shown
in the PR description/tests (`group "/api", namespace: ..., { ... }`), which
Groovy desugars to `group(String, Map, Closure)`. With the current signature
(`group(Map, String, Closure)`), this will result in a MissingMethodException
at runtime. Consider changing the overload to `group(String uri,
Map<String,Object> defaults, Closure<?> mappingsClosure)` (and adjust the
recursion builder override accordingly).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
