[#225] introduce Transaction.Status enum and deprecate Status#226
[#225] introduce Transaction.Status enum and deprecate Status#226gavinking wants to merge 4 commits intojakartaee:masterfrom
Conversation
also give Synchronization default noop impls of its methods for jakartaee#225
gavinking
changed the title
| * | ||
| * @since JTA 2.1 | ||
| */ | ||
| public Status getCurrentStatus() throws SystemException; |
There was a problem hiding this comment.
Maybe simply currentStatus() or status()? Wonder if the Javabean style is still required?
There was a problem hiding this comment.
This is something I've been thinking about recently. I had sorta decided that it made sense to go with the short form for things that are "like record fields", i.e. things that are by nature simple immutable values.
The tx status is mutable, so I would say stick with the old-style getter here.
A more aggressive strategy would be: "use the short form for everything which doesn't have a setter". But there's a couple of reasons to not go in that direction:
- we have so many legacy APIs which don't conform to that, and
- in some cases, you might want to add a setter later, or on a subtype.
starksm64
left a comment
starksm64
left a comment
There was a problem hiding this comment.
I like this one better than mine. I was worried that not having a symbol different than Status would cause issues, but scoping it to Transaction class resolves this.
There is just one other location missing that should be updated, the javadoc in TransactionScoped.java:
UserTransaction.getCurrentStatusorTransactionManager.getCurrentStatus- is one of the following states:
-
- Transaction.Status.STATUS_ACTIVE
- Transaction.Status.STATUS_MARKED_ROLLBACK
- Transaction.Status.STATUS_PREPARED
- Transaction.Status.STATUS_UNKNOWN
- Transaction.Status.STATUS_PREPARING
- Transaction.Status.STATUS_COMMITTING
- Transaction.Status.STATUS_ROLLING_BACK
| /** | ||
| * The equivalent legacy integer-valued code defined by {@link jakarta.transaction.Status}. | ||
| */ | ||
| public int code() { |
There was a problem hiding this comment.
This is not technically needed as you can just return Transation.Status.ACTIVE.ordinal() for example, but this would safeguard against a reordering of the enums breaking consistency with legacy Status constant value.
There was a problem hiding this comment.
Yeah I was just being extra super-anal here.
| * @since JTA 2.1 | ||
| */ | ||
| enum Status { | ||
| /** |
There was a problem hiding this comment.
If the Status enum names were left exactly the same as the legacy jakarta.transaction.Status.STATUS_* constants, you could go a global search and replace to upgrade to enums. Many IDEs do support regex search and replace, so it still could be done and you would have less verbose enum names. It is probably better to drop the STATUS_ prefix from the enum names.
There was a problem hiding this comment.
Your concern is about renaming MARKED_ROLLBACK to MARKED_FOR_ROLLBACK and ROLLEDBACK to ROLLED_BACK, right?
There was a problem hiding this comment.
Really more of just leaving the names exactly as found in the legacy Status which means the the full STATUS_MARKED_ROLLBACK vs new MARKED_FOR_ROLLBACK. I'm really only thinking of if one wants to update an existing codebase to use the new enum. I'm iffy as to whether that is worth it.
|
Can one of the admins verify this patch? |
|
ok to test |
| * The equivalent legacy integer-valued code defined by {@link jakarta.transaction.Status}. | ||
| */ | ||
| public int code() { | ||
| switch (this) { |
There was a problem hiding this comment.
Given the next feature release of Jakarta Transactions is part of the platform 12 which is targetting EE12 (https://jakartaee.github.io/platform/jakartaee12/JakartaEE12ReleasePlan), having talked to Gavin it sounds like this switch could be possible to refactor to switch expressions (hopefully then not needing the default to be defined). This is not a blocker but it might be nice
| /** | ||
| * The equivalent legacy integer-valued code defined by {@link jakarta.transaction.Status}. | ||
| */ | ||
| public int code() { |
There was a problem hiding this comment.
Having talked this through with Gavin, I think int is is really what is deprecated here and so we should either add this method as Deprecated or not add this method
tomjenkinson
left a comment
tomjenkinson
left a comment
There was a problem hiding this comment.
Thank you for very much for the contribution to our project @gavinking ! I will share it on the mailing list for a few days for further reviews and merge it next week.
|
Advertised to the community at https://www.eclipse.org/lists/jta-dev/msg00375.html |
|
Closing in favor of #241 after rebasing. |
6 participants
also give
Synchronizationdefault noop impls of its methodsfor #225