Support controller attribute in tags with resource attribute and resolve correct identity Fixes #13627 #13628

codeconsole · 2024-09-07T23:07:51Z

Fixes the following

<g:link method="GET" resource="user" controller="userAdmin"></g:link>

which should link to /userAdmin/show/1

but instead renders /user/show/1

Also fixes

class Sample {
    Long uniqueId
    static mapping {
        id: name: 'uniqueId'
    }
}

Currently (bug) grails creates a link to /sample/index for this class instead of /sample/${sample.indent()}/show because it does not have an id property and uses a static id mapping.

Design Assumptions

If a resource is a PersistentEntity or a Domain Artifact an attempt to call ident() is made, otherwise a MethodMissing exception occurs and a fallback to the id property is used.

Reason for Incorrect Behavior

The following code is where it is problematic
https://github.com/grails/grails-core/blob/da8668daa13c9c1b2157535e11986f49615e8a4b/grails-web-url-mappings/src/main/groovy/org/grails/web/mapping/DefaultLinkGenerator.groovy#L174-L189

and is correctly handled here:
https://github.com/grails/grails-core/blob/da8668daa13c9c1b2157535e11986f49615e8a4b/grails-web-url-mappings/src/main/groovy/org/grails/web/mapping/CachingLinkGenerator.java#L120-L139

https://github.com/grails/grails-core/blob/0daf7c69f98cd3f2f25ad990b52e005f7c976f5a/grails-core/src/main/groovy/grails/util/GrailsMetaClassUtils.java#L241-L248

…pache#13627

matrei

Could you write a test that shows the bug and that this change fixes it?

codeconsole · 2024-09-08T21:09:29Z

Could you write a test that shows the bug and that this change fixes it?

Why can't I get the same treatment?

but I approve this so you can get on with more important tasks.

…rty that is not id. e.g. static mapping = { id: name: 'customId' }

matrei · 2024-09-09T05:31:01Z

Why can't I get the same treatment?

Haha, sorry 😄!

This changes the behavior of Grails and there should be a test showing and testing the correct behavior.
Also protects against regressions in future changes.

And it should be a simple test to write.

matrei

matrei · 2024-09-10T06:40:24Z

grails-web-url-mappings/src/main/groovy/org/grails/web/mapping/DefaultLinkGenerator.groovy

+            if (ident) {
+                return ident.toString()
+            }
+        } catch (MissingMethodException | IllegalStateException e) {


You can use exception parameter name ignored to signal that this is expected.

I think it's fine to use try-catch here, until we find it's a performance problem or find a better way.

matrei · 2024-09-10T06:50:12Z

grails-web-url-mappings/src/test/groovy/org/grails/web/mapping/LinkGeneratorSpec.groovy

@@ -341,7 +365,8 @@ class LinkGeneratorSpec extends Specification {
        def generator = cache ? new CachingLinkGenerator(baseUrl, context) : new DefaultLinkGenerator(baseUrl, context)
        final callable = { String controller, String action, String namespace, String pluginName, String httpMethod, Map params ->
            [createRelativeURL: { String c, String a, String n, String p, Map parameterValues, String encoding, String fragment ->
-                "${namespace ? '/' + namespace : ''}/$controller/$action".toString()
+
+                "${namespace ? '/' + namespace : ''}/$controller${parameterValues.id? '/'+parameterValues.id:''}/$action".toString()


To my knowledge, most common in Grails is using /controller/action/id. In the PR description you are using /controller/action/id and /controller/id/action. Should we be consistent, and use /controller/action/id here?

I do believe this needs to be namespace/controller/action/id

static mappings = { "/$namespace/$controller/$action?/$id?(.$format)?" { constraints { } } }

@matrei @jamesfredley I had made it a "restful mapping" for the test as it was specific to the test. To eliminate confusion, I just changed it to the format you guys are referring to.

matrei · 2024-09-10T06:56:27Z

grails-web-url-mappings/src/test/groovy/org/grails/web/mapping/LinkGeneratorSpec.groovy

@@ -289,6 +290,29 @@ class LinkGeneratorSpec extends Specification {
            cacheKey == "somePrefix[resource:org.grails.web.mapping.Widget->2]"
    }

+    //
+    def 'resource links should use ident and allow controller override'() {


Nice!

You could add @Issue('https://github.com/grails/grails-core/issues/13627') on the test.

jamesfredley · 2024-09-10T18:43:19Z

grails-web-url-mappings/src/test/groovy/org/grails/web/mapping/LinkGeneratorSpec.groovy

@@ -341,7 +365,8 @@ class LinkGeneratorSpec extends Specification {
        def generator = cache ? new CachingLinkGenerator(baseUrl, context) : new DefaultLinkGenerator(baseUrl, context)
        final callable = { String controller, String action, String namespace, String pluginName, String httpMethod, Map params ->
            [createRelativeURL: { String c, String a, String n, String p, Map parameterValues, String encoding, String fragment ->
-                "${namespace ? '/' + namespace : ''}/$controller/$action".toString()
+
+                "${namespace ? '/' + namespace : ''}/$controller${parameterValues.id? '/'+parameterValues.id:''}/$action".toString()


I do believe this needs to be namespace/controller/action/id

jamesfredley · 2024-09-10T18:47:17Z

grails-web-url-mappings/src/test/groovy/org/grails/web/mapping/LinkGeneratorSpec.groovy

@@ -341,7 +365,8 @@ class LinkGeneratorSpec extends Specification {
        def generator = cache ? new CachingLinkGenerator(baseUrl, context) : new DefaultLinkGenerator(baseUrl, context)
        final callable = { String controller, String action, String namespace, String pluginName, String httpMethod, Map params ->
            [createRelativeURL: { String c, String a, String n, String p, Map parameterValues, String encoding, String fragment ->
-                "${namespace ? '/' + namespace : ''}/$controller/$action".toString()
+
+                "${namespace ? '/' + namespace : ''}/$controller${parameterValues.id? '/'+parameterValues.id:''}/$action".toString()


static mappings = { "/$namespace/$controller/$action?/$id?(.$format)?" { constraints { } } }

jamesfredley · 2024-09-10T18:48:30Z

grails-web-url-mappings/src/test/groovy/org/grails/web/mapping/LinkGeneratorSpec.groovy

+        linkParams.resource = new Widget(id: 1, name: 'Some Widget')
+
+        then:
+        link == "/bar/widget/1/show"


Related to https://github.com/grails/grails-core/pull/13628/files#diff-3e98c5bfca0e230c95b2e444b5345889eb2770d43cd5124003b3287efaee4d3eR368

jamesfredley · 2024-09-10T18:48:35Z

grails-web-url-mappings/src/test/groovy/org/grails/web/mapping/LinkGeneratorSpec.groovy

+        linkParams.controller = 'widgetAdmin'
+
+        then:
+        link == "/bar/widgetAdmin/1/show"


Related to https://github.com/grails/grails-core/pull/13628/files#diff-3e98c5bfca0e230c95b2e444b5345889eb2770d43cd5124003b3287efaee4d3eR368

jamesfredley · 2024-09-10T19:02:26Z

the getResourceId(resourceAttribute) fix is big. I'm not sure how that had not come up before.

Support controller attribute in tags with resource attribute. Fixes a…

Loading
Loading status checks…

7cd2e81

…pache#13627

grails-build added the bug label Sep 7, 2024

codeconsole requested review from matrei and jamesfredley September 7, 2024 23:08

matrei reviewed Sep 8, 2024

View reviewed changes

Fix for when domain classes have a static id mapping to another prope…

Loading
Loading status checks…

7b13fd0

…rty that is not id. e.g. static mapping = { id: name: 'customId' }

Document options. Remove accidental toString()

Loading
Loading status checks…

4a0ea2b

codeconsole changed the title ~~Support controller attribute in tags with resource attribute. Fixes #13627~~ Support controller attribute in tags with resource attribute and resolve correct identity Fixes #13627 Sep 9, 2024

codeconsole added 2 commits September 9, 2024 11:18

update comment

Loading
Loading status checks…

807a6d6

Add unit test for apache#13627

Loading
Loading status checks…

1dd266e

codeconsole requested review from matrei and jeffscottbrown September 9, 2024 19:39

matrei reviewed Sep 10, 2024

View reviewed changes

codeconsole added 3 commits September 10, 2024 10:19

Add link to github issue

Loading
Loading status checks…

72ab5f8

Update copyright

Loading
Loading status checks…

16ec0c1

change exception variable to ignored

Loading
Loading status checks…

f0602de

jamesfredley reviewed Sep 10, 2024

View reviewed changes

Change from resources mapping to default mapping

Loading
Loading status checks…

bf6a0c3

codeconsole merged commit 8871703 into apache:6.2.x Sep 10, 2024
12 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support controller attribute in tags with resource attribute and resolve correct identity Fixes #13627 #13628

Support controller attribute in tags with resource attribute and resolve correct identity Fixes #13627 #13628

codeconsole commented Sep 7, 2024 •

edited

Loading

matrei left a comment

codeconsole commented Sep 8, 2024 •

edited

Loading

matrei commented Sep 9, 2024

matrei left a comment

matrei Sep 10, 2024

codeconsole Sep 10, 2024

matrei Sep 10, 2024

jamesfredley Sep 10, 2024

jamesfredley Sep 10, 2024

codeconsole Sep 10, 2024

matrei Sep 10, 2024

jamesfredley Sep 10, 2024

jamesfredley Sep 10, 2024

jamesfredley Sep 10, 2024

jamesfredley Sep 10, 2024

jamesfredley commented Sep 10, 2024

Support controller attribute in tags with resource attribute and resolve correct identity Fixes #13627 #13628

Support controller attribute in tags with resource attribute and resolve correct identity Fixes #13627 #13628

Conversation

codeconsole commented Sep 7, 2024 • edited Loading

Design Assumptions

Reason for Incorrect Behavior

matrei left a comment

Choose a reason for hiding this comment

codeconsole commented Sep 8, 2024 • edited Loading

matrei commented Sep 9, 2024

matrei left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamesfredley commented Sep 10, 2024

codeconsole commented Sep 7, 2024 •

edited

Loading

codeconsole commented Sep 8, 2024 •

edited

Loading