jdaugherty commented on code in PR #16272:
URL: https://github.com/apache/grails-core/pull/16272#discussion_r4126702348
##########
grails-web-url-mappings/src/main/groovy/org/grails/web/mapping/DefaultLinkGenerator.groovy:
##########
@@ -314,93 +406,336 @@ class DefaultLinkGenerator implements LinkGenerator,
PluginManagerAware {
return null
}
- Set<String> namespaces =
getControllerNamespacesByName().get(controller)
- if (namespaces == null || namespaces.isEmpty()) {
- return null
- }
-
- // The normal case: exactly one controller has this name, so use its
namespace (which may be
- // the non-namespaced/default one). Links therefore "just work" from
controller and action
- // alone, with no namespace attribute required.
- if (namespaces.size() == 1) {
- return namespaces.iterator().next()
+ ControllerIndex index = currentControllerIndex()
+ Set<ControllerRef> candidates = index.named(controller)
+ if (candidates.isEmpty()) {
+ // No registered controller has the name, so nothing is nearer
than the namespace the link is
+ // made from: stay in it, as an unqualified reference resolves
against where it is made.
+ return requestStateLookupStrategy.controllerNamespace
}
-
- // Otherwise the same controller name is defined in more than one
namespace - a discouraged
- // design that the caller is expected to disambiguate with an explicit
namespace. Fall back to
- // a sensible default rather than guessing: prefer the non-namespaced
controller when one
- // exists, then a controller in the current request namespace; leave
anything still ambiguous
- // to the existing reverse-mapping default.
- if (namespaces.contains(null)) {
+ ControllerRef nearest = nearestController(candidates, controller,
null, false)
+ if (nearest == null) {
+ reportAmbiguousNamespace(index, controller, candidates)
return null
}
- String currentNamespace =
requestStateLookupStrategy.controllerNamespace
- if (currentNamespace != null && namespaces.contains(currentNamespace))
{
- return currentNamespace
+ return nearest.namespace
+ }
+
+ /**
+ * Warns, once per controller name for the controllers the index holds,
that a link named a controller
+ * defined in several namespaces without saying which, from outside all of
them, so no namespace could be
+ * inferred.
+ */
+ private void reportAmbiguousNamespace(ControllerIndex index, String
controller, Set<ControllerRef> candidates) {
+ if (index.reportedControllerNames.add(controller)) {
+ Set<String> namespaces = new TreeSet<>()
+ for (ControllerRef candidate in candidates) {
+ namespaces.add(candidate.namespace)
+ }
+ log.warn('A link to controller [{}] names no namespace, but the
controller is defined in the namespaces {} and in neither the default namespace
nor the namespace of the current request. No namespace was inferred; pass a
namespace attribute to choose one.',
+ controller, namespaces)
}
- return null
}
- private Map<String, Set<String>> getControllerNamespacesByName() {
+ /**
+ * The registered controllers, indexed for resolving links. {@code
getArtefacts} returns a cached array
+ * that is replaced with a new instance whenever the set of controllers
changes (late registration in
+ * tests, a development-mode reload, or a namespace edit), so comparing
the array identity rebuilds the
+ * index on any such change while staying O(1) on the common path where
nothing changed.
+ */
+ private ControllerIndex currentControllerIndex() {
GrailsApplication application = grailsApplication
if (application == null) {
- return Collections.emptyMap()
+ return ControllerIndex.EMPTY
}
- // getArtefacts returns a cached array that is replaced with a new
instance whenever the set of
- // controllers changes (late registration in tests, a development-mode
reload, or a namespace
- // edit). Comparing the array identity rebuilds the index on any such
change while staying O(1)
- // on the common path where nothing changed.
GrailsClass[] controllers =
application.getArtefacts(ControllerArtefactHandler.TYPE)
- Map<String, Set<String>> index = controllerNamespacesByName
- if (index == null || !controllers.is(cachedControllers)) {
- index = buildControllerNamespaceIndex(controllers)
- controllerNamespacesByName = index
- cachedControllers = controllers
+ MappingContext context = mappingContext
+ ControllerIndex index = controllerIndex
+ if (index == null || !index.isFor(controllers, context)) {
+ index = buildControllerIndex(controllers, context)
+ controllerIndex = index
}
return index
}
+ private ControllerIndex buildControllerIndex(GrailsClass[] controllers,
MappingContext context) {
+ Map<String, Set<ControllerRef>> byName = new HashMap<>()
+ Map<String, Set<ControllerRef>> byDomainClass = new HashMap<>()
+ Map<ControllerRef, Set<String>> actions = new HashMap<>()
+ for (GrailsClass gc in controllers) {
+ GrailsControllerClass controllerClass = (GrailsControllerClass) gc
+ String name = controllerClass.logicalPropertyName
+ if (name == null) {
+ continue
+ }
+ ControllerRef ref = new ControllerRef(name,
controllerClass.namespace)
+ indexUnder(byName, name, ref)
+ Set<String> refActions = actions.get(ref)
+ if (refActions == null) {
+ refActions = new HashSet<>()
+ actions.put(ref, refActions)
+ }
+ refActions.addAll(controllerClass.actions)
+ String domainClassName = context != null ?
domainClassNameFor(controllerClass.clazz, context) : null
+ if (domainClassName != null) {
+ indexUnder(byDomainClass, domainClassName, ref)
+ }
+ }
+ return new ControllerIndex(controllers, context, byName,
byDomainClass, actions)
+ }
+
+ private static void indexUnder(Map<String, Set<ControllerRef>> index,
String key, ControllerRef ref) {
+ Set<ControllerRef> refs = index.get(key)
+ if (refs == null) {
+ refs = new HashSet<>()
+ index.put(key, refs)
+ }
+ refs.add(ref)
+ }
+
+ /**
+ * Clears the cached index of controllers, and with it the ambiguities
reported against them, so it is
+ * rebuilt on next use. The index is also rebuilt whenever the registered
controllers change, as during
+ * a development-mode reload.
+ */
+ void resetControllerNamespaceCache() {
+ controllerIndex = null
+ }
+
+ /**
+ * Resolves the controller a {@code resource} link for the given entity
targets: the nearest controller
+ * serving the domain class, as {@link #nearestController} chooses it. A
controller serves a domain
+ * class when it is named after it, or when it declares it as a generic
type argument, as
+ * {@code PeopleController extends RestfulController<Person>} does, and it
defines the action the link
+ * targets, so a controller declaring the domain class for another
purpose, such as a report, is not
+ * sent links it cannot handle. The link targets the explicit {@code
namespace} when one is given, and
+ * otherwise the request's own namespace.
+ *
+ * <p>When no candidate is unambiguous, the link targets the controller
name the candidates share, if
+ * they share one, and otherwise the domain class name, as before; either
way the namespace is then
+ * inferred for that name. Falling back to the domain class name while
more than one candidate was
+ * equally near is reported, as the link may then target a controller that
does not exist.</p>
+ *
+ * @param entity the domain class being linked to
+ * @param attrs the link attributes, which may carry an explicit {@code
namespace}
+ * @param action the action the link targets, as {@link #resourceAction}
determines it
+ * @return the target controller, and its namespace when the scope chain
found it
+ */
+ private ResourceTarget resolveResourceTarget(PersistentEntity entity, Map
attrs, String action) {
+ String derivedName = entity.getDecapitalizedName()
+ ControllerIndex index = currentControllerIndex()
+ Set<ControllerRef> serving = servingControllers(index, entity,
derivedName, action)
+ if (serving.isEmpty()) {
+ return new ResourceTarget(derivedName, null, false)
+ }
+ boolean explicitNamespace = attrs != null &&
attrs.containsKey(ATTRIBUTE_NAMESPACE)
+ String targetNamespace = explicitNamespace ?
resolveNamespace(derivedName, null, attrs) : null
+ ControllerRef nearest = nearestController(serving, derivedName,
targetNamespace, explicitNamespace)
+ if (nearest != null) {
+ // An explicit namespace is applied by the caller already;
otherwise carry the one found.
+ return new ResourceTarget(nearest.name, nearest.namespace,
!explicitNamespace)
+ }
+ Set<String> names = new HashSet<>()
+ for (ControllerRef ref in serving) {
+ names.add(ref.name)
+ }
+ if (names.size() == 1) {
+ return new ResourceTarget(names.iterator().next(), null, false)
+ }
+ // An explicit namespace with no candidate in it is the caller's
choice rather than an ambiguity.
+ Set<ControllerRef> tied = new HashSet<>()
+ for (ControllerRef ref in serving) {
+ if (!explicitNamespace || Objects.equals(ref.namespace,
targetNamespace)) {
+ tied.add(ref)
+ }
+ }
+ if (tied.size() > 1) {
+ reportAmbiguousResource(index, entity, derivedName, tied)
+ }
+ return new ResourceTarget(derivedName, null, false)
+ }
+
+ /**
+ * Warns, once per domain class for the controllers the index holds, that
a resource link named no
+ * controller and more than one controller serving the domain class was
equally near, so the controller
+ * named after it was assumed.
+ */
+ private void reportAmbiguousResource(ControllerIndex index,
PersistentEntity entity, String derivedName,
+ Set<ControllerRef> tied) {
+ if (index.reportedDomainClasses.add(entity.name)) {
+ Set<String> controllers = new TreeSet<>()
+ for (ControllerRef ref in tied) {
+ controllers.add(ref.namespace != null ?
"${ref.namespace}/${ref.name}".toString() : ref.name)
+ }
+ log.warn('A link to a [{}] names no controller, and the
controllers serving it, {}, are equally near to where it is rendered. The
controller named after the domain class, [{}], was assumed; pass a controller
attribute to choose one.',
+ entity.name, controllers, derivedName)
+ }
+ }
+
+ /**
+ * Chooses, among the controllers a link could target, the one nearest to
where the link is rendered,
+ * trying scopes from the most specific to the least, as code resolves a
name:
+ *
+ * <ol>
+ * <li>the controller handling the current request, if it is a candidate
in the targeted
+ * namespace</li>
+ * <li>the candidate in the targeted namespace</li>
+ * <li>the candidate in the default namespace</li>
+ * <li>the candidate in any namespace</li>
+ * </ol>
+ *
+ * <p>A scope holding more than one candidate chooses the one with the
conventional name, the
+ * controller named after the domain class for a resource link, and is
otherwise ambiguous, so the
+ * next scope is tried rather than guessing. An explicit namespace
confines the choice to that
+ * namespace.</p>
+ *
+ * @param candidates the controllers the link could target
+ * @param conventionalName the name that settles a tie within a scope
+ * @param namespace the namespace the link names, when {@code
explicitNamespace} is set
+ * @param explicitNamespace whether the link names a namespace, rather
than targeting the request's
+ * @return the chosen controller, or {@code null} when no scope settles on
one
+ */
+ private ControllerRef nearestController(Set<ControllerRef> candidates,
String conventionalName,
+ String namespace, boolean
explicitNamespace) {
+ if (!explicitNamespace && candidates.size() == 1) {
+ // Every scope ends at the only candidate, so there is nothing to
choose between.
+ return candidates.iterator().next()
+ }
+ String currentNamespace =
requestStateLookupStrategy.controllerNamespace
+ String targetNamespace = explicitNamespace ? namespace :
currentNamespace
+ String currentController = requestStateLookupStrategy.controllerName
+ if (currentController != null) {
+ ControllerRef current = new ControllerRef(currentController,
currentNamespace)
Review Comment:
**Bug: under the hyphenated URL converter this scope never matches a
multi-word controller name.** With `grails.web.url.converter: hyphenated`,
`requestStateLookupStrategy.controllerName` is the URL form (`tour-desk`),
while `controllerNamespace` stays logical (`backOffice`) and the index holds
logical names (`tourDesk`). `current` therefore never equals a candidate, and
wherever this scope is what should decide, the link or redirect goes to a
different controller.
Repro, with the hyphenated converter and a
`/$namespace/$controller/$action?/$id?(.$format)?` mapping:
```groovy
class CityGuidesController extends RestfulController<TourGuide> { //
default namespace
CityGuidesController() { super(TourGuide) }
}
class TourDeskController extends RestfulController<TourGuide> {
static namespace = 'backOffice'
TourDeskController() { super(TourGuide) }
}
class GuideLedgerController extends RestfulController<TourGuide> {
static namespace = 'backOffice'
GuideLedgerController() { super(TourGuide) }
}
```
| | 8.0.x | this PR |
|---|---|---|
| form POST to `/back-office/tour-desk/save`, `Location` |
`/backOffice/tour-guide/show/2` | `/city-guides/show/2` |
| `createLink(resource: guide, action: 'show')` rendered by
`TourDeskController` | `/tour-guide/show/1` | `/city-guides/show/1` |
| same, rendered by `GuideLedgerController` | `/tour-guide/show/1` |
`/city-guides/show/1` |
With this scope skipped, `backOffice` holds two candidates and neither is
named after the domain class, so the default namespace wins. A user saving on
`TourDeskController` lands on `CityGuidesController`'s show page. Any
multi-word controller that shares a scope with another controller serving the
same domain class is affected.
Matching the request's name against
`grailsUrlConverter.toUrlElement(candidate.name)` as well as the logical name
put all three on `tour-desk` and `guide-ledger` in a local run. Could you fix
this and cover it under the hyphenated converter, in
`LinkGeneratorResourceControllerSpec` and in the `hyphenated` functional app?
Separately, and on 8.0.x as well: the namespace segment is generated in its
logical form (`/backOffice/tour-desk/show/1` after that change). It routes, as
`/back-office/...` does, but a test expecting the hyphenated form will see it.
--
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]
