From 6eae64d61dfeb103e5c112098f88ee9bb37771db Mon Sep 17 00:00:00 2001 From: labkey-nicka Date: Fri, 7 Nov 2025 16:58:41 -0800 Subject: [PATCH 01/10] Issue 54218: Escape special characters in form field names --- .../org/labkey/api/assay/AssayFileWriter.java | 127 ++++++++++-------- .../org/labkey/api/data/TableViewForm.java | 53 +++++++- .../org/labkey/api/query/QueryUpdateForm.java | 43 +----- 3 files changed, 122 insertions(+), 101 deletions(-) diff --git a/api/src/org/labkey/api/assay/AssayFileWriter.java b/api/src/org/labkey/api/assay/AssayFileWriter.java index 5164c5689e3..40953e74b80 100644 --- a/api/src/org/labkey/api/assay/AssayFileWriter.java +++ b/api/src/org/labkey/api/assay/AssayFileWriter.java @@ -24,6 +24,7 @@ import org.labkey.api.audit.provider.FileSystemAuditProvider; import org.labkey.api.collections.CollectionUtils; import org.labkey.api.data.Container; +import org.labkey.api.data.TableViewForm; import org.labkey.api.exp.ExperimentException; import org.labkey.api.exp.api.ExpProtocol; import org.labkey.api.pipeline.PipeRoot; @@ -40,9 +41,11 @@ import java.io.IOException; import java.io.InputStream; import java.util.ArrayDeque; +import java.util.ArrayList; +import java.util.Collections; import java.util.Deque; +import java.util.HashMap; import java.util.HashSet; -import java.util.Iterator; import java.util.List; import java.util.Map; import java.util.Set; @@ -229,72 +232,84 @@ public String getFileName(MultipartFile file) public Map savePostedFiles(ContextType context, @NotNull Set parameterNames, boolean allowMultiple, boolean ensureExpData) throws ExperimentException, IOException { + if (!(context.getRequest() instanceof MultipartHttpServletRequest multipartRequest)) + return Collections.emptyMap(); + Map files = CollectionUtils.enforceValueClass(new TreeMap<>(), FileLike.class); Set originalFileNames = new HashSet<>(); - if (context.getRequest() instanceof MultipartHttpServletRequest multipartRequest) + Map encodedParameterNames = new HashMap<>(); + for (String parameterName : parameterNames) + encodedParameterNames.put(TableViewForm.getMultiPartFormFieldNameForColumn(parameterName), parameterName); + + Deque overflowFiles = new ArrayDeque<>(); // using a deque for easy removal of single elements + Set unusedParameterNames = new HashSet<>(parameterNames); + List auditEvents = new ArrayList<>(); + + for (Map.Entry> entry : multipartRequest.getMultiFileMap().entrySet()) { - Iterator>> iter = multipartRequest.getMultiFileMap().entrySet().iterator(); - Deque overflowFiles = new ArrayDeque<>(); // using a deque for easy removal of single elements - Set unusedParameterNames = new HashSet<>(parameterNames); - while (iter.hasNext()) + if (!(encodedParameterNames.containsKey(entry.getKey()) || parameterNames.contains(entry.getKey()))) + continue; + + boolean isAfterFirstFile = false; + for (MultipartFile multipartFile : entry.getValue()) { - Map.Entry> entry = iter.next(); - if (parameterNames.contains(entry.getKey())) + String fileName = getFileName(multipartFile); + if (!fileName.isEmpty() && !originalFileNames.add(fileName)) + throw new ExperimentException("The file '" + fileName + " ' was uploaded twice - all files must be unique"); + + if (multipartFile.isEmpty()) + continue; + + FileLike dir = getFileTargetDir(context); + FileLike file = FileUtil.findUniqueFileName(fileName, dir); + multipartFile.transferTo(toFileForWrite(file)); + if (!dir.toURI().getPath().contains(TEMP_DIR_NAME)) { - List multipartFiles = entry.getValue(); - boolean isAfterFirstFile = false; - for (MultipartFile multipartFile : multipartFiles) - { - String fileName = getFileName(multipartFile); - if (!fileName.isEmpty() && !originalFileNames.add(fileName)) - { - throw new ExperimentException("The file '" + fileName + " ' was uploaded twice - all files must be unique"); - } - if (!multipartFile.isEmpty()) - { - FileLike dir = getFileTargetDir(context); - FileLike file = FileUtil.findUniqueFileName(fileName, dir); - multipartFile.transferTo(toFileForWrite(file)); - if (!dir.toURI().getPath().contains(TEMP_DIR_NAME)) - { - FileSystemAuditProvider.FileSystemAuditEvent event = new FileSystemAuditProvider.FileSystemAuditEvent(context.getContainer(), allowMultiple ? "File field provided for assay import" : "Primary file provided for assay import"); - event.setProvidedFileName(fileName); - event.setFile(file.getName()); - event.setDirectory(dir.toURI().getPath()); - AuditLogService.get().addEvent(context.getUser(), event); - } - if (!isAfterFirstFile) // first file gets stored with multipartFile's name - { - files.put(multipartFile.getName(), file); - isAfterFirstFile = true; - unusedParameterNames.remove(multipartFile.getName()); - } - else // other files get stored in leftover keys later to store only one file per key (bit of a hack) - { - overflowFiles.add(file); - } - - if (ensureExpData) - AbstractQueryUpdateService.ensureExpData(context.getUser(), context.getContainer(), toFileForWrite(file)); - } - } + FileSystemAuditProvider.FileSystemAuditEvent event = new FileSystemAuditProvider.FileSystemAuditEvent(context.getContainer(), allowMultiple ? "File field provided for assay import" : "Primary file provided for assay import"); + event.setProvidedFileName(fileName); + event.setFile(file.getName()); + event.setDirectory(dir.toURI().getPath()); + auditEvents.add(event); } - } - // now process overflow files, if any - for (String unusedParameterName : unusedParameterNames) - { - if (overflowFiles.isEmpty()) - break; // we're done - else + if (!isAfterFirstFile) // first file gets stored with multipartFile's name { - files.put(unusedParameterName, overflowFiles.remove()); + String name = multipartFile.getName(); + String param = encodedParameterNames.get(name); + if (param == null && parameterNames.contains(name)) + param = name; + if (param == null) + throw new ExperimentException("No parameter name found for multipart file '" + name + "'"); + + files.put(param, file); + isAfterFirstFile = true; + unusedParameterNames.remove(param); } + else // other files get stored in leftover keys later to store only one file per key (bit of a hack) + { + overflowFiles.add(file); + } + + if (ensureExpData) + AbstractQueryUpdateService.ensureExpData(context.getUser(), context.getContainer(), toFileForWrite(file)); } + } + + if (!auditEvents.isEmpty()) + AuditLogService.get().addEvents(context.getUser(), auditEvents); - if (!overflowFiles.isEmpty() && !allowMultiple) // too many files; shouldn't happen, but if it does, throw an error - throw new ExperimentException("Tried to save too many files: number of keys is " + parameterNames.size() + - ", but " + overflowFiles.size() + " extra file(s) were found."); + // now process overflow files, if any + for (String unusedParameterName : unusedParameterNames) + { + if (overflowFiles.isEmpty()) + break; // we're done + + files.put(unusedParameterName, overflowFiles.remove()); } + + if (!overflowFiles.isEmpty() && !allowMultiple) // too many files; shouldn't happen, but if it does, throw an error + throw new ExperimentException("Tried to save too many files: number of keys is " + parameterNames.size() + + ", but " + overflowFiles.size() + " extra file(s) were found."); + return files; } diff --git a/api/src/org/labkey/api/data/TableViewForm.java b/api/src/org/labkey/api/data/TableViewForm.java index e8f073b79fe..a8ab49ac55b 100644 --- a/api/src/org/labkey/api/data/TableViewForm.java +++ b/api/src/org/labkey/api/data/TableViewForm.java @@ -31,6 +31,7 @@ import org.labkey.api.action.NullSafeBindException; import org.labkey.api.action.SpringActionController; import org.labkey.api.collections.CaseInsensitiveHashMap; +import org.labkey.api.dataiterator.DataIteratorUtil; import org.labkey.api.ontology.Quantity; import org.labkey.api.security.permissions.DeletePermission; import org.labkey.api.security.permissions.InsertPermission; @@ -759,20 +760,64 @@ private String getPropertyCaption(String propName) return null==column ? propName : column.getLabel(); } + // RFC 2616 / RFC 7230: Escape characters inside the field name + private static final char BACKSLASH = '\\'; + private static final String SPECIAL_CHARS = BACKSLASH + "\";=,"; + + public static String getFormFieldNameForColumn(@NotNull String columnName) + { + StringBuilder sb = new StringBuilder(); + for (char c : columnName.toCharArray()) + { + if (SPECIAL_CHARS.indexOf(c) >= 0) + sb.append(BACKSLASH); + sb.append(c); + } + + return sb.toString(); + } + public String getFormFieldName(@NotNull ColumnInfo column) { - return column.getName(); + return getFormFieldNameForColumn(column.getName()); + } + + public static String getMultiPartFormFieldNameForColumn(@NotNull String columnName) + { + return DataIteratorUtil.MatchType.multiPartFormData.getMatchedName(getFormFieldNameForColumn(columnName)); } public String getMultiPartFormFieldName(@NotNull ColumnInfo column) { - return getFormFieldName(column); + return getMultiPartFormFieldNameForColumn(column.getName()); } @Nullable - public ColumnInfo getColumnByFormFieldName(@NotNull String name) + public ColumnInfo getColumnByFormFieldName(@NotNull String fieldName) { - return null == getTable() ? null : getTable().getColumn(name); + if (null == getTable()) + return null; + + StringBuilder sb = new StringBuilder(fieldName.length()); + boolean escaping = false; + for (char c : fieldName.toCharArray()) + { + if (escaping) + { + sb.append(c); + escaping = false; + } + else if (c == BACKSLASH) + escaping = true; + else + sb.append(c); + } + + // Issue 54094: Ensure it works when backslash is the last character + if (escaping) + sb.append(BACKSLASH); + + return getTable().getColumn(sb.toString()); } @Override diff --git a/api/src/org/labkey/api/query/QueryUpdateForm.java b/api/src/org/labkey/api/query/QueryUpdateForm.java index c2585120947..ef3f88d2649 100644 --- a/api/src/org/labkey/api/query/QueryUpdateForm.java +++ b/api/src/org/labkey/api/query/QueryUpdateForm.java @@ -21,7 +21,6 @@ import org.labkey.api.data.ColumnInfo; import org.labkey.api.data.TableInfo; import org.labkey.api.data.TableViewForm; -import org.labkey.api.dataiterator.DataIteratorUtil; import org.labkey.api.view.ViewContext; import org.springframework.validation.BindException; @@ -68,10 +67,6 @@ public QueryUpdateForm(@NotNull TableInfo table, @NotNull ViewContext ctx, @Null } } - // RFC 2616 / RFC 7230: Escape characters inside the field name - private static final char BACKSLASH = '\\'; - private static final String SPECIAL_CHARS = BACKSLASH + "\";=,"; - @Override @Nullable public ColumnInfo getColumnByFormFieldName(@NotNull String fieldName) @@ -81,47 +76,13 @@ public ColumnInfo getColumnByFormFieldName(@NotNull String fieldName) var columnName = _ignorePrefix ? fieldName : fieldName.substring(PREFIX.length()); - StringBuilder sb = new StringBuilder(columnName.length()); - boolean escaping = false; - for (char c : columnName.toCharArray()) - { - if (escaping) - { - sb.append(c); - escaping = false; - } - else if (c == BACKSLASH) - escaping = true; - else - sb.append(c); - } - - // Issue 54094: Ensure it works when backslash is the last character - if (escaping) - sb.append(BACKSLASH); - - return getTable().getColumn(sb.toString()); + return super.getColumnByFormFieldName(columnName); } @Override public String getFormFieldName(@NotNull ColumnInfo column) { - String columnName = column.getName(); - StringBuilder sb = new StringBuilder(); - for (char c : columnName.toCharArray()) - { - if (SPECIAL_CHARS.indexOf(c) >= 0) - sb.append(BACKSLASH); - sb.append(c); - } - - String fieldName = sb.toString(); + String fieldName = super.getFormFieldName(column); return _ignorePrefix ? fieldName : PREFIX + fieldName; } - - @Override - public String getMultiPartFormFieldName(@NotNull ColumnInfo column) - { - return DataIteratorUtil.MatchType.multiPartFormData.getMatchedName(getFormFieldName(column)); - } } From 6fce90bee546cc797f3796c7ac30162752f1b066 Mon Sep 17 00:00:00 2001 From: labkey-nicka Date: Tue, 11 Nov 2025 10:12:14 -0800 Subject: [PATCH 02/10] Escape characters in form field names --- .../api/assay/actions/UploadWizardAction.java | 46 +++++++++---------- .../tests/assay/AssayReimportIndexTest.java | 5 +- 2 files changed, 24 insertions(+), 27 deletions(-) diff --git a/api/src/org/labkey/api/assay/actions/UploadWizardAction.java b/api/src/org/labkey/api/assay/actions/UploadWizardAction.java index ead3f9259c8..e8dc2eeb99e 100644 --- a/api/src/org/labkey/api/assay/actions/UploadWizardAction.java +++ b/api/src/org/labkey/api/assay/actions/UploadWizardAction.java @@ -220,14 +220,12 @@ public ModelAndView getView(FormType form, boolean reshow, BindException errors) return new HtmlView(msg); } //pipe root does not exist - else if (isCloudAndUnsupported(pipeRoot, _protocol)) + else if (isCloudAndUnsupported(pipeRoot)) { return HtmlView.err("The pipeline provider for this assay does not support using cloud-based storage. Please contact your administrator."); } - else - { - return getBatchPropertiesView(form, false, errors); - } + + return getBatchPropertiesView(form, false, errors); } else { @@ -291,7 +289,7 @@ public URLHelper getSuccessURL(FormType form) return handler.getSuccessUrl(form); } - private boolean isCloudAndUnsupported(@NotNull PipeRoot pipeRoot, ExpProtocol protocol) + private boolean isCloudAndUnsupported(@NotNull PipeRoot pipeRoot) { return (pipeRoot.isCloudRoot() && null != _provider && @@ -340,7 +338,7 @@ protected InsertView createInsertView(TableInfo baseTable, String lsidCol, List< parameterValueMap.put(dp.getName(), runProperties.get(dp)); } } - catch(ExperimentException e) + catch (ExperimentException e) { errors.addError(new ObjectError("main", null, null, e.getMessage())); } @@ -378,7 +376,7 @@ protected InsertView createInsertView(TableInfo baseTable, String lsidCol, List< view.setInitialValues(inputNameToValue); } - // issue 19090: add hidden form field for properties with default value that are hidden from insert view + // Issue 19090: add a hidden form field for properties with default value that are hidden from the insert view for (DomainProperty prop : properties) { String inputName = getInputName(prop); @@ -507,11 +505,11 @@ private ModelAndView getBatchPropertiesView(FormType runForm, boolean errorResho } /** - * Decide whether or not to show the batch properties step in the wizard. + * Decide whether to show the batch properties step in the wizard. * @param form the form with posted values * @param batchDomain domain for the batch fields */ - protected boolean showBatchStep(FormType form, Domain batchDomain) throws ServletException + protected boolean showBatchStep(FormType form, Domain batchDomain) { return batchDomain != null && !batchDomain.getProperties().isEmpty(); } @@ -666,12 +664,12 @@ protected ModelAndView getRunPropertiesView(FormType newRunForm, boolean errorRe VBox vbox = new VBox(); - JspView warningsView = new JspView<>("/org/labkey/api/assay/actions/newUploadWarnings.jsp", newRunForm); + JspView> warningsView = new JspView<>("/org/labkey/api/assay/actions/newUploadWarnings.jsp", newRunForm); if (newRunForm.getTransformResult().getWarnings() != null) warningsView.setTitle("Transform Warnings"); vbox.addView(warningsView); - JspView assayPropsView = new JspView<>("/org/labkey/assay/view/newUploadAssayProperties.jsp", newRunForm); + JspView> assayPropsView = new JspView<>("/org/labkey/assay/view/newUploadAssayProperties.jsp", newRunForm); assayPropsView.setTitle("Assay Properties"); vbox.addView(assayPropsView); @@ -687,7 +685,7 @@ protected ModelAndView getRunPropertiesView(FormType newRunForm, boolean errorRe AssayWellExclusionService svc = AssayWellExclusionService.getProvider(_protocol); if (svc != null) { - HttpView exclusionWarning = svc.getAssayReImportWarningView(getContainer(), newRunForm.getReRun()); + HttpView exclusionWarning = svc.getAssayReImportWarningView(getContainer(), newRunForm.getReRun()); if (exclusionWarning != null) { vbox.addView(exclusionWarning); @@ -695,7 +693,7 @@ protected ModelAndView getRunPropertiesView(FormType newRunForm, boolean errorRe } AssayQCService qcService = AssayQCService.getProvider(); - HttpView qcWarning = qcService.getAssayReImportWarningView(getContainer(), newRunForm.getReRun()); + HttpView qcWarning = qcService.getAssayReImportWarningView(getContainer(), newRunForm.getReRun()); if (qcWarning != null) { vbox.addView(qcWarning); @@ -769,9 +767,9 @@ protected void addHiddenRunProperties(FormType newRunForm, InsertView insertView public static String getInputName(DomainProperty property, String disambiguationId) { if (disambiguationId != null) - return disambiguationId + "_" + property.getName(); + return TableViewForm.getMultiPartFormFieldNameForColumn(disambiguationId + "_" + property.getName()); else - return property.getName(); + return TableViewForm.getMultiPartFormFieldNameForColumn(property.getName()); } public static String getInputName(DomainProperty property) @@ -835,14 +833,14 @@ protected void addSampleInputColumns(FormType form, InsertView insertView) } @Override - @Nullable public void addNavTrail(NavTree root) { if (null != _protocol) { - ActionURL helper = PageFlowUtil.urlProvider(AssayUrls.class).getAssayRunsURL(getContainer(), _protocol); - root.addChild("Assay List", PageFlowUtil.urlProvider(AssayUrls.class).getAssayListURL(getContainer())); - root.addChild(_protocol.getName(), PageFlowUtil.urlProvider(AssayUrls.class).getAssayRunsURL(getContainer(), _protocol)); + AssayUrls urlProvider = PageFlowUtil.urlProvider(AssayUrls.class); + ActionURL helper = urlProvider.getAssayRunsURL(getContainer(), _protocol); + root.addChild("Assay List", urlProvider.getAssayListURL(getContainer())); + root.addChild(_protocol.getName(), urlProvider.getAssayRunsURL(getContainer(), _protocol)); String finalChild = "Data Import"; if (_stepDescription != null) { @@ -951,14 +949,14 @@ public void validateStep(FormType form, Errors errors) } @Override - public boolean executeStep(FormType form, BindException errors) throws ServletException, SQLException, ExperimentException + public boolean executeStep(FormType form, BindException errors) { // nothing to handle but need to show the run step handler return false; } @Override - public ModelAndView getNextStep(FormType form, BindException errors) throws ServletException, SQLException, ExperimentException + public ModelAndView getNextStep(FormType form, BindException errors) throws ServletException, ExperimentException { if ((form.isResetDefaultValues() || errors.hasErrors()) && showBatchStep(form, form.getProvider().getBatchDomain(form.getProtocol()))) return getBatchPropertiesView(form, !form.isResetDefaultValues(), errors); @@ -1068,8 +1066,8 @@ public boolean executeStep(FormType form, BindException errors) throws ServletEx { for (ValidationError error : e.getErrors()) { - if (error instanceof PropertyValidationError) - errors.addError(new FieldError("AssayUploadForm", ((PropertyValidationError)error).getProperty(), null, false, + if (error instanceof PropertyValidationError pve) + errors.addError(new FieldError("AssayUploadForm", pve.getProperty(), null, false, new String[]{ERROR_MSG}, new Object[0], error.getMessage() == null ? error.toString() : error.getMessage())); else errors.reject(ERROR_MSG, error.getMessage() == null ? error.toString() : error.getMessage()); diff --git a/assay/test/src/org/labkey/test/tests/assay/AssayReimportIndexTest.java b/assay/test/src/org/labkey/test/tests/assay/AssayReimportIndexTest.java index 95d511e261a..0ca6ee76f09 100644 --- a/assay/test/src/org/labkey/test/tests/assay/AssayReimportIndexTest.java +++ b/assay/test/src/org/labkey/test/tests/assay/AssayReimportIndexTest.java @@ -29,9 +29,8 @@ public class AssayReimportIndexTest extends BaseWebDriverTest { private static final String ASSAY_NAME = DomainKind.Assay.randomName("test+assay"); - // TODO: Replace each of the following with DomainKind.Assay.randomField(, ColumnType.File) once Issue 54218 is fixed. - private static final FieldInfo BATCH_FILE_FIELD = new FieldInfo("batchFile", ColumnType.File); - private static final FieldInfo RUN_FILE_FIELD = new FieldInfo("runFile", ColumnType.File); + private static final FieldInfo BATCH_FILE_FIELD = DomainKind.Assay.randomField("batchFile", ColumnType.File); + private static final FieldInfo RUN_FILE_FIELD = DomainKind.Assay.randomField("runFile", ColumnType.File); @BeforeClass public static void setupProject() throws Exception From fb71290f9985d4bce3cdcc4417dff729fc417f82 Mon Sep 17 00:00:00 2001 From: labkey-nicka Date: Wed, 12 Nov 2025 08:20:47 -0800 Subject: [PATCH 03/10] Incremental step --- .../api/assay/AssayRunDatabaseContext.java | 10 --- .../api/assay/AssayRunUploadContext.java | 9 ++- .../api/assay/actions/AssayRunUploadForm.java | 65 ++++++++++--------- .../api/assay/actions/UploadWizardAction.java | 32 ++++----- .../assay/pipeline/AssayRunAsyncContext.java | 7 -- .../labkey/api/qc/TsvDataExchangeHandler.java | 7 -- .../api/assay/AssayResultUpdateService.java | 6 -- 7 files changed, 56 insertions(+), 80 deletions(-) diff --git a/api/src/org/labkey/api/assay/AssayRunDatabaseContext.java b/api/src/org/labkey/api/assay/AssayRunDatabaseContext.java index f19138d5369..b50a6a38037 100644 --- a/api/src/org/labkey/api/assay/AssayRunDatabaseContext.java +++ b/api/src/org/labkey/api/assay/AssayRunDatabaseContext.java @@ -48,8 +48,6 @@ /** * Information about an assay run that has already been imported into the database. - * User: jeckels - * Date: Oct 7, 2011 */ public class AssayRunDatabaseContext implements AssayRunUploadContext { @@ -195,14 +193,6 @@ public FileLike getOriginalFileLocation() return null; } - @NotNull - @Override - public Map getInputDatas() - { - // CONSIDER: get the run's input datas - return Collections.emptyMap(); - } - @Override public ProviderType getProvider() { diff --git a/api/src/org/labkey/api/assay/AssayRunUploadContext.java b/api/src/org/labkey/api/assay/AssayRunUploadContext.java index 4b69318e848..828d185fc69 100644 --- a/api/src/org/labkey/api/assay/AssayRunUploadContext.java +++ b/api/src/org/labkey/api/assay/AssayRunUploadContext.java @@ -36,7 +36,6 @@ import org.labkey.api.writer.ContainerUser; import org.labkey.vfs.FileLike; -import java.io.File; import java.util.Map; import java.util.Set; @@ -113,7 +112,10 @@ default DataIteratorBuilder getRawData() * NOTE: These files will not be parsed or imported by the assay's DataHandler -- use {@link #getUploadedData()} instead. */ @NotNull - Map getInputDatas(); + default Map getInputDatas() + { + return emptyMap(); + } @NotNull default Map getOutputDatas() @@ -181,16 +183,19 @@ default ReImportOption getReImportOption() void uploadComplete(ExpRun run) throws ExperimentException; + @Nullable default String getJobDescription() { return null; } + @Nullable default String getJobNotificationProvider() { return null; } + @Nullable default String getPipelineJobGUID() { return null; diff --git a/api/src/org/labkey/api/assay/actions/AssayRunUploadForm.java b/api/src/org/labkey/api/assay/actions/AssayRunUploadForm.java index d230f3ecf73..261be069dcc 100644 --- a/api/src/org/labkey/api/assay/actions/AssayRunUploadForm.java +++ b/api/src/org/labkey/api/assay/actions/AssayRunUploadForm.java @@ -83,6 +83,7 @@ import java.util.TreeMap; import static java.util.Collections.emptyMap; +import static java.util.Collections.unmodifiableMap; public class AssayRunUploadForm extends ProtocolIdForm implements AssayRunUploadContext { @@ -99,7 +100,7 @@ public class AssayRunUploadForm extends Prot private Map _uploadedData; private boolean _successfulUploadComplete; private String _uploadAttemptID = GUID.makeGUID(); - private Map _additionalFiles; + private Map> _additionalFiles; private Long _batchId; private Long _reRunId; private String _severityLevel; @@ -121,25 +122,25 @@ public List getRunDataProperties() public Map getRunProperties() throws ExperimentException { if (_runProperties == null) - _runProperties = getPropertyMapFromRequest(getProvider().getRunDomain(getProtocol())); + _runProperties = Collections.unmodifiableMap(getPropertyMapFromRequest(getProvider().getRunDomain(getProtocol()))); - return Collections.unmodifiableMap(_runProperties); + return _runProperties; } @Override public Map getBatchProperties() throws ExperimentException { if (_batchProperties == null) - _batchProperties = getPropertyMapFromRequest(getProvider().getBatchDomain(getProtocol())); + _batchProperties = Collections.unmodifiableMap(getPropertyMapFromRequest(getProvider().getBatchDomain(getProtocol()))); - return Collections.unmodifiableMap(_batchProperties); + return _batchProperties; } protected Map getPropertyMapFromRequest(Domain domain) throws ExperimentException { List columns = domain.getProperties(); Map properties = new LinkedHashMap<>(); - Map additionalFiles = getAdditionalPostedFiles(columns); + Map additionalFiles = getAdditionalPostedFiles(domain, columns); Map defaults = getDefaultValues(domain, false); for (DomainProperty pd : columns) @@ -282,7 +283,7 @@ public AssayDataCollector getSelectedDataCollector() public void init() throws ExperimentException { AssayDataCollector collector = getSelectedDataCollector(); - if(null != collector) + if (null != collector) { collector.initDir(this); } @@ -312,9 +313,17 @@ public Map getUploadedData() throws ExperimentException return _uploadedData; } - public Map getAdditionalFiles() + public Map getAdditionalFiles(@NotNull Domain domain) { - return _additionalFiles; + return _additionalFiles.get(domain.getTypeURI()); + } + + private Map setAdditionalFilesEntry(@NotNull Domain domain, Map files) + { + var _files = unmodifiableMap(files); + _additionalFiles.put(domain.getTypeURI(), _files); + + return _files; } public static File getAssayDirectory(Container c, File root) @@ -325,10 +334,13 @@ public static File getAssayDirectory(Container c, File root) return new File(PipelineService.get().findPipelineRoot(c).getRootPath().getAbsolutePath(), AssayFileWriter.DIR_NAME); } - public Map getAdditionalPostedFiles(List pds) throws ExperimentException + private Map getAdditionalPostedFiles(Domain domain, List pds) throws ExperimentException { - if (_additionalFiles != null) - return _additionalFiles; + if (_additionalFiles == null) + _additionalFiles = new HashMap<>(); + + if (_additionalFiles.containsKey(domain.getTypeURI())) + return _additionalFiles.get(domain.getTypeURI()); Map fileParameters = new TreeMap<>(String.CASE_INSENSITIVE_ORDER); List filePdNames = new ArrayList<>(); @@ -343,32 +355,34 @@ public Map getAdditionalPostedFiles(List> writer = new AssayFileWriter<>(); + Map additionalFiles = new HashMap<>(); + try { // Initialize member variable so we know that we've already tried to save the posted files in case of error - _additionalFiles = new HashMap<>(); Map postedFiles = writer.savePostedFiles(this, fileParameters.keySet(), false, false); for (Map.Entry entry : postedFiles.entrySet()) - _additionalFiles.put(fileParameters.get(entry.getKey()), entry.getValue()); + additionalFiles.put(fileParameters.get(entry.getKey()), entry.getValue()); if (!(getViewContext().getRequest() instanceof MultipartHttpServletRequest request)) - return _additionalFiles; + return setAdditionalFilesEntry(domain, additionalFiles); File assayDirectory = getAssayDirectory(getContainer(), null); + Map requestFiles = request.getFileMap(); // Hidden values in form containing previously uploaded files if the previous upload resulted in error for (String fileParam : filePdNames) { DomainProperty domainProperty = fileParameters.get(fileParam); - MultipartFile multiFile = request.getFileMap().get(fileParam); + MultipartFile multiFile = requestFiles.get(fileParam); // If the file is removed from form after error, override hidden file name with an empty file - if (null != multiFile && multiFile.getOriginalFilename().isEmpty()) + if (null != multiFile && StringUtils.trimToNull(multiFile.getOriginalFilename()) == null) { - _additionalFiles.put(domainProperty, BLANK_FILE); + additionalFiles.put(domainProperty, BLANK_FILE); continue; } @@ -379,8 +393,8 @@ public Map getAdditionalPostedFiles(List getAdditionalPostedFiles(List getInputDatas() - { - return emptyMap(); + return setAdditionalFilesEntry(domain, additionalFiles); } @Override diff --git a/api/src/org/labkey/api/assay/actions/UploadWizardAction.java b/api/src/org/labkey/api/assay/actions/UploadWizardAction.java index e8dc2eeb99e..b55d6ffd3c2 100644 --- a/api/src/org/labkey/api/assay/actions/UploadWizardAction.java +++ b/api/src/org/labkey/api/assay/actions/UploadWizardAction.java @@ -95,6 +95,7 @@ import org.labkey.api.view.template.ClientDependency; import org.labkey.api.writer.ContainerUser; import org.labkey.api.writer.HtmlWriter; +import org.labkey.vfs.FileLike; import org.springframework.context.MessageSourceResolvable; import org.springframework.validation.BindException; import org.springframework.validation.Errors; @@ -127,9 +128,7 @@ public class UploadWizardAction> _stepHandlers = new HashMap<>(); - protected String _stepDescription; public UploadWizardAction() @@ -404,13 +403,14 @@ protected InsertView createInsertView(TableInfo baseTable, String lsidCol, List< view.getDataRegion().addHiddenFormField("uploadAttemptID", form.getUploadAttemptID()); if (form.isAllowCrossRunFileInputs()) view.getDataRegion().addHiddenFormField("allowCrossRunFileInputs", "true"); - if (errorReshow) + if (errorReshow && domain != null) { // Add unique name of uploaded files as hidden parameters - for (DomainProperty dp : form.getAdditionalFiles().keySet()) - { - view.getDataRegion().addHiddenFormField(dp.getName(), form.getAdditionalFiles().get(dp).getName()); - } + Map fileProps = new HashMap<>(); + for (Map.Entry entry : form.getAdditionalFiles(domain).entrySet()) + fileProps.put(entry.getKey(), entry.getValue().getName()); + + addHiddenProperties(fileProps, view); } if (form.getReRunId() != null) { @@ -457,7 +457,7 @@ private void decodePropertyValues(Map inputNameToValue, String p inputNameToValue.put(propName, propValue); } - private ModelAndView getBatchPropertiesView(FormType runForm, boolean errorReshow, BindException errors) throws ServletException, ExperimentException + private ModelAndView getBatchPropertiesView(FormType runForm, boolean errorReshow, BindException errors) throws ExperimentException { // Check if the user is trying to replace a run that's already been replaced if (runForm.getReRun() != null) @@ -764,7 +764,7 @@ protected void addHiddenRunProperties(FormType newRunForm, InsertView insertView addHiddenProperties(newRunForm.getRunProperties(), insertView); } - public static String getInputName(DomainProperty property, String disambiguationId) + public static String getInputName(DomainProperty property, @Nullable String disambiguationId) { if (disambiguationId != null) return TableViewForm.getMultiPartFormFieldNameForColumn(disambiguationId + "_" + property.getName()); @@ -779,21 +779,15 @@ public static String getInputName(DomainProperty property) protected void addHiddenProperties(Map properties, InsertView insertView) { - for (Map.Entry entry : properties.entrySet()) - { - String name = entry.getKey().getName(); - String value = entry.getValue(); - insertView.getDataRegion().addHiddenFormField(name, value); - } + addHiddenProperties(properties, insertView, null); } - protected void addHiddenProperties(Map properties, InsertView insertView, String disambiguationId) + protected void addHiddenProperties(Map properties, InsertView insertView, @Nullable String disambiguationId) { for (Map.Entry entry : properties.entrySet()) { String name = getInputName(entry.getKey(), disambiguationId); - String value = entry.getValue(); - insertView.getDataRegion().addHiddenFormField(name, value); + insertView.getDataRegion().addHiddenFormField(name, entry.getValue()); } } @@ -956,7 +950,7 @@ public boolean executeStep(FormType form, BindException errors) } @Override - public ModelAndView getNextStep(FormType form, BindException errors) throws ServletException, ExperimentException + public ModelAndView getNextStep(FormType form, BindException errors) throws ExperimentException { if ((form.isResetDefaultValues() || errors.hasErrors()) && showBatchStep(form, form.getProvider().getBatchDomain(form.getProtocol()))) return getBatchPropertiesView(form, !form.isResetDefaultValues(), errors); diff --git a/api/src/org/labkey/api/assay/pipeline/AssayRunAsyncContext.java b/api/src/org/labkey/api/assay/pipeline/AssayRunAsyncContext.java index e8b97177c41..b3bedbb71ff 100644 --- a/api/src/org/labkey/api/assay/pipeline/AssayRunAsyncContext.java +++ b/api/src/org/labkey/api/assay/pipeline/AssayRunAsyncContext.java @@ -343,13 +343,6 @@ public Map getUploadedData() return _uploadedData; } - @NotNull - @Override - public Map getInputDatas() - { - return Collections.emptyMap(); - } - @Override public boolean isAllowCrossRunFileInputs() { diff --git a/api/src/org/labkey/api/qc/TsvDataExchangeHandler.java b/api/src/org/labkey/api/qc/TsvDataExchangeHandler.java index 552d468f729..2d9166cf774 100644 --- a/api/src/org/labkey/api/qc/TsvDataExchangeHandler.java +++ b/api/src/org/labkey/api/qc/TsvDataExchangeHandler.java @@ -1351,13 +1351,6 @@ public Map getUploadedData() return emptyMap(); } - @NotNull - @Override - public Map getInputDatas() - { - return emptyMap(); - } - @Override public AssayProvider getProvider() { diff --git a/assay/api-src/org/labkey/api/assay/AssayResultUpdateService.java b/assay/api-src/org/labkey/api/assay/AssayResultUpdateService.java index c04afcebd78..e894caacbfe 100644 --- a/assay/api-src/org/labkey/api/assay/AssayResultUpdateService.java +++ b/assay/api-src/org/labkey/api/assay/AssayResultUpdateService.java @@ -477,12 +477,6 @@ public ActionURL getActionURL() return _data; } - @Override - public @NotNull Map getInputDatas() - { - return Collections.emptyMap(); - } - @Override public AssayProvider getProvider() { From 18fc5d1218aa11a21c251c6f601c573ca572a822 Mon Sep 17 00:00:00 2001 From: labkey-nicka Date: Wed, 12 Nov 2025 10:33:11 -0800 Subject: [PATCH 04/10] Pass through files --- .../api/assay/actions/AssayRunUploadForm.java | 28 +++++++++++-------- .../api/assay/actions/UploadWizardAction.java | 2 +- .../api/data/AbstractFileDisplayColumn.java | 6 ++-- .../org/labkey/api/data/TableViewForm.java | 15 ++-------- 4 files changed, 23 insertions(+), 28 deletions(-) diff --git a/api/src/org/labkey/api/assay/actions/AssayRunUploadForm.java b/api/src/org/labkey/api/assay/actions/AssayRunUploadForm.java index 261be069dcc..17678583ba2 100644 --- a/api/src/org/labkey/api/assay/actions/AssayRunUploadForm.java +++ b/api/src/org/labkey/api/assay/actions/AssayRunUploadForm.java @@ -72,7 +72,7 @@ import java.io.File; import java.io.IOException; -import java.util.ArrayList; +import java.nio.file.Path; import java.util.Collections; import java.util.HashMap; import java.util.HashSet; @@ -343,15 +343,11 @@ private Map getAdditionalPostedFiles(Domain domain, Li return _additionalFiles.get(domain.getTypeURI()); Map fileParameters = new TreeMap<>(String.CASE_INSENSITIVE_ORDER); - List filePdNames = new ArrayList<>(); for (DomainProperty pd : pds) { if (pd.getPropertyDescriptor().getPropertyType() == PropertyType.FILE_LINK) - { fileParameters.put(UploadWizardAction.getInputName(pd), pd); - filePdNames.add(pd.getName()); - } } if (fileParameters.isEmpty()) @@ -374,9 +370,10 @@ private Map getAdditionalPostedFiles(Domain domain, Li Map requestFiles = request.getFileMap(); // Hidden values in form containing previously uploaded files if the previous upload resulted in error - for (String fileParam : filePdNames) + for (Map.Entry entry : fileParameters.entrySet()) { - DomainProperty domainProperty = fileParameters.get(fileParam); + String fileParam = entry.getKey(); + DomainProperty domainProperty = entry.getValue(); MultipartFile multiFile = requestFiles.get(fileParam); // If the file is removed from form after error, override hidden file name with an empty file @@ -386,15 +383,22 @@ private Map getAdditionalPostedFiles(Domain domain, Li continue; } - String previousFileName = request.getParameter(fileParam); - if (previousFileName == null) + if (additionalFiles.containsKey(domainProperty)) + continue; + + String previousFilePath = StringUtils.trimToNull(request.getParameter(fileParam)); + if (previousFilePath == null) continue; // Only add a hidden file parameter if it is a valid file in the pipeline root directory and // a new file hasn't been uploaded for that parameter - File previousFile = FileUtil.appendName(assayDirectory, previousFileName); - if (previousFile.isFile() && FileUtils.directoryContains(assayDirectory, previousFile) && !additionalFiles.containsKey(domainProperty)) - additionalFiles.put(domainProperty, FileSystemLike.wrapFile(previousFile)); + Path path = FileUtil.stringToPath(getContainer(), previousFilePath, false); + if (FileUtil.isFileAndExists(path)) + { + File previousFile = path.toFile(); + if (previousFile.isFile() && FileUtils.directoryContains(assayDirectory, previousFile)) + additionalFiles.put(domainProperty, FileSystemLike.wrapFile(previousFile)); + } } } catch (IOException e) diff --git a/api/src/org/labkey/api/assay/actions/UploadWizardAction.java b/api/src/org/labkey/api/assay/actions/UploadWizardAction.java index b55d6ffd3c2..31a500a06ef 100644 --- a/api/src/org/labkey/api/assay/actions/UploadWizardAction.java +++ b/api/src/org/labkey/api/assay/actions/UploadWizardAction.java @@ -408,7 +408,7 @@ protected InsertView createInsertView(TableInfo baseTable, String lsidCol, List< // Add unique name of uploaded files as hidden parameters Map fileProps = new HashMap<>(); for (Map.Entry entry : form.getAdditionalFiles(domain).entrySet()) - fileProps.put(entry.getKey(), entry.getValue().getName()); + fileProps.put(entry.getKey(), entry.getValue().toString()); addHiddenProperties(fileProps, view); } diff --git a/api/src/org/labkey/api/data/AbstractFileDisplayColumn.java b/api/src/org/labkey/api/data/AbstractFileDisplayColumn.java index 9d30bd91a60..a37f801bbf6 100644 --- a/api/src/org/labkey/api/data/AbstractFileDisplayColumn.java +++ b/api/src/org/labkey/api/data/AbstractFileDisplayColumn.java @@ -286,7 +286,7 @@ public void renderInputHtml(RenderContext ctx, HtmlWriter out, Object value) if (null != filename) { // Existing value, so tell the user the file name, allow the file to be removed, and a new file uploaded - renderThumbnailAndRemoveLink(out, ctx, formFieldName, filename, input); + renderThumbnailAndRemoveLink(out, ctx, formFieldName, filename, value, input); } else { @@ -307,7 +307,7 @@ protected String getRemovalWarningText(String filename) return "Previous file " + filename + " will be removed."; } - private void renderThumbnailAndRemoveLink(HtmlWriter out, RenderContext ctx, String fieldName, String filename, InputBuilder filePicker) + private void renderThumbnailAndRemoveLink(HtmlWriter out, RenderContext ctx, String fieldName, String filename, Object value, InputBuilder filePicker) { String divId = GUID.makeGUID(); String linkId = "remove" + divId; @@ -317,7 +317,7 @@ private void renderThumbnailAndRemoveLink(HtmlWriter out, RenderContext ctx, Str .data("fieldName", fieldName) .cl("lk-remove-file"), (Renderable) ret -> { - renderIconAndFilename(ctx, out, filename, false, false); + renderIconAndFilename(ctx, out, value instanceof String s ? s : filename, false, false); out.write(HtmlString.NBSP); out.write("["); out.write(LinkBuilder.simpleLink("remove", "#").id(linkId)); diff --git a/api/src/org/labkey/api/data/TableViewForm.java b/api/src/org/labkey/api/data/TableViewForm.java index a8ab49ac55b..7ad8881a193 100644 --- a/api/src/org/labkey/api/data/TableViewForm.java +++ b/api/src/org/labkey/api/data/TableViewForm.java @@ -591,7 +591,7 @@ else if (includeUntyped && _stringValues.containsKey(getFormFieldName(column))) } else if (getRequest() instanceof MultipartHttpServletRequest request) { - String fieldName = getMultiPartFormFieldName(column); + String fieldName = getFormFieldName(column); Object typedValue = _getTypedValues().get(fieldName); if (typedValue != null) @@ -707,7 +707,6 @@ else if (value instanceof Collection col && col.stream().allMatch(e -> null== _values = null; } - public void validateBind(BindException errors) { populateValues(errors); @@ -718,13 +717,11 @@ public Object getOldValues() return _oldValues; } - public void setOldValues(Object oldValues) { _oldValues = oldValues; } - public void forceReselect() { Object[] pk = getPkVals(); @@ -734,7 +731,6 @@ public void forceReselect() setDataLoaded(false); } - protected Class getTruePropType(String propName) { ColumnInfo column = getColumnByFormFieldName(propName); @@ -764,7 +760,7 @@ private String getPropertyCaption(String propName) private static final char BACKSLASH = '\\'; private static final String SPECIAL_CHARS = BACKSLASH + "\";=,"; - public static String getFormFieldNameForColumn(@NotNull String columnName) + private static String getFormFieldNameForColumn(@NotNull String columnName) { StringBuilder sb = new StringBuilder(); for (char c : columnName.toCharArray()) @@ -779,7 +775,7 @@ public static String getFormFieldNameForColumn(@NotNull String columnName) public String getFormFieldName(@NotNull ColumnInfo column) { - return getFormFieldNameForColumn(column.getName()); + return DataIteratorUtil.MatchType.multiPartFormData.getMatchedName(getFormFieldNameForColumn(column.getName())); } public static String getMultiPartFormFieldNameForColumn(@NotNull String columnName) @@ -787,11 +783,6 @@ public static String getMultiPartFormFieldNameForColumn(@NotNull String columnNa return DataIteratorUtil.MatchType.multiPartFormData.getMatchedName(getFormFieldNameForColumn(columnName)); } - public String getMultiPartFormFieldName(@NotNull ColumnInfo column) - { - return getMultiPartFormFieldNameForColumn(column.getName()); - } - @Nullable public ColumnInfo getColumnByFormFieldName(@NotNull String fieldName) { From 44e4761614a73f3571ad7690226255b802f6b86b Mon Sep 17 00:00:00 2001 From: labkey-nicka Date: Wed, 12 Nov 2025 13:10:10 -0800 Subject: [PATCH 05/10] Distinguish between multipart requests --- .../api/assay/actions/AssayRunUploadForm.java | 67 +++++++++++-------- .../api/assay/actions/UploadWizardAction.java | 9 ++- .../org/labkey/api/data/TableViewForm.java | 11 ++- 3 files changed, 54 insertions(+), 33 deletions(-) diff --git a/api/src/org/labkey/api/assay/actions/AssayRunUploadForm.java b/api/src/org/labkey/api/assay/actions/AssayRunUploadForm.java index 17678583ba2..60879a32fce 100644 --- a/api/src/org/labkey/api/assay/actions/AssayRunUploadForm.java +++ b/api/src/org/labkey/api/assay/actions/AssayRunUploadForm.java @@ -21,6 +21,7 @@ import org.apache.commons.io.FileUtils; import org.apache.commons.lang3.StringUtils; import org.apache.logging.log4j.Logger; +import org.apache.logging.log4j.util.Strings; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; import org.labkey.api.assay.AbstractAssayProvider; @@ -122,7 +123,7 @@ public List getRunDataProperties() public Map getRunProperties() throws ExperimentException { if (_runProperties == null) - _runProperties = Collections.unmodifiableMap(getPropertyMapFromRequest(getProvider().getRunDomain(getProtocol()))); + _runProperties = Collections.unmodifiableMap(getPropertyMapFromRequest(getProvider().getRunDomain(getProtocol()), true)); return _runProperties; } @@ -131,52 +132,61 @@ public Map getRunProperties() throws ExperimentException public Map getBatchProperties() throws ExperimentException { if (_batchProperties == null) - _batchProperties = Collections.unmodifiableMap(getPropertyMapFromRequest(getProvider().getBatchDomain(getProtocol()))); + _batchProperties = Collections.unmodifiableMap(getPropertyMapFromRequest(getProvider().getBatchDomain(getProtocol()), false)); return _batchProperties; } - protected Map getPropertyMapFromRequest(Domain domain) throws ExperimentException + protected Map getPropertyMapFromRequest(Domain domain, boolean isRunDomain) throws ExperimentException { List columns = domain.getProperties(); Map properties = new LinkedHashMap<>(); Map additionalFiles = getAdditionalPostedFiles(domain, columns); Map defaults = getDefaultValues(domain, false); + boolean isMultiPartRequest = getRequest() instanceof MultipartHttpServletRequest; for (DomainProperty pd : columns) { - String propName = UploadWizardAction.getInputName(pd); - String value = StringUtils.trimToNull(getRequest().getParameter(propName)); - if (pd.getPropertyDescriptor().getPropertyType() == PropertyType.BOOLEAN && (value == null || value.isEmpty())) + String propName = isMultiPartRequest ? UploadWizardAction.getMultiPartInputName(pd) : UploadWizardAction.getInputName(pd); + String rawValue = getRequest().getParameter(propName); + String value = StringUtils.trimToNull(rawValue); + + if (PropertyType.BOOLEAN == pd.getPropertyDescriptor().getPropertyType() && value == null) value = Boolean.FALSE.toString(); if (AbstractAssayProvider.PARTICIPANT_VISIT_RESOLVER_PROPERTY_NAME.equalsIgnoreCase(pd.getName()) && value != null) value = ParticipantVisitResolverType.Serializer.encode(value, getRequest()); - if (additionalFiles.containsKey(pd)) - { - if (BLANK_FILE.equals(additionalFiles.get(pd))) // file has been removed - properties.put(pd, null); - else - properties.put(pd, additionalFiles.get(pd).toNioPathForRead().toString()); - } - else - properties.put(pd, value); - - // Issue 54112: Retain previous file values when reimporting a run if (PropertyType.FILE_LINK == pd.getPropertyDescriptor().getPropertyType()) { - boolean postedNewFile = additionalFiles.containsKey(pd); - String current = properties.get(pd); - boolean hasValue = current != null && !current.isEmpty(); - - if (!postedNewFile && !hasValue) + if (additionalFiles.containsKey(pd)) { + // The file was explicitly removed + if (BLANK_FILE.equals(additionalFiles.get(pd))) + { + // When this is a batch domain we set to Strings.EMPTY to + // allow round-tripping removed values through the run step. + properties.put(pd, isRunDomain ? null : Strings.EMPTY); + } + else + properties.put(pd, additionalFiles.get(pd).toNioPathForRead().toString()); + } + else if (Strings.EMPTY.equals(rawValue)) + { + // The file was previously removed + properties.put(pd, null); + } + else if (StringUtils.trimToNull(properties.get(pd)) == null) + { + // Issue 54112: Retain previous file values when reimporting a run Object prior = defaults.get(pd); if (prior != null) properties.put(pd, prior.toString()); } } + + if (!properties.containsKey(pd)) + properties.put(pd, value); } return properties; @@ -339,15 +349,20 @@ private Map getAdditionalPostedFiles(Domain domain, Li if (_additionalFiles == null) _additionalFiles = new HashMap<>(); + // Additional files have already been computed for this domain if (_additionalFiles.containsKey(domain.getTypeURI())) return _additionalFiles.get(domain.getTypeURI()); + // This is not a multipart request, so no files have been posted + if (!(getRequest() instanceof MultipartHttpServletRequest request)) + return setAdditionalFilesEntry(domain, emptyMap()); + Map fileParameters = new TreeMap<>(String.CASE_INSENSITIVE_ORDER); for (DomainProperty pd : pds) { if (pd.getPropertyDescriptor().getPropertyType() == PropertyType.FILE_LINK) - fileParameters.put(UploadWizardAction.getInputName(pd), pd); + fileParameters.put(pd.getName(), pd); } if (fileParameters.isEmpty()) @@ -358,21 +373,17 @@ private Map getAdditionalPostedFiles(Domain domain, Li try { - // Initialize member variable so we know that we've already tried to save the posted files in case of error Map postedFiles = writer.savePostedFiles(this, fileParameters.keySet(), false, false); for (Map.Entry entry : postedFiles.entrySet()) additionalFiles.put(fileParameters.get(entry.getKey()), entry.getValue()); - if (!(getViewContext().getRequest() instanceof MultipartHttpServletRequest request)) - return setAdditionalFilesEntry(domain, additionalFiles); - File assayDirectory = getAssayDirectory(getContainer(), null); Map requestFiles = request.getFileMap(); // Hidden values in form containing previously uploaded files if the previous upload resulted in error for (Map.Entry entry : fileParameters.entrySet()) { - String fileParam = entry.getKey(); + String fileParam = UploadWizardAction.getMultiPartInputName(entry.getValue()); DomainProperty domainProperty = entry.getValue(); MultipartFile multiFile = requestFiles.get(fileParam); diff --git a/api/src/org/labkey/api/assay/actions/UploadWizardAction.java b/api/src/org/labkey/api/assay/actions/UploadWizardAction.java index 31a500a06ef..539ca23151a 100644 --- a/api/src/org/labkey/api/assay/actions/UploadWizardAction.java +++ b/api/src/org/labkey/api/assay/actions/UploadWizardAction.java @@ -767,9 +767,14 @@ protected void addHiddenRunProperties(FormType newRunForm, InsertView insertView public static String getInputName(DomainProperty property, @Nullable String disambiguationId) { if (disambiguationId != null) - return TableViewForm.getMultiPartFormFieldNameForColumn(disambiguationId + "_" + property.getName()); + return TableViewForm.getFormFieldNameForColumn(disambiguationId + "_" + property.getName()); else - return TableViewForm.getMultiPartFormFieldNameForColumn(property.getName()); + return TableViewForm.getFormFieldNameForColumn(property.getName()); + } + + public static String getMultiPartInputName(DomainProperty property) + { + return TableViewForm.getMultiPartFormFieldNameForColumn(property.getName()); } public static String getInputName(DomainProperty property) diff --git a/api/src/org/labkey/api/data/TableViewForm.java b/api/src/org/labkey/api/data/TableViewForm.java index 7ad8881a193..228b0d9ac66 100644 --- a/api/src/org/labkey/api/data/TableViewForm.java +++ b/api/src/org/labkey/api/data/TableViewForm.java @@ -591,7 +591,7 @@ else if (includeUntyped && _stringValues.containsKey(getFormFieldName(column))) } else if (getRequest() instanceof MultipartHttpServletRequest request) { - String fieldName = getFormFieldName(column); + String fieldName = getMultiPartFormFieldName(column); Object typedValue = _getTypedValues().get(fieldName); if (typedValue != null) @@ -760,7 +760,7 @@ private String getPropertyCaption(String propName) private static final char BACKSLASH = '\\'; private static final String SPECIAL_CHARS = BACKSLASH + "\";=,"; - private static String getFormFieldNameForColumn(@NotNull String columnName) + public static String getFormFieldNameForColumn(@NotNull String columnName) { StringBuilder sb = new StringBuilder(); for (char c : columnName.toCharArray()) @@ -775,7 +775,7 @@ private static String getFormFieldNameForColumn(@NotNull String columnName) public String getFormFieldName(@NotNull ColumnInfo column) { - return DataIteratorUtil.MatchType.multiPartFormData.getMatchedName(getFormFieldNameForColumn(column.getName())); + return getFormFieldNameForColumn(column.getName()); } public static String getMultiPartFormFieldNameForColumn(@NotNull String columnName) @@ -783,6 +783,11 @@ public static String getMultiPartFormFieldNameForColumn(@NotNull String columnNa return DataIteratorUtil.MatchType.multiPartFormData.getMatchedName(getFormFieldNameForColumn(columnName)); } + public String getMultiPartFormFieldName(@NotNull ColumnInfo column) + { + return getMultiPartFormFieldNameForColumn(column.getName()); + } + @Nullable public ColumnInfo getColumnByFormFieldName(@NotNull String fieldName) { From ec3138fcf70d07fdeb40b244e21e450b1aa9e0f6 Mon Sep 17 00:00:00 2001 From: labkey-nicka Date: Wed, 12 Nov 2025 13:10:17 -0800 Subject: [PATCH 06/10] Test updates --- api/src/org/labkey/api/data/AbstractFileDisplayColumn.java | 6 +++--- .../org/labkey/test/tests/assay/AssayReimportIndexTest.java | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/api/src/org/labkey/api/data/AbstractFileDisplayColumn.java b/api/src/org/labkey/api/data/AbstractFileDisplayColumn.java index a37f801bbf6..464a54051b5 100644 --- a/api/src/org/labkey/api/data/AbstractFileDisplayColumn.java +++ b/api/src/org/labkey/api/data/AbstractFileDisplayColumn.java @@ -286,7 +286,7 @@ public void renderInputHtml(RenderContext ctx, HtmlWriter out, Object value) if (null != filename) { // Existing value, so tell the user the file name, allow the file to be removed, and a new file uploaded - renderThumbnailAndRemoveLink(out, ctx, formFieldName, filename, value, input); + renderThumbnailAndRemoveLink(out, ctx, getBoundColumn(), filename, value, input); } else { @@ -307,14 +307,14 @@ protected String getRemovalWarningText(String filename) return "Previous file " + filename + " will be removed."; } - private void renderThumbnailAndRemoveLink(HtmlWriter out, RenderContext ctx, String fieldName, String filename, Object value, InputBuilder filePicker) + private void renderThumbnailAndRemoveLink(HtmlWriter out, RenderContext ctx, ColumnInfo column, String filename, Object value, InputBuilder filePicker) { String divId = GUID.makeGUID(); String linkId = "remove" + divId; DIV( id(divId) - .data("fieldName", fieldName) + .data("fieldName", column.getName()) .cl("lk-remove-file"), (Renderable) ret -> { renderIconAndFilename(ctx, out, value instanceof String s ? s : filename, false, false); diff --git a/assay/test/src/org/labkey/test/tests/assay/AssayReimportIndexTest.java b/assay/test/src/org/labkey/test/tests/assay/AssayReimportIndexTest.java index 0ca6ee76f09..de3c4fc4828 100644 --- a/assay/test/src/org/labkey/test/tests/assay/AssayReimportIndexTest.java +++ b/assay/test/src/org/labkey/test/tests/assay/AssayReimportIndexTest.java @@ -166,8 +166,8 @@ public void testFileFieldValuesRetainedRunReimport() var row = new AssayRunsPage(getDriver()) .getTable() .getRowDataAsMap("Name", runName); - reimportBatchFileValue = StringUtils.trimToNull(row.get("Batch/" + BATCH_FILE_FIELD.getName())); - reimportRunFileValue = StringUtils.trimToNull(row.get(RUN_FILE_FIELD.getName())); + reimportBatchFileValue = StringUtils.trimToNull(row.get("Batch/" + BATCH_FILE_FIELD.getFieldKey())); + reimportRunFileValue = StringUtils.trimToNull(row.get(RUN_FILE_FIELD.getFieldKey().toString())); checker().verifyEquals("Expected batch file to appear in data", batchFile.getName(), reimportBatchFileValue); checker().verifyEquals("Expected run file to appear in data", runFile.getName(), reimportRunFileValue); From a37df833c91ed23fa48155ce779d7234e7c7e351 Mon Sep 17 00:00:00 2001 From: labkey-nicka Date: Wed, 12 Nov 2025 13:54:31 -0800 Subject: [PATCH 07/10] Fix overload --- api/src/org/labkey/api/data/TableViewForm.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/src/org/labkey/api/data/TableViewForm.java b/api/src/org/labkey/api/data/TableViewForm.java index 228b0d9ac66..39a02cb9750 100644 --- a/api/src/org/labkey/api/data/TableViewForm.java +++ b/api/src/org/labkey/api/data/TableViewForm.java @@ -785,7 +785,7 @@ public static String getMultiPartFormFieldNameForColumn(@NotNull String columnNa public String getMultiPartFormFieldName(@NotNull ColumnInfo column) { - return getMultiPartFormFieldNameForColumn(column.getName()); + return DataIteratorUtil.MatchType.multiPartFormData.getMatchedName(getFormFieldName(column)); } @Nullable From 617b1f26abddba9825e9fe3feb2818da97bd32b3 Mon Sep 17 00:00:00 2001 From: labkey-nicka Date: Wed, 12 Nov 2025 14:21:22 -0800 Subject: [PATCH 08/10] Fix path resolution --- .../org/labkey/api/assay/actions/AssayRunUploadForm.java | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/api/src/org/labkey/api/assay/actions/AssayRunUploadForm.java b/api/src/org/labkey/api/assay/actions/AssayRunUploadForm.java index 60879a32fce..26ebcbe6256 100644 --- a/api/src/org/labkey/api/assay/actions/AssayRunUploadForm.java +++ b/api/src/org/labkey/api/assay/actions/AssayRunUploadForm.java @@ -404,12 +404,9 @@ private Map getAdditionalPostedFiles(Domain domain, Li // Only add a hidden file parameter if it is a valid file in the pipeline root directory and // a new file hasn't been uploaded for that parameter Path path = FileUtil.stringToPath(getContainer(), previousFilePath, false); - if (FileUtil.isFileAndExists(path)) - { - File previousFile = path.toFile(); - if (previousFile.isFile() && FileUtils.directoryContains(assayDirectory, previousFile)) - additionalFiles.put(domainProperty, FileSystemLike.wrapFile(previousFile)); - } + File previousFile = FileUtil.appendName(assayDirectory, path.getFileName().toString()); + if (previousFile.isFile() && FileUtils.directoryContains(assayDirectory, previousFile)) + additionalFiles.put(domainProperty, FileSystemLike.wrapFile(previousFile)); } } catch (IOException e) From c4e6e80a66f5b912c87dadac2b5b86ad2ded9ae4 Mon Sep 17 00:00:00 2001 From: labkey-nicka Date: Wed, 12 Nov 2025 15:58:45 -0800 Subject: [PATCH 09/10] Additional tests --- .../org/labkey/api/data/TableViewForm.java | 2 +- .../tests/assay/AssayReimportIndexTest.java | 100 ++++++++++++++---- 2 files changed, 79 insertions(+), 23 deletions(-) diff --git a/api/src/org/labkey/api/data/TableViewForm.java b/api/src/org/labkey/api/data/TableViewForm.java index 39a02cb9750..56909e471b1 100644 --- a/api/src/org/labkey/api/data/TableViewForm.java +++ b/api/src/org/labkey/api/data/TableViewForm.java @@ -756,7 +756,7 @@ private String getPropertyCaption(String propName) return null==column ? propName : column.getLabel(); } - // RFC 2616 / RFC 7230: Escape characters inside the field name + // Issue 54218: RFC 2616 / RFC 7230: Escape characters inside the field name private static final char BACKSLASH = '\\'; private static final String SPECIAL_CHARS = BACKSLASH + "\";=,"; diff --git a/assay/test/src/org/labkey/test/tests/assay/AssayReimportIndexTest.java b/assay/test/src/org/labkey/test/tests/assay/AssayReimportIndexTest.java index de3c4fc4828..4faafd3b53e 100644 --- a/assay/test/src/org/labkey/test/tests/assay/AssayReimportIndexTest.java +++ b/assay/test/src/org/labkey/test/tests/assay/AssayReimportIndexTest.java @@ -29,7 +29,9 @@ public class AssayReimportIndexTest extends BaseWebDriverTest { private static final String ASSAY_NAME = DomainKind.Assay.randomName("test+assay"); + private static final FieldInfo BATCH_FIELD = DomainKind.Assay.randomField("batchData", ColumnType.String); private static final FieldInfo BATCH_FILE_FIELD = DomainKind.Assay.randomField("batchFile", ColumnType.File); + private static final FieldInfo RUN_FIELD = DomainKind.Assay.randomField("runString", ColumnType.String); private static final FieldInfo RUN_FILE_FIELD = DomainKind.Assay.randomField("runFile", ColumnType.File); @BeforeClass @@ -44,15 +46,8 @@ private void doSetup() throws Exception _containerHelper.createProject(getProjectName(), "Assay"); goToProjectHome(); - List batchFields = List.of( - DomainKind.Assay.randomField("batchData", ColumnType.String).getFieldDefinition(), - BATCH_FILE_FIELD.getFieldDefinition() - ); - - List runFields = List.of( - DomainKind.Assay.randomField("runString", ColumnType.String).getFieldDefinition(), - RUN_FILE_FIELD.getFieldDefinition() - ); + List batchFields = List.of(BATCH_FIELD.getFieldDefinition(), BATCH_FILE_FIELD.getFieldDefinition()); + List runFields = List.of(RUN_FIELD.getFieldDefinition(), RUN_FILE_FIELD.getFieldDefinition()); new GeneralAssayDesign(ASSAY_NAME) .setBatchFields(batchFields, true) @@ -133,44 +128,105 @@ public void testIndexLatestAssayRun() searchResultPage3.hasResultLocatedBy(Locator.linkWithText("Assay Run - " + secondRun))); } - @Test // Issue 54112 + @Test // Issue 54112, Issue 54218 public void testFileFieldValuesRetainedRunReimport() { String runName = TestDataGenerator.randomString(TestDataGenerator.randomInt(10, 50)); + String batchFieldValue = TestDataGenerator.randomString(TestDataGenerator.randomInt(10, 50)); + String runFieldValue = TestDataGenerator.randomString(TestDataGenerator.randomInt(10, 50)); File batchFile = TestFileUtils.getSampleData("dataLoading/excel/fruits.tsv"); File runFile = TestFileUtils.getSampleData("dataLoading/excel/ClientAPITestList.xls"); File dataFile = TestFileUtils.getSampleData("assay/GPAT_Run1.tsv"); // import the initial run importNewRun() + .setNamedInputText(BATCH_FIELD.getName(), batchFieldValue) .setFileField(BATCH_FILE_FIELD.getName(), batchFile) .clickNext() .setNamedInputText("Name", runName) + .setNamedInputText(RUN_FIELD.getName(), runFieldValue) .setFileField(RUN_FILE_FIELD.getName(), runFile) .setDataFile(dataFile) .clickSaveAndFinish(); - // Reimport the run and verify expected file field values + // Verify batch values during reimport var importPage = reimportRun(runName); - var reimportBatchFileValue = importPage.getFileFieldValue(BATCH_FILE_FIELD.getName()); - checker().withScreenshot("reimport-run-batch-field-value") - .verifyEquals("Expected batch file name", batchFile.getName(), reimportBatchFileValue); + String actualBatchValue = importPage.getFieldValue(BATCH_FIELD.getName()); + checker().withScreenshot("reimport-batch-field-value") + .verifyEquals("Unexpected batch field value", batchFieldValue, actualBatchValue); + actualBatchValue = importPage.getFileFieldValue(BATCH_FILE_FIELD.getName()); + checker().withScreenshot("reimport-batch-file-field-value") + .verifyEquals("Unexpected batch file name", batchFile.getName(), actualBatchValue); + + // Verify run values during reimport importPage = importPage.clickNext(); - var reimportRunFileValue = importPage.getFileFieldValue(RUN_FILE_FIELD.getName()); - checker().withScreenshot("reimport-run-run-field-value") - .verifyEquals("Expected run file name", runFile.getName(), reimportRunFileValue); + String actualRunValue = importPage.getFieldValue(RUN_FIELD.getName()); + checker().withScreenshot("reimport-batch-field-value") + .verifyEquals("Unexpected run field value", runFieldValue, actualRunValue); + actualRunValue = importPage.getFileFieldValue(RUN_FILE_FIELD.getName()); + checker().withScreenshot("reimport-run-file-field-value") + .verifyEquals("Unexpected run file name", runFile.getName(), actualRunValue); importPage.selectUploadFileRadioButton() .clickSaveAndFinish(); - // Verify that the reimport retained the file values + // Verify that the reimport retained the values var row = new AssayRunsPage(getDriver()) .getTable() .getRowDataAsMap("Name", runName); - reimportBatchFileValue = StringUtils.trimToNull(row.get("Batch/" + BATCH_FILE_FIELD.getFieldKey())); - reimportRunFileValue = StringUtils.trimToNull(row.get(RUN_FILE_FIELD.getFieldKey().toString())); + actualBatchValue = StringUtils.trimToNull(row.get("Batch/" + BATCH_FIELD.getFieldKey())); + actualRunValue = StringUtils.trimToNull(row.get(RUN_FIELD.getFieldKey().toString())); + checker().verifyEquals("Unexpected batch field value in grid", batchFieldValue, actualBatchValue); + checker().verifyEquals("Unexpected run field value in grid", runFieldValue, actualRunValue); + + actualBatchValue = StringUtils.trimToNull(row.get("Batch/" + BATCH_FILE_FIELD.getFieldKey())); + actualRunValue = StringUtils.trimToNull(row.get(RUN_FILE_FIELD.getFieldKey().toString())); + checker().verifyEquals("Unexpected batch file in grid", batchFile.getName(), actualBatchValue); + checker().verifyEquals("Unexpected run file in grid", runFile.getName(), actualRunValue); + + // Verify deleting batch file value is respected + reimportRun(runName) + .removeFileValue(BATCH_FILE_FIELD.getName()) + .clickNext() + .selectUploadFileRadioButton() + .clickSaveAndFinish(); + + row = new AssayRunsPage(getDriver()) + .getTable() + .getRowDataAsMap("Name", runName); + actualBatchValue = StringUtils.trimToNull(row.get("Batch/" + BATCH_FILE_FIELD.getFieldKey())); + actualRunValue = StringUtils.trimToNull(row.get(RUN_FILE_FIELD.getFieldKey().toString())); + checker().verifyNull("Unexpected batch field value in grid", actualBatchValue); + checker().verifyEquals("Unexpected run field value in grid", runFile.getName(), actualRunValue); + + // Verify other values remain unchanged + actualBatchValue = StringUtils.trimToNull(row.get("Batch/" + BATCH_FIELD.getFieldKey())); + actualRunValue = StringUtils.trimToNull(row.get(RUN_FIELD.getFieldKey().toString())); + checker().verifyEquals("Unexpected batch field value in grid", batchFieldValue, actualBatchValue); + checker().verifyEquals("Unexpected run field value in grid", runFieldValue, actualRunValue); + + // Verify deleting run file value is respected + batchFile = TestFileUtils.getSampleData("dataLoading/excel/fruits.xls"); + runFieldValue = TestDataGenerator.randomString(TestDataGenerator.randomInt(10, 50)); + reimportRun(runName) + .setFileField(BATCH_FILE_FIELD.getName(), batchFile) + .clickNext() + .removeFileValue(RUN_FILE_FIELD.getName()) + .setNamedInputText(RUN_FIELD.getName(), runFieldValue) + .selectUploadFileRadioButton() + .clickSaveAndFinish(); + + row = new AssayRunsPage(getDriver()) + .getTable() + .getRowDataAsMap("Name", runName); + actualBatchValue = StringUtils.trimToNull(row.get("Batch/" + BATCH_FIELD.getFieldKey())); + actualRunValue = StringUtils.trimToNull(row.get(RUN_FIELD.getFieldKey().toString())); + checker().verifyEquals("Unexpected batch field value in grid", batchFieldValue, actualBatchValue); + checker().verifyEquals("Unexpected run field value in grid", runFieldValue, actualRunValue); - checker().verifyEquals("Expected batch file to appear in data", batchFile.getName(), reimportBatchFileValue); - checker().verifyEquals("Expected run file to appear in data", runFile.getName(), reimportRunFileValue); + actualBatchValue = StringUtils.trimToNull(row.get("Batch/" + BATCH_FILE_FIELD.getFieldKey())); + actualRunValue = StringUtils.trimToNull(row.get(RUN_FILE_FIELD.getFieldKey().toString())); + checker().verifyEquals("Unexpected batch file in grid", batchFile.getName(), actualBatchValue); + checker().verifyNull("Unexpected run file in grid", actualRunValue); } private AssayImportPage importNewRun() From b424951254118974de57fa624786506a2aa36a67 Mon Sep 17 00:00:00 2001 From: labkey-nicka Date: Wed, 12 Nov 2025 15:59:14 -0800 Subject: [PATCH 10/10] Rename to AssayReimportTest --- .../{AssayReimportIndexTest.java => AssayReimportTest.java} | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) rename assay/test/src/org/labkey/test/tests/assay/{AssayReimportIndexTest.java => AssayReimportTest.java} (99%) diff --git a/assay/test/src/org/labkey/test/tests/assay/AssayReimportIndexTest.java b/assay/test/src/org/labkey/test/tests/assay/AssayReimportTest.java similarity index 99% rename from assay/test/src/org/labkey/test/tests/assay/AssayReimportIndexTest.java rename to assay/test/src/org/labkey/test/tests/assay/AssayReimportTest.java index 4faafd3b53e..4c100ab85f8 100644 --- a/assay/test/src/org/labkey/test/tests/assay/AssayReimportIndexTest.java +++ b/assay/test/src/org/labkey/test/tests/assay/AssayReimportTest.java @@ -26,7 +26,7 @@ import java.util.List; @Category({Daily.class, Assays.class}) -public class AssayReimportIndexTest extends BaseWebDriverTest +public class AssayReimportTest extends BaseWebDriverTest { private static final String ASSAY_NAME = DomainKind.Assay.randomName("test+assay"); private static final FieldInfo BATCH_FIELD = DomainKind.Assay.randomField("batchData", ColumnType.String); @@ -37,7 +37,7 @@ public class AssayReimportIndexTest extends BaseWebDriverTest @BeforeClass public static void setupProject() throws Exception { - AssayReimportIndexTest init = getCurrentTest(); + AssayReimportTest init = getCurrentTest(); init.doSetup(); }