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