Write batches of GuestBookResponses for performance

doc/release-notes/12479-write-guestbook-improvements.md

@@ -0,0 +1 @@
+The performance when saving counts/guestbook responses has been improved (particularly for datasets with large numbers of files).

src/main/java/edu/harvard/iq/dataverse/DataFileServiceBean.java

@@ -283,14 +283,20 @@ public List<FileMetadata> findFileMetadataByDatasetVersionId(Long datasetVersion
                     .getResultList();
     }
 
+    public List<Long> findDataFileIdsByDatasetVersionId(Long datasetVersionId) {
+        return findDataFileIdsByDatasetVersionIdLabelSearchTerm(datasetVersionId, null, null, null);
+    }
+
     public List<Long> findDataFileIdsByDatasetVersionIdLabelSearchTerm(Long datasetVersionId, String userSuppliedSearchTerm, String userSuppliedSortField, String userSuppliedSortOrder) {
-        FileSortFieldAndOrder sortFieldAndOrder = new FileSortFieldAndOrder(userSuppliedSortField, userSuppliedSortOrder);
         String searchTerm = !StringUtils.isBlank(userSuppliedSearchTerm) ? "%"+userSuppliedSearchTerm.trim().toLowerCase()+"%" : null;
 
         String selectClause = "select o.datafile_id from FileMetadata o where o.datasetversion_id = " + datasetVersionId;
         String searchClause = searchTerm != null ? " and (lower(o.label) like ? or lower(o.description) like ?)" : "";
-        String orderByClause = " order by o." + sortFieldAndOrder.getSortField() + " " + sortFieldAndOrder.getSortOrder();
-
+        String orderByClause = "";
+        if(StringUtils.isNotBlank(userSuppliedSortField) || StringUtils.isNotBlank(userSuppliedSortOrder)) {
+            FileSortFieldAndOrder sortFieldAndOrder = new FileSortFieldAndOrder(userSuppliedSortField, userSuppliedSortOrder);
+            orderByClause = " order by o." + sortFieldAndOrder.getSortField() + " " + sortFieldAndOrder.getSortOrder();
+        }
         Query query = em.createNativeQuery(selectClause + searchClause + orderByClause);
         if (searchTerm != null) {
             query.setParameter(1, searchTerm);

src/main/java/edu/harvard/iq/dataverse/DatasetVersionServiceBean.java

@@ -1279,8 +1279,7 @@ public DatasetVersion merge( DatasetVersion ver ) {
     /**
      * Execute a query to return DatasetVersion
      * 
-     * @param queryString
-     * @return 
+     * @return
      */
     public List<DatasetVersion> getUnarchivedDatasetVersions(){
 
@@ -1359,4 +1358,5 @@ public void persistArchivalCopyLocation(DatasetVersion dv) {
             logger.log(Level.SEVERE, "Could not find DatasetVersion with id={0} to retry persisting archival copy location after OptimisticLockException.", dv.getId());
         }
     }
+
 }

src/main/java/edu/harvard/iq/dataverse/FileDownloadServiceBean.java

@@ -7,6 +7,7 @@
 import edu.harvard.iq.dataverse.authorization.users.User;
 import edu.harvard.iq.dataverse.dataaccess.DataAccess;
 import edu.harvard.iq.dataverse.dataaccess.StorageIO;
+import edu.harvard.iq.dataverse.engine.command.DataverseRequest;
 import edu.harvard.iq.dataverse.engine.command.exception.CommandException;
 import edu.harvard.iq.dataverse.engine.command.impl.CreateGuestbookResponseCommand;
 import edu.harvard.iq.dataverse.engine.command.impl.RequestAccessCommand;
@@ -18,20 +19,24 @@
 import edu.harvard.iq.dataverse.util.*;
 import jakarta.ejb.EJB;
 import jakarta.ejb.Stateless;
+import jakarta.faces.application.FacesMessage;
 import jakarta.faces.context.FacesContext;
 import jakarta.inject.Inject;
 import jakarta.inject.Named;
+import jakarta.ejb.TransactionAttribute;
+import jakarta.ejb.TransactionAttributeType;
 import jakarta.persistence.EntityManager;
 import jakarta.persistence.PersistenceContext;
 import jakarta.servlet.ServletOutputStream;
 import jakarta.servlet.http.HttpServletResponse;
+
 import org.primefaces.PrimeFaces;
 
 import java.io.IOException;
 import java.sql.Timestamp;
 import java.util.*;
 import java.util.logging.Logger;
-//import org.primefaces.context.RequestContext;
+import java.io.FileNotFoundException;
 
 /**
  *
@@ -43,6 +48,7 @@
 @Named
 public class FileDownloadServiceBean implements java.io.Serializable {
 
+    public static final double GUESTBOOK_RESPONSE_BATCH_SIZE = 250;
     @PersistenceContext(unitName = "VDCNet-ejbPU")
     private EntityManager em;
 
@@ -67,6 +73,9 @@ public class FileDownloadServiceBean implements java.io.Serializable {
     @EJB
     MailServiceBean mailService;
 
+    @EJB
+    FileDownloadServiceBean self;
+
     @Inject
     DataverseSession session;
 
@@ -123,23 +132,40 @@ public void writeGuestbookAndStartBatchDownload(GuestbookResponse guestbookRespo
 
         String customZipDownloadUrl = settingsService.getValueForKey(SettingsServiceBean.Key.CustomZipDownloadServiceUrl);
         boolean useCustomZipService = customZipDownloadUrl != null; 
-        String zipServiceKey = null; 
+        String zipServiceKey = null;
+        List<String> fileIdsList = new ArrayList<>(Arrays.asList(fileIds));
 
-        // Do we need to write GuestbookRecord entries for the files? 
-        if (!doNotSaveGuestbookRecord || useCustomZipService) {
+        List<DataFile> selectedDataFiles = new ArrayList<>();
+        //Should not be getting exceptions with Dataverse generating the fileIds
+        try {
+            selectedDataFiles = resolveSelectedDataFilesInDataset(fileIdsList, dvRequestService.getDataverseRequest());
+        } catch (FileNotFoundException e) {
+            PrimeFaces.current().dialog().showMessageDynamic(new FacesMessage(FacesMessage.SEVERITY_ERROR, "Error", e.getMessage()));
+            return;
+        } catch (MultipleDatasetsException e) {
+            PrimeFaces.current().dialog().showMessageDynamic(new FacesMessage(FacesMessage.SEVERITY_ERROR, "Error", e.getMessage()));
+            return;
+        }
 
-            List<String> list = new ArrayList<>(Arrays.asList(guestbookResponse.getSelectedFileIds().split(",")));
-            Timestamp timestamp = null; 
-
-            for (String idAsString : list) {
+        // Do we need to write GuestbookRecord entries for the files?
+        if (!doNotSaveGuestbookRecord) {
+            // Code here assumes user can download all files (which should be true when called from DatasetPage)
+            // Authorization will be checked in the custom zipper or redirect URL, so the only impact would be extra guestbookresponses
+            List<String> gbrids = writeGuestbookResponseRecords(guestbookResponse, selectedDataFiles);
+            if(!gbrids.isEmpty()){
+                gbrid = gbrids.getFirst();
+            } else {
+                logger.warning("No GuestbookResponse records were created for the files that were selected for download.");
+            }
+        }
+        if(useCustomZipService) {
+
+            Timestamp timestamp = null;
+            //Reset values
+
+            for (DataFile df : selectedDataFiles) {
                 //DataFile df = datafileService.findCheapAndEasy(new Long(idAsString));
-                DataFile df = datafileService.find(new Long(idAsString));
                 if (df != null) {
-                    if (!doNotSaveGuestbookRecord) {
-                        guestbookResponse.setDataFile(df);
-                        gbrid = writeGuestbookResponseRecord(guestbookResponse);
-                    }
-
                     if (useCustomZipService) {
                         if (zipServiceKey == null) {
                             zipServiceKey = generateServiceKey();
@@ -152,8 +178,8 @@ public void writeGuestbookAndStartBatchDownload(GuestbookResponse guestbookRespo
                     }
                 }
             }
+
         }
-
         if (useCustomZipService) {
             redirectToCustomZipDownloadService(customZipDownloadUrl, zipServiceKey);
         } else {
@@ -203,7 +229,10 @@ public void writeGuestbookResponseAndRequestAccess(GuestbookResponse guestbookRe
 
         int countRequestAccessSuccess = 0;
 
+        // ToDo - rewrite to avoid 1 transaction per guestbookresponse/file
         for(DataFile dataFile : selectedDataFiles){
+            // The guestbookResponse can be reused because the final save() of
+            // the guestbookResponse is in a new transaction. (Otherwise we'd need to make a copy)
             guestbookResponse.setDataFile(dataFile);
             writeGuestbookResponseRecordForRequestAccess(guestbookResponse);
             if(requestAccess(dataFile,guestbookResponse)){
@@ -226,27 +255,158 @@ public void writeGuestbookResponseRecord(GuestbookResponse guestbookResponse, Fi
             writeGuestbookResponseRecord(guestbookResponse);
         }
     }
-
+
+    public List<String> writeGuestbookResponseRecords(GuestbookResponse guestbookResponse, List<DataFile> selectedDataFiles) {
+        if (guestbookResponse == null || selectedDataFiles == null) {
+            return Collections.emptyList();
+        }
+
+        if (selectedDataFiles.isEmpty()) {
+            return Collections.emptyList();
+        }
+
+        List<GuestbookResponse> responsesToPersist = new ArrayList<>(selectedDataFiles.size());
+        for (DataFile dataFile : selectedDataFiles) {
+            GuestbookResponse perFileResponse = new GuestbookResponse(guestbookResponse);
+            perFileResponse.setDataFile(dataFile);
+            responsesToPersist.add(perFileResponse);
+        }
+        List<String> savedIds = self.saveGuestbookResponseRecordsAndMDCLogEntries(responsesToPersist);
+        return savedIds;
+    }
+
+    public List<DataFile> resolveSelectedDataFilesInDataset(List<String> rawFileIds, DataverseRequest req)
+            throws FileNotFoundException, MultipleDatasetsException {
+
+        List<DataFile> selectedDataFiles = new ArrayList<>(rawFileIds.size());
+        Long datasetId = null;
+
+        for (String rawFileId : rawFileIds) {
+            if (rawFileId == null || rawFileId.isBlank()) {
+                continue;
+            }
+
+            Long fileId;
+            try {
+                fileId = Long.valueOf(rawFileId.trim());
+            } catch (NumberFormatException nfe) {
+                throw new FileNotFoundException("Invalid file id: " + rawFileId);
+            }
+
+            DataFile dataFile = datafileService.find(fileId);
+            if (dataFile == null) {
+                throw new FileNotFoundException("No file found for id: " + fileId);
+            }
+
+            if (selectedDataFiles.isEmpty()) {
+                if (dataFile.isLocallyFAIR() && !permissionService.hasLocallyFAIRAccess(req, dataFile)) {
+                    throw new FileNotFoundException("No file found for id: " + fileId);
+                }
+            }
+
+            Long currentDatasetId = dataFile.getOwner().getId();
+            if (datasetId == null) {
+                datasetId = currentDatasetId;
+            } else if (!datasetId.equals(currentDatasetId)) {
+                throw new MultipleDatasetsException(
+                        "Selected files do not belong to the same dataset."
+                );
+            }
+
+            selectedDataFiles.add(dataFile);
+        }
+
+        return selectedDataFiles;
+    }
+
+    @TransactionAttribute(TransactionAttributeType.REQUIRES_NEW)
+    public List<String> saveGuestbookResponseRecordsAndMDCLogEntries(List<GuestbookResponse> guestbookResponses) {
+        if (guestbookResponses == null || guestbookResponses.isEmpty()) {
+            return Collections.emptyList();
+        }
+
+        List<String> savedIds = new ArrayList<>(guestbookResponses.size());
+
+        Timestamp now = new Timestamp(System.currentTimeMillis());
+
+        for (int i = 0; i < guestbookResponses.size(); i++) {
+            GuestbookResponse response = guestbookResponses.get(i);
+
+            try {
+                response.setResponseTime(now);
+                em.persist(response);
+
+                DatasetVersion version = response.getDatasetVersion();
+                if (version == null) {
+                    version = response.getDataset().getReleasedVersion();
+                }
+
+                DataFile dataFile = response.getDataFile();
+                MakeDataCountEntry entry = new MakeDataCountEntry(
+                        FacesContext.getCurrentInstance(),
+                        dvRequestService,
+                        version,
+                        dataFile
+                );
+                entry.setTargetUrl("/api/access/datafile/" + dataFile.getId());
+                entry.setRequestUrl("/api/access/datafile/" + dataFile.getId());
+                mdcLogService.logEntry(entry);
+
+                if ((i + 1) % GUESTBOOK_RESPONSE_BATCH_SIZE == 0) {
+                    em.flush();
+                    em.clear();
+                }
+            } catch (RuntimeException ex) {
+                DataFile dataFile = response.getDataFile();
+                logger.warning("Exception writing GuestbookResponse"
+                        + (dataFile != null ? " for file: " + dataFile.getId() : "")
+                        + " : " + ex.getLocalizedMessage());
+            }
+        }
+
+        em.flush();
+        em.clear();
+        for (GuestbookResponse response : guestbookResponses) {
+            savedIds.add(response.getId().toString());
+        }
+        return savedIds;
+    }
+
     public String writeGuestbookResponseRecord(GuestbookResponse guestbookResponse) {
         String guestbookResponseIds = "";
+
         try {
-            CreateGuestbookResponseCommand cmd = new CreateGuestbookResponseCommand(dvRequestService.getDataverseRequest(), guestbookResponse, guestbookResponse.getDataset());
+            CreateGuestbookResponseCommand cmd = new CreateGuestbookResponseCommand(
+                    dvRequestService.getDataverseRequest(),
+                    guestbookResponse,
+                    guestbookResponse.getDataset()
+            );
             commandEngine.submit(cmd);
             guestbookResponseIds = guestbookResponse.getId().toString();
+
             DatasetVersion version = guestbookResponse.getDatasetVersion();
-            
+
             //Sometimes guestbookResponse doesn't have a version, so we grab the released version
             if (null == version) {
                 version = guestbookResponse.getDataset().getReleasedVersion();
             }
-            MakeDataCountEntry entry = new MakeDataCountEntry(FacesContext.getCurrentInstance(), dvRequestService, version, guestbookResponse.getDataFile());
+
+            MakeDataCountEntry entry = new MakeDataCountEntry(
+                    FacesContext.getCurrentInstance(),
+                    dvRequestService,
+                    version,
+                    guestbookResponse.getDataFile()
+            );
             //As the api download url is not available at this point we construct it manually
             entry.setTargetUrl("/api/access/datafile/" + guestbookResponse.getDataFile().getId());
             entry.setRequestUrl("/api/access/datafile/" + guestbookResponse.getDataFile().getId());
             mdcLogService.logEntry(entry);
         } catch (CommandException e) {
             //if an error occurs here then download won't happen no need for response recs...
-            logger.warning("Exception writing GuestbookResponse for file: " + guestbookResponse.getDataFile().getId() + " : " + e.getLocalizedMessage());
+            logger.warning("Exception writing GuestbookResponse for file: "
+                    + guestbookResponse.getDataFile().getId()
+                    + " : "
+                    + e.getLocalizedMessage());
         }
         return guestbookResponseIds;
     }
@@ -638,4 +798,9 @@ public String getDirectStorageLocatrion(String storageLocation) {
 
         return null; 
     }
+
+    public class MultipleDatasetsException extends Throwable {
+        public MultipleDatasetsException(String s) {
+        }
+    }
 }

src/main/java/edu/harvard/iq/dataverse/GuestbookResponse.java

@@ -68,13 +68,12 @@ public class GuestbookResponse implements Serializable {
     @JoinColumn(nullable=true)
     private AuthenticatedUser authenticatedUser;
 
-    @OneToMany(mappedBy="guestbookResponse",cascade={CascadeType.REMOVE, CascadeType.MERGE, CascadeType.PERSIST},fetch = FetchType.LAZY)
-    //private FileAccessRequest fileAccessRequest;
-    private List<FileAccessRequest> fileAccessRequests;
+    @OneToOne(mappedBy="guestbookResponse",cascade={CascadeType.REMOVE, CascadeType.MERGE, CascadeType.PERSIST},fetch = FetchType.LAZY)
+    private FileAccessRequest fileAccessRequest;
 
     @OneToMany(mappedBy="guestbookResponse",cascade={CascadeType.REMOVE, CascadeType.MERGE, CascadeType.PERSIST},orphanRemoval=true)
     @OrderBy ("id")
-    private List<CustomQuestionResponse> customQuestionResponses;
+    private List<CustomQuestionResponse> customQuestionResponses = new ArrayList<>();
 
     @Size(max = 255, message = "{guestbook.response.nameLength}")
     private String name;
@@ -258,7 +257,9 @@ public List<CustomQuestionResponse> getCustomQuestionResponses() {
     }
 
     public List<CustomQuestionResponse> getCustomQuestionResponsesSorted(){
-
+        if (customQuestionResponses == null) {
+            customQuestionResponses = new ArrayList<>();
+        }
         Collections.sort(customQuestionResponses, (CustomQuestionResponse cqr1, CustomQuestionResponse cqr2) -> {
             int a = cqr1.getCustomQuestion().getDisplayOrder();
             int b = cqr2.getCustomQuestion().getDisplayOrder();
@@ -270,15 +271,18 @@ public List<CustomQuestionResponse> getCustomQuestionResponsesSorted(){
     }
 
     public void setCustomQuestionResponses(List<CustomQuestionResponse> customQuestionResponses) {
+        if(customQuestionResponses == null) {
+            customQuestionResponses = new ArrayList<>();
+        }
         this.customQuestionResponses = customQuestionResponses;
     }
 
-    public List<FileAccessRequest> getFileAccessRequests(){
-        return fileAccessRequests;
+    public FileAccessRequest getFileAccessRequest(){
+        return fileAccessRequest;
     }
 
-    public void setFileAccessRequest(List<FileAccessRequest> fARs){
-        this.fileAccessRequests = fARs;
+    public void setFileAccessRequest(FileAccessRequest fAR){
+        this.fileAccessRequest = fAR;
     }
 
     public Dataset getDataset() {