65571: REST changes to delete of an item

- fix RelationshipDeleteRestRepositoryIT test failures
    - fix issue with deleting a collection with ResourcePolicies
- remove a redundant itemService from ItemRestRepository
- implement input validation on the copyVirtualMetadata parameter
    - add test for this
- use Object.deepEquals() for array comparison
- documented deleteMultipleRelationshipsCopyVirtualMetadata()

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

@@ -761,6 +761,8 @@ public class CollectionServiceImpl extends DSpaceObjectServiceImpl<Collection> i
         // Remove any workflow roles
         collectionRoleService.deleteByCollection(context, collection);
 
+        collection.getResourcePolicies().clear();
+
         // Remove default administrators group
         Group g = collection.getAdministrators();
 

dspace-server-webapp/src/main/java/org/dspace/app/rest/repository/ItemRestRepository.java

@@ -10,11 +10,10 @@ package org.dspace.app.rest.repository;
 import java.io.IOException;
 import java.sql.SQLException;
 import java.util.ArrayList;
-import java.util.Arrays;
 import java.util.Iterator;
 import java.util.List;
+import java.util.Objects;
 import java.util.UUID;
-import java.util.stream.Collectors;
 import javax.servlet.ServletInputStream;
 import javax.servlet.http.HttpServletRequest;
 
@@ -64,7 +63,9 @@ public class ItemRestRepository extends DSpaceObjectRestRepository<Item, ItemRes
 
     private static final Logger log = Logger.getLogger(ItemRestRepository.class);
 
-    private final ItemService is;
+    private static final String[] COPYVIRTUAL_ALL = {"all"};
+    private static final String[] COPYVIRTUAL_CONFIGURED = {"configured"};
+    private static final String REQUESTPARAMETER_COPYVIRTUALMETADATA = "copyVirtualMetadata";
 
     @Autowired
     MetadataConverter metadataConverter;
@@ -94,7 +95,6 @@ public class ItemRestRepository extends DSpaceObjectRestRepository<Item, ItemRes
                               ItemConverter dsoConverter,
                               ItemPatch dsoPatch) {
         super(dsoService, dsoConverter, dsoPatch);
-        this.is = dsoService;
     }
 
     @Override
@@ -102,7 +102,7 @@ public class ItemRestRepository extends DSpaceObjectRestRepository<Item, ItemRes
     public ItemRest findOne(Context context, UUID id) {
         Item item = null;
         try {
-            item = is.find(context, id);
+            item = itemService.find(context, id);
         } catch (SQLException e) {
             throw new RuntimeException(e.getMessage(), e);
         }
@@ -119,8 +119,8 @@ public class ItemRestRepository extends DSpaceObjectRestRepository<Item, ItemRes
         List<Item> items = new ArrayList<Item>();
         int total = 0;
         try {
-            total = is.countTotal(context);
+            total = itemService.countTotal(context);
-            it = is.findAll(context, pageable.getPageSize(), pageable.getOffset());
+            it = itemService.findAll(context, pageable.getPageSize(), pageable.getOffset());
             while (it.hasNext()) {
                 Item i = it.next();
                 items.add(i);
@@ -147,14 +147,14 @@ public class ItemRestRepository extends DSpaceObjectRestRepository<Item, ItemRes
         Context context = obtainContext();
         if (itemRest.getWithdrawn() != item.isWithdrawn()) {
             if (itemRest.getWithdrawn()) {
-                is.withdraw(context, item);
+                itemService.withdraw(context, item);
             } else {
-                is.reinstate(context, item);
+                itemService.reinstate(context, item);
             }
         }
         if (itemRest.getDiscoverable() != item.isDiscoverable()) {
             item.setDiscoverable(itemRest.getDiscoverable());
-            is.update(context, item);
+            itemService.update(context, item);
         }
     }
 
@@ -172,16 +172,17 @@ public class ItemRestRepository extends DSpaceObjectRestRepository<Item, ItemRes
     @PreAuthorize("hasAuthority('ADMIN')")
     protected void delete(Context context, UUID id) throws AuthorizeException {
         String[] copyVirtual =
-            requestService.getCurrentRequest().getServletRequest().getParameterValues("copyVirtualMetadata");
+            requestService.getCurrentRequest().getServletRequest()
+                .getParameterValues(REQUESTPARAMETER_COPYVIRTUALMETADATA);
 
         Item item = null;
         try {
-            item = is.find(context, id);
+            item = itemService.find(context, id);
             if (item == null) {
                 throw new ResourceNotFoundException(ItemRest.CATEGORY + "." + ItemRest.NAME +
                                                         " with id: " + id + " not found");
             }
-            if (is.isInProgressSubmission(context, item)) {
+            if (itemService.isInProgressSubmission(context, item)) {
                 throw new UnprocessableEntityException("The item cannot be deleted. "
                         + "It's part of a in-progress submission.");
             }
@@ -194,7 +195,7 @@ public class ItemRestRepository extends DSpaceObjectRestRepository<Item, ItemRes
         }
         try {
             deleteMultipleRelationshipsCopyVirtualMetadata(context, copyVirtual, item);
-            is.delete(context, item);
+            itemService.delete(context, item);
         } catch (SQLException | IOException e) {
             throw new RuntimeException(e.getMessage(), e);
         }
@@ -203,21 +204,31 @@ public class ItemRestRepository extends DSpaceObjectRestRepository<Item, ItemRes
     private void deleteMultipleRelationshipsCopyVirtualMetadata(Context context, String[] copyVirtual, Item item)
         throws SQLException, AuthorizeException {
 
-        if (copyVirtual == null) {
+        if (copyVirtual == null || copyVirtual.length == 0) {
+            // Don't delete nor copy any metadata here if the "copyVirtualMetadata" parameter wasn't passed. The
+            // relationships not deleted in this method will be deleted implicitly by the this.delete() method
+            // anyway.
             return;
         }
-        if (copyVirtual.length == 1 && StringUtils.equals("all", copyVirtual[0])) {
+        if (Objects.deepEquals(copyVirtual, COPYVIRTUAL_ALL)) {
+            // Option 1: Copy all virtual metadata of this item to its related items. Iterate over all of the item's
+            //           relationships and copy their data.
             for (Relationship relationship : relationshipService.findByItem(context, item)) {
                 deleteRelationshipCopyVirtualMetadata(item, relationship);
             }
-        } else if (copyVirtual.length == 1 && StringUtils.equals("configured", copyVirtual[0])) {
+        } else if (Objects.deepEquals(copyVirtual, COPYVIRTUAL_CONFIGURED)) {
+            // Option 2: Use a configuration value to determine if virtual metadata needs to be copied. Iterate over all
+            //           of the item's relationships and copy their data depending on the
+            //           configuration.
             for (Relationship relationship : relationshipService.findByItem(context, item)) {
                 relationshipService.delete(obtainContext(), relationship);
             }
-        } else if (copyVirtual.length > 0) {
+        } else {
-            List<Integer> relationshipIds =
+            // Option 3: Copy the virtual metadata of selected types of this item to its related items. The copyVirtual
-                Arrays.stream(copyVirtual).filter(StringUtils::isNumeric).collect(Collectors.toList())
+            //           array should only contain numeric values at this point. These values are used to select the
-                    .stream().map(Integer::parseInt).collect(Collectors.toList());
+            //           types. Iterate over all selected types and copy the corresponding values to this item's
+            //           relatives.
+            List<Integer> relationshipIds = parseVirtualMetadataTypes(copyVirtual);
             for (Integer relationshipId : relationshipIds) {
                 RelationshipType relationshipType = relationshipTypeService.find(context, relationshipId);
                 for (Relationship relationship : relationshipService
@@ -229,6 +240,19 @@ public class ItemRestRepository extends DSpaceObjectRestRepository<Item, ItemRes
         }
     }
 
+    private List<Integer> parseVirtualMetadataTypes(String[] copyVirtual) {
+        List<Integer> types = new ArrayList<>();
+        for (String typeString: copyVirtual) {
+            if (!StringUtils.isNumeric(typeString)) {
+                throw new DSpaceBadRequestException("parameter " + REQUESTPARAMETER_COPYVIRTUALMETADATA
+                    + " should only contain a single value '" + COPYVIRTUAL_ALL[0] + "', '" + COPYVIRTUAL_CONFIGURED[0]
+                    + "' or a list of numbers.");
+            }
+            types.add(Integer.parseInt(typeString));
+        }
+        return types;
+    }
+
     private void deleteRelationshipCopyVirtualMetadata(Item itemToDelete, Relationship relationshipToDelete)
         throws SQLException, AuthorizeException {
 

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

@@ -18,7 +18,6 @@ import java.util.List;
 
 import org.dspace.app.rest.builder.CollectionBuilder;
 import org.dspace.app.rest.builder.CommunityBuilder;
-import org.dspace.app.rest.builder.GroupBuilder;
 import org.dspace.app.rest.builder.ItemBuilder;
 import org.dspace.app.rest.builder.RelationshipBuilder;
 import org.dspace.app.rest.test.AbstractEntityIntegrationTest;
@@ -106,7 +105,6 @@ public class RelationshipDeleteRestRepositoryIT extends AbstractEntityIntegratio
             collectionAdmin = ePersonService.findByEmail(context, "collectionAdminTest@email.com");
             if (collectionAdmin != null) {
                 ePersonService.delete(context, collectionAdmin);
-                context.commit();
             }
         } catch (Exception e) {
             throw new RuntimeException(e);
@@ -454,6 +452,38 @@ public class RelationshipDeleteRestRepositoryIT extends AbstractEntityIntegratio
         assertThat(projectRelationships.size(), equalTo(0));
     }
 
+    @Test
+    public void deleteItemCopyVirtualMetadataInvalid() throws Exception {
+        initPersonProjectPublication();
+
+        getClient(adminAuthToken).perform(
+            delete("/api/core/items/" + personItem.getID()
+                + "?copyVirtualMetadata=" + publicationPersonRelationshipType.getID()
+                + "&copyVirtualMetadata=" + personProjectRelationshipType.getID()
+                + "&copyVirtualMetadata=SomeThingWrong"))
+            .andExpect(status().isBadRequest());
+
+        publicationItem = itemService.find(context, publicationItem.getID());
+        List<MetadataValue> publicationAuthorList =
+            itemService.getMetadata(publicationItem, "dc", "contributor", "author", Item.ANY);
+        assertThat(publicationAuthorList.size(), equalTo(1));
+        assertThat(publicationAuthorList.get(0).getValue(), equalTo("Smith, Donald"));
+        assertThat(publicationAuthorList.get(0).getAuthority(), startsWith("virtual::"));
+        List<MetadataValue> publicationRelationships = itemService.getMetadata(publicationItem,
+            "relation", "isAuthorOfPublication", Item.ANY, Item.ANY);
+        assertThat(publicationRelationships.size(), equalTo(1));
+
+        projectItem = itemService.find(context, projectItem.getID());
+        List<MetadataValue> projectAuthorList =
+            itemService.getMetadata(projectItem, "project", "contributor", "author", Item.ANY);
+        assertThat(projectAuthorList.size(), equalTo(1));
+        assertThat(projectAuthorList.get(0).getValue(), equalTo("Smith, Donald"));
+        assertThat(projectAuthorList.get(0).getAuthority(), startsWith("virtual::"));
+        List<MetadataValue> projectRelationships = itemService.getMetadata(projectItem,
+            "relation", "isPersonOfProject", Item.ANY, Item.ANY);
+        assertThat(projectRelationships.size(), equalTo(1));
+    }
+
     @Test
     public void deleteItemCopyVirtualMetadataAllNoPermissions() throws Exception {
         initPersonProjectPublication();
@@ -461,6 +491,26 @@ public class RelationshipDeleteRestRepositoryIT extends AbstractEntityIntegratio
         getClient(getAuthToken(eperson.getEmail(), password)).perform(
             delete("/api/core/items/" + personItem.getID()))
             .andExpect(status().isForbidden());
+
+        publicationItem = itemService.find(context, publicationItem.getID());
+        List<MetadataValue> publicationAuthorList =
+            itemService.getMetadata(publicationItem, "dc", "contributor", "author", Item.ANY);
+        assertThat(publicationAuthorList.size(), equalTo(1));
+        assertThat(publicationAuthorList.get(0).getValue(), equalTo("Smith, Donald"));
+        assertThat(publicationAuthorList.get(0).getAuthority(), startsWith("virtual::"));
+        List<MetadataValue> publicationRelationships = itemService.getMetadata(publicationItem,
+            "relation", "isAuthorOfPublication", Item.ANY, Item.ANY);
+        assertThat(publicationRelationships.size(), equalTo(1));
+
+        projectItem = itemService.find(context, projectItem.getID());
+        List<MetadataValue> projectAuthorList =
+            itemService.getMetadata(projectItem, "project", "contributor", "author", Item.ANY);
+        assertThat(projectAuthorList.size(), equalTo(1));
+        assertThat(projectAuthorList.get(0).getValue(), equalTo("Smith, Donald"));
+        assertThat(projectAuthorList.get(0).getAuthority(), startsWith("virtual::"));
+        List<MetadataValue> projectRelationships = itemService.getMetadata(projectItem,
+            "relation", "isPersonOfProject", Item.ANY, Item.ANY);
+        assertThat(projectRelationships.size(), equalTo(1));
     }
 
     @Test
@@ -470,6 +520,26 @@ public class RelationshipDeleteRestRepositoryIT extends AbstractEntityIntegratio
         getClient(getAuthToken(collectionAdmin.getEmail(), password)).perform(
             delete("/api/core/items/" + personItem.getID()))
             .andExpect(status().isForbidden());
+
+        publicationItem = itemService.find(context, publicationItem.getID());
+        List<MetadataValue> publicationAuthorList =
+            itemService.getMetadata(publicationItem, "dc", "contributor", "author", Item.ANY);
+        assertThat(publicationAuthorList.size(), equalTo(1));
+        assertThat(publicationAuthorList.get(0).getValue(), equalTo("Smith, Donald"));
+        assertThat(publicationAuthorList.get(0).getAuthority(), startsWith("virtual::"));
+        List<MetadataValue> publicationRelationships = itemService.getMetadata(publicationItem,
+            "relation", "isAuthorOfPublication", Item.ANY, Item.ANY);
+        assertThat(publicationRelationships.size(), equalTo(1));
+
+        projectItem = itemService.find(context, projectItem.getID());
+        List<MetadataValue> projectAuthorList =
+            itemService.getMetadata(projectItem, "project", "contributor", "author", Item.ANY);
+        assertThat(projectAuthorList.size(), equalTo(1));
+        assertThat(projectAuthorList.get(0).getValue(), equalTo("Smith, Donald"));
+        assertThat(projectAuthorList.get(0).getAuthority(), startsWith("virtual::"));
+        List<MetadataValue> projectRelationships = itemService.getMetadata(projectItem,
+            "relation", "isPersonOfProject", Item.ANY, Item.ANY);
+        assertThat(projectRelationships.size(), equalTo(1));
     }
 
     @Test
@@ -480,5 +550,25 @@ public class RelationshipDeleteRestRepositoryIT extends AbstractEntityIntegratio
             delete("/api/core/items/" + personItem.getID()
                 + "?copyVirtualMetadata=" + publicationPersonRelationshipType.getID()))
             .andExpect(status().isForbidden());
+
+        publicationItem = itemService.find(context, publicationItem.getID());
+        List<MetadataValue> publicationAuthorList =
+            itemService.getMetadata(publicationItem, "dc", "contributor", "author", Item.ANY);
+        assertThat(publicationAuthorList.size(), equalTo(1));
+        assertThat(publicationAuthorList.get(0).getValue(), equalTo("Smith, Donald"));
+        assertThat(publicationAuthorList.get(0).getAuthority(), startsWith("virtual::"));
+        List<MetadataValue> publicationRelationships = itemService.getMetadata(publicationItem,
+            "relation", "isAuthorOfPublication", Item.ANY, Item.ANY);
+        assertThat(publicationRelationships.size(), equalTo(1));
+
+        projectItem = itemService.find(context, projectItem.getID());
+        List<MetadataValue> projectAuthorList =
+            itemService.getMetadata(projectItem, "project", "contributor", "author", Item.ANY);
+        assertThat(projectAuthorList.size(), equalTo(1));
+        assertThat(projectAuthorList.get(0).getValue(), equalTo("Smith, Donald"));
+        assertThat(projectAuthorList.get(0).getAuthority(), startsWith("virtual::"));
+        List<MetadataValue> projectRelationships = itemService.getMetadata(projectItem,
+            "relation", "isPersonOfProject", Item.ANY, Item.ANY);
+        assertThat(projectRelationships.size(), equalTo(1));
     }
 }

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

@@ -226,13 +226,8 @@ public abstract class AbstractDSpaceObjectBuilder<T extends DSpaceObject>
             if (attachedDso != null) {
                 getService().delete(c, attachedDso);
             }
-            try {
             c.complete();
         }
-            catch (Exception e) {
-                throw new RuntimeException(e);
-            }
-        }
 
         indexingService.commit();
     }

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

@@ -131,7 +131,6 @@ public class CollectionBuilder extends AbstractDSpaceObjectBuilder<Collection> {
     @Override
     public void cleanup() throws Exception {
         deleteWorkflowGroups(collection);
-        //collectionService.removeAdministrators(context, collection);
         delete(collection);
     }