[DURACOM-125][#8736] Preserved order of modified MetadataValues

feat:
  - Introduced new Class with static methods usable to order
`MetadataValue` lists;
  - Introduced ITs also for `PatchMetadata` replace operations;
  - Introduced new method `DspaceObject#getMetadataFieldId`.
ref:
  - Replaced old sort method inside `ItemServiceImpl` with the new one

dspace-api/src/main/java/org/dspace/content/DSpaceObject.java

@@ -48,6 +48,12 @@ public abstract class DSpaceObject implements Serializable, ReloadableEntity<jav
     @Transient
     private StringBuffer eventDetails = null;
 
+    /**
+     * The same order should be applied inside this comparator
+     * {@link MetadataValueComparators#defaultComparator} to preserve
+     * ordering while the list has been modified and not yet persisted
+     * and reloaded.
+     */
     @OneToMany(fetch = FetchType.LAZY, mappedBy = "dSpaceObject", cascade = CascadeType.ALL, orphanRemoval = true)
     @OrderBy("metadataField, place")
     private List<MetadataValue> metadata = new ArrayList<>();
@@ -116,7 +122,7 @@ public abstract class DSpaceObject implements Serializable, ReloadableEntity<jav
      * @return summary of event details, or null if there are none.
      */
     public String getDetails() {
-        return (eventDetails == null ? null : eventDetails.toString());
+        return eventDetails == null ? null : eventDetails.toString();
     }
 
     /**
@@ -145,7 +151,7 @@ public abstract class DSpaceObject implements Serializable, ReloadableEntity<jav
      * one
      */
     public String getHandle() {
-        return (CollectionUtils.isNotEmpty(handles) ? handles.get(0).getHandle() : null);
+        return CollectionUtils.isNotEmpty(handles) ? handles.get(0).getHandle() : null;
     }
 
     void setHandle(List<Handle> handle) {

dspace-api/src/main/java/org/dspace/content/DSpaceObjectServiceImpl.java

@@ -126,6 +126,11 @@ public abstract class DSpaceObjectServiceImpl<T extends DSpaceObject> implements
             }
         }
 
+        // Sort the metadataValues if they have been modified,
+        // is used to preserve the default order.
+        if (dso.isMetadataModified()) {
+            values.sort(MetadataValueComparators.defaultComparator);
+        }
         // Create an array of matching values
         return values;
     }
@@ -542,7 +547,7 @@ public abstract class DSpaceObjectServiceImpl<T extends DSpaceObject> implements
 
         int add = 4 - tokens.length;
         if (add > 0) {
-            tokens = (String[]) ArrayUtils.addAll(tokens, new String[add]);
+            tokens = ArrayUtils.addAll(tokens, new String[add]);
         }
 
         return tokens;
@@ -603,21 +608,18 @@ public abstract class DSpaceObjectServiceImpl<T extends DSpaceObject> implements
             //If two places are the same then the MetadataValue instance will be placed before the
             //RelationshipMetadataValue instance.
             //This is done to ensure that the order is correct.
-            metadataValues.sort(new Comparator<MetadataValue>() {
+            metadataValues.sort((o1, o2) -> {
-                @Override
+                int compare = o1.getPlace() - o2.getPlace();
-                public int compare(MetadataValue o1, MetadataValue o2) {
+                if (compare == 0) {
-                    int compare = o1.getPlace() - o2.getPlace();
+                    if (o1 instanceof RelationshipMetadataValue && o2 instanceof RelationshipMetadataValue) {
-                    if (compare == 0) {
+                        return compare;
-                        if (o1 instanceof RelationshipMetadataValue && o2 instanceof RelationshipMetadataValue) {
+                    } else if (o1 instanceof RelationshipMetadataValue) {
-                            return compare;
+                        return 1;
-                        } else if (o1 instanceof RelationshipMetadataValue) {
+                    } else if (o2 instanceof RelationshipMetadataValue) {
-                            return 1;
+                        return -1;
-                        } else if (o2 instanceof RelationshipMetadataValue) {
-                            return -1;
-                        }
                     }
-                    return compare;
                 }
+                return compare;
             });
             for (MetadataValue metadataValue : metadataValues) {
                 //Retrieve & store the place for each metadata value
@@ -634,7 +636,7 @@ public abstract class DSpaceObjectServiceImpl<T extends DSpaceObject> implements
                     String authority = metadataValue.getAuthority();
                     String relationshipId = StringUtils.split(authority, "::")[1];
                     Relationship relationship = relationshipService.find(context, Integer.parseInt(relationshipId));
-                    if (relationship.getLeftItem().equals((Item) dso)) {
+                    if (relationship.getLeftItem().equals(dso)) {
                         relationship.setLeftPlace(mvPlace);
                     } else {
                         relationship.setRightPlace(mvPlace);

dspace-api/src/main/java/org/dspace/content/ItemServiceImpl.java

@@ -12,7 +12,6 @@ import java.io.InputStream;
 import java.sql.SQLException;
 import java.util.ArrayList;
 import java.util.Arrays;
-import java.util.Comparator;
 import java.util.Date;
 import java.util.Iterator;
 import java.util.LinkedList;
@@ -288,9 +287,10 @@ public class ItemServiceImpl extends DSpaceObjectServiceImpl<Item> implements It
         return itemDAO.findAll(context, true, true);
     }
 
+    @Override
     public Iterator<Item> findAllRegularItems(Context context) throws SQLException {
         return itemDAO.findAllRegularItems(context);
-    };
+    }
 
     @Override
     public Iterator<Item> findBySubmitter(Context context, EPerson eperson) throws SQLException {
@@ -1054,7 +1054,7 @@ public class ItemServiceImpl extends DSpaceObjectServiceImpl<Item> implements It
         List<Collection> linkedCollections = item.getCollections();
         List<Collection> notLinkedCollections = new ArrayList<>(allCollections.size() - linkedCollections.size());
 
-        if ((allCollections.size() - linkedCollections.size()) == 0) {
+        if (allCollections.size() - linkedCollections.size() == 0) {
             return notLinkedCollections;
         }
         for (Collection collection : allCollections) {
@@ -1149,6 +1149,7 @@ public class ItemServiceImpl extends DSpaceObjectServiceImpl<Item> implements It
      * @return <code>true</code> if the item is an inprogress submission, i.e. a WorkspaceItem or WorkflowItem
      * @throws SQLException An exception that provides information on a database access error or other errors.
      */
+    @Override
     public boolean isInProgressSubmission(Context context, Item item) throws SQLException {
         return workspaceItemService.findByItem(context, item) != null
             || workflowItemService.findByItem(context, item) != null;
@@ -1179,8 +1180,8 @@ prevent the generation of resource policy entry values with null dspace_object a
             if (!authorizeService
                 .isAnIdenticalPolicyAlreadyInPlace(context, dso, defaultPolicy.getGroup(), Constants.READ,
                     defaultPolicy.getID()) &&
-                   ((!appendMode && this.isNotAlreadyACustomRPOfThisTypeOnDSO(context, dso)) ||
+                   (!appendMode && this.isNotAlreadyACustomRPOfThisTypeOnDSO(context, dso) ||
-                    (appendMode && this.shouldBeAppended(context, dso, defaultPolicy)))) {
+                    appendMode && this.shouldBeAppended(context, dso, defaultPolicy))) {
                 ResourcePolicy newPolicy = resourcePolicyService.clone(context, defaultPolicy);
                 newPolicy.setdSpaceObject(dso);
                 newPolicy.setAction(Constants.READ);
@@ -1611,7 +1612,7 @@ prevent the generation of resource policy entry values with null dspace_object a
             fullMetadataValueList.addAll(relationshipMetadataService.getRelationshipMetadata(item, true));
             fullMetadataValueList.addAll(dbMetadataValues);
 
-            item.setCachedMetadata(sortMetadataValueList(fullMetadataValueList));
+            item.setCachedMetadata(MetadataValueComparators.sort(fullMetadataValueList));
         }
 
         log.debug("Called getMetadata for " + item.getID() + " based on cache");
@@ -1653,28 +1654,6 @@ prevent the generation of resource policy entry values with null dspace_object a
         }
     }
 
-    /**
-     * This method will sort the List of MetadataValue objects based on the MetadataSchema, MetadataField Element,
-     * MetadataField Qualifier and MetadataField Place in that order.
-     * @param listToReturn  The list to be sorted
-     * @return The list sorted on those criteria
-     */
-    private List<MetadataValue> sortMetadataValueList(List<MetadataValue> listToReturn) {
-        Comparator<MetadataValue> comparator = Comparator.comparing(
-            metadataValue -> metadataValue.getMetadataField().getMetadataSchema().getName(),
-            Comparator.nullsFirst(Comparator.naturalOrder()));
-        comparator = comparator.thenComparing(metadataValue -> metadataValue.getMetadataField().getElement(),
-                                              Comparator.nullsFirst(Comparator.naturalOrder()));
-        comparator = comparator.thenComparing(metadataValue -> metadataValue.getMetadataField().getQualifier(),
-                                              Comparator.nullsFirst(Comparator.naturalOrder()));
-        comparator = comparator.thenComparing(metadataValue -> metadataValue.getPlace(),
-                                              Comparator.nullsFirst(Comparator.naturalOrder()));
-
-        Stream<MetadataValue> metadataValueStream = listToReturn.stream().sorted(comparator);
-        listToReturn = metadataValueStream.collect(Collectors.toList());
-        return listToReturn;
-    }
-
     @Override
     public MetadataValue addMetadata(Context context, Item dso, String schema, String element, String qualifier,
             String lang, String value, String authority, int confidence, int place) throws SQLException {

dspace-api/src/main/java/org/dspace/content/MetadataValue.java

@@ -19,6 +19,7 @@ import javax.persistence.Lob;
 import javax.persistence.ManyToOne;
 import javax.persistence.SequenceGenerator;
 import javax.persistence.Table;
+import javax.persistence.Transient;
 
 import org.dspace.core.Context;
 import org.dspace.core.ReloadableEntity;
@@ -171,6 +172,14 @@ public class MetadataValue implements ReloadableEntity<Integer> {
         this.metadataField = metadataField;
     }
 
+    /**
+     * @return {@code MetadataField#getID()}
+     */
+    @Transient
+    protected Integer getMetadataFieldId() {
+        return getMetadataField().getID();
+    }
+
     /**
      * Get the metadata value.
      *

dspace-api/src/main/java/org/dspace/content/MetadataValueComparators.java

@@ -0,0 +1,51 @@
+/**
+ * The contents of this file are subject to the license and copyright
+ * detailed in the LICENSE and NOTICE files at the root of the source
+ * tree and available online at
+ *
+ * http://www.dspace.org/license/
+ */
+package org.dspace.content;
+
+import java.util.Comparator;
+import java.util.List;
+import java.util.stream.Collectors;
+
+/**
+ * This class contains only static members that can be used
+ * to sort list of {@link MetadataValue}
+ *
+ * @author Vincenzo Mecca (vins01-4science - vincenzo.mecca at 4science.com)
+ *
+ */
+public final class MetadataValueComparators {
+
+    private MetadataValueComparators() {}
+
+    /**
+     * This is the default comparator that mimics the ordering
+     * applied by the standard {@code @OrderBy} annotation inside
+     * {@link DSpaceObject#getMetadata()}
+     */
+    public static final Comparator<MetadataValue> defaultComparator =
+        Comparator.comparing(MetadataValue::getMetadataFieldId)
+            .thenComparing(
+                MetadataValue::getPlace,
+                Comparator.nullsFirst(Comparator.naturalOrder())
+            );
+
+    /**
+     * This method creates a new {@code List<MetadataValue>} ordered by the
+     * {@code MetadataComparators#defaultComparator}.
+     *
+     * @param metadataValues
+     * @return {@code List<MetadataValue>} ordered copy list using stream.
+     */
+    public static final List<MetadataValue> sort(List<MetadataValue> metadataValues) {
+        return metadataValues
+                .stream()
+                .sorted(MetadataValueComparators.defaultComparator)
+                .collect(Collectors.toList());
+    }
+
+}

dspace-server-webapp/src/test/java/org/dspace/app/rest/PatchMetadataIT.java

@@ -11,8 +11,8 @@ import static com.jayway.jsonpath.JsonPath.read;
 import static org.hamcrest.CoreMatchers.equalTo;
 import static org.hamcrest.CoreMatchers.not;
 import static org.hamcrest.CoreMatchers.startsWith;
+import static org.hamcrest.MatcherAssert.assertThat;
 import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertThat;
 import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get;
 import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.patch;
 import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.post;
@@ -23,8 +23,11 @@ import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.
 import java.io.IOException;
 import java.sql.SQLException;
 import java.util.ArrayList;
+import java.util.Comparator;
 import java.util.List;
+import java.util.Map;
 import java.util.concurrent.atomic.AtomicReference;
+import java.util.stream.Collectors;
 import java.util.stream.IntStream;
 
 import org.dspace.app.rest.matcher.MetadataMatcher;
@@ -50,6 +53,7 @@ import org.dspace.content.service.EntityTypeService;
 import org.dspace.content.service.ItemService;
 import org.dspace.content.service.RelationshipTypeService;
 import org.dspace.content.service.WorkspaceItemService;
+import org.dspace.services.ConfigurationService;
 import org.hamcrest.Matcher;
 import org.hamcrest.Matchers;
 import org.junit.After;
@@ -63,6 +67,13 @@ import org.springframework.http.MediaType;
  */
 public class PatchMetadataIT extends AbstractEntityIntegrationTest {
 
+    private static final String SECTIONS_TRADITIONALPAGEONE_DC_CONTRIBUTOR_AUTHOR =
+        "/sections/traditionalpageone/dc.contributor.author/%1$s";
+
+    private static final String getPath(Object element) {
+        return String.format(SECTIONS_TRADITIONALPAGEONE_DC_CONTRIBUTOR_AUTHOR, element);
+    }
+
     @Autowired
     private RelationshipTypeService relationshipTypeService;
 
@@ -75,6 +86,9 @@ public class PatchMetadataIT extends AbstractEntityIntegrationTest {
     @Autowired
     private WorkspaceItemService workspaceItemService;
 
+    @Autowired
+    private ConfigurationService configurationService;
+
     private Collection collection;
     private Collection collection2;
     private WorkspaceItem publicationWorkspaceItem;
@@ -297,8 +311,6 @@ public class PatchMetadataIT extends AbstractEntityIntegrationTest {
                                                        .withEntityType("Publication")
                                                        .build();
 
-        String adminToken = getAuthToken(admin.getEmail(), password);
-
         // Make sure we grab the latest instance of the Item from the database before adding a regular author
         WorkspaceItem publication = workspaceItemService.find(context, publicationWorkspaceItem.getID());
         itemService.addMetadata(context, publication.getItem(),
@@ -920,6 +932,41 @@ public class PatchMetadataIT extends AbstractEntityIntegrationTest {
         replaceTraditionalPageOneAuthorTest(3, expectedOrder);
     }
 
+    @Test
+    public void replaceMultipleTraditionalPageOnePlainTextAuthorTest() throws Exception {
+        final boolean virtualMetadataEnabled =
+            configurationService.getBooleanProperty("item.enable-virtual-metadata", false);
+
+        configurationService.setProperty("item.enable-virtual-metadata", false);
+        try {
+            initPlainTextPublicationWorkspace();
+
+            Map<Integer, String> replacedAuthors =
+                Map.of(
+                    0, authorsOriginalOrder.get(4),
+                    1, authorsOriginalOrder.get(1),
+                    2, authorsOriginalOrder.get(2),
+                    3, authorsOriginalOrder.get(3),
+                    4, authorsOriginalOrder.get(0)
+                );
+
+            List<String> expectedOrder =
+                List.of(
+                    authorsOriginalOrder.get(4),
+                    authorsOriginalOrder.get(1),
+                    authorsOriginalOrder.get(2),
+                    authorsOriginalOrder.get(3),
+                    authorsOriginalOrder.get(0)
+                );
+
+            replaceTraditionalPageMultipleAuthorsTest(replacedAuthors, expectedOrder);
+        } catch (Exception e) {
+            throw e;
+        } finally {
+            configurationService.setProperty("item.enable-virtual-metadata", virtualMetadataEnabled);
+        }
+    }
+
 
     /**
      * This test will add an author (dc.contributor.author) within a workspace publication's "traditionalpageone"
@@ -1393,24 +1440,7 @@ public class PatchMetadataIT extends AbstractEntityIntegrationTest {
         ops.add(moveOperation);
         String patchBody = getPatchContent(ops);
 
-        String token = getAuthToken(admin.getEmail(), password);
+        assertReplacementOrder(expectedOrder, patchBody);
-
-        getClient(token).perform(patch("/api/submission/workspaceitems/" + publicationWorkspaceItem.getID())
-                .content(patchBody)
-                .contentType(javax.ws.rs.core.MediaType.APPLICATION_JSON_PATCH_JSON))
-                .andExpect(status().isOk());
-
-        String authorField = "dc.contributor.author";
-        getClient(token).perform(get("/api/submission/workspaceitems/" + publicationWorkspaceItem.getID()))
-                .andExpect(status().isOk())
-                .andExpect(content().contentType(contentType))
-                .andExpect(jsonPath("$.sections.traditionalpageone", Matchers.allOf(
-                        Matchers.is(MetadataMatcher.matchMetadata(authorField, expectedOrder.get(0), 0)),
-                        Matchers.is(MetadataMatcher.matchMetadata(authorField, expectedOrder.get(1), 1)),
-                        Matchers.is(MetadataMatcher.matchMetadata(authorField, expectedOrder.get(2), 2)),
-                        Matchers.is(MetadataMatcher.matchMetadata(authorField, expectedOrder.get(3), 3)),
-                        Matchers.is(MetadataMatcher.matchMetadata(authorField, expectedOrder.get(4), 4))
-                )));
     }
 
     /**
@@ -1450,33 +1480,66 @@ public class PatchMetadataIT extends AbstractEntityIntegrationTest {
      * @param expectedOrder     A list of author names sorted in the expected order
      */
     private void replaceTraditionalPageOneAuthorTest(int path, List<String> expectedOrder) throws Exception {
-        List<Operation> ops = new ArrayList<Operation>();
+        String patchBody =
-        MetadataValueRest value = new MetadataValueRest();
+            getPatchContent(
-        value.setValue(replacedAuthor);
+                List.of(
+                    this.mapToReplaceOperation(path, replacedAuthor)
+                )
+            );
+
+        assertReplacementOrder(expectedOrder, patchBody);
+    }
+
+    private void replaceTraditionalPageMultipleAuthorsTest(
+        Map<Integer, String> values, List<String> expectedOrder
+    ) throws Exception {
+        List<Operation> ops =
+            values
+                .entrySet()
+                .stream()
+                .sorted(Comparator.comparing(Map.Entry::getKey))
+                .map(entry -> mapToReplaceOperation(entry.getKey(), entry.getValue()))
+                .collect(Collectors.toList());
 
-        ReplaceOperation replaceOperation = new ReplaceOperation("/sections/traditionalpageone/dc.contributor.author/"
-                                                                         + path, value);
-        ops.add(replaceOperation);
         String patchBody = getPatchContent(ops);
 
+        assertReplacementOrder(expectedOrder, patchBody);
+    }
+
+    private ReplaceOperation mapToReplaceOperation(int path, String author) {
+        return new ReplaceOperation(getPath(path), new MetadataValueRest(author));
+    }
+
+    private void assertReplacementOrder(List<String> expectedOrder, String patchBody) throws Exception, SQLException {
         String token = getAuthToken(admin.getEmail(), password);
 
-        getClient(token).perform(patch("/api/submission/workspaceitems/" + publicationWorkspaceItem.getID())
+        getClient(token)
-                                         .content(patchBody)
+            .perform(
-                                         .contentType(javax.ws.rs.core.MediaType.APPLICATION_JSON_PATCH_JSON))
+                patch("/api/submission/workspaceitems/" + publicationWorkspaceItem.getID())
-                        .andExpect(status().isOk());
+                .content(patchBody)
+                .contentType(javax.ws.rs.core.MediaType.APPLICATION_JSON_PATCH_JSON)
+            )
+            .andExpect(status().isOk());
 
         String authorField = "dc.contributor.author";
-        getClient(token).perform(get("/api/submission/workspaceitems/" + publicationWorkspaceItem.getID()))
+        getClient(token)
-                   .andExpect(status().isOk())
+            .perform(get("/api/submission/workspaceitems/" + publicationWorkspaceItem.getID()))
-                   .andExpect(content().contentType(contentType))
+            .andExpect(status().isOk())
-                   .andExpect(jsonPath("$.sections.traditionalpageone", Matchers.allOf(
+            .andExpect(
-                           Matchers.is(MetadataMatcher.matchMetadata(authorField, expectedOrder.get(0), 0)),
+                content().contentType(contentType)
-                           Matchers.is(MetadataMatcher.matchMetadata(authorField, expectedOrder.get(1), 1)),
+            )
-                           Matchers.is(MetadataMatcher.matchMetadata(authorField, expectedOrder.get(2), 2)),
+            .andExpect(
-                           Matchers.is(MetadataMatcher.matchMetadata(authorField, expectedOrder.get(3), 3)),
+                jsonPath(
-                           Matchers.is(MetadataMatcher.matchMetadata(authorField, expectedOrder.get(4), 4))
+                    "$.sections.traditionalpageone",
-                   )));
+                    Matchers.allOf(
+                        Matchers.is(MetadataMatcher.matchMetadata(authorField, expectedOrder.get(0), 0)),
+                        Matchers.is(MetadataMatcher.matchMetadata(authorField, expectedOrder.get(1), 1)),
+                        Matchers.is(MetadataMatcher.matchMetadata(authorField, expectedOrder.get(2), 2)),
+                        Matchers.is(MetadataMatcher.matchMetadata(authorField, expectedOrder.get(3), 3)),
+                        Matchers.is(MetadataMatcher.matchMetadata(authorField, expectedOrder.get(4), 4))
+                    )
+                )
+            );
     }
 
     /**
@@ -1490,8 +1553,7 @@ public class PatchMetadataIT extends AbstractEntityIntegrationTest {
         List<Operation> ops = new ArrayList<Operation>();
         MetadataValueRest value = new MetadataValueRest();
         value.setValue(addedAuthor);
-        AddOperation addOperation = new AddOperation("/sections/traditionalpageone/dc.contributor.author/" + path,
+        AddOperation addOperation = new AddOperation(getPath(path), value);
-                                                     value);
         ops.add(addOperation);
         String patchBody = getPatchContent(ops);
 
@@ -1525,8 +1587,7 @@ public class PatchMetadataIT extends AbstractEntityIntegrationTest {
      */
     private void removeTraditionalPageOneAuthorTest(int path, List<String> expectedOrder) throws Exception {
         List<Operation> ops = new ArrayList<Operation>();
-        RemoveOperation removeOperation = new RemoveOperation("/sections/traditionalpageone/dc.contributor.author/"
+        RemoveOperation removeOperation = new RemoveOperation(getPath(path));
-                                                                      + path);
         ops.add(removeOperation);
         String patchBody = getPatchContent(ops);
 
@@ -1600,8 +1661,10 @@ public class PatchMetadataIT extends AbstractEntityIntegrationTest {
      * @param path  The "path" index to use for the Move operation
      */
     private MoveOperation getTraditionalPageOneMoveAuthorOperation(int from, int path) {
-        return new MoveOperation("/sections/traditionalpageone/dc.contributor.author/" + path,
+        return new MoveOperation(
-                "/sections/traditionalpageone/dc.contributor.author/" + from);
+            getPath(path),
+            getPath(from)
+        );
     }
 
     /**