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 generated namespace segment stays in
its logical form (`/backOffice/tour-desk/show/1` after that change), while
requests arrive as `/back-office/...`, so a functional test here will run into
that too.
##########
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)
+ if (Objects.equals(current.namespace, targetNamespace) &&
candidates.contains(current)) {
+ return current
+ }
+ }
+ ControllerRef chosen = chooseWithin(candidates, conventionalName,
true, targetNamespace)
+ if (chosen != null || explicitNamespace) {
+ return chosen
+ }
+ if (targetNamespace != null) {
+ chosen = chooseWithin(candidates, conventionalName, true, null)
+ if (chosen != null) {
+ return chosen
+ }
+ }
+ return chooseWithin(candidates, conventionalName, false, null)
+ }
+
+ /**
+ * @return the only candidate in the scope, or failing that the only one
in it with the conventional
+ * name, or {@code null}; the scope is the given namespace, or
every namespace when
+ * {@code inNamespace} is {@code false}
+ */
+ private static ControllerRef chooseWithin(Set<ControllerRef> candidates,
String conventionalName,
+ boolean inNamespace, String
namespace) {
+ ControllerRef only = null
+ ControllerRef conventional = null
+ int count = 0
+ int conventionalCount = 0
+ for (ControllerRef candidate in candidates) {
+ if (inNamespace && !Objects.equals(candidate.namespace,
namespace)) {
+ continue
+ }
+ count++
+ only = candidate
+ if (Objects.equals(candidate.name, conventionalName)) {
+ conventionalCount++
+ conventional = candidate
+ }
+ }
+ if (count == 1) {
+ return only
+ }
+ return conventionalCount == 1 ? conventional : null
+ }
+
/**
- * @return {@code true} if at least one registered controller declares a
namespace. Used by the
- * caching link generator to decide whether a request's namespace context
must be folded into the
- * cache key for link shapes whose target controller it cannot cheaply
resolve (resource links).
+ * @return the controllers named after the entity or declaring it that
define the given action, or
+ * every such controller when the action is not known
*/
- protected boolean hasNamespacedControllers() {
- for (Set<String> namespaces in
getControllerNamespacesByName().values()) {
- for (String namespace in namespaces) {
- if (namespace != null) {
- return true
+ private Set<ControllerRef> servingControllers(ControllerIndex index,
PersistentEntity entity, String derivedName,
+ String action) {
+ String actionElement = action != null && grailsUrlConverter != null ?
grailsUrlConverter.toUrlElement(action) : action
+ Set<ControllerRef> serving = new HashSet<>()
+ for (Set<ControllerRef> candidates in [index.byName.get(derivedName),
index.byDomainClass.get(entity.name)]) {
Review Comment:
**Two domain classes with the same simple name link to each other's
controllers.** The candidates here are the union of the controllers named after
the entity's decapitalized name and the controllers declaring the entity, so a
controller named `item` that declares a *different* `Item` still counts:
```groovy
// namespaces/catalog/ItemController.groovy, default namespace
class ItemController extends RestfulController<namespaces.catalog.Item> {
... }
// namespaces/archive/ItemController.groovy
class ItemController extends RestfulController<namespaces.archive.Item> {
static namespace = 'archive'
...
}
```
| `show` link to | rendered by | 8.0.x and this PR | should be |
|---|---|---|---|
| an `archive.Item` | a default-namespace controller | `/item/show/1` |
`/archive/item/show/1` |
| an `archive.Item` | `catalog.ItemController` | `/item/show/1` |
`/archive/item/show/1` |
| a `catalog.Item` | `archive.ItemController` | `/archive/item/show/1` |
`/item/show/1` |
These don't 404. `/item/show/1` renders the catalog item that happens to
share the id, so the user sees a different record without any error. It isn't
new, but this PR defines the target as the controllers serving the domain
class, and the index already records which class each controller declares.
Could a by-name candidate that declares a different domain class be dropped?
The `scaffolding` example app has this layout (`com.example.User` and
`com.example.community.User`, each with its own `UserController`).
##########
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)
+ if (Objects.equals(current.namespace, targetNamespace) &&
candidates.contains(current)) {
+ return current
+ }
+ }
+ ControllerRef chosen = chooseWithin(candidates, conventionalName,
true, targetNamespace)
+ if (chosen != null || explicitNamespace) {
+ return chosen
+ }
+ if (targetNamespace != null) {
+ chosen = chooseWithin(candidates, conventionalName, true, null)
+ if (chosen != null) {
+ return chosen
+ }
+ }
+ return chooseWithin(candidates, conventionalName, false, null)
+ }
+
+ /**
+ * @return the only candidate in the scope, or failing that the only one
in it with the conventional
+ * name, or {@code null}; the scope is the given namespace, or
every namespace when
+ * {@code inNamespace} is {@code false}
+ */
+ private static ControllerRef chooseWithin(Set<ControllerRef> candidates,
String conventionalName,
+ boolean inNamespace, String
namespace) {
+ ControllerRef only = null
+ ControllerRef conventional = null
+ int count = 0
+ int conventionalCount = 0
+ for (ControllerRef candidate in candidates) {
+ if (inNamespace && !Objects.equals(candidate.namespace,
namespace)) {
+ continue
+ }
+ count++
+ only = candidate
+ if (Objects.equals(candidate.name, conventionalName)) {
+ conventionalCount++
+ conventional = candidate
+ }
+ }
+ if (count == 1) {
+ return only
+ }
+ return conventionalCount == 1 ? conventional : null
+ }
+
/**
- * @return {@code true} if at least one registered controller declares a
namespace. Used by the
- * caching link generator to decide whether a request's namespace context
must be folded into the
- * cache key for link shapes whose target controller it cannot cheaply
resolve (resource links).
+ * @return the controllers named after the entity or declaring it that
define the given action, or
+ * every such controller when the action is not known
*/
- protected boolean hasNamespacedControllers() {
- for (Set<String> namespaces in
getControllerNamespacesByName().values()) {
- for (String namespace in namespaces) {
- if (namespace != null) {
- return true
+ private Set<ControllerRef> servingControllers(ControllerIndex index,
PersistentEntity entity, String derivedName,
+ String action) {
+ String actionElement = action != null && grailsUrlConverter != null ?
grailsUrlConverter.toUrlElement(action) : action
+ Set<ControllerRef> serving = new HashSet<>()
+ for (Set<ControllerRef> candidates in [index.byName.get(derivedName),
index.byDomainClass.get(entity.name)]) {
+ if (candidates == null) {
+ continue
+ }
+ for (ControllerRef candidate in candidates) {
+ if (action == null || index.defines(candidate, action,
actionElement)) {
+ serving.add(candidate)
}
}
}
- return false
+ return serving
}
- private Map<String, Set<String>>
buildControllerNamespaceIndex(GrailsClass[] controllers) {
- Map<String, Set<String>> index = new HashMap<>()
- for (GrailsClass gc in controllers) {
- GrailsControllerClass controllerClass = (GrailsControllerClass) gc
- String name = controllerClass.logicalPropertyName
- if (name == null) {
+ /**
+ * The action a resource link targets, for choosing a controller that
handles it: the action it names,
+ * or else the one its HTTP method maps to, a link naming neither being
followed with a {@code GET}.
+ *
+ * @return the action, or {@code null} for an HTTP method no action maps to
+ */
+ private static String resourceAction(String action, Object
methodAttribute, Object id) {
+ if (truthy(action)) {
+ return action
+ }
+ String method = truthy(methodAttribute) ?
methodAttribute.toString().toUpperCase() : HttpMethod.GET.toString()
+ if (HttpMethod.GET.name().equals(method) && truthy(id)) {
+ method = 'GET_ID'
+ }
+ return REST_RESOURCE_HTTP_METHOD_TO_ACTION_MAP.get(method)
+ }
+
+ /**
+ * Walks a controller's supertypes looking for a generic type argument
that the mapping context
+ * recognises as a persistent entity. Superclasses and interfaces are both
walked, so a domain class
+ * declared by an intermediate base class or by a Groovy trait is still
found, and matching on the
+ * mapping context rather than on a known base type keeps this class free
of any dependency on the
+ * REST controller hierarchy.
+ *
+ * <p>A supertype that declares more than one persistent entity is
ambiguous and is skipped rather
+ * than guessed at, so a base class parameterised on both a parent and a
child resource does not
+ * index the controller under the wrong one.</p>
+ */
+ private String domainClassNameFor(Class<?> controllerClass, MappingContext
context) {
Review Comment:
**A controller that declares the domain class for another purpose takes that
class's links for its whole namespace.** This walk counts any superclass,
interface or trait with a single entity type argument. `nearestController` then
tries the request's namespace before the default one, so such a controller
outranks the controller named after the domain class on every page in its
namespace, not just on its own pages. The action check stops helping as soon as
it has the action.
```groovy
abstract class ReportBase<T> {} //
src/main/groovy
class GadgetReportController extends ReportBase<Gadget> {
static namespace = 'admin'
def show(Long id) { render "Gadget Report ${id}" }
}
class GadgetController extends RestfulController<Gadget> { // default
namespace
GadgetController() { super(Gadget) }
}
```
| from `admin/PageController` | 8.0.x | this PR |
|---|---|---|
| `createLink(resource: gadget, action: 'show')` | `/gadget/show/1` |
`/admin/gadgetReport/show/1` |
| `redirect gadget` | `/admin/gadget/show/1` | `/admin/gadgetReport/show/1` |
8.0.x gets the link right, and nothing flags the change: the report's `show`
receives the gadget's id and renders. A report, export or audit controller
built on a generic base or trait parameterised on a domain class is an ordinary
shape. Could the inference be limited to controllers that actually serve the
resource (`RestfulController`, `@Scaffold`, or an explicit opt-in), or could
the controller named after the domain class win over one that only declares it,
outside the declaring controller's own pages?
--
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]
