Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Grails @Listener doesn't allow validation to work correctly #430

Open
xpusostomos opened this issue Dec 4, 2021 · 8 comments
Open

Grails @Listener doesn't allow validation to work correctly #430

xpusostomos opened this issue Dec 4, 2021 · 8 comments

Comments

@xpusostomos
Copy link

Grails has a special field "lastUpdated", which auto-populates the update date. You can make this field nullable: false, which makes sense... why should an auto-updated field be nullable.

But what if I want to make my own auto-populated field? Like say, the user who updated the record. It would seem that Grails @Listener service was designed primarily to implement auto-updated fields. At least that's what the documentation implies, I'm sure other uses might exist, but this its main reason. So we could implement auto user update field like this:

class RecordUserAuditService {
    def springSecurityService
    @Listener([Account])
    void onPreInsert(PreInsertEvent event) {
        User user = springSecurityService.currentUser
        event.entityAccess.setProperty('userCreated', user)
        event.entityAccess.setProperty('userUpdated', user)
    } 
    @Listener([Account])
    void onPreUpdate(PreUpdateEvent event) {
        User user = springSecurityService.currentUser
        event.entityAccess.setProperty('userUpdated', user)
    }
}

ok, so this will autopopulate the field...
but wait... this only gets called when the session is flushed, it doesn't get called prior to validate, and so it is impossible to auto-populate a nullable: false field. It is impossible to do what grails does for you with lastUpdated. When you call save() on your object, it will call validate() and fail... even though it would have later been correctly populated.

I think if this feature was designed properly, Grails could cease having "lastUpdated" as special, and the user could do it themselves. Personally, I hate "lastUpdated" as a name, I'd rather call it updateDate or something, but I don't have a choice, because the @Listener service doesn't work well enough to roll your own.

Another problem of @Listener is it forces you to list every single domain class. Why shouldn't I be able to have a wildcard match, and be able to check myself if my domain class has the field in question? If 95% of my domains have "userUpdated", it would be no overhead to always call it for all classes.

I also note that the example in the documentation populates Book.friendlyUrl.... it makes friendlyUrl nullable: true... but makes no mention of how critical it is to its functioning that it be set to nullable: true.

One might be tempted to make it NOT NULL in the database, and nullable: true in grails, just to force it to work... I guess that's a workaround, BUT... if you're using grails to auto-generate the schema, then that will fail during the development cycle.

Expected Behaviour

We need a Listener that gets called before validate. We need a listener that is capable of implementing something like lastUpdated.

We need a listener that optionally gets called for all domain classes and can check if the class has a paricular property.

Actual Behaviour

Tell us what happens instead

Environment Information

  • Grails Version: 4.0.12
  • JDK Version: 1.8
@osscontributor
Copy link
Member

but wait... this only gets called when the session is flushed, it doesn't get called prior to validate

Does an approach like the one shown at src/main/groovy/gorm/events/demo/listener/AuditListener.groovy#L29-L37 work?

@xpusostomos
Copy link
Author

@jeffbrown it does sound like that would solve the issue. It would be nice to see some documentation since the user guide doesn't mention these other events, and even HibernateEventListener doesn't seem to have basic javadoc. But if the user guide at least linked you to HibernateEventListener, it would be easier to become aware of other options. Thanks.

@xpusostomos
Copy link
Author

Although, I still wish there was an option to listen for all domain classes, rather than having to enumerate them all. How would you even do that if you're auditing domain classes from different plugins.

@osscontributor
Copy link
Member

osscontributor commented Dec 13, 2021

I still wish there was an option to listen for all domain classes, rather than having to enumerate them all.

I believe there is. I don't think there are any situations where in response to a persistence event you would have to enumerate all domain classes for any reason.

How would you even do that if you're auditing domain classes from different plugins.

There are a number of ways to do it. One is to register a bean in the Spring context that has a @Listener method in in it. For example:

import grails.events.annotation.gorm.Listener
import org.grails.datastore.mapping.engine.event.ValidationEvent


class MyListener {

    @Listener
    void validateEvent(ValidationEvent event) {
        // do whatever you like...
    }
}

@osscontributor osscontributor transferred this issue from grails/grails-core Dec 13, 2021
@osscontributor
Copy link
Member

I have moved this issue to gorm-hibernate5. I think the only actionable item in the thread is the following:

It would be nice to see some documentation since the user guide doesn't mention these other events, and even HibernateEventListener doesn't seem to have basic javadoc.

@osscontributor
Copy link
Member

Pull requests for the user guide (gorm.grails.org/7.1.0/hibernate/manual/index.html) and the javadocs (gorm.grails.org/7.1.0/hibernate/api/org/grails/orm/hibernate/event/listener/HibernateEventListener.html) are welcome. Thank you for the feedback!

@xpusostomos
Copy link
Author

xpusostomos commented Dec 15, 2021

@jeffbrown it turns out that using Listener's onValidate doesn't work very well. Because grails does binding validation before calling your controller method, it will call the Listener's onValidate before... for example... you have had a chance to call save() on any nested objects. What that means is if you call isDirty() on an object... which you would want to do to set the lastUpdatedUser, isDirty throws an exception that there are transitive unsaved objects. So basically, what you can do inside of Listener's onValidate is so limited as to be nearly useless. And you can't really escape because it is called before your controller method during binding.

It seems to me that there is kind of a bug here that you can't call isDirty() on objects that haven't been saved yet.

@osscontributor
Copy link
Member

It seems to me that there is kind of a bug here that you can't call isDirty() on objects that haven't been saved yet.

If that is the case, please file a separate bug report and provide a sample app and we will investigate that. Not being able to call isDirty() is a separate issue from how to register persistence event listeners, which is what this issue is addressing.

Thank you for your feedback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants