Merge branch 'main' into DS-8636

.github/workflows/build.yml

@@ -79,6 +79,39 @@ jobs:
           name: ${{ matrix.type }} results
           path: ${{ matrix.resultsdir }}
 
-      # https://github.com/codecov/codecov-action
+      # Upload code coverage report to artifact, so that it can be shared with the 'codecov' job (see below)
+      - name: Upload code coverage report to Artifact
+        uses: actions/upload-artifact@v3
+        with:
+          name: ${{ matrix.type }} coverage report
+          path: 'dspace/target/site/jacoco-aggregate/jacoco.xml'
+          retention-days: 14
+
+  # Codecov upload is a separate job in order to allow us to restart this separate from the entire build/test
+  # job above. This is necessary because Codecov uploads seem to randomly fail at times.
+  # See https://community.codecov.com/t/upload-issues-unable-to-locate-build-via-github-actions-api/3954
+  codecov:
+    # Must run after 'tests' job above
+    needs: tests
+    runs-on: ubuntu-latest
+    steps:
+      - name: Checkout
+        uses: actions/checkout@v3
+
+      # Download artifacts from previous 'tests' job
+      - name: Download coverage artifacts
+        uses: actions/download-artifact@v3
+
+      # Now attempt upload to Codecov using its action.
+      # NOTE: We use a retry action to retry the Codecov upload if it fails the first time.
+      #
+      # Retry action: https://github.com/marketplace/actions/retry-action
+      # Codecov action: https://github.com/codecov/codecov-action
       - name: Upload coverage to Codecov.io
-        uses: codecov/codecov-action@v3
+        uses: Wandalen/wretry.action@v1.0.36
+        with:
+          action: codecov/codecov-action@v3
+          # Try upload 5 times max
+          attempt_limit: 5
+          # Run again in 30 seconds
+          attempt_delay: 30000

.github/workflows/issue_opened.yml

@@ -16,7 +16,7 @@ jobs:
       # Only add to project board if issue is flagged as "needs triage" or has no labels
       # NOTE: By default we flag new issues as "needs triage" in our issue template
       if: (contains(github.event.issue.labels.*.name, 'needs triage') || join(github.event.issue.labels.*.name) == '')
-      uses: actions/add-to-project@v0.3.0
+      uses: actions/add-to-project@v0.5.0
       # Note, the authentication token below is an ORG level Secret. 
       # It must be created/recreated manually via a personal access token with admin:org, project, public_repo permissions
       # See: https://docs.github.com/en/actions/configuring-and-managing-workflows/authenticating-with-the-github_token#permissions-for-the-github_token

.github/workflows/label_merge_conflicts.yml

@@ -23,7 +23,7 @@ jobs:
     steps:
       # See: https://github.com/prince-chrismc/label-merge-conflicts-action
       - name: Auto-label PRs with merge conflicts
-        uses: prince-chrismc/label-merge-conflicts-action@v2
+        uses: prince-chrismc/label-merge-conflicts-action@v3
         # Add "merge conflict" label if a merge conflict is detected. Remove it when resolved.
         # Note, the authentication token is created automatically
         # See: https://docs.github.com/en/actions/configuring-and-managing-workflows/authenticating-with-the-github_token

dspace-api/pom.xml

@@ -776,7 +776,7 @@
         <dependency>
             <groupId>org.json</groupId>
             <artifactId>json</artifactId>
-            <version>20180130</version>
+            <version>20230227</version>
         </dependency>
 
         <!-- Useful for testing command-line tools -->

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

@@ -332,8 +332,8 @@ public void updateLastModified(Context context, Bitstream bitstream) {
     }
 
     @Override
-    public List<Bitstream> findDeletedBitstreams(Context context) throws SQLException {
-        return bitstreamDAO.findDeletedBitstreams(context);
+    public List<Bitstream> findDeletedBitstreams(Context context, int limit, int offset) throws SQLException {
+        return bitstreamDAO.findDeletedBitstreams(context, limit, offset);
     }
 
     @Override

dspace-api/src/main/java/org/dspace/content/dao/BitstreamDAO.java

@@ -29,7 +29,7 @@ public interface BitstreamDAO extends DSpaceObjectLegacySupportDAO<Bitstream> {
 
     public Iterator<Bitstream> findAll(Context context, int limit, int offset) throws SQLException;
 
-    public List<Bitstream> findDeletedBitstreams(Context context) throws SQLException;
+    public List<Bitstream> findDeletedBitstreams(Context context, int limit, int offset) throws SQLException;
 
     public List<Bitstream> findDuplicateInternalIdentifier(Context context, Bitstream bitstream) throws SQLException;
 

dspace-api/src/main/java/org/dspace/content/dao/impl/BitstreamDAOImpl.java

@@ -41,13 +41,14 @@ protected BitstreamDAOImpl() {
     }
 
     @Override
-    public List<Bitstream> findDeletedBitstreams(Context context) throws SQLException {
+    public List<Bitstream> findDeletedBitstreams(Context context, int limit, int offset) throws SQLException {
         CriteriaBuilder criteriaBuilder = getCriteriaBuilder(context);
         CriteriaQuery criteriaQuery = getCriteriaQuery(criteriaBuilder, Bitstream.class);
         Root<Bitstream> bitstreamRoot = criteriaQuery.from(Bitstream.class);
         criteriaQuery.select(bitstreamRoot);
+        criteriaQuery.orderBy(criteriaBuilder.desc(bitstreamRoot.get(Bitstream_.ID)));
         criteriaQuery.where(criteriaBuilder.equal(bitstreamRoot.get(Bitstream_.deleted), true));
-        return list(context, criteriaQuery, false, Bitstream.class, -1, -1);
+        return list(context, criteriaQuery, false, Bitstream.class, limit, offset);
 
     }
 

dspace-api/src/main/java/org/dspace/content/service/BitstreamService.java

@@ -183,7 +183,7 @@ public InputStream retrieve(Context context, Bitstream bitstream)
      * @return a list of all bitstreams that have been "deleted"
      * @throws SQLException if database error
      */
-    public List<Bitstream> findDeletedBitstreams(Context context) throws SQLException;
+    public List<Bitstream> findDeletedBitstreams(Context context, int limit, int offset) throws SQLException;
 
 
     /**

dspace-api/src/main/java/org/dspace/identifier/doi/DOIConsumer.java

@@ -141,7 +141,6 @@ public void consume(Context ctx, Event event) throws Exception {
                             + item.getID() + " and DOI " + doi + ".", ex);
                 }
             }
-            ctx.commit();
         }
     }
 

dspace-api/src/main/java/org/dspace/storage/bitstore/BitstreamStorageServiceImpl.java

@@ -17,6 +17,7 @@
 import java.util.UUID;
 import javax.annotation.Nullable;
 
+import org.apache.commons.collections.CollectionUtils;
 import org.apache.commons.collections4.MapUtils;
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
@@ -224,25 +225,62 @@ public InputStream retrieve(Context context, Bitstream bitstream)
     @Override
     public void cleanup(boolean deleteDbRecords, boolean verbose) throws SQLException, IOException, AuthorizeException {
         Context context = new Context(Context.Mode.BATCH_EDIT);
-        int commitCounter = 0;
+
+        int offset = 0;
+        int limit = 100;
+
+        int cleanedBitstreamCount = 0;
+
+        int deletedBitstreamCount = bitstreamService.countDeletedBitstreams(context);
+        System.out.println("Found " + deletedBitstreamCount + " deleted bistream to cleanup");
 
         try {
             context.turnOffAuthorisationSystem();
 
-            List<Bitstream> storage = bitstreamService.findDeletedBitstreams(context);
-            for (Bitstream bitstream : storage) {
-                UUID bid = bitstream.getID();
-                Map wantedMetadata = new HashMap();
-                wantedMetadata.put("size_bytes", null);
-                wantedMetadata.put("modified", null);
-                Map receivedMetadata = this.getStore(bitstream.getStoreNumber()).about(bitstream, wantedMetadata);
+            while (cleanedBitstreamCount < deletedBitstreamCount) {
 
+                List<Bitstream> storage = bitstreamService.findDeletedBitstreams(context, limit, offset);
+
+                if (CollectionUtils.isEmpty(storage)) {
+                    break;
+                }
+
+                for (Bitstream bitstream : storage) {
+                    UUID bid = bitstream.getID();
+                    Map wantedMetadata = new HashMap();
+                    wantedMetadata.put("size_bytes", null);
+                    wantedMetadata.put("modified", null);
+                    Map receivedMetadata = this.getStore(bitstream.getStoreNumber()).about(bitstream, wantedMetadata);
+
+
+                    // Make sure entries which do not exist are removed
+                    if (MapUtils.isEmpty(receivedMetadata)) {
+                        log.debug("bitstore.about is empty, so file is not present");
+                        if (deleteDbRecords) {
+                            log.debug("deleting record");
+                            if (verbose) {
+                                System.out.println(" - Deleting bitstream information (ID: " + bid + ")");
+                            }
+                            checksumHistoryService.deleteByBitstream(context, bitstream);
+                            if (verbose) {
+                                System.out.println(" - Deleting bitstream record from database (ID: " + bid + ")");
+                            }
+                            bitstreamService.expunge(context, bitstream);
+                        }
+                        context.uncacheEntity(bitstream);
+                        continue;
+                    }
+
+                    // This is a small chance that this is a file which is
+                    // being stored -- get it next time.
+                    if (isRecent(Long.valueOf(receivedMetadata.get("modified").toString()))) {
+                        log.debug("file is recent");
+                        context.uncacheEntity(bitstream);
+                        continue;
+                    }
 
-                // Make sure entries which do not exist are removed
-                if (MapUtils.isEmpty(receivedMetadata)) {
-                    log.debug("bitstore.about is empty, so file is not present");
                     if (deleteDbRecords) {
-                        log.debug("deleting record");
+                        log.debug("deleting db record");
                         if (verbose) {
                             System.out.println(" - Deleting bitstream information (ID: " + bid + ")");
                         }
@@ -252,64 +290,42 @@ public void cleanup(boolean deleteDbRecords, boolean verbose) throws SQLExceptio
                         }
                         bitstreamService.expunge(context, bitstream);
                     }
-                    context.uncacheEntity(bitstream);
-                    continue;
-                }
-
-                // This is a small chance that this is a file which is
-                // being stored -- get it next time.
-                if (isRecent(Long.valueOf(receivedMetadata.get("modified").toString()))) {
-                    log.debug("file is recent");
-                    context.uncacheEntity(bitstream);
-                    continue;
-                }
 
-                if (deleteDbRecords) {
-                    log.debug("deleting db record");
-                    if (verbose) {
-                        System.out.println(" - Deleting bitstream information (ID: " + bid + ")");
+                    if (isRegisteredBitstream(bitstream.getInternalId())) {
+                        context.uncacheEntity(bitstream);
+                        continue; // do not delete registered bitstreams
                     }
-                    checksumHistoryService.deleteByBitstream(context, bitstream);
-                    if (verbose) {
-                        System.out.println(" - Deleting bitstream record from database (ID: " + bid + ")");
+
+
+                    // Since versioning allows for multiple bitstreams, check if the internal
+                    // identifier isn't used on
+                    // another place
+                    if (bitstreamService.findDuplicateInternalIdentifier(context, bitstream).isEmpty()) {
+                        this.getStore(bitstream.getStoreNumber()).remove(bitstream);
+
+                        String message = ("Deleted bitstreamID " + bid + ", internalID " + bitstream.getInternalId());
+                        if (log.isDebugEnabled()) {
+                            log.debug(message);
+                        }
+                        if (verbose) {
+                            System.out.println(message);
+                        }
                     }
-                    bitstreamService.expunge(context, bitstream);
-                }
 
-                if (isRegisteredBitstream(bitstream.getInternalId())) {
                     context.uncacheEntity(bitstream);
-                    continue;            // do not delete registered bitstreams
                 }
 
+                // Commit actual changes to DB after dispatch events
+                System.out.print("Performing incremental commit to the database...");
+                context.commit();
+                System.out.println(" Incremental commit done!");
 
-                // Since versioning allows for multiple bitstreams, check if the internal identifier isn't used on
-                // another place
-                if (bitstreamService.findDuplicateInternalIdentifier(context, bitstream).isEmpty()) {
-                    this.getStore(bitstream.getStoreNumber()).remove(bitstream);
-
-                    String message = ("Deleted bitstreamID " + bid + ", internalID " + bitstream.getInternalId());
-                    if (log.isDebugEnabled()) {
-                        log.debug(message);
-                    }
-                    if (verbose) {
-                        System.out.println(message);
-                    }
-                }
+                cleanedBitstreamCount = cleanedBitstreamCount + storage.size();
 
-                // Make sure to commit our outstanding work every 100
-                // iterations. Otherwise you risk losing the entire transaction
-                // if we hit an exception, which isn't useful at all for large
-                // amounts of bitstreams.
-                commitCounter++;
-                if (commitCounter % 100 == 0) {
-                    context.dispatchEvents();
-                    // Commit actual changes to DB after dispatch events
-                    System.out.print("Performing incremental commit to the database...");
-                    context.commit();
-                    System.out.println(" Incremental commit done!");
+                if (!deleteDbRecords) {
+                    offset = offset + limit;
                 }
 
-                context.uncacheEntity(bitstream);
             }
 
             System.out.print("Committing changes to the database...");

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

@@ -39,6 +39,7 @@
 import org.springframework.web.bind.annotation.PathVariable;
 import org.springframework.web.bind.annotation.RequestMapping;
 import org.springframework.web.bind.annotation.RequestMethod;
+import org.springframework.web.bind.annotation.RequestParam;
 import org.springframework.web.bind.annotation.RestController;
 
 /**
@@ -69,6 +70,8 @@ public class ItemOwningCollectionUpdateRestController {
      * moving the item to the new collection.
      *
      * @param uuid The UUID of the item that will be moved
+     * @param inheritCollectionPolicies   Boolean flag whether to inherit the target collection policies when
+     *                                    moving the item
      * @param response The response object
      * @param request  The request object
      * @return The wrapped resource containing the new owning collection or null when the item was not moved
@@ -79,7 +82,10 @@ public class ItemOwningCollectionUpdateRestController {
     @RequestMapping(method = RequestMethod.PUT, consumes = {"text/uri-list"})
     @PreAuthorize("hasPermission(#uuid, 'ITEM','WRITE')")
     @PostAuthorize("returnObject != null")
-    public CollectionRest move(@PathVariable UUID uuid, HttpServletResponse response,
+    public CollectionRest move(@PathVariable UUID uuid,
+                               @RequestParam(name = "inheritPolicies", defaultValue = "false")
+                               Boolean inheritCollectionPolicies,
+                               HttpServletResponse response,
                                HttpServletRequest request)
             throws SQLException, IOException, AuthorizeException {
         Context context = ContextUtil.obtainContext(request);
@@ -91,7 +97,8 @@ public CollectionRest move(@PathVariable UUID uuid, HttpServletResponse response
                                                            "or the data cannot be resolved to a collection.");
         }
 
-        Collection targetCollection = performItemMove(context, uuid, (Collection) dsoList.get(0));
+        Collection targetCollection = performItemMove(context, uuid, (Collection) dsoList.get(0),
+                inheritCollectionPolicies);
 
         if (targetCollection == null) {
             return null;
@@ -107,17 +114,19 @@ public CollectionRest move(@PathVariable UUID uuid, HttpServletResponse response
      * @param item              The item to be moved
      * @param currentCollection The current owning collection of the item
      * @param targetCollection  The target collection of the item
+     * @param inheritPolicies   Boolean flag whether to inherit the target collection policies when moving the item
      * @return The target collection
      * @throws SQLException       If something goes wrong
      * @throws IOException        If something goes wrong
      * @throws AuthorizeException If the user is not authorized to perform the move action
      */
     private Collection moveItem(final Context context, final Item item, final Collection currentCollection,
-                                final Collection targetCollection)
+                                final Collection targetCollection,
+                                final boolean inheritPolicies)
             throws SQLException, IOException, AuthorizeException {
-        itemService.move(context, item, currentCollection, targetCollection);
-        //Necessary because Controller does not pass through general RestResourceController, and as such does not do its
-        //  commit in DSpaceRestRepository.createAndReturn() or similar
+        itemService.move(context, item, currentCollection, targetCollection, inheritPolicies);
+        // Necessary because Controller does not pass through general RestResourceController, and as such does not do
+        // its commit in DSpaceRestRepository.createAndReturn() or similar
         context.commit();
 
         return context.reloadEntity(targetCollection);
@@ -129,12 +138,14 @@ private Collection moveItem(final Context context, final Item item, final Collec
      * @param context          The context Object
      * @param itemUuid         The uuid of the item to be moved
      * @param targetCollection The target collection
+     * @param inheritPolicies  Whether to inherit the target collection policies when moving the item
      * @return The new owning collection of the item when authorized or null when not authorized
      * @throws SQLException       If something goes wrong
      * @throws IOException        If something goes wrong
      * @throws AuthorizeException If the user is not authorized to perform the move action
      */
-    private Collection performItemMove(final Context context, final UUID itemUuid, final Collection targetCollection)
+    private Collection performItemMove(final Context context, final UUID itemUuid, final Collection targetCollection,
+                                       boolean inheritPolicies)
             throws SQLException, IOException, AuthorizeException {
 
         Item item = itemService.find(context, itemUuid);
@@ -153,7 +164,7 @@ private Collection performItemMove(final Context context, final UUID itemUuid, f
 
         if (authorizeService.authorizeActionBoolean(context, currentCollection, Constants.ADMIN)) {
 
-            return moveItem(context, item, currentCollection, targetCollection);
+            return moveItem(context, item, currentCollection, targetCollection, inheritPolicies);
         }
 
         return null;