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 https://github.com/grails/grails-core/commit/c46d2cd9f5fbe9305def590ef88021ad8ca6bdcb 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.

[codeconsole]

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?

the Map is expected to be null. It shouldn't bind and it should (currently) give a warning during compilation.

I would like to introduce an annotation in the future to suppress that warning https://github.com/grails/grails-core/issues/13634

[codeconsole]

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.

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

I just gave one example, but other cases could be show(Serializable id), show(T instance)... Those currently don't bind at all, but it would be great if they did.

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

There are many reasons behind the super controller and sometimes they chain multiple layers

For instance UserController<User> extends SecuredController<User> extends GenericController<User>

Other chaining could be for specific business logic or caching or indexing.

SecuredController<T> extends GenericController<T>  {
     @Secured('ROLE_ADMIN')
     def show(Map map) {
         super.show(map)
     }
}

This eliminates the need to annotate every controller you want secured a specific general way.

[jeffscottbrown]

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

[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.