From 10cc5886105233724006904edc31a9246ba29d9c Mon Sep 17 00:00:00 2001 From: labkey-jeckels Date: Thu, 22 Oct 2026 11:15:39 -0700 Subject: [PATCH 01/11] GH Issue 1720: Separate lookup from creation in save handler data and material handling --- .../labkey/api/data/ExpDataFileConverter.java | 17 ++- .../exp/api/DefaultExperimentSaveHandler.java | 104 +++++++++++------- .../api/assay/DefaultAssaySaveHandler.java | 16 ++- 3 files changed, 91 insertions(+), 46 deletions(-) diff --git a/api/src/org/labkey/api/data/ExpDataFileConverter.java b/api/src/org/labkey/api/data/ExpDataFileConverter.java index b5fb6e57969..2d5feedeb2c 100644 --- a/api/src/org/labkey/api/data/ExpDataFileConverter.java +++ b/api/src/org/labkey/api/data/ExpDataFileConverter.java @@ -63,6 +63,17 @@ public class ExpDataFileConverter public static final Converter FILE_CONVERTER = new FileConverter(); public static ExpData resolveExpData(JSONObject dataObject, @NotNull Container container, @NotNull User user, @NotNull Collection knownTypes) + { + return resolveExpData(dataObject, container, user, knownTypes, true); + } + + /** Like resolveExpData, but data for a file that has none yet comes back unsaved. */ + public static ExpData resolveExpDataWithoutSaving(JSONObject dataObject, @NotNull Container container, @NotNull User user, @NotNull Collection knownTypes) + { + return resolveExpData(dataObject, container, user, knownTypes, false); + } + + private static ExpData resolveExpData(JSONObject dataObject, @NotNull Container container, @NotNull User user, @NotNull Collection knownTypes, boolean save) { ExperimentService expSvc = ExperimentService.get(); @@ -122,7 +133,8 @@ else if (dataObject.has(ExperimentJSONConverter.PIPELINE_PATH) && dataObject.get String lsid = expSvc.generateLSID(container, type, name); data = expSvc.createData(container, name, lsid); data.setDataFileURI(file.toPath().toUri()); - data.save(user); + if (save) + data.save(user); } return data; } @@ -170,7 +182,8 @@ else if (dataObject.has(ExperimentJSONConverter.ABSOLUTE_PATH) && dataObject.get String lsid = expSvc.generateLSID(container, type, name); data = expSvc.createData(container, name, lsid); data.setDataFileURI(f.toURI()); - data.save(user); + if (save) + data.save(user); } return data; } diff --git a/api/src/org/labkey/api/exp/api/DefaultExperimentSaveHandler.java b/api/src/org/labkey/api/exp/api/DefaultExperimentSaveHandler.java index c2704721e6c..7f1b2cfb36c 100644 --- a/api/src/org/labkey/api/exp/api/DefaultExperimentSaveHandler.java +++ b/api/src/org/labkey/api/exp/api/DefaultExperimentSaveHandler.java @@ -25,6 +25,7 @@ import org.json.JSONException; import org.json.JSONObject; import org.labkey.api.action.ApiUsageException; +import org.labkey.api.assay.AssayDataType; import org.labkey.api.assay.AssayService; import org.labkey.api.data.Container; import org.labkey.api.data.ExpDataFileConverter; @@ -554,10 +555,58 @@ protected ExpExperiment saveExperimentRun(ViewContext context, ExpProtocol proto @Override public ExpMaterial handleMaterial(ViewContext context, JSONObject materialObject) throws ValidationException + { + MaterialLookup lookup = lookupMaterial(context, materialObject); + if (lookup == null) + return null; + + ExpSampleType sampleType = lookup.sampleType(); + ExpMaterial material = lookup.material(); + boolean createdHere = material == null; + if (createdHere) + { + material = createMaterial(context, sampleType, lookup.name()); + createdMaterialLsids.add(material.getLSID()); + } + + if (materialObject.has(ExperimentJSONConverter.PROPERTIES)) + { + JSONObject materialProperties = materialObject.getJSONObject(ExperimentJSONConverter.PROPERTIES); + // Treat an empty properties collection as if there were no property map at all. + // To delete a property, include a property map with that property and set its value to null. + if (!materialProperties.isEmpty()) + { + List dps = sampleType != null ? sampleType.getDomain().getProperties() : Collections.emptyList(); + + if (propertiesWouldChange(context, material, dps, materialProperties)) + { + // A sample created by this request needs only insert rights; an existing one needs edit rights. + // The status check below applies either way. + Class editPerm = createdHere + ? InsertPermission.class + : SampleTypeService.SampleOperations.EditMetadata.getPermissionClass(); + if (editPerm == null || !material.getContainer().hasPermission(context.getUser(), editPerm)) + throw new UnauthorizedException("User does not have permission to edit sample '" + material.getName() + "'"); + if (!material.isOperationPermitted(SampleTypeService.SampleOperations.EditMetadata)) + throw new UnauthorizedException(SampleTypeService.get().getOperationNotPermittedMessage( + List.of(material), SampleTypeService.SampleOperations.EditMetadata)); + } + + handleProperties(context, material, dps, materialProperties); + } + } + + return material; + } + + /** An existing material, or, when material is null, the sample type and name of the one handleMaterial would create. */ + protected record MaterialLookup(@Nullable ExpMaterial material, @Nullable ExpSampleType sampleType, @NotNull String name) {} + + /** Resolves the material handleMaterial would use, without writing anything. Returns null when there's nothing to resolve. */ + protected @Nullable MaterialLookup lookupMaterial(ViewContext context, JSONObject materialObject) throws ValidationException { ExpSampleType sampleType = null; ExpMaterial material = null; - boolean createdHere = false; if (materialObject.has(ExperimentJSONConverter.ID)) { int materialRowId = materialObject.getInt(ExperimentJSONConverter.ID); @@ -640,46 +689,11 @@ else if (sampleType != null && !sampleType.hasNameAsIdCol() && materialObject.ha if (material != null && !material.getContainer().hasPermission(context.getUser(), ReadPermission.class)) throw new NotFoundException("Could not find material '" + materialName + "'"); - if (material == null) - { - material = createMaterial(context, sampleType, materialName); - createdHere = true; - createdMaterialLsids.add(material.getLSID()); - } + return new MaterialLookup(material, sampleType, materialName); } } - if (material == null) - return null; - - if (materialObject.has(ExperimentJSONConverter.PROPERTIES)) - { - JSONObject materialProperties = materialObject.getJSONObject(ExperimentJSONConverter.PROPERTIES); - // Treat an empty properties collection as if there were no property map at all. - // To delete a property, include a property map with that property and set its value to null. - if (!materialProperties.isEmpty()) - { - List dps = sampleType != null ? sampleType.getDomain().getProperties() : Collections.emptyList(); - - if (propertiesWouldChange(context, material, dps, materialProperties)) - { - // A sample created by this request needs only insert rights; an existing one needs edit rights. - // The status check below applies either way. - Class editPerm = createdHere - ? InsertPermission.class - : SampleTypeService.SampleOperations.EditMetadata.getPermissionClass(); - if (editPerm == null || !material.getContainer().hasPermission(context.getUser(), editPerm)) - throw new UnauthorizedException("User does not have permission to edit sample '" + material.getName() + "'"); - if (!material.isOperationPermitted(SampleTypeService.SampleOperations.EditMetadata)) - throw new UnauthorizedException(SampleTypeService.get().getOperationNotPermittedMessage( - List.of(material), SampleTypeService.SampleOperations.EditMetadata)); - } - - handleProperties(context, material, dps, materialProperties); - } - } - - return material; + return material == null ? null : new MaterialLookup(material, sampleType, material.getName()); } private boolean propertiesWouldChange(ViewContext context, ExpMaterial material, List dps, JSONObject submitted) throws ValidationException @@ -752,7 +766,19 @@ private static boolean valuesEqual(Object stored, Object submitted) @Override public ExpData handleData(ViewContext context, JSONObject dataObject) throws ValidationException { - return ExpDataFileConverter.resolveExpData(dataObject, context.getContainer(), context.getUser(), Collections.emptyList()); + return ExpDataFileConverter.resolveExpData(dataObject, context.getContainer(), context.getUser(), getKnownDataTypes()); + } + + /** Resolves the data handleData would use, without writing anything. Data that doesn't exist yet comes back unsaved. */ + protected ExpData lookupData(ViewContext context, JSONObject dataObject) + { + return ExpDataFileConverter.resolveExpDataWithoutSaving(dataObject, context.getContainer(), context.getUser(), getKnownDataTypes()); + } + + /** Data types used to pick the type of data created for a new file. */ + protected @NotNull Collection getKnownDataTypes() + { + return Collections.emptyList(); } // XXX: doesn't handle SampleType parent property magic diff --git a/assay/api-src/org/labkey/api/assay/DefaultAssaySaveHandler.java b/assay/api-src/org/labkey/api/assay/DefaultAssaySaveHandler.java index 5882233b6de..e3142fd4171 100644 --- a/assay/api-src/org/labkey/api/assay/DefaultAssaySaveHandler.java +++ b/assay/api-src/org/labkey/api/assay/DefaultAssaySaveHandler.java @@ -25,7 +25,6 @@ import org.labkey.api.action.BaseViewAction; import org.labkey.api.audit.TransactionAuditProvider; import org.labkey.api.data.Container; -import org.labkey.api.data.ExpDataFileConverter; import org.labkey.api.data.MvUtil; import org.labkey.api.dataiterator.MapDataIterator; import org.labkey.api.exp.ExperimentException; @@ -50,6 +49,7 @@ import org.labkey.remoteapi.collections.CaseInsensitiveHashMap; import java.util.ArrayList; +import java.util.Collection; import java.util.Collections; import java.util.List; import java.util.Map; @@ -328,6 +328,15 @@ protected ExpExperiment saveAssayRun(AssayRunUploadContext context, ExpExperi @Override @Deprecated // Use new JSONObject variant above public ExpData handleData(ViewContext context, JSONObject dataObject) throws ValidationException + { + ExpData data = super.handleData(context, dataObject); + + handleProperties(context, data, Collections.emptyList(), dataObject); + return data; + } + + @Override + protected @NotNull Collection getKnownDataTypes() { List knownTypes = new ArrayList<>(); if (getProvider().getDataType() != null) @@ -335,9 +344,6 @@ public ExpData handleData(ViewContext context, JSONObject dataObject) throws Val knownTypes.add(getProvider().getDataType()); } knownTypes.addAll(getProvider().getRelatedDataTypes()); - ExpData data = ExpDataFileConverter.resolveExpData(dataObject, context.getContainer(), context.getUser(), knownTypes); - - handleProperties(context, data, Collections.emptyList(), dataObject); - return data; + return knownTypes; } } From c6b0987574424f715c4fd2a67128750a64ad2392 Mon Sep 17 00:00:00 2001 From: labkey-jeckels Date: Thu, 22 Oct 2026 11:49:32 -0700 Subject: [PATCH 02/11] GH Issue 1720: Run saveBatch and saveRuns transform scripts before the save transaction --- .../labkey/api/assay/AssaySaveHandler.java | 19 +++ .../assay/transform/PreparedTransforms.java | 48 ++++++++ .../api/assay/DefaultAssaySaveHandler.java | 111 ++++++++++++++++++ .../org/labkey/assay/TsvAssaySaveHandler.java | 8 +- .../assay/actions/BaseProtocolAPIAction.java | 12 ++ .../assay/actions/SaveAssayBatchAction.java | 17 ++- .../assay/actions/SaveAssayRunsAction.java | 5 +- 7 files changed, 216 insertions(+), 4 deletions(-) create mode 100644 api/src/org/labkey/api/assay/transform/PreparedTransforms.java diff --git a/api/src/org/labkey/api/assay/AssaySaveHandler.java b/api/src/org/labkey/api/assay/AssaySaveHandler.java index 6102e101a2c..f71c7441b7e 100644 --- a/api/src/org/labkey/api/assay/AssaySaveHandler.java +++ b/api/src/org/labkey/api/assay/AssaySaveHandler.java @@ -16,7 +16,16 @@ package org.labkey.api.assay; +import org.jetbrains.annotations.NotNull; +import org.json.JSONObject; +import org.labkey.api.assay.transform.PreparedTransforms; +import org.labkey.api.exp.ExperimentException; +import org.labkey.api.exp.api.ExpProtocol; import org.labkey.api.exp.api.ExperimentSaveHandler; +import org.labkey.api.query.ValidationException; +import org.labkey.api.view.ViewContext; + +import java.util.List; /** * User: dax @@ -64,4 +73,14 @@ public interface AssaySaveHandler extends ExperimentSaveHandler */ void setProvider(AssayProvider provider); AssayProvider getProvider(); + + /** + * Runs the transform scripts of the given runs before the save's transaction opens, so a script's callbacks into + * the server can't deadlock with it. The handler records the outputs as it saves each run; runs it doesn't prepare + * run their scripts inside the transaction. Called after beforeSave. + */ + default @NotNull PreparedTransforms prepareTransforms(ViewContext context, ExpProtocol protocol, List runJsons) throws ExperimentException, ValidationException + { + return new PreparedTransforms(); + } } diff --git a/api/src/org/labkey/api/assay/transform/PreparedTransforms.java b/api/src/org/labkey/api/assay/transform/PreparedTransforms.java new file mode 100644 index 00000000000..32c85fa336c --- /dev/null +++ b/api/src/org/labkey/api/assay/transform/PreparedTransforms.java @@ -0,0 +1,48 @@ +/* + * Copyright (c) 2026 LabKey Corporation + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.labkey.api.assay.transform; + +import org.jetbrains.annotations.NotNull; +import org.jetbrains.annotations.Nullable; +import org.json.JSONObject; + +import java.util.IdentityHashMap; +import java.util.Map; + +/** + * The {@link PreparedTransform}s for the runs of one save request, keyed by each run's JSON object. + * Close it only after the transaction that recorded them has committed or rolled back. + */ +public class PreparedTransforms implements AutoCloseable +{ + private final Map _byRunJson = new IdentityHashMap<>(); + + public void put(@NotNull JSONObject runJson, @NotNull PreparedTransform prepared) + { + _byRunJson.put(runJson, prepared); + } + + public @Nullable PreparedTransform get(@NotNull JSONObject runJson) + { + return _byRunJson.get(runJson); + } + + @Override + public void close() + { + _byRunJson.values().forEach(PreparedTransform::close); + } +} diff --git a/assay/api-src/org/labkey/api/assay/DefaultAssaySaveHandler.java b/assay/api-src/org/labkey/api/assay/DefaultAssaySaveHandler.java index e3142fd4171..9542f211040 100644 --- a/assay/api-src/org/labkey/api/assay/DefaultAssaySaveHandler.java +++ b/assay/api-src/org/labkey/api/assay/DefaultAssaySaveHandler.java @@ -23,6 +23,8 @@ import org.json.JSONArray; import org.json.JSONObject; import org.labkey.api.action.BaseViewAction; +import org.labkey.api.assay.transform.PreparedTransform; +import org.labkey.api.assay.transform.PreparedTransforms; import org.labkey.api.audit.TransactionAuditProvider; import org.labkey.api.data.Container; import org.labkey.api.data.MvUtil; @@ -39,6 +41,7 @@ import org.labkey.api.exp.api.ExpProtocol; import org.labkey.api.exp.api.ExpRun; import org.labkey.api.exp.api.ExperimentJSONConverter; +import org.labkey.api.exp.api.ExperimentService; import org.labkey.api.exp.api.ProvenanceService; import org.labkey.api.exp.property.Domain; import org.labkey.api.exp.property.DomainProperty; @@ -51,6 +54,7 @@ import java.util.ArrayList; import java.util.Collection; import java.util.Collections; +import java.util.HashMap; import java.util.List; import java.util.Map; @@ -58,6 +62,7 @@ public class DefaultAssaySaveHandler extends DefaultExperimentSaveHandler implem { protected static final Logger LOG = LogManager.getLogger(DefaultAssaySaveHandler.class); protected AssayProvider _provider; + private PreparedTransforms _preparedTransforms = new PreparedTransforms(); @Override public AssayProvider getProvider() @@ -221,11 +226,117 @@ protected ExpExperiment saveExperimentRun( AssayRunUploadContext uploadContext = createRunUploadContext(context, protocol, runJson, dataRows, inputData, outputData, inputMaterial, outputMaterial); + PreparedTransform prepared = _preparedTransforms.get(runJson); + if (prepared != null && uploadContext != null) + { + if (prepared.getRunDataInputs() != null) + { + Map runDataInputs = new HashMap<>(prepared.getRunDataInputs()); + runDataInputs.putAll(inputData); + prepared.setRunDataInputs(runDataInputs); + } + uploadContext.setPreparedTransform(prepared); + } + return saveAssayRun(uploadContext, batch, run, BaseViewAction.getTransactionAuditDetails(context)); } return null; } + /** Whether runs are saved exactly as this class saves them, which the first pass of prepareTransforms assumes. */ + protected boolean isPrepareTransformsSupported() + { + return false; + } + + @Override + public @NotNull PreparedTransforms prepareTransforms(ViewContext context, ExpProtocol protocol, List runJsons) throws ExperimentException, ValidationException + { + PreparedTransforms preparedTransforms = new PreparedTransforms(); + if (!isPrepareTransformsSupported() + || !getProvider().isTransformBeforeRunSupported(protocol) + || getProvider().getValidationAndAnalysisScripts(protocol, AssayProvider.Scope.ALL).isEmpty()) + return preparedTransforms; + + boolean success = false; + try + { + for (JSONObject runJson : runJsons) + { + PreparedTransform prepared = prepareTransform(context, protocol, runJson); + if (prepared != null) + preparedTransforms.put(runJson, prepared); + } + success = true; + } + finally + { + if (!success) + preparedTransforms.close(); + } + _preparedTransforms = preparedTransforms; + return preparedTransforms; + } + + /** Builds the upload context handleRun would build for this run, without writing anything, and runs its transform scripts. */ + private @Nullable PreparedTransform prepareTransform(ViewContext context, ExpProtocol protocol, JSONObject runJson) throws ExperimentException, ValidationException + { + if (!runJson.has(AssayJSONConverter.DATA_ROWS) && !runJson.has(ExperimentJSONConverter.DATA_INPUTS) + && !runJson.has(ExperimentJSONConverter.MATERIAL_INPUTS) && !runJson.has(ExperimentJSONConverter.DATA_OUTPUTS)) + return null; + + ExpRun existingRun = null; + if (!runJson.isNull(ExperimentJSONConverter.ID)) + { + existingRun = ExperimentService.get().getExpRun(runJson.getInt(ExperimentJSONConverter.ID)); + // Leave the error to handleRun + if (existingRun == null || !existingRun.getContainer().equals(context.getContainer())) + return null; + } + + JSONArray dataArray = existingRun != null ? serializeRunData(context, existingRun, runJson, protocol, null) : postedArray(runJson, AssayJSONConverter.DATA_ROWS); + JSONArray dataInputs = existingRun != null ? serializeDataInputs(context, existingRun, runJson, protocol, null) : postedArray(runJson, ExperimentJSONConverter.DATA_INPUTS); + JSONArray materialInputs = existingRun != null ? serializeMaterialInputs(context, existingRun, runJson, protocol, null) : postedArray(runJson, ExperimentJSONConverter.MATERIAL_INPUTS); + JSONArray dataOutputs = existingRun != null ? serializeDataOutputs(context, existingRun, runJson, protocol, null) : postedArray(runJson, ExperimentJSONConverter.DATA_OUTPUTS); + JSONArray materialOutputs = existingRun != null ? serializeMaterialOutputs(context, existingRun, runJson, protocol, null) : postedArray(runJson, ExperimentJSONConverter.MATERIAL_OUTPUTS); + + Map inputData = new HashMap<>(); + for (JSONObject dataObject : JsonUtil.toJSONObjectList(dataInputs)) + inputData.put(lookupData(context, dataObject), dataObject.optString(ExperimentJSONConverter.ROLE, ExpDataRunInput.DEFAULT_ROLE)); + Map outputData = new HashMap<>(); + for (JSONObject dataObject : JsonUtil.toJSONObjectList(dataOutputs)) + outputData.put(lookupData(context, dataObject), dataObject.optString(ExperimentJSONConverter.ROLE, ExpDataRunInput.DEFAULT_ROLE)); + + // Scripts don't see the run's materials, so these lookups only reject bad references before any script runs + for (JSONObject materialObject : JsonUtil.toJSONObjectList(materialInputs)) + lookupMaterial(context, materialObject); + for (JSONObject materialObject : JsonUtil.toJSONObjectList(materialOutputs)) + lookupMaterial(context, materialObject); + + // saveExperimentRun skips the assay save, and with it the scripts, when there are neither result rows nor outputs + List> dataRows = convertRunData(dataArray, context.getContainer(), protocol); + if (dataRows.isEmpty() && outputData.isEmpty()) + return null; + + AssayRunUploadContext uploadContext = createRunUploadContext(context, protocol, runJson, dataRows, + inputData, outputData, Collections.emptyMap(), Collections.emptyMap()); + AssayRunCreator runCreator = getProvider().getRunCreator(); + PreparedTransform prepared = runCreator.prepareTransform(uploadContext, false); + if (prepared != null && prepared.getRunDataInputs() != null) + { + // saveExperimentRun adds the saved inputs back; keep only those the participant visit resolver added + Map resolverInputs = new HashMap<>(prepared.getRunDataInputs()); + resolverInputs.keySet().removeAll(inputData.keySet()); + prepared.setRunDataInputs(resolverInputs); + } + return prepared; + } + + private static JSONArray postedArray(JSONObject runJson, String key) + { + return runJson.has(key) ? runJson.getJSONArray(key) : new JSONArray(); + } + /** * Handle any mv indicator columns plus any additional conversion on the results domain data before run creation. */ diff --git a/assay/src/org/labkey/assay/TsvAssaySaveHandler.java b/assay/src/org/labkey/assay/TsvAssaySaveHandler.java index 46c10e02db0..f8a888e0429 100644 --- a/assay/src/org/labkey/assay/TsvAssaySaveHandler.java +++ b/assay/src/org/labkey/assay/TsvAssaySaveHandler.java @@ -22,8 +22,14 @@ * User: kevink * Date: 8/10/14 */ -public class TsvAssaySaveHandler extends DefaultAssaySaveHandler +public final class TsvAssaySaveHandler extends DefaultAssaySaveHandler { + @Override + protected boolean isPrepareTransformsSupported() + { + return true; + } + @Override public void setProvider(AssayProvider provider) { diff --git a/assay/src/org/labkey/assay/actions/BaseProtocolAPIAction.java b/assay/src/org/labkey/assay/actions/BaseProtocolAPIAction.java index 96a9f831760..c6ce1c8ab54 100644 --- a/assay/src/org/labkey/assay/actions/BaseProtocolAPIAction.java +++ b/assay/src/org/labkey/assay/actions/BaseProtocolAPIAction.java @@ -22,14 +22,18 @@ import org.labkey.api.action.MutatingApiAction; import org.labkey.api.action.SimpleApiJsonForm; import org.labkey.api.assay.AssayProvider; +import org.labkey.api.assay.AssaySaveHandler; import org.labkey.api.assay.AssayService; +import org.labkey.api.assay.transform.PreparedTransforms; import org.labkey.api.data.Container; +import org.labkey.api.exp.ExperimentException; import org.labkey.api.exp.api.AssayJSONConverter; import org.labkey.api.exp.api.DefaultExperimentSaveHandler; import org.labkey.api.exp.api.ExpProtocol; import org.labkey.api.exp.api.ExperimentJSONConverter; import org.labkey.api.exp.api.ExperimentSaveHandler; import org.labkey.api.exp.api.ExperimentService; +import org.labkey.api.query.ValidationException; import org.labkey.api.util.Pair; import org.labkey.api.view.NotFoundException; import org.springframework.validation.BindException; @@ -152,4 +156,12 @@ public ExperimentSaveHandler getExperimentSaveHandler(@Nullable AssayProvider pr return saveHandler; } + + /** GH Issue 1720: run transform scripts before the save's transaction so their callbacks into the server can't deadlock with it */ + protected PreparedTransforms prepareTransforms(ExperimentSaveHandler saveHandler, ExpProtocol protocol, List runJsons) throws ExperimentException, ValidationException + { + if (saveHandler instanceof AssaySaveHandler assaySaveHandler) + return assaySaveHandler.prepareTransforms(getViewContext(), protocol, runJsons); + return new PreparedTransforms(); + } } diff --git a/assay/src/org/labkey/assay/actions/SaveAssayBatchAction.java b/assay/src/org/labkey/assay/actions/SaveAssayBatchAction.java index 8c15c890678..b5949c1f38e 100644 --- a/assay/src/org/labkey/assay/actions/SaveAssayBatchAction.java +++ b/assay/src/org/labkey/assay/actions/SaveAssayBatchAction.java @@ -21,6 +21,7 @@ import org.labkey.api.action.ApiVersion; import org.labkey.api.action.SimpleApiJsonForm; import org.labkey.api.assay.AssayProvider; +import org.labkey.api.assay.transform.PreparedTransforms; import org.labkey.api.data.DbScope; import org.labkey.api.exp.api.AssayJSONConverter; import org.labkey.api.exp.api.ExpExperiment; @@ -29,6 +30,7 @@ import org.labkey.api.exp.api.ExperimentService; import org.labkey.api.security.RequiresPermission; import org.labkey.api.security.permissions.InsertPermission; +import org.labkey.api.util.JsonUtil; import org.springframework.validation.BindException; import java.util.ArrayList; @@ -81,7 +83,11 @@ private ApiResponse executeAction(ExperimentSaveHandler saveHandler, ExpProtocol { saveHandler.beforeSave(getViewContext(), rootJsonObject, protocol); List batches = new ArrayList<>(batchesJsonArray.length()); - try (DbScope.Transaction transaction = ExperimentService.get().ensureTransaction()) + List runJsons = new ArrayList<>(); + for (JSONObject batchJsonObject : JsonUtil.toJSONObjectList(batchesJsonArray)) + runJsons.addAll(getRunJsons(batchJsonObject)); + try (PreparedTransforms ignored = prepareTransforms(saveHandler, protocol, runJsons); + DbScope.Transaction transaction = ExperimentService.get().ensureTransaction()) { for (int i = 0; i < batchesJsonArray.length(); i++) { @@ -102,7 +108,8 @@ private ApiResponse executeAction(ExperimentSaveHandler saveHandler, ExpProtocol List batches = new ArrayList<>(1); saveHandler.beforeSave(getViewContext(), rootJsonObject, protocol); - try (DbScope.Transaction transaction = ExperimentService.get().ensureTransaction()) + try (PreparedTransforms ignored = prepareTransforms(saveHandler, protocol, getRunJsons(batchJsonObject)); + DbScope.Transaction transaction = ExperimentService.get().ensureTransaction()) { batches.add(saveHandler.handleBatch(getViewContext(), batchJsonObject, protocol)); transaction.commit(); @@ -110,5 +117,11 @@ private ApiResponse executeAction(ExperimentSaveHandler saveHandler, ExpProtocol saveHandler.afterSave(getViewContext(), batches, protocol); return AssayJSONConverter.serializeResult(provider, protocol, batches.getFirst(), getUser()); } + + private static List getRunJsons(JSONObject batchJsonObject) + { + JSONArray runsArray = batchJsonObject.optJSONArray(AssayJSONConverter.RUNS); + return runsArray == null ? List.of() : JsonUtil.toJSONObjectList(runsArray); + } } diff --git a/assay/src/org/labkey/assay/actions/SaveAssayRunsAction.java b/assay/src/org/labkey/assay/actions/SaveAssayRunsAction.java index 05fb6a1c0dd..62c4d68427d 100644 --- a/assay/src/org/labkey/assay/actions/SaveAssayRunsAction.java +++ b/assay/src/org/labkey/assay/actions/SaveAssayRunsAction.java @@ -20,6 +20,7 @@ import org.labkey.api.action.ApiResponse; import org.labkey.api.action.SimpleApiJsonForm; import org.labkey.api.assay.AssayProvider; +import org.labkey.api.assay.transform.PreparedTransforms; import org.labkey.api.data.DbScope; import org.labkey.api.exp.api.AssayJSONConverter; import org.labkey.api.exp.api.ExpProtocol; @@ -29,6 +30,7 @@ import org.labkey.api.exp.api.ExperimentService; import org.labkey.api.security.RequiresPermission; import org.labkey.api.security.permissions.InsertPermission; +import org.labkey.api.util.JsonUtil; import org.springframework.validation.BindException; import java.util.ArrayList; @@ -61,7 +63,8 @@ protected ApiResponse executeAction(ExpProtocol protocol, SimpleApiJsonForm form private ApiResponse executeAction(ExperimentSaveHandler saveHandler, ExpProtocol protocol, AssayProvider provider, JSONArray runsJsonArray) throws Exception { List runs = new ArrayList<>(); - try (DbScope.Transaction transaction = ExperimentService.get().ensureTransaction()) + try (PreparedTransforms ignored = prepareTransforms(saveHandler, protocol, JsonUtil.toJSONObjectList(runsJsonArray)); + DbScope.Transaction transaction = ExperimentService.get().ensureTransaction()) { for (int i = 0; i < runsJsonArray.length(); i++) { From 714a64e3b1ef562fb67504b115296c657184fdda Mon Sep 17 00:00:00 2001 From: labkey-jeckels Date: Thu, 22 Oct 2026 11:53:51 -0700 Subject: [PATCH 03/11] GH Issue 1720: Let callers decide when to skip preparing transforms for background imports --- api/src/org/labkey/api/assay/AssayRunCreator.java | 4 ++-- api/src/org/labkey/api/assay/DefaultAssayRunCreator.java | 4 ++-- .../api-src/org/labkey/api/assay/DefaultAssaySaveHandler.java | 2 +- assay/src/org/labkey/assay/actions/ImportRunApiAction.java | 2 +- assay/src/org/labkey/assay/pipeline/AssayImportRunTask.java | 2 +- 5 files changed, 7 insertions(+), 7 deletions(-) diff --git a/api/src/org/labkey/api/assay/AssayRunCreator.java b/api/src/org/labkey/api/assay/AssayRunCreator.java index 01b108ca878..ef8706b3739 100644 --- a/api/src/org/labkey/api/assay/AssayRunCreator.java +++ b/api/src/org/labkey/api/assay/AssayRunCreator.java @@ -55,10 +55,10 @@ Pair saveExperimentRun(AssayRunUploadContext context, boolean forceAsync) + default @Nullable PreparedTransform prepareTransform(AssayRunUploadContext context) throws ExperimentException, ValidationException { return null; diff --git a/api/src/org/labkey/api/assay/DefaultAssayRunCreator.java b/api/src/org/labkey/api/assay/DefaultAssayRunCreator.java index bda56e87bdd..994c0ea5c80 100644 --- a/api/src/org/labkey/api/assay/DefaultAssayRunCreator.java +++ b/api/src/org/labkey/api/assay/DefaultAssayRunCreator.java @@ -134,9 +134,9 @@ public TransformResult transform(AssayRunUploadContext context, Ex } @Override - public @Nullable PreparedTransform prepareTransform(AssayRunUploadContext context, boolean forceAsync) throws ExperimentException, ValidationException + public @Nullable PreparedTransform prepareTransform(AssayRunUploadContext context) throws ExperimentException, ValidationException { - if (isBackgroundImport(context.getProvider(), context.getProtocol(), forceAsync) || !hasTransformBeforeRun(context)) + if (!hasTransformBeforeRun(context)) return null; context.init(); diff --git a/assay/api-src/org/labkey/api/assay/DefaultAssaySaveHandler.java b/assay/api-src/org/labkey/api/assay/DefaultAssaySaveHandler.java index 9542f211040..609ba08763b 100644 --- a/assay/api-src/org/labkey/api/assay/DefaultAssaySaveHandler.java +++ b/assay/api-src/org/labkey/api/assay/DefaultAssaySaveHandler.java @@ -321,7 +321,7 @@ protected boolean isPrepareTransformsSupported() AssayRunUploadContext uploadContext = createRunUploadContext(context, protocol, runJson, dataRows, inputData, outputData, Collections.emptyMap(), Collections.emptyMap()); AssayRunCreator runCreator = getProvider().getRunCreator(); - PreparedTransform prepared = runCreator.prepareTransform(uploadContext, false); + PreparedTransform prepared = runCreator.prepareTransform(uploadContext); if (prepared != null && prepared.getRunDataInputs() != null) { // saveExperimentRun adds the saved inputs back; keep only those the participant visit resolver added diff --git a/assay/src/org/labkey/assay/actions/ImportRunApiAction.java b/assay/src/org/labkey/assay/actions/ImportRunApiAction.java index 30aac021156..c9238782d50 100644 --- a/assay/src/org/labkey/assay/actions/ImportRunApiAction.java +++ b/assay/src/org/labkey/assay/actions/ImportRunApiAction.java @@ -352,7 +352,7 @@ else if (rawData != null && !rawData.isEmpty()) .create(); // GH Issue 1707: run transform scripts before the transaction so their callbacks into the server can't deadlock with it - try (PreparedTransform ignored = provider.getRunCreator().prepareTransform(uploadContext, forceAsync); + try (PreparedTransform ignored = importInBackground ? null : provider.getRunCreator().prepareTransform(uploadContext); DbScope.Transaction transaction = ExperimentService.get().getSchema().getScope().ensureTransaction(DefaultAssayRunCreator.protocolImportLocks(importInBackground))) { Map transactionDetails = getTransactionAuditDetails(); diff --git a/assay/src/org/labkey/assay/pipeline/AssayImportRunTask.java b/assay/src/org/labkey/assay/pipeline/AssayImportRunTask.java index 845f6c49dd7..7bdb69fa7d5 100644 --- a/assay/src/org/labkey/assay/pipeline/AssayImportRunTask.java +++ b/assay/src/org/labkey/assay/pipeline/AssayImportRunTask.java @@ -808,7 +808,7 @@ public RecordedActionSet run() throws PipelineJobException AssayRunUploadContext uploadContext = factory.create(); // GH Issue 1707: run transform scripts before the transaction so their callbacks into the server can't deadlock with it - try (PreparedTransform ignored = provider.getRunCreator().prepareTransform(uploadContext, false); + try (PreparedTransform ignored = provider.getRunCreator().prepareTransform(uploadContext); DbScope.Transaction tx = scope.ensureTransaction()) { String fileWatcherDescription = getJob().getDescription(); From 84e7453ceff3c2563f032fc022c977ab6917d11a Mon Sep 17 00:00:00 2001 From: labkey-jeckels Date: Thu, 22 Oct 2026 13:26:50 -0700 Subject: [PATCH 04/11] GH Issue 1720: Log transform scripts that still run inside a transaction once per assay --- .../api/assay/transform/DataTransformService.java | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/api/src/org/labkey/api/assay/transform/DataTransformService.java b/api/src/org/labkey/api/assay/transform/DataTransformService.java index 45cc73f46c3..8c02040daed 100644 --- a/api/src/org/labkey/api/assay/transform/DataTransformService.java +++ b/api/src/org/labkey/api/assay/transform/DataTransformService.java @@ -56,6 +56,7 @@ import java.util.List; import java.util.Map; import java.util.Set; +import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.TimeUnit; import static org.labkey.api.security.SecurityManager.FEATUREFLAG_ALLOW_TRANSFORM_SESSION_ID; @@ -65,6 +66,8 @@ public class DataTransformService private static final DataTransformService _instance = new DataTransformService(); private static final Logger LOG = LogHelper.getLogger(DataTransformService.class, "Assay transform scripts"); + private final Set _protocolsLoggedInTransaction = ConcurrentHashMap.newKeySet(); + private DataTransformService() {} public static DataTransformService get() @@ -299,12 +302,12 @@ public TransformResult recordTransform(AssayRunUploadContext context, @Nullab private void logIfInTransaction(AssayRunUploadContext context, TransformOperation operation) { - if (LOG.isDebugEnabled() - && operation == TransformOperation.INSERT + if (operation == TransformOperation.INSERT && context.getProvider().isTransformBeforeRunSupported(context.getProtocol()) - && ExperimentService.get().getSchema().getScope().isTransactionActive()) + && ExperimentService.get().getSchema().getScope().isTransactionActive() + && _protocolsLoggedInTransaction.add(context.getProtocol().getRowId())) { - LOG.debug("Transform script for assay '{}' is running inside a database transaction; callbacks from the script into the server can deadlock", context.getProtocol().getName(), new Throwable()); + LOG.warn("Transform script for assay '{}' is running inside a database transaction; callbacks from the script into the server can deadlock. Logged once per assay.", context.getProtocol().getName(), new Throwable()); } } From 755e1a22c348f4bfccc33278e70008496894b71d Mon Sep 17 00:00:00 2001 From: labkey-jeckels Date: Thu, 22 Oct 2026 14:15:21 -0700 Subject: [PATCH 05/11] Claude review --- .../labkey/api/assay/AssaySaveHandler.java | 2 +- .../assay/transform/DataTransformService.java | 17 -- .../exp/api/DefaultExperimentSaveHandler.java | 148 +++++++++--------- .../api/assay/DefaultAssaySaveHandler.java | 117 +++++++------- 4 files changed, 128 insertions(+), 156 deletions(-) diff --git a/api/src/org/labkey/api/assay/AssaySaveHandler.java b/api/src/org/labkey/api/assay/AssaySaveHandler.java index f71c7441b7e..091ddf2da56 100644 --- a/api/src/org/labkey/api/assay/AssaySaveHandler.java +++ b/api/src/org/labkey/api/assay/AssaySaveHandler.java @@ -77,7 +77,7 @@ public interface AssaySaveHandler extends ExperimentSaveHandler /** * Runs the transform scripts of the given runs before the save's transaction opens, so a script's callbacks into * the server can't deadlock with it. The handler records the outputs as it saves each run; runs it doesn't prepare - * run their scripts inside the transaction. Called after beforeSave. + * run their scripts inside the transaction. Don't rely on beforeSave having run: saveAssayRuns never calls it. */ default @NotNull PreparedTransforms prepareTransforms(ViewContext context, ExpProtocol protocol, List runJsons) throws ExperimentException, ValidationException { diff --git a/api/src/org/labkey/api/assay/transform/DataTransformService.java b/api/src/org/labkey/api/assay/transform/DataTransformService.java index 8c02040daed..78fa0d5c0f2 100644 --- a/api/src/org/labkey/api/assay/transform/DataTransformService.java +++ b/api/src/org/labkey/api/assay/transform/DataTransformService.java @@ -27,7 +27,6 @@ import org.labkey.api.exp.api.ExpData; import org.labkey.api.exp.api.ExpProtocol; import org.labkey.api.exp.api.ExpRun; -import org.labkey.api.exp.api.ExperimentService; import org.labkey.api.exp.property.DomainProperty; import org.labkey.api.query.ValidationException; import org.labkey.api.reader.Readers; @@ -56,7 +55,6 @@ import java.util.List; import java.util.Map; import java.util.Set; -import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.TimeUnit; import static org.labkey.api.security.SecurityManager.FEATUREFLAG_ALLOW_TRANSFORM_SESSION_ID; @@ -66,8 +64,6 @@ public class DataTransformService private static final DataTransformService _instance = new DataTransformService(); private static final Logger LOG = LogHelper.getLogger(DataTransformService.class, "Assay transform scripts"); - private final Set _protocolsLoggedInTransaction = ConcurrentHashMap.newKeySet(); - private DataTransformService() {} public static DataTransformService get() @@ -157,8 +153,6 @@ public TransformResult transformAndValidate( "the configuration for this assay design."); } - logIfInTransaction(context, operation); - if (operation == TransformOperation.UPDATE) SimpleMetricsService.get().increment("Assay", "transformScript", "executionCountForUpdate"); if (operation == TransformOperation.INSERT) @@ -300,17 +294,6 @@ public TransformResult recordTransform(AssayRunUploadContext context, @Nullab return prepared.getResult(); } - private void logIfInTransaction(AssayRunUploadContext context, TransformOperation operation) - { - if (operation == TransformOperation.INSERT - && context.getProvider().isTransformBeforeRunSupported(context.getProtocol()) - && ExperimentService.get().getSchema().getScope().isTransactionActive() - && _protocolsLoggedInTransaction.add(context.getProtocol().getRowId())) - { - LOG.warn("Transform script for assay '{}' is running inside a database transaction; callbacks from the script into the server can deadlock. Logged once per assay.", context.getProtocol().getName(), new Throwable()); - } - } - public void addStandardParameters(@Nullable HttpServletRequest request, @Nullable Container container, @Nullable FileLike scriptFile, @Nullable String apiKey, @NotNull Map paramMap) { if (scriptFile != null) diff --git a/api/src/org/labkey/api/exp/api/DefaultExperimentSaveHandler.java b/api/src/org/labkey/api/exp/api/DefaultExperimentSaveHandler.java index 7f1b2cfb36c..8a7b603e690 100644 --- a/api/src/org/labkey/api/exp/api/DefaultExperimentSaveHandler.java +++ b/api/src/org/labkey/api/exp/api/DefaultExperimentSaveHandler.java @@ -282,26 +282,8 @@ protected JSONArray serializeMaterialOutputs(ViewContext context, ExpRun run, JS @Override public ExpRun handleRun(ViewContext context, JSONObject runJsonObject, ExpProtocol protocol, @Nullable ExpExperiment batch) throws JSONException, ValidationException, ExperimentException { - ExpRun run; - - if (!runJsonObject.isNull(ExperimentJSONConverter.ID)) - { - int runId = runJsonObject.getInt(ExperimentJSONConverter.ID); - run = ExperimentService.get().getExpRun(runId); - if (run == null) - { - throw new NotFoundException("Could not find experiment run " + runId); - } - if (!run.getContainer().equals(context.getContainer())) - { - throw new NotFoundException("Could not find experiment run " + runId + " in folder " + context.getContainer()); - } - if (null != batch && !batch.getContainer().equals(context.getContainer())) - { - throw new NotFoundException("Could not find experiment run " + runId + " in folder " + context.getContainer()); - } - } - else + ExpRun run = lookupRun(context, runJsonObject, batch); + if (run == null) { String name = runJsonObject.optString(ExperimentJSONConverter.NAME, null); run = createRun(name, context.getContainer(), protocol); @@ -316,10 +298,7 @@ public ExpRun handleRun(ViewContext context, JSONObject runJsonObject, ExpProtoc { handleStandardProperties(context, runJsonObject, run, getRunDomainProperties(protocol)); - if (runJsonObject.has(AssayJSONConverter.DATA_ROWS) || - runJsonObject.has(ExperimentJSONConverter.DATA_INPUTS) || - runJsonObject.has(ExperimentJSONConverter.MATERIAL_INPUTS) || - runJsonObject.has(ExperimentJSONConverter.DATA_OUTPUTS)) + if (hasRunContent(runJsonObject)) { JSONArray dataRows = serializeRunData(context, run, runJsonObject, protocol, batch); JSONArray dataInputs = serializeDataInputs(context, run, runJsonObject, protocol, batch); @@ -340,6 +319,38 @@ public ExpRun handleRun(ViewContext context, JSONObject runJsonObject, ExpProtoc return run; } + /** The existing run the post identifies, or null when it posts a new run. */ + protected @Nullable ExpRun lookupRun(ViewContext context, JSONObject runJsonObject, @Nullable ExpExperiment batch) + { + if (runJsonObject.isNull(ExperimentJSONConverter.ID)) + return null; + + int runId = runJsonObject.getInt(ExperimentJSONConverter.ID); + ExpRun run = ExperimentService.get().getExpRun(runId); + if (run == null) + { + throw new NotFoundException("Could not find experiment run " + runId); + } + if (!run.getContainer().equals(context.getContainer())) + { + throw new NotFoundException("Could not find experiment run " + runId + " in folder " + context.getContainer()); + } + if (null != batch && !batch.getContainer().equals(context.getContainer())) + { + throw new NotFoundException("Could not find experiment run " + runId + " in folder " + context.getContainer()); + } + return run; + } + + /** Whether the post replaces the run's data and protocol applications rather than only its properties. */ + protected static boolean hasRunContent(JSONObject runJsonObject) + { + return runJsonObject.has(AssayJSONConverter.DATA_ROWS) || + runJsonObject.has(ExperimentJSONConverter.DATA_INPUTS) || + runJsonObject.has(ExperimentJSONConverter.MATERIAL_INPUTS) || + runJsonObject.has(ExperimentJSONConverter.DATA_OUTPUTS); + } + @Override public void handleProperties(ViewContext context, ExpObject object, List dps, JSONObject propertiesJsonObject) throws ValidationException, JSONException { @@ -555,58 +566,10 @@ protected ExpExperiment saveExperimentRun(ViewContext context, ExpProtocol proto @Override public ExpMaterial handleMaterial(ViewContext context, JSONObject materialObject) throws ValidationException - { - MaterialLookup lookup = lookupMaterial(context, materialObject); - if (lookup == null) - return null; - - ExpSampleType sampleType = lookup.sampleType(); - ExpMaterial material = lookup.material(); - boolean createdHere = material == null; - if (createdHere) - { - material = createMaterial(context, sampleType, lookup.name()); - createdMaterialLsids.add(material.getLSID()); - } - - if (materialObject.has(ExperimentJSONConverter.PROPERTIES)) - { - JSONObject materialProperties = materialObject.getJSONObject(ExperimentJSONConverter.PROPERTIES); - // Treat an empty properties collection as if there were no property map at all. - // To delete a property, include a property map with that property and set its value to null. - if (!materialProperties.isEmpty()) - { - List dps = sampleType != null ? sampleType.getDomain().getProperties() : Collections.emptyList(); - - if (propertiesWouldChange(context, material, dps, materialProperties)) - { - // A sample created by this request needs only insert rights; an existing one needs edit rights. - // The status check below applies either way. - Class editPerm = createdHere - ? InsertPermission.class - : SampleTypeService.SampleOperations.EditMetadata.getPermissionClass(); - if (editPerm == null || !material.getContainer().hasPermission(context.getUser(), editPerm)) - throw new UnauthorizedException("User does not have permission to edit sample '" + material.getName() + "'"); - if (!material.isOperationPermitted(SampleTypeService.SampleOperations.EditMetadata)) - throw new UnauthorizedException(SampleTypeService.get().getOperationNotPermittedMessage( - List.of(material), SampleTypeService.SampleOperations.EditMetadata)); - } - - handleProperties(context, material, dps, materialProperties); - } - } - - return material; - } - - /** An existing material, or, when material is null, the sample type and name of the one handleMaterial would create. */ - protected record MaterialLookup(@Nullable ExpMaterial material, @Nullable ExpSampleType sampleType, @NotNull String name) {} - - /** Resolves the material handleMaterial would use, without writing anything. Returns null when there's nothing to resolve. */ - protected @Nullable MaterialLookup lookupMaterial(ViewContext context, JSONObject materialObject) throws ValidationException { ExpSampleType sampleType = null; ExpMaterial material = null; + boolean createdHere = false; if (materialObject.has(ExperimentJSONConverter.ID)) { int materialRowId = materialObject.getInt(ExperimentJSONConverter.ID); @@ -689,11 +652,46 @@ else if (sampleType != null && !sampleType.hasNameAsIdCol() && materialObject.ha if (material != null && !material.getContainer().hasPermission(context.getUser(), ReadPermission.class)) throw new NotFoundException("Could not find material '" + materialName + "'"); - return new MaterialLookup(material, sampleType, materialName); + if (material == null) + { + material = createMaterial(context, sampleType, materialName); + createdHere = true; + createdMaterialLsids.add(material.getLSID()); + } } } - return material == null ? null : new MaterialLookup(material, sampleType, material.getName()); + if (material == null) + return null; + + if (materialObject.has(ExperimentJSONConverter.PROPERTIES)) + { + JSONObject materialProperties = materialObject.getJSONObject(ExperimentJSONConverter.PROPERTIES); + // Treat an empty properties collection as if there were no property map at all. + // To delete a property, include a property map with that property and set its value to null. + if (!materialProperties.isEmpty()) + { + List dps = sampleType != null ? sampleType.getDomain().getProperties() : Collections.emptyList(); + + if (propertiesWouldChange(context, material, dps, materialProperties)) + { + // A sample created by this request needs only insert rights; an existing one needs edit rights. + // The status check below applies either way. + Class editPerm = createdHere + ? InsertPermission.class + : SampleTypeService.SampleOperations.EditMetadata.getPermissionClass(); + if (editPerm == null || !material.getContainer().hasPermission(context.getUser(), editPerm)) + throw new UnauthorizedException("User does not have permission to edit sample '" + material.getName() + "'"); + if (!material.isOperationPermitted(SampleTypeService.SampleOperations.EditMetadata)) + throw new UnauthorizedException(SampleTypeService.get().getOperationNotPermittedMessage( + List.of(material), SampleTypeService.SampleOperations.EditMetadata)); + } + + handleProperties(context, material, dps, materialProperties); + } + } + + return material; } private boolean propertiesWouldChange(ViewContext context, ExpMaterial material, List dps, JSONObject submitted) throws ValidationException diff --git a/assay/api-src/org/labkey/api/assay/DefaultAssaySaveHandler.java b/assay/api-src/org/labkey/api/assay/DefaultAssaySaveHandler.java index 609ba08763b..f9a73e7cfa2 100644 --- a/assay/api-src/org/labkey/api/assay/DefaultAssaySaveHandler.java +++ b/assay/api-src/org/labkey/api/assay/DefaultAssaySaveHandler.java @@ -63,6 +63,7 @@ public class DefaultAssaySaveHandler extends DefaultExperimentSaveHandler implem protected static final Logger LOG = LogManager.getLogger(DefaultAssaySaveHandler.class); protected AssayProvider _provider; private PreparedTransforms _preparedTransforms = new PreparedTransforms(); + private final Map _serializedRuns = new HashMap<>(); @Override public AssayProvider getProvider() @@ -102,7 +103,7 @@ protected JSONArray serializeRunData(ViewContext context, ExpRun run, JSONObject { // Client didn't post the rows so reuse the values that are currently attached to the run // Inefficient but easy - JSONObject serializedRun = AssayJSONConverter.serializeRun(run, _provider, protocol, context.getUser(), ExperimentJSONConverter.DEFAULT_SETTINGS); + JSONObject serializedRun = serializeExistingRun(context, run, protocol); dataRows = serializedRun.getJSONArray(AssayJSONConverter.DATA_ROWS); } else @@ -118,7 +119,7 @@ protected JSONArray serializeDataInputs(ViewContext context, ExpRun run, JSONObj JSONArray dataInputs; if (!runJson.has(ExperimentJSONConverter.DATA_INPUTS)) { - JSONObject serializedRun = AssayJSONConverter.serializeRun(run, _provider, protocol, context.getUser(), ExperimentJSONConverter.DEFAULT_SETTINGS); + JSONObject serializedRun = serializeExistingRun(context, run, protocol); dataInputs = serializedRun.getJSONArray(ExperimentJSONConverter.DATA_INPUTS); } else @@ -134,7 +135,7 @@ protected JSONArray serializeMaterialInputs(ViewContext context, ExpRun run, JSO JSONArray materialInputs; if (!runJson.has(ExperimentJSONConverter.MATERIAL_INPUTS)) { - JSONObject serializedRun = AssayJSONConverter.serializeRun(run, _provider, protocol, context.getUser(), ExperimentJSONConverter.DEFAULT_SETTINGS); + JSONObject serializedRun = serializeExistingRun(context, run, protocol); materialInputs = serializedRun.getJSONArray(ExperimentJSONConverter.MATERIAL_INPUTS); } else @@ -150,7 +151,7 @@ protected JSONArray serializeDataOutputs(ViewContext context, ExpRun run, JSONOb JSONArray dataOutputs; if (!runJson.has(ExperimentJSONConverter.DATA_OUTPUTS)) { - JSONObject serializedRun = AssayJSONConverter.serializeRun(run, _provider, protocol, context.getUser(), ExperimentJSONConverter.DEFAULT_SETTINGS); + JSONObject serializedRun = serializeExistingRun(context, run, protocol); dataOutputs = serializedRun.getJSONArray(ExperimentJSONConverter.DATA_OUTPUTS); } else @@ -166,7 +167,7 @@ protected JSONArray serializeMaterialOutputs(ViewContext context, ExpRun run, JS JSONArray materialOutputs; if (!runJson.has(ExperimentJSONConverter.MATERIAL_OUTPUTS)) { - JSONObject serializedRun = AssayJSONConverter.serializeRun(run, _provider, protocol, context.getUser(), ExperimentJSONConverter.DEFAULT_SETTINGS); + JSONObject serializedRun = serializeExistingRun(context, run, protocol); materialOutputs = serializedRun.getJSONArray(ExperimentJSONConverter.MATERIAL_OUTPUTS); } else @@ -176,6 +177,13 @@ protected JSONArray serializeMaterialOutputs(ViewContext context, ExpRun run, JS return materialOutputs; } + /** Serializes a run once for all the serialize methods, across both passes of a save. */ + private JSONObject serializeExistingRun(ViewContext context, ExpRun run, ExpProtocol protocol) + { + return _serializedRuns.computeIfAbsent(run.getLSID(), lsid -> + AssayJSONConverter.serializeRun(run, _provider, protocol, context.getUser(), ExperimentJSONConverter.DEFAULT_SETTINGS)); + } + @Override protected void clearOutputDatas(ViewContext context, ExpRun run) { @@ -202,45 +210,36 @@ protected ExpExperiment saveExperimentRun( Map outputMaterial ) throws ExperimentException, ValidationException { + // Done with this run's serialization; a later post of the same run must see this save + _serializedRuns.remove(run.getLSID()); + List> dataRows = convertRunData(dataArray, run.getContainer(), protocol); - ExpData tsvData = DefaultAssayRunCreator.generateResultData(context.getUser(), run.getContainer(), getProvider(), dataRows, (Map) outputData, null); + if (skipsAssayRun(dataRows, !outputData.isEmpty())) + return null; + + DefaultAssayRunCreator.generateResultData(context.getUser(), run.getContainer(), getProvider(), dataRows, (Map) outputData, null); + AssayRunUploadContext uploadContext = createRunUploadContext(context, protocol, runJson, dataRows, + inputData, outputData, inputMaterial, outputMaterial); - // CONSIDER: Is this block still needed? - // Try to find a data object to attach our data rows to - if (tsvData == null && !outputData.isEmpty()) + PreparedTransform prepared = _preparedTransforms.get(runJson); + if (prepared != null && uploadContext != null) { - tsvData = outputData.keySet().iterator().next(); - for (Map.Entry entry : outputData.entrySet()) + if (prepared.getRunDataInputs() != null) { - // Prefer the generic "Data" role if we have one - if (ExpDataRunInput.DEFAULT_ROLE.equals(entry.getValue())) - { - tsvData = entry.getKey(); - break; - } + Map runDataInputs = new HashMap<>(prepared.getRunDataInputs()); + runDataInputs.putAll(inputData); + prepared.setRunDataInputs(runDataInputs); } + uploadContext.setPreparedTransform(prepared); } - if (tsvData != null) - { - AssayRunUploadContext uploadContext = createRunUploadContext(context, protocol, runJson, dataRows, - inputData, outputData, inputMaterial, outputMaterial); - - PreparedTransform prepared = _preparedTransforms.get(runJson); - if (prepared != null && uploadContext != null) - { - if (prepared.getRunDataInputs() != null) - { - Map runDataInputs = new HashMap<>(prepared.getRunDataInputs()); - runDataInputs.putAll(inputData); - prepared.setRunDataInputs(runDataInputs); - } - uploadContext.setPreparedTransform(prepared); - } + return saveAssayRun(uploadContext, batch, run, BaseViewAction.getTransactionAuditDetails(context)); + } - return saveAssayRun(uploadContext, batch, run, BaseViewAction.getTransactionAuditDetails(context)); - } - return null; + /** Whether saveExperimentRun skips the assay save, and with it the transform scripts. */ + private static boolean skipsAssayRun(List> dataRows, boolean hasOutputData) + { + return dataRows.isEmpty() && !hasOutputData; } /** Whether runs are saved exactly as this class saves them, which the first pass of prepareTransforms assumes. */ @@ -253,7 +252,9 @@ protected boolean isPrepareTransformsSupported() public @NotNull PreparedTransforms prepareTransforms(ViewContext context, ExpProtocol protocol, List runJsons) throws ExperimentException, ValidationException { PreparedTransforms preparedTransforms = new PreparedTransforms(); + // Inside a caller's transaction the scripts can't run outside it, so leave them to the save if (!isPrepareTransformsSupported() + || ExperimentService.get().getSchema().getScope().isTransactionActive() || !getProvider().isTransformBeforeRunSupported(protocol) || getProvider().getValidationAndAnalysisScripts(protocol, AssayProvider.Scope.ALL).isEmpty()) return preparedTransforms; @@ -281,45 +282,26 @@ protected boolean isPrepareTransformsSupported() /** Builds the upload context handleRun would build for this run, without writing anything, and runs its transform scripts. */ private @Nullable PreparedTransform prepareTransform(ViewContext context, ExpProtocol protocol, JSONObject runJson) throws ExperimentException, ValidationException { - if (!runJson.has(AssayJSONConverter.DATA_ROWS) && !runJson.has(ExperimentJSONConverter.DATA_INPUTS) - && !runJson.has(ExperimentJSONConverter.MATERIAL_INPUTS) && !runJson.has(ExperimentJSONConverter.DATA_OUTPUTS)) + ExpRun existingRun = lookupRun(context, runJson, null); + if (!hasRunContent(runJson)) return null; - ExpRun existingRun = null; - if (!runJson.isNull(ExperimentJSONConverter.ID)) - { - existingRun = ExperimentService.get().getExpRun(runJson.getInt(ExperimentJSONConverter.ID)); - // Leave the error to handleRun - if (existingRun == null || !existingRun.getContainer().equals(context.getContainer())) - return null; - } - JSONArray dataArray = existingRun != null ? serializeRunData(context, existingRun, runJson, protocol, null) : postedArray(runJson, AssayJSONConverter.DATA_ROWS); JSONArray dataInputs = existingRun != null ? serializeDataInputs(context, existingRun, runJson, protocol, null) : postedArray(runJson, ExperimentJSONConverter.DATA_INPUTS); - JSONArray materialInputs = existingRun != null ? serializeMaterialInputs(context, existingRun, runJson, protocol, null) : postedArray(runJson, ExperimentJSONConverter.MATERIAL_INPUTS); JSONArray dataOutputs = existingRun != null ? serializeDataOutputs(context, existingRun, runJson, protocol, null) : postedArray(runJson, ExperimentJSONConverter.DATA_OUTPUTS); - JSONArray materialOutputs = existingRun != null ? serializeMaterialOutputs(context, existingRun, runJson, protocol, null) : postedArray(runJson, ExperimentJSONConverter.MATERIAL_OUTPUTS); + Map newDataByUrl = new HashMap<>(); Map inputData = new HashMap<>(); for (JSONObject dataObject : JsonUtil.toJSONObjectList(dataInputs)) - inputData.put(lookupData(context, dataObject), dataObject.optString(ExperimentJSONConverter.ROLE, ExpDataRunInput.DEFAULT_ROLE)); - Map outputData = new HashMap<>(); - for (JSONObject dataObject : JsonUtil.toJSONObjectList(dataOutputs)) - outputData.put(lookupData(context, dataObject), dataObject.optString(ExperimentJSONConverter.ROLE, ExpDataRunInput.DEFAULT_ROLE)); - - // Scripts don't see the run's materials, so these lookups only reject bad references before any script runs - for (JSONObject materialObject : JsonUtil.toJSONObjectList(materialInputs)) - lookupMaterial(context, materialObject); - for (JSONObject materialObject : JsonUtil.toJSONObjectList(materialOutputs)) - lookupMaterial(context, materialObject); - - // saveExperimentRun skips the assay save, and with it the scripts, when there are neither result rows nor outputs + inputData.put(lookupData(context, dataObject, newDataByUrl), dataObject.optString(ExperimentJSONConverter.ROLE, ExpDataRunInput.DEFAULT_ROLE)); + List> dataRows = convertRunData(dataArray, context.getContainer(), protocol); - if (dataRows.isEmpty() && outputData.isEmpty()) + if (skipsAssayRun(dataRows, !dataOutputs.isEmpty())) return null; + // Scripts never see output data, so the outputs are only counted, not resolved AssayRunUploadContext uploadContext = createRunUploadContext(context, protocol, runJson, dataRows, - inputData, outputData, Collections.emptyMap(), Collections.emptyMap()); + inputData, Collections.emptyMap(), Collections.emptyMap(), Collections.emptyMap()); AssayRunCreator runCreator = getProvider().getRunCreator(); PreparedTransform prepared = runCreator.prepareTransform(uploadContext); if (prepared != null && prepared.getRunDataInputs() != null) @@ -332,6 +314,15 @@ protected boolean isPrepareTransformsSupported() return prepared; } + /** Each lookup of a not-yet-saved file returns a new data object, but handleData saves the first and reuses it for the rest. */ + private ExpData lookupData(ViewContext context, JSONObject dataObject, Map newDataByUrl) + { + ExpData data = lookupData(context, dataObject); + if (data.getRowId() == 0 && data.getDataFileUrl() != null) + return newDataByUrl.computeIfAbsent(data.getDataFileUrl(), url -> data); + return data; + } + private static JSONArray postedArray(JSONObject runJson, String key) { return runJson.has(key) ? runJson.getJSONArray(key) : new JSONArray(); From b09dffc6361656ad40c76e2e054f73f53c7138ef Mon Sep 17 00:00:00 2001 From: labkey-jeckels Date: Thu, 22 Oct 2026 15:55:51 -0700 Subject: [PATCH 06/11] GH Issue 1720: Reject stale, duplicate, and missing-batch saves before transform output is recorded A run edited while its scripts ran now fails the save instead of overwriting the edit, duplicate runs and missing batches are rejected before any script runs, and prepared inputs are keyed by file. --- .../api/assay/DefaultAssaySaveHandler.java | 78 ++++++++++++++++--- .../assay/actions/BaseProtocolAPIAction.java | 11 +++ .../assay/actions/SaveAssayBatchAction.java | 13 ++++ 3 files changed, 90 insertions(+), 12 deletions(-) diff --git a/assay/api-src/org/labkey/api/assay/DefaultAssaySaveHandler.java b/assay/api-src/org/labkey/api/assay/DefaultAssaySaveHandler.java index f9a73e7cfa2..604037f838a 100644 --- a/assay/api-src/org/labkey/api/assay/DefaultAssaySaveHandler.java +++ b/assay/api-src/org/labkey/api/assay/DefaultAssaySaveHandler.java @@ -55,8 +55,11 @@ import java.util.Collection; import java.util.Collections; import java.util.HashMap; +import java.util.IdentityHashMap; import java.util.List; import java.util.Map; +import java.util.Set; +import java.util.stream.Collectors; public class DefaultAssaySaveHandler extends DefaultExperimentSaveHandler implements AssaySaveHandler { @@ -64,6 +67,9 @@ public class DefaultAssaySaveHandler extends DefaultExperimentSaveHandler implem protected AssayProvider _provider; private PreparedTransforms _preparedTransforms = new PreparedTransforms(); private final Map _serializedRuns = new HashMap<>(); + /** For each prepared run, the parts of its existing serialization its scripts read before the transaction. */ + private final Map _preparedSerializations = new IdentityHashMap<>(); + private static final List PREPARED_KEYS = List.of(AssayJSONConverter.DATA_ROWS, ExperimentJSONConverter.DATA_INPUTS, ExperimentJSONConverter.DATA_OUTPUTS); @Override public AssayProvider getProvider() @@ -177,7 +183,7 @@ protected JSONArray serializeMaterialOutputs(ViewContext context, ExpRun run, JS return materialOutputs; } - /** Serializes a run once for all the serialize methods, across both passes of a save. */ + /** Serializes a run once for all the serialize methods within one pass of a save. */ private JSONObject serializeExistingRun(ViewContext context, ExpRun run, ExpProtocol protocol) { return _serializedRuns.computeIfAbsent(run.getLSID(), lsid -> @@ -210,8 +216,8 @@ protected ExpExperiment saveExperimentRun( Map outputMaterial ) throws ExperimentException, ValidationException { - // Done with this run's serialization; a later post of the same run must see this save - _serializedRuns.remove(run.getLSID()); + // This run's read inside the transaction, to check against what its scripts saw + JSONObject serializedRun = _serializedRuns.remove(run.getLSID()); List> dataRows = convertRunData(dataArray, run.getContainer(), protocol); if (skipsAssayRun(dataRows, !outputData.isEmpty())) @@ -224,6 +230,10 @@ protected ExpExperiment saveExperimentRun( PreparedTransform prepared = _preparedTransforms.get(runJson); if (prepared != null && uploadContext != null) { + // Rerunning the scripts inside the transaction would reopen the deadlock they ran early to avoid + if (!matchesPrepared(runJson, serializedRun)) + throw new ValidationException("Run " + run.getRowId() + " changed while its transform scripts ran. Retry the save."); + if (prepared.getRunDataInputs() != null) { Map runDataInputs = new HashMap<>(prepared.getRunDataInputs()); @@ -236,6 +246,27 @@ protected ExpExperiment saveExperimentRun( return saveAssayRun(uploadContext, batch, run, BaseViewAction.getTransactionAuditDetails(context)); } + /** Whether the run, read inside the transaction, still holds what its scripts saw before it. */ + private boolean matchesPrepared(JSONObject runJson, @Nullable JSONObject serializedRun) + { + JSONObject seen = _preparedSerializations.get(runJson); + if (seen == null) + return true; + return serializedRun != null && seen.similar(readFromSerialization(runJson, serializedRun)); + } + + /** The arrays read from an existing run's serialization because the client didn't post them. */ + private static JSONObject readFromSerialization(JSONObject runJson, JSONObject serializedRun) + { + JSONObject read = new JSONObject(); + for (String key : PREPARED_KEYS) + { + if (!runJson.has(key)) + read.put(key, serializedRun.getJSONArray(key)); + } + return read; + } + /** Whether saveExperimentRun skips the assay save, and with it the transform scripts. */ private static boolean skipsAssayRun(List> dataRows, boolean hasOutputData) { @@ -286,14 +317,33 @@ protected boolean isPrepareTransformsSupported() if (!hasRunContent(runJson)) return null; + try + { + PreparedTransform prepared = prepareTransform(context, protocol, runJson, existingRun); + JSONObject serializedRun = existingRun == null ? null : _serializedRuns.get(existingRun.getLSID()); + if (prepared != null && serializedRun != null) + _preparedSerializations.put(runJson, readFromSerialization(runJson, serializedRun)); + return prepared; + } + finally + { + // The save pass reads the run again inside its transaction + if (existingRun != null) + _serializedRuns.remove(existingRun.getLSID()); + } + } + + private @Nullable PreparedTransform prepareTransform(ViewContext context, ExpProtocol protocol, JSONObject runJson, @Nullable ExpRun existingRun) throws ExperimentException, ValidationException + { JSONArray dataArray = existingRun != null ? serializeRunData(context, existingRun, runJson, protocol, null) : postedArray(runJson, AssayJSONConverter.DATA_ROWS); JSONArray dataInputs = existingRun != null ? serializeDataInputs(context, existingRun, runJson, protocol, null) : postedArray(runJson, ExperimentJSONConverter.DATA_INPUTS); JSONArray dataOutputs = existingRun != null ? serializeDataOutputs(context, existingRun, runJson, protocol, null) : postedArray(runJson, ExperimentJSONConverter.DATA_OUTPUTS); - Map newDataByUrl = new HashMap<>(); - Map inputData = new HashMap<>(); + // Unsaved data all have rowId 0 and can share an LSID, so key by file instead of ExpData equality + Map dataByKey = new HashMap<>(); + Map inputData = new IdentityHashMap<>(); for (JSONObject dataObject : JsonUtil.toJSONObjectList(dataInputs)) - inputData.put(lookupData(context, dataObject, newDataByUrl), dataObject.optString(ExperimentJSONConverter.ROLE, ExpDataRunInput.DEFAULT_ROLE)); + inputData.put(lookupData(context, dataObject, dataByKey), dataObject.optString(ExperimentJSONConverter.ROLE, ExpDataRunInput.DEFAULT_ROLE)); List> dataRows = convertRunData(dataArray, context.getContainer(), protocol); if (skipsAssayRun(dataRows, !dataOutputs.isEmpty())) @@ -307,20 +357,24 @@ protected boolean isPrepareTransformsSupported() if (prepared != null && prepared.getRunDataInputs() != null) { // saveExperimentRun adds the saved inputs back; keep only those the participant visit resolver added - Map resolverInputs = new HashMap<>(prepared.getRunDataInputs()); - resolverInputs.keySet().removeAll(inputData.keySet()); + Set postedKeys = inputData.keySet().stream().map(DefaultAssaySaveHandler::dataKey).collect(Collectors.toSet()); + Map resolverInputs = new IdentityHashMap<>(prepared.getRunDataInputs()); + resolverInputs.keySet().removeIf(data -> postedKeys.contains(dataKey(data))); prepared.setRunDataInputs(resolverInputs); } return prepared; } /** Each lookup of a not-yet-saved file returns a new data object, but handleData saves the first and reuses it for the rest. */ - private ExpData lookupData(ViewContext context, JSONObject dataObject, Map newDataByUrl) + private ExpData lookupData(ViewContext context, JSONObject dataObject, Map dataByKey) { ExpData data = lookupData(context, dataObject); - if (data.getRowId() == 0 && data.getDataFileUrl() != null) - return newDataByUrl.computeIfAbsent(data.getDataFileUrl(), url -> data); - return data; + return dataByKey.computeIfAbsent(dataKey(data), key -> data); + } + + private static Object dataKey(ExpData data) + { + return data.getDataFileUrl() != null ? data.getDataFileUrl() : data.getRowId(); } private static JSONArray postedArray(JSONObject runJson, String key) diff --git a/assay/src/org/labkey/assay/actions/BaseProtocolAPIAction.java b/assay/src/org/labkey/assay/actions/BaseProtocolAPIAction.java index c6ce1c8ab54..909de067a0a 100644 --- a/assay/src/org/labkey/assay/actions/BaseProtocolAPIAction.java +++ b/assay/src/org/labkey/assay/actions/BaseProtocolAPIAction.java @@ -19,6 +19,7 @@ import org.jetbrains.annotations.Nullable; import org.json.JSONObject; import org.labkey.api.action.ApiResponse; +import org.labkey.api.action.ApiUsageException; import org.labkey.api.action.MutatingApiAction; import org.labkey.api.action.SimpleApiJsonForm; import org.labkey.api.assay.AssayProvider; @@ -38,7 +39,9 @@ import org.labkey.api.view.NotFoundException; import org.springframework.validation.BindException; +import java.util.HashSet; import java.util.List; +import java.util.Set; import java.util.stream.Collectors; public abstract class BaseProtocolAPIAction
extends MutatingApiAction @@ -160,6 +163,14 @@ public ExperimentSaveHandler getExperimentSaveHandler(@Nullable AssayProvider pr /** GH Issue 1720: run transform scripts before the save's transaction so their callbacks into the server can't deadlock with it */ protected PreparedTransforms prepareTransforms(ExperimentSaveHandler saveHandler, ExpProtocol protocol, List runJsons) throws ExperimentException, ValidationException { + // A repeated run's later posts would save over what its scripts saw before the transaction + Set runIds = new HashSet<>(); + for (JSONObject runJson : runJsons) + { + if (!runJson.isNull(ExperimentJSONConverter.ID) && !runIds.add(runJson.getInt(ExperimentJSONConverter.ID))) + throw new ApiUsageException("Run " + runJson.getInt(ExperimentJSONConverter.ID) + " appears more than once in the request"); + } + if (saveHandler instanceof AssaySaveHandler assaySaveHandler) return assaySaveHandler.prepareTransforms(getViewContext(), protocol, runJsons); return new PreparedTransforms(); diff --git a/assay/src/org/labkey/assay/actions/SaveAssayBatchAction.java b/assay/src/org/labkey/assay/actions/SaveAssayBatchAction.java index b5949c1f38e..6b06d32b82e 100644 --- a/assay/src/org/labkey/assay/actions/SaveAssayBatchAction.java +++ b/assay/src/org/labkey/assay/actions/SaveAssayBatchAction.java @@ -24,8 +24,10 @@ import org.labkey.api.assay.transform.PreparedTransforms; import org.labkey.api.data.DbScope; import org.labkey.api.exp.api.AssayJSONConverter; +import org.labkey.api.exp.api.DefaultExperimentSaveHandler; import org.labkey.api.exp.api.ExpExperiment; import org.labkey.api.exp.api.ExpProtocol; +import org.labkey.api.exp.api.ExperimentJSONConverter; import org.labkey.api.exp.api.ExperimentSaveHandler; import org.labkey.api.exp.api.ExperimentService; import org.labkey.api.security.RequiresPermission; @@ -85,7 +87,10 @@ private ApiResponse executeAction(ExperimentSaveHandler saveHandler, ExpProtocol List batches = new ArrayList<>(batchesJsonArray.length()); List runJsons = new ArrayList<>(); for (JSONObject batchJsonObject : JsonUtil.toJSONObjectList(batchesJsonArray)) + { + checkBatch(batchJsonObject); runJsons.addAll(getRunJsons(batchJsonObject)); + } try (PreparedTransforms ignored = prepareTransforms(saveHandler, protocol, runJsons); DbScope.Transaction transaction = ExperimentService.get().ensureTransaction()) { @@ -108,6 +113,7 @@ private ApiResponse executeAction(ExperimentSaveHandler saveHandler, ExpProtocol List batches = new ArrayList<>(1); saveHandler.beforeSave(getViewContext(), rootJsonObject, protocol); + checkBatch(batchJsonObject); try (PreparedTransforms ignored = prepareTransforms(saveHandler, protocol, getRunJsons(batchJsonObject)); DbScope.Transaction transaction = ExperimentService.get().ensureTransaction()) { @@ -118,6 +124,13 @@ private ApiResponse executeAction(ExperimentSaveHandler saveHandler, ExpProtocol return AssayJSONConverter.serializeResult(provider, protocol, batches.getFirst(), getUser()); } + /** Rejects a missing batch before any transform script runs. */ + private void checkBatch(JSONObject batchJsonObject) + { + if (!batchJsonObject.isNull(ExperimentJSONConverter.ID)) + DefaultExperimentSaveHandler.lookupBatch(getContainer(), batchJsonObject.getInt(ExperimentJSONConverter.ID)); + } + private static List getRunJsons(JSONObject batchJsonObject) { JSONArray runsArray = batchJsonObject.optJSONArray(AssayJSONConverter.RUNS); From e50f2d20eab3ed2d150563f6a8466f3a271dc4cb Mon Sep 17 00:00:00 2001 From: labkey-jeckels Date: Fri, 23 Oct 2026 17:53:32 -0700 Subject: [PATCH 07/11] GH Issue 1707: Record file audit events from transform prep in the import transaction Events for files saved before the transform runs are held on the PreparedTransform; a failed import writes them only for files that still exist. --- .../org/labkey/api/assay/AssayFileWriter.java | 7 +- .../api/assay/AssayRunUploadContext.java | 12 ++ .../api/assay/AssayRunUploadContextImpl.java | 13 ++ .../api/assay/DefaultAssayRunCreator.java | 20 ++- .../api/assay/actions/AssayRunUploadForm.java | 13 ++ .../assay/transform/PreparedTransform.java | 130 +++++++++++++++++- assay/src/org/labkey/assay/AssayModule.java | 4 +- 7 files changed, 193 insertions(+), 6 deletions(-) diff --git a/api/src/org/labkey/api/assay/AssayFileWriter.java b/api/src/org/labkey/api/assay/AssayFileWriter.java index 087a2378f05..70a3a62b3f1 100644 --- a/api/src/org/labkey/api/assay/AssayFileWriter.java +++ b/api/src/org/labkey/api/assay/AssayFileWriter.java @@ -20,6 +20,7 @@ import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; import org.jetbrains.annotations.NotNull; +import org.labkey.api.assay.transform.PreparedTransform; import org.labkey.api.audit.AuditLogService; import org.labkey.api.audit.provider.FileSystemAuditProvider; import org.labkey.api.collections.CollectionUtils; @@ -262,7 +263,11 @@ public Map savePostedFiles(ContextType context, @NotNull Set pending = context.getPendingFileAudits(); + if (pending != null) + pending.add(new PreparedTransform.PendingFileAudit(event, file)); + else + AuditLogService.get().addEvent(context.getUser(), event); } if (!isAfterFirstFile) // first file gets stored with multipartFile's name { diff --git a/api/src/org/labkey/api/assay/AssayRunUploadContext.java b/api/src/org/labkey/api/assay/AssayRunUploadContext.java index 4ac118c772c..98726d7501c 100644 --- a/api/src/org/labkey/api/assay/AssayRunUploadContext.java +++ b/api/src/org/labkey/api/assay/AssayRunUploadContext.java @@ -37,6 +37,7 @@ import org.labkey.api.writer.ContainerUser; import org.labkey.vfs.FileLike; +import java.util.List; import java.util.Map; import java.util.Set; @@ -188,6 +189,17 @@ default void setPreparedTransform(@Nullable PreparedTransform prepared) throw new UnsupportedOperationException(getClass().getName() + " can't carry a prepared transform"); } + /** Audits of files saved while a transform is prepared, or null to add each event as its file is saved. */ + default @Nullable List getPendingFileAudits() + { + return null; + } + + /** Contexts that can't hold audits ignore this and add each event as its file is saved. */ + default void setPendingFileAudits(@Nullable List audits) + { + } + /** The RowId for the run that is being deleted and reuploaded, or null if this is a new run */ Long getReRunId(); diff --git a/api/src/org/labkey/api/assay/AssayRunUploadContextImpl.java b/api/src/org/labkey/api/assay/AssayRunUploadContextImpl.java index 35351036bfe..b004d47b76e 100644 --- a/api/src/org/labkey/api/assay/AssayRunUploadContextImpl.java +++ b/api/src/org/labkey/api/assay/AssayRunUploadContextImpl.java @@ -95,6 +95,7 @@ public class AssayRunUploadContextImpl imple // Mutable fields private TransformResult _transformResult; private @Nullable PreparedTransform _preparedTransform; + private @Nullable List _pendingFileAudits; // async fields protected String _jobDescription; @@ -453,6 +454,18 @@ public void setPreparedTransform(@Nullable PreparedTransform prepared) _preparedTransform = prepared; } + @Override + public @Nullable List getPendingFileAudits() + { + return _pendingFileAudits; + } + + @Override + public void setPendingFileAudits(@Nullable List audits) + { + _pendingFileAudits = audits; + } + @Override public void uploadComplete(ExpRun run) { diff --git a/api/src/org/labkey/api/assay/DefaultAssayRunCreator.java b/api/src/org/labkey/api/assay/DefaultAssayRunCreator.java index 994c0ea5c80..477a8b6aeef 100644 --- a/api/src/org/labkey/api/assay/DefaultAssayRunCreator.java +++ b/api/src/org/labkey/api/assay/DefaultAssayRunCreator.java @@ -148,9 +148,23 @@ private PreparedTransform prepareTransformAfterInit(AssayRunUploadContext allProperties = new HashMap<>(context.getRunProperties()); allProperties.putAll(context.getBatchProperties()); + // Collecting the inputs saves posted files, whose audit events belong to the import transaction that isn't open yet + List fileAudits = new ArrayList<>(); + context.setPendingFileAudits(fileAudits); Map inputDatas = new HashMap<>(); - addRunDataInputs(context, null, findResolverType(allProperties), inputDatas); - PreparedTransform prepared = runTransformBeforeRun(context, inputDatas.keySet()); + PreparedTransform prepared = null; + try + { + addRunDataInputs(context, null, findResolverType(allProperties), inputDatas); + prepared = runTransformBeforeRun(context, inputDatas.keySet()); + } + finally + { + context.setPendingFileAudits(null); + if (prepared == null) + PreparedTransform.addFileAuditEventsForRemainingFiles(context.getUser(), fileAudits); + } + prepared.holdFileAudits(context.getUser(), fileAudits); // Run creation reuses these rather than collecting again, since a thaw list resolver writes a new file each time prepared.setRunDataInputs(inputDatas); return prepared; @@ -433,6 +447,8 @@ public ExpExperiment saveExperimentRun( AbstractQueryUpdateService.addTransactionAuditEvent(transaction, context.getUser(), auditEvent); } } + if (preparedBeforeRun != null) + preparedBeforeRun.recordFileAuditEvents(transaction.getAuditEvent()); boolean saveBatchProps = forceSaveBatchProps; // Add any material/data inputs related to the specimen IDs, etc in the incoming data. diff --git a/api/src/org/labkey/api/assay/actions/AssayRunUploadForm.java b/api/src/org/labkey/api/assay/actions/AssayRunUploadForm.java index f0cb8551789..b962b1f3139 100644 --- a/api/src/org/labkey/api/assay/actions/AssayRunUploadForm.java +++ b/api/src/org/labkey/api/assay/actions/AssayRunUploadForm.java @@ -108,6 +108,7 @@ public class AssayRunUploadForm extends Prot private List _collectors; private TransformResult _transformResult = DefaultTransformResult.createEmptyResult(); private @Nullable PreparedTransform _preparedTransform; + private @Nullable List _pendingFileAudits; private ExpRun _reRun; private boolean _allowCrossRunFileInputs; @@ -727,6 +728,18 @@ public void setPreparedTransform(@Nullable PreparedTransform prepared) _preparedTransform = prepared; } + @Override + public @Nullable List getPendingFileAudits() + { + return _pendingFileAudits; + } + + @Override + public void setPendingFileAudits(@Nullable List audits) + { + _pendingFileAudits = audits; + } + @Override public Long getReRunId() { diff --git a/api/src/org/labkey/api/assay/transform/PreparedTransform.java b/api/src/org/labkey/api/assay/transform/PreparedTransform.java index 13a7cb43f50..ca7f9ac086f 100644 --- a/api/src/org/labkey/api/assay/transform/PreparedTransform.java +++ b/api/src/org/labkey/api/assay/transform/PreparedTransform.java @@ -18,14 +18,30 @@ import org.apache.logging.log4j.Logger; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; +import org.labkey.api.audit.AuditLogService; +import org.labkey.api.audit.TransactionAuditProvider; +import org.junit.After; +import org.junit.Assert; +import org.junit.Before; +import org.junit.Test; +import org.labkey.api.audit.provider.FileSystemAuditProvider; +import org.labkey.api.data.SimpleFilter; import org.labkey.api.exp.api.ExpData; +import org.labkey.api.query.AbstractQueryUpdateService; +import org.labkey.api.query.FieldKey; +import org.labkey.api.query.QueryService; +import org.labkey.api.security.User; import org.labkey.api.util.FileUtil; +import org.labkey.api.util.GUID; +import org.labkey.api.util.JunitUtil; +import org.labkey.api.util.TestContext; import org.labkey.api.util.logging.LogHelper; import org.labkey.vfs.FileLike; import java.io.File; import java.io.IOException; import java.nio.file.Path; +import java.util.ArrayList; import java.util.HashSet; import java.util.List; import java.util.Map; @@ -34,7 +50,8 @@ /** * The outcome of running an assay's transform scripts, before any of it is written to the database. * {@link DataTransformService#recordTransform} writes it against the saved run; closing it deletes the scripts' - * working directories, so close it only after the transaction that recorded it has committed or rolled back. + * working directories and adds unrecorded file audit events for files still on disk, so close it only after the transaction that + * recorded it has committed or rolled back. */ public class PreparedTransform implements AutoCloseable { @@ -46,6 +63,8 @@ public class PreparedTransform implements AutoCloseable private final @NotNull List _scriptDirs; private boolean _recorded; private @Nullable Map _runDataInputs; + private final @NotNull List _fileAudits = new ArrayList<>(); + private @Nullable User _fileAuditUser; PreparedTransform(@Nullable DataExchangeHandler handler, @NotNull TransformResult result, @NotNull List scriptOutputs, @NotNull List scriptDirs) { @@ -104,6 +123,38 @@ public void setRunDataInputs(@NotNull Map runDataInputs) _runDataInputs = runDataInputs; } + /** The audit event of a file saved while preparing, held until the import transaction opens. */ + public record PendingFileAudit(@NotNull FileSystemAuditProvider.FileSystemAuditEvent event, @NotNull FileLike file) {} + + /** Holds the audits of files saved while preparing, to record against the import transaction once it opens. */ + public void holdFileAudits(@Nullable User user, @NotNull List audits) + { + _fileAuditUser = user; + _fileAudits.addAll(audits); + } + + public void recordFileAuditEvents(@Nullable TransactionAuditProvider.TransactionAuditEvent transactionEvent) + { + List events = _fileAudits.stream().map(PendingFileAudit::event).toList(); + _fileAudits.clear(); + if (events.isEmpty()) + return; + if (transactionEvent != null) + events.forEach(event -> event.setTransactionEvent(transactionEvent, FileSystemAuditProvider.EVENT_TYPE)); + AuditLogService.get().addEvents(_fileAuditUser, events); + } + + /** For an import that failed before recording its held audits; cleanup may already have deleted some of the files. */ + public static void addFileAuditEventsForRemainingFiles(@Nullable User user, @NotNull List audits) + { + List events = audits.stream() + .filter(audit -> audit.file().exists()) + .map(PendingFileAudit::event) + .toList(); + if (!events.isEmpty()) + AuditLogService.get().addEvents(user, events); + } + public static @NotNull Path normalize(@NotNull File file) { return file.toPath().toAbsolutePath().normalize(); @@ -122,7 +173,15 @@ void setRecorded() @Override public void close() { - deleteScriptDirs(_scriptDirs); + try + { + addFileAuditEventsForRemainingFiles(_fileAuditUser, _fileAudits); + _fileAudits.clear(); + } + finally + { + deleteScriptDirs(_scriptDirs); + } } static void deleteScriptDirs(List scriptDirs) @@ -142,4 +201,71 @@ static void deleteScriptDirs(List scriptDirs) } } } + + public static class TestCase extends Assert + { + private FileLike _dir; + + @Before + public void createDirectory() throws IOException + { + _dir = FileUtil.createTempDirectoryFileLike("preparedTransformTest"); + } + + @After + public void deleteDirectory() throws IOException + { + FileUtil.deleteDir(_dir.toNioPathForWrite().toFile()); + } + + @Test + public void testCloseAddsEventsOnlyForRemainingFiles() throws IOException + { + String comment = "PreparedTransform close " + GUID.makeGUID(); + FileLike kept = FileUtil.createTempFile("kept", ".tsv", _dir); + FileLike deleted = FileUtil.createTempFile("deleted", ".tsv", _dir); + + try (PreparedTransform prepared = PreparedTransform.empty()) + { + prepared.holdFileAudits(TestContext.get().getUser(), List.of(audit(comment, kept), audit(comment, deleted))); + deleted.delete(); + } + + List events = selectEvents(comment); + assertEquals("Events added on close", List.of(kept.getName()), events.stream().map(FileSystemAuditProvider.FileSystemAuditEvent::getFile).toList()); + assertNull("Transaction of an event added on close", events.getFirst().getTransactionId()); + } + + @Test + public void testRecordLinksTransactionAndCloseAddsNothingMore() throws IOException + { + String comment = "PreparedTransform record " + GUID.makeGUID(); + FileLike file = FileUtil.createTempFile("recorded", ".tsv", _dir); + TransactionAuditProvider.TransactionAuditEvent transactionEvent = AbstractQueryUpdateService.createTransactionAuditEvent(JunitUtil.getTestContainer(), QueryService.AuditAction.INSERT); + + try (PreparedTransform prepared = PreparedTransform.empty()) + { + prepared.holdFileAudits(TestContext.get().getUser(), List.of(audit(comment, file))); + prepared.recordFileAuditEvents(transactionEvent); + } + + List events = selectEvents(comment); + assertEquals("Events after recording and closing", 1, events.size()); + assertEquals("Transaction of the recorded event", Long.valueOf(transactionEvent.getRowId()), events.getFirst().getTransactionId()); + } + + private static PendingFileAudit audit(String comment, FileLike file) + { + FileSystemAuditProvider.FileSystemAuditEvent event = new FileSystemAuditProvider.FileSystemAuditEvent(JunitUtil.getTestContainer(), comment); + event.setFile(file.getName()); + event.setProvidedFileName(file.getName()); + return new PendingFileAudit(event, file); + } + + private static List selectEvents(String comment) + { + SimpleFilter filter = new SimpleFilter(FieldKey.fromParts("Comment"), comment); + return AuditLogService.get().getAuditEvents(JunitUtil.getTestContainer(), TestContext.get().getUser(), FileSystemAuditProvider.EVENT_TYPE, filter, null); + } + } } diff --git a/assay/src/org/labkey/assay/AssayModule.java b/assay/src/org/labkey/assay/AssayModule.java index bf087ee5e64..e2351f13a3f 100644 --- a/assay/src/org/labkey/assay/AssayModule.java +++ b/assay/src/org/labkey/assay/AssayModule.java @@ -35,6 +35,7 @@ import org.labkey.api.assay.plate.PlateService; import org.labkey.api.assay.plate.PlateUtils; import org.labkey.api.assay.plate.PositionImpl; +import org.labkey.api.assay.transform.PreparedTransform; import org.labkey.api.audit.AuditLogService; import org.labkey.api.cache.CacheManager; import org.labkey.api.data.Container; @@ -385,7 +386,8 @@ public Set getProvisionedSchemaNames() return Set.of( ModuleAssayCache.TestCase.class, PlateManagerTest.class, - PlateSchemaTest.class + PlateSchemaTest.class, + PreparedTransform.TestCase.class ); } From 122d4631c4e8f6a2193e6fd3dd9d1d12f90e2d03 Mon Sep 17 00:00:00 2001 From: labkey-jeckels Date: Sat, 24 Oct 2026 09:28:07 -0700 Subject: [PATCH 08/11] Claude review --- .../api/assay/DefaultAssayRunCreator.java | 2 +- .../assay/transform/PreparedTransform.java | 71 +++++- .../assay/transform/PreparedTransforms.java | 19 +- .../exp/api/DefaultExperimentSaveHandler.java | 237 ++++++++++++------ .../api/assay/DefaultAssaySaveHandler.java | 18 ++ 5 files changed, 268 insertions(+), 79 deletions(-) diff --git a/api/src/org/labkey/api/assay/DefaultAssayRunCreator.java b/api/src/org/labkey/api/assay/DefaultAssayRunCreator.java index 477a8b6aeef..30da650d5f6 100644 --- a/api/src/org/labkey/api/assay/DefaultAssayRunCreator.java +++ b/api/src/org/labkey/api/assay/DefaultAssayRunCreator.java @@ -448,7 +448,7 @@ public ExpExperiment saveExperimentRun( } } if (preparedBeforeRun != null) - preparedBeforeRun.recordFileAuditEvents(transaction.getAuditEvent()); + preparedBeforeRun.recordFileAuditEvents(transaction); boolean saveBatchProps = forceSaveBatchProps; // Add any material/data inputs related to the specimen IDs, etc in the incoming data. diff --git a/api/src/org/labkey/api/assay/transform/PreparedTransform.java b/api/src/org/labkey/api/assay/transform/PreparedTransform.java index ca7f9ac086f..78c6e4edf38 100644 --- a/api/src/org/labkey/api/assay/transform/PreparedTransform.java +++ b/api/src/org/labkey/api/assay/transform/PreparedTransform.java @@ -18,6 +18,7 @@ import org.apache.logging.log4j.Logger; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; +import org.json.JSONObject; import org.labkey.api.audit.AuditLogService; import org.labkey.api.audit.TransactionAuditProvider; import org.junit.After; @@ -25,6 +26,7 @@ import org.junit.Before; import org.junit.Test; import org.labkey.api.audit.provider.FileSystemAuditProvider; +import org.labkey.api.data.DbScope; import org.labkey.api.data.SimpleFilter; import org.labkey.api.exp.api.ExpData; import org.labkey.api.query.AbstractQueryUpdateService; @@ -133,15 +135,19 @@ public void holdFileAudits(@Nullable User user, @NotNull List _fileAudits.addAll(audits); } - public void recordFileAuditEvents(@Nullable TransactionAuditProvider.TransactionAuditEvent transactionEvent) + /** Records the held audits in the import transaction; a rollback discards them too, so they stay held for close() until it commits. */ + public void recordFileAuditEvents(@NotNull DbScope.Transaction transaction) { - List events = _fileAudits.stream().map(PendingFileAudit::event).toList(); - _fileAudits.clear(); - if (events.isEmpty()) + List audits = List.copyOf(_fileAudits); + if (audits.isEmpty()) return; + List events = audits.stream().map(PendingFileAudit::event).toList(); + TransactionAuditProvider.TransactionAuditEvent transactionEvent = transaction.getAuditEvent(); if (transactionEvent != null) events.forEach(event -> event.setTransactionEvent(transactionEvent, FileSystemAuditProvider.EVENT_TYPE)); AuditLogService.get().addEvents(_fileAuditUser, events); + transaction.addCommitTask(() -> _fileAudits.removeAll(audits), DbScope.CommitTaskOption.POSTCOMMIT); + transaction.addCommitTask(() -> events.forEach(event -> event.setTransactionId(null)), DbScope.CommitTaskOption.POSTROLLBACK); } /** For an import that failed before recording its held audits; cleanup may already have deleted some of the files. */ @@ -246,7 +252,12 @@ public void testRecordLinksTransactionAndCloseAddsNothingMore() throws IOExcepti try (PreparedTransform prepared = PreparedTransform.empty()) { prepared.holdFileAudits(TestContext.get().getUser(), List.of(audit(comment, file))); - prepared.recordFileAuditEvents(transactionEvent); + try (DbScope.Transaction transaction = DbScope.getLabKeyScope().ensureTransaction()) + { + AbstractQueryUpdateService.addTransactionAuditEvent(transaction, TestContext.get().getUser(), transactionEvent); + prepared.recordFileAuditEvents(transaction); + transaction.commit(); + } } List events = selectEvents(comment); @@ -254,6 +265,56 @@ public void testRecordLinksTransactionAndCloseAddsNothingMore() throws IOExcepti assertEquals("Transaction of the recorded event", Long.valueOf(transactionEvent.getRowId()), events.getFirst().getTransactionId()); } + @Test + public void testRolledBackRecordLeavesEventsForClose() throws IOException + { + String comment = "PreparedTransform rollback " + GUID.makeGUID(); + FileLike file = FileUtil.createTempFile("rolledBack", ".tsv", _dir); + TransactionAuditProvider.TransactionAuditEvent transactionEvent = AbstractQueryUpdateService.createTransactionAuditEvent(JunitUtil.getTestContainer(), QueryService.AuditAction.INSERT); + + try (PreparedTransform prepared = PreparedTransform.empty()) + { + prepared.holdFileAudits(TestContext.get().getUser(), List.of(audit(comment, file))); + try (DbScope.Transaction transaction = DbScope.getLabKeyScope().ensureTransaction()) + { + AbstractQueryUpdateService.addTransactionAuditEvent(transaction, TestContext.get().getUser(), transactionEvent); + prepared.recordFileAuditEvents(transaction); + } + } + + List events = selectEvents(comment); + assertEquals("Events after a rolled back record and closing", 1, events.size()); + assertNull("Transaction of an event added on close", events.getFirst().getTransactionId()); + } + + @Test + public void testCloseAllContinuesPastFailure() throws IOException + { + FileLike scriptDir = _dir.resolveChild("run").resolveChild("work"); + FileUtil.mkdirs(scriptDir); + // Closing order isn't defined, so two failures guarantee one comes before another close + PreparedTransforms preparedTransforms = new PreparedTransforms(); + preparedTransforms.put(new JSONObject(), failingTransform()); + preparedTransforms.put(new JSONObject(), failingTransform()); + preparedTransforms.put(new JSONObject(), new PreparedTransform(null, DefaultTransformResult.createEmptyResult(), List.of(), List.of(scriptDir))); + + IllegalStateException e = assertThrows(IllegalStateException.class, preparedTransforms::close); + assertEquals("Failures suppressed into the first", 1, e.getSuppressed().length); + assertFalse("Working directory of the run that closed cleanly", scriptDir.exists()); + } + + private static PreparedTransform failingTransform() + { + return new PreparedTransform(null, DefaultTransformResult.createEmptyResult(), List.of(), List.of()) + { + @Override + public void close() + { + throw new IllegalStateException("close failed"); + } + }; + } + private static PendingFileAudit audit(String comment, FileLike file) { FileSystemAuditProvider.FileSystemAuditEvent event = new FileSystemAuditProvider.FileSystemAuditEvent(JunitUtil.getTestContainer(), comment); diff --git a/api/src/org/labkey/api/assay/transform/PreparedTransforms.java b/api/src/org/labkey/api/assay/transform/PreparedTransforms.java index 32c85fa336c..2e61785454a 100644 --- a/api/src/org/labkey/api/assay/transform/PreparedTransforms.java +++ b/api/src/org/labkey/api/assay/transform/PreparedTransforms.java @@ -43,6 +43,23 @@ public void put(@NotNull JSONObject runJson, @NotNull PreparedTransform prepared @Override public void close() { - _byRunJson.values().forEach(PreparedTransform::close); + // Closing writes audit events, so one failure must not leave the other runs' working directories behind + RuntimeException failure = null; + for (PreparedTransform prepared : _byRunJson.values()) + { + try + { + prepared.close(); + } + catch (RuntimeException e) + { + if (failure == null) + failure = e; + else + failure.addSuppressed(e); + } + } + if (failure != null) + throw failure; } } diff --git a/api/src/org/labkey/api/exp/api/DefaultExperimentSaveHandler.java b/api/src/org/labkey/api/exp/api/DefaultExperimentSaveHandler.java index 8a7b603e690..93376eba7ef 100644 --- a/api/src/org/labkey/api/exp/api/DefaultExperimentSaveHandler.java +++ b/api/src/org/labkey/api/exp/api/DefaultExperimentSaveHandler.java @@ -62,6 +62,7 @@ import java.util.Map; import java.util.Objects; import java.util.Set; +import java.util.stream.Collectors; import static org.labkey.api.exp.api.ExperimentService.SAMPLE_ALIQUOT_PROTOCOL_LSID; @@ -472,30 +473,7 @@ protected void handleProtocolApplications( aliquotRootRowId = parent.getRootMaterialRowId() == null ? parent.getRowId() : parent.getRootMaterialRowId(); } - Map existingOutputs = new HashMap<>(); - ExpProtocolApplication existingOutputApp = run.getOutputProtocolApplication(); - if (existingOutputApp != null) - { - for (ExpMaterialRunInput input : existingOutputApp.getMaterialInputs()) - existingOutputs.put(input.getMaterial().getRowId(), input.getMaterial()); - } - - Set existingMaterialInputRowIds = new HashSet<>(); - for (ExpMaterial in : run.getMaterialInputs().keySet()) - existingMaterialInputRowIds.add(in.getRowId()); - Set postedMaterialInputRowIds = new HashSet<>(); - for (ExpMaterial in : inputMaterial.keySet()) - postedMaterialInputRowIds.add(in.getRowId()); - - Set existingDataInputRowIds = new HashSet<>(); - for (ExpData in : run.getDataInputs().keySet()) - existingDataInputRowIds.add(in.getRowId()); - Set postedDataInputRowIds = new HashSet<>(); - for (ExpData in : inputData.keySet()) - postedDataInputRowIds.add(in.getRowId()); - - boolean inputsUnchanged = existingMaterialInputRowIds.equals(postedMaterialInputRowIds) - && existingDataInputRowIds.equals(postedDataInputRowIds); + ExistingOutputs existingOutputs = ExistingOutputs.read(run, rowIds(inputData.keySet()), rowIds(inputMaterial.keySet())); run.deleteProtocolApplications(context.getUser()); @@ -519,12 +497,7 @@ protected void handleProtocolApplications( if (material != null) { keptOutputRowIds.add(material.getRowId()); - - // Attaching an output rewrites the material's lineage, so require write access - but skip - // materials that were already outputs of this run, since a re-save isn't changing them. - // A sample created in this request only needs insert rights. - if (!existingOutputs.containsKey(material.getRowId()) || !inputsUnchanged) - assertCanEditLineage(context.getUser(), material.getLSID(), material, createdMaterialLsids.contains(material.getLSID())); + existingOutputs.assertCanAttach(context.getUser(), material, createdMaterialLsids.contains(material.getLSID())); if (isAliquotProtocol) { @@ -536,13 +509,8 @@ protected void handleProtocolApplications( } } - // Detaching an output also rewrites its lineage: authorize any prior output the post didn't keep. // This runs inside the save's transaction, so a denial rolls the detach above back. - for (Map.Entry entry : existingOutputs.entrySet()) - { - if (!keptOutputRowIds.contains(entry.getKey())) - assertCanEditLineage(context.getUser(), entry.getValue().getLSID(), entry.getValue()); - } + existingOutputs.assertCanDetachOthers(context.getUser(), keptOutputRowIds); checkForCycles(inputData, outputData); checkForCycles(inputMaterial, outputMaterial); @@ -550,6 +518,109 @@ protected void handleProtocolApplications( saveExperimentRun(context, protocol, batch, run, runJsonObject, dataArray, inputData, outputData, inputMaterial, outputMaterial); } + /** + * Runs handleProtocolApplications' permission and cycle checks without writing anything, to refuse a post + * before work a rollback can't undo, like running transform scripts. The save pass still runs every check. + */ + protected void checkProtocolApplications(ViewContext context, @Nullable ExpRun run, JSONArray inputDataArray, JSONArray inputMaterialArray, JSONArray outputDataArray, JSONArray outputMaterialArray) throws ExperimentException, ValidationException + { + Map inputData = lookupDatas(context, inputDataArray); + Map outputData = lookupDatas(context, outputDataArray); + checkForCycles(inputData, outputData); + + Map inputMaterial = new HashMap<>(); + Set inputMaterialRowIds = new HashSet<>(); + for (JSONObject materialObject : JsonUtil.toJSONObjectList(inputMaterialArray)) + { + MaterialLookup lookup = checkMaterial(context, materialObject); + if (lookup.material() != null) + { + inputMaterial.put(lookup.material(), materialObject.optString(ExperimentJSONConverter.ROLE, ExpMaterialRunInput.DEFAULT_ROLE)); + inputMaterialRowIds.add(lookup.material().getRowId()); + } + else if (lookup.newName() != null) + inputMaterialRowIds.add(0L); // Not created yet, but still a changed input + } + + ExistingOutputs existingOutputs = ExistingOutputs.read(run, rowIds(inputData.keySet()), inputMaterialRowIds); + Map outputMaterial = new HashMap<>(); + for (JSONObject materialObject : JsonUtil.toJSONObjectList(outputMaterialArray)) + { + // A material the save would create needs only insert rights, which the save actions already require + ExpMaterial material = checkMaterial(context, materialObject).material(); + if (material != null) + { + existingOutputs.assertCanAttach(context.getUser(), material, false); + outputMaterial.put(material, materialObject.optString(ExperimentJSONConverter.ROLE, ExpMaterial.DEFAULT_CPAS_TYPE)); + } + } + existingOutputs.assertCanDetachOthers(context.getUser(), rowIds(outputMaterial.keySet())); + checkForCycles(inputMaterial, outputMaterial); + } + + private Map lookupDatas(ViewContext context, JSONArray dataArray) + { + Map datas = new HashMap<>(); + for (JSONObject dataObject : JsonUtil.toJSONObjectList(dataArray)) + datas.put(lookupData(context, dataObject), dataObject.optString(ExperimentJSONConverter.ROLE, ExpDataRunInput.DEFAULT_ROLE)); + return datas; + } + + private MaterialLookup checkMaterial(ViewContext context, JSONObject materialObject) throws ValidationException + { + MaterialLookup lookup = lookupMaterial(context, materialObject); + JSONObject materialProperties = materialObject.optJSONObject(ExperimentJSONConverter.PROPERTIES); + if (lookup.material() != null && materialProperties != null && !materialProperties.isEmpty()) + assertCanEditProperties(context, lookup.material(), lookup.domainProperties(), materialProperties, false); + return lookup; + } + + private static Set rowIds(Collection items) + { + return items.stream().map(ExpRunItem::getRowId).collect(Collectors.toSet()); + } + + /** A run's output materials before the save replaces its protocol applications, and whether the post keeps its inputs. */ + private record ExistingOutputs(Map materials, boolean inputsUnchanged) + { + static ExistingOutputs read(@Nullable ExpRun run, Set postedDataInputRowIds, Set postedMaterialInputRowIds) + { + if (run == null) + return new ExistingOutputs(Map.of(), false); + + Map materials = new HashMap<>(); + ExpProtocolApplication outputApp = run.getOutputProtocolApplication(); + if (outputApp != null) + { + for (ExpMaterialRunInput input : outputApp.getMaterialInputs()) + materials.put(input.getMaterial().getRowId(), input.getMaterial()); + } + + boolean inputsUnchanged = rowIds(run.getMaterialInputs().keySet()).equals(postedMaterialInputRowIds) + && rowIds(run.getDataInputs().keySet()).equals(postedDataInputRowIds); + return new ExistingOutputs(materials, inputsUnchanged); + } + + // Attaching an output rewrites the material's lineage, so require write access - but skip + // materials that were already outputs of this run, since a re-save isn't changing them. + // A sample created in this request only needs insert rights. + void assertCanAttach(User user, ExpMaterial material, boolean createdHere) + { + if (!materials.containsKey(material.getRowId()) || !inputsUnchanged) + assertCanEditLineage(user, material.getLSID(), material, createdHere); + } + + /** Detaching an output also rewrites its lineage: authorize any prior output the post didn't keep. */ + void assertCanDetachOthers(User user, Set keptRowIds) + { + for (Map.Entry entry : materials.entrySet()) + { + if (!keptRowIds.contains(entry.getKey())) + assertCanEditLineage(user, entry.getValue().getLSID(), entry.getValue()); + } + } + } + protected ExpExperiment saveExperimentRun(ViewContext context, ExpProtocol protocol, ExpExperiment batch, ExpRun run, JSONObject runJson, JSONArray dataArray, @@ -566,10 +637,49 @@ protected ExpExperiment saveExperimentRun(ViewContext context, ExpProtocol proto @Override public ExpMaterial handleMaterial(ViewContext context, JSONObject materialObject) throws ValidationException + { + MaterialLookup lookup = lookupMaterial(context, materialObject); + ExpMaterial material = lookup.material(); + boolean createdHere = false; + if (material == null && lookup.newName() != null) + { + material = createMaterial(context, lookup.sampleType(), lookup.newName()); + createdHere = true; + createdMaterialLsids.add(material.getLSID()); + } + + if (material == null) + return null; + + if (materialObject.has(ExperimentJSONConverter.PROPERTIES)) + { + JSONObject materialProperties = materialObject.getJSONObject(ExperimentJSONConverter.PROPERTIES); + // Treat an empty properties collection as if there were no property map at all. + // To delete a property, include a property map with that property and set its value to null. + if (!materialProperties.isEmpty()) + { + List dps = lookup.domainProperties(); + assertCanEditProperties(context, material, dps, materialProperties, createdHere); + handleProperties(context, material, dps, materialProperties); + } + } + + return material; + } + + /** A posted material that exists, or the sample type and name of the one handleMaterial would create. */ + private record MaterialLookup(@Nullable ExpMaterial material, @Nullable ExpSampleType sampleType, @Nullable String newName) + { + List domainProperties() + { + return sampleType != null ? sampleType.getDomain().getProperties() : Collections.emptyList(); + } + } + + private MaterialLookup lookupMaterial(ViewContext context, JSONObject materialObject) throws ValidationException { ExpSampleType sampleType = null; ExpMaterial material = null; - boolean createdHere = false; if (materialObject.has(ExperimentJSONConverter.ID)) { int materialRowId = materialObject.getInt(ExperimentJSONConverter.ID); @@ -587,6 +697,7 @@ else if (materialObject.has(ExperimentJSONConverter.LSID)) sampleType = material.getSampleType(); } + String newName = null; if (material == null) { // Get SampleType by id, name, or lsid @@ -653,45 +764,27 @@ else if (sampleType != null && !sampleType.hasNameAsIdCol() && materialObject.ha throw new NotFoundException("Could not find material '" + materialName + "'"); if (material == null) - { - material = createMaterial(context, sampleType, materialName); - createdHere = true; - createdMaterialLsids.add(material.getLSID()); - } + newName = materialName; } } + return new MaterialLookup(material, sampleType, newName); + } - if (material == null) - return null; - - if (materialObject.has(ExperimentJSONConverter.PROPERTIES)) - { - JSONObject materialProperties = materialObject.getJSONObject(ExperimentJSONConverter.PROPERTIES); - // Treat an empty properties collection as if there were no property map at all. - // To delete a property, include a property map with that property and set its value to null. - if (!materialProperties.isEmpty()) - { - List dps = sampleType != null ? sampleType.getDomain().getProperties() : Collections.emptyList(); - - if (propertiesWouldChange(context, material, dps, materialProperties)) - { - // A sample created by this request needs only insert rights; an existing one needs edit rights. - // The status check below applies either way. - Class editPerm = createdHere - ? InsertPermission.class - : SampleTypeService.SampleOperations.EditMetadata.getPermissionClass(); - if (editPerm == null || !material.getContainer().hasPermission(context.getUser(), editPerm)) - throw new UnauthorizedException("User does not have permission to edit sample '" + material.getName() + "'"); - if (!material.isOperationPermitted(SampleTypeService.SampleOperations.EditMetadata)) - throw new UnauthorizedException(SampleTypeService.get().getOperationNotPermittedMessage( - List.of(material), SampleTypeService.SampleOperations.EditMetadata)); - } - - handleProperties(context, material, dps, materialProperties); - } - } + // A sample created by this request needs only insert rights; an existing one needs edit rights. + // The status check below applies either way. + private void assertCanEditProperties(ViewContext context, ExpMaterial material, List dps, JSONObject materialProperties, boolean createdHere) throws ValidationException + { + if (!propertiesWouldChange(context, material, dps, materialProperties)) + return; - return material; + Class editPerm = createdHere + ? InsertPermission.class + : SampleTypeService.SampleOperations.EditMetadata.getPermissionClass(); + if (editPerm == null || !material.getContainer().hasPermission(context.getUser(), editPerm)) + throw new UnauthorizedException("User does not have permission to edit sample '" + material.getName() + "'"); + if (!material.isOperationPermitted(SampleTypeService.SampleOperations.EditMetadata)) + throw new UnauthorizedException(SampleTypeService.get().getOperationNotPermittedMessage( + List.of(material), SampleTypeService.SampleOperations.EditMetadata)); } private boolean propertiesWouldChange(ViewContext context, ExpMaterial material, List dps, JSONObject submitted) throws ValidationException diff --git a/assay/api-src/org/labkey/api/assay/DefaultAssaySaveHandler.java b/assay/api-src/org/labkey/api/assay/DefaultAssaySaveHandler.java index 604037f838a..fe2e5520811 100644 --- a/assay/api-src/org/labkey/api/assay/DefaultAssaySaveHandler.java +++ b/assay/api-src/org/labkey/api/assay/DefaultAssaySaveHandler.java @@ -290,6 +290,10 @@ protected boolean isPrepareTransformsSupported() || getProvider().getValidationAndAnalysisScripts(protocol, AssayProvider.Scope.ALL).isEmpty()) return preparedTransforms; + // A script's side effects can't be rolled back, so refuse the request before any of them runs + for (JSONObject runJson : runJsons) + checkRun(context, protocol, runJson); + boolean success = false; try { @@ -310,6 +314,20 @@ protected boolean isPrepareTransformsSupported() return preparedTransforms; } + private void checkRun(ViewContext context, ExpProtocol protocol, JSONObject runJson) throws ExperimentException, ValidationException + { + ExpRun existingRun = lookupRun(context, runJson, null); + if (!hasRunContent(runJson)) + return; + + // An existing run's serialization stays cached for prepareTransform, which reads it next + checkProtocolApplications(context, existingRun, + existingRun != null ? serializeDataInputs(context, existingRun, runJson, protocol, null) : postedArray(runJson, ExperimentJSONConverter.DATA_INPUTS), + existingRun != null ? serializeMaterialInputs(context, existingRun, runJson, protocol, null) : postedArray(runJson, ExperimentJSONConverter.MATERIAL_INPUTS), + existingRun != null ? serializeDataOutputs(context, existingRun, runJson, protocol, null) : postedArray(runJson, ExperimentJSONConverter.DATA_OUTPUTS), + existingRun != null ? serializeMaterialOutputs(context, existingRun, runJson, protocol, null) : postedArray(runJson, ExperimentJSONConverter.MATERIAL_OUTPUTS)); + } + /** Builds the upload context handleRun would build for this run, without writing anything, and runs its transform scripts. */ private @Nullable PreparedTransform prepareTransform(ViewContext context, ExpProtocol protocol, JSONObject runJson) throws ExperimentException, ValidationException { From 6c06705db919eb3740f5084bad9036b5c6bcb5a0 Mon Sep 17 00:00:00 2001 From: labkey-jeckels Date: Sat, 24 Oct 2026 09:59:28 -0700 Subject: [PATCH 09/11] Register in the owning module --- api/src/org/labkey/api/ApiModule.java | 2 ++ assay/src/org/labkey/assay/AssayModule.java | 4 +--- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/api/src/org/labkey/api/ApiModule.java b/api/src/org/labkey/api/ApiModule.java index da21346df63..216f19d5447 100644 --- a/api/src/org/labkey/api/ApiModule.java +++ b/api/src/org/labkey/api/ApiModule.java @@ -30,6 +30,7 @@ import org.labkey.api.assay.AssayResultsFileWriter; import org.labkey.api.assay.ReplacedRunFilter; import org.labkey.api.assay.sample.MaterialInputRoleComparator; +import org.labkey.api.assay.transform.PreparedTransform; import org.labkey.api.attachments.AttachmentService; import org.labkey.api.attachments.ImageServlet; import org.labkey.api.attachments.LookAndFeelResourceType; @@ -581,6 +582,7 @@ public void registerServlets(ServletContext servletCtx) ParameterSubstitutionTest.class, Portal.TestCase.class, PostgresSnapshot.TestCase.class, + PreparedTransform.TestCase.class, PropertyManager.TestCase.class, QueryDataIteratorBuilder.TestCase.class, QueryView.TestCase.class, diff --git a/assay/src/org/labkey/assay/AssayModule.java b/assay/src/org/labkey/assay/AssayModule.java index e2351f13a3f..bf087ee5e64 100644 --- a/assay/src/org/labkey/assay/AssayModule.java +++ b/assay/src/org/labkey/assay/AssayModule.java @@ -35,7 +35,6 @@ import org.labkey.api.assay.plate.PlateService; import org.labkey.api.assay.plate.PlateUtils; import org.labkey.api.assay.plate.PositionImpl; -import org.labkey.api.assay.transform.PreparedTransform; import org.labkey.api.audit.AuditLogService; import org.labkey.api.cache.CacheManager; import org.labkey.api.data.Container; @@ -386,8 +385,7 @@ public Set getProvisionedSchemaNames() return Set.of( ModuleAssayCache.TestCase.class, PlateManagerTest.class, - PlateSchemaTest.class, - PreparedTransform.TestCase.class + PlateSchemaTest.class ); } From d351f3113fb793705bf4cc71bd288cd7e5520964 Mon Sep 17 00:00:00 2001 From: labkey-jeckels Date: Wed, 28 Oct 2026 10:18:14 -0700 Subject: [PATCH 10/11] Checkpoint --- .../org/labkey/api/assay/AssayFileWriter.java | 6 +-- .../api/assay/AssayRunUploadContext.java | 5 ++- .../api/assay/AssayRunUploadContextImpl.java | 7 ++-- .../api/assay/DefaultAssayRunCreator.java | 3 +- .../api/assay/actions/AssayRunUploadForm.java | 7 ++-- .../api/assay/transform/PendingFileAudit.java | 23 +++++++++++ .../assay/transform/PreparedTransform.java | 25 +++++++++++- .../exp/api/DefaultExperimentSaveHandler.java | 4 +- .../api/assay/DefaultAssaySaveHandler.java | 39 ++++++------------- 9 files changed, 76 insertions(+), 43 deletions(-) create mode 100644 api/src/org/labkey/api/assay/transform/PendingFileAudit.java diff --git a/api/src/org/labkey/api/assay/AssayFileWriter.java b/api/src/org/labkey/api/assay/AssayFileWriter.java index 70a3a62b3f1..c15f72e257e 100644 --- a/api/src/org/labkey/api/assay/AssayFileWriter.java +++ b/api/src/org/labkey/api/assay/AssayFileWriter.java @@ -20,7 +20,7 @@ import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; import org.jetbrains.annotations.NotNull; -import org.labkey.api.assay.transform.PreparedTransform; +import org.labkey.api.assay.transform.PendingFileAudit; import org.labkey.api.audit.AuditLogService; import org.labkey.api.audit.provider.FileSystemAuditProvider; import org.labkey.api.collections.CollectionUtils; @@ -263,9 +263,9 @@ public Map savePostedFiles(ContextType context, @NotNull Set pending = context.getPendingFileAudits(); + List pending = context.getPendingFileAudits(); if (pending != null) - pending.add(new PreparedTransform.PendingFileAudit(event, file)); + pending.add(new PendingFileAudit(event, file)); else AuditLogService.get().addEvent(context.getUser(), event); } diff --git a/api/src/org/labkey/api/assay/AssayRunUploadContext.java b/api/src/org/labkey/api/assay/AssayRunUploadContext.java index 98726d7501c..3fa23381de1 100644 --- a/api/src/org/labkey/api/assay/AssayRunUploadContext.java +++ b/api/src/org/labkey/api/assay/AssayRunUploadContext.java @@ -28,6 +28,7 @@ import org.labkey.api.exp.api.ExpProtocol; import org.labkey.api.exp.api.ExpRun; import org.labkey.api.exp.property.DomainProperty; +import org.labkey.api.assay.transform.PendingFileAudit; import org.labkey.api.assay.transform.PreparedTransform; import org.labkey.api.assay.transform.TransformResult; import org.labkey.api.security.User; @@ -190,13 +191,13 @@ default void setPreparedTransform(@Nullable PreparedTransform prepared) } /** Audits of files saved while a transform is prepared, or null to add each event as its file is saved. */ - default @Nullable List getPendingFileAudits() + default @Nullable List getPendingFileAudits() { return null; } /** Contexts that can't hold audits ignore this and add each event as its file is saved. */ - default void setPendingFileAudits(@Nullable List audits) + default void setPendingFileAudits(@Nullable List audits) { } diff --git a/api/src/org/labkey/api/assay/AssayRunUploadContextImpl.java b/api/src/org/labkey/api/assay/AssayRunUploadContextImpl.java index b004d47b76e..0dce81f7ee7 100644 --- a/api/src/org/labkey/api/assay/AssayRunUploadContextImpl.java +++ b/api/src/org/labkey/api/assay/AssayRunUploadContextImpl.java @@ -32,6 +32,7 @@ import org.labkey.api.exp.property.Domain; import org.labkey.api.exp.property.DomainProperty; import org.labkey.api.assay.transform.DefaultTransformResult; +import org.labkey.api.assay.transform.PendingFileAudit; import org.labkey.api.assay.transform.PreparedTransform; import org.labkey.api.assay.transform.TransformResult; import org.labkey.api.security.User; @@ -95,7 +96,7 @@ public class AssayRunUploadContextImpl imple // Mutable fields private TransformResult _transformResult; private @Nullable PreparedTransform _preparedTransform; - private @Nullable List _pendingFileAudits; + private @Nullable List _pendingFileAudits; // async fields protected String _jobDescription; @@ -455,13 +456,13 @@ public void setPreparedTransform(@Nullable PreparedTransform prepared) } @Override - public @Nullable List getPendingFileAudits() + public @Nullable List getPendingFileAudits() { return _pendingFileAudits; } @Override - public void setPendingFileAudits(@Nullable List audits) + public void setPendingFileAudits(@Nullable List audits) { _pendingFileAudits = audits; } diff --git a/api/src/org/labkey/api/assay/DefaultAssayRunCreator.java b/api/src/org/labkey/api/assay/DefaultAssayRunCreator.java index 30da650d5f6..c67ff35e188 100644 --- a/api/src/org/labkey/api/assay/DefaultAssayRunCreator.java +++ b/api/src/org/labkey/api/assay/DefaultAssayRunCreator.java @@ -29,6 +29,7 @@ import org.labkey.api.assay.pipeline.AssayUploadPipelineJob; import org.labkey.api.assay.sample.AssaySampleLookupContext; import org.labkey.api.assay.transform.DataTransformService; +import org.labkey.api.assay.transform.PendingFileAudit; import org.labkey.api.assay.transform.PreparedTransform; import org.labkey.api.assay.transform.TransformDataHandler; import org.labkey.api.assay.transform.TransformResult; @@ -149,7 +150,7 @@ private PreparedTransform prepareTransformAfterInit(AssayRunUploadContext fileAudits = new ArrayList<>(); + List fileAudits = new ArrayList<>(); context.setPendingFileAudits(fileAudits); Map inputDatas = new HashMap<>(); PreparedTransform prepared = null; diff --git a/api/src/org/labkey/api/assay/actions/AssayRunUploadForm.java b/api/src/org/labkey/api/assay/actions/AssayRunUploadForm.java index b962b1f3139..3ffbf3b5410 100644 --- a/api/src/org/labkey/api/assay/actions/AssayRunUploadForm.java +++ b/api/src/org/labkey/api/assay/actions/AssayRunUploadForm.java @@ -49,6 +49,7 @@ import org.labkey.api.exp.property.DomainProperty; import org.labkey.api.pipeline.PipelineService; import org.labkey.api.assay.transform.DefaultTransformResult; +import org.labkey.api.assay.transform.PendingFileAudit; import org.labkey.api.assay.transform.PreparedTransform; import org.labkey.api.assay.transform.TransformResult; import org.labkey.api.qc.TsvDataExchangeHandler; @@ -108,7 +109,7 @@ public class AssayRunUploadForm extends Prot private List _collectors; private TransformResult _transformResult = DefaultTransformResult.createEmptyResult(); private @Nullable PreparedTransform _preparedTransform; - private @Nullable List _pendingFileAudits; + private @Nullable List _pendingFileAudits; private ExpRun _reRun; private boolean _allowCrossRunFileInputs; @@ -729,13 +730,13 @@ public void setPreparedTransform(@Nullable PreparedTransform prepared) } @Override - public @Nullable List getPendingFileAudits() + public @Nullable List getPendingFileAudits() { return _pendingFileAudits; } @Override - public void setPendingFileAudits(@Nullable List audits) + public void setPendingFileAudits(@Nullable List audits) { _pendingFileAudits = audits; } diff --git a/api/src/org/labkey/api/assay/transform/PendingFileAudit.java b/api/src/org/labkey/api/assay/transform/PendingFileAudit.java new file mode 100644 index 00000000000..20f45be1ce2 --- /dev/null +++ b/api/src/org/labkey/api/assay/transform/PendingFileAudit.java @@ -0,0 +1,23 @@ +/* + * Copyright (c) 2026 LabKey Corporation + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.labkey.api.assay.transform; + +import org.jetbrains.annotations.NotNull; +import org.labkey.api.audit.provider.FileSystemAuditProvider; +import org.labkey.vfs.FileLike; + +/** The audit event of a file saved while preparing a transform, held until the import transaction opens. */ +public record PendingFileAudit(@NotNull FileSystemAuditProvider.FileSystemAuditEvent event, @NotNull FileLike file) {} diff --git a/api/src/org/labkey/api/assay/transform/PreparedTransform.java b/api/src/org/labkey/api/assay/transform/PreparedTransform.java index 78c6e4edf38..068137e80d6 100644 --- a/api/src/org/labkey/api/assay/transform/PreparedTransform.java +++ b/api/src/org/labkey/api/assay/transform/PreparedTransform.java @@ -44,6 +44,7 @@ import java.io.IOException; import java.nio.file.Path; import java.util.ArrayList; +import java.util.HashMap; import java.util.HashSet; import java.util.List; import java.util.Map; @@ -65,6 +66,7 @@ public class PreparedTransform implements AutoCloseable private final @NotNull List _scriptDirs; private boolean _recorded; private @Nullable Map _runDataInputs; + private @Nullable JSONObject _scriptInputs; private final @NotNull List _fileAudits = new ArrayList<>(); private @Nullable User _fileAuditUser; @@ -125,8 +127,27 @@ public void setRunDataInputs(@NotNull Map runDataInputs) _runDataInputs = runDataInputs; } - /** The audit event of a file saved while preparing, held until the import transaction opens. */ - public record PendingFileAudit(@NotNull FileSystemAuditProvider.FileSystemAuditEvent event, @NotNull FileLike file) {} + /** Adds inputs to those collected before the scripts ran; a no-op when none were collected. */ + public void addRunDataInputs(@NotNull Map runDataInputs) + { + if (_runDataInputs == null) + return; + Map merged = new HashMap<>(_runDataInputs); + merged.putAll(runDataInputs); + _runDataInputs = merged; + } + + /** What the scripts read from the existing run, for the save to compare once its transaction reads the run again. */ + public void setScriptInputs(@NotNull JSONObject scriptInputs) + { + _scriptInputs = scriptInputs; + } + + /** Whether the run, read again inside the transaction, still holds what the scripts read; true when they read nothing from it. */ + public boolean scriptInputsMatch(@Nullable JSONObject current) + { + return _scriptInputs == null || (current != null && _scriptInputs.similar(current)); + } /** Holds the audits of files saved while preparing, to record against the import transaction once it opens. */ public void holdFileAudits(@Nullable User user, @NotNull List audits) diff --git a/api/src/org/labkey/api/exp/api/DefaultExperimentSaveHandler.java b/api/src/org/labkey/api/exp/api/DefaultExperimentSaveHandler.java index 93376eba7ef..ea534b4bb56 100644 --- a/api/src/org/labkey/api/exp/api/DefaultExperimentSaveHandler.java +++ b/api/src/org/labkey/api/exp/api/DefaultExperimentSaveHandler.java @@ -423,14 +423,14 @@ protected void clearOutputDatas(ViewContext context, ExpRun run) } // Disallow creating a run with inputs which are also outputs - private void checkForCycles(Map inputs, Map outputs) throws ExperimentException + private void checkForCycles(Map inputs, Map outputs) throws ValidationException { for (ExpRunItem input : inputs.keySet()) { if (outputs.containsKey(input)) { String role = outputs.get(input); - throw new ExperimentException("Circular input/output '" + input.getName() + "' with role '" + role + "'"); + throw new ValidationException("Circular input/output '" + input.getName() + "' with role '" + role + "'"); } } } diff --git a/assay/api-src/org/labkey/api/assay/DefaultAssaySaveHandler.java b/assay/api-src/org/labkey/api/assay/DefaultAssaySaveHandler.java index fe2e5520811..d2639de44a9 100644 --- a/assay/api-src/org/labkey/api/assay/DefaultAssaySaveHandler.java +++ b/assay/api-src/org/labkey/api/assay/DefaultAssaySaveHandler.java @@ -67,9 +67,7 @@ public class DefaultAssaySaveHandler extends DefaultExperimentSaveHandler implem protected AssayProvider _provider; private PreparedTransforms _preparedTransforms = new PreparedTransforms(); private final Map _serializedRuns = new HashMap<>(); - /** For each prepared run, the parts of its existing serialization its scripts read before the transaction. */ - private final Map _preparedSerializations = new IdentityHashMap<>(); - private static final List PREPARED_KEYS = List.of(AssayJSONConverter.DATA_ROWS, ExperimentJSONConverter.DATA_INPUTS, ExperimentJSONConverter.DATA_OUTPUTS); + private static final List SCRIPT_INPUT_KEYS = List.of(AssayJSONConverter.DATA_ROWS, ExperimentJSONConverter.DATA_INPUTS, ExperimentJSONConverter.DATA_OUTPUTS); @Override public AssayProvider getProvider() @@ -231,35 +229,21 @@ protected ExpExperiment saveExperimentRun( if (prepared != null && uploadContext != null) { // Rerunning the scripts inside the transaction would reopen the deadlock they ran early to avoid - if (!matchesPrepared(runJson, serializedRun)) + if (!prepared.scriptInputsMatch(serializedRun == null ? null : readFromSerialization(runJson, serializedRun))) throw new ValidationException("Run " + run.getRowId() + " changed while its transform scripts ran. Retry the save."); - if (prepared.getRunDataInputs() != null) - { - Map runDataInputs = new HashMap<>(prepared.getRunDataInputs()); - runDataInputs.putAll(inputData); - prepared.setRunDataInputs(runDataInputs); - } + prepared.addRunDataInputs(inputData); uploadContext.setPreparedTransform(prepared); } return saveAssayRun(uploadContext, batch, run, BaseViewAction.getTransactionAuditDetails(context)); } - /** Whether the run, read inside the transaction, still holds what its scripts saw before it. */ - private boolean matchesPrepared(JSONObject runJson, @Nullable JSONObject serializedRun) - { - JSONObject seen = _preparedSerializations.get(runJson); - if (seen == null) - return true; - return serializedRun != null && seen.similar(readFromSerialization(runJson, serializedRun)); - } - /** The arrays read from an existing run's serialization because the client didn't post them. */ private static JSONObject readFromSerialization(JSONObject runJson, JSONObject serializedRun) { JSONObject read = new JSONObject(); - for (String key : PREPARED_KEYS) + for (String key : SCRIPT_INPUT_KEYS) { if (!runJson.has(key)) read.put(key, serializedRun.getJSONArray(key)); @@ -299,7 +283,7 @@ protected boolean isPrepareTransformsSupported() { for (JSONObject runJson : runJsons) { - PreparedTransform prepared = prepareTransform(context, protocol, runJson); + PreparedTransform prepared = transformRun(context, protocol, runJson); if (prepared != null) preparedTransforms.put(runJson, prepared); } @@ -320,7 +304,7 @@ private void checkRun(ViewContext context, ExpProtocol protocol, JSONObject runJ if (!hasRunContent(runJson)) return; - // An existing run's serialization stays cached for prepareTransform, which reads it next + // An existing run's serialization stays cached for transformRun, which reads it next checkProtocolApplications(context, existingRun, existingRun != null ? serializeDataInputs(context, existingRun, runJson, protocol, null) : postedArray(runJson, ExperimentJSONConverter.DATA_INPUTS), existingRun != null ? serializeMaterialInputs(context, existingRun, runJson, protocol, null) : postedArray(runJson, ExperimentJSONConverter.MATERIAL_INPUTS), @@ -328,8 +312,8 @@ private void checkRun(ViewContext context, ExpProtocol protocol, JSONObject runJ existingRun != null ? serializeMaterialOutputs(context, existingRun, runJson, protocol, null) : postedArray(runJson, ExperimentJSONConverter.MATERIAL_OUTPUTS)); } - /** Builds the upload context handleRun would build for this run, without writing anything, and runs its transform scripts. */ - private @Nullable PreparedTransform prepareTransform(ViewContext context, ExpProtocol protocol, JSONObject runJson) throws ExperimentException, ValidationException + /** Runs this run's transform scripts and records what they read from the existing run, for the save to check. */ + private @Nullable PreparedTransform transformRun(ViewContext context, ExpProtocol protocol, JSONObject runJson) throws ExperimentException, ValidationException { ExpRun existingRun = lookupRun(context, runJson, null); if (!hasRunContent(runJson)) @@ -337,10 +321,10 @@ private void checkRun(ViewContext context, ExpProtocol protocol, JSONObject runJ try { - PreparedTransform prepared = prepareTransform(context, protocol, runJson, existingRun); + PreparedTransform prepared = runTransformScripts(context, protocol, runJson, existingRun); JSONObject serializedRun = existingRun == null ? null : _serializedRuns.get(existingRun.getLSID()); if (prepared != null && serializedRun != null) - _preparedSerializations.put(runJson, readFromSerialization(runJson, serializedRun)); + prepared.setScriptInputs(readFromSerialization(runJson, serializedRun)); return prepared; } finally @@ -351,7 +335,8 @@ private void checkRun(ViewContext context, ExpProtocol protocol, JSONObject runJ } } - private @Nullable PreparedTransform prepareTransform(ViewContext context, ExpProtocol protocol, JSONObject runJson, @Nullable ExpRun existingRun) throws ExperimentException, ValidationException + /** Builds the upload context handleRun would build for this run, without writing anything, and runs its transform scripts. */ + private @Nullable PreparedTransform runTransformScripts(ViewContext context, ExpProtocol protocol, JSONObject runJson, @Nullable ExpRun existingRun) throws ExperimentException, ValidationException { JSONArray dataArray = existingRun != null ? serializeRunData(context, existingRun, runJson, protocol, null) : postedArray(runJson, AssayJSONConverter.DATA_ROWS); JSONArray dataInputs = existingRun != null ? serializeDataInputs(context, existingRun, runJson, protocol, null) : postedArray(runJson, ExperimentJSONConverter.DATA_INPUTS); From 9318f416f2d95923ce67708612a2ecb69dad0543 Mon Sep 17 00:00:00 2001 From: labkey-jeckels Date: Wed, 28 Oct 2026 11:08:59 -0700 Subject: [PATCH 11/11] Consolidation and simplification --- .../api/assay/DefaultAssayRunCreator.java | 42 ++++---- .../assay/transform/PreparedTransform.java | 23 ++--- .../api/assay/DefaultAssaySaveHandler.java | 98 ++++++++++--------- .../assay/actions/BaseProtocolAPIAction.java | 11 --- 4 files changed, 76 insertions(+), 98 deletions(-) diff --git a/api/src/org/labkey/api/assay/DefaultAssayRunCreator.java b/api/src/org/labkey/api/assay/DefaultAssayRunCreator.java index c67ff35e188..2889e5c849f 100644 --- a/api/src/org/labkey/api/assay/DefaultAssayRunCreator.java +++ b/api/src/org/labkey/api/assay/DefaultAssayRunCreator.java @@ -152,22 +152,29 @@ private PreparedTransform prepareTransformAfterInit(AssayRunUploadContext fileAudits = new ArrayList<>(); context.setPendingFileAudits(fileAudits); - Map inputDatas = new HashMap<>(); - PreparedTransform prepared = null; + Map resolverInputs = new HashMap<>(); + PreparedTransform prepared; try { - addRunDataInputs(context, null, findResolverType(allProperties), inputDatas); + ParticipantVisitResolverType resolverType = findResolverType(allProperties); + if (resolverType != null) + resolverType.configureRun(context, null, resolverInputs); + Map inputDatas = new HashMap<>(resolverInputs); + addInputDatas(context, inputDatas); prepared = runTransformBeforeRun(context, inputDatas.keySet()); } + catch (Exception e) + { + PreparedTransform.addFileAuditEventsForRemainingFiles(context.getUser(), fileAudits); + throw e; + } finally { context.setPendingFileAudits(null); - if (prepared == null) - PreparedTransform.addFileAuditEventsForRemainingFiles(context.getUser(), fileAudits); } prepared.holdFileAudits(context.getUser(), fileAudits); - // Run creation reuses these rather than collecting again, since a thaw list resolver writes a new file each time - prepared.setRunDataInputs(inputDatas); + // Run creation reuses these rather than resolving again, since a thaw list resolver writes a new file each time + prepared.setResolverDataInputs(resolverInputs); return prepared; } @@ -180,18 +187,6 @@ private PreparedTransform prepareTransformAfterInit(AssayRunUploadContext context, - @Nullable ExpRun run, - @Nullable ParticipantVisitResolverType resolverType, - Map inputDatas - ) throws ExperimentException, ValidationException - { - if (resolverType != null) - resolverType.configureRun(context, run, inputDatas); - addInputDatas(context, inputDatas); - } - private boolean hasTransformBeforeRun(AssayRunUploadContext context) { return context.getProvider().isTransformBeforeRunSupported(context.getProtocol()) @@ -411,10 +406,11 @@ public ExpExperiment saveExperimentRun( ParticipantVisitResolverType resolverType = findResolverType(allProperties); PreparedTransform preparedBeforeRun = context.getPreparedTransform(); - if (preparedBeforeRun != null && preparedBeforeRun.getRunDataInputs() != null) - inputDatas.putAll(preparedBeforeRun.getRunDataInputs()); - else - addRunDataInputs(context, run, resolverType, inputDatas); + if (preparedBeforeRun != null && preparedBeforeRun.getResolverDataInputs() != null) + inputDatas.putAll(preparedBeforeRun.getResolverDataInputs()); + else if (resolverType != null) + resolverType.configureRun(context, run, inputDatas); + addInputDatas(context, inputDatas); // TODO: Share these RemapCache and materialCache instances with AbstractAssayTsvDataHandler.checkData and ExpressionMatrixDataHandler.importFile // Cache of resolved alternate lookup keys -> rowId diff --git a/api/src/org/labkey/api/assay/transform/PreparedTransform.java b/api/src/org/labkey/api/assay/transform/PreparedTransform.java index 068137e80d6..9848cf76883 100644 --- a/api/src/org/labkey/api/assay/transform/PreparedTransform.java +++ b/api/src/org/labkey/api/assay/transform/PreparedTransform.java @@ -44,7 +44,6 @@ import java.io.IOException; import java.nio.file.Path; import java.util.ArrayList; -import java.util.HashMap; import java.util.HashSet; import java.util.List; import java.util.Map; @@ -65,7 +64,7 @@ public class PreparedTransform implements AutoCloseable private final @NotNull List _scriptOutputs; private final @NotNull List _scriptDirs; private boolean _recorded; - private @Nullable Map _runDataInputs; + private @Nullable Map _resolverDataInputs; private @Nullable JSONObject _scriptInputs; private final @NotNull List _fileAudits = new ArrayList<>(); private @Nullable User _fileAuditUser; @@ -116,25 +115,15 @@ static PreparedTransform empty() return files; } - /** The run's data inputs, when collected before the scripts ran; null otherwise. */ - public @Nullable Map getRunDataInputs() + /** The data inputs the participant visit resolver added before the scripts ran; null when it didn't run then. */ + public @Nullable Map getResolverDataInputs() { - return _runDataInputs; + return _resolverDataInputs; } - public void setRunDataInputs(@NotNull Map runDataInputs) + public void setResolverDataInputs(@NotNull Map resolverDataInputs) { - _runDataInputs = runDataInputs; - } - - /** Adds inputs to those collected before the scripts ran; a no-op when none were collected. */ - public void addRunDataInputs(@NotNull Map runDataInputs) - { - if (_runDataInputs == null) - return; - Map merged = new HashMap<>(_runDataInputs); - merged.putAll(runDataInputs); - _runDataInputs = merged; + _resolverDataInputs = resolverDataInputs; } /** What the scripts read from the existing run, for the save to compare once its transaction reads the run again. */ diff --git a/assay/api-src/org/labkey/api/assay/DefaultAssaySaveHandler.java b/assay/api-src/org/labkey/api/assay/DefaultAssaySaveHandler.java index d2639de44a9..800b65f4e2e 100644 --- a/assay/api-src/org/labkey/api/assay/DefaultAssaySaveHandler.java +++ b/assay/api-src/org/labkey/api/assay/DefaultAssaySaveHandler.java @@ -22,6 +22,7 @@ import org.jetbrains.annotations.Nullable; import org.json.JSONArray; import org.json.JSONObject; +import org.labkey.api.action.ApiUsageException; import org.labkey.api.action.BaseViewAction; import org.labkey.api.assay.transform.PreparedTransform; import org.labkey.api.assay.transform.PreparedTransforms; @@ -53,13 +54,13 @@ import java.util.ArrayList; import java.util.Collection; +import java.util.HashSet; import java.util.Collections; import java.util.HashMap; import java.util.IdentityHashMap; import java.util.List; import java.util.Map; import java.util.Set; -import java.util.stream.Collectors; public class DefaultAssaySaveHandler extends DefaultExperimentSaveHandler implements AssaySaveHandler { @@ -232,7 +233,6 @@ protected ExpExperiment saveExperimentRun( if (!prepared.scriptInputsMatch(serializedRun == null ? null : readFromSerialization(runJson, serializedRun))) throw new ValidationException("Run " + run.getRowId() + " changed while its transform scripts ran. Retry the save."); - prepared.addRunDataInputs(inputData); uploadContext.setPreparedTransform(prepared); } @@ -274,18 +274,34 @@ protected boolean isPrepareTransformsSupported() || getProvider().getValidationAndAnalysisScripts(protocol, AssayProvider.Scope.ALL).isEmpty()) return preparedTransforms; + // A repeated run's later posts would save over what its scripts saw before the transaction + Set runIds = new HashSet<>(); + for (JSONObject runJson : runJsons) + { + if (!runJson.isNull(ExperimentJSONConverter.ID) && !runIds.add(runJson.getInt(ExperimentJSONConverter.ID))) + throw new ApiUsageException("Run " + runJson.getInt(ExperimentJSONConverter.ID) + " appears more than once in the request"); + } + // A script's side effects can't be rolled back, so refuse the request before any of them runs + List postedRuns = new ArrayList<>(); for (JSONObject runJson : runJsons) - checkRun(context, protocol, runJson); + { + PostedRun posted = readPostedRun(context, protocol, runJson); + if (posted != null) + { + checkProtocolApplications(context, posted.existingRun(), posted.dataInputs(), posted.materialInputs(), posted.dataOutputs(), posted.materialOutputs()); + postedRuns.add(posted); + } + } boolean success = false; try { - for (JSONObject runJson : runJsons) + for (PostedRun posted : postedRuns) { - PreparedTransform prepared = transformRun(context, protocol, runJson); + PreparedTransform prepared = runTransformScripts(context, protocol, posted); if (prepared != null) - preparedTransforms.put(runJson, prepared); + preparedTransforms.put(posted.json(), prepared); } success = true; } @@ -298,73 +314,61 @@ protected boolean isPrepareTransformsSupported() return preparedTransforms; } - private void checkRun(ViewContext context, ExpProtocol protocol, JSONObject runJson) throws ExperimentException, ValidationException - { - ExpRun existingRun = lookupRun(context, runJson, null); - if (!hasRunContent(runJson)) - return; - - // An existing run's serialization stays cached for transformRun, which reads it next - checkProtocolApplications(context, existingRun, - existingRun != null ? serializeDataInputs(context, existingRun, runJson, protocol, null) : postedArray(runJson, ExperimentJSONConverter.DATA_INPUTS), - existingRun != null ? serializeMaterialInputs(context, existingRun, runJson, protocol, null) : postedArray(runJson, ExperimentJSONConverter.MATERIAL_INPUTS), - existingRun != null ? serializeDataOutputs(context, existingRun, runJson, protocol, null) : postedArray(runJson, ExperimentJSONConverter.DATA_OUTPUTS), - existingRun != null ? serializeMaterialOutputs(context, existingRun, runJson, protocol, null) : postedArray(runJson, ExperimentJSONConverter.MATERIAL_OUTPUTS)); - } + /** A posted run with the arrays handleRun would save, read from the existing run where the client didn't post them. */ + private record PostedRun(JSONObject json, @Nullable ExpRun existingRun, JSONArray dataRows, JSONArray dataInputs, JSONArray materialInputs, + JSONArray dataOutputs, JSONArray materialOutputs, @Nullable JSONObject scriptInputs) {} - /** Runs this run's transform scripts and records what they read from the existing run, for the save to check. */ - private @Nullable PreparedTransform transformRun(ViewContext context, ExpProtocol protocol, JSONObject runJson) throws ExperimentException, ValidationException + /** Null when the post changes only the run's properties, which runs no scripts. */ + private @Nullable PostedRun readPostedRun(ViewContext context, ExpProtocol protocol, JSONObject runJson) { ExpRun existingRun = lookupRun(context, runJson, null); if (!hasRunContent(runJson)) return null; + if (existingRun == null) + { + return new PostedRun(runJson, null, postedArray(runJson, AssayJSONConverter.DATA_ROWS), + postedArray(runJson, ExperimentJSONConverter.DATA_INPUTS), postedArray(runJson, ExperimentJSONConverter.MATERIAL_INPUTS), + postedArray(runJson, ExperimentJSONConverter.DATA_OUTPUTS), postedArray(runJson, ExperimentJSONConverter.MATERIAL_OUTPUTS), null); + } + try { - PreparedTransform prepared = runTransformScripts(context, protocol, runJson, existingRun); - JSONObject serializedRun = existingRun == null ? null : _serializedRuns.get(existingRun.getLSID()); - if (prepared != null && serializedRun != null) - prepared.setScriptInputs(readFromSerialization(runJson, serializedRun)); - return prepared; + JSONArray dataRows = serializeRunData(context, existingRun, runJson, protocol, null); + JSONArray dataInputs = serializeDataInputs(context, existingRun, runJson, protocol, null); + JSONArray materialInputs = serializeMaterialInputs(context, existingRun, runJson, protocol, null); + JSONArray dataOutputs = serializeDataOutputs(context, existingRun, runJson, protocol, null); + JSONArray materialOutputs = serializeMaterialOutputs(context, existingRun, runJson, protocol, null); + JSONObject serializedRun = _serializedRuns.get(existingRun.getLSID()); + return new PostedRun(runJson, existingRun, dataRows, dataInputs, materialInputs, dataOutputs, materialOutputs, + serializedRun == null ? null : readFromSerialization(runJson, serializedRun)); } finally { // The save pass reads the run again inside its transaction - if (existingRun != null) - _serializedRuns.remove(existingRun.getLSID()); + _serializedRuns.remove(existingRun.getLSID()); } } /** Builds the upload context handleRun would build for this run, without writing anything, and runs its transform scripts. */ - private @Nullable PreparedTransform runTransformScripts(ViewContext context, ExpProtocol protocol, JSONObject runJson, @Nullable ExpRun existingRun) throws ExperimentException, ValidationException + private @Nullable PreparedTransform runTransformScripts(ViewContext context, ExpProtocol protocol, PostedRun posted) throws ExperimentException, ValidationException { - JSONArray dataArray = existingRun != null ? serializeRunData(context, existingRun, runJson, protocol, null) : postedArray(runJson, AssayJSONConverter.DATA_ROWS); - JSONArray dataInputs = existingRun != null ? serializeDataInputs(context, existingRun, runJson, protocol, null) : postedArray(runJson, ExperimentJSONConverter.DATA_INPUTS); - JSONArray dataOutputs = existingRun != null ? serializeDataOutputs(context, existingRun, runJson, protocol, null) : postedArray(runJson, ExperimentJSONConverter.DATA_OUTPUTS); - // Unsaved data all have rowId 0 and can share an LSID, so key by file instead of ExpData equality Map dataByKey = new HashMap<>(); Map inputData = new IdentityHashMap<>(); - for (JSONObject dataObject : JsonUtil.toJSONObjectList(dataInputs)) + for (JSONObject dataObject : JsonUtil.toJSONObjectList(posted.dataInputs())) inputData.put(lookupData(context, dataObject, dataByKey), dataObject.optString(ExperimentJSONConverter.ROLE, ExpDataRunInput.DEFAULT_ROLE)); - List> dataRows = convertRunData(dataArray, context.getContainer(), protocol); - if (skipsAssayRun(dataRows, !dataOutputs.isEmpty())) + List> dataRows = convertRunData(posted.dataRows(), context.getContainer(), protocol); + if (skipsAssayRun(dataRows, !posted.dataOutputs().isEmpty())) return null; // Scripts never see output data, so the outputs are only counted, not resolved - AssayRunUploadContext uploadContext = createRunUploadContext(context, protocol, runJson, dataRows, + AssayRunUploadContext uploadContext = createRunUploadContext(context, protocol, posted.json(), dataRows, inputData, Collections.emptyMap(), Collections.emptyMap(), Collections.emptyMap()); - AssayRunCreator runCreator = getProvider().getRunCreator(); - PreparedTransform prepared = runCreator.prepareTransform(uploadContext); - if (prepared != null && prepared.getRunDataInputs() != null) - { - // saveExperimentRun adds the saved inputs back; keep only those the participant visit resolver added - Set postedKeys = inputData.keySet().stream().map(DefaultAssaySaveHandler::dataKey).collect(Collectors.toSet()); - Map resolverInputs = new IdentityHashMap<>(prepared.getRunDataInputs()); - resolverInputs.keySet().removeIf(data -> postedKeys.contains(dataKey(data))); - prepared.setRunDataInputs(resolverInputs); - } + PreparedTransform prepared = getProvider().getRunCreator().prepareTransform(uploadContext); + if (prepared != null && posted.scriptInputs() != null) + prepared.setScriptInputs(posted.scriptInputs()); return prepared; } diff --git a/assay/src/org/labkey/assay/actions/BaseProtocolAPIAction.java b/assay/src/org/labkey/assay/actions/BaseProtocolAPIAction.java index 909de067a0a..c6ce1c8ab54 100644 --- a/assay/src/org/labkey/assay/actions/BaseProtocolAPIAction.java +++ b/assay/src/org/labkey/assay/actions/BaseProtocolAPIAction.java @@ -19,7 +19,6 @@ import org.jetbrains.annotations.Nullable; import org.json.JSONObject; import org.labkey.api.action.ApiResponse; -import org.labkey.api.action.ApiUsageException; import org.labkey.api.action.MutatingApiAction; import org.labkey.api.action.SimpleApiJsonForm; import org.labkey.api.assay.AssayProvider; @@ -39,9 +38,7 @@ import org.labkey.api.view.NotFoundException; import org.springframework.validation.BindException; -import java.util.HashSet; import java.util.List; -import java.util.Set; import java.util.stream.Collectors; public abstract class BaseProtocolAPIAction extends MutatingApiAction @@ -163,14 +160,6 @@ public ExperimentSaveHandler getExperimentSaveHandler(@Nullable AssayProvider pr /** GH Issue 1720: run transform scripts before the save's transaction so their callbacks into the server can't deadlock with it */ protected PreparedTransforms prepareTransforms(ExperimentSaveHandler saveHandler, ExpProtocol protocol, List runJsons) throws ExperimentException, ValidationException { - // A repeated run's later posts would save over what its scripts saw before the transaction - Set runIds = new HashSet<>(); - for (JSONObject runJson : runJsons) - { - if (!runJson.isNull(ExperimentJSONConverter.ID) && !runIds.add(runJson.getInt(ExperimentJSONConverter.ID))) - throw new ApiUsageException("Run " + runJson.getInt(ExperimentJSONConverter.ID) + " appears more than once in the request"); - } - if (saveHandler instanceof AssaySaveHandler assaySaveHandler) return assaySaveHandler.prepareTransforms(getViewContext(), protocol, runJsons); return new PreparedTransforms();