Document the requirement for a transaction for stateful repositories in Javadoc#1429
Document the requirement for a transaction for stateful repositories in Javadoc#1429njr-11 wants to merge 2 commits intojakartaee:mainfrom
Conversation
gavinking
left a comment
gavinking
left a comment
There was a problem hiding this comment.
Yesterday on the call I tried to explain the reasons this is difficult to specify. You've fallen right into the trap I mentioned.
| * A lifecycle method must be annotated with a lifecycle annotation such as | ||
| * {@link Insert}, {@link Update}, {@link Save}, or {@link Delete}. The | ||
| * method must accept a single parameter, whose type is either:</p> | ||
| * The lifecycle methods of a stateless repository must be annotated with a | ||
| * stateless lifecycle annotation such as {@link Insert}, {@link Update}, | ||
| * {@link Save}, or {@link Delete}. The lifecycle methods of a | ||
| * <a href="../jakarta.data.stateful/">stateful repository</a>, which apply | ||
| * to a persistence context, must be annotated with one of the stateful | ||
| * lifecycle annotations: {@code Detach}, {@code Merge}, {@code Persist}. | ||
| * {@code Refresh}, or {@code Remove}. For both stateful as well as stateless | ||
| * lifecycle methods, the lifecycle method must accept a single parameter, | ||
| * whose type is either:</p> |
There was a problem hiding this comment.
Pluralization does not agree here.
lifecycle methods ... a lifecycle annotation
Please revert back to the singular form which makes clear that each (singular) lifecycle method has one (singular) lifecycle annotation.
There was a problem hiding this comment.
I also dislike this change and think it should simply be rolled back. It was perfectly fine as it was before. This new language makes it look like you've enumerated all possible lifecycle annotations, rather than them being an open-ended set.
There was a problem hiding this comment.
I removed the list of stateful methods and switched back to singular.
The intended purpose of my changes in this section was:
- indicate that we also have stateful lifecycle annotations, and link to the stateful module where they can be found
- require that stateful and stateless not be intermixed on a repository. The singular wording "A lifecycle method must..." did not achieve this. That was my reason for switching to plural, but thanks for pointing out it causes other confusion. I like your suggestion of "each" for singular, which solves that without adding confusion.
| * case of {@code @Delete}, have a return type that is the same as the type | ||
| * case of {@code @Delete} and the stateful lifecycle annotations other than | ||
| * {@code @Merge}, have a return type that is the same as the type |
There was a problem hiding this comment.
Again, this is getting worse. Lifecycle annotation are an open-ended set. We should not be explicitly enumerating them like this.
There was a problem hiding this comment.
I didn't like what I added here either, but with the introduction of the stateless lifecycle annotations, the old statement here is no longer true and cannot be left as is. In my followup commit, I have switched the statement to be more generic and defer to the lifecycle method instead.
| * <p>A transaction must be active on the thread from which a repository | ||
| * method annotated {@code @Merge} is invoked. | ||
| * </p> |
There was a problem hiding this comment.
This is not correct. JDBC transactions are not associated with threads, and so when a repository is backed by a non-JTA datasource, it's the persistence context which determines the transaction. I mean, sure, you can argue that, well, usually there's some affinity between the thread and the persistence context, but even that need not be true.
This is what I was trying to tell you on the call. The thing you want to say here is a very thing to formulate in the abstract.
There was a problem hiding this comment.
This is not correct. JDBC transactions are not associated with threads
JDBC transactions are associated with a java.sql.Connection object. Although it is true the JDBC specification doesn't explicitly forbid JDBC drivers from supporting concurrent access to a java.sql.Connection, it is certainly not a requirement of the JDBC specification, nor is it a practice I have ever seen offered or encouraged by a JDBC vendor. That part is not a concern.
The problem with JDBC, that I just thought of, is that it has no explicit begin operation for a transaction, lazily starting (with autocommit=false) when a statement needs to be sent to the database. That won't always have happened before merge(entity) is called. So even though the transaction is tied to the thread for any normal usage, it won't yet be active in some cases. So we can't use the wording I came up with and we need something else here.
There was a problem hiding this comment.
What I'm saying is that the whole concept of a "transaction being active on [a] thread" is specific to JTA transactions. It doesn't even make sense to speak in those terms for non-JTA datasources, for reactive repositories, and so on.
There was a problem hiding this comment.
I made an attempt at improving the language to allow for these other patterns.
| * <p>When using a stateful repository, all lifecycle methods and all updates | ||
| * performed on entities obtained from query methods must run within a | ||
| * transaction. A persistence context remains active for the duration of the | ||
| * transaction, after which all updates must be committed or rolled back in the | ||
| * data store, per the outcome of the transaction. In Jakarta EE environments, | ||
| * a Jakarta Transactions (JTA) transaction is used.</p> |
There was a problem hiding this comment.
No, wrong. It's just not the case that every persistence context is a JTA transaction-scoped persistence context. This is exactly the problem I was trying to explain yesterday.
There was a problem hiding this comment.
I rewrote this section in the same way I attempted to correct the Merge/Remove/Persist documentation.
|
It's important to realize that, in full generality:
|
True, but lacking a transaction, there is no boundary that Jakarta Data offers after which the application can expect their changes to be persisted to the database vs rolled back or left in pending state. So I though we decided on the call that Jakarta Data would require a transaction.
We already wrote into the Jakarta Data spec a requirement that, "A persistence context is never shared across transactions." Maybe that was intended to say that it is never shared across transactions that are active at the same time and make an allowance to span transactions serially? But we currently don't have that allowance in there. |
Saying that there is a transaction is not the same thing as saying that there is a direct affinity between the persistence content and the transaction.
For write operations, sure. We were talking about write operations. But that tx might be controlled by, for example, the application directly starting/ending a resource-local transaction via
Yes. |
2 participants
I decided to start with the Javadoc for explicitly stating the requirement to use stateful repositories within a transaction (except when using find to read and not update entities) because that is primarly what applications which need to know this information will be looking at.