URL mapping group defaults support#15526
Conversation
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Pull request overview
Adds support for group-level default mapping parameters in the URL mappings DSL, so child mappings can inherit controller/namespace/etc. from a surrounding group(...) and override them selectively.
Changes:
- Introduces a new
groupoverload intended to accept default mapping parameters and apply them to child mappings. - Extends
MetaMappingInfowith agroupDefaultsmap to support inheritance across nested groups/mappings. - Adds documentation and a new Spock spec covering inheritance/override behavior.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
grails-web-url-mappings/src/test/groovy/org/grails/web/mapping/GroupDefaultsSpec.groovy |
Adds tests for group defaults inheritance/override and nested group inheritance. |
grails-web-url-mappings/src/main/groovy/org/grails/web/mapping/MetaMappingInfo.groovy |
Stores groupDefaults on mapping metadata to enable inheritance. |
grails-web-url-mappings/src/main/groovy/org/grails/web/mapping/DefaultUrlMappingEvaluator.java |
Implements group-defaults support and inheritance during mapping evaluation. |
grails-doc/src/en/guide/theWebLayer/urlmappings/mappingToControllersAndActions.adoc |
Documents the new “Group Defaults” feature with examples and supported keys. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| uri = parentResource.uri.concat(uri); | ||
| } | ||
| super.group(defaults, uri, mappings); | ||
| } |
There was a problem hiding this 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.
| } | |
| } | |
| @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); | |
| } |
There was a problem hiding this comment.
@copilot Having a Map<String, Object> default first as arguments makes it a Named argument collector.
| 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()); | ||
| } | ||
| } |
There was a problem hiding this 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
Outdated
Show resolved
Hide resolved
| void "group defaults are inherited by child mappings"() { | ||
| given: | ||
| def holder = getUrlMappingsHolder { | ||
| group "/api", namespace: 'api', controller: 'resource', { | ||
| "/list"(action: 'list') | ||
| "/show"(action: 'show') | ||
| } | ||
| } |
There was a problem hiding this 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).
| 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(); |
There was a problem hiding this 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).
grails-web-url-mappings/src/main/groovy/org/grails/web/mapping/DefaultUrlMappingEvaluator.java
Outdated
Show resolved
Hide resolved
| uri = parentResource.uri.concat(uri); | ||
| } | ||
| super.group(defaults, uri, mappings); | ||
| } |
There was a problem hiding this comment.
@copilot Having a Map<String, Object> default first as arguments makes it a Named argument collector.
This comment has been minimized.
This comment has been minimized.
✅ All tests passed ✅Test Summary
🏷️ Commit: 37658a2 Learn more about TestLens at testlens.app. |
matrei
left a comment
There was a problem hiding this comment.
Looks solid and very useful, thank you @codeconsole !
Dramatically simplifies group url mappings by allowing parameter inheritance.
e.g.
you can even do