refactor fmd change check, use in permissions as well

qqmyers · qqmyers · commit 010322fa8e5e · 2026-01-16T11:04:13.000-05:00
diff --git a/src/main/java/edu/harvard/iq/dataverse/FileMetadata.java b/src/main/java/edu/harvard/iq/dataverse/FileMetadata.java
@@ -67,15 +67,15 @@
  */
 @Table(indexes = {@Index(columnList="datafile_id"), @Index(columnList="datasetversion_id")} )
 @NamedNativeQuery(
-        name = "FileMetadata.compareFileMetadata",
+        name = "FileMetadata.getDatafilesWithChangedMetadata",
         query = "WITH fm_categories AS (" +
                 "    SELECT fmd.filemetadatas_id, " +
                 "           STRING_AGG(dfc.name, ',' ORDER BY dfc.name) AS categories " +
                 "    FROM FileMetadata_DataFileCategory fmd " +
                 "    JOIN DataFileCategory dfc ON fmd.filecategories_id = dfc.id " +
                 "    GROUP BY fmd.filemetadatas_id " +
                 ") " +
-                "SELECT fm1.id " +
+                "SELECT fm1.datafile_id " +
                 "FROM FileMetadata fm1 " +
                 "LEFT JOIN FileMetadata fm2 ON fm1.datafile_id = fm2.datafile_id " +
                 "    AND fm2.datasetversion_id = ?1 " +
@@ -93,11 +93,11 @@
                 "                 ) " +
                 "            ) " +
                 "        )",
-                resultSetMapping = "IdToLongMapping"
+                resultSetMapping = "IdToIntegerMapping"
     )
 /* When this mapping was to Long.class, Postgres was still returning an Integer, causing indexing failures - see #11776 */ 
 @SqlResultSetMapping(
-        name = "IdToLongMapping",
+        name = "IdToIntegerMapping",
         columns = @ColumnResult(name = "id", type = Integer.class)
     )
 @Entity
diff --git a/src/main/java/edu/harvard/iq/dataverse/search/IndexServiceBean.java b/src/main/java/edu/harvard/iq/dataverse/search/IndexServiceBean.java
@@ -602,10 +602,22 @@ private void doIndexDataset(Dataset dataset, boolean doNormalSolrDocCleanUp) thr
                 writeDebugInfo(debug, dataset);
             }
             if (doNormalSolrDocCleanUp) {
+                List<String> solrIdsOfPermissionDocsToDelete = new ArrayList<>();
                 try {
                     solrIdsOfDocsToDelete = findFilesOfParentDataset(dataset.getId());
                     logger.fine("Existing file docs: " + String.join(", ", solrIdsOfDocsToDelete));
                     if (!solrIdsOfDocsToDelete.isEmpty()) {
+                        if (!latestVersion.isDraft()) {
+                            // For draft datasets after a published version, we're not reindexing the files unless their metadata changes
+                            // Therefore, to make sure their
+                            for (String fileDocId : solrIdsOfDocsToDelete) {
+                                if (!fileDocId.endsWith(draftSuffix)) {
+                                    solrIdsOfPermissionDocsToDelete.add(fileDocId + draftSuffix + discoverabilityPermissionSuffix);
+                                }
+                            }
+
+                            logger.fine("Existing permission docs: " + String.join(", ", solrIdsOfPermissionDocsToDelete));
+                        }
                         // We keep the latest version's docs unless it is deaccessioned and there is no
                         // published/released version
                         // So skip the loop removing those docs from the delete list except in that case
@@ -649,7 +661,7 @@ private void doIndexDataset(Dataset dataset, boolean doNormalSolrDocCleanUp) thr
                 logger.fine("Solr docs to delete: " + String.join(", ", solrIdsOfDocsToDelete));
 
                 if (!solrIdsOfDocsToDelete.isEmpty()) {
-                    List<String> solrIdsOfPermissionDocsToDelete = new ArrayList<>();
+
                     for (String file : solrIdsOfDocsToDelete) {
                         // Also remove associated permission docs
                         solrIdsOfPermissionDocsToDelete.add(file + discoverabilityPermissionSuffix);
@@ -1416,7 +1428,7 @@ public SolrInputDocuments toSolrDocs(IndexableDataset indexableDataset, Set<Long
         long maxSize = maxFTIndexingSize != null ? maxFTIndexingSize.longValue() : Long.MAX_VALUE;
 
         List<String> filesIndexed = new ArrayList<>();
-        final List<Long> changedFileMetadataIds = new ArrayList<>();
+        final List<Long> changedFileIds = new ArrayList<>();
         if (datasetVersion != null) {
             List<FileMetadata> fileMetadatas = datasetVersion.getFileMetadatas();
             List<FileMetadata> rfm = new ArrayList<>();
@@ -1427,42 +1439,17 @@ public SolrInputDocuments toSolrDocs(IndexableDataset indexableDataset, Set<Long
                     fileMap.put(released.getDataFile().getId(), released);
                 }
 
-                Query query = em.createNamedQuery("FileMetadata.compareFileMetadata", Long.class);
-                query.setParameter(1, dataset.getReleasedVersion().getId());
-                query.setParameter(2, datasetVersion.getId());
-
-                /*
-                 * When the query was configured to return Long, it was returning Integer. The query has been changed to return Integer now. The code here is robust if that changes in the future.
-                 */
-                List<Object> queryResults = query.getResultList();
-                for (Object result : queryResults) {
-                    if (result != null) {
-                        // Ensure we're adding Long objects to the list
-                        if (result instanceof Integer intResult) {
-                            logger.finest("Converted Integer result to Long: " + result);
-                            changedFileMetadataIds.add(Long.valueOf(intResult));
-                        } else if (result instanceof Long longResult) {
-                            // Already a Long, add directly
-                            logger.finest("Added existing Long to list: " + result);
-                            changedFileMetadataIds.add(longResult);
-                        } else {
-                            // If it's not a Long, convert it to one via String
-                            try {
-                                changedFileMetadataIds.add(Long.valueOf(result.toString()));
-                                logger.finest("Converted non-Long result to Long: " + result + " of type " + result.getClass().getName());
-                            } catch (NumberFormatException e) {
-                                logger.warning("Could not convert query result to Long: " + result);
-                            }
-                        }
-                    }
-                }
+                solrIndexService.populateChangedFileIds(
+                        dataset.getReleasedVersion().getId(),
+                        datasetVersion.getId(),
+                        changedFileIds);
                 logger.fine(
                         "We are indexing a draft version of a dataset that has a released version. We'll be checking file metadatas if they are exact clones of the released versions.");
             } else if (datasetVersion.isDraft()) {
                 // Add all file metadata ids to changedFileMetadataIds
-                changedFileMetadataIds.addAll(
+                changedFileIds.addAll(
                     fileMetadatas.stream()
-                        .map(FileMetadata::getId)
+                        .map(fm -> fm.getDataFile().getId())
                         .collect(Collectors.toList())
                 );
             }
@@ -1526,7 +1513,7 @@ public SolrInputDocuments toSolrDocs(IndexableDataset indexableDataset, Set<Long
                 }
                 boolean indexThisFile = false;
 
-                if (indexThisMetadata && (isReleasedVersion || changedFileMetadataIds.contains(fileMetadata.getId()))) {
+                if (indexThisMetadata && (isReleasedVersion || changedFileIds.contains(datafile.getId()))) {
                     indexThisFile = true;
                 } else if (indexThisMetadata) {
                     // Draft version, file is not new or all file metadata matches the released version
diff --git a/src/main/java/edu/harvard/iq/dataverse/search/SolrIndexServiceBean.java b/src/main/java/edu/harvard/iq/dataverse/search/SolrIndexServiceBean.java
@@ -36,6 +36,7 @@
 import jakarta.json.JsonObjectBuilder;
 import jakarta.persistence.EntityManager;
 import jakarta.persistence.PersistenceContext;
+import jakarta.persistence.Query;
 
 import org.apache.solr.client.solrj.SolrServerException;
 import org.apache.solr.common.SolrInputDocument;
@@ -410,41 +411,100 @@ public void indexDatasetBatchInNewTransaction(List<Long> datasetIds, final int[]
                 indexPermissionsForOneDvObject(dataset);
 
                 // Process files for this dataset
-                for (DatasetVersion version : datasetVersionsToBuildCardsFor(dataset)) {
-                        processDatasetVersionFiles(version, fileCounter, fileQueryMin, versions.size()>1);
+                List<DatasetVersion> versions = datasetVersionsToBuildCardsFor(dataset);
+                final List<Long> changedFileIds = new ArrayList<>();
+                if(versions.size()>1) {
+                    Long releasedVersionId = versions.get(versions.get(0).isReleased() ? 0 : 1).getId();
+                    Long draftVersionId = versions.get(versions.get(0).isReleased() ? 1 : 0).getId();
+                    
+                    populateChangedFileIds(
+                            releasedVersionId, 
+                            draftVersionId, 
+                            changedFileIds
+                        );
+                }
+                for (DatasetVersion version : versions) {
+                    processDatasetVersionFiles(version, fileCounter, fileQueryMin, (versions.size()>1 && version.isDraft()) ? changedFileIds : null);
                     }
                 }
             }
         }
 
     @TransactionAttribute(TransactionAttributeType.REQUIRES_NEW)
     public void indexDatasetFilesInNewTransaction(List<DatasetVersion> versions, final int[] fileCounter, int fileQueryMin) {
+        final List<Long> changedFileIds = new ArrayList<>();
+        if(versions.size()>1) {
+            Long releasedVersionId = versions.get(versions.get(0).isReleased() ? 0 : 1).getId();
+            Long draftVersionId = versions.get(versions.get(0).isReleased() ? 1 : 0).getId();
+            
+            populateChangedFileIds(
+                    releasedVersionId, 
+                    draftVersionId, 
+                    changedFileIds
+                );
+        }
         for (DatasetVersion version : versions) {
             // The version object is detached, but its fileMetadatas collection is already loaded.
             // We only need its ID and state, which are available.
-            processDatasetVersionFiles(version, fileCounter, fileQueryMin, versions.size()>1);
+            processDatasetVersionFiles(version, fileCounter, fileQueryMin, (versions.size()>1 && version.isDraft()) ? changedFileIds : null);
         }
     }
 
+    /**
+     * Retrieves the IDs of file metadatas that have changed between the released version
+     * and the draft version of a dataset.
+     * 
+     * @param releasedVersionId the ID of the released dataset version
+     * @param draftVersionId the ID of the draft dataset version
+     * @param changedFileMetadataIds the list to populate with changed file metadata IDs
+     */
+    protected void populateChangedFileIds(Long releasedVersionId, Long draftVersionId, List<Long> changedFileIds) {
+        Query query = em.createNamedQuery("FileMetadata.getDatafilesWithChangedMetadata", Long.class);
+        query.setParameter(1, releasedVersionId);
+        query.setParameter(2, draftVersionId);
+
+        /*
+         * When the query was configured to return Long, it was returning Integer. 
+         * The query has been changed to return Integer now. The code here is robust 
+         * if that changes in the future.
+         */
+        List<Object> queryResults = query.getResultList();
+        for (Object result : queryResults) {
+            if (result != null) {
+                // Ensure we're adding Long objects to the list
+                if (result instanceof Integer intResult) {
+                    logger.finest("Converted Integer result to Long: " + result);
+                    changedFileIds.add(Long.valueOf(intResult));
+                } else if (result instanceof Long longResult) {
+                    // Already a Long, add directly
+                    logger.finest("Added existing Long to list: " + result);
+                    changedFileIds.add(longResult);
+                } else {
+                    // If it's not a Long, convert it to one via String
+                    try {
+                        changedFileIds.add(Long.valueOf(result.toString()));
+                        logger.finest("Converted non-Long result to Long: " + result + " of type " + result.getClass().getName());
+                    } catch (NumberFormatException e) {
+                        logger.warning("Could not convert query result to Long: " + result);
+                    }
+                }
+            }
+        }
+    }
+    
     private void processDatasetVersionFiles(DatasetVersion version,
-            final int[] fileCounter, int fileQueryMin, boolean isReleased) {
+            final int[] fileCounter, int fileQueryMin, List<Long> changedFileIds) {
         List<String> cachedPerms = searchPermissionsService.findDatasetVersionPerms(version);
         String solrIdEnd = getDatasetOrDataFileSolrEnding(version.getVersionState());
         Long versionId = version.getId();
         List<DataFileProxy> filesToReindexAsBatch = new ArrayList<>();
 
         // If the version is draft and there is a released version, 
-        // we only need perm docs for the files with filemetadata changes == those with _draft solr docs already
-        Set<Long> fileIdsToReindex = null;
-        if (version.getVersionState().equals(DatasetVersion.VersionState.DRAFT) && isReleased) {
-            fileIdsToReindex = getFileIdsWithSolrDocs(versionId);
-            logger.fine("Found " + fileIdsToReindex.size() + " files with draft Solr docs for version " + versionId);
-        }
+        // we only need perm docs for the files with filemetadata changes == those in changedFileMetadataIds
         
         // Process files in batches of 100
         int batchSize = 100;
 
-        final Set<Long> finalFileIdsToReindex = fileIdsToReindex;
         if (dataFileService.findCountByDatasetVersionId(version.getId()).intValue() > fileQueryMin) {
             // For large datasets, use a more efficient SQL query
             // ToDo - only get the ones in finalFileIdsToReindex
@@ -453,7 +513,7 @@ private void processDatasetVersionFiles(DatasetVersion version,
                 // Process files in batches to avoid memory issues
                 fileStream.forEach(fileInfo -> {
                     // Only add files that need reindexing
-                    if (finalFileIdsToReindex == null || finalFileIdsToReindex.contains(fileInfo.getFileId())) {
+                if (changedFileIds == null || changedFileIds.contains(fileInfo.getFileId())) {
                         filesToReindexAsBatch.add(fileInfo);
                         fileCounter[0]++;
 
@@ -470,7 +530,7 @@ private void processDatasetVersionFiles(DatasetVersion version,
             for (FileMetadata fmd : version.getFileMetadatas()) {
                 // Only add files that need reindexing
                 DataFileProxy fileProxy = new DataFileProxy(fmd);
-                if (finalFileIdsToReindex == null || finalFileIdsToReindex.contains(fileProxy.getFileId())) {
+                if (changedFileIds == null || changedFileIds.contains(fileProxy.getFileId())) {
                     filesToReindexAsBatch.add(fileProxy);
                     fileCounter[0]++;
 
@@ -510,55 +570,6 @@ private void reindexFilesInBatches(List<DataFileProxy> filesToReindexAsBatch, Li
         }
     }
 
-    /**
-     * Queries Solr to find file IDs that have draft documents for the given dataset version.
-     * This is used to optimize permission reindexing by only processing files that have
-     * metadata changes in the draft version.
-     * 
-     * @param datasetVersionId The ID of the dataset version
-     * @return A set of file IDs that have Solr documents associated with this version
-     */
-    private Set<Long> getFileIdsWithSolrDocs(Long datasetVersionId) {
-        Set<Long> fileIds = new HashSet<>();
-        
-        try {
-            SolrQuery solrQuery = new SolrQuery();
-            
-            // Query for files in this specific version with draft suffix
-            solrQuery.setQuery("*:*");
-            solrQuery.addFilterQuery(SearchFields.TYPE + ":" + SearchConstants.FILES);
-            solrQuery.addFilterQuery(SearchFields.DATASET_VERSION_ID + ":" + datasetVersionId);
-            
-            // Only return the entity ID field
-            solrQuery.setFields(SearchFields.ENTITY_ID);
-            
-            // We want all matching documents
-            solrQuery.setRows(Integer.MAX_VALUE);
-            
-            logger.fine("Solr query to find draft files: " + solrQuery);
-            
-            QueryResponse queryResponse = solrClientService.getSolrClient().query(solrQuery);
-            SolrDocumentList docs = queryResponse.getResults();
-            
-            for (SolrDocument doc : docs) {
-                Long entityId = (Long) doc.getFieldValue(SearchFields.ENTITY_ID);
-                if (entityId != null) {
-                    fileIds.add(entityId);
-                }
-            }
-            
-            logger.fine("Found " + fileIds.size() + " files with draft Solr docs for version " + datasetVersionId);
-            
-        } catch (SolrServerException | IOException ex) {
-            logger.log(Level.WARNING, "Error querying Solr for draft file IDs for version " + datasetVersionId + 
-                    ". Will reindex all files as fallback.", ex);
-            // Return null to indicate we should process all files
-            return null;
-        }
-        
-        return fileIds;
-    }
-    
     public IndexResponse deleteMultipleSolrIds(List<String> solrIdsToDelete) {
         if (solrIdsToDelete.isEmpty()) {
             return new IndexResponse("nothing to delete");
