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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
