JPA specialization [Proposal] #140

otaviojava · 2023-05-04T08:44:41Z

Hey everyone, this is one proposal.

I was concerned about missing the DDD repository's core idea during the last meeting.
Our API lost encapsulation and has several JPA details such as transaction, SQL, JPQL, JTA, etc.

I propose to create a subdomain JPA where the ideas around JPA can move there and keep the CrudDataRepository the most generic to work to any persistence layer engine, such as NoSQL types, web services, and so on.

A relational database is the most advanced and mature database. It does not make sense to wai others simultaneously; the CrudRepository should be more generic to work with any persistence layer. Thus, moving JPA forward without impacting the other types is an excellent benefit.

Please, let me know if it makes sense; it still left a lot of work on documentation, but first, let me know if this proposal makes sense.

⚠️ It is unfinished work, the idea is to show the vision with some code, so it will be easier to understand.

@JPARepository (dataStore="JNDI-REFERENCE")
public interface Airport extends JpaCrudRepository<AirPlane, Id>{
}

Some market references:

Signed-off-by: Otavio Santana <[email protected]>

KyleAure

Overall, I do not agree with this change.
I thought the point of Jakarta Data was to have a persistence layer that was agnostic (in your words) to the underlying database. If we separate off Persistence repositories into their own package then as a vendor or user what benefit do I get out of Jakarta Data? I could just switch and use Jakarta Persistence.

The whole point of Jakarta Data is that I can write my application using a Data Repository. Right now I want my underlying database to be Oracle, but in the future, I might want to switch to use MongoDB without having to do a massive refactor of my application.

This change would break that goal and make this specification useless (IMHO)

KyleAure · 2023-05-04T13:58:25Z

api/src/main/java/jakarta/data/repository/jpa/JPARepository.java

+@Documented
+@Retention(RetentionPolicy.RUNTIME)
+@Target(ElementType.TYPE)
+public @interface JPARepository {


I believe the JPA initialism is a copyright of Oracle.
You will need to change this class name to PersistenceRepository
And the package name to jakarta.data.repository.persistence

We can use it: Jakarta Persistence API!

otaviojava · 2023-05-04T14:27:55Z

@KyleAure

I thought the point of Jakarta Data was to have a persistence layer that was agnostic (in your words) to the underlying database.

We already broke it when we put a lot of transactions and JPA details on it.

Technically, I cannot affirm that all database engine has transaction and JPQL.

I'm proposing how to solve it.

Book references:

njr-11

The concept of having a Jakarta Persistence/JPA-specific extension to CrudRepository as Spring and Micronaut do could certainly be useful if done in the same manner as Spring and Micronaut, which is to add built-in repository methods that are unique to Jakarta Persistence.

The approach taken under this pull does not do that and instead appears to be a misguided attempt to group together and isolate aspects of the specification that are unfavored for NoSQL, calling these "JPA".

JPA itself is not a correct acronym in Jakarta. JPA stands for Java Persistence API, which Oracle owns as part of Java EE. In Jakarta, it is just "Jakarta Persistence". Refer to the Jakarta Persistence specification document, in which the acronym JPA only appears when referencing Java EE's Java Persistence API.

Jakarta Data and the repository pattern within are much broader than Jakarta Persistence and Jakarta NoSQL and must not be characterized as one versus the other. Beyond these, a provider might instead implement repositories on JDBC, which shares some, but not all, aspects with Jakarta Persistence. A provider could alternatively be backed by REST (I think that was in Otavio's original proposal) or really anything that accesses and persists data. A database could even skip over Jakarta NoSQL, Jakarta Persistence, JDBC and REST altogether and directly supply a Jakarta Data provider for its database to be usable in a compliant way under the specification and TCK.

We should not arbitrarily forbid such implementations from providing features like keyset pagination, JTA transactions, or multiple database support under the specification just because they aren't Jakarta Persistence. Jakarta Data should stay with its current approach of seeking to generalize and unify rather than fragment the programming model, especially where the fragmentation is inaccurate.

njr-11 · 2023-05-04T14:11:03Z

api/src/main/java/jakarta/data/repository/jpa/SQL.java

+/**
+ * Defines the SQL string as SQL, JPA-QL that should be executed.
+ */
+public @interface SQL {


JPQL is not the same as SQL. Our specification should not confuse the two.

njr-11 · 2023-05-04T14:15:23Z

api/src/main/java/jakarta/data/repository/Repository.java

-     *
-     * @return the name of a data store or {@link #DEFAULT_DATA_STORE}.
-     */
-    String dataStore() default DEFAULT_DATA_STORE;


This is not specific to Jakarta Persistence. It applies to every type of backend where there can be multiple, which is all types I'm aware of.

Usually, we're using convention over configuration allied with The Twelve-Factor App:
https://12factor.net/
E.g.:

Spring

Micronaut

Quarkus

That is why I proposed to keep it on JPA subdomain.

I don't follow this argument at all with respect to what it has to do with JPA being the deciding factor in whether or not you are able to use multiple databases.

njr-11 · 2023-05-04T14:18:28Z

api/src/main/java/jakarta/data/repository/Query.java

-     * @return a query for counting the number of elements across all pages.
-     *         Empty string indicates that no counting query is provided.
-     */
-    String count() default "";


This is not specific to Jakarta Persistence. Page counts and total element counts across all pages apply to every type of database that supports pagination.

It is not valid.
You can paginate using a stream or any "pointer" or cursor of this data.

Samples:

Cassandra

MongoDB

Neo4J Cursos

⚠️ Neo4J also have offset-based option

Right, I didn't word that very well. The point is that although not all databases can count total results, types other than Jakarta Persistence/JPA do have that ability and it is therefore wrong to restrict it to Jakarta Persistence/JPA alone. We know for a fact that JDBC can do this via SQL independently of Jakarta Persistence. Even MongoDB which you cited as a counter example exposes an API for counting documents so it is not unreasonable to allow for the possibility they might some day want to let the same thing to be done via whatever query language they support. If a count query isn't useful for a particular type that doesn't support it, then just don't use it with those types. But let it remain for all of those where it useful. It seems like you are boxing in and limiting everything that isn't Jakarta Persistence where it isn't necessary and doesn't make sense.

Ok, some databases can support count, but why not use OOP in our favor and create specializations?
It is the beauty of OOP!

You can have generic and specializations instead of having several optional methods.

Specializations should be used where they make sense - when there are categories that provide accurate delineation between where a function is applicable versus not applicable. We have been struggling to find that clear separation here. JPA vs everything else and relational vs everything else don't perfectly fit. While we can state that Jakarta Persistence will always have certain capabilities, we cannot accurately state that non-JPA will always not.

njr-11 · 2023-05-04T14:21:25Z

api/src/main/java/jakarta/data/repository/jpa/KeysetAwareSlice.java

@@ -15,7 +15,11 @@
 *
 *  SPDX-License-Identifier: Apache-2.0
 */
-package jakarta.data.repository;
+package jakarta.data.repository.jpa;


Keyset pagination is not specific to Jakarta Persistence. It also applies to JDBC and to any database type that supports the And and Or keywords. And in simple scenarios with only one sort criterion, it doesn't even need Or.

If we renamed it to "relational" is that ok?

Then it would exclude everything that isn't a relational database. That helps JDBC out, but has us arbitrarily deciding that nothing else shall support keyset pagination without regard for whether those individual databases want to or not. Seems like a bad decision.

njr-11 · 2023-05-04T14:22:05Z

api/src/main/java/jakarta/data/repository/jpa/OrderBy.java

@@ -15,7 +15,11 @@
 *
 * SPDX-License-Identifier: Apache-2.0
 */
-package jakarta.data.repository;
+package jakarta.data.repository.jpa;


Nothing about this is specific to Jakarta Persistence.

njr-11 · 2023-05-04T14:22:24Z

api/src/main/java/jakarta/data/repository/jpa/KeysetAwarePage.java

@@ -15,7 +15,10 @@
 *
 *  SPDX-License-Identifier: Apache-2.0
 */
-package jakarta.data.repository;
+package jakarta.data.repository.jpa;


Keyset pagination is not specific to Jakarta Persistence. It also applies to JDBC and to any database type that supports the And and Or keywords. And in simple scenarios with only one sort criterion, it doesn't even need Or.

As above, if we renamed the package name to "relational"

otaviojava · 2023-05-04T15:11:41Z

Jakarta Data and the repository pattern within are much broader than Jakarta Persistence and Jakarta NoSQL and must not be characterized as one versus the other. Beyond these, a provider might instead implement repositories on JDBC, which shares some, but not all, aspects with Jakarta Persistence. A provider could be backed by REST (I think that was in Otavio's original proposal) or anything that accesses and persists data. A database could even skip over Jakarta NoSQL, Jakarta Persistence, JDBC and REST altogether and directly supply a Jakarta Data provider for its database to be usable in a compliant way under the specification and TCK.

That is right! And that is our current bug 🐞

I've created an issue:

#143

My question is: the relational work that does here makes sense and I don't want to remove it, but to keep it without impacting others' engine solutions. But how?

njr-11 · 2023-05-04T15:36:07Z

We already broke it when we put a lot of transactions and JPA details on it.

Technically, I cannot affirm that all database engine has transaction and JPQL.

As we are all aware, not all databases do support transactions and JPQL. However, you have it backwards here. Defining how Jakarta Transactions and Jakarta Data specifications interact is obligatory of a well written specification in the Jakarta platform. Neglecting to define how Jakarta Transactions and Jakarta Data specifications interact is what would mean a broken specification. It would be a broken specification if we were requiring databases without transaction support to participate in transactions. But the spec does the opposite and applies requirements with respect to Jakarta Transactions only to data sources that are capable of transaction enlistment. If the language isn't clear, it can be clarified further. It's the wrong answer to say that only Jakarta Persistence-backed data sources are transactional and codify that into an API because it isn't true.

njr-11 · 2023-05-04T16:48:10Z

My question is: the relational work that does here makes sense and I don't want to remove it, but to keep it without impacting others' engine solutions. But how?

Can you point out specifically what is impacting other persistence types that you believe would prevent them from having a compliant Jakarta Data provider? We have been very intentionally about writing language into the spec that doesn't ask for enlistment into JTA transactions or features like keyset pagination (relies on Or capability) when the database doesn't have the capability. TCK tests don't exist yet, but our plan involves catching expected exceptions that indicate lack of support for Or and of course JTA-related tests will be limited to web profile and Jakarta Persistence-backed providers. I'm not aware of any actual problems that these proposed changes fix.

otaviojava · 2023-05-04T17:01:41Z

Can you point out specifically what is impacting other persistence types that you believe would prevent them from having a compliant Jakarta Data provider? We have been very intentionally about writing language into the spec that doesn't ask for enlistment into JTA transactions or features like keyset pagination (relies on Or capability) when the database doesn't have the capability. TCK tests don't exist yet, but our plan involves catching expected exceptions that indicate lack of support for Or and of course JTA-related tests will be limited to web profile and Jakarta Persistence-backed providers. I'm not aware of any actual problems that these proposed changes fix.

Ok, I'll list those, hope it will be better than this PR.

otaviojava added 8 commits May 4, 2023 09:10

feat: create JPA repository

876a198

Signed-off-by: Otavio Santana <[email protected]>

doc: update documentation at JPA repository

f28001b

Signed-off-by: Otavio Santana <[email protected]>

feat: move the whole detailsto JPArepository

1b0e6f3

Signed-off-by: Otavio Santana <[email protected]>

docs: update package-info

518f0a4

Signed-off-by: Otavio Santana <[email protected]>

refactor: move the specialization of SQL

432a310

Signed-off-by: Otavio Santana <[email protected]>

docs: update Pageable api

46af832

Signed-off-by: Otavio Santana <[email protected]>

feat: creates specialization to JPA repository here

25d34c4

Signed-off-by: Otavio Santana <[email protected]>

docs: add a todo item to repository

9e75483

Signed-off-by: Otavio Santana <[email protected]>

otaviojava requested a review from njr-11 May 4, 2023 08:44

otaviojava added 2 commits May 4, 2023 09:47

chore: add SQL license header

b7eec04

Signed-off-by: Otavio Santana <[email protected]>

docs: fix repository docs

986e870

Signed-off-by: Otavio Santana <[email protected]>

otaviojava mentioned this pull request May 4, 2023

BE agnostic to annotation and DI [TCK] #141

Closed

KyleAure requested changes May 4, 2023

View reviewed changes

njr-11 requested changes May 4, 2023

View reviewed changes

otaviojava closed this May 4, 2023

otaviojava deleted the jpa-specialization branch May 4, 2023 17:02

graemerocher mentioned this pull request Aug 30, 2023

Define the semantics of operations of CrudRepository #229

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JPA specialization [Proposal] #140

JPA specialization [Proposal] #140

otaviojava commented May 4, 2023 •

edited

Loading

KyleAure left a comment

KyleAure May 4, 2023

otaviojava May 4, 2023

otaviojava commented May 4, 2023 •

edited

Loading

njr-11 left a comment

njr-11 May 4, 2023

njr-11 May 4, 2023

otaviojava May 4, 2023 •

edited

Loading

njr-11 May 4, 2023

njr-11 May 4, 2023

otaviojava May 4, 2023

njr-11 May 4, 2023 •

edited

Loading

otaviojava May 4, 2023

njr-11 May 4, 2023

njr-11 May 4, 2023

otaviojava May 4, 2023

njr-11 May 4, 2023

njr-11 May 4, 2023

njr-11 May 4, 2023

otaviojava May 4, 2023

otaviojava commented May 4, 2023 •

edited

Loading

njr-11 commented May 4, 2023

njr-11 commented May 4, 2023

otaviojava commented May 4, 2023

JPA specialization [Proposal] #140

JPA specialization [Proposal] #140

Conversation

otaviojava commented May 4, 2023 • edited Loading

KyleAure left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

otaviojava commented May 4, 2023 • edited Loading

njr-11 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

otaviojava May 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

njr-11 May 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

otaviojava commented May 4, 2023 • edited Loading

njr-11 commented May 4, 2023

njr-11 commented May 4, 2023

otaviojava commented May 4, 2023

otaviojava commented May 4, 2023 •

edited

Loading

otaviojava commented May 4, 2023 •

edited

Loading

otaviojava May 4, 2023 •

edited

Loading

njr-11 May 4, 2023 •

edited

Loading

otaviojava commented May 4, 2023 •

edited

Loading