From 10e16151a585491e0f80f05bf37189d4a8c34ab4 Mon Sep 17 00:00:00 2001 From: XingY Date: Mon, 26 May 2025 18:41:28 -0700 Subject: [PATCH 1/6] Issue 52959: LKSM/LKB: Existing file not shown in Bulk Edit Issue 52900: LKSM: Moving a sample to another folder breaks other references to shared files Issue 53070: Moving files could result in duplicate exp.data records --- .../api/assay/AbstractAssayProvider.java | 22 +++-- .../org/labkey/api/assay/AssayProvider.java | 2 +- .../labkey/api/exp/api/ExperimentService.java | 4 +- .../api/files/TableUpdaterFileListener.java | 23 ++++-- .../experiment/ExpDataFileListener.java | 9 +- .../experiment/FileLinkFileListener.java | 13 ++- .../experiment/api/ExpRunTableImpl.java | 3 +- .../experiment/api/ExperimentServiceImpl.java | 82 +++++++++++++++---- .../experiment/api/SampleTypeServiceImpl.java | 25 +++--- .../api/SampleTypeUpdateServiceDI.java | 4 +- 10 files changed, 139 insertions(+), 48 deletions(-) diff --git a/api/src/org/labkey/api/assay/AbstractAssayProvider.java b/api/src/org/labkey/api/assay/AbstractAssayProvider.java index a4bf95f51c7..d45ad8837d9 100644 --- a/api/src/org/labkey/api/assay/AbstractAssayProvider.java +++ b/api/src/org/labkey/api/assay/AbstractAssayProvider.java @@ -1787,7 +1787,7 @@ public record AssayFileMoveData(ExpRun run, Container sourceContainer, String fi public record AssayMoveData(Map counts, Map> fileMovesByRunId) {} @Override - public void moveRuns(List runs, Container targetContainer, User user, AbstractAssayProvider.AssayMoveData assayMoveData) + public void moveRuns(List runs, Container targetContainer, User user, AbstractAssayProvider.AssayMoveData assayMoveData) throws ExperimentException { if (runs.isEmpty()) return; @@ -1867,7 +1867,7 @@ public void moveRuns(List runs, Container targetContainer, User user, Ab moveAssayResults(runs, assayProtocol, sourceContainer, targetContainer, user, assayMoveData); } - private void moveRunsBatch(List runs, Container sourceContainer, Container targetContainer, User user, AssayMoveData assayMoveData) + private void moveRunsBatch(List runs, Container sourceContainer, Container targetContainer, User user, AssayMoveData assayMoveData) throws ExperimentException { Map> movedFiles = assayMoveData.fileMovesByRunId(); @@ -1953,6 +1953,9 @@ private void moveRunsBatch(List runs, Container sourceContainer, Contain File updatedFile = fileContentService.getMoveTargetFile(sourceFileName, sourceContainer, targetContainer); if (updatedFile != null) { + if (!ExperimentService.get().canMoveFileReference(user, sourceContainer, sourceFile)) + throw new ExperimentException("Assay batch " + experiment.getName() + " cannot be moved since it references a shared file: " + sourceFile.getName()); + ExpRun run = batchRun.get(experiment.getRowId()); Integer runId = run.getRowId(); movedFiles.putIfAbsent(runId, new ArrayList<>()); @@ -1974,7 +1977,7 @@ private void moveRunsBatch(List runs, Container sourceContainer, Contain updateCounts.put("expObject", updateCounts.getOrDefault("expObject", 0) + expObjectCount); } - private void updateRunFiles(List runs, Container sourceContainer, Container targetContainer, User user, AssayMoveData assayMoveData) + private void updateRunFiles(List runs, Container sourceContainer, Container targetContainer, User user, AssayMoveData assayMoveData) throws ExperimentException { Map> movedFiles = assayMoveData.fileMovesByRunId(); @@ -2015,6 +2018,9 @@ private void updateRunFiles(List runs, Container sourceContainer, Contai File updatedFile = fileContentService.getMoveTargetFile(sourceFileName, sourceContainer, targetContainer); if (updatedFile != null) { + if (!ExperimentService.get().canMoveFileReference(user, sourceContainer, sourceFile)) + throw new ExperimentException("Assay run " + run.getName() + " cannot be moved since it references a shared file: " + sourceFile.getName()); + Integer runId = run.getRowId(); movedFiles.putIfAbsent(runId, new ArrayList<>()); movedFiles.get(runId).add(new AssayFileMoveData(run, run.getContainer(), fileProp.getName(), sourceFile, updatedFile)); @@ -2058,6 +2064,9 @@ private void updateDataFileUrl(List runs, Container sourceContainer, Con if (updatedFile != null) { ExpRun run = runMap.get(runId); + if (!ExperimentService.get().canMoveFileReference(user, sourceContainer, sourceFile)) + throw new ExperimentException("Assay run " + run.getName() + " cannot be moved since it references a shared file: " + sourceFile.getName()); + movedFiles.putIfAbsent(runId, new ArrayList<>()); movedFiles.get(runId).add(new AssayFileMoveData(run, run.getContainer(), null, sourceFile, updatedFile)); fileContentService.fireFileMoveEvent(sourceFile.toPath(), updatedFile.toPath(), user, sourceContainer, targetContainer); @@ -2070,7 +2079,7 @@ private void updateDataFileUrl(List runs, Container sourceContainer, Con } } - protected void moveAssayResults(List runs, ExpProtocol protocol, Container sourceContainer, Container targetContainer, User user, AssayMoveData assayMoveData) + protected void moveAssayResults(List runs, ExpProtocol protocol, Container sourceContainer, Container targetContainer, User user, AssayMoveData assayMoveData) throws ExperimentException { String tableName = AssayProtocolSchema.DATA_TABLE_NAME; AssaySchema schema = createProtocolSchema(user, targetContainer, protocol, null); @@ -2081,7 +2090,7 @@ protected void moveAssayResults(List runs, ExpProtocol protocol, Contain updateResultFiles(assayResultTable, runs, protocol, sourceContainer, targetContainer, user, assayMoveData); } - private void updateResultFiles(FilteredTable assayResultTable, List runs, ExpProtocol assayProtocol, Container sourceContainer, Container targetContainer, User user, AssayMoveData assayMoveData) + private void updateResultFiles(FilteredTable assayResultTable, List runs, ExpProtocol assayProtocol, Container sourceContainer, Container targetContainer, User user, AssayMoveData assayMoveData) throws ExperimentException { FileContentService fileContentService = FileContentService.get(); if (fileContentService == null) @@ -2131,6 +2140,9 @@ private void updateResultFiles(FilteredTable assayResultTable, List runs File sourceFile = new File(sourceFileName); ExpRun run = runMap.get(resultRunId); + if (!ExperimentService.get().canMoveFileReference(user, sourceContainer, sourceFile)) + throw new ExperimentException("Assay run " + run.getName() + " cannot be moved since it references a shared file: " + sourceFile.getName()); + movedFiles.putIfAbsent(resultRunId, new ArrayList<>()); movedFiles.get(resultRunId).add(new AssayFileMoveData(run, run.getContainer(), fileField, sourceFile, updatedFile)); diff --git a/api/src/org/labkey/api/assay/AssayProvider.java b/api/src/org/labkey/api/assay/AssayProvider.java index 925d74281ca..4e74be4acbd 100644 --- a/api/src/org/labkey/api/assay/AssayProvider.java +++ b/api/src/org/labkey/api/assay/AssayProvider.java @@ -389,7 +389,7 @@ default Long getResultRowCount(List protocols) return null; } - void moveRuns(List runs, Container targetContainer, User user, AbstractAssayProvider.AssayMoveData assayMoveData); + void moveRuns(List runs, Container targetContainer, User user, AbstractAssayProvider.AssayMoveData assayMoveData) throws ExperimentException; default void ensurePropertyDomainName(ExpProtocol protocol, ObjectProperty prop) { diff --git a/api/src/org/labkey/api/exp/api/ExperimentService.java b/api/src/org/labkey/api/exp/api/ExperimentService.java index db2d42348bf..791d6f4da9f 100644 --- a/api/src/org/labkey/api/exp/api/ExperimentService.java +++ b/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); + enum QueryOptions { UseLsidForUpdate, @@ -1124,7 +1126,7 @@ List getExpProtocolsWithParameterValue( int moveExperimentRuns(List runs, Container targetContainer, User user); - Map moveAssayRuns(List assayRuns, Container container, Container targetContainer, User user, String userComment, AuditBehaviorType auditBehavior); + Map moveAssayRuns(List assayRuns, Container container, Container targetContainer, User user, String userComment, AuditBehaviorType auditBehavior) throws ExperimentException; int aliasMapRowContainerUpdate(TableInfo aliasMapTable, List dataIds, Container targetContainer); diff --git a/api/src/org/labkey/api/files/TableUpdaterFileListener.java b/api/src/org/labkey/api/files/TableUpdaterFileListener.java index e76a38a45db..4d3f1793819 100644 --- a/api/src/org/labkey/api/files/TableUpdaterFileListener.java +++ b/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.makeLegalIdentifier(_table.getSchema().getName()); + String table = sqlDialect.makeLegalIdentifier(_table.getName()); + String column = sqlDialect.makeLegalIdentifier(_pathColumn.getName()); + return sqlDialect.getStringHandler().quoteStringLiteral(schema + "." + table + "." + column); + } + @Override public void fileCreated(@NotNull File created, @Nullable User user, @Nullable Container container) { @@ -344,12 +353,11 @@ public Collection 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(" ").append(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; } diff --git a/experiment/src/org/labkey/experiment/ExpDataFileListener.java b/experiment/src/org/labkey/experiment/ExpDataFileListener.java index 76f6374869a..6ebf42c7c22 100644 --- a/experiment/src/org/labkey/experiment/ExpDataFileListener.java +++ b/experiment/src/org/labkey/experiment/ExpDataFileListener.java @@ -76,8 +76,13 @@ public int fileMoved(@NotNull Path src, @NotNull Path dest, @Nullable User user, // 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; + ExpData existingData = ExperimentService.get().getExpDataByURL(data.getFilePath(), targetContainer); + // Issue 53070: Moving files could result in duplicate exp.data records + if (existingData != null) + { + data.save(user); + extra = 1; + } } return super.fileMoved(src, dest, user, sourceContainer) + extra; diff --git a/experiment/src/org/labkey/experiment/FileLinkFileListener.java b/experiment/src/org/labkey/experiment/FileLinkFileListener.java index 650c3bd45c8..57c379a615a 100644 --- a/experiment/src/org/labkey/experiment/FileLinkFileListener.java +++ b/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"); @@ -297,7 +306,7 @@ public SQLFragment listFilesQuery(boolean skipCreatedModified) 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(updater.listFilesQuery(skipCreatedModified, filePath)); }); return frag; diff --git a/experiment/src/org/labkey/experiment/api/ExpRunTableImpl.java b/experiment/src/org/labkey/experiment/api/ExpRunTableImpl.java index fc5e86aa57a..760093d8b3b 100644 --- a/experiment/src/org/labkey/experiment/api/ExpRunTableImpl.java +++ b/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 moveRows(User user, Container container, Container ta Map 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())); } diff --git a/experiment/src/org/labkey/experiment/api/ExperimentServiceImpl.java b/experiment/src/org/labkey/experiment/api/ExperimentServiceImpl.java index 2e8a6b23471..5f64a3dc508 100644 --- a/experiment/src/org/labkey/experiment/api/ExperimentServiceImpl.java +++ b/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 runs, Map> mo } @Override - public Map moveAssayRuns(@NotNull List assayRuns, Container container, Container targetContainer, User user, String userComment, AuditBehaviorType auditBehavior) + public Map moveAssayRuns(@NotNull List 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 moveAssayRuns(@NotNull List assayR for (List runFileRenameData : assayMoveData.fileMovesByRunId().values()) { for (AbstractAssayProvider.AssayFileMoveData renameData : runFileRenameData) - moveFile(renameData); + moveFile(renameData, container, user); } }, POSTCOMMIT); @@ -9826,7 +9829,7 @@ public Map moveAssayRuns(@NotNull List 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,65 @@ 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 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 " + actionComment, sourceFile.getAbsolutePath(), targetFile.getAbsolutePath())); + 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) + { + return getFileReferenceCount(user, container, file) <= 1; + } + public Map> doMissingFilesCheck(User user, Container container, boolean trackMissingFiles) throws SQLException { if (container == null) diff --git a/experiment/src/org/labkey/experiment/api/SampleTypeServiceImpl.java b/experiment/src/org/labkey/experiment/api/SampleTypeServiceImpl.java index b9b43484560..22e50258c18 100644 --- a/experiment/src/org/labkey/experiment/api/SampleTypeServiceImpl.java +++ b/experiment/src/org/labkey/experiment/api/SampleTypeServiceImpl.java @@ -1941,7 +1941,7 @@ public Map moveSamples(Collection sample for (List sampleFileRenameData : fileMovesBySampleId.values()) { for (FileFieldRenameData renameData : sampleFileRenameData) - moveFile(renameData); + moveFile(renameData, sourceContainer, user); } }, POSTCOMMIT); @@ -2053,7 +2053,7 @@ private int splitExperimentRuns(List runs, Map } // return the map of file renames - private Map> updateSampleFilePaths(ExpSampleType sampleType, List samples, Container targetContainer, User user) + private Map> updateSampleFilePaths(ExpSampleType sampleType, List samples, Container targetContainer, User user) throws ExperimentException { Map> sampleFileRenames = new HashMap<>(); @@ -2086,10 +2086,14 @@ private Map> updateSampleFilePaths(ExpSampleT for (DomainProperty fileProp : fileDomainProps ) { String sourceFileName = (String) sample.getProperty(fileProp); + // check if file is referenced by others File updatedFile = FileContentService.get().getMoveTargetFile(sourceFileName, sample.getContainer(), targetContainer); if (updatedFile != null) { - FileFieldRenameData renameData = new FileFieldRenameData(sampleType, sample.getName(), fileProp.getName(), new File(sourceFileName), updatedFile); + File sourceFile = new File(sourceFileName); + if (!ExperimentServiceImpl.get().canMoveFileReference(user, sample.getContainer(), sourceFile)) + throw new ExperimentException("Sample " + sample.getName() + " cannot be moved since it references a shared file: " + sourceFile.getName()); + FileFieldRenameData renameData = new FileFieldRenameData(sampleType, sample.getName(), fileProp.getName(), sourceFile, updatedFile); sampleFileRenames.putIfAbsent(sample.getRowId(), new ArrayList<>()); List fieldRenameData = sampleFileRenames.get(sample.getRowId()); fieldRenameData.add(renameData); @@ -2108,7 +2112,7 @@ private Map> updateSampleFilePaths(ExpSampleT return sampleFileRenames; } - private boolean moveFile(FileFieldRenameData renameData) + private boolean moveFile(FileFieldRenameData renameData, Container sourceContainer, User user) { if (!renameData.targetFile.getParentFile().exists()) { @@ -2131,18 +2135,9 @@ private boolean moveFile(FileFieldRenameData renameData) LOG.warn(errorMsg + e.getMessage()); } } - if (!renameData.sourceFile.renameTo(renameData.targetFile)) - { - LOG.warn(String.format("Rename of '%s' to '%s' for '%s' sample '%s' (field: '%s') failed.", - renameData.sourceFile.getAbsolutePath(), - renameData.targetFile.getAbsolutePath(), - renameData.sampleType.getName(), - renameData.sampleName, - renameData.fieldName)); - return false; - } - return true; + String changeDetail = String.format("'%s' sample '%s' (field: '%s')", renameData.sampleType.getName(), renameData.sampleName, renameData.fieldName); + return ExperimentServiceImpl.get().moveFileLinkFile(renameData.sourceFile, renameData.targetFile, sourceContainer, user, changeDetail); } @Override diff --git a/experiment/src/org/labkey/experiment/api/SampleTypeUpdateServiceDI.java b/experiment/src/org/labkey/experiment/api/SampleTypeUpdateServiceDI.java index 41f41da54ab..3323988eed5 100644 --- a/experiment/src/org/labkey/experiment/api/SampleTypeUpdateServiceDI.java +++ b/experiment/src/org/labkey/experiment/api/SampleTypeUpdateServiceDI.java @@ -554,7 +554,7 @@ public List> updateRows(User user, Container container, List @Override public Map moveRows(User user, Container container, Container targetContainer, List> rows, BatchValidationException errors, @Nullable Map configParameters, @Nullable Map extraScriptContext) - throws BatchValidationException, QueryUpdateServiceException + throws BatchValidationException { Map allContainerResponse = new HashMap<>(); @@ -576,7 +576,7 @@ public Map moveRows(User user, Container container, Container ta } catch (ExperimentException e) { - throw new QueryUpdateServiceException(e); + throw new BatchValidationException(new ValidationException(e.getMessage())); } } From c2b57eb647d21dc5f72b17daf750db5704137fb0 Mon Sep 17 00:00:00 2001 From: XingY Date: Tue, 27 May 2025 17:53:40 -0700 Subject: [PATCH 2/6] code review changes --- .../org/labkey/api/files/TableUpdaterFileListener.java | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/api/src/org/labkey/api/files/TableUpdaterFileListener.java b/api/src/org/labkey/api/files/TableUpdaterFileListener.java index 4d3f1793819..ce46e143c87 100644 --- a/api/src/org/labkey/api/files/TableUpdaterFileListener.java +++ b/api/src/org/labkey/api/files/TableUpdaterFileListener.java @@ -198,10 +198,10 @@ public String getSourceName() public String getSourceSelect(SqlDialect sqlDialect) { - String schema = sqlDialect.makeLegalIdentifier(_table.getSchema().getName()); - String table = sqlDialect.makeLegalIdentifier(_table.getName()); - String column = sqlDialect.makeLegalIdentifier(_pathColumn.getName()); - return sqlDialect.getStringHandler().quoteStringLiteral(schema + "." + table + "." + column); + 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 @@ -401,7 +401,7 @@ else if (_table.getColumn("Folder") != null) selectFrag.append(" NULL AS SourceKey,\n"); //selectFrag.append(" ? AS SourceName\n").add(getName()); - selectFrag.append(" ").append(getSourceSelect(_table.getSchema().getSqlDialect())).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()); From b9b9ee0f2bfe29d4793a728872fa06dc4e5a8987 Mon Sep 17 00:00:00 2001 From: XingY Date: Wed, 28 May 2025 14:36:44 -0700 Subject: [PATCH 3/6] Fix issue with files added via bulk update --- experiment/src/org/labkey/experiment/FileLinkFileListener.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/experiment/src/org/labkey/experiment/FileLinkFileListener.java b/experiment/src/org/labkey/experiment/FileLinkFileListener.java index 57c379a615a..7edfdc154d2 100644 --- a/experiment/src/org/labkey/experiment/FileLinkFileListener.java +++ b/experiment/src/org/labkey/experiment/FileLinkFileListener.java @@ -305,7 +305,7 @@ public SQLFragment listFilesQuery(boolean skipCreatedModified, String filePath) 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("UNION").append(StringUtils.isEmpty(filePath) ? "" : " ALL" /*keep duplicate*/).append("\n"); frag.append(updater.listFilesQuery(skipCreatedModified, filePath)); }); From 276a1cd675884a0d20affa5284814c63bbcb742a Mon Sep 17 00:00:00 2001 From: XingY Date: Thu, 29 May 2025 20:27:51 -0700 Subject: [PATCH 4/6] support moving a batch of the same file reference. add selenium tests --- .../api/assay/AbstractAssayProvider.java | 119 +++++++++++++----- .../labkey/api/exp/api/ExperimentService.java | 2 +- .../experiment/ExpDataFileListener.java | 25 ++-- .../experiment/api/ExperimentServiceImpl.java | 4 +- .../experiment/api/SampleTypeServiceImpl.java | 29 +++-- 5 files changed, 127 insertions(+), 52 deletions(-) diff --git a/api/src/org/labkey/api/assay/AbstractAssayProvider.java b/api/src/org/labkey/api/assay/AbstractAssayProvider.java index d45ad8837d9..d021b65453d 100644 --- a/api/src/org/labkey/api/assay/AbstractAssayProvider.java +++ b/api/src/org/labkey/api/assay/AbstractAssayProvider.java @@ -74,7 +74,6 @@ import org.labkey.api.exp.api.IAssayDomainType; import org.labkey.api.exp.property.Domain; import org.labkey.api.exp.property.DomainProperty; -import org.labkey.api.exp.property.DomainUtil; import org.labkey.api.exp.property.PropertyService; import org.labkey.api.exp.query.ExpRunTable; import org.labkey.api.files.FileContentService; @@ -1926,6 +1925,9 @@ private void moveRunsBatch(List runs, Container sourceContainer, Contain .getProperties().stream() .filter(prop -> PropertyType.FILE_LINK.getTypeUri().equals(prop.getRangeURI())).toList(); + Map fileMoveReferences = new HashMap<>(); + Map fileMoveCounts = new HashMap<>(); + FileContentService fileContentService = FileContentService.get(); if (fileContentService != null && !fileDomainProps.isEmpty()) { @@ -1953,18 +1955,32 @@ private void moveRunsBatch(List runs, Container sourceContainer, Contain File updatedFile = fileContentService.getMoveTargetFile(sourceFileName, sourceContainer, targetContainer); if (updatedFile != null) { - if (!ExperimentService.get().canMoveFileReference(user, sourceContainer, sourceFile)) - throw new ExperimentException("Assay batch " + experiment.getName() + " cannot be moved since it references a shared file: " + sourceFile.getName()); + if (!fileMoveReferences.containsKey(sourceFileName)) + fileMoveReferences.put(sourceFileName, new AssayFileMoveReference(sourceFileName, updatedFile, experiment.getName())); + + if (!fileMoveCounts.containsKey(sourceFileName)) + fileMoveCounts.put(sourceFileName, 0); + fileMoveCounts.put(sourceFileName, fileMoveCounts.get(sourceFileName) + 1); ExpRun run = batchRun.get(experiment.getRowId()); Integer runId = run.getRowId(); movedFiles.putIfAbsent(runId, new ArrayList<>()); movedFiles.get(runId).add(new AssayFileMoveData(run, run.getContainer(), fileProp.getName(), sourceFile, updatedFile)); - fileContentService.fireFileMoveEvent(sourceFile.toPath(), updatedFile.toPath(), user, sourceContainer, targetContainer); } } } + for (String sourceFileName : fileMoveReferences.keySet()) + { + AssayFileMoveReference ref = fileMoveReferences.get(sourceFileName); + int count = fileMoveCounts.get(sourceFileName); + File sourceFile = new File(sourceFileName); + if (!ExperimentService.get().canMoveFileReference(user, sourceContainer, sourceFile, count)) + throw new ExperimentException("Assay batch " + ref.runName + " cannot be moved since it references a shared file: " + sourceFile.getName()); + + fileContentService.fireFileMoveEvent(sourceFile.toPath(), ref.updatedFile.toPath(), user, sourceContainer, targetContainer); + } + } // update Exp.Object where object.objecturi IN batchLsids @@ -1994,6 +2010,9 @@ private void updateRunFiles(List runs, Container sourceContainer, Contai if (fileContentService == null) return; + Map fileMoveReferences = new HashMap<>(); + Map fileMoveCounts = new HashMap<>(); + for (ExpRun run : runs) { for (DomainProperty fileProp : fileDomainProps) @@ -2018,16 +2037,31 @@ private void updateRunFiles(List runs, Container sourceContainer, Contai File updatedFile = fileContentService.getMoveTargetFile(sourceFileName, sourceContainer, targetContainer); if (updatedFile != null) { - if (!ExperimentService.get().canMoveFileReference(user, sourceContainer, sourceFile)) - throw new ExperimentException("Assay run " + run.getName() + " cannot be moved since it references a shared file: " + sourceFile.getName()); + if (!fileMoveReferences.containsKey(sourceFileName)) + fileMoveReferences.put(sourceFileName, new AssayFileMoveReference(sourceFileName, updatedFile, run.getName())); + + if (!fileMoveCounts.containsKey(sourceFileName)) + fileMoveCounts.put(sourceFileName, 0); + fileMoveCounts.put(sourceFileName, fileMoveCounts.get(sourceFileName) + 1); Integer runId = run.getRowId(); movedFiles.putIfAbsent(runId, new ArrayList<>()); movedFiles.get(runId).add(new AssayFileMoveData(run, run.getContainer(), fileProp.getName(), sourceFile, updatedFile)); - fileContentService.fireFileMoveEvent(sourceFile.toPath(), updatedFile.toPath(), user, sourceContainer, targetContainer); } } } + + for (String sourceFileName : fileMoveReferences.keySet()) + { + AssayFileMoveReference ref = fileMoveReferences.get(sourceFileName); + int count = fileMoveCounts.get(sourceFileName); + File sourceFile = new File(sourceFileName); + if (!ExperimentService.get().canMoveFileReference(user, sourceContainer, sourceFile, count)) + throw new ExperimentException("Assay run " + ref.runName + " cannot be moved since it references a shared file: " + sourceFile.getName()); + + fileContentService.fireFileMoveEvent(sourceFile.toPath(), ref.updatedFile.toPath(), user, sourceContainer, targetContainer); + } + } private void updateDataFileUrl(List runs, Container sourceContainer, Container targetContainer, User user, AssayMoveData assayMoveData) @@ -2064,7 +2098,7 @@ private void updateDataFileUrl(List runs, Container sourceContainer, Con if (updatedFile != null) { ExpRun run = runMap.get(runId); - if (!ExperimentService.get().canMoveFileReference(user, sourceContainer, sourceFile)) + if (!ExperimentService.get().canMoveFileReference(user, sourceContainer, sourceFile, 1)) throw new ExperimentException("Assay run " + run.getName() + " cannot be moved since it references a shared file: " + sourceFile.getName()); movedFiles.putIfAbsent(runId, new ArrayList<>()); @@ -2090,6 +2124,8 @@ protected void moveAssayResults(List runs, ExpProtocol protocol, Contain updateResultFiles(assayResultTable, runs, protocol, sourceContainer, targetContainer, user, assayMoveData); } + record AssayFileMoveReference(String sourceFilePath, File updatedFile, String runName) {} + private void updateResultFiles(FilteredTable assayResultTable, List runs, ExpProtocol assayProtocol, Container sourceContainer, Container targetContainer, User user, AssayMoveData assayMoveData) throws ExperimentException { FileContentService fileContentService = FileContentService.get(); @@ -2121,6 +2157,8 @@ private void updateResultFiles(FilteredTable assayResultTable, List runs Map runMap = new HashMap<>(); runs.forEach(run -> runMap.put(run.getRowId(), run)); + Map fileMoveReferences = new HashMap<>(); + Map> fileMoveResultRowIds = new HashMap<>(); for (String fileField : fileFields) { var fileColumn = assayResultTable.getColumn(fileField); @@ -2134,43 +2172,58 @@ private void updateResultFiles(FilteredTable assayResultTable, List runs continue; Integer resultRowId = (Integer) resultRow.get("rowid"); Integer resultRunId = (Integer) resultRow.get("run"); + if (!fileMoveResultRowIds.containsKey(sourceFileName)) + fileMoveResultRowIds.put(sourceFileName, new ArrayList<>()); + fileMoveResultRowIds.get(sourceFileName).add(resultRowId); File updatedFile = fileContentService.getMoveTargetFile(sourceFileName, sourceContainer, targetContainer); if (updatedFile != null) { File sourceFile = new File(sourceFileName); ExpRun run = runMap.get(resultRunId); - if (!ExperimentService.get().canMoveFileReference(user, sourceContainer, sourceFile)) - throw new ExperimentException("Assay run " + run.getName() + " cannot be moved since it references a shared file: " + sourceFile.getName()); + if (!fileMoveReferences.containsKey(sourceFileName)) + fileMoveReferences.put(sourceFileName, new AssayFileMoveReference(sourceFileName, updatedFile, run.getName())); movedFiles.putIfAbsent(resultRunId, new ArrayList<>()); movedFiles.get(resultRunId).add(new AssayFileMoveData(run, run.getContainer(), fileField, sourceFile, updatedFile)); + } + } - // update the exp.Object row for the file results row - // TODO refactor so that the updates to the related exp.Object rows are done in the ExpDataFileListener (see similar updates in moveRuns() and moveRunsBatch()) - // TODO also use exp.data.objectId instead of objecturi - TableInfo expDataTable = ExperimentService.get().getTinfoData(); - TableInfo expObjectTable = OntologyManager.getTinfoObject(); - SQLFragment updateSql = new SQLFragment("UPDATE ").append(expObjectTable) - .append(" SET container = ").appendValue(targetContainer.getEntityId()) - .append(" WHERE objecturi IN ( SELECT lsid FROM ") - .append(expDataTable) - .append(" WHERE datafileurl = ").appendValue(FileUtil.pathToString(sourceFile.toPath())); - updateSql.append(")"); - new SqlExecutor(expObjectTable.getSchema()).execute(updateSql); - - fileContentService.fireFileMoveEvent(sourceFile.toPath(), updatedFile.toPath(), user, sourceContainer, targetContainer); + for (String sourceFileName : fileMoveReferences.keySet()) + { + AssayFileMoveReference ref = fileMoveReferences.get(sourceFileName); + int count = fileMoveResultRowIds.get(sourceFileName).size(); + File sourceFile = new File(sourceFileName); + if (!ExperimentService.get().canMoveFileReference(user, sourceContainer, sourceFile, count)) + throw new ExperimentException("Assay run " + ref.runName + " cannot be moved since it references a shared file: " + sourceFile.getName()); + + File updatedFile = ref.updatedFile; + // update the exp.Object row for the file results row + // TODO refactor so that the updates to the related exp.Object rows are done in the ExpDataFileListener (see similar updates in moveRuns() and moveRunsBatch()) + // TODO also use exp.data.objectId instead of objecturi + TableInfo expDataTable = ExperimentService.get().getTinfoData(); + TableInfo expObjectTable = OntologyManager.getTinfoObject(); + SQLFragment updateSql = new SQLFragment("UPDATE ").append(expObjectTable) + .append(" SET container = ").appendValue(targetContainer.getEntityId()) + .append(" WHERE objecturi IN ( SELECT lsid FROM ") + .append(expDataTable) + .append(" WHERE datafileurl = ").appendValue(FileUtil.pathToString(sourceFile.toPath())); + updateSql.append(")"); + new SqlExecutor(expObjectTable.getSchema()).execute(updateSql); + + fileContentService.fireFileMoveEvent(sourceFile.toPath(), updatedFile.toPath(), user, sourceContainer, targetContainer); + + var realTable = assayResultTable.getRealTable(); + var realFileColumn = realTable.getColumn(fileField); + updateSql = new SQLFragment("UPDATE ").append(assayResultTable.getRealTable()) + .append(" SET ") + .appendIdentifier(realFileColumn.getSelectIdentifier()) + .append(" = ").appendValue(updatedFile.getAbsolutePath()) + .append(" WHERE rowId ").appendInClause(fileMoveResultRowIds.get(sourceFileName), realTable.getSqlDialect()); + new SqlExecutor(assayResultTable.getSchema()).execute(updateSql); - var realTable = assayResultTable.getRealTable(); - var realFileColumn = realTable.getColumn(fileField); - updateSql = new SQLFragment("UPDATE ").append(assayResultTable.getRealTable()) - .append(" SET ") - .appendIdentifier(realFileColumn.getSelectIdentifier()) - .append(" = ").appendValue(updatedFile.getAbsolutePath()) - .append(" WHERE rowId = ").appendValue(resultRowId); - new SqlExecutor(assayResultTable.getSchema()).execute(updateSql); - } } + } } } diff --git a/api/src/org/labkey/api/exp/api/ExperimentService.java b/api/src/org/labkey/api/exp/api/ExperimentService.java index 791d6f4da9f..729dcb1f9af 100644 --- a/api/src/org/labkey/api/exp/api/ExperimentService.java +++ b/api/src/org/labkey/api/exp/api/ExperimentService.java @@ -138,7 +138,7 @@ static void setInstance(ExperimentService impl) ServiceRegistry.get().registerService(ExperimentService.class, impl); } - boolean canMoveFileReference(User user, Container sourceContainer, File sourceFile); + boolean canMoveFileReference(User user, Container sourceContainer, File sourceFile, int moveCount); enum QueryOptions { diff --git a/experiment/src/org/labkey/experiment/ExpDataFileListener.java b/experiment/src/org/labkey/experiment/ExpDataFileListener.java index 6ebf42c7c22..42f66d904db 100644 --- a/experiment/src/org/labkey/experiment/ExpDataFileListener.java +++ b/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,15 +77,18 @@ 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); - ExpData existingData = ExperimentService.get().getExpDataByURL(data.getFilePath(), targetContainer); - // Issue 53070: Moving files could result in duplicate exp.data records - if (existingData != null) + 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; } diff --git a/experiment/src/org/labkey/experiment/api/ExperimentServiceImpl.java b/experiment/src/org/labkey/experiment/api/ExperimentServiceImpl.java index 5f64a3dc508..db076e6ca18 100644 --- a/experiment/src/org/labkey/experiment/api/ExperimentServiceImpl.java +++ b/experiment/src/org/labkey/experiment/api/ExperimentServiceImpl.java @@ -10082,9 +10082,9 @@ public long getFileReferenceCount(User user, Container container, File file) } @Override - public boolean canMoveFileReference(User user, Container container, File file) + public boolean canMoveFileReference(User user, Container container, File file, int moveCount) { - return getFileReferenceCount(user, container, file) <= 1; + return getFileReferenceCount(user, container, file) <= moveCount; } public Map> doMissingFilesCheck(User user, Container container, boolean trackMissingFiles) throws SQLException diff --git a/experiment/src/org/labkey/experiment/api/SampleTypeServiceImpl.java b/experiment/src/org/labkey/experiment/api/SampleTypeServiceImpl.java index 22e50258c18..b068c133637 100644 --- a/experiment/src/org/labkey/experiment/api/SampleTypeServiceImpl.java +++ b/experiment/src/org/labkey/experiment/api/SampleTypeServiceImpl.java @@ -2052,6 +2052,8 @@ private int splitExperimentRuns(List runs, Map return runCount; } + record SampleFileMoveReference(String sourceFilePath, File targetFile, Container sourceContainer, String sampleName) {} + // return the map of file renames private Map> updateSampleFilePaths(ExpSampleType sampleType, List samples, Container targetContainer, User user) throws ExperimentException { @@ -2077,6 +2079,8 @@ private Map> updateSampleFilePaths(ExpSampleT return sampleFileRenames; Map hasFileRoot = new HashMap<>(); + Map fileMoveCounts = new HashMap<>(); + Map fileMoveReferences = new HashMap<>(); for (ExpMaterial sample : samples) { boolean hasSourceRoot = hasFileRoot.computeIfAbsent(sample.getContainer(), (container) -> fileService.getFileRoot(container) != null); @@ -2086,13 +2090,19 @@ private Map> updateSampleFilePaths(ExpSampleT for (DomainProperty fileProp : fileDomainProps ) { String sourceFileName = (String) sample.getProperty(fileProp); - // check if file is referenced by others + if (StringUtils.isBlank(sourceFileName)) + continue; File updatedFile = FileContentService.get().getMoveTargetFile(sourceFileName, sample.getContainer(), targetContainer); if (updatedFile != null) { + + if (!fileMoveReferences.containsKey(sourceFileName)) + fileMoveReferences.put(sourceFileName, new SampleFileMoveReference(sourceFileName, updatedFile, sample.getContainer(), sample.getName())); + if (!fileMoveCounts.containsKey(sourceFileName)) + fileMoveCounts.put(sourceFileName, 0); + fileMoveCounts.put(sourceFileName, fileMoveCounts.get(sourceFileName) + 1); + File sourceFile = new File(sourceFileName); - if (!ExperimentServiceImpl.get().canMoveFileReference(user, sample.getContainer(), sourceFile)) - throw new ExperimentException("Sample " + sample.getName() + " cannot be moved since it references a shared file: " + sourceFile.getName()); FileFieldRenameData renameData = new FileFieldRenameData(sampleType, sample.getName(), fileProp.getName(), sourceFile, updatedFile); sampleFileRenames.putIfAbsent(sample.getRowId(), new ArrayList<>()); List fieldRenameData = sampleFileRenames.get(sample.getRowId()); @@ -2101,12 +2111,15 @@ private Map> updateSampleFilePaths(ExpSampleT } } - // TODO, support batch fireFileMoveEvent to avoid excessive FileLinkFileListener.hardTableFileLinkColumns calls - for (int sampleId: sampleFileRenames.keySet()) + for (String filePath : fileMoveReferences.keySet()) { - List fieldRenameRecords = sampleFileRenames.get(sampleId); - for (FileFieldRenameData renameData : fieldRenameRecords) - fileService.fireFileMoveEvent(renameData.sourceFile, renameData.targetFile, user, targetContainer); + SampleFileMoveReference ref = fileMoveReferences.get(filePath); + File sourceFile = new File(filePath); + if (!ExperimentServiceImpl.get().canMoveFileReference(user, ref.sourceContainer, sourceFile, fileMoveCounts.get(filePath))) + throw new ExperimentException("Sample " + ref.sampleName + " cannot be moved since it references a shared file: " + sourceFile.getName()); + + // TODO, support batch fireFileMoveEvent to avoid excessive FileLinkFileListener.hardTableFileLinkColumns calls + fileService.fireFileMoveEvent(sourceFile, ref.targetFile, user, targetContainer); } return sampleFileRenames; From e879479353193df5168f917a2e00c7b0742d776c Mon Sep 17 00:00:00 2001 From: XingY Date: Fri, 30 May 2025 13:49:43 -0700 Subject: [PATCH 5/6] add test for moving multiple runs --- .../src/org/labkey/experiment/api/ExperimentServiceImpl.java | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/experiment/src/org/labkey/experiment/api/ExperimentServiceImpl.java b/experiment/src/org/labkey/experiment/api/ExperimentServiceImpl.java index db076e6ca18..4ca0b31b3ba 100644 --- a/experiment/src/org/labkey/experiment/api/ExperimentServiceImpl.java +++ b/experiment/src/org/labkey/experiment/api/ExperimentServiceImpl.java @@ -10034,6 +10034,9 @@ public boolean isLookupToMaterials(DomainProperty dp) */ 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; @@ -10042,7 +10045,7 @@ public boolean moveFileLinkFile(File sourceFile, File targetFile, Container sour boolean success = sourceFile.renameTo(targetFile); if (!success) { - LOG.warn(String.format("Rename of '%s' to '%s' failed for " + actionComment, sourceFile.getAbsolutePath(), targetFile.getAbsolutePath())); + LOG.warn(String.format("Rename of '%s' to '%s' failed for %s" , sourceFile.getAbsolutePath(), targetFile.getAbsolutePath(), actionComment)); return false; } From 0419cfe444c7c301c331a627554ad67c21f019e9 Mon Sep 17 00:00:00 2001 From: XingY Date: Fri, 30 May 2025 15:50:40 -0700 Subject: [PATCH 6/6] fix exception related to invalid name expression --- .../org/labkey/api/data/NameGeneratorState.java | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/api/src/org/labkey/api/data/NameGeneratorState.java b/api/src/org/labkey/api/data/NameGeneratorState.java index 79c5d4eff52..5011de75e1e 100644 --- a/api/src/org/labkey/api/data/NameGeneratorState.java +++ b/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 rowMap, Map 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 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 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);