grails / grails-core

The Grails Web Application Framework
http://grails.org
Apache License 2.0
2.79k stars 949 forks source link

Grails Controller Web Binding does not work with Serializable or Generic Binding Objects #13634

Open codeconsole opened 1 month ago

codeconsole commented 1 month ago

The following (understandably) does not work, but a mechanism should be provided to facilitate binding

class MyController<T>  {

    def get(Serializable id) { // given a request to /my/1234
         assert(id, null) // true
         assert(params.id, != null) // true 
         assert(params.long('id'), != null) // true
    }

    def save(T instance)  {
        assert(instance, null)
    } 
}

ControllerActionTransformer.java

accepts a parameter of type [java.io.Serializable]. Interface types and abstract class types are not supported as command objects. This parameter will be ignored.

https://github.com/grails/grails-core/blob/8871703dc979e99bbc28669588ccbc48eca39342/grails-plugin-controllers/src/main/groovy/org/grails/compiler/web/ControllerActionTransformer.java#L844-L856

https://github.com/grails/grails-core/blob/9ea9c60bbed69718197549be83b900b2ed42eccf/grails-plugin-controllers/src/main/groovy/grails/artefact/Controller.groovy#L363-L460

Solution Proposal - @Bind annotation

@Bind(Sample) 
class MyController extends GenericController<Sample> {
    SampleController() {
        super(Sample)
    }
}

class GenericController<T>  {
    GenericController(Class<T> domainClass) {
        this.domainClass = domainClass
    }

    def get(Serializable id) { // given a request to /my/1234
         assert(id, !null) // true
         respond domainClass.get(id)
    }

    def save(T instance)  {
        assert(instance.class, domainClass) // true
        domainClass.save instance, flush: true
        respond instance
    } 

    def update(T instance, @Bind(skip=true) Map model) {
         // model is null but this is useful for inheritance where if
         // this class is extended and super.update(instance, [message:'Hello world']) is called
         // this method could do the update and return the model.
         domainClass.save instance, flush: true
         respond instance, model: (model?:[message:'Instance updated successfully'])
     }
}

additionally the following will work the same and allow generic and serializable binding

@Scaffold(value = GenericController, domain = User)
class UserController {
     def update(User user) {
         super.update(user, [message: 'User updated'])
     }
}

@Bind should

  1. skip=true allow suppression of controller warning messages.
  2. enable binding of Serializable id to the same type of the id of the domain object.
  3. enable binding of generic types to the specified object.
  4. allow annotating of other annotations where it uses the domain attribute or generic attribute of that attribute. For instance annotation @Scaffold annotation definition with @Bind will allow @Scaffold(domain=Sample) to also function as @Bind and only require 1 attribute.
matrei commented 1 month ago

There is already a grails.databinding.BindUsing annotation. Would it be possible/beneficial to add this functionality to that annotation instead of creating a new one, for this 'edgy' case?

For example:

@BindUsing(domain = Sample)
codeconsole commented 1 month ago

@matrei @BindUsing serves a completely different purpose. The annotation defined here is for instructing an @AstTransformer specifically on Controllers at compile time. I know this sounds edgy, but it actually has a lot of potential to dramatically promote code reuse.

matrei commented 1 month ago

The annotation defined here is for instructing an @AstTransformer specifically on Controllers at compile time.

Aha, I see.

I know this sounds edgy, but it actually has a lot of potential to dramatically promote code reuse.

I don't doubt it. I might be missing something from the bigger picture, but it is primarily to be used for scaffolding, right? And not something the average developer would use in his own application code.

@Bind is kind of generic. Maybe @BindDomain or @BindType to not hog the @Bind annotation name.

codeconsole commented 1 month ago

@matrei actually this could be used for any sort of controller inheritance and is not specific to scaffolding. It also provides a mechanism to squash warnings from ControllerActionTransformer.java. The main point behind this is to further eliminate the need to repeat common code functionality across controllers. This tag would only be used by ControllerActionTransformer which adds code to each controller to allow it to bind request attributes to method parameters. Without this, you have to bind manually in situations where you want to share controller logic.

This is what RestfulContoller does.

matrei commented 1 month ago

actually this could be used for any sort of controller inheritance and is not specific to scaffolding

Ok, good info!

The skip parameter name is a bit hard to decipher. Is it this warning from AST processing that will be skipped: https://github.com/grails/grails-core/blob/8871703dc979e99bbc28669588ccbc48eca39342/grails-plugin-controllers/src/main/groovy/org/grails/compiler/web/ControllerActionTransformer.java#L760-L766

codeconsole commented 1 month ago

That is 1 case. It will forcefully ignore binding with the annotation and also would suppress warning messages like you see there.