diff --git a/src/main/java/org/datanucleus/store/types/SCOUtils.java b/src/main/java/org/datanucleus/store/types/SCOUtils.java index c95c7b97..cd5b61e3 100644 --- a/src/main/java/org/datanucleus/store/types/SCOUtils.java +++ b/src/main/java/org/datanucleus/store/types/SCOUtils.java @@ -857,13 +857,16 @@ public static boolean updateListWithListElements(List list, List elements) } } - // Update position of elements in the list to match the new order + // Update position of elements in the list to match the new order. + // Take a snapshot of the current order via iteration rather than using list.get(position) directly, + // to avoid inconsistencies between delegate and backing store when collection caching is disabled. + java.util.ArrayList currentOrder = new java.util.ArrayList(list); elementsIter = elements.iterator(); int position = 0; while (elementsIter.hasNext()) { Object element = elementsIter.next(); - Object currentElement = list.get(position); + Object currentElement = currentOrder.get(position); boolean updatePosition = false; if ((element == null && currentElement != null) || (element != null && currentElement == null)) { @@ -879,6 +882,7 @@ else if (element != null && currentElement != null && !currentElement.equals(ele { // Update the position, taking care not to have dependent-field deletes taking place ((SCOList) list).set(position, element, false); + currentOrder.set(position, element); updated = true; } diff --git a/src/main/java/org/datanucleus/store/types/wrappers/backed/ArrayList.java b/src/main/java/org/datanucleus/store/types/wrappers/backed/ArrayList.java index f41707de..330055fc 100644 --- a/src/main/java/org/datanucleus/store/types/wrappers/backed/ArrayList.java +++ b/src/main/java/org/datanucleus/store/types/wrappers/backed/ArrayList.java @@ -1035,10 +1035,8 @@ public E set(int index, E element, boolean allowDependentField) makeDirty(); - if (useCache) - { - loadFromStore(); - } + // Always load the delegate, even when useCache=false, since delegate.set() requires it to be populated + loadFromStore(); E delegateReturn = delegate.set(index, element); if (backingStore != null) diff --git a/src/main/java/org/datanucleus/store/types/wrappers/backed/LinkedList.java b/src/main/java/org/datanucleus/store/types/wrappers/backed/LinkedList.java index 09413c14..a023453f 100644 --- a/src/main/java/org/datanucleus/store/types/wrappers/backed/LinkedList.java +++ b/src/main/java/org/datanucleus/store/types/wrappers/backed/LinkedList.java @@ -1059,10 +1059,8 @@ public E set(int index, E element, boolean allowDependentField) makeDirty(); - if (useCache) - { - loadFromStore(); - } + // Always load the delegate, even when useCache=false, since delegate.set() requires it to be populated + loadFromStore(); E delegateReturn = delegate.set(index, element); if (backingStore != null) diff --git a/src/main/java/org/datanucleus/store/types/wrappers/backed/List.java b/src/main/java/org/datanucleus/store/types/wrappers/backed/List.java index 545c7db7..93ec10e5 100644 --- a/src/main/java/org/datanucleus/store/types/wrappers/backed/List.java +++ b/src/main/java/org/datanucleus/store/types/wrappers/backed/List.java @@ -1033,10 +1033,8 @@ public E set(int index, E element, boolean allowDependentField) makeDirty(); - if (useCache) - { - loadFromStore(); - } + // Always load the delegate, even when useCache=false, since delegate.set() requires it to be populated + loadFromStore(); // Update the delegate since updating this can cause removal of the original object E delegateReturn = delegate.set(index, element); diff --git a/src/main/java/org/datanucleus/store/types/wrappers/backed/Stack.java b/src/main/java/org/datanucleus/store/types/wrappers/backed/Stack.java index edf503a4..acc89178 100644 --- a/src/main/java/org/datanucleus/store/types/wrappers/backed/Stack.java +++ b/src/main/java/org/datanucleus/store/types/wrappers/backed/Stack.java @@ -1116,10 +1116,8 @@ public E set(int index, E element, boolean allowDependentField) makeDirty(); - if (useCache) - { - loadFromStore(); - } + // Always load the delegate, even when useCache=false, since delegate.set() requires it to be populated + loadFromStore(); E delegateReturn = delegate.set(index, element); if (backingStore != null) diff --git a/src/main/java/org/datanucleus/store/types/wrappers/backed/Vector.java b/src/main/java/org/datanucleus/store/types/wrappers/backed/Vector.java index da7b609c..ae98da41 100644 --- a/src/main/java/org/datanucleus/store/types/wrappers/backed/Vector.java +++ b/src/main/java/org/datanucleus/store/types/wrappers/backed/Vector.java @@ -1131,10 +1131,8 @@ public E set(int index, E element, boolean allowDependentField) makeDirty(); - if (useCache) - { - loadFromStore(); - } + // Always load the delegate, even when useCache=false, since delegate.set() requires it to be populated + loadFromStore(); E delegateReturn = delegate.set(index, element); if (backingStore != null) diff --git a/src/test/java/org/datanucleus/store/types/sco/SCOUtilsTest.java b/src/test/java/org/datanucleus/store/types/sco/SCOUtilsTest.java index f58bdf9a..3384835f 100644 --- a/src/test/java/org/datanucleus/store/types/sco/SCOUtilsTest.java +++ b/src/test/java/org/datanucleus/store/types/sco/SCOUtilsTest.java @@ -5,9 +5,11 @@ import junit.framework.TestCase; +import org.datanucleus.FetchPlanState; import org.datanucleus.metadata.AbstractMemberMetaData; import org.datanucleus.state.DNStateManager; import org.datanucleus.store.StoreManager; +import org.datanucleus.store.types.SCOList; import org.datanucleus.store.types.SCOUtils; import org.datanucleus.store.types.scostore.CollectionStore; @@ -42,6 +44,105 @@ public void testToArrayCollectionStoreStateManagerObjectArray() assertEquals(arr[1],"TEST2"); } + /** + * Test that updateListWithListElements handles reordering when an element is inserted at the beginning. + * Reproduces the scenario from GitHub issue #526. + * @see GitHub #526 + * @see Integration test + */ + public void testUpdateListWithListElementsReorder() + { + TestSCOList list = new TestSCOList(); + list.add("A"); + list.add("B"); + list.add("C"); + + // Desired order: insert "D" at beginning + java.util.List elements = new java.util.ArrayList<>(); + elements.add("D"); + elements.add("A"); + elements.add("B"); + elements.add("C"); + + boolean updated = SCOUtils.updateListWithListElements(list, elements); + + assertTrue("List should be marked as updated", updated); + assertEquals(4, list.size()); + assertEquals("D", list.get(0)); + assertEquals("A", list.get(1)); + assertEquals("B", list.get(2)); + assertEquals("C", list.get(3)); + } + + /** + * Test that updateListWithListElements handles combined add, remove, and reorder. + */ + public void testUpdateListWithListElementsAddRemoveReorder() + { + TestSCOList list = new TestSCOList(); + list.add("A"); + list.add("B"); + list.add("C"); + + // Remove "B", add "D", reorder to [C, D, A] + java.util.List elements = new java.util.ArrayList<>(); + elements.add("C"); + elements.add("D"); + elements.add("A"); + + boolean updated = SCOUtils.updateListWithListElements(list, elements); + + assertTrue("List should be marked as updated", updated); + assertEquals(3, list.size()); + assertEquals("C", list.get(0)); + assertEquals("D", list.get(1)); + assertEquals("A", list.get(2)); + } + + /** + * Test that updateListWithListElements returns false when nothing changes. + */ + public void testUpdateListWithListElementsNoChange() + { + TestSCOList list = new TestSCOList(); + list.add("A"); + list.add("B"); + + java.util.List elements = new java.util.ArrayList<>(); + elements.add("A"); + elements.add("B"); + + boolean updated = SCOUtils.updateListWithListElements(list, elements); + assertFalse("List should not be marked as updated when nothing changes", updated); + } + + /** + * Minimal SCOList implementation backed by an ArrayList, for testing updateListWithListElements. + */ + @SuppressWarnings({"rawtypes", "unchecked"}) + private static class TestSCOList extends java.util.ArrayList implements SCOList + { + public Object set(int index, Object element, boolean allowDependentField) + { + return super.set(index, element); + } + + public void updateEmbeddedElement(Object element, int fieldNumber, Object value, boolean makeDirty) {} + public boolean remove(Object element, boolean allowCascadeDelete) { return super.remove(element); } + public void load() {} + public boolean isLoaded() { return true; } + public void setValue(Object value) {} + public void initialise(Object value) {} + public void initialise() {} + public void initialise(Object newValue, Object oldValue) {} + public String getFieldName() { return null; } + public Object getOwner() { return null; } + public void unsetOwner() {} + public Object getValue() { return this; } + public Object detachCopy(FetchPlanState state) { return null; } + public void attachCopy(Object value) {} + } + private static class BackingStore implements CollectionStore { Collection elm;