codeconsole commented on code in PR #16272:
URL: https://github.com/apache/grails-core/pull/16272#discussion_r4128523836
##########
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:
Fixed in 97bbb161c8. The requesting controller now matches a candidate by
its logical name or by that name as the URL converter writes it, in the
resource scope chain and in the current-controller check of
`getDefaultNamespace`, which had the same gap for plugin targets. Covered in
`LinkGeneratorResourceControllerSpec` (your three controllers under
`HyphenatedUrlConverter`, with real reverse mappings) and
`LinkGeneratorNamespaceInferenceSpec`. Your `HyphenatedLinkResolutionSpec` is
in f56bf2d950 and all 7 pass. The namespace segment is unchanged, so it still
expects `/backOffice/`.
##########
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:
Fixed in d5b714c80a. The index records the domain class each controller
serves, and a controller named after the entity that serves a different class
is no longer a candidate. Covered in `LinkGeneratorResourceControllerSpec`, and
the three `Item` rows of your `LinkResolutionSpec` (e8f0a338f2) now pass.
##########
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:
Fixed in 8b871ccd73. A controller serves a domain class only when it is
named after it or extends `RestfulController` parameterised on it: directly,
through a base such as `RestfulServiceController`, or through `@Scaffold` /
`static scaffold`, which the injector compiles to `RestfulController<Domain>`.
Generic bases and traits no longer count. `RestfulController` is matched by
class name, since grails-rest-transforms depends on this module. In your
`LinkResolutionSpec` (e8f0a338f2) the two `GadgetReport` rows now pass. I
changed the row for the report's own page to expect `/gadget/show/{gadget}`,
since the report no longer serves `Gadget`.
--
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]
