diff --git a/api/src/org/labkey/api/dataiterator/DataIteratorUtil.java b/api/src/org/labkey/api/dataiterator/DataIteratorUtil.java index 620b31fd639..6958c128865 100644 --- a/api/src/org/labkey/api/dataiterator/DataIteratorUtil.java +++ b/api/src/org/labkey/api/dataiterator/DataIteratorUtil.java @@ -60,13 +60,15 @@ import java.util.function.Consumer; import java.util.function.Function; import java.util.function.Supplier; +import java.util.function.UnaryOperator; import java.util.stream.Stream; import java.util.stream.StreamSupport; /** - * User: matthewb - * Date: 2011-05-31 - * Time: 12:52 PM + * Static helpers for assembling DataIterator pipelines: matching a source iterator's columns to a target + * TableInfo's columns (by property URI, name, import alias, or JDBC-legal name) and copying or merging the + * result into that table. Also provides adapters that present an iterator as scrollable, map-based, + * map-transforming, or a Stream of maps. */ public class DataIteratorUtil { @@ -373,6 +375,28 @@ public static void closeQuietly(DataIterator it) } } + /** + * DataIteratorBuilder.getDataIterator() calls this to simplify implementing the success or close() input contract + */ + public static @Nullable DataIterator wrapOrClose(DataIteratorBuilder in, DataIteratorContext context, UnaryOperator wrapper) + { + DataIterator di = in.getDataIterator(context); + if (null == di) + return null; + + try + { + DataIterator out = wrapper.apply(di); + if (null == out) + closeQuietly(di); + return out; + } + catch (RuntimeException | Error e) + { + closeQuietly(di); + throw e; + } + } /* * Wrapping functions to add functionality to existing DataIterators diff --git a/experiment/src/org/labkey/experiment/ExpDataIterators.java b/experiment/src/org/labkey/experiment/ExpDataIterators.java index 841ce6bf8d2..893b020109b 100644 --- a/experiment/src/org/labkey/experiment/ExpDataIterators.java +++ b/experiment/src/org/labkey/experiment/ExpDataIterators.java @@ -212,60 +212,58 @@ public CounterDataIteratorBuilder(@NotNull DataIteratorBuilder in, Container con @Override public DataIterator getDataIterator(DataIteratorContext context) { - DataIterator pre = _in.getDataIterator(context); - if (pre == null) - return null; // can happen if context has errors + return DataIteratorUtil.wrapOrClose(_in, context, pre -> { + SimpleTranslator counterTranslator = new SimpleTranslator(pre, context); + counterTranslator.setDebugName("Counter Def"); + Set skipColumns = new CaseInsensitiveHashSet(); + Map columnNameMap = DataIteratorUtil.createColumnNameMap(pre); - SimpleTranslator counterTranslator = new SimpleTranslator(pre, context); - counterTranslator.setDebugName("Counter Def"); - Set skipColumns = new CaseInsensitiveHashSet(); - Map columnNameMap = DataIteratorUtil.createColumnNameMap(pre); - - for (CounterDefinition counterDefinition : _expTable.getCounterDefinitions()) - { - Set attachedColumnNames = counterDefinition.getAttachedColumnNames(); - skipColumns.addAll(attachedColumnNames); - - // validate we have all the paired columns - List pairedIndexes = new IntArrayList(); - for (String pairedColumnName : counterDefinition.getPairedColumnNames()) + for (CounterDefinition counterDefinition : _expTable.getCounterDefinitions()) { - Integer i = columnNameMap.get(pairedColumnName); - if (i == null) - { - // immediately return error iterator tied to the input DataIterator instead of counterTranslator - ValidationException setupError = new ValidationException(); - setupError.addGlobalError("Paired column '" + pairedColumnName + "' is required for counter '" + counterDefinition.getCounterName() + "'"); - return ErrorIterator.wrap(pre, context, true, setupError); - } - else - { - pairedIndexes.add(i); - } - } + Set attachedColumnNames = counterDefinition.getAttachedColumnNames(); + skipColumns.addAll(attachedColumnNames); - // add a sequence column for each of the attached columns - for (String columnName : attachedColumnNames) - { - Integer i = columnNameMap.get(columnName); - ColumnInfo column; - if (null != i) + // validate we have all the paired columns + List pairedIndexes = new IntArrayList(); + for (String pairedColumnName : counterDefinition.getPairedColumnNames()) { - column = pre.getColumnInfo(i); - skipColumns.add(columnName); + Integer i = columnNameMap.get(pairedColumnName); + if (i == null) + { + // immediately return error iterator tied to the input DataIterator instead of counterTranslator + ValidationException setupError = new ValidationException(); + setupError.addGlobalError("Paired column '" + pairedColumnName + "' is required for counter '" + counterDefinition.getCounterName() + "'"); + return ErrorIterator.wrap(pre, context, true, setupError); + } + else + { + pairedIndexes.add(i); + } } - else + + // add a sequence column for each of the attached columns + for (String columnName : attachedColumnNames) { - column = _expTable.getColumn(columnName); - } + Integer i = columnNameMap.get(columnName); + ColumnInfo column; + if (null != i) + { + column = pre.getColumnInfo(i); + skipColumns.add(columnName); + } + else + { + column = _expTable.getColumn(columnName); + } - counterTranslator.addPairedSequenceColumn(column, i, _container, counterDefinition, pairedIndexes, _sequencePrefix, _id, 100); + counterTranslator.addPairedSequenceColumn(column, i, _container, counterDefinition, pairedIndexes, _sequencePrefix, _id, 100); + } } - } - counterTranslator.selectAll(skipColumns); + counterTranslator.selectAll(skipColumns); - return LoggingDataIterator.wrap(counterTranslator); + return LoggingDataIterator.wrap(counterTranslator); + }); } } @@ -347,11 +345,8 @@ public AliquotRollupDataIteratorBuilder(@NotNull DataIteratorBuilder in, Contain @Override public DataIterator getDataIterator(DataIteratorContext context) { - DataIterator pre = _in.getDataIterator(context); - if (pre == null) - return null; // can happen if context has errors - - return LoggingDataIterator.wrap(new AliquotRollupDataIterator(pre, context, _container)); + return DataIteratorUtil.wrapOrClose(_in, context, + pre -> LoggingDataIterator.wrap(new AliquotRollupDataIterator(pre, context, _container))); } } @@ -510,11 +505,8 @@ public AliasDataIteratorBuilder(@NotNull DataIteratorBuilder in, Container conta @Override public DataIterator getDataIterator(DataIteratorContext context) { - DataIterator di = _in.getDataIterator(context); - if (di == null) - return null; // can happen if context has errors - - return LoggingDataIterator.wrap(new AliasDataIterator(di, context, _container, _user, _expAliasTable, _dataType, _isSample)); + return DataIteratorUtil.wrapOrClose(_in, context, + di -> LoggingDataIterator.wrap(new AliasDataIterator(di, context, _container, _user, _expAliasTable, _dataType, _isSample))); } } @@ -631,11 +623,8 @@ public AutoLinkToStudyDataIteratorBuilder(@NotNull DataIteratorBuilder in, UserS @Override public DataIterator getDataIterator(DataIteratorContext context) { - DataIterator pre = _in.getDataIterator(context); - if (pre == null) - return null; // can happen if context has errors - - return LoggingDataIterator.wrap(new AutoLinkToStudyDataIterator(DataIteratorUtil.wrapMap(pre, false), _schema, _container, _user, _sampleType)); + return DataIteratorUtil.wrapOrClose(_in, context, + pre -> LoggingDataIterator.wrap(new AutoLinkToStudyDataIterator(DataIteratorUtil.wrapMap(pre, false), _schema, _container, _user, _sampleType))); } } @@ -754,11 +743,8 @@ public FlagDataIteratorBuilder(@NotNull DataIteratorBuilder in, User user, boole @Override public DataIterator getDataIterator(DataIteratorContext context) { - DataIterator pre = _in.getDataIterator(context); - if (pre == null) - return null; // can happen if context has errors - - return LoggingDataIterator.wrap(new FlagDataIterator(pre, context, _user, _isSample, _expObject, _container)); + return DataIteratorUtil.wrapOrClose(_in, context, + pre -> LoggingDataIterator.wrap(new FlagDataIterator(pre, context, _user, _isSample, _expObject, _container))); } } @@ -895,21 +881,19 @@ public DerivationDataIteratorBuilder(DataIteratorBuilder pre, Container containe @Override public DataIterator getDataIterator(DataIteratorContext context) { - DataIterator di = _pre.getDataIterator(context); - if (di == null) - return null; // can happen if context has errors + return DataIteratorUtil.wrapOrClose(_pre, context, di -> { + if (context.getConfigParameters().containsKey(SampleTypeUpdateServiceDI.Options.SkipDerivation)) + return di; - if (context.getConfigParameters().containsKey(SampleTypeUpdateServiceDI.Options.SkipDerivation)) - return di; - - if (context.getInsertOption() != QueryUpdateService.InsertOption.UPDATE) - di = new DerivationDataIterator(di, context, _container, _user, _currentDataType, _isSample, _skipAliquot); - else if (_isSample) - di = new SampleUpdateDerivationDataIterator(di, context, _container, _user, _currentDataType, _checkRequiredParents); - else - di = new DataUpdateDerivationDataIterator(di, context, _container, _user, _currentDataType, _checkRequiredParents); + if (context.getInsertOption() != QueryUpdateService.InsertOption.UPDATE) + di = new DerivationDataIterator(di, context, _container, _user, _currentDataType, _isSample, _skipAliquot); + else if (_isSample) + di = new SampleUpdateDerivationDataIterator(di, context, _container, _user, _currentDataType, _checkRequiredParents); + else + di = new DataUpdateDerivationDataIterator(di, context, _container, _user, _currentDataType, _checkRequiredParents); - return LoggingDataIterator.wrap(di); + return LoggingDataIterator.wrap(di); + }); } } @@ -2124,11 +2108,8 @@ public SearchIndexIteratorBuilder(DataIteratorBuilder pre, Function LoggingDataIterator.wrap(new SearchIndexIterator(pre, context, _indexFunction))); } } @@ -2365,10 +2346,11 @@ public PersistDataIteratorBuilder setFileLinkDirectory(String dir) @Override public DataIterator getDataIterator(DataIteratorContext context) { - DataIterator input = _in.getDataIterator(context); - if (null == input) - return null; // Can happen if context has errors + return DataIteratorUtil.wrapOrClose(_in, context, input -> build(input, context)); + } + private DataIterator build(DataIterator input, DataIteratorContext context) + { // useTransactionAuditCache already set for import and merge in AbstractQueryImportAction.createDataIteratorContext if (context.getInsertOption() == QueryUpdateService.InsertOption.INSERT) { @@ -2536,26 +2518,21 @@ public SampleUpdateOnlyValidatorsIteratorBuilder(@NotNull DataIteratorBuilder in @Override public DataIterator getDataIterator(DataIteratorContext context) { - DataIterator di = _in.getDataIterator(context); - if (di == null) - return null; // can happen if context has errors - - ValidatorIterator validate = new ValidatorIterator(di, context, _container, _user); - Map map = DataIteratorUtil.createColumnNameMap(validate); - - Integer index = map.get(Name.name()); - if (index != null) - { - ColumnInfo column = di.getColumnInfo(index); - validate.addValidator(index, new RequiredValidator(column.getColumnName(), column.getJdbcType(), false, false, "Sample name cannot be blank")); - } + return DataIteratorUtil.wrapOrClose(_in, context, di -> { + ValidatorIterator validate = new ValidatorIterator(di, context, _container, _user); + Map map = DataIteratorUtil.createColumnNameMap(validate); - // Add other column validators here... + Integer index = map.get(Name.name()); + if (index != null) + { + ColumnInfo column = di.getColumnInfo(index); + validate.addValidator(index, new RequiredValidator(column.getColumnName(), column.getJdbcType(), false, false, "Sample name cannot be blank")); + } - if (validate.hasValidators()) - di = validate; + // Add other column validators here... - return LoggingDataIterator.wrap(di); + return LoggingDataIterator.wrap(validate.hasValidators() ? validate : di); + }); } } @@ -2575,11 +2552,8 @@ public SampleNameChangeDataIteratorBuilder(@NotNull DataIteratorBuilder in, User @Override public DataIterator getDataIterator(DataIteratorContext context) { - DataIterator di = _in.getDataIterator(context); - if (di == null) - return null; // can happen if context has errors - - return LoggingDataIterator.wrap(new SampleNameChangeDataIterator(di, context, _user, _canUpdateNames)); + return DataIteratorUtil.wrapOrClose(_in, context, + di -> LoggingDataIterator.wrap(new SampleNameChangeDataIterator(di, context, _user, _canUpdateNames))); } } @@ -3202,11 +3176,8 @@ public MultiDataTypeCrossProjectDataIteratorBuilder(@NotNull User user, @NotNull @Override public DataIterator getDataIterator(DataIteratorContext context) { - DataIterator di = _in.getDataIterator(context); - if (di == null) - return null; // can happen if context has errors - - return LoggingDataIterator.wrap(new MultiDataTypeCrossProjectDataIterator(di, context, _container, _user, _isCrossType, _dataType, _isSamples)); + return DataIteratorUtil.wrapOrClose(_in, context, + di -> LoggingDataIterator.wrap(new MultiDataTypeCrossProjectDataIterator(di, context, _container, _user, _isCrossType, _dataType, _isSamples))); } } @@ -3232,11 +3203,8 @@ public SampleStatusCheckIteratorBuilder(@NotNull DataIteratorBuilder in, Contain @Override public DataIterator getDataIterator(DataIteratorContext context) { - DataIterator pre = _in.getDataIterator(context); - if (pre == null) - return null; // can happen if context has errors - - return LoggingDataIterator.wrap(new SampleStatusCheckDataIterator(pre, context, _container)); + return DataIteratorUtil.wrapOrClose(_in, context, + pre -> LoggingDataIterator.wrap(new SampleStatusCheckDataIterator(pre, context, _container))); } } diff --git a/experiment/src/org/labkey/experiment/ExperimentModule.java b/experiment/src/org/labkey/experiment/ExperimentModule.java index 5cd675c69c4..ebd0baf6802 100644 --- a/experiment/src/org/labkey/experiment/ExperimentModule.java +++ b/experiment/src/org/labkey/experiment/ExperimentModule.java @@ -132,6 +132,7 @@ import org.labkey.experiment.api.ExperimentServiceImpl; import org.labkey.experiment.api.ExperimentStressTest; import org.labkey.experiment.api.GraphAlgorithms; +import org.labkey.experiment.api.ImportAbortResourceTestCase; import org.labkey.experiment.api.LineageTest; import org.labkey.experiment.api.LogDataType; import org.labkey.experiment.api.Protocol; @@ -1135,6 +1136,7 @@ public Collection getSummary(Container c) ExperimentServiceImpl.ParseInputOutputAliasTestCase.class, ExperimentServiceImpl.TestCase.class, ExperimentStressTest.class, + ImportAbortResourceTestCase.class, LineagePerfTest.class, LineageTest.class, OntologyManager.TestCase.class, diff --git a/experiment/src/org/labkey/experiment/api/ExpDataClassDataTableImpl.java b/experiment/src/org/labkey/experiment/api/ExpDataClassDataTableImpl.java index 8ac51e4d2f5..db15c9d080a 100644 --- a/experiment/src/org/labkey/experiment/api/ExpDataClassDataTableImpl.java +++ b/experiment/src/org/labkey/experiment/api/ExpDataClassDataTableImpl.java @@ -978,10 +978,12 @@ private static boolean isReservedHeader(String name) public DataIterator getDataIterator(DataIteratorContext context) { _context = context; - DataIterator input = _in.getDataIterator(context); - if (null == input) - return null; // Can happen if context has errors + return DataIteratorUtil.wrapOrClose(_in, context, input -> build(input, context)); + } + /** Returning null here (after adding an error) or throwing leaves `input` to be closed by wrapOrClose. */ + private DataIterator build(DataIterator input, DataIteratorContext context) + { boolean isMerge = context.getInsertOption() == QueryUpdateService.InsertOption.MERGE; boolean isUpdate = context.getInsertOption() == QueryUpdateService.InsertOption.UPDATE; diff --git a/experiment/src/org/labkey/experiment/api/ImportAbortResourceTestCase.java b/experiment/src/org/labkey/experiment/api/ImportAbortResourceTestCase.java new file mode 100644 index 00000000000..d1336d725b0 --- /dev/null +++ b/experiment/src/org/labkey/experiment/api/ImportAbortResourceTestCase.java @@ -0,0 +1,217 @@ +/* + * 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.experiment.api; + +import org.junit.AfterClass; +import org.junit.Assert; +import org.junit.BeforeClass; +import org.junit.Test; +import org.labkey.api.collections.CaseInsensitiveHashMap; +import org.labkey.api.data.Container; +import org.labkey.api.data.TableInfo; +import org.labkey.api.data.TableSelector; +import org.labkey.api.dataiterator.DataIterator; +import org.labkey.api.dataiterator.DataIteratorBuilder; +import org.labkey.api.dataiterator.DataIteratorContext; +import org.labkey.api.dataiterator.MapDataIterator; +import org.labkey.api.dataiterator.WrapperDataIterator; +import org.labkey.api.exp.api.ExpSampleType; +import org.labkey.api.exp.api.ExperimentService; +import org.labkey.api.exp.api.SampleTypeService; +import org.labkey.api.exp.query.ExpSchema; +import org.labkey.api.exp.query.SamplesSchema; +import org.labkey.api.gwt.client.model.GWTPropertyDescriptor; +import org.labkey.api.query.QueryService; +import org.labkey.api.query.QueryUpdateService; +import org.labkey.api.query.QueryUpdateService.InsertOption; +import org.labkey.api.security.User; +import org.labkey.api.settings.OptionalFeatureService; +import org.labkey.api.util.JunitUtil; +import org.labkey.api.util.TestContext; +import org.labkey.experiment.ExpDataIterators; + +import java.io.IOException; +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; +import java.util.Map; + +/** + * A DataIteratorBuilder that bails out after its input has been built must close that input. The input is + * frequently a query-backed iterator holding a live ResultSet, its Statement, and a pooled Connection; dropping + * it strands all three until the ResultSetImpl Cleaner runs at GC, which on a short-scheduled ETL can exhaust + * the pool. Asserting on close() rather than on pool counts keeps this deterministic - no GC, no timing. + */ +public class ImportAbortResourceTestCase extends Assert +{ + private static Container _c; + private static User _user; + private static boolean _restoreAllowRowIdMerge; + + @BeforeClass + public static void setUp() + { + JunitUtil.deleteTestContainer(); + _c = JunitUtil.getTestContainer(); + _user = TestContext.get().getUser(); + + // These tests drive the RowId-on-merge rejection, so the server-wide opt-out must be off. + _restoreAllowRowIdMerge = OptionalFeatureService.get().isFeatureEnabled(ExperimentService.EXPERIMENTAL_FEATURE_ALLOW_ROW_ID_MERGE); + if (_restoreAllowRowIdMerge) + OptionalFeatureService.get().setFeatureEnabled(ExperimentService.EXPERIMENTAL_FEATURE_ALLOW_ROW_ID_MERGE, false, _user); + } + + @AfterClass + public static void tearDown() + { + if (_restoreAllowRowIdMerge) + OptionalFeatureService.get().setFeatureEnabled(ExperimentService.EXPERIMENTAL_FEATURE_ALLOW_ROW_ID_MERGE, true, _user); + JunitUtil.deleteTestContainer(); + } + + /** A row source that records every iterator it hands out, so an unclosed one is a deterministic failure. */ + private static class RecordingSource implements DataIteratorBuilder + { + private final DataIteratorBuilder _rows; + private final List _built = new ArrayList<>(); + + RecordingSource(List> rows) + { + _rows = MapDataIterator.of(rows); + } + + @Override + public DataIterator getDataIterator(DataIteratorContext context) + { + CloseRecordingDataIterator di = new CloseRecordingDataIterator(_rows.getDataIterator(context)); + _built.add(di); + return di; + } + + private static class CloseRecordingDataIterator extends WrapperDataIterator + { + private boolean _closed = false; + + CloseRecordingDataIterator(DataIterator di) + { + super(di); + } + + @Override + public void close() throws IOException + { + _closed = true; + super.close(); + } + } + } + + private void assertAllSourcesClosed(RecordingSource source) + { + assertFalse("source iterator was never built, so this test proves nothing", source._built.isEmpty()); + for (RecordingSource.CloseRecordingDataIterator di : source._built) + assertTrue("abandoned source DataIterator was left open", di._closed); + } + + /** Runs a merge expected to abort on a validation error, then asserts the source was closed. */ + private void assertMergeAbortClosesSource(TableInfo table, List> rows, String expectedError) throws Exception + { + RecordingSource source = new RecordingSource(rows); + DataIteratorContext context = new DataIteratorContext(); + context.setInsertOption(InsertOption.MERGE); + + QueryUpdateService qus = table.getUpdateService(); + assertNotNull("no QueryUpdateService for " + table.getName(), qus); + + // loadRows returns 0 whenever the context has errors, so it says nothing about what was written - count the table. + assertEquals("import reported inserted rows", 0, qus.loadRows(_user, _c, source, context, null)); + assertTrue("expected a validation error", context.getErrors().hasErrors()); + assertTrue("unexpected error: " + context.getErrors().getMessage(), + context.getErrors().getMessage().contains(expectedError)); + assertEquals("expected the import to abort without inserting", 0L, new TableSelector(table).getRowCount()); + assertAllSourcesClosed(source); + } + + private TableInfo createDataClassTable(String name) throws Exception + { + List props = List.of(new GWTPropertyDescriptor("prop", "string")); + ExperimentServiceImpl.get().createDataClass(_c, _user, name, null, props, Collections.emptyList(), null, null); + TableInfo table = QueryService.get().getUserSchema(_user, _c, ExpSchema.SCHEMA_EXP_DATA).getTable(name); + assertNotNull("could not resolve data class table " + name, table); + return table; + } + + private TableInfo createSampleTypeTable(String name) throws Exception + { + List props = List.of( + new GWTPropertyDescriptor("name", "string"), + new GWTPropertyDescriptor("prop", "string")); + ExpSampleType st = SampleTypeService.get().createSampleType(_c, _user, name, null, props, Collections.emptyList(), -1, -1, -1, -1, null); + TableInfo table = QueryService.get().getUserSchema(_user, _c, SamplesSchema.SCHEMA_NAME).getTable(st.getName()); + assertNotNull("could not resolve sample type table " + name, table); + return table; + } + + @Test + public void testDataClassRowIdOnMergeClosesSource() throws Exception + { + TableInfo table = createDataClassTable("AbortRowIdMerge"); + List> rows = List.of(CaseInsensitiveHashMap.of("Name", "D-1", "RowId", 1, "prop", "a")); + assertMergeAbortClosesSource(table, rows, "RowId is not accepted when merging data"); + } + + @Test + public void testDataClassLsidOnlyKeyOnMergeClosesSource() throws Exception + { + TableInfo table = createDataClassTable("AbortLsidMerge"); + // LSID as the only key column is rejected; Name and RowId are deliberately absent. + List> rows = List.of(CaseInsensitiveHashMap.of("LSID", "urn:lsid:labkey.com:Data.Folder-1:abort", "prop", "a")); + assertMergeAbortClosesSource(table, rows, "LSID is no longer accepted as a key for data"); + } + + @Test + public void testSampleTypeRowIdOnMergeClosesSource() throws Exception + { + TableInfo table = createSampleTypeTable("AbortRowIdMergeSamples"); + List> rows = List.of(CaseInsensitiveHashMap.of("Name", "S-1", "RowId", 1, "prop", "a")); + assertMergeAbortClosesSource(table, rows, "RowId is not accepted when merging samples"); + } + + @Test + public void testSampleTypeLsidOnlyKeyOnMergeClosesSource() throws Exception + { + TableInfo table = createSampleTypeTable("AbortLsidMergeSamples"); + List> rows = List.of(CaseInsensitiveHashMap.of("LSID", "urn:lsid:labkey.com:Sample.Folder-1:abort", "prop", "a")); + assertMergeAbortClosesSource(table, rows, "LSID is no longer accepted as a key for sample"); + } + + /** + * The other half of the contract: a builder whose iterator constructor throws must also close its input. + * AliasDataIteratorBuilder stands in for every ExpDataIterators builder routed through DataIteratorUtil.wrapOrClose. + */ + @Test + public void testBuilderThrowClosesSource() + { + RecordingSource source = new RecordingSource(List.of(CaseInsensitiveHashMap.of("Name", "D-1", "prop", "a"))); + DataIteratorContext context = new DataIteratorContext(); + context.setInsertOption(InsertOption.UPDATE); + + // A map-backed source doesn't support getExistingRecord(), which the AliasDataIterator ctor rejects on update. + DataIteratorBuilder builder = new ExpDataIterators.AliasDataIteratorBuilder(source, _c, _user, null, null, false); + assertThrows(IllegalArgumentException.class, () -> builder.getDataIterator(context)); + assertAllSourcesClosed(source); + } +} diff --git a/experiment/src/org/labkey/experiment/api/SampleTypeUpdateServiceDI.java b/experiment/src/org/labkey/experiment/api/SampleTypeUpdateServiceDI.java index 39b5dabde21..495b28e1caf 100644 --- a/experiment/src/org/labkey/experiment/api/SampleTypeUpdateServiceDI.java +++ b/experiment/src/org/labkey/experiment/api/SampleTypeUpdateServiceDI.java @@ -1267,10 +1267,12 @@ public PreTriggerDataIteratorBuilder(@NotNull ExpSampleTypeImpl sampleType, ExpM @Override public DataIterator getDataIterator(DataIteratorContext context) { - DataIterator di = builder.getDataIterator(context); - if (di == null) - return null; // can happen if context has errors + return DataIteratorUtil.wrapOrClose(builder, context, di -> build(di, context)); + } + /** Returning null here (after adding an error) or throwing leaves `di` to be closed by wrapOrClose. */ + private DataIterator build(DataIterator di, DataIteratorContext context) + { boolean isMerge = context.getInsertOption() == InsertOption.MERGE; boolean isUpdate = context.getInsertOption() == InsertOption.UPDATE;