Merge pull request #8742 from 4Science/duracom-125

MetadataValues' list doesn't respect ordering after been modified

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 @@ protected void addDetails(String d) {
      * @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 UUID getID() {
      * 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 List<MetadataValue> getMetadata(T dso, String schema, String element, Str
             }
         }
 
+        // 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 @@ protected String[] getElements(String fieldName) {
 
         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 void update(Context context, T dso) throws SQLException, AuthorizeExcepti
             //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>() {
-                @Override
-                public int compare(MetadataValue o1, MetadataValue o2) {
-                    int compare = o1.getPlace() - o2.getPlace();
-                    if (compare == 0) {
-                        if (o1 instanceof RelationshipMetadataValue && o2 instanceof RelationshipMetadataValue) {
-                            return compare;
-                        } else if (o1 instanceof RelationshipMetadataValue) {
-                            return 1;
-                        } else if (o2 instanceof RelationshipMetadataValue) {
-                            return -1;
-                        }
+            metadataValues.sort((o1, o2) -> {
+                int compare = o1.getPlace() - o2.getPlace();
+                if (compare == 0) {
+                    if (o1 instanceof RelationshipMetadataValue && o2 instanceof RelationshipMetadataValue) {
+                        return compare;
+                    } else if (o1 instanceof RelationshipMetadataValue) {
+                        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 int compare(MetadataValue o1, MetadataValue o2) {
                     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.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 Iterator<Item> findAllUnfiltered(Context context) throws SQLException {
         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 List<Collection> getCollectionsNotLinked(Context context, Item item) thro
         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 int countItemsWithEdit(Context context) throws SQLException, SearchServic
      * @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 @@ protected void addDefaultPoliciesNotInPlace(Context context, DSpaceObject dso,
             if (!authorizeService
                 .isAnIdenticalPolicyAlreadyInPlace(context, dso, defaultPolicy.getGroup(), Constants.READ,
                     defaultPolicy.getID()) &&
-                   ((!appendMode && this.isNotAlreadyACustomRPOfThisTypeOnDSO(context, dso)) ||
-                    (appendMode && this.shouldBeAppended(context, dso, defaultPolicy)))) {
+                   (!appendMode && this.isNotAlreadyACustomRPOfThisTypeOnDSO(context, dso) ||
+                    appendMode && this.shouldBeAppended(context, dso, defaultPolicy))) {
                 ResourcePolicy newPolicy = resourcePolicyService.clone(context, defaultPolicy);
                 newPolicy.setdSpaceObject(dso);
                 newPolicy.setAction(Constants.READ);
@@ -1222,7 +1223,7 @@ private boolean isNotAlreadyACustomRPOfThisTypeOnDSO(Context context, DSpaceObje
      * Check if the provided default policy should be appended or not to the final
      * item. If an item has at least one custom READ policy any anonymous READ
      * policy with empty start/end date should be skipped
-     * 
+     *
      * @param context       DSpace context
      * @param dso           DSpace object to check for custom read RP
      * @param defaultPolicy The policy to check
@@ -1611,7 +1612,7 @@ public List<MetadataValue> getMetadata(Item item, String schema, String element,
             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 @@ protected void moveSingleMetadataValue(Context context, Item dso, int place, Met
         }
     }
 
-    /**
-     * 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.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 void setMetadataField(MetadataField metadataField) {
         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());
+    }
+
+}