Remove unintended @ReadOnly from GormService - #16212
Open
codeconsole wants to merge 1 commit into
Open
Conversation
GormService carries a class-level @readonly, so the transactional AST transform wraps get/list/count in a GrailsTransactionTemplate. With the default REQUIRED propagation, a read taken outside an existing transaction becomes the outermost transaction, and committing it flushes the session: DatastoreTransactionManager.doCommit skips its own flush when the transaction is read-only, then calls transaction.commit(), and MongoTransaction.commit() flushes unconditionally. A read can therefore write. When one of these reads runs inside a flush — a referential check in a validator, or a beforeInsert hook — the commit re-validates the entity being saved, the validator reads again, and the cycle repeats until the stack is exhausted. The annotation is also redundant where scaffolded reads are actually served: RestfulServiceController already declares @readonly at class level, so that path keeps its read-only boundary either way. The service annotation only takes effect when the service is called outside one, which is precisely where opening and committing a transaction is unwanted. The write methods keep their own @transactional.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 8.0.x #16212 +/- ##
==================================================
- Coverage 54.1238% 54.1155% -0.0084%
+ Complexity 20307 20302 -5
==================================================
Files 2107 2107
Lines 101144 101143 -1
Branches 17921 17921
==================================================
- Hits 54743 54734 -9
- Misses 38595 38602 +7
- Partials 7806 7807 +1
🚀 New features to boost your workflow:
|
✅ All tests passed ✅🏷️ Commit: 608b8ea Learn more about TestLens at testlens.app/docs. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When I originally created GormService, the purpose was to provide CRUD operations implemented using GORM. It was modeled after
RestfulControllerused by the scaffolding plugin. The intent was to have aRestfulServiceControllerthat used a service layer instead of direct datastore access. I mistakenly added@ReadOnly.@ReadOnlyshould not be there because GORMgetdoes not happen in a transaction so GormServicegetshould not either.Problem
GormServiceis annotated@ReadOnlyat class level, so the transactional AST transform wrapsget/list/countin aGrailsTransactionTemplate. With the defaultREQUIREDpropagation, a read taken outside an existing transaction becomes the outermost transaction — and committing it flushes the session:So a read can write.
@ReadOnlysuppresses the transaction manager's own flush, but not the one insidecommit().That becomes a hang when such a read runs during a flush. A referential check in a validator is the ordinary case:
If
Principal.get(...)reachesGormService, the commit of its read-only transaction flushes the session, the flush re-validates the entity being saved, the validator reads again, and the cycle repeats until the stack is exhausted. It surfaces well away from the cause — asIllegalStateException: Transaction synchronization is not activethrown out of the commit unwind, with theStackOverflowErrorlost.beforeInsert/beforeUpdatehooks are exposed the same way.Change
Drop the class-level
@ReadOnlyfromGormService. The write methods keep their own@Transactional.The annotation is redundant where scaffolded reads are actually served:
RestfulServiceControlleralready declares@ReadOnlyat class level, so that path keeps its read-only boundary either way. The service annotation only takes effect when the service is called outside one — which is exactly where opening and committing a transaction is unwanted, and where callers reasonably expectget(id)to behave like the GORM static call it delegates to.Tests
:grails-scaffolding:testpluscheckon:grails-test-examples-scaffolding,:grails-test-examples-scaffolding-fields, and:grails-test-examples-hibernate7-scaffolding-fields(integration tests included) all pass.Related
#16214 aligns the behaviour underneath this.
@ReadOnlysuppresses the flush on Hibernate but not on theDatastoreTransactionManagerpath, wheredoCommit'sif (!status.isReadOnly())guard is defeated bytransaction.commit()flushing unconditionally in bothMongoTransactionandSessionOnlyTransaction.The two are independent. This one removes a
@ReadOnlythat a scaffold service should not carry regardless of what read-only ends up meaning; #16214 makes read-only mean the same thing on both paths. Either can land without the other.(Minor, noticed while tracing it: the
// Just set to NEVER in case of a new Session for this transaction.comment indoBeginis inherited from Spring'sHibernateTransactionManager, which sets Hibernate's ownFlushMode.MANUAL.jakarta.persistence.FlushModeTypeoffers onlyAUTOandCOMMIT, so the code is already as strict as the enum allows and the comment is what is stale. #16214 corrects it.)