DD-1563 Some improvement points to the dd-manage-deposits (#8)

* DD-1492 dd-manage-deposits improvemnet and fixes for DD-1419 * Update RPM link was not correct * DD-1563 improvement points --------- Co-authored-by: Ali Sheikhi <[email protected]> Co-authored-by: Ali Sheikhi <[email protected]>
DANS-KNAW · Jul 2, 2024 · bc5fbbd · bc5fbbd
1 parent 496cff5
commit bc5fbbd
Show file tree

Hide file tree

Showing 8 changed files with 128 additions and 24 deletions.
diff --git a/docs/api/api.yml b/docs/api/api.yml
@@ -108,4 +108,4 @@ paths:
         '204':
           description: the deletion was carried out successfully
         '500':
-          description: the deletion could not be carred out
+          description: the deletion could not be carried out
diff --git a/src/main/java/nl/knaw/dans/managedeposit/core/service/IngestPathMonitor.java b/src/main/java/nl/knaw/dans/managedeposit/core/service/IngestPathMonitor.java
@@ -80,7 +80,7 @@ public void start() throws Exception {
         }
         catch (IOException | InterruptedException e) {
             log.error(e.getMessage(), e);
-            throw new InvalidTransferItemException(e.getMessage());
+            throw new Exception(e);
         }
     }
 
@@ -100,8 +100,12 @@ public void stop() throws Exception {
 
     @Override
     public void onFileCreate(File file) {
+        Path absolutePath = Path.of(file.getAbsolutePath());
         log.debug("onFileCreate: '{}'", file.getAbsolutePath());
-        depositStatusUpdater.onDepositCreate(file);
+        if (checkCleanBaseFolders(absolutePath))
+            depositStatusUpdater.onDepositCreate(file);
+        else
+            log.error("Error: file '{}' is directly in a base-folder.", file.getAbsolutePath());
     }
 
     @Override
@@ -116,4 +120,13 @@ public void onFileChange(File file) {
         depositStatusUpdater.onDepositChange(file);
     }
 
+    private boolean checkCleanBaseFolders(Path depositPropertiesFile) {
+        Path parent = depositPropertiesFile.getParent();
+        for (Path folder : toMonitorPaths) {
+            if (parent.endsWith(folder))
+                return false;
+        }
+        return true;
+    }
+
 }
diff --git a/...service/InvalidTransferItemException.java → ...ice/InvalidRequestParameterException.java b/...service/InvalidTransferItemException.java → ...ice/InvalidRequestParameterException.java
@@ -15,13 +15,13 @@
  */
 package nl.knaw.dans.managedeposit.core.service;
 
-public class InvalidTransferItemException extends Exception {
+public class InvalidRequestParameterException extends RuntimeException {
 
-    public InvalidTransferItemException(String msg, Throwable t) {
+    public InvalidRequestParameterException(String msg, Throwable t) {
         super(msg, t);
     }
 
-    public InvalidTransferItemException(String msg) {
+    public InvalidRequestParameterException(String msg) {
         this(msg, null);
     }
 }
diff --git a/src/main/java/nl/knaw/dans/managedeposit/db/DepositPropertiesDAO.java b/src/main/java/nl/knaw/dans/managedeposit/db/DepositPropertiesDAO.java
@@ -17,6 +17,7 @@
 
 import io.dropwizard.hibernate.AbstractDAO;
 import nl.knaw.dans.managedeposit.core.DepositProperties;
+import nl.knaw.dans.managedeposit.core.service.InvalidRequestParameterException;
 import org.hibernate.SessionFactory;
 import org.hibernate.query.Query;
 import org.slf4j.Logger;
@@ -86,8 +87,9 @@ public List<DepositProperties> findSelection(Map<String, List<String>> queryPara
 
     public Optional<Integer> deleteSelection(Map<String, List<String>> queryParameters) {
         var criteriaBuilder = currentSession().getCriteriaBuilder();
-        if (queryParameters.isEmpty())                   // Note: all records will be deleted (accidentally) without any specified query parameter
-            return Optional.of(0);
+        if (queryParameters.isEmpty()) {
+            throw new InvalidRequestParameterException("Delete command without argument would make database empty. It is not accepted.");
+        }
 
         CriteriaDelete<DepositProperties> deleteQuery = criteriaBuilder.createCriteriaDelete(DepositProperties.class);
         Root<DepositProperties> root = deleteQuery.from(DepositProperties.class);
@@ -101,11 +103,15 @@ public Optional<Integer> deleteSelection(Map<String, List<String>> queryParamete
 
     private Predicate buildQueryCriteria(Map<String, List<String>> queryParameters, CriteriaBuilder criteriaBuilder, Root<DepositProperties> root) {
         List<Predicate> predicates = new ArrayList<>();
+        boolean startDateSpecifiedAlready = false, endDateSpecifiedAlready = false;
         Predicate predicate;
 
         for (String key : queryParameters.keySet()) {
             List<String> values = queryParameters.get(key);
             String parameter = key.toLowerCase();
+            if (values.isEmpty()) {
+                throw new InvalidRequestParameterException(String.format("Empty value of parameter %s", parameter));
+            }
             //javax.persistence.criteria
             Predicate orPredicateItem;
             List<Predicate> orPredicatesList = new ArrayList<>();
@@ -141,20 +147,29 @@ private Predicate buildQueryCriteria(Map<String, List<String>> queryParameters,
                                 LocalDate date = LocalDate.parse(value, formatter);
                                 var asked_date = OffsetDateTime.of(date.atStartOfDay(), ZoneOffset.UTC);
 
-                                if (parameter.equals("startdate"))
+                                if (parameter.equals("startdate")) {
+                                    if (startDateSpecifiedAlready)
+                                        throw new InvalidRequestParameterException(String.format("Duplicated startdate parameter %s = %s ", parameter, value));
                                     orPredicateItem = criteriaBuilder.greaterThan(root.get("depositCreationTimestamp"), asked_date);
-                                else
+                                    startDateSpecifiedAlready = true;
+                                }
+                                else {
+                                    if (endDateSpecifiedAlready)
+                                        throw new InvalidRequestParameterException(String.format("Duplicated enddate parameter %s = %s ", parameter, value));
                                     orPredicateItem = criteriaBuilder.lessThan(root.get("depositCreationTimestamp"), asked_date);
+                                    endDateSpecifiedAlready = true;
+                                }
                             }
                             catch (DateTimeException e) {
-                                log.warn("Error parsing the date: {}", e.getMessage());
-                                continue;
+                                log.error("Invalid or incorrectly formatted date parameter {}", e.getMessage());
+                                String error = String.format("Invalid or incorrectly formatted parameter %s = %s ", parameter, value);
+                                throw new InvalidRequestParameterException(String.format("Invalid or incorrectly formatted parameter %s = %s ", parameter, value));
                             }
                         }
                         break;
 
                     default:
-                        orPredicateItem = criteriaBuilder.equal(root.get(key), value);
+                        throw new InvalidRequestParameterException(String.format("Unknown parameter %s = %s ", parameter, value));
                 }
                 orPredicatesList.add(orPredicateItem);
             }

diff --git a/src/main/java/nl/knaw/dans/managedeposit/resources/DepositPropertiesDeleteResource.java b/src/main/java/nl/knaw/dans/managedeposit/resources/DepositPropertiesDeleteResource.java
@@ -16,19 +16,27 @@
 package nl.knaw.dans.managedeposit.resources;
 
 import io.dropwizard.hibernate.UnitOfWork;
+import nl.knaw.dans.managedeposit.core.service.InvalidRequestParameterException;
 import nl.knaw.dans.managedeposit.db.DepositPropertiesDAO;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
+import javax.ws.rs.ClientErrorException;
 import javax.ws.rs.Consumes;
 import javax.ws.rs.NotFoundException;
 import javax.ws.rs.POST;
 import javax.ws.rs.Path;
 import javax.ws.rs.Produces;
+import javax.ws.rs.WebApplicationException;
 import javax.ws.rs.core.Context;
 import javax.ws.rs.core.MediaType;
+import javax.ws.rs.core.Response;
 import javax.ws.rs.core.UriInfo;
+import java.util.Optional;
 
 @Path("/delete-deposit")
 public class DepositPropertiesDeleteResource {
+    private static final Logger log = LoggerFactory.getLogger(DepositPropertiesDeleteResource.class);
     private final DepositPropertiesDAO depositPropertiesDAO;
 
     public DepositPropertiesDeleteResource(DepositPropertiesDAO depositPropertiesDAO) {
@@ -39,9 +47,29 @@ public DepositPropertiesDeleteResource(DepositPropertiesDAO depositPropertiesDAO
     @UnitOfWork
     @Produces("text/plain")
     @Consumes(MediaType.TEXT_PLAIN)
-    public String deleteDepositPropertiesUsingParams(@Context UriInfo uriInfo) {
-        int deletedNumber = depositPropertiesDAO.deleteSelection(uriInfo.getQueryParameters()).orElseThrow(() -> new NotFoundException("Not such deposit with given criteria"));
-        return String.format("Deleted number(s): %d.", deletedNumber);
+    public Response deleteDepositPropertiesUsingParams(@Context UriInfo uriInfo) {
+        try {
+            Optional<Integer> deletedNumber = depositPropertiesDAO.deleteSelection(uriInfo.getQueryParameters());
+            if (deletedNumber.isPresent()) {
+                log.debug("Deleted number(s): {}.", deletedNumber);
+                return Response.status(Response.Status.OK)
+                    .encoding(String.format("Deleted number(s): %d.", deletedNumber.get()))
+                    .build();
+            }
+            else {
+                log.warn("Not such deposit with given criteria: {}", uriInfo);
+                throw new NotFoundException(String.format("No such deposit: %s", uriInfo));
+            }
+
+        }
+        catch (InvalidRequestParameterException e) {
+            log.error(e.getMessage());
+            throw new ClientErrorException(e.getMessage(), Response.Status.BAD_REQUEST);
+        }
+        catch (Exception e) {
+            log.error(e.getMessage());
+            throw new WebApplicationException(e.getMessage(), Response.Status.BAD_REQUEST);
+        }
     }
 
 }
diff --git a/src/main/java/nl/knaw/dans/managedeposit/resources/DepositPropertiesReportResource.java b/src/main/java/nl/knaw/dans/managedeposit/resources/DepositPropertiesReportResource.java
@@ -17,20 +17,26 @@
 
 import io.dropwizard.hibernate.UnitOfWork;
 import nl.knaw.dans.managedeposit.core.DepositProperties;
+import nl.knaw.dans.managedeposit.core.service.InvalidRequestParameterException;
 import nl.knaw.dans.managedeposit.db.DepositPropertiesDAO;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
+import javax.ws.rs.ClientErrorException;
 import javax.ws.rs.GET;
 import javax.ws.rs.NotFoundException;
 import javax.ws.rs.Path;
 import javax.ws.rs.PathParam;
 import javax.ws.rs.Produces;
+import javax.ws.rs.WebApplicationException;
 import javax.ws.rs.core.Context;
+import javax.ws.rs.core.Response;
 import javax.ws.rs.core.UriInfo;
-import java.util.List;
 import java.util.Optional;
 
 @Path("/report")
 public class DepositPropertiesReportResource {
+    private static final Logger log = LoggerFactory.getLogger(DepositPropertiesReportResource.class);
     private final DepositPropertiesDAO depositPropertiesDAO;
 
     public DepositPropertiesReportResource(DepositPropertiesDAO depositPropertiesDAO) {
@@ -40,16 +46,35 @@ public DepositPropertiesReportResource(DepositPropertiesDAO depositPropertiesDAO
     @GET
     @UnitOfWork
     @Produces({ "application/json", "text/csv" })
-    public List<DepositProperties> listDepositProperties(@Context UriInfo uriInfo) {
-        return depositPropertiesDAO.findSelection(uriInfo.getQueryParameters());
+    public Response listDepositProperties(@Context UriInfo uriInfo) {
+        try {
+            return Response.status(Response.Status.OK)
+                .entity(depositPropertiesDAO.findSelection(uriInfo.getQueryParameters()))
+                .build();
+        }
+        catch (InvalidRequestParameterException e) {
+            log.error(e.getMessage());
+            throw new ClientErrorException(e.getMessage(), Response.Status.BAD_REQUEST);
+        }
+        catch (Exception e) {
+            log.error(e.getMessage());
+            throw new WebApplicationException(e.getMessage(), Response.Status.BAD_REQUEST);
+        }
+
     }
 
+    @SuppressWarnings("OptionalUsedAsFieldOrParameterType")
     @GET
     @UnitOfWork
     @Produces("application/json")
     @Path("/{depositId}")
     public DepositProperties getDepositId(@PathParam("depositId") Optional<String> depositId) {
-        return depositPropertiesDAO.findById(depositId.get()).orElseThrow(() -> new NotFoundException(String.format("No such deposit: %s", depositId.orElse(""))));
+        if (depositId.isPresent())
+            return depositPropertiesDAO.findById(depositId.get()).orElseThrow(() -> new NotFoundException(String.format("No such deposit: %s", depositId.orElse(""))));
+        else {
+            log.warn("Empty argument 'depositId'");
+            throw new ClientErrorException("Empty argument 'depositId'", Response.Status.BAD_REQUEST);
+        }
     }
 
 }
diff --git a/src/main/java/nl/knaw/dans/managedeposit/resources/DepositPropertiesResource.java b/src/main/java/nl/knaw/dans/managedeposit/resources/DepositPropertiesResource.java
@@ -52,7 +52,8 @@ private String writeHelpInfoText() {
                     - To give an undefined parameter (when column's value is empty or null): 'parameterName=' (ex. 'user=')\s
                   Examples:\s
                     curl -i -X GET  basePath/report?startdate=yyyy-MM-dd\s
-                    curl -i -X GET  basePath/delete-deposit?user=XXX&state=REJECTED\s
+                    curl -i -X GET  basePath/report?user=XXX&state=REJECTED\s
+                    curl -i -X GET  basePath/report/{depositId}\s
                     curl -i -X POST basePath/delete-deposit?user=XXX\s
                     curl -i -X POST basePath/delete-deposit?user=XXX&state=REJECTED""";
     }

diff --git a/src/test/java/nl/knaw/dans/managedeposit/core/service/IngestPathMonitorTest.java b/src/test/java/nl/knaw/dans/managedeposit/core/service/IngestPathMonitorTest.java
@@ -15,18 +15,36 @@
  */
 package nl.knaw.dans.managedeposit.core.service;
 
+import ch.qos.logback.classic.Level;
+import ch.qos.logback.classic.Logger;
+import ch.qos.logback.classic.spi.ILoggingEvent;
+import ch.qos.logback.core.read.ListAppender;
 import nl.knaw.dans.managedeposit.AbstractTestWithTestDir;
 import org.apache.commons.io.FileUtils;
+import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 import org.mockito.Mockito;
+import org.slf4j.LoggerFactory;
 
 import java.nio.file.Files;
 
 import static java.nio.file.Files.createDirectories;
 import static java.util.Collections.singletonList;
 import static org.assertj.core.api.Assertions.assertThatThrownBy;
+import static org.assertj.core.api.AssertionsForClassTypes.assertThat;
 
 public class IngestPathMonitorTest extends AbstractTestWithTestDir {
+    private ListAppender<ILoggingEvent> listAppender;
+
+    @BeforeEach
+    public void setup() throws Exception {
+        super.setUp();
+        var logger = (Logger) LoggerFactory.getLogger(IngestPathMonitor.class);
+        listAppender = new ListAppender<>();
+        listAppender.start();
+        logger.setLevel(Level.ERROR);
+        logger.addAppender(listAppender);
+    }
 
     private IngestPathMonitor startMonitor(DepositStatusUpdater mockUpdater, int pollingInterval) throws Exception {
         var monitor = new IngestPathMonitor(singletonList(testDir), mockUpdater, pollingInterval);
@@ -44,9 +62,14 @@ public void should_ignore_properties_in_root() throws Exception {
         var propertiesFile = Files.createFile(testDir.resolve("deposit.properties"));
         Thread.sleep(70);// Wait for the monitor to pick up the new file
 
-        Mockito.verify(mockUpdater, Mockito.times(0))
-            .onDepositCreate(propertiesFile.toFile());
+        monitor.onFileCreate(propertiesFile.toFile());
         Mockito.verifyNoMoreInteractions(mockUpdater);
+        Thread.sleep(70);// Wait for the monitor to pick up the new file
+
+        // Check the logs
+        var formattedMessage = listAppender.list.get(0).getFormattedMessage();
+        assertThat(formattedMessage).startsWith("Error: file");
+        assertThat(formattedMessage).endsWith("is directly in a base-folder.");
 
         monitor.stop();
     }
@@ -83,7 +106,6 @@ public void should_ignore_properties_in_bag_content() throws Exception {
         monitor.stop();
     }
 
-
     @Test
     public void should_pick_up_deleted_bag() throws Exception {
         var mockUpdater = Mockito.mock(DepositStatusUpdater.class);