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

Changes to withNewSession semantics make it useless for audit log listener #173

Closed
longwa opened this issue Aug 8, 2018 · 2 comments · Fixed by #212
Closed

Changes to withNewSession semantics make it useless for audit log listener #173

longwa opened this issue Aug 8, 2018 · 2 comments · Fixed by #212

Comments

@longwa
Copy link
Contributor

longwa commented Aug 8, 2018

Per behavior changes in:
grails/grails-data-mapping#1130

The call to withNewSession in the listener should be removed as it isn't occurring in the same transactional context as the caller anymore.

Audit logging should be atomic to the transaction making the changes such that both commit or rollback together. Previously, withNewSession was useful to prevent contaminating the outer session.

@longwa
Copy link
Contributor Author

longwa commented Aug 8, 2018

I'm testing this change in our application now. Assuming no issues, we should probably include this in 3.0.2 as well.

@longwa
Copy link
Contributor Author

longwa commented Aug 9, 2018

Doing a bit more testing, it actually is required in some cases as the listener is being called during a flush and should add anything else to the session in that case. However, it's not clear with the withNewSession behavior if the transactional context is preserved. One option could be to defer the actual save logic until after successful commit of the transaction by using TransactionSynchronizationManager.registerSynchronization and add an afterCommit handler.

That might not be a bad idea anyway since it would defer any impact of the audit log until successful commit in all cases (not just in the flushMode = COMMIT) case.

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

Successfully merging a pull request may close this issue.

1 participant