Implements Jakarta Persistence 3.2#144
Conversation
* Updated tentative version to 4.2.0-SNAPSHOT * Updated java version to 17
* Updating project to exclude almost all java.security deprecated calls * Passes default profile tests, but fails with postgres * Fixed some non-deterministic tests that fail with postgresql
* Tested and passed XML support using postgresql-17 as target db
* Replacing string number constructors * Removing dangling SecurityContext references
* removed TestSecurityContext because it is terminally deprecated since 17 and already removed in current JDK versions * updated h2-2 test profile jdbc url to remove strict definition * updated openjpa-slice and openjpa-xmlstore pom system variables definitions * updated GH actions workflows to use test-h2-2 profiles
* Project passes tests on derby, h2-2, postgres:latest, mysql:lts, mariadb:lts
* Updated dependency version * Added API new methods to API implementation classes with methods that throw UnsupportedOperationException, except for four methods in EntityManagerImpl that required proper implementations to pass tests * Project is still passing tests on derby and postgresql, at least
* Added XML JPA 3.2 schema and definitions * Added configuration support by 3.2 version * Added SchemaManager impl and corresponding methods in BrokerFactory interface * Added concrete working (not for h2-2) implementation of SchemaManager methods for JDBCBrokerFactory * Added concrete working impl for EMF#getName()
* Reverting unnecessary changes * Fixing broken map synchronization
* Changing signature of BrokerFactory API on schema dealing validate method * Adding test to check if validate operation throws exception when it fails * Changing GH CI workflow to allow usage of both self-hosted and GH hosted runners * Tested on derby, h2-2, postgresql:latest, mysql:lts, mariadb:lts
* Implementing emf creation passing PersistenceConfiguration
* Removing unused import in BrokerImpl * Implemented new PersistenceUnitUtil load methods
* Moved PUU loading tests to test unit already present * Updated test unit to junit 4.x format
|
Hello @rzo1, All: I was able to fix my I'll continue my testing and will report back |
|
[status update] I was able to fix one of |
| return Collections.unmodifiableCollection(_brokers); | ||
| } | ||
|
|
||
There was a problem hiding this comment.
IMO trailing spaces need to be removed :)
There was a problem hiding this comment.
Same as below (no rights on the branch). Feel free.
| tableLengthIncludesSchema); | ||
| buf.append("CREATE TABLE ").append(tableName); | ||
| if (supportsComments && table.hasComment()) { | ||
| buf.append(" "); |
There was a problem hiding this comment.
I remember it had something to do with spec compliant schema generation but it's from 3 months ago but we can restore it (no rights on that branch though). So feel free to do whatever it needed.
… are addressed (#150) * [OPENJPA-2940] Reserved words are better handled * Tests are fixed; Some maven warnings are addressed
…ecause it uses tinyint as bool
|
Hello all, little status update: all I'm trying to fix Oracle :) |
… of openjpa tree; Automatic test script is added
|
I'll start another thread :) I would like to propose this script https://github.com/apache/openjpa/blob/9da3e207072a3b5cf553e8a7f17cd41f609839bc/scripts/complex_test.sh IMO it is better because:
WDYT? |
|
Ok for me. |
… MySQL because it uses tinyint as bool" This reverts commit 8d1befb. Skipping TestJDBCSchemaManager#testValidate is not necessary when jdbc connection to mysql uses configuration property transformedBitIsBoolean=true
|
@ilgrosso for whatever reason this Could you please take a look at it? |
@solomax I took a quick look but could not figure out how that test might be unstable; actually, I was able to track down a single failure by looking at GitHub workflow runs. Since it is a mocked test, I can only suppose that there could be some dependency upgrade in this PR which is interfering somehow. |
|
Another update: |
@eolivelli maybe you would like to fix |
| // For DELETE actions, tolerate count=0 because the row may | ||
| // have already been removed by a database-level ON DELETE | ||
| // CASCADE triggered by a related row's deletion. | ||
| if (count == 0 && row.getAction() == Row.ACTION_DELETE) { |
There was a problem hiding this comment.
is this by spec? how do I know my delete was a noop as a caller now?
There was a problem hiding this comment.
Good catch - I think as written this is too broad.
This was added for two TCK tests (persistMX1Test2, cascadeAllMX1Test2) where a parent and child are removed in the same tx and the FK has a DB-level ON DELETE CASCADE. OpenJPA flushes the parent DELETE first, the DB cascades and removes the child row, and then OpenJPA's own DELETE of the child returns count == 0. Before this change that 0 always produced a spurious OptimisticException, because RowManagerImpl.getRow() sets a non-null failed object on essentially every managed row; so the old code couldn't tell "benign already-gone" from a real conflict either.
But you're right on the spec point (just checked again): count == 0 on a DELETE also legitimately means an optimistic-lock failure. For a @Version entity the SQL is DELETE .. WHERE id = ? AND version = ?, and 0 rows there is a concurrent update/delete that the spec says must surface as OptimisticLockException.
As currently written we'd swallow that, and the caller loses the only signal we had - exactly your "how do I know it was a no-op" concern. The TCK entities here are non-versioned, which is why it slipped through.
We could gate/check for an entity having no version field; tolerate count == 0 on DELETE only when there's no @Version (so there's no optimistic signal to honor), and otherwise keep the existing OptimisticException path for flushAndUpdate, flushSingleRow, and checkUpdateCount case 0.
| OpenJPAStateManager sm = (OpenJPAStateManager) pc.pcGetStateManager(); | ||
| ClassMapping cm = | ||
| (ClassMapping) _conf.getMetaDataRepositoryInstance().getCachedMetaData(pc.getClass()); | ||
| if (sm == null) { |
There was a problem hiding this comment.
wonder if these null cases shouldn't throw, means enhancement is broken or setup (agent) is broken no?
There was a problem hiding this comment.
I would already have thrown ClassCastException at the (PersistenceCapable) o cast on line 465 if there is broken enhancement, no?
| // For DELETE actions, tolerate count=0 because the row may | ||
| // have already been removed by a database-level ON DELETE | ||
| // CASCADE triggered by a related row's deletion. | ||
| if (count == 0 && row.getAction() == Row.ACTION_DELETE) { |
There was a problem hiding this comment.
same, to review if we don't loop a case there
…e addressed; dependabot was dropped
|
|
||
| try (Statement stmt = conn.createStatement()) { | ||
| stmt.executeUpdate("ALTER TABLE UUIDEntity DROP COLUMN value_"); | ||
| stmt.executeUpdate("ALTER TABLE UUIDEntity ADD " + (dict instanceof OracleDictionary ? "" : "COLUMN") + " value_ BOOLEAN DEFAULT FALSE"); |
There was a problem hiding this comment.
The reason validate() stops throwing once you swap BOOLEAN → INT is MappingInfo.mergeColumn(). When the live column is numeric (INT → Oracle NUMBER → reflected as NUMERIC) and the mapping expects VARCHAR, there's a branch that silently upgrades numeric → VARCHAR instead of reporting drift. That branch was added for the @MapKeyEnumerated ORDINAL/STRING shared-column case, so it's intentional but it also defeats validate()'s drift detection here.
It's the same thin already documented for PostgreSQL in PostgresDictionary.newColumn() (PG reports bool as Types.BIT, which is "numeric" to that code
too) -> there we remap bool → Types.BOOLEAN so the drift is seen again.
We can't reuse that trick on Oracle: pre-23c there's no native BOOLEAN, and an INT column genuinely is a NUMBER, so numeric-vs-VARCHAR is exactly the case the code is built to tolerate.
So rather than a version guard, we could make the test induce a drift that's genuinely incompatible with the String → VARCHAR2 mapping on every Oracle version: replace the column with a BLOB on Oracle (keep BOOLEAN elsewhere). OracleDictionary.getColumns() reports BLOB as Types.BLOB, which is neither varchar- nor numeric-compatible, so validate() throws as expected. Something like:
boolean isOracle = dict instanceof OracleDictionary;
stmt.executeUpdate("ALTER TABLE UUIDEntity DROP COLUMN value_");
if (isOracle) {
// Oracle has no native BOOLEAN before 23c, and an INT/NUMBER replacement
// is silently coerced to VARCHAR by MappingInfo.mergeColumn (the numeric<->varchar
// tolerance added for @MapKeyEnumerated), so validate() wouldn't detect the drift.
// BLOB is genuinely incompatible with the String -> VARCHAR mapping on every Oracle version.
stmt.executeUpdate("ALTER TABLE UUIDEntity ADD value_ BLOB");
} else {
stmt.executeUpdate("ALTER TABLE UUIDEntity ADD COLUMN value_ BOOLEAN DEFAULT FALSE");
}
The alternative is to fix numeric↔varchar coercion in mergeColumn on adapt; coerce when building/adapting a schema (adapt=true), but throw when validating (adapt=false). Then validate() becomes strict on every DB, the Oracle INT drift is correctly detected, and the test needs no DB-specific branch at all. Something in that way maybe? wdyt?
There was a problem hiding this comment.
When I was checking the issue with MySQL, I found the issue emerging from the @MapKeyEnumerated treatment, but the fix only for that case demands some structural changes. Maybe the proposed change also avoids the need of a external parameter on jdbc connecto for mysql.
There was a problem hiding this comment.
Million thanks for clarifications @rzo1 :))
This is something I suspect but was unsure how to find it in the code ...
I'll keep BLOB-for-everyone due to it will simplify test code (native query might be the same for all DBs)
and will add comments, something like this:
stmt.executeUpdate("ALTER TABLE UUIDEntity DROP COLUMN value_");
// this test originally has BOOLEAN column but in addition of different 'ALTER TABLE' syntax
// Oracle has no native BOOLEAN before 23c, and an INT/NUMBER replacement
// is silently coerced to VARCHAR by MappingInfo.mergeColumn (the numeric<->varchar
// tolerance added for @MapKeyEnumerated), so validate() wouldn't detect the drift.
// BLOB is genuinely incompatible with the String -> VARCHAR mapping on every Oracle version.
stmt.executeUpdate("ALTER TABLE UUIDEntity ADD " + (dict instanceof OracleDictionary ? "" : "COLUMN") + " value_ BLOB");
Long n = (Long) em
.createNativeQuery("SELECT COUNT(1) FROM UUIDEntity WHERE id_ = ? AND value_ IS NULL", Long.class)
.setParameter(1, ue1.getId())
.getSingleResult();
Please let me know if this test might be further improved :)
Meanwhile I'll start huge Oracle test (I'm afraid it will require days on my laptop :((( )
I'll report back here
5 participants
Hi! This work is an effort to implement JPA 3.2. I've started it a long time ago. Richard Zowalla picked it up and, with AI help (Claude), implemented the missing features, including some JPA <3.0 that weren't implemented.
I've tested it against default database, h2, mariadb(lts) e postgresql (18).
Please, check it against your favorite DB so we may fix some edge cases. It would be great if you can run TCK to be sure of the implementations.