You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@openjpa.apache.org by rz...@apache.org on 2026/08/29 17:53:19 UTC
(openjpa) 01/01: [OPENJPA-2968] Do not swallow failures around IdClass primary key values
This is an automated email from the ASF dual-hosted git repository.
rzo1 pushed a commit to branch OPENJPA-2968
in repository https://gitbox.apache.org/repos/asf/openjpa.git
commit 618a7b6594a0334fb884c6a802e9e8b5be890dbd
Author: Richard Zowalla <rz...@apache.org>
AuthorDate: Sat Aug 29 19:49:30 2026 +0200
[OPENJPA-2968] Do not swallow failures around IdClass primary key values
The @IdClass extraction and reconstruction paths added for the JPA 2.4.1.3
example 2b case caught every exception and continued: on write the primary
key columns were filled with nulls, on read the field was left null, so a
corrupt identity could be persisted or loaded without any trace. Both paths
now raise a StoreException naming the field, the id class and the mapping,
with the original failure chained, and a null id class value on the write
path is logged as a warning.
The columns are also no longer paired with the id class fields by
Class.getDeclaredFields() order, which the JVM does not guarantee. Where
every column names its target through referencedColumnName the fields are
matched by that name; the declaration order remains only as the fallback
for mappings that provide no name.
TestDerivedIdEx2c covers the ordering: its id class declares the fields in
the order opposite to the join columns, so a find() misses on the previous
code and succeeds now.
---
.../jdbc/meta/strats/EmbedValueHandler.java | 123 +++++++++++++++------
.../openjpa/jdbc/meta/strats/localizer.properties | 8 ++
.../openjpa/persistence/embed/DID2cDependent.java | 59 ++++++++++
.../persistence/embed/DID2cDependentId.java | 57 ++++++++++
.../openjpa/persistence/embed/DID2cEmployee.java | 49 ++++++++
.../openjpa/persistence/embed/DID2cEmployeeId.java | 60 ++++++++++
.../persistence/embed/TestDerivedIdEx2c.java | 103 +++++++++++++++++
7 files changed, 425 insertions(+), 34 deletions(-)
diff --git a/openjpa-jdbc/src/main/java/org/apache/openjpa/jdbc/meta/strats/EmbedValueHandler.java b/openjpa-jdbc/src/main/java/org/apache/openjpa/jdbc/meta/strats/EmbedValueHandler.java
index 7cdb3da69..29dbc3ede 100644
--- a/openjpa-jdbc/src/main/java/org/apache/openjpa/jdbc/meta/strats/EmbedValueHandler.java
+++ b/openjpa-jdbc/src/main/java/org/apache/openjpa/jdbc/meta/strats/EmbedValueHandler.java
@@ -38,10 +38,12 @@ import org.apache.openjpa.jdbc.sql.DBDictionary;
import org.apache.openjpa.kernel.ObjectIdStateManager;
import org.apache.openjpa.kernel.OpenJPAStateManager;
import org.apache.openjpa.kernel.StateManagerImpl;
+import org.apache.openjpa.lib.log.Log;
import org.apache.openjpa.lib.util.Localizer;
import org.apache.openjpa.meta.FieldMetaData;
import org.apache.openjpa.meta.MetaDataModes;
import org.apache.openjpa.util.MetaDataException;
+import org.apache.openjpa.util.StoreException;
/**
* Base class for embedded value handlers.
@@ -216,21 +218,27 @@ public abstract class EmbedValueHandler
if (mic != null && !mic.isEmpty()) {
Object idObj = (em == null) ? null : em.fetch(i);
if (idObj != null) {
- try {
- List<java.lang.reflect.Field> df =
- getInstanceFields(idObj.getClass(), mic.size());
- for (java.lang.reflect.Field f : df) {
- Object fv = f.get(idObj);
- if (cols.length == 1) rvals.add(fv);
- else ((Object[]) rvals.get(0))[idx++] = fv;
- }
- } catch (Exception ex) {
- for (int c = 0; c < mic.size(); c++) {
- if (cols.length == 1) rvals.add(null);
- else ((Object[]) rvals.get(0))[idx++] = null;
+ List<java.lang.reflect.Field> df =
+ getIdClassFields(idObj.getClass(), mic);
+ for (java.lang.reflect.Field f : df) {
+ Object fv;
+ try {
+ fv = f.get(idObj);
+ } catch (Exception ex) {
+ // never write partial identities silently
+ throw new StoreException(_loc.get(
+ "mapsid-extract-failed", f.getName(),
+ idObj.getClass().getName(),
+ fms[i].getFullName(false))).setCause(ex);
}
+ if (cols.length == 1) rvals.add(fv);
+ else ((Object[]) rvals.get(0))[idx++] = fv;
}
} else {
+ Log log = fms[i].getRepository().getLog();
+ if (log.isWarnEnabled())
+ log.warn(_loc.get("mapsid-null-id",
+ fms[i].getFullName(false)));
for (int c = 0; c < mic.size(); c++) {
if (cols.length == 1) rvals.add(null);
else ((Object[]) rvals.get(0))[idx++] = null;
@@ -325,25 +333,37 @@ public abstract class EmbedValueHandler
if (!(fm.getStrategy() instanceof Embeddable)) {
List<Column> mic = fm.getValueInfo().getMapsIdColumns();
if (mic != null && !mic.isEmpty()) {
+ Object idObj;
try {
- Object idObj = fm.getDeclaredType()
+ idObj = fm.getDeclaredType()
.getDeclaredConstructor().newInstance();
- List<java.lang.reflect.Field> df =
- getInstanceFields(fm.getDeclaredType(), mic.size());
- int ci = 0;
- for (java.lang.reflect.Field f : df) {
- Object cv;
- if (val instanceof Object[])
- cv = ((Object[]) val)[idx + ci];
- else
- cv = val;
+ } catch (Exception ex) {
+ throw new StoreException(_loc.get(
+ "mapsid-instantiate-failed",
+ fm.getDeclaredType().getName(),
+ fm.getFullName(false))).setCause(ex);
+ }
+ List<java.lang.reflect.Field> df =
+ getIdClassFields(fm.getDeclaredType(), mic);
+ int ci = 0;
+ for (java.lang.reflect.Field f : df) {
+ Object cv;
+ if (val instanceof Object[])
+ cv = ((Object[]) val)[idx + ci];
+ else
+ cv = val;
+ try {
f.set(idObj, cv);
- ci++;
+ } catch (Exception ex) {
+ // never load a partial identity silently
+ throw new StoreException(_loc.get(
+ "mapsid-reconstruct-failed", f.getName(),
+ fm.getDeclaredType().getName(),
+ fm.getFullName(false))).setCause(ex);
}
- em.store(fm.getIndex(), idObj);
- } catch (Exception ex) {
- // field stays null
+ ci++;
}
+ em.store(fm.getIndex(), idObj);
idx += mic.size();
}
continue;
@@ -442,20 +462,55 @@ public abstract class EmbedValueHandler
}
/**
- * Returns up to {@code limit} non-static declared fields from the class,
- * each made accessible. Used for @IdClass POJO field reflection.
+ * Returns the @IdClass POJO fields backing the given MapsId columns, in
+ * column order, each made accessible.
+ * <p>
+ * A column names its target through <code>@JoinColumn.referencedColumnName</code>;
+ * where every column does so, the fields are matched by that name. Otherwise the
+ * fields are taken in declaration order, which is all the mapping offers even
+ * though {@link Class#getDeclaredFields} does not guarantee it.
*/
- private static List<java.lang.reflect.Field> getInstanceFields(
- Class<?> cls, int limit) {
- List<java.lang.reflect.Field> result = new ArrayList<>();
+ private static List<java.lang.reflect.Field> getIdClassFields(
+ Class<?> cls, List<Column> mic) {
+ List<java.lang.reflect.Field> declared = new ArrayList<>();
for (java.lang.reflect.Field f : cls.getDeclaredFields()) {
if (java.lang.reflect.Modifier.isStatic(f.getModifiers()))
continue;
- if (result.size() >= limit) break;
f.setAccessible(true);
- result.add(f);
+ declared.add(f);
+ }
+
+ List<java.lang.reflect.Field> byName = new ArrayList<>(mic.size());
+ for (Column col : mic) {
+ DBIdentifier target = col.getTargetIdentifier();
+ java.lang.reflect.Field match = DBIdentifier.isEmpty(target)
+ ? null : findField(declared, target.getName());
+ if (match == null) {
+ byName = null;
+ break;
+ }
+ byName.add(match);
}
- return result;
+ if (byName != null)
+ return byName;
+
+ List<java.lang.reflect.Field> positional = new ArrayList<>(mic.size());
+ for (java.lang.reflect.Field f : declared) {
+ if (positional.size() >= mic.size()) break;
+ positional.add(f);
+ }
+ return positional;
+ }
+
+ private static java.lang.reflect.Field findField(
+ List<java.lang.reflect.Field> fields, String name) {
+ for (java.lang.reflect.Field f : fields)
+ if (f.getName().equals(name))
+ return f;
+ for (java.lang.reflect.Field f : fields)
+ if (f.getName().equalsIgnoreCase(name))
+ return f;
+ return null;
}
}
diff --git a/openjpa-jdbc/src/main/resources/org/apache/openjpa/jdbc/meta/strats/localizer.properties b/openjpa-jdbc/src/main/resources/org/apache/openjpa/jdbc/meta/strats/localizer.properties
index 934489ab9..bc34c777b 100644
--- a/openjpa-jdbc/src/main/resources/org/apache/openjpa/jdbc/meta/strats/localizer.properties
+++ b/openjpa-jdbc/src/main/resources/org/apache/openjpa/jdbc/meta/strats/localizer.properties
@@ -156,3 +156,11 @@ unknown-enumerated-value: The database value "{0}" does not map to any \
enumerated-value-bad-type: The @EnumeratedValue field "{1}" of enum type \
"{0}" has the unsupported type "{2}". Only String, char, Character and \
the integral types byte, short, int, long and their wrappers can be used.
+mapsid-extract-failed: Could not read field "{0}" of id class "{1}" while \
+ writing the primary key columns of "{2}".
+mapsid-reconstruct-failed: Could not set field "{0}" of id class "{1}" while \
+ reading the primary key columns of "{2}".
+mapsid-instantiate-failed: Could not instantiate id class "{0}" while reading \
+ the primary key columns of "{1}". It must have a public no-arg constructor.
+mapsid-null-id: The id class value of "{0}" is null; its primary key columns \
+ are written as null.
diff --git a/openjpa-persistence-jdbc/src/test/java/org/apache/openjpa/persistence/embed/DID2cDependent.java b/openjpa-persistence-jdbc/src/test/java/org/apache/openjpa/persistence/embed/DID2cDependent.java
new file mode 100644
index 000000000..c9d42c101
--- /dev/null
+++ b/openjpa-persistence-jdbc/src/test/java/org/apache/openjpa/persistence/embed/DID2cDependent.java
@@ -0,0 +1,59 @@
+/*
+ * 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.embed;
+
+import java.io.Serializable;
+
+import jakarta.persistence.EmbeddedId;
+import jakarta.persistence.Entity;
+import jakarta.persistence.JoinColumn;
+import jakarta.persistence.JoinColumns;
+import jakarta.persistence.ManyToOne;
+import jakarta.persistence.MapsId;
+import jakarta.persistence.Table;
+
+/**
+ * Dependent with @EmbeddedId + @MapsId targeting a non-@Embeddable IdClass
+ * field whose members are declared in the reverse of the join column order.
+ */
+@Entity
+@Table(name = "DID2C_DEP")
+public class DID2cDependent implements Serializable {
+ private static final long serialVersionUID = 1L;
+
+ @EmbeddedId
+ DID2cDependentId id;
+
+ @MapsId("empPK")
+ @JoinColumns({
+ @JoinColumn(name = "FK_FIRSTNAME", referencedColumnName = "firstName"),
+ @JoinColumn(name = "FK_LASTNAME", referencedColumnName = "lastName")
+ })
+ @ManyToOne
+ DID2cEmployee emp;
+
+ public DID2cDependent() {}
+ public DID2cDependent(DID2cDependentId did, DID2cEmployee e) {
+ this.id = did;
+ this.emp = e;
+ }
+ public DID2cDependentId getId() { return id; }
+ public DID2cEmployee getEmp() { return emp; }
+ public void setEmp(DID2cEmployee e) { emp = e; }
+}
diff --git a/openjpa-persistence-jdbc/src/test/java/org/apache/openjpa/persistence/embed/DID2cDependentId.java b/openjpa-persistence-jdbc/src/test/java/org/apache/openjpa/persistence/embed/DID2cDependentId.java
new file mode 100644
index 000000000..3bf689f03
--- /dev/null
+++ b/openjpa-persistence-jdbc/src/test/java/org/apache/openjpa/persistence/embed/DID2cDependentId.java
@@ -0,0 +1,57 @@
+/*
+ * 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.embed;
+
+import java.io.Serializable;
+
+import jakarta.persistence.Embeddable;
+
+/**
+ * EmbeddedId for DID2cDependent. Contains a non-@Embeddable IdClass field.
+ */
+@Embeddable
+public class DID2cDependentId implements Serializable {
+ private static final long serialVersionUID = 1L;
+
+ String name;
+ DID2cEmployeeId empPK;
+
+ public DID2cDependentId() {}
+ public DID2cDependentId(String n, DID2cEmployeeId e) { name = n; empPK = e; }
+ public String getName() { return name; }
+ public void setName(String n) { name = n; }
+ public DID2cEmployeeId getEmpPK() { return empPK; }
+ public void setEmpPK(DID2cEmployeeId e) { empPK = e; }
+
+ @Override
+ public boolean equals(Object o) {
+ if (!(o instanceof DID2cDependentId)) return false;
+ DID2cDependentId x = (DID2cDependentId) o;
+ return eq(name, x.name) && eq(empPK, x.empPK);
+ }
+
+ @Override
+ public int hashCode() {
+ return (name != null ? name.hashCode() : 0);
+ }
+
+ private static boolean eq(Object a, Object b) {
+ return a == b || (a != null && a.equals(b));
+ }
+}
diff --git a/openjpa-persistence-jdbc/src/test/java/org/apache/openjpa/persistence/embed/DID2cEmployee.java b/openjpa-persistence-jdbc/src/test/java/org/apache/openjpa/persistence/embed/DID2cEmployee.java
new file mode 100644
index 000000000..3ca617cad
--- /dev/null
+++ b/openjpa-persistence-jdbc/src/test/java/org/apache/openjpa/persistence/embed/DID2cEmployee.java
@@ -0,0 +1,49 @@
+/*
+ * 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.embed;
+
+import java.io.Serializable;
+
+import jakarta.persistence.Entity;
+import jakarta.persistence.Id;
+import jakarta.persistence.IdClass;
+import jakarta.persistence.Table;
+
+/**
+ * Employee with composite @IdClass PK whose id class declares its members in
+ * an order that does not match the join column order (JPA 2.4.1.3 ex2b).
+ */
+@Entity
+@Table(name = "DID2C_EMP")
+@IdClass(DID2cEmployeeId.class)
+public class DID2cEmployee implements Serializable {
+ private static final long serialVersionUID = 1L;
+ @Id String firstName;
+ @Id String lastName;
+
+ public DID2cEmployee() {}
+ public DID2cEmployee(DID2cEmployeeId eid) {
+ this.firstName = eid.getFirstName();
+ this.lastName = eid.getLastName();
+ }
+ public String getFirstName() { return firstName; }
+ public void setFirstName(String fn) { firstName = fn; }
+ public String getLastName() { return lastName; }
+ public void setLastName(String ln) { lastName = ln; }
+}
diff --git a/openjpa-persistence-jdbc/src/test/java/org/apache/openjpa/persistence/embed/DID2cEmployeeId.java b/openjpa-persistence-jdbc/src/test/java/org/apache/openjpa/persistence/embed/DID2cEmployeeId.java
new file mode 100644
index 000000000..d677d9f07
--- /dev/null
+++ b/openjpa-persistence-jdbc/src/test/java/org/apache/openjpa/persistence/embed/DID2cEmployeeId.java
@@ -0,0 +1,60 @@
+/*
+ * 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.embed;
+
+import java.io.Serializable;
+
+/**
+ * IdClass for DID2cEmployee. NOT @Embeddable.
+ *
+ * Identical to DID2bEmployeeId except that the members are declared in the
+ * REVERSE order of the @JoinColumns of DID2cDependent, so that mapping the
+ * @MapsId columns by declaration order instead of by name is observable.
+ */
+public class DID2cEmployeeId implements Serializable {
+ private static final long serialVersionUID = 1L;
+
+ // deliberately declared in the opposite order of the join columns
+ String lastName;
+ String firstName;
+
+ public DID2cEmployeeId() {}
+ public DID2cEmployeeId(String fn, String ln) { firstName = fn; lastName = ln; }
+ public String getFirstName() { return firstName; }
+ public void setFirstName(String fn) { firstName = fn; }
+ public String getLastName() { return lastName; }
+ public void setLastName(String ln) { lastName = ln; }
+
+ @Override
+ public boolean equals(Object o) {
+ if (!(o instanceof DID2cEmployeeId)) return false;
+ DID2cEmployeeId x = (DID2cEmployeeId) o;
+ return eq(firstName, x.firstName) && eq(lastName, x.lastName);
+ }
+
+ @Override
+ public int hashCode() {
+ return (firstName != null ? firstName.hashCode() : 0)
+ + 31 * (lastName != null ? lastName.hashCode() : 0);
+ }
+
+ private static boolean eq(Object a, Object b) {
+ return a == b || (a != null && a.equals(b));
+ }
+}
diff --git a/openjpa-persistence-jdbc/src/test/java/org/apache/openjpa/persistence/embed/TestDerivedIdEx2c.java b/openjpa-persistence-jdbc/src/test/java/org/apache/openjpa/persistence/embed/TestDerivedIdEx2c.java
new file mode 100644
index 000000000..03d847dc4
--- /dev/null
+++ b/openjpa-persistence-jdbc/src/test/java/org/apache/openjpa/persistence/embed/TestDerivedIdEx2c.java
@@ -0,0 +1,103 @@
+/*
+ * 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.embed;
+
+import java.util.List;
+
+import jakarta.persistence.EntityManager;
+
+import org.apache.openjpa.persistence.test.SingleEMFTestCase;
+
+/**
+ * Regression test for OPENJPA-2968: the @MapsId columns of a non-@Embeddable
+ * @IdClass field inside an @EmbeddedId must be matched to the members of the
+ * id class by NAME, not by the order in which the id class happens to declare
+ * its fields. DID2cEmployeeId declares lastName before firstName, the reverse
+ * of the @JoinColumns order of DID2cDependent.
+ */
+public class TestDerivedIdEx2c extends SingleEMFTestCase {
+
+ @Override
+ public void setUp() {
+ setUp(DROP_TABLES,
+ DID2cEmployee.class,
+ DID2cDependent.class,
+ DID2cDependentId.class);
+ }
+
+ private DID2cDependent create(EntityManager em) {
+ DID2cEmployeeId eId = new DID2cEmployeeId("Sam", "Vaughn");
+ DID2cEmployee emp = new DID2cEmployee(eId);
+ DID2cDependent dep = new DID2cDependent(
+ new DID2cDependentId("Joe", eId), emp);
+ em.persist(emp);
+ em.persist(dep);
+ em.flush();
+ return dep;
+ }
+
+ public void testFindDoesNotDependOnIdClassFieldOrder() {
+ EntityManager em = emf.createEntityManager();
+ em.getTransaction().begin();
+ create(em);
+ em.clear();
+
+ DID2cDependent found = em.find(DID2cDependent.class,
+ new DID2cDependentId("Joe", new DID2cEmployeeId("Sam", "Vaughn")));
+ assertNotNull("Should find dependent by EmbeddedId key", found);
+ assertNotNull(found.getId().getEmpPK());
+ assertEquals("Sam", found.getId().getEmpPK().getFirstName());
+ assertEquals("Vaughn", found.getId().getEmpPK().getLastName());
+
+ em.getTransaction().commit();
+ em.close();
+ }
+
+ public void testQueryViaEmbeddedIdPath() {
+ EntityManager em = emf.createEntityManager();
+ em.getTransaction().begin();
+ create(em);
+
+ List<?> depList = em.createQuery(
+ "select d from DID2cDependent d where d.id.name = :n "
+ + "and d.id.empPK.firstName = :fn")
+ .setParameter("n", "Joe")
+ .setParameter("fn", "Sam")
+ .getResultList();
+ assertEquals(1, depList.size());
+
+ em.getTransaction().commit();
+ em.close();
+ }
+
+ public void testQueryViaRelation() {
+ EntityManager em = emf.createEntityManager();
+ em.getTransaction().begin();
+ create(em);
+
+ List<?> depList = em.createQuery(
+ "select d from DID2cDependent d where d.emp.firstName = :fn")
+ .setParameter("fn", "Sam")
+ .getResultList();
+ assertEquals(1, depList.size());
+
+ em.getTransaction().commit();
+ em.close();
+ }
+}