gpc / grails-audit-logging-plugin

The Grails Audit Logging Plugin
Apache License 2.0
50 stars 60 forks source link

Use withTransaction instead of withNewTransaction, support inlining plugin #208

Closed felixscheinost closed 3 years ago

felixscheinost commented 3 years ago

Use withTransaction instead of withNewTransaction to reuse an existing transaction.

I think this makes more sense as in most cases we already should have an existing transaction and in this way we only store audit log events in case the outer transaction commits.

@longwa I understand that the withNewTransaction was only a workaround for Grails 4 but isn't withTransaction a better workaround?

longwa commented 3 years ago

This would've been the obvious and better change, but if I recall, this didn't work correctly at the time. I believe the transactional context was already gone by the time the audit listener was being called.

I had actually worked on a different implementation that used a TransactionSynchronizationAdapter to register an afterCommit handler and only commit the audit results if the transaction was successfully committed, which is definitely the desired behavior.

I'm not sure if I ever pushed any progress on that branch, but that worked and showed promise, I just didn't have time to chase down the breakage it caused in a few places in our application.

felixscheinost commented 3 years ago

Yeah, I think you are right this doesn't work correctly. I picked up your branch instead and will continue work there.