solomax commented on code in PR #227:
URL: https://github.com/apache/openjpa/pull/227#discussion_r4384276420
##########
openjpa-kernel/src/main/java/org/apache/openjpa/kernel/BrokerImpl.java:
##########
@@ -4372,6 +4383,26 @@ void dirtyCheckWholeConverted() {
}
}
+ /**
+ * Put back the values of fields converted as a whole that were changed
+ * in place and not yet found by a flush, since they were not saved for
+ * rollback. Done before the instances are rolled back.
+ */
+ private void restoreWholeConverted() {
+ if (_wholeConverted == null || _wholeConverted.isEmpty()) {
+ return;
+ }
+
+ lock();
+ try {
+ for (StateManagerImpl sm : new ArrayList<>(_wholeConverted)) {
Review Comment:
Why do we need `ArrayList` here?
##########
openjpa-kernel/src/main/java/org/apache/openjpa/kernel/AbstractPCData.java:
##########
@@ -243,6 +247,18 @@ protected Object toEmbeddedField(OpenJPAStateManager sm,
ValueMetaData vmd,
protected Object toData(FieldMetaData fmd, Object val, StoreContext ctx) {
if (val == null)
return null;
+ // a converted value is cached as its database value, so that each
+ // instance loaded from the cache gets its own value from the
+ // converter; it is not cached when the converter fails
+ if (fmd.isConvertedObject()) {
+ Object data;
+ try {
+ data = fmd.getExternalValue(val, ctx);
+ } catch (RuntimeException re) {
+ return NULL;
+ }
+ return (data == null) ? NULL : data;
Review Comment:
```suggestion
return data == null ? NULL : data;
```
##########
openjpa-kernel/src/main/java/org/apache/openjpa/kernel/SavepointFieldManager.java:
##########
@@ -221,7 +221,10 @@ public void storeObjectField(int field, Object curVal) {
_field = proxy.copyDate((Date) curVal);
break;
case JavaTypes.OBJECT:
- _field = proxy.copyCustom(curVal);
+ // a converted value is copied through its converter
+ _field = (fmd.isConvertedObject())
Review Comment:
```suggestion
_field = fmd.isConvertedObject()
```
##########
openjpa-kernel/src/main/java/org/apache/openjpa/kernel/DetachedStateManager.java:
##########
@@ -366,6 +388,45 @@ protected Object getDetachedObjectId(AttachManager manager,
return _oid;
}
+ /**
+ * Whether a field converted as a whole of this embedded instance
+ * converts differently than in the embedded instance its managed owner
+ * holds, since a value or element changed in place while detached does
+ * not mark the field dirty. Assumes a change when there is no such
+ * instance to compare with.
+ */
+ private boolean isEmbeddedWholeConvertedChanged(BrokerImpl broker,
+ ClassMetaData meta, OpenJPAStateManager owner,
+ ValueMetaData ownerMeta) {
+ FieldMetaData[] fmds = meta.getWholeConvertedFields();
+ if (fmds.length == 0) {
+ return false;
+ }
+ FieldMetaData ownerField = ownerMeta.getFieldMetaData();
+ if (ownerMeta != ownerField
+ || !owner.getLoaded().get(ownerField.getIndex())) {
+ return true;
+ }
+ OpenJPAStateManager cur = broker.getStateManager(
+ owner.fetchField(ownerField.getIndex(), false));
+ if (!(cur instanceof StateManagerImpl)) {
+ return true;
+ }
+ for (FieldMetaData fmd : fmds) {
+ int i = fmd.getIndex();
+ if (!_loaded.get(i)) {
+ continue;
+ }
+ provideField(i);
+ Object val = objval;
+ objval = null;
+ if (((StateManagerImpl) cur).isWholeConvertedChanged(i, val)) {
Review Comment:
I would move this cast out of the loop
like
```
StateManagerImpl curImpl;
if (cur instanceof StateManagerImpl curState) {
curImpl = curState;
} else {
return true;
}
```
--
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]
