From cff73f127a835f453bf3f261019eb20ff6ec125c Mon Sep 17 00:00:00 2001 From: Richard Zowalla Date: Sat, 29 Aug 2026 19:52:14 +0200 Subject: [PATCH 1/2] [OPENJPA-2967] Make VERSION() work for surrogate versions VersionVal.getType() returned null for a type whose version is mapped by the version strategy rather than by a @Version field. initialize() admits such a type on purpose, since its version columns exist, so the null escaped into JPQLExpressionBuilder (type.isPrimitive() for SELECT VERSION(e)) and into Filters.wrap (for WHERE VERSION(e) = :v), both of which failed with a NullPointerException. getType() now falls back to Object.class, the same answer the in-memory VersionVal gives, and toDataStoreValue() converts through the version column's java type. initialize() also reported an invalid VERSION() argument with the bad-getobjectid message, which describes an object id failure and whose argument is null for a bare identification variable. It now uses a dedicated message naming the path. Known limitations, each worth its own issue: a multi-column surrogate version still compares and loads through the first column only, and a subclass of a versioned entity is rejected because SuperclassVersionStrategy maps no columns of its own. --- .../openjpa/jdbc/kernel/exps/VersionVal.java | 53 +++++++++++---- .../jdbc/kernel/exps/localizer.properties | 2 + .../functions/SurrogateVersionEntity.java | 64 +++++++++++++++++++ .../functions/TestVersionFunctionOrderBy.java | 63 +++++++++++++++++- 4 files changed, 169 insertions(+), 13 deletions(-) create mode 100644 openjpa-persistence-jdbc/src/test/java/org/apache/openjpa/persistence/jpql/functions/SurrogateVersionEntity.java diff --git a/openjpa-jdbc/src/main/java/org/apache/openjpa/jdbc/kernel/exps/VersionVal.java b/openjpa-jdbc/src/main/java/org/apache/openjpa/jdbc/kernel/exps/VersionVal.java index 2b9e9516b8..f4d471a6d0 100644 --- a/openjpa-jdbc/src/main/java/org/apache/openjpa/jdbc/kernel/exps/VersionVal.java +++ b/openjpa-jdbc/src/main/java/org/apache/openjpa/jdbc/kernel/exps/VersionVal.java @@ -31,6 +31,7 @@ import org.apache.openjpa.lib.util.Localizer; import org.apache.openjpa.meta.ClassMetaData; import org.apache.openjpa.meta.FieldMetaData; +import org.apache.openjpa.meta.JavaTypes; import org.apache.openjpa.util.UserException; /** @@ -78,11 +79,19 @@ public void setMetaData(ClassMetaData meta) { @Override public Class getType() { - FieldMetaData versionField = _path.getMetaData().getVersionField(); - if (versionField != null) { - return versionField.getType(); - } - return null; + // note: getColumns()/initialize() resolve the target type from the + // ExpState; getType() has none, so it uses the path's class. + ClassMetaData meta = _path.getMetaData(); + FieldMetaData versionField = (meta == null) ? null : meta.getVersionField(); + if (versionField != null) + return versionField.getType(); + + // surrogate version: the version strategy maps column(s) but there is + // no version field, so no java type is declared here. Type the value + // loosely, exactly as the in-memory VersionVal does, rather than + // returning null: callers dereference this (result shapes, comparison + // type checks, Filters.convert). + return Object.class; } @Override @@ -98,23 +107,43 @@ public ExpState initialize(Select sel, ExpContext ctx, int flags) { // non-pc fields at all ClassMapping cls = _path.getClassMapping(state); if (cls == null || cls.getEmbeddingMapping() != null) - throw new UserException(_loc.get("bad-getobjectid", _path.getFieldMapping(state))); + throw new UserException(_loc.get("bad-version-path", pathDescription())); // types that are not versioned have no version columns to select, // group, order or compare by; fail with a meaningful message rather // than a NullPointerException further down the line - if (cls.getVersion() == null || cls.getVersion().getColumns().length == 0) + if (cls.getVersion().getColumns().length == 0) throw new UserException(_loc.get("no-version-field", cls)); return state; } + /** + * A user-recognizable description of the VERSION() argument, for error + * messages. + */ + private String pathDescription() { + String desc = _path.getPCPathString(); + if (desc != null && desc.endsWith(".")) + desc = desc.substring(0, desc.length() - 1); + if (desc != null && desc.length() > 0) + return desc; + String alias = _path.getSchemaAlias(); + return (alias != null) ? alias : String.valueOf(_path.getMetaData()); + } + @Override public Object toDataStoreValue(Select sel, ExpContext ctx, ExpState state, Object val) { - ClassMapping mapping = _path.getClassMapping(state); - if (mapping.getVersion() != null) { - return Filters.convert(val, getType()); - } - return null; + ClassMapping cls = _path.getClassMapping(state); + FieldMetaData versionField = cls.getVersionField(); + if (versionField != null) + return Filters.convert(val, versionField.getType()); + + // surrogate version: convert using the version column's java type, + // which the version strategy stamped onto the column + Column[] cols = cls.getVersion().getColumns(); + if (cols.length == 1) + return JavaTypes.convert(val, cols[0].getJavaType()); + return val; } @Override diff --git a/openjpa-jdbc/src/main/resources/org/apache/openjpa/jdbc/kernel/exps/localizer.properties b/openjpa-jdbc/src/main/resources/org/apache/openjpa/jdbc/kernel/exps/localizer.properties index cdf5b2070e..d61ae128b5 100644 --- a/openjpa-jdbc/src/main/resources/org/apache/openjpa/jdbc/kernel/exps/localizer.properties +++ b/openjpa-jdbc/src/main/resources/org/apache/openjpa/jdbc/kernel/exps/localizer.properties @@ -17,6 +17,8 @@ bad-getobjectid: Attempt to get the object id of a non-persistent or \ embedded object field "{0}". +bad-version-path: Attempt to obtain the version of a non-persistent or \ + embedded object "{0}". no-version-field: Attempt to obtain the version of type "{0}", which does \ not have a version field or version column(s). non-pers-field: Field "{0}" is not persistent, and thus cannot be queried. diff --git a/openjpa-persistence-jdbc/src/test/java/org/apache/openjpa/persistence/jpql/functions/SurrogateVersionEntity.java b/openjpa-persistence-jdbc/src/test/java/org/apache/openjpa/persistence/jpql/functions/SurrogateVersionEntity.java new file mode 100644 index 0000000000..e79e9b41b9 --- /dev/null +++ b/openjpa-persistence-jdbc/src/test/java/org/apache/openjpa/persistence/jpql/functions/SurrogateVersionEntity.java @@ -0,0 +1,64 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.openjpa.persistence.jpql.functions; + +import jakarta.persistence.Entity; +import jakarta.persistence.Id; +import jakarta.persistence.Table; + +import org.apache.openjpa.persistence.jdbc.VersionColumn; + +/** + * An entity with a surrogate version: the version strategy maps a version + * column, but the type has no version field. + */ +@Entity +@Table(name = "SURVERENT") +@VersionColumn +public class SurrogateVersionEntity { + + @Id + private int id; + + private String name; + + public SurrogateVersionEntity() { + } + + public SurrogateVersionEntity(int id, String name) { + this.id = id; + this.name = name; + } + + public int getId() { + return id; + } + + public void setId(int id) { + this.id = id; + } + + public String getName() { + return name; + } + + public void setName(String name) { + this.name = name; + } +} diff --git a/openjpa-persistence-jdbc/src/test/java/org/apache/openjpa/persistence/jpql/functions/TestVersionFunctionOrderBy.java b/openjpa-persistence-jdbc/src/test/java/org/apache/openjpa/persistence/jpql/functions/TestVersionFunctionOrderBy.java index bcd86a86b3..dd03b88650 100644 --- a/openjpa-persistence-jdbc/src/test/java/org/apache/openjpa/persistence/jpql/functions/TestVersionFunctionOrderBy.java +++ b/openjpa-persistence-jdbc/src/test/java/org/apache/openjpa/persistence/jpql/functions/TestVersionFunctionOrderBy.java @@ -36,13 +36,15 @@ public class TestVersionFunctionOrderBy extends SQLListenerTestCase { @Override public void setUp() { - setUp(CLEAR_TABLES, CompUser.class, CompVerUser.class, Address.class); + setUp(CLEAR_TABLES, CompUser.class, CompVerUser.class, Address.class, + SurrogateVersionEntity.class); EntityManager em = emf.createEntityManager(); em.getTransaction().begin(); em.persist(new CompVerUser("Bob", "n/a", null, 21)); em.persist(new CompVerUser("Alice", "n/a", null, 22)); em.persist(new CompUser("Ugo", "n/a", null, 23)); + em.persist(new SurrogateVersionEntity(1, "Surro")); em.getTransaction().commit(); em.close(); } @@ -107,6 +109,65 @@ public void testVersionOnUnversionedTypeThrowsUserException() { msg.contains(CompUser.class.getName())); assertFalse("Should not fail with a NullPointerException: " + msg, hasNPE(e)); + assertFalse("VERSION() must not report an object-id failure: " + msg, + msg.contains("object id")); + } finally { + em.close(); + } + } + + public void testProjectVersionOfSurrogateVersionedType() { + EntityManager em = emf.createEntityManager(); + resetSQL(); + + List result = em.createQuery( + "SELECT VERSION(e) FROM SurrogateVersionEntity e").getResultList(); + + assertEquals(1, result.size()); + assertNotNull(result.get(0)); + assertContainsSQL("versn"); + em.close(); + } + + public void testWhereVersionOnSurrogateVersionedType() { + EntityManager em = emf.createEntityManager(); + resetSQL(); + + List result = em.createQuery( + "SELECT e FROM SurrogateVersionEntity e WHERE VERSION(e) = :v") + .setParameter("v", 1).getResultList(); + + assertEquals(1, result.size()); + assertContainsSQL("versn = ?"); + em.close(); + } + + public void testOrderByVersionOnSurrogateVersionedType() { + EntityManager em = emf.createEntityManager(); + resetSQL(); + + List result = em.createQuery( + "SELECT e FROM SurrogateVersionEntity e ORDER BY VERSION(e)").getResultList(); + + // the version column is selected with the entity anyway, so assert on + // the ORDER BY clause itself rather than on the column name alone + assertEquals(1, result.size()); + assertContainsSQL("ORDER BY"); + assertContainsSQL("ORDER BY t0.versn"); + em.close(); + } + + public void testProjectVersionOnUnversionedTypeThrowsUserException() { + EntityManager em = emf.createEntityManager(); + try { + em.createQuery("SELECT VERSION(u) FROM CompUser u").getResultList(); + fail("VERSION() on an unversioned type should be rejected"); + } catch (RuntimeException e) { + String msg = getNestedMessages(e); + assertTrue("Unexpected failure: " + msg, + msg.contains("does not") && msg.contains("version")); + assertFalse("Should not fail with a NullPointerException: " + msg, + hasNPE(e)); } finally { em.close(); } From 2a8b0e24ef6180926390896ca7d6408403854389 Mon Sep 17 00:00:00 2001 From: Richard Zowalla Date: Mon, 31 Aug 2026 21:14:41 +0200 Subject: [PATCH 2/2] [OPENJPA-2967] Brace the blocks touched by this change Review feedback. --- .../openjpa/jdbc/kernel/exps/VersionVal.java | 21 ++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/openjpa-jdbc/src/main/java/org/apache/openjpa/jdbc/kernel/exps/VersionVal.java b/openjpa-jdbc/src/main/java/org/apache/openjpa/jdbc/kernel/exps/VersionVal.java index f4d471a6d0..84809e45fc 100644 --- a/openjpa-jdbc/src/main/java/org/apache/openjpa/jdbc/kernel/exps/VersionVal.java +++ b/openjpa-jdbc/src/main/java/org/apache/openjpa/jdbc/kernel/exps/VersionVal.java @@ -83,8 +83,9 @@ public Class getType() { // ExpState; getType() has none, so it uses the path's class. ClassMetaData meta = _path.getMetaData(); FieldMetaData versionField = (meta == null) ? null : meta.getVersionField(); - if (versionField != null) + if (versionField != null) { return versionField.getType(); + } // surrogate version: the version strategy maps column(s) but there is // no version field, so no java type is declared here. Type the value @@ -106,14 +107,16 @@ public ExpState initialize(Select sel, ExpContext ctx, int flags) { // without screwing up the SQL, to just don't let users call it on // non-pc fields at all ClassMapping cls = _path.getClassMapping(state); - if (cls == null || cls.getEmbeddingMapping() != null) + if (cls == null || cls.getEmbeddingMapping() != null) { throw new UserException(_loc.get("bad-version-path", pathDescription())); + } // types that are not versioned have no version columns to select, // group, order or compare by; fail with a meaningful message rather // than a NullPointerException further down the line - if (cls.getVersion().getColumns().length == 0) + if (cls.getVersion().getColumns().length == 0) { throw new UserException(_loc.get("no-version-field", cls)); + } return state; } @@ -123,10 +126,12 @@ public ExpState initialize(Select sel, ExpContext ctx, int flags) { */ private String pathDescription() { String desc = _path.getPCPathString(); - if (desc != null && desc.endsWith(".")) + if (desc != null && desc.endsWith(".")) { desc = desc.substring(0, desc.length() - 1); - if (desc != null && desc.length() > 0) + } + if (desc != null && desc.length() > 0) { return desc; + } String alias = _path.getSchemaAlias(); return (alias != null) ? alias : String.valueOf(_path.getMetaData()); } @@ -135,14 +140,16 @@ private String pathDescription() { public Object toDataStoreValue(Select sel, ExpContext ctx, ExpState state, Object val) { ClassMapping cls = _path.getClassMapping(state); FieldMetaData versionField = cls.getVersionField(); - if (versionField != null) + if (versionField != null) { return Filters.convert(val, versionField.getType()); + } // surrogate version: convert using the version column's java type, // which the version strategy stamped onto the column Column[] cols = cls.getVersion().getColumns(); - if (cols.length == 1) + if (cols.length == 1) { return JavaTypes.convert(val, cols[0].getJavaType()); + } return val; }