You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@openjpa.apache.org by "rzo1 (via GitHub)" <gi...@apache.org> on 2026/08/29 17:53:24 UTC
[PR] [OPENJPA-2968] Do not swallow failures around IdClass primary key values [openjpa]
rzo1 opened a new pull request, #167:
URL: https://github.com/apache/openjpa/pull/167
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 a trace.
Both paths now raise a `StoreException` naming the field, the id class and the mapping, with the original failure chained. A null id class value on the write path is legitimate, so it still writes nulls, but logs 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; declaration order remains only as the fallback for mappings that provide no name, since that is all such a mapping offers.
`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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@openjpa.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [OPENJPA-2968] Do not swallow failures around IdClass primary key values [openjpa]
Posted by "rzo1 (via GitHub)" <gi...@apache.org>.
rzo1 commented on code in PR #167:
URL: https://github.com/apache/openjpa/pull/167#discussion_r3897687284
##########
openjpa-jdbc/src/main/java/org/apache/openjpa/jdbc/meta/strats/EmbedValueHandler.java:
##########
@@ -216,21 +218,27 @@ protected int toDataStoreValue1(OpenJPAStateManager em, ValueMapping vm,
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 =
Review Comment:
Fixed — `Field` and `Modifier` are imported now. The fully qualified names were carried over from the code this replaces.
##########
openjpa-jdbc/src/main/java/org/apache/openjpa/jdbc/meta/strats/EmbedValueHandler.java:
##########
@@ -216,21 +218,27 @@ protected int toDataStoreValue1(OpenJPAStateManager em, ValueMapping vm,
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())
Review Comment:
Added.
##########
openjpa-jdbc/src/main/java/org/apache/openjpa/jdbc/meta/strats/EmbedValueHandler.java:
##########
@@ -442,20 +462,55 @@ public static void getIdColumns(FieldMapping fmd, List cols) {
}
/**
- * 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)
Review Comment:
Braces added. On the stream: I kept the loops. `findField` does two passes on purpose — exact name first, then case-insensitive — so the stream version would need two `filter(...).findFirst()` chains anyway, and this runs per row on the read path. Happy to switch if you prefer the stream form for readability.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@openjpa.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [OPENJPA-2968] Do not swallow failures around IdClass primary key values [openjpa]
Posted by "rzo1 (via GitHub)" <gi...@apache.org>.
rzo1 merged PR #167:
URL: https://github.com/apache/openjpa/pull/167
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@openjpa.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [OPENJPA-2968] Do not swallow failures around IdClass primary key values [openjpa]
Posted by "solomax (via GitHub)" <gi...@apache.org>.
solomax commented on code in PR #167:
URL: https://github.com/apache/openjpa/pull/167#discussion_r3891528989
##########
openjpa-jdbc/src/main/java/org/apache/openjpa/jdbc/meta/strats/EmbedValueHandler.java:
##########
@@ -216,21 +218,27 @@ protected int toDataStoreValue1(OpenJPAStateManager em, ValueMapping vm,
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())
Review Comment:
I would add `{}` here :)
##########
openjpa-jdbc/src/main/java/org/apache/openjpa/jdbc/meta/strats/EmbedValueHandler.java:
##########
@@ -216,21 +218,27 @@ protected int toDataStoreValue1(OpenJPAStateManager em, ValueMapping vm,
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 =
Review Comment:
weird enough `java.lang.reflect.Field` is not moved to `import`'s
##########
openjpa-jdbc/src/main/java/org/apache/openjpa/jdbc/meta/strats/EmbedValueHandler.java:
##########
@@ -442,20 +462,55 @@ public static void getIdColumns(FieldMapping fmd, List cols) {
}
/**
- * 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)
Review Comment:
I would add `{}` in these `for/if`'s :))
Is the stream API will be slower here? :)
like: `fields.stream().filter(f -> f.getName().equals(name)).findFirst().orElse(null)`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@openjpa.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org