Issue 52900: LKSM: Moving a sample to another folder breaks other references to shared files

api/src/org/labkey/api/assay/AssayProvider.java

@@ -389,7 +389,7 @@ default Long getResultRowCount(List<? extends ExpProtocol> protocols)
         return null;
     }
 
-    void moveRuns(List<ExpRun> runs, Container targetContainer, User user, AbstractAssayProvider.AssayMoveData assayMoveData);
+    void moveRuns(List<ExpRun> runs, Container targetContainer, User user, AbstractAssayProvider.AssayMoveData assayMoveData) throws ExperimentException;
 
     default void ensurePropertyDomainName(ExpProtocol protocol, ObjectProperty prop)
     {

api/src/org/labkey/api/data/NameGeneratorState.java

@@ -89,7 +89,7 @@ public NameGeneratorState(@NotNull NameGenerator nameGenerator, boolean incremen
         if (incrementSampleCounts) // determine if need to incrementRootSampleCount
         {
             DbSequence sampleCountSeq = SampleTypeService.get().getSampleCountSequence(_container, false);
-            if (expressionSummary.hasProjectSampleCounter() || sampleCountSeq.current() > 0) // if ${sampleCount} is present, or if ${sampleCount} was previously evaluated
+            if ((expressionSummary != null && expressionSummary.hasProjectSampleCounter()) || sampleCountSeq.current() > 0) // if ${sampleCount} is present, or if ${sampleCount} was previously evaluated
             {
                 sampleCounterSequence = sampleCountSeq;
                 if (sampleCounterSequence != null)
@@ -105,7 +105,7 @@ else if (sampleCountSeq.current() < expressionMin)
                 sampleCounterSequence = null;
 
             DbSequence rootCountSeq = SampleTypeService.get().getSampleCountSequence(_container, true);
-            if (expressionSummary.hasProjectSampleRootCounter() || rootCountSeq.current() > 0) // if ${rootSampleCount} is present, or if ${rootSampleCount} was previously evaluated
+            if ((expressionSummary != null && expressionSummary.hasProjectSampleRootCounter()) || rootCountSeq.current() > 0) // if ${rootSampleCount} is present, or if ${rootSampleCount} was previously evaluated
             {
                 rootCounterSequence = rootCountSeq;
                 if (rootCounterSequence != null)
@@ -228,7 +228,7 @@ private String genName(@NotNull Map<String, Object> rowMap,
         Map<String, Long> sampleCounts = null;
         if (_incrementSampleCounts)
         {
-            if (!_nameGenerator.getExpressionSummary().hasDateBasedSampleCounter())
+            if (!(_nameGenerator.getExpressionSummary() != null && _nameGenerator.getExpressionSummary().hasDateBasedSampleCounter()))
             {
                 if (null == getSampleCountsFunction)
                 {
@@ -281,12 +281,16 @@ private String genName(@NotNull Map<String, Object> rowMap,
                 return currName.trim();
         }
 
+        // allow using alternative expression for evaluation.
+        // for example, use AliquotNameExpression instead of NameExpression if sample is aliquot
+        NameGenerator activeNameGenerator = getActiveNameGenerator(altNameGenerator);
+        if (!activeNameGenerator.getSyntaxErrors().isEmpty())
+            throw new IllegalArgumentException("Invalid naming expression. " + StringUtils.join(activeNameGenerator.getSyntaxErrors(), "\n"));
+
         // Add extra context variables
         Map<FieldKey, Object> ctx = additionalContext(rowMap, parentDatas, parentSamples, sampleCounts, extraProps, altNameGenerator);
 
-        // allow using alternative expression for evaluation.
-        // for example, use AliquotNameExpression instead of NameExpression if sample is aliquot
-        StringExpressionFactory.FieldKeyStringExpression expression = getActiveNameGenerator(altNameGenerator).getParsedNameExpression();
+        StringExpressionFactory.FieldKeyStringExpression expression = activeNameGenerator.getParsedNameExpression();
         String name = null;
         if (expression instanceof NameGenerator.NameGenerationExpression)
             name = ((NameGenerator.NameGenerationExpression) expression).eval(ctx, _prefixCounterSequences);

api/src/org/labkey/api/exp/api/ExperimentService.java

@@ -138,6 +138,8 @@ static void setInstance(ExperimentService impl)
         ServiceRegistry.get().registerService(ExperimentService.class, impl);
     }
 
+    boolean canMoveFileReference(User user, Container sourceContainer, File sourceFile, int moveCount);
+
     enum QueryOptions
     {
         UseLsidForUpdate,
@@ -1124,7 +1126,7 @@ List<? extends ExpProtocol> getExpProtocolsWithParameterValue(
 
     int moveExperimentRuns(List<ExpRun> runs, Container targetContainer, User user);
 
-    Map<String, Integer> moveAssayRuns(List<? extends ExpRun> assayRuns, Container container, Container targetContainer, User user, String userComment, AuditBehaviorType auditBehavior);
+    Map<String, Integer> moveAssayRuns(List<? extends ExpRun> assayRuns, Container container, Container targetContainer, User user, String userComment, AuditBehaviorType auditBehavior) throws ExperimentException;
 
     int aliasMapRowContainerUpdate(TableInfo aliasMapTable, List<Integer> dataIds, Container targetContainer);
 

api/src/org/labkey/api/files/TableUpdaterFileListener.java

@@ -15,6 +15,7 @@
  */
 package org.labkey.api.files;
 
+import org.apache.commons.lang3.StringUtils;
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
 import org.jetbrains.annotations.NotNull;
@@ -195,6 +196,14 @@ public String getSourceName()
         return _table.getSchema().getName() + "." + _table.getName() + "." + _pathColumn.getName();
     }
 
+    public String getSourceSelect(SqlDialect sqlDialect)
+    {
+        String schema  = sqlDialect.getStringHandler().quoteStringLiteral(_table.getSchema().getName());
+        String table = sqlDialect.getStringHandler().quoteStringLiteral(_table.getName());
+        String column = sqlDialect.getStringHandler().quoteStringLiteral(_pathColumn.getName());
+        return schema + "." + table + "." + column;
+    }
+
     @Override
     public void fileCreated(@NotNull File created, @Nullable User user, @Nullable Container container)
     {
@@ -344,12 +353,11 @@ public Collection<File> listFiles(@Nullable Container container)
     @Override
     public SQLFragment listFilesQuery()
     {
-        return listFilesQuery(false);
+        return listFilesQuery(false, null);
     }
 
-    public SQLFragment listFilesQuery(boolean skipCreatedModified)
+    public SQLFragment listFilesQuery(boolean skipCreatedModified, String filePath)
     {
-        SqlDialect dialect = _table.getSqlDialect();
         SQLFragment selectFrag = new SQLFragment();
         selectFrag.append("SELECT\n");
 
@@ -393,10 +401,15 @@ else if (_table.getColumn("Folder") != null)
             selectFrag.append("  NULL AS SourceKey,\n");
 
         //selectFrag.append("  ? AS SourceName\n").add(getName());
-        selectFrag.append("  ").append(_table.getSchema().getSqlDialect().getStringHandler().quoteStringLiteral(getSourceName())).append(" AS SourceName\n");
+        selectFrag.append("  ").appendValue(getSourceSelect(_table.getSchema().getSqlDialect())).append(" AS SourceName\n");
 
         selectFrag.append("FROM ").append(_table, TABLE_ALIAS).append("\n");
-        selectFrag.append("WHERE ").appendIdentifier(_pathColumn.getSelectIdentifier()).append(" IS NOT NULL\n");
+        selectFrag.append("WHERE ").appendIdentifier(_pathColumn.getSelectIdentifier());
+
+        if (StringUtils.isEmpty(filePath))
+            selectFrag.append(" IS NOT NULL\n");
+        else
+            selectFrag.append(" = ").appendStringLiteral(filePath, _table.getSchema().getSqlDialect()).append("\n");
 
         return selectFrag;
     }

experiment/src/org/labkey/experiment/ExpDataFileListener.java

@@ -56,6 +56,12 @@ public int fileMoved(@NotNull Path src, @NotNull Path dest, @Nullable User user,
     @Override
     public int fileMoved(@NotNull Path src, @NotNull Path dest, @Nullable User user, @Nullable Container sourceContainer, @Nullable Container targetContainer)
     {
+        // Issue 53070: Moving files could result in duplicate exp.data records
+        boolean deleteSrc = false;
+        ExpData existingConflict = ExperimentService.get().getExpDataByURL(dest, targetContainer);
+        if (existingConflict != null)
+            deleteSrc = true;
+
         ExpData data = ExperimentService.get().getExpDataByURL(src, sourceContainer);
 
         if (data == null)
@@ -71,13 +77,21 @@ public int fileMoved(@NotNull Path src, @NotNull Path dest, @Nullable User user,
         int extra = 0;
         if (data != null && src.equals(data.getFilePath()) && !src.equals(dest))
         {
-            // The file has been renamed, so rename the exp.data row if its name matches
-            data.setName(FileUtil.getFileName(dest));
-            // if the data object moved containers, set that as well
-            if (targetContainer != null && !targetContainer.equals(sourceContainer))
-                data.setContainer(targetContainer);
-            data.save(user); // FIXME: Issue 53070: Moving files could result in duplicate exp.data records
-            extra = 1;
+            if (deleteSrc)
+            {
+                data.delete(user);
+                return 0;
+            }
+            else
+            {
+                // The file has been renamed, so rename the exp.data row if its name matches
+                data.setName(FileUtil.getFileName(dest));
+                // if the data object moved containers, set that as well
+                if (targetContainer != null && !targetContainer.equals(sourceContainer))
+                    data.setContainer(targetContainer);
+                data.save(user);
+                extra = 1;
+            }
         }
 
         return super.fileMoved(src, dest, user, sourceContainer) + extra;

experiment/src/org/labkey/experiment/FileLinkFileListener.java

@@ -15,6 +15,7 @@
  */
 package org.labkey.experiment;
 
+import org.apache.commons.lang3.StringUtils;
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
 import org.jetbrains.annotations.NotNull;
@@ -265,6 +266,11 @@ public SQLFragment listFilesQuery()
     }
 
     public SQLFragment listFilesQuery(boolean skipCreatedModified)
+    {
+        return listFilesQuery(skipCreatedModified, null);
+    }
+
+    public SQLFragment listFilesQuery(boolean skipCreatedModified, String filePath)
     {
         final SQLFragment frag = new SQLFragment();
 
@@ -285,7 +291,10 @@ public SQLFragment listFilesQuery(boolean skipCreatedModified)
         frag.append("  ").append(OntologyManager.getTinfoObjectProperty(), "op").append(",\n");
         frag.append("  ").append(OntologyManager.getTinfoObject(), "o").append("\n");
         frag.append("WHERE\n");
-        frag.append("  op.StringValue IS NOT NULL AND\n");
+        if (StringUtils.isEmpty(filePath))
+            frag.append("  op.StringValue IS NOT NULL AND\n");
+        else
+            frag.append("  op.StringValue = ").appendStringLiteral(filePath, OntologyManager.getTinfoObject().getSqlDialect()).append(" AND\n");
         frag.append("  o.ObjectId = op.ObjectId AND\n");
         frag.append("  PropertyId IN (\n");
         frag.append("    SELECT PropertyId\n");
@@ -296,8 +305,8 @@ public SQLFragment listFilesQuery(boolean skipCreatedModified)
         hardTableFileLinkColumns((schema, table, pathColumn, containerId, domainUri) -> {
             SQLFragment containerFrag = new SQLFragment("?", containerId);
             TableUpdaterFileListener updater = new TableUpdaterFileListener(table, pathColumn.getColumnName(), TableUpdaterFileListener.Type.filePath, null, containerFrag);
-            frag.append("UNION\n");
-            frag.append(updater.listFilesQuery(skipCreatedModified));
+            frag.append("UNION").append(StringUtils.isEmpty(filePath) ? "" : " ALL" /*keep duplicate*/).append("\n");
+            frag.append(updater.listFilesQuery(skipCreatedModified, filePath));
         });
 
         return frag;

experiment/src/org/labkey/experiment/api/ExpRunTableImpl.java

@@ -27,6 +27,7 @@
 import org.labkey.api.collections.NamedObjectList;
 import org.labkey.api.data.*;
 import org.labkey.api.dataiterator.DetailedAuditLogDataIterator;
+import org.labkey.api.exp.ExperimentException;
 import org.labkey.api.exp.PropertyColumn;
 import org.labkey.api.exp.PropertyDescriptor;
 import org.labkey.api.exp.PropertyType;
@@ -1139,7 +1140,7 @@ public Map<String, Object> moveRows(User user, Container container, Container ta
                         Map<String, Integer> response = ExperimentService.get().moveAssayRuns(expRuns.get(c), c, targetContainer, user, auditUserComment, auditType);
                         incrementCounts(allContainerResponse, response);
                     }
-                    catch (IllegalArgumentException e)
+                    catch (IllegalArgumentException | ExperimentException e)
                     {
                         throw new BatchValidationException(new ValidationException(e.getMessage()));
                     }

experiment/src/org/labkey/experiment/api/ExperimentServiceImpl.java

@@ -52,6 +52,7 @@
 import org.labkey.api.audit.ExperimentAuditEvent;
 import org.labkey.api.audit.TransactionAuditProvider;
 import org.labkey.api.audit.provider.ContainerAuditProvider;
+import org.labkey.api.audit.provider.FileSystemAuditProvider;
 import org.labkey.api.cache.Cache;
 import org.labkey.api.cache.CacheLoader;
 import org.labkey.api.cache.CacheManager;
@@ -210,6 +211,7 @@
 import org.labkey.api.security.permissions.AdminPermission;
 import org.labkey.api.security.permissions.DeletePermission;
 import org.labkey.api.security.permissions.ReadPermission;
+import org.labkey.api.security.permissions.UpdatePermission;
 import org.labkey.api.security.roles.ProjectAdminRole;
 import org.labkey.api.settings.AppProps;
 import org.labkey.api.study.Dataset;
@@ -258,6 +260,7 @@
 import java.io.UncheckedIOException;
 import java.net.MalformedURLException;
 import java.net.URI;
+import java.nio.file.Files;
 import java.nio.file.Path;
 import java.sql.Connection;
 import java.sql.PreparedStatement;
@@ -9761,7 +9764,7 @@ private int splitExperimentRuns(List<ExpRun> runs, Map<Integer, Set<ExpData>> mo
     }
 
     @Override
-    public Map<String, Integer> moveAssayRuns(@NotNull List<? extends ExpRun> assayRuns, Container container, Container targetContainer, User user, String userComment, AuditBehaviorType auditBehavior)
+    public Map<String, Integer> moveAssayRuns(@NotNull List<? extends ExpRun> assayRuns, Container container, Container targetContainer, User user, String userComment, AuditBehaviorType auditBehavior) throws ExperimentException
     {
         if (assayRuns.isEmpty())
             throw new IllegalArgumentException("No assayRuns provided to move operation.");
@@ -9816,7 +9819,7 @@ public Map<String, Integer> moveAssayRuns(@NotNull List<? extends ExpRun> assayR
                 for (List<AbstractAssayProvider.AssayFileMoveData> runFileRenameData : assayMoveData.fileMovesByRunId().values())
                 {
                     for (AbstractAssayProvider.AssayFileMoveData renameData : runFileRenameData)
-                        moveFile(renameData);
+                        moveFile(renameData, container, user);
                 }
             }, POSTCOMMIT);
 
@@ -9826,7 +9829,7 @@ public Map<String, Integer> moveAssayRuns(@NotNull List<? extends ExpRun> assayR
         return assayMoveData.counts();
     }
 
-    private boolean moveFile(AbstractAssayProvider.AssayFileMoveData renameData)
+    private boolean moveFile(AbstractAssayProvider.AssayFileMoveData renameData, Container sourceContainer, User user)
     {
         String fieldName = renameData.fieldName() == null ? "datafileurl" : renameData.fieldName();
         File targetFile = renameData.targetFile();
@@ -9846,18 +9849,10 @@ private boolean moveFile(AbstractAssayProvider.AssayFileMoveData renameData)
                 return false;
             }
         }
-        if (!sourceFile.renameTo(targetFile))
-        {
-            LOG.warn(String.format("Rename of '%s' to '%s' for '%s' assay run '%s' (field: '%s') failed.",
-                    sourceFile.getAbsolutePath(),
-                    targetFile.getAbsolutePath(),
-                    assayName,
-                    runName,
-                    fieldName));
-            return false;
-        }
 
-        return true;
+
+        String changeDetail = String.format("'%s' assay run '%s' (field: '%s')", assayName, runName, fieldName);
+        return moveFileLinkFile(sourceFile, targetFile, sourceContainer, user, changeDetail);
     }
 
     @Override
@@ -10033,6 +10028,68 @@ public boolean isLookupToMaterials(DomainProperty dp)
         return type.isText() || type.isInteger();
     }
 
+    /**
+     *
+     * Move file (post-commit) after moving sample/assay data
+     */
+    public boolean moveFileLinkFile(File sourceFile, File targetFile, Container sourceFileContainer, User user, String actionComment)
+    {
+        if (!sourceFile.exists())
+            return false;
+
+        // if not otherwise referenced, move the file. Otherwise, copy the file
+        boolean isMove = getFileReferenceCount(user, sourceFileContainer, sourceFile) == 0; // source record already moved to target folder
+        FileSystemAuditProvider.FileSystemAuditEvent event = null;
+        if (isMove)
+        {
+            boolean success = sourceFile.renameTo(targetFile);
+            if (!success)
+            {
+                LOG.warn(String.format("Rename of '%s' to '%s' failed for %s" , sourceFile.getAbsolutePath(), targetFile.getAbsolutePath(), actionComment));
+                return false;
+            }
+
+            event = new FileSystemAuditProvider.FileSystemAuditEvent(sourceFileContainer, "File moved to: " + targetFile.getAbsolutePath() + " for " + actionComment);
+        }
+        else
+        {
+            try
+            {
+                Files.copy(sourceFile.toPath(), targetFile.toPath());
+                event = new FileSystemAuditProvider.FileSystemAuditEvent(sourceFileContainer, "File copied to: " + targetFile.getAbsolutePath() + " for " + actionComment);
+            }
+            catch (IOException e)
+            {
+                LOG.warn(String.format("Copy of '%s' to '%s' failed for " + actionComment, sourceFile.getAbsolutePath(), targetFile.getAbsolutePath()));
+                return false;
+            }
+        }
+
+        event.setDirectory(sourceFile.getParentFile().getAbsolutePath());
+        event.setFile(sourceFile.getName());
+        event.setResourcePath(sourceFile.getAbsolutePath());
+
+        AuditLogService.get().addEvent(user, event);
+        return true;
+    }
+
+    public long getFileReferenceCount(User user, Container container, File file)
+    {
+        if (!container.hasPermission(user, UpdatePermission.class))
+            throw new UnauthorizedException("You don't have the required permission to perform this action");
+
+        FileLinkFileListener fileListener = new FileLinkFileListener();
+        SQLFragment unionSql = fileListener.listFilesQuery(true, file.getAbsolutePath());
+
+        return new SqlSelector(CoreSchema.getInstance().getSchema(), unionSql).getRowCount();
+    }
+
+    @Override
+    public boolean canMoveFileReference(User user, Container container, File file, int moveCount)
+    {
+        return getFileReferenceCount(user, container, file) <= moveCount;
+    }
+
     public Map<String, Map<String, MissingFilesCheckInfo>> doMissingFilesCheck(User user, Container container, boolean trackMissingFiles) throws SQLException
     {
         if (container == null)