Grails 6.2.0 - Cast Exception if Action takes Command

[jdaugherty]

Expected Behavior

After upgrading to Grails 6.2.0, one of our controllers started to error. I was able to reproduce this issue in a sample application. Visiting http://localhost:8080/example/list should render the list action renders without issue. Instead an exception is thrown.

Changing a variable name in an unrelated action fixes the issue.

Actual Behaviour

A class cast exception occurs:

Cannot cast object 'brokenBinding.ExampleSearchCommand@116e5496' with class 'brokenBinding.ExampleSearchCommand' to class 'brokenBinding.ExampleCommand'

Steps To Reproduce

Details in README.md

1. Launch grails app
2. Visit http://localhost:8080/example/list

Environment Information

• Operating System: Mac OS 14.4.1 & Latest Stable Linux (Debian)
• JDK: 11.0.22 Liberica (.sdkmanrc has version in example project)

Example Application

https://github.com/jdaugherty/grails-broken-binding

Version

6.2.0

[jdaugherty]

Here's a screenshot of the byte code generated by Grails for the list action that gets created to handle command object creation:

I've circled in red what's causing the cast exception.

[matrei]

That is interesting! Could you also post the generated code when the variable is renamed?

[jdaugherty]

If I rename the variable in the edit action, here's the generated code of the list action:

[jdaugherty]

This behavior wasn't present in our project for Grails 5.x. We first noticed it when we updated to Grails 6.x.

[rick-boardontrack]

FYI: I ran into this too.

I found that if I use the method parameter name 'cmd' for the command object, then it fails:

def save(AssessmentCommand cmd) {

produces the org.codehaus.groovy.runtime.typehandling.GroovyCastException when I call save().

But when changing the parameter named to 'acmd', everything works as expected:

def save(AssessmentCommand acmd) {

[rick-boardontrack]

I've learned a bit more... it is not JUST the name. I have another case where the name of the parameter is 'cmd' and it works just fine.

[jeffscottbrown]

I am investigating this issue. At https://github.com/osscontributor/issue13486 I have a simple sample that demonstrates the issue:

grails-app/controllers/issue13486/DemoController.groovy

package issue13486


class DemoController {
    def list(DemoCommand x) {
    }

    // commenting out this method or the variable declaration in it
    // will allow the unit test to pass
    private void placeToDeclareLocalVariable() {
        String x;
    }

}

class DemoCommand {
}

src/test/groovy/issue13486/DemoControllerSpec.groovy

package issue13486

import grails.testing.web.controllers.ControllerUnitTest
import spock.lang.Specification

class DemoControllerSpec extends Specification implements ControllerUnitTest<DemoController> {

     void "test index action"() {
        when: 'the controller action is invoked'
        controller.list()

        then:
        noExceptionThrown()
     }
}

I will update soon.

[jeffscottbrown]

Will investigate whether or not c46d2cd is relevant. (looks unlikely right now)

[jeffscottbrown]

FYI... The expression at https://github.com/grails/grails-core/blob/v6.0.0/grails-plugin-controllers/src/main/groovy/org/grails/compiler/web/ControllerActionTransformer.java#L853 does appear to be relevant, details still not entirely clear.

[jeffscottbrown]

FYI, For an update, this is what I am seeing. If I declare a method in the controller like this…

private void placeToDeclareLocalVariable() {
    String x
}

The ast generated no-arg list() method will contain the following:

String x = null;
x = __$stMC || BytecodeInterface8.disabledStandardMetaClass() ?
    ShortTypeHandling.castToString((Object)arrcallSite[43].callCurrent((GroovyObject)this, DemoCommand.class, (Object)"x")) :
    ShortTypeHandling.castToString((Object)this.initializeCommandObject(DemoCommand.class, (String)"x"));

If I change that method to look like this…

private void placeToDeclareLocalVariable() {
    java.sql.Connection x
}

The ast generated no-arg list() method will contain the following:

Connection x = null;
x = __$stMC || BytecodeInterface8.disabledStandardMetaClass() ?
    (Connection)ScriptBytecodeAdapter.castToType((Object)arrcallSite[43].callCurrent((GroovyObject)this, DemoCommand.class, (Object)"x"), Connection.class) :
    (Connection)ScriptBytecodeAdapter.castToType((Object)this.initializeCommandObject(DemoCommand.class, (String)"x"), Connection.class);

If I remove that method altogether, the following is generated (which looks right):

DemoCommand x = null;
x = __$stMC || BytecodeInterface8.disabledStandardMetaClass() ?
    (DemoCommand)ScriptBytecodeAdapter.castToType((Object)arrcallSite[43].callCurrent((GroovyObject)this, DemoCommand.class, (Object)"x"), DemoCommand.class) :
    (DemoCommand)ScriptBytecodeAdapter.castToType((Object)this.initializeCommandObject(DemoCommand.class, (String)"x"), DemoCommand.class);

It is not yet clear how the type in that local variable is affecting the code that gets generated for that list() method.

[jeffscottbrown]

@codeconsole Are there particular types of constructor args that are not working for you? I am building up some automated tests around this using inheritance and constructor args and am trying to identify a scenario that doesn't work. Thank you for your input!

[jeffscottbrown]

The info I have right now is the following:

breaks extending controllers and passing the super args

I have built up tests that involve extending controller and passing super args and will put those in a PR soon. If you can help me know what the problematic scenarios are I can get those addressed and I appreciate your help describing the issue.

[jeffscottbrown]

@codeconsole I got some info this afternoon that indicates that changes you have recently made to scaffolding may demonstrate the issue. I am going to test with the latest scaffolding code and that may lead to where I need to be. In the meantime, if you can share any info here describing the problem you found that might prove helpful. Thank you!

[codeconsole]

@osscontributor I don't think you will get it with the scaffolding specifically. I will create an example for you. It's an edge case if you reuse a parent controller and pass a parameter to the parent. A compilation error occurs with the change. The error did not occur prior to the change. The parameter does not implement Validateable.

@Artefact("Controller")
class GenericController<T> {
    Class<T> resource

    GenericController(Class<T> resource) {
        this.resource = resource
    }
    def show(Map model) {
        if (!model) {
            model = [:]
        }
        respond resource.get(params.id), model: model
    }
}

class UserController extends GenericController<User> {
    public UserController() {
         super(User)
    }

    def show() {
         super.show([hello:'world'])
    }
}

> Task :compileGroovy FAILED

/GenericController.groovy: -1: The current scope already contains a variable of the name model
 @ line -1, column -1.

[jeffscottbrown]

Thank you! This is helpful.

I will work up some tests and a solution.

[codeconsole]

Thanks @osscontributor, also be aware that there can be multiple parameters to the method. You probably don't even need generics to replicate and can just extend a normal controller.

    def show(Long id, String name, Map model) {
        if (!model) {
            model = [:]
        }
        respond resource.get(id), model: model
    }

I know this is kind of edgy, but it really adds flexibility to the codebase and eliminates a lot of duplicate generated code.

[jeffscottbrown]

If you have a show() method in UserController, a show(Map) method in GenericController and you want UserController to extend GenericController, which of those methods would you expect to be invoked using the default generated URL mappings if a request is sent to /user/show? That clarification will help.

We added code a long time ago to prevent overloading controller actions and this is bumping up against some assumptions that are made in the code.

Thank you for clarification!

[jeffscottbrown]

@codeconsole If you can share a small simple sample 6.2.1-SNAPSHOT project which demonstrates code that doesn't compile with the change that was proposed and later reverted, I am happy to resolve that. Your help would be appreciated.

[codeconsole]

If you have a show() method in UserController, a show(Map) method in GenericController and you want UserController to extend GenericController, which of those methods would you expect to be invoked using the default generated URL mappings if a request is sent to /user/show? That clarification will help.

We added code a long time ago to prevent overloading controller actions and this is bumping up against some assumptions that are made in the code.

Thank you for clarification!

You would want UserController.show() to be called instead of the parent method on GenericController.show(Map). The only way GenericController.show(Map) would be called is if UserController does not have a show method. This is the way it is currently behaving.

[codeconsole]

@codeconsole If you can share a small simple sample 6.2.1-SNAPSHOT project which demonstrates code that doesn't compile with the change that was proposed and later reverted, I am happy to resolve that. Your help would be appreciated.

I will create you an example app that replicates the issue with the previous commit.

[jeffscottbrown]

I will create you an example app that replicates the issue with the previous commit.

Thank you!

[jeffscottbrown]

The only way GenericController.show(Map) would be called is if UserController does not have a show method. This is the way it is currently behaving.

When show(Map) is invoked, what is it that is expected to be in the Map?

[jeffscottbrown]

It looks like the show(Map) method in GenericController is only there to be a wrapper around respond resource.get(params.id), model: model. If that is the case, I don't think that method should be configured as a controller action. I think it would just be a helper method.

[jeffscottbrown]

No because when there is no show in UserController, the super GenericController.show(Map) executes.

What is it that you would expect to be in the Map for a scenario like that?

[jeffscottbrown]

A nice feature in the future would also be to have a model always bound and not have to pass it.

There isn't that but FYI there is a modelAndView property.

[codeconsole]

Hi @osscontributor, I appreciate your help figuring this one out.

Thank you. I will look forward to the sample app and that will help me provide more clarity.

So I over generalized the problem. I can not replicate this issue specifically with a Map. I created a situation where I was able to replicate and I made it is as simple as possible. https://github.com/codeconsole/issue13486.git
All you have to do is checkout the repository at your fix point, install it to mavenLocal, then run assemble on that example and you will see the error. I have no idea what is causing it and it works fine currently and prior to the fix so if you comment out mavenLocal() you can see it work as well.

What is it that you would expect to be in the Map for a scenario like that?

null, which has been the case

There isn't that but FYI there is a modelAndView property.

yeah, I was aware, but in this particular case I am only interested in passing a model.

[jeffscottbrown]

yeah, I was aware, but in this particular case I am only interested in passing a model.

I misunderstood and thought you wanted to avoid passing the model.

I can not replicate this issue specifically with a Map

That is good info. I spent yesterday trying to do that and found the same. Thank you for the confirmation!

I created a situation where I was able to replicate and I made it is as simple as possible.

That is perfect. I will investigate that repo today. I really appreciate your help.

[jeffscottbrown]

Is it possible for the problem to manifest in a Grails application? I know I wasn't explicit about asking for the app to be a Grails app, but that what would be helpful. Thank you for your feedback!

[jeffscottbrown]

Locally here when I copy the GenericController and ModelResolver class into a Grails app, the controller is compiling fine. I expect I am missing a build dependency but I can't tell what it is. If I can reproduce the problem in a Grails app, I am sure I can resolve it.

[jeffscottbrown]

Interesting is that if I remove org.springframework.boot:spring-boot-autoconfigure from your app, the code seems to compile. That same dependency is in the Grails app environment I am trying to test with and the code does compile with that dependency. I don't yet know what that means, but is a point of interest.

[jeffscottbrown]

I think in your project the the build will fail with the following dependencies:

dependencies {
implementation "org.springframework.boot:spring-boot-autoconfigure"
implementation "org.grails:grails-logging"
implementation "org.grails:grails-plugin-url-mappings"
}

And will compile successfully with the following (only difference is order):

dependencies {
implementation "org.grails:grails-logging"
implementation "org.springframework.boot:spring-boot-autoconfigure"
implementation "org.grails:grails-plugin-url-mappings"
}

I see the same thing in the grails app. If I declare the org.springframework.boot:spring-boot-autoconfigure dependency first, then the compile error emerges.

[jeffscottbrown]

@codeconsole Is it the case that as far as we know this issue has nothing to do with super args and nothing to do with inheritance?

[jeffscottbrown]

I am not 100% certain but it looks like as long as some dependency declared before org.springframework.boot:spring-boot-autoconfigure is pulling in Groovy, then the code compiles, including explicitly adding implementation 'org.codehaus.groovy:groovy:3.0.21' before implementation "org.springframework.boot:spring-boot-autoconfigure".

[jeffscottbrown]

@paulk-asert - does this behavior make sense to you?...

mkdir ~/someTempWorkingDir
cd ~/someTempWorkingDir

git clone [email protected]:codeconsole/issue13486.git
git clone [email protected]:grails/grails-core.git

cd grails-core
git checkout 9135770f4e9d75a7973142d174520a56a93d59c0
./gradlew pTML

cd ../issue13486
git checkout 355eccfc1804ca0b6cfe912a1f8309e8735b0740
./gradlew assemble

# I expect compilation to have failed

# add 'org.codehaus.groovy:groovy:3.0.21' as an 
# implementation dependency in issue13486/build.gradle 
# as the first dependency listed in the build file
# and run "./gradlew assemble" again and I expect
# it to compile

[codeconsole]

I misunderstood and thought you wanted to avoid passing the model.

I was suggesting perhaps in the future it would be nice to always have access to a bound default model and not have to pass it explicitly.

Is it possible for the problem to manifest in a Grails application? I know I wasn't explicit about asking for the app to be a Grails app, but that what would be helpful. Thank you for your feedback!

This was a Grails plugin that previously also ran using bootRun, but I deleted all the surrounding code so that I could isolate the issue as specific as possible.

This was the initial dependency block before I stripped it down to replicate the issue:

dependencies {
    developmentOnly("org.springframework.boot:spring-boot-devtools")
    implementation "org.springframework.boot:spring-boot-starter-logging"
    implementation "org.springframework.boot:spring-boot-autoconfigure"
    implementation "org.grails:grails-core"
    implementation "org.springframework.boot:spring-boot-starter-actuator"
    implementation "org.springframework.boot:spring-boot-starter-tomcat"
    implementation "org.grails:grails-web-boot"
    implementation "org.grails:grails-logging"
    implementation "org.grails:grails-plugin-rest"
    implementation "org.grails:grails-plugin-databinding"
    implementation "org.grails:grails-plugin-i18n"
    implementation "org.grails:grails-plugin-services"
    implementation "org.grails:grails-plugin-url-mappings"
    implementation "org.grails:grails-plugin-interceptors"
    implementation "org.grails.plugins:cache"
    implementation "org.grails.plugins:async"
    implementation "org.grails.plugins:scaffolding"
    implementation "org.grails.plugins:gsp"
    implementation "org.springframework.security:spring-security-core:5.8.14"
    compileOnly "io.micronaut:micronaut-inject-groovy"
    console "org.grails:grails-console"
    profile "org.grails.profiles:web-plugin"
    testImplementation "io.micronaut:micronaut-inject-groovy"
    testImplementation "org.grails:grails-gorm-testing-support"
    testImplementation "org.mockito:mockito-core"
    testImplementation "org.grails:grails-web-testing-support"
}

Is it the case that as far as we know this issue has nothing to do with super args and nothing to do with inheritance?

I really don't know exactly because the issue goes away when commit 355eccfc1804ca0b6cfe912a1f8309e8735b0740 is removed. This is what makes it baffling. All you have to do is remove mavenLocal() from my example and the code assembles fine using the existing Grails 6.2.1-SNAPSHOT.

[jeffscottbrown]

I really don't know exactly because the issue goes away when commit 355eccfc1804ca0b6cfe912a1f8309e8735b0740 is removed.

Thank you. I am trying to chase down what role the inheritance and super args play. As far as I can tell so far they aren't relevant. I will report back.

[codeconsole]

Thank you. I am trying to chase down what role the inheritance and super args play. As far as I can tell so far they aren't relevant. I will report back.

It might be safe to assume this has nothing to do with inheritance at this point considering compilation still fails without it.

[jeffscottbrown]

Understanding why adding a dependency like org.codehaus.groovy:groovy:3.0.21 to the top of the dependencies list makes this particular error go away may prove helpful. I am investigating and have asked for help.

[jeffscottbrown]

Another way to make that failing build mentioned above compile successfully is by removing the grails-logging dependency at https://github.com/codeconsole/issue13486/blob/355eccfc1804ca0b6cfe912a1f8309e8735b0740/build.gradle#L27.

I am not suggesting that is a solution. It is just a data point while looking at the role the dependencies are playing.

[codeconsole]

Is the artifact still being properly processed when those changes are made or does making those changes prevent the artifact from being processed which is why the compilation ends up being successful?

[jeffscottbrown]

Is the artifact still being properly processed when those changes are made or does making those changes prevent the artifact from being processed which is why the compilation ends up being successful?

In the tests I am looking at now, they are being processed.

[paulk-asert]

@osscontributor I am not sure I understand the full behavior expected for that AST transform but does this achieve the desired behaviors:

diff --git a/grails-plugin-controllers/src/main/groovy/org/grails/compiler/web/ControllerActionTransformer.java b/grails-plugin-controllers/src/main/groovy/org/grails/compiler/web/ControllerActionTransformer.java
--- a/grails-plugin-controllers/src/main/groovy/org/grails/compiler/web/ControllerActionTransformer.java	(revision 9135770f4e9d75a7973142d174520a56a93d59c0)
+++ b/grails-plugin-controllers/src/main/groovy/org/grails/compiler/web/ControllerActionTransformer.java	(date 1727355444198)
@@ -846,7 +846,7 @@
         final ArgumentListExpression initializeCommandObjectArguments = args(classX(commandObjectNode), constX(paramName));
         final MethodCallExpression initializeCommandObjectMethodCall = callThisX("initializeCommandObject", initializeCommandObjectArguments);
         applyDefaultMethodTarget(initializeCommandObjectMethodCall, commandObjectNode);
-        final Expression assignCommandObjectToParameter = declX(localVarX(paramName), initializeCommandObjectMethodCall);
+        final Expression assignCommandObjectToParameter = assignX(varX(paramName, commandObjectNode), initializeCommandObjectMethodCall);
         wrapper.addStatement(stmt(assignCommandObjectToParameter));
     }

[jeffscottbrown]

@paulk-asert That fixes the "The current scope already contains a variable of the name..." issue but brings back the root issue that this issue was originally about.

This is a good pointer. I will move forward and find a solution that addresses both of those. I have a better idea now of what the problematic scenarios are.

Thank you for the input!

[jdaugherty]

I believe #13698 addresses this so this can be closed? @osscontributor can you please confirm?