Fix transactional annotation #419
Draft
Conversation
This file contains 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
ushkarev
force-pushed
the
transactional-annotations
branch
2 times, most recently
from
1ab4762 to
d6e2c24
Compare
ushkarev
commented
Apr 15, 2023
| @@ -344,7 +350,6 @@ class PrisonerIepLevelReviewService( | |||
| ) | |||
| } | |||
|
|
|||
| @Transactional | |||
There was a problem hiding this comment.
the only existing call to this method is from the processOffenderEvent in this class so the transactional annotation has no effect. but sadly we cannot make it private as there's a test case using it.
ushkarev
commented
Apr 15, 2023
| @@ -135,6 +135,7 @@ class PrisonerIepLevelReviewService( | |||
| suspend fun getReviewById(id: Long): IepDetail = | |||
| prisonerIepLevelRepository.findById(id)?.toIepDetail(incentiveLevelService.getAllIncentiveLevelsMapByCode()) ?: throw NoDataFoundException(id) | |||
|
|
|||
| @Transactional | |||
There was a problem hiding this comment.
adding annotation here so all effects of the received event are transactional now… is that too much? is there any harm in createIepForReceivedPrisoner becoming transactional?
There was a problem hiding this comment.
BUT if createIepForReceivedPrisoner or mergedPrisonerDetails were to raise domain events, then they might be received by another party prior to the transaction committing leading to a read of stale data!
ushkarev
force-pushed
the
transactional-annotations
branch
2 times, most recently
from
f12ad5e to
02806b6
Compare
ushkarev
force-pushed
the
transactional-annotations
branch
from
02806b6 to
29c1977
Compare
Spring only creates transactions when calling annotated methods on _other_ classes, invocations on `this` will not create transactions as the proxy object isn't used
ushkarev
force-pushed
the
transactional-annotations
branch
from
29c1977 to
75a0871
Compare
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.
Spring only creates transactions when calling annotated methods on other classes using a proxy object, invocations on
thiswill not create transactions as the transaction-managing proxy object isn’t used.🚨 Since transactions haven’t been created in practice, I’m not sure how safe this change is! If our event-receiving leads to DB changes and new events being published, they might be received by third parties prior to data being fully persisted.