From b5e701dab756c78e456fdb2473e7637d004a15af Mon Sep 17 00:00:00 2001 From: labkey-nicka Date: Fri, 16 May 2025 14:14:11 -0700 Subject: [PATCH 01/22] AliasDataIterator: support merge operation --- .../labkey/experiment/ExpDataIterators.java | 65 +++++++++++++++---- .../api/ExpDataClassDataTableImpl.java | 2 +- .../experiment/api/ExpMaterialTableImpl.java | 2 +- 3 files changed, 56 insertions(+), 13 deletions(-) diff --git a/experiment/src/org/labkey/experiment/ExpDataIterators.java b/experiment/src/org/labkey/experiment/ExpDataIterators.java index 1bdb4385312..5c2585b153d 100644 --- a/experiment/src/org/labkey/experiment/ExpDataIterators.java +++ b/experiment/src/org/labkey/experiment/ExpDataIterators.java @@ -492,20 +492,24 @@ public static class AliasDataIteratorBuilder implements DataIteratorBuilder private final Container _container; private final User _user; private final TableInfo _expAliasTable; + private final boolean _isSample; + private final ExpObject _dataType; - public AliasDataIteratorBuilder(@NotNull DataIteratorBuilder in, Container container, User user, TableInfo expTable) + public AliasDataIteratorBuilder(@NotNull DataIteratorBuilder in, Container container, User user, TableInfo expTable, ExpObject dataType, boolean isSample) { _in = in; _container = container; _user = user; _expAliasTable = expTable; + _isSample = isSample; + _dataType = dataType; } @Override public DataIterator getDataIterator(DataIteratorContext context) { DataIterator pre = _in.getDataIterator(context); - return LoggingDataIterator.wrap(new AliasDataIterator(pre, context, _container, _user, _expAliasTable)); + return LoggingDataIterator.wrap(new AliasDataIterator(pre, context, _container, _user, _expAliasTable, _dataType, _isSample)); } } @@ -513,25 +517,42 @@ private static class AliasDataIterator extends WrapperDataIterator { // For some reason I don't quite understand we don't want to pass through a column called "alias" so we rename it to ALIASCOLUMNALIAS final DataIteratorContext _context; - final Supplier _lsidCol; final Supplier _aliasCol; + final Supplier _lsidCol; + final Supplier _nameCol; final Container _container; final User _user; Map _lsidAliasMap = new HashMap<>(); private final TableInfo _expAliasTable; + private final ExpDataClass _dataClass; + private final ExpSampleType _sampleType; + private final boolean _isSample; - protected AliasDataIterator(DataIterator di, DataIteratorContext context, Container container, User user, TableInfo expTable) + protected AliasDataIterator(DataIterator di, DataIteratorContext context, Container container, User user, TableInfo expTable, ExpObject dataType, boolean isSample) { super(di); _context = context; Map map = DataIteratorUtil.createColumnNameMap(di); - _lsidCol = map.get("lsid")==null ? null : di.getSupplier(map.get("lsid")); - _aliasCol = map.get(ALIASCOLUMNALIAS)==null ? null : di.getSupplier(map.get(ALIASCOLUMNALIAS)); + _aliasCol = map.get(ALIASCOLUMNALIAS) == null ? null : di.getSupplier(map.get(ALIASCOLUMNALIAS)); + _lsidCol = map.get("lsid") == null ? null : di.getSupplier(map.get("lsid")); + _nameCol = map.get("name") == null ? null : di.getSupplier(map.get("name")); _container = container; _user = user; _expAliasTable = expTable; + _isSample = isSample; + + if (isSample) + { + _sampleType = (ExpSampleType) dataType; + _dataClass = null; + } + else + { + _sampleType = null; + _dataClass = (ExpDataClass) dataType; + } } private BatchValidationException getErrors() @@ -568,14 +589,36 @@ public boolean next() throws BatchValidationException } // For each iteration, collect the lsid and alias col values. - if (_lsidCol != null && _aliasCol != null) + if (_aliasCol != null) { - Object lsidValue = _lsidCol.get(); - Object aliasValue = _aliasCol.get(); + if (_context.getInsertOption() == QueryUpdateService.InsertOption.MERGE && _nameCol != null) + { + Object nameValue = _nameCol.get(); + if (nameValue instanceof String name) + { + ExpObject obj; + if (_isSample) + obj = _sampleType.getSample(_container, name); + else + obj = _dataClass.getData(_container, name); - if (aliasValue != null && lsidValue instanceof String) + if (obj != null) + { + String lsid = obj.getLSID(); + if (lsid != null && !lsid.isEmpty()) + _lsidAliasMap.put(lsid, _aliasCol.get()); + } + } + } + else if (_lsidCol != null) { - _lsidAliasMap.put((String) lsidValue, aliasValue); + Object lsidValue = _lsidCol.get(); + Object aliasValue = _aliasCol.get(); + + if (aliasValue != null && lsidValue instanceof String lsidString) + { + _lsidAliasMap.put(lsidString, aliasValue); + } } } return true; diff --git a/experiment/src/org/labkey/experiment/api/ExpDataClassDataTableImpl.java b/experiment/src/org/labkey/experiment/api/ExpDataClassDataTableImpl.java index 90408498d8b..917d31981a1 100644 --- a/experiment/src/org/labkey/experiment/api/ExpDataClassDataTableImpl.java +++ b/experiment/src/org/labkey/experiment/api/ExpDataClassDataTableImpl.java @@ -955,7 +955,7 @@ public DataIteratorBuilder persistRows(DataIteratorBuilder data, DataIteratorCon }, DbScope.CommitTaskOption.POSTCOMMIT)); } DataIteratorBuilder builder = LoggingDataIterator.wrap(step0); - return LoggingDataIterator.wrap(new AliasDataIteratorBuilder(builder, getUserSchema().getContainer(), getUserSchema().getUser(), ExperimentService.get().getTinfoDataAliasMap())); + return LoggingDataIterator.wrap(new AliasDataIteratorBuilder(builder, getUserSchema().getContainer(), getUserSchema().getUser(), ExperimentService.get().getTinfoDataAliasMap(), _dataClass, false)); } catch (IOException e) { diff --git a/experiment/src/org/labkey/experiment/api/ExpMaterialTableImpl.java b/experiment/src/org/labkey/experiment/api/ExpMaterialTableImpl.java index a91f7f8ef55..4bfa864166b 100644 --- a/experiment/src/org/labkey/experiment/api/ExpMaterialTableImpl.java +++ b/experiment/src/org/labkey/experiment/api/ExpMaterialTableImpl.java @@ -1627,7 +1627,7 @@ public DataIteratorBuilder persistRows(DataIteratorBuilder data, DataIteratorCon } DataIteratorBuilder builder = LoggingDataIterator.wrap(persist); - return LoggingDataIterator.wrap(new AliasDataIteratorBuilder(builder, getUserSchema().getContainer(), getUserSchema().getUser(), ExperimentService.get().getTinfoMaterialAliasMap())); + return LoggingDataIterator.wrap(new AliasDataIteratorBuilder(builder, getUserSchema().getContainer(), getUserSchema().getUser(), ExperimentService.get().getTinfoMaterialAliasMap(), _ss, true)); } catch (IOException e) { From 49b2294845c7843042f22391124e05d083a392a2 Mon Sep 17 00:00:00 2001 From: labkey-nicka Date: Fri, 16 May 2025 15:40:32 -0700 Subject: [PATCH 02/22] ExpObjectDataIterator --- .../api/exp/query/ExpMaterialTable.java | 4 +- .../labkey/experiment/ExpDataIterators.java | 292 ++++++------------ .../experiment/ExpObjectDataIterator.java | 92 ++++++ 3 files changed, 180 insertions(+), 208 deletions(-) create mode 100644 experiment/src/org/labkey/experiment/ExpObjectDataIterator.java diff --git a/api/src/org/labkey/api/exp/query/ExpMaterialTable.java b/api/src/org/labkey/api/exp/query/ExpMaterialTable.java index f9aa266de51..5b38a5767e1 100644 --- a/api/src/org/labkey/api/exp/query/ExpMaterialTable.java +++ b/api/src/org/labkey/api/exp/query/ExpMaterialTable.java @@ -37,6 +37,7 @@ enum Column Folder, Inputs, IsAliquot, + IsPlated, LSID, MaterialExpDate, MaterialSourceId, @@ -60,8 +61,7 @@ enum Column SourceProtocolApplication, SourceProtocolLSID, StoredAmount, - Units, - IsPlated; + Units; public FieldKey fieldKey() { diff --git a/experiment/src/org/labkey/experiment/ExpDataIterators.java b/experiment/src/org/labkey/experiment/ExpDataIterators.java index 5c2585b153d..02deeeac252 100644 --- a/experiment/src/org/labkey/experiment/ExpDataIterators.java +++ b/experiment/src/org/labkey/experiment/ExpDataIterators.java @@ -513,51 +513,24 @@ public DataIterator getDataIterator(DataIteratorContext context) } } - private static class AliasDataIterator extends WrapperDataIterator + private static class AliasDataIterator extends ExpObjectDataIterator { // For some reason I don't quite understand we don't want to pass through a column called "alias" so we rename it to ALIASCOLUMNALIAS - final DataIteratorContext _context; final Supplier _aliasCol; final Supplier _lsidCol; final Supplier _nameCol; - final Container _container; - final User _user; Map _lsidAliasMap = new HashMap<>(); private final TableInfo _expAliasTable; - private final ExpDataClass _dataClass; - private final ExpSampleType _sampleType; - private final boolean _isSample; protected AliasDataIterator(DataIterator di, DataIteratorContext context, Container container, User user, TableInfo expTable, ExpObject dataType, boolean isSample) { - super(di); - _context = context; + super(di, context, container, user, dataType, isSample); Map map = DataIteratorUtil.createColumnNameMap(di); _aliasCol = map.get(ALIASCOLUMNALIAS) == null ? null : di.getSupplier(map.get(ALIASCOLUMNALIAS)); _lsidCol = map.get("lsid") == null ? null : di.getSupplier(map.get("lsid")); _nameCol = map.get("name") == null ? null : di.getSupplier(map.get("name")); - - _container = container; - _user = user; _expAliasTable = expTable; - _isSample = isSample; - - if (isSample) - { - _sampleType = (ExpSampleType) dataType; - _dataClass = null; - } - else - { - _sampleType = null; - _dataClass = (ExpDataClass) dataType; - } - } - - private BatchValidationException getErrors() - { - return _context.getErrors(); } @Override @@ -569,59 +542,56 @@ public boolean next() throws BatchValidationException if (getErrors().hasErrors()) return hasNext; - // after the last row, insert all aliases - if (!hasNext) + if (hasNext) { - final ExperimentService svc = ExperimentService.get(); - - try (DbScope.Transaction transaction = svc.getTinfoDataClass().getSchema().getScope().ensureTransaction()) + // For each iteration, collect the lsid and alias col values. + if (_aliasCol != null) { - for (Map.Entry entry : _lsidAliasMap.entrySet()) + if (_context.getInsertOption().mergeRows && _nameCol != null) { - String lsid = entry.getKey(); - Object aliases = entry.getValue(); - AliasInsertHelper.handleInsertUpdate(_container, _user, lsid, _expAliasTable, aliases); + Object nameValue = _nameCol.get(); + if (nameValue instanceof String name) + { + ExpObject obj = getExpObjectByName(name); + if (obj != null) + { + String lsid = obj.getLSID(); + if (lsid != null && !lsid.isEmpty()) + _lsidAliasMap.put(lsid, _aliasCol.get()); + } + } } - transaction.commit(); - } - - return false; - } - - // For each iteration, collect the lsid and alias col values. - if (_aliasCol != null) - { - if (_context.getInsertOption() == QueryUpdateService.InsertOption.MERGE && _nameCol != null) - { - Object nameValue = _nameCol.get(); - if (nameValue instanceof String name) + else if (_lsidCol != null) { - ExpObject obj; - if (_isSample) - obj = _sampleType.getSample(_container, name); - else - obj = _dataClass.getData(_container, name); + Object lsidValue = _lsidCol.get(); + Object aliasValue = _aliasCol.get(); - if (obj != null) + if (aliasValue != null && lsidValue instanceof String lsidString) { - String lsid = obj.getLSID(); - if (lsid != null && !lsid.isEmpty()) - _lsidAliasMap.put(lsid, _aliasCol.get()); + _lsidAliasMap.put(lsidString, aliasValue); } } } - else if (_lsidCol != null) - { - Object lsidValue = _lsidCol.get(); - Object aliasValue = _aliasCol.get(); - if (aliasValue != null && lsidValue instanceof String lsidString) - { - _lsidAliasMap.put(lsidString, aliasValue); - } + return true; + } + + if (_lsidAliasMap.isEmpty()) + return false; + + // after the last row, insert all aliases + try (DbScope.Transaction transaction = ExperimentService.get().ensureTransaction()) + { + for (Map.Entry entry : _lsidAliasMap.entrySet()) + { + String lsid = entry.getKey(); + Object aliases = entry.getValue(); + AliasInsertHelper.handleInsertUpdate(_container, _user, lsid, _expAliasTable, aliases); } + transaction.commit(); } - return true; + + return false; } } @@ -770,45 +740,22 @@ public DataIterator getDataIterator(DataIteratorContext context) } } - private static class FlagDataIterator extends WrapperDataIterator + private static class FlagDataIterator extends ExpObjectDataIterator { final DataIteratorContext _context; - final User _user; final Integer _lsidCol; final Integer _nameCol; final Integer _flagCol; - final boolean _isSample; // as opposed to DataClass - private final ExpSampleType _sampleType; - private final ExpDataClass _dataClass; - final Container _container; - protected FlagDataIterator(DataIterator di, DataIteratorContext context, User user, boolean isSample, ExpObject expObject, Container container) + protected FlagDataIterator(DataIterator di, DataIteratorContext context, User user, boolean isSample, ExpObject dataType, Container container) { - super(di); + super(di, context, container, user, dataType, isSample); _context = context; - _user = user; - _isSample = isSample; Map map = DataIteratorUtil.createColumnNameMap(di); _lsidCol = map.get("lsid"); _nameCol = map.get("name"); _flagCol = map.containsKey("flag") ? map.get("flag") : map.get("comment"); - if (isSample) - { - _sampleType = (ExpSampleType) expObject; - _dataClass = null; - } - else - { - _sampleType = null; - _dataClass = (ExpDataClass) expObject; - } - _container = container; - } - - private BatchValidationException getErrors() - { - return _context.getErrors(); } @Override @@ -825,43 +772,27 @@ public boolean next() throws BatchValidationException if (_lsidCol != null && _flagCol != null) { Object lsidValue = get(_lsidCol); - Object flagValue = get(_flagCol); if (lsidValue instanceof String lsid) { - String flag = Objects.toString(flagValue, null); - try { - if (_isSample) + ExpObject expObject = null; + if (_context.getInsertOption().mergeRows && _nameCol != null) { - ExpMaterial sample = null; - if (_context.getInsertOption() == QueryUpdateService.InsertOption.MERGE && _nameCol != null) - { - Object nameValue = get(_nameCol); - if (nameValue instanceof String) - sample = _sampleType.getSample(_container, (String) nameValue); - } - - if (sample == null) - sample = ExperimentService.get().getExpMaterial(lsid); - if (sample != null) - sample.setComment(_user, flag, false); + Object nameValue = get(_nameCol); + if (nameValue instanceof String name) + expObject = getExpObjectByName(name); } - else + + if (expObject == null) + expObject = getExpObjectByLsid(lsid); + + if (expObject != null) { - ExpData data = null; - if (_context.getInsertOption() == QueryUpdateService.InsertOption.MERGE && _nameCol != null) - { - Object nameValue = get(_nameCol); - if (nameValue instanceof String) - data = _dataClass.getData(_container, (String) nameValue); - } - if (data == null) - data = ExperimentService.get().getExpData(lsid); - if (data != null) - data.setComment(_user, flag, false); + String flag = Objects.toString(flagValue, null); + expObject.setComment(_user, flag, false); } } catch (ValidationException e) @@ -928,9 +859,9 @@ public DataIterator getDataIterator(DataIteratorContext context) static boolean hasAliquots(int sampleTypeRowId, List names) { - SimpleFilter f = new SimpleFilter(FieldKey.fromParts("Name"), names, IN); - f.addCondition(FieldKey.fromParts("MaterialSourceId"), sampleTypeRowId); - f.addCondition(FieldKey.fromParts(AliquotedFromLSID.name()), null, CompareType.NONBLANK); + SimpleFilter f = new SimpleFilter(Name.fieldKey(), names, IN); + f.addCondition(MaterialSourceId.fieldKey(), sampleTypeRowId); + f.addCondition(AliquotedFromLSID.fieldKey(), null, CompareType.NONBLANK); return new TableSelector(ExperimentService.get().getTinfoMaterial(), f, null).exists(); } @@ -963,9 +894,8 @@ static Collection getParentNames(Object parentObj, TSVWriter tsvWriter, return values == null ? null : values.collect(Collectors.toList()); } - static class DerivationDataIteratorBase extends WrapperDataIterator + static class DerivationDataIteratorBase extends ExpObjectDataIterator { - final DataIteratorContext _context; final Integer _lsidCol; final Integer _nameCol; final Map _parentCols; @@ -974,46 +904,25 @@ static class DerivationDataIteratorBase extends WrapperDataIterator /** Cache sample type lookups because even though we do caching in SampleTypeService, it's still a lot of overhead to check permissions for the user */ final Map _sampleTypes = new HashMap<>(); final Map _dataClasses = new HashMap<>(); - - final ExpSampleType _currentSampleType; - final ExpDataClass _currentDataClass; - - final Container _container; - final User _user; - final boolean _isSample; final TSVWriter _tsvWriter; protected DerivationDataIteratorBase(DataIterator di, DataIteratorContext context, Container container, User user, ExpObject currentDataType, boolean isSample, boolean checkRequiredParent) { - super(di); - _context = context; - _isSample = isSample; + super(di, context, container, user, currentDataType, isSample); Set requiredParents = new CaseInsensitiveHashSet(); - if (isSample) + + try { - _currentSampleType = (ExpSampleType) currentDataType; - try - { - if (checkRequiredParent) - requiredParents.addAll(_currentSampleType.getRequiredImportAliases().values()); - } - catch (IOException ignore) + if (checkRequiredParent) { + if (isSample()) + requiredParents.addAll(getSampleType().getRequiredImportAliases().values()); + else + requiredParents.addAll(getDataClass().getRequiredImportAliases().values()); } - _currentDataClass = null; } - else + catch (IOException ignore) { - _currentSampleType = null; - _currentDataClass = (ExpDataClass) currentDataType; - try - { - if (checkRequiredParent) - requiredParents.addAll(_currentDataClass.getRequiredImportAliases().values()); - } - catch (IOException ignore) - { - } } Map map = DataIteratorUtil.createColumnNameMap(di); @@ -1022,13 +931,11 @@ protected DerivationDataIteratorBase(DataIterator di, DataIteratorContext contex _parentCols = new HashMap<>(); _requiredParentCols = new HashMap<>(); _aliquotParents = new LinkedHashMap<>(); - _container = container; - _user = user; for (Map.Entry entry : map.entrySet()) { String name = entry.getKey(); - if (ExperimentService.isInputOutputColumn(name) || _isSample && equalsIgnoreCase("parent",name)) + if (ExperimentService.isInputOutputColumn(name) || isSample() && equalsIgnoreCase("parent", name)) { _parentCols.put(entry.getValue(), entry.getKey()); if (requiredParents.contains(name)) @@ -1046,11 +953,6 @@ protected int write() }; } - private BatchValidationException getErrors() - { - return _context.getErrors(); - } - protected Set> _getParentParts() { Set> allParts = new HashSet<>(); @@ -1077,6 +979,7 @@ protected Set> _getParentParts() allParts.add(new Pair<>(_parentCols.get(parentCol), null)); } } + return allParts; } @@ -1111,7 +1014,7 @@ protected void _processRun(ExpRunItem runItem, if (pair.first == null && pair.second == null) // no parents or children columns provided in input data and no existing parents to be updated return; - if (_isSample && aliquotedFrom == null && !((ExpMaterial) runItem).isOperationPermitted(SampleTypeService.SampleOperations.EditLineage)) + if (isSample() && aliquotedFrom == null && !((ExpMaterial) runItem).isOperationPermitted(SampleTypeService.SampleOperations.EditLineage)) throw new ValidationException(String.format("Sample %s with status %s cannot have its lineage updated.", runItem.getName(), ((ExpMaterial) runItem).getStateLabel())); // the parent columns provided in the input are all empty and there are no existing parents not mentioned in the input that need to be retained. @@ -1137,7 +1040,7 @@ protected void _processRun(ExpRunItem runItem, previousSampleRelatives = clearRunItemSourceRun(_user, runItem, false); } - if (_isSample) + if (isSample()) { ExpMaterial sample = (ExpMaterial) runItem; currentMaterialMap = new HashMap<>(); @@ -1212,7 +1115,7 @@ protected DerivationDataIterator(DataIterator di, DataIteratorContext context, C for (Map.Entry entry : map.entrySet()) { String name = entry.getKey(); - if (_isSample && ExpMaterial.ALIQUOTED_FROM_INPUT.equalsIgnoreCase(name)) + if (isSample() && ExpMaterial.ALIQUOTED_FROM_INPUT.equalsIgnoreCase(name)) { aliquotParentCol = entry.getValue(); } @@ -1221,11 +1124,6 @@ protected DerivationDataIterator(DataIterator di, DataIteratorContext context, C _aliquotParentCol = aliquotParentCol; } - private BatchValidationException getErrors() - { - return _context.getErrors(); - } - @Override public boolean next() throws BatchValidationException { @@ -1279,11 +1177,11 @@ else if (!_skipAliquot && _context.getInsertOption().mergeRows) Map materialCache = new HashMap<>(); Map dataCache = new HashMap<>(); - if (_isSample && _context.getInsertOption().mergeRows) + if (isSample() && _context.getInsertOption().mergeRows) { if (!_candidateAliquotNames.isEmpty()) { - if (hasAliquots(_currentSampleType.getRowId(), _candidateAliquotNames)) + if (hasAliquots(getSampleType().getRowId(), _candidateAliquotNames)) { // AliquotedFrom is used to determine if aliquot/meta field value should be retained or discarded // In the case of merge, one can argue AliquotedFrom can be queried for existing data, instead of making it a required field. @@ -1302,21 +1200,17 @@ else if (!_skipAliquot && _context.getInsertOption().mergeRows) lsids.addAll(_aliquotParents.keySet()); for (String lsid : lsids) { - Set> parentNames = _parentNames.getOrDefault(lsid, Collections.emptySet()); - ExpRunItem runItem; String aliquotedFrom = _aliquotParents.get(lsid); String dataType = null; - if (_isSample) + if (isSample()) { ExpMaterial m = null; if (_context.getInsertOption().mergeRows) // column lsid generated from dbseq might not be valid for existing materials, lookup by name instead { String sampleName = _lsidNames.get(lsid); if (!StringUtils.isEmpty(sampleName)) - { - m = _currentSampleType.getSample(_container, sampleName); - } + m = getSampleType().getSample(_container, sampleName); } if (m == null) @@ -1336,23 +1230,22 @@ else if (!_skipAliquot && _context.getInsertOption().mergeRows) { String dataName = _lsidNames.get(lsid); if (!StringUtils.isEmpty(dataName)) - { - d = _currentDataClass.getData(_container, dataName); - } + d = getDataClass().getData(_container, dataName); } if (d == null) d = ExperimentService.get().getExpData(lsid); if (d != null) - { dataCache.put(d.getRowId(), d); - } + runItem = d; } + if (runItem == null) // nothing to do if the item does not exist continue; + Set> parentNames = _parentNames.getOrDefault(lsid, Collections.emptySet()); _processRun(runItem, runRecords, parentNames, cache, materialCache, dataCache, aliquotedFrom, dataType, false); } @@ -1372,8 +1265,6 @@ else if (!_skipAliquot && _context.getInsertOption().mergeRows) } return hasNext; } - - } static class ImportWithUpdateDerivationDataIterator extends DerivationDataIteratorBase @@ -1383,7 +1274,6 @@ static class ImportWithUpdateDerivationDataIterator extends DerivationDataIterat final Integer _aliquotParentCol; // Map of Data name and its aliquotedFromLSID final Map _aliquotParents; - final boolean _useLsid; protected ImportWithUpdateDerivationDataIterator(DataIterator di, DataIteratorContext context, Container container, User user, ExpObject currentDataType, boolean isSample, boolean checkRequiredParent) @@ -1397,22 +1287,16 @@ protected ImportWithUpdateDerivationDataIterator(DataIterator di, DataIteratorCo Integer aliquotParentCol = -1; for (Map.Entry entry : map.entrySet()) { - if (_isSample && "AliquotedFromLSID".equalsIgnoreCase(entry.getKey())) + if (isSample() && "AliquotedFromLSID".equalsIgnoreCase(entry.getKey())) { aliquotParentCol = entry.getValue(); } } _aliquotParentCol = aliquotParentCol; - _useLsid = map.containsKey("lsid") && context.getConfigParameterBoolean(ExperimentService.QueryOptions.UseLsidForUpdate); } - private BatchValidationException getErrors() - { - return _context.getErrors(); - } - @Override public boolean next() throws BatchValidationException { @@ -1484,7 +1368,6 @@ else if (o instanceof Number) Map materialCache = new HashMap<>(); Map dataCache = new HashMap<>(); - List runRecords = new ArrayList<>(); Set keys = new LinkedHashSet<>(); keys.addAll(_parentNames.keySet()); @@ -1498,25 +1381,25 @@ else if (o instanceof Number) ExpRunItem runItem; String aliquotedFromLSID = _aliquotParents.get(key); String dataType = null; - if (_isSample) + if (isSample()) { - ExpMaterial m = _useLsid ? experimentService.getExpMaterial(key) : _currentSampleType.getSample(_container, key); + ExpMaterial m = _useLsid ? experimentService.getExpMaterial(key) : getSampleType().getSample(_container, key); if (m != null) { materialCache.put(m.getRowId(), m); - dataType = _currentSampleType.getName(); + dataType = getSampleType().getName(); } runItem = m; } else { - ExpData d = _useLsid ? experimentService.getExpData(key) : _currentDataClass.getData(_container, key); + ExpData d = _useLsid ? experimentService.getExpData(key) : getDataClass().getData(_container, key); if (d != null) { dataCache.put(d.getRowId(), d); - dataType = _currentDataClass.getName(); + dataType = getDataClass().getName(); } runItem = d; } @@ -1543,7 +1426,6 @@ else if (o instanceof Number) } return hasNext; } - } private static String checkForLockedSampleRelativeChange(Set previousSampleRelatives, Set currentSampleRelatives, String sampleName, String relationPlural) @@ -3179,7 +3061,6 @@ public MultiDataTypeCrossProjectDataIteratorBuilder(@NotNull User user, @NotNull _isCrossFolder = isCrossFolder; _dataType = dataType; _isSamples = isSamples; - } @Override @@ -3325,5 +3206,4 @@ public boolean next() throws BatchValidationException return true; } } - } diff --git a/experiment/src/org/labkey/experiment/ExpObjectDataIterator.java b/experiment/src/org/labkey/experiment/ExpObjectDataIterator.java new file mode 100644 index 00000000000..b1849865e3a --- /dev/null +++ b/experiment/src/org/labkey/experiment/ExpObjectDataIterator.java @@ -0,0 +1,92 @@ +package org.labkey.experiment; + +import org.jetbrains.annotations.NotNull; +import org.labkey.api.data.Container; +import org.labkey.api.dataiterator.DataIterator; +import org.labkey.api.dataiterator.DataIteratorContext; +import org.labkey.api.dataiterator.WrapperDataIterator; +import org.labkey.api.exp.api.ExpDataClass; +import org.labkey.api.exp.api.ExpObject; +import org.labkey.api.exp.api.ExpSampleType; +import org.labkey.api.exp.api.ExperimentService; +import org.labkey.api.query.BatchValidationException; +import org.labkey.api.security.User; + +/** + * Common base class for iterators that operate on ExpObjects and need to track + * sample/data class information. + */ +public class ExpObjectDataIterator extends WrapperDataIterator +{ + protected final DataIteratorContext _context; + protected final Container _container; + protected final User _user; + + private final boolean _isSample; + private final ExpDataClass _dataClass; + private final ExpSampleType _sampleType; + + protected ExpObjectDataIterator(DataIterator di, DataIteratorContext context, Container container, User user, ExpObject dataType, boolean isSample) + { + super(di); + _context = context; + _container = container; + _user = user; + _isSample = isSample; + + if (isSample) + { + _sampleType = (ExpSampleType) dataType; + _dataClass = null; + } + else + { + _sampleType = null; + _dataClass = (ExpDataClass) dataType; + } + } + + protected BatchValidationException getErrors() + { + return _context.getErrors(); + } + + protected boolean isSample() + { + return _isSample; + } + + protected ExpObject getExpObjectByLsid(String lsid) + { + ExpObject obj; + if (isSample()) + obj = ExperimentService.get().getExpMaterial(lsid); + else + obj = ExperimentService.get().getExpData(lsid); + return obj; + } + + protected ExpObject getExpObjectByName(String name) + { + ExpObject obj; + if (isSample()) + obj = getSampleType().getSample(_container, name); + else + obj = getDataClass().getData(_container, name); + return obj; + } + + protected @NotNull ExpSampleType getSampleType() + { + if (_sampleType == null) + throw new IllegalStateException("Requested a sample type when the iterator is not for samples. Check isSample() first."); + return _sampleType; + } + + protected @NotNull ExpDataClass getDataClass() + { + if (_dataClass == null) + throw new IllegalStateException("Requested a data class when the iterator is not for data. Check isSample() first."); + return _dataClass; + } +} From 940aff6f854da45bd35c7b543d42dc6b30ae60c9 Mon Sep 17 00:00:00 2001 From: labkey-nicka Date: Fri, 23 May 2025 11:05:48 -0700 Subject: [PATCH 03/22] Rename --- experiment/src/org/labkey/experiment/ExpDataIterators.java | 6 +++--- ...bjectDataIterator.java => ExpDataTypeDataIterator.java} | 7 +++---- 2 files changed, 6 insertions(+), 7 deletions(-) rename experiment/src/org/labkey/experiment/{ExpObjectDataIterator.java => ExpDataTypeDataIterator.java} (88%) diff --git a/experiment/src/org/labkey/experiment/ExpDataIterators.java b/experiment/src/org/labkey/experiment/ExpDataIterators.java index 02deeeac252..3be15114c6c 100644 --- a/experiment/src/org/labkey/experiment/ExpDataIterators.java +++ b/experiment/src/org/labkey/experiment/ExpDataIterators.java @@ -513,7 +513,7 @@ public DataIterator getDataIterator(DataIteratorContext context) } } - private static class AliasDataIterator extends ExpObjectDataIterator + private static class AliasDataIterator extends ExpDataTypeDataIterator { // For some reason I don't quite understand we don't want to pass through a column called "alias" so we rename it to ALIASCOLUMNALIAS final Supplier _aliasCol; @@ -740,7 +740,7 @@ public DataIterator getDataIterator(DataIteratorContext context) } } - private static class FlagDataIterator extends ExpObjectDataIterator + private static class FlagDataIterator extends ExpDataTypeDataIterator { final DataIteratorContext _context; final Integer _lsidCol; @@ -894,7 +894,7 @@ static Collection getParentNames(Object parentObj, TSVWriter tsvWriter, return values == null ? null : values.collect(Collectors.toList()); } - static class DerivationDataIteratorBase extends ExpObjectDataIterator + static class DerivationDataIteratorBase extends ExpDataTypeDataIterator { final Integer _lsidCol; final Integer _nameCol; diff --git a/experiment/src/org/labkey/experiment/ExpObjectDataIterator.java b/experiment/src/org/labkey/experiment/ExpDataTypeDataIterator.java similarity index 88% rename from experiment/src/org/labkey/experiment/ExpObjectDataIterator.java rename to experiment/src/org/labkey/experiment/ExpDataTypeDataIterator.java index b1849865e3a..6d55c52a188 100644 --- a/experiment/src/org/labkey/experiment/ExpObjectDataIterator.java +++ b/experiment/src/org/labkey/experiment/ExpDataTypeDataIterator.java @@ -13,10 +13,9 @@ import org.labkey.api.security.User; /** - * Common base class for iterators that operate on ExpObjects and need to track - * sample/data class information. + * WrapperDataIterator that operate on ExpObjects and need to track sample/data class information. */ -public class ExpObjectDataIterator extends WrapperDataIterator +public class ExpDataTypeDataIterator extends WrapperDataIterator { protected final DataIteratorContext _context; protected final Container _container; @@ -26,7 +25,7 @@ public class ExpObjectDataIterator extends WrapperDataIterator private final ExpDataClass _dataClass; private final ExpSampleType _sampleType; - protected ExpObjectDataIterator(DataIterator di, DataIteratorContext context, Container container, User user, ExpObject dataType, boolean isSample) + protected ExpDataTypeDataIterator(DataIterator di, DataIteratorContext context, Container container, User user, ExpObject dataType, boolean isSample) { super(di); _context = context; From 60cfef0553f1db9548097af8157115242f2d7924 Mon Sep 17 00:00:00 2001 From: labkey-nicka Date: Fri, 23 May 2025 11:12:27 -0700 Subject: [PATCH 04/22] More consolidation --- .../labkey/api/data/NameGeneratorState.java | 13 +++--- .../NameExpressionDataIterator.java | 3 +- .../labkey/experiment/ExpDataIterators.java | 19 ++++----- .../api/SampleTypeUpdateServiceDI.java | 40 +++++++++---------- 4 files changed, 36 insertions(+), 39 deletions(-) diff --git a/api/src/org/labkey/api/data/NameGeneratorState.java b/api/src/org/labkey/api/data/NameGeneratorState.java index 2b10c77b068..6c025c1270d 100644 --- a/api/src/org/labkey/api/data/NameGeneratorState.java +++ b/api/src/org/labkey/api/data/NameGeneratorState.java @@ -292,8 +292,8 @@ private String genName(@NotNull Map rowMap, StringExpressionFactory.FieldKeyStringExpression expression = activeNameGenerator.getParsedNameExpression(); String name; - if (expression instanceof NameGenerator.NameGenerationExpression) - name = ((NameGenerator.NameGenerationExpression) expression).eval(ctx, _prefixCounterSequences); + if (expression instanceof NameGenerator.NameGenerationExpression nge) + name = nge.eval(ctx, _prefixCounterSequences); else name = expression.eval(ctx); if (name == null || name.isEmpty()) @@ -417,14 +417,14 @@ private void addAncestorLookupValues( String parentType = ancestorOptions.parentType(); if (!StringUtils.isEmpty(parentType)) { - if (parentObject instanceof ExpMaterial) + if (parentObject instanceof ExpMaterial expMaterial) { - if (!((ExpMaterial) parentObject).getSampleType().getName().equalsIgnoreCase(parentType)) + if (!expMaterial.getSampleType().getName().equalsIgnoreCase(parentType)) continue; } - else if (parentObject instanceof ExpData) + else if (parentObject instanceof ExpData expData) { - if (!((ExpData) parentObject).getDataClass(_user).getName().equalsIgnoreCase(parentType)) + if (!expData.getDataClass(_user).getName().equalsIgnoreCase(parentType)) continue; } } @@ -490,7 +490,6 @@ else if (parentObject instanceof ExpData) } } } - } private void addParentLookupContext(String parentTypeName/* already decoded */, diff --git a/api/src/org/labkey/api/dataiterator/NameExpressionDataIterator.java b/api/src/org/labkey/api/dataiterator/NameExpressionDataIterator.java index 0ead39c9aac..9eddbcbe92a 100644 --- a/api/src/org/labkey/api/dataiterator/NameExpressionDataIterator.java +++ b/api/src/org/labkey/api/dataiterator/NameExpressionDataIterator.java @@ -63,7 +63,6 @@ public NameExpressionDataIterator(DataIterator di, DataIteratorContext context, _container = container; _getNonConflictCountFn = getNonConflictCountFn; _counterSeqPrefix = counterSeqPrefix; - } public NameExpressionDataIterator setAllowUserSpecifiedNames(boolean allowUserSpecifiedNames) @@ -80,7 +79,7 @@ public NameExpressionDataIterator addExtraPropsFn(Supplier> MapDataIterator getInput() { - return (MapDataIterator)_delegate; + return (MapDataIterator) _delegate; } private BatchValidationException getErrors() diff --git a/experiment/src/org/labkey/experiment/ExpDataIterators.java b/experiment/src/org/labkey/experiment/ExpDataIterators.java index 3be15114c6c..7063bda6197 100644 --- a/experiment/src/org/labkey/experiment/ExpDataIterators.java +++ b/experiment/src/org/labkey/experiment/ExpDataIterators.java @@ -280,8 +280,8 @@ public ExpMaterialValidatorIterator(DataIterator data, DataIteratorContext conte boolean isUpdateOnly = context.getInsertOption().updateOnly; Map columnNameMap = DataIteratorUtil.createColumnNameMap(data); - if (!isUpdateOnly && columnNameMap.containsKey(ExpMaterial.ALIQUOTED_FROM_INPUT)) - _aliquotedFromColIdx = columnNameMap.get(ExpMaterial.ALIQUOTED_FROM_INPUT); + if (!isUpdateOnly && columnNameMap.containsKey(ALIQUOTED_FROM_INPUT)) + _aliquotedFromColIdx = columnNameMap.get(ALIQUOTED_FROM_INPUT); else if (isUpdateOnly && columnNameMap.containsKey(AliquotedFromLSID.name())) _aliquotedFromColIdx = columnNameMap.get(AliquotedFromLSID.name()); else @@ -378,7 +378,7 @@ protected AliquotRollupDataIterator(DataIterator di, DataIteratorContext context _storedAmountCol = map.get(StoredAmount.name()); _unitsCol = map.get(Units.name()); _sampleStateCol = map.get(SampleState.name()); - _aliquotedFromCol = map.get(ExpMaterial.ALIQUOTED_FROM_INPUT); + _aliquotedFromCol = map.get(ALIQUOTED_FROM_INPUT); _rootMaterialRowIdCol = map.get(RootMaterialRowId.name()); _rootIdToRecomputeCol = map.get(ROOT_RECOMPUTE_ROWID_COL); _parentNameToRecomputeCol = map.get(PARENT_RECOMPUTE_NAME_COL); @@ -648,7 +648,7 @@ protected AutoLinkToStudyDataIterator(DataIterator di, UserSchema schema, Contai for (String name : nameMap.keySet()) { - if (ExperimentService.isInputOutputColumn(name) || equalsIgnoreCase("parent", name) || equalsIgnoreCase("AliquotedFrom", name)) + if (ExperimentService.isInputOutputColumn(name) || equalsIgnoreCase("parent", name) || equalsIgnoreCase(ALIQUOTED_FROM_INPUT, name)) { _parentCols.add(nameMap.get(name)); } @@ -862,12 +862,13 @@ static boolean hasAliquots(int sampleTypeRowId, List names) SimpleFilter f = new SimpleFilter(Name.fieldKey(), names, IN); f.addCondition(MaterialSourceId.fieldKey(), sampleTypeRowId); f.addCondition(AliquotedFromLSID.fieldKey(), null, CompareType.NONBLANK); - return new TableSelector(ExperimentService.get().getTinfoMaterial(), f, null).exists(); + + return new TableSelector(ExperimentService.get().getTinfoMaterial(), Set.of(RowId.name()), f, null).exists(); } public static String getAliquotParent(Object parentObj, DataIteratorContext context, TSVWriter tsvWriter) { - Collection parentNames = getParentNames(parentObj, tsvWriter, "AliquotedFrom", null); + Collection parentNames = getParentNames(parentObj, tsvWriter, ALIQUOTED_FROM_INPUT, null); if (parentNames != null) { List parents = parentNames.stream() @@ -877,7 +878,7 @@ public static String getAliquotParent(Object parentObj, DataIteratorContext cont if (!parents.isEmpty()) { if (parents.size() > 1) - context.getErrors().addRowError(new ValidationException("Multiple AliquotedFrom values are provided.")); + context.getErrors().addRowError(new ValidationException(String.format("Multiple %s values are provided.", ALIQUOTED_FROM_INPUT))); return parents.get(0); } } @@ -1115,7 +1116,7 @@ protected DerivationDataIterator(DataIterator di, DataIteratorContext context, C for (Map.Entry entry : map.entrySet()) { String name = entry.getKey(); - if (isSample() && ExpMaterial.ALIQUOTED_FROM_INPUT.equalsIgnoreCase(name)) + if (isSample() && ALIQUOTED_FROM_INPUT.equalsIgnoreCase(name)) { aliquotParentCol = entry.getValue(); } @@ -1145,7 +1146,7 @@ public boolean next() throws BatchValidationException { Object o = get(_aliquotParentCol); - String aliquotParentName = getAliquotParent(o, _context, _tsvWriter); + String aliquotParentName = getAliquotParent(o, _context, _tsvWriter); if (aliquotParentName != null) _aliquotParents.put(lsid, aliquotParentName.trim()); diff --git a/experiment/src/org/labkey/experiment/api/SampleTypeUpdateServiceDI.java b/experiment/src/org/labkey/experiment/api/SampleTypeUpdateServiceDI.java index b8ff3fd8425..3e46eb9e2a9 100644 --- a/experiment/src/org/labkey/experiment/api/SampleTypeUpdateServiceDI.java +++ b/experiment/src/org/labkey/experiment/api/SampleTypeUpdateServiceDI.java @@ -338,9 +338,9 @@ public boolean next() throws BatchValidationException Object nameObj = (_delegate).get(parentNameToRecomputeCol); if (nameObj != null) { - if (nameObj instanceof String) + if (nameObj instanceof String name) { - nameToRecompute.add((String) nameObj); + nameToRecompute.add(name); } else if (nameObj instanceof Number) { @@ -807,7 +807,7 @@ else if (!isAliquot && isAliquotField) validRowCopy.put(updateField, updateValue); } - if (ExpMaterialTable.Column.SampleState.name().toLowerCase().equals(updateField.toLowerCase())) + if (ExpMaterialTable.Column.SampleState.name().equalsIgnoreCase(updateField)) hasStatusCol = true; } // had a locked status before and either not updating the status or updating to a new locked status @@ -989,9 +989,9 @@ private Map getMaterialMap(Integer rowId, String lsid, User user { Filter filter; if (rowId != null) - filter = new SimpleFilter(FieldKey.fromParts(ExpMaterialTable.Column.RowId), rowId); + filter = new SimpleFilter(ExpMaterialTable.Column.RowId.fieldKey(), rowId); else if (lsid != null) - filter = new SimpleFilter(FieldKey.fromParts(LSID), lsid); + filter = new SimpleFilter(LSID.fieldKey(), lsid); else throw new QueryUpdateServiceException("Either RowId or LSID is required to get Sample Type Material."); @@ -1072,8 +1072,8 @@ public boolean hasExistingRowsInOtherContainers(Container container, Map[] rows = new TableSelector(queryTableInfo, selectColumns, filter, null).getMapArray(); for (Map row : rows) @@ -1222,8 +1221,8 @@ else if (name != null && materialSourceId != null) if (!nameRowNumMap.isEmpty()) { allKeys.addAll(nameRowNumMap.keySet()); - SimpleFilter filter = new SimpleFilter(FieldKey.fromParts("MaterialSourceId"), sampleTypeId); - filter.addCondition(FieldKey.fromParts("Name"), nameRowNumMap.keySet(), CompareType.IN); + SimpleFilter filter = new SimpleFilter(ExpMaterialTable.Column.MaterialSourceId.fieldKey(), sampleTypeId); + filter.addCondition(ExpMaterialTable.Column.Name.fieldKey(), nameRowNumMap.keySet(), CompareType.IN); filter.addCondition(FieldKey.fromParts("Container"), container); Map[] rows = new TableSelector(queryTableInfo, selectColumns, filter, null).getMapArray(); for (Map row : rows) @@ -1243,7 +1242,7 @@ else if (name != null && materialSourceId != null) // Issue 52922: cross folder merge without Product Folders enabled silently ignores the cross folder row update ContainerFilter allCf = new ContainerFilter.AllInProjectPlusShared(container, user); // use a relaxed CF to find existing data from cross containers - SimpleFilter existingDataFilter = new SimpleFilter(FieldKey.fromParts("MaterialSourceId"), sampleTypeId); + SimpleFilter existingDataFilter = new SimpleFilter(ExpMaterialTable.Column.MaterialSourceId.fieldKey(), sampleTypeId); existingDataFilter.addCondition(FieldKey.fromParts("Container"), allCf.getIds(), CompareType.IN); existingDataFilter.addCondition(FieldKey.fromParts(useLsid ? "LSID" : "Name"), allKeys, CompareType.IN); @@ -1254,7 +1253,6 @@ else if (name != null && materialSourceId != null) if (!dataContainer.equals(container.getId())) throw new InvalidKeyException("Sample does not belong to " + container.getName() + " container: " + row.get("name") + "."); } - } if (verifyExisting && !allKeys.isEmpty()) @@ -1795,7 +1793,7 @@ private boolean rowExists(String name) static class _SamplesCoerceDataIterator extends SimpleTranslator { private static final String INVALID_ALIQUOT_PROPERTY = "An aliquot-specific property [%1$s] value has been ignored for a non-aliquot sample."; - private static final String INVALID_NONALIQUOT_PROPERTY = "A sample property [%1$s] value has been ignored for an aliquot."; + private static final String INVALID_NON_ALIQUOT_PROPERTY = "A sample property [%1$s] value has been ignored for an aliquot."; private final ExpSampleTypeImpl _sampleType; private final Measurement.Unit _metricUnit; @@ -1832,9 +1830,9 @@ void init(TableInfo target, boolean useImportAliases, boolean initRollupCounts) { if (getAliquotedFromColName().equalsIgnoreCase(from.getName())) aliquotedFromDataColInd = i; - else if ("Units".equalsIgnoreCase(from.getName())) + else if (Units.name().equalsIgnoreCase(from.getName())) unitDataColInd = i; - else if ("StoredAmount".equalsIgnoreCase(from.getName()) || "Amount".equalsIgnoreCase(from.getName())) + else if (StoredAmount.name().equalsIgnoreCase(from.getName()) || "Amount".equalsIgnoreCase(from.getName())) amountDataColInd = i; } } @@ -1850,7 +1848,7 @@ else if ("StoredAmount".equalsIgnoreCase(from.getName()) || "Amount".equalsIgnor boolean isScopedField = scopedFields.containsKey(name); String ignoredAliquotPropValue = String.format(INVALID_ALIQUOT_PROPERTY, name); - String ignoredMetaPropValue = String.format(INVALID_NONALIQUOT_PROPERTY, name); + String ignoredMetaPropValue = String.format(INVALID_NON_ALIQUOT_PROPERTY, name); if (to.getPropertyType() == PropertyType.ATTACHMENT || to.getPropertyType() == PropertyType.FILE_LINK) { if (isScopedField) @@ -1873,11 +1871,11 @@ else if (to.getFk() instanceof MultiValuedForeignKey) else addColumn(to.getName(), i); } - else if (name.equalsIgnoreCase("Units")) + else if (Units.name().equalsIgnoreCase(name)) { addColumn(to, new SampleUnitsConvertColumn(name, i, to.getJdbcType())); } - else if (name.equalsIgnoreCase("StoredAmount")) + else if (StoredAmount.name().equalsIgnoreCase(name)) { addColumn(to, new SampleAmountConvertColumn(name, i, to.getJdbcType())); } @@ -1933,7 +1931,7 @@ private void _addConvertColumn(String name, int fromIndex, JdbcType toType, Fore private void _addConvertColumn(ColumnInfo col, int fromIndex, int derivationDataColInd, boolean isAliquotField) { SimpleConvertColumn c = createConvertColumn(col, fromIndex, RemapMissingBehavior.Error); - c = new DerivationScopedConvertColumn(fromIndex, c, derivationDataColInd, isAliquotField, String.format(INVALID_ALIQUOT_PROPERTY, col.getName()), String.format(INVALID_NONALIQUOT_PROPERTY, col.getName())); + c = new DerivationScopedConvertColumn(fromIndex, c, derivationDataColInd, isAliquotField, String.format(INVALID_ALIQUOT_PROPERTY, col.getName()), String.format(INVALID_NON_ALIQUOT_PROPERTY, col.getName())); addColumn(col, c); } @@ -1993,7 +1991,7 @@ else if (aliquotedFrom instanceof Number) { aliquotParentName = aliquotedFrom.toString(); } - if (aliquotParentName == null || StringUtils.isEmpty(aliquotParentName)) // if AliquotedFrom is empty, is root + if (StringUtils.isEmpty(aliquotParentName)) // if AliquotedFrom is empty, is root return 0; return null; // for aliquot, initialize rollup count/amount to null From dfa4396fb9277dc38c568bc0ad1022f852b696fa Mon Sep 17 00:00:00 2001 From: labkey-nicka Date: Mon, 2 Jun 2025 12:18:11 -0700 Subject: [PATCH 05/22] Issue 53153: support "RowId" as value for "AliquotedFrom" --- .../org/labkey/api/data/NameGenerator.java | 4 +- .../labkey/api/data/NameGeneratorState.java | 12 +- .../experiment/api/ExpSampleTypeImpl.java | 2 +- .../api/SampleTypeUpdateServiceDI.java | 180 ++++++++++-------- 4 files changed, 106 insertions(+), 92 deletions(-) diff --git a/api/src/org/labkey/api/data/NameGenerator.java b/api/src/org/labkey/api/data/NameGenerator.java index bf35e3250c6..c4c2b1aeb84 100644 --- a/api/src/org/labkey/api/data/NameGenerator.java +++ b/api/src/org/labkey/api/data/NameGenerator.java @@ -2099,13 +2099,13 @@ public static class NameGenerationException extends Exception { private final int _rowNumber; - NameGenerationException(String message, int rowNumber) + public NameGenerationException(String message, int rowNumber) { super(message); _rowNumber = rowNumber; } - NameGenerationException(int rowNumber, Throwable t) + public NameGenerationException(int rowNumber, Throwable t) { super(t); _rowNumber = rowNumber; diff --git a/api/src/org/labkey/api/data/NameGeneratorState.java b/api/src/org/labkey/api/data/NameGeneratorState.java index 6c025c1270d..5ee110e36d2 100644 --- a/api/src/org/labkey/api/data/NameGeneratorState.java +++ b/api/src/org/labkey/api/data/NameGeneratorState.java @@ -57,12 +57,12 @@ public class NameGeneratorState implements AutoCloseable { private final @NotNull NameGenerator _nameGenerator; private final boolean _incrementSampleCounts; - private final User _user; + protected final User _user; private final Map _batchExpressionContext; private Function,Map> getSampleCountsFunction; private final Map _newNames = new CaseInsensitiveHashMap<>(); - private int _rowNumber = 0; + protected int _rowNumber = 0; private final Map, Object> _lookupCache; private final Map> _ancestorCache; private final Map> _ancestorSearchCache; @@ -70,11 +70,11 @@ public class NameGeneratorState implements AutoCloseable private final NameGenerator.ProjectSampleCounters _sampleCounters; private boolean _counterSequencesCleaned = false; - private final Container _container; + protected final Container _container; - private final Map materialCache = new HashMap<>(); - private final Map dataCache = new HashMap<>(); - private final RemapCache renameCache; + protected final Map materialCache = new HashMap<>(); + protected final Map dataCache = new HashMap<>(); + protected final RemapCache renameCache; private final Map> objectPropertiesCache = new HashMap<>(); public NameGeneratorState(@NotNull NameGenerator nameGenerator, boolean incrementSampleCounts, NameGenerator.SampleNameExpressionSummary expressionSummary) diff --git a/experiment/src/org/labkey/experiment/api/ExpSampleTypeImpl.java b/experiment/src/org/labkey/experiment/api/ExpSampleTypeImpl.java index 289dcdea767..9829a81b78a 100644 --- a/experiment/src/org/labkey/experiment/api/ExpSampleTypeImpl.java +++ b/experiment/src/org/labkey/experiment/api/ExpSampleTypeImpl.java @@ -655,7 +655,7 @@ public SampleTypeUpdateServiceDI.SampleNameGeneratorState getNameGenState(boolea primaryGen.setExpressionSummary(new NameGenerator.ExpressionSummary(sampleNameExpressionSummary, expressionSummary.hasDateBasedSampleCounter(), expressionSummary.hasParentInputs(), expressionSummary.hasParentLookup(), expressionSummary.hasAncestorSearch())); } - return new SampleTypeUpdateServiceDI.SampleNameGeneratorState(primaryGen, incrementSampleCounts, aliquotNameGen); + return new SampleTypeUpdateServiceDI.SampleNameGeneratorState(this, primaryGen, incrementSampleCounts, aliquotNameGen); } private Container getGenIdSequenceContainer() diff --git a/experiment/src/org/labkey/experiment/api/SampleTypeUpdateServiceDI.java b/experiment/src/org/labkey/experiment/api/SampleTypeUpdateServiceDI.java index 3e46eb9e2a9..e340c96c716 100644 --- a/experiment/src/org/labkey/experiment/api/SampleTypeUpdateServiceDI.java +++ b/experiment/src/org/labkey/experiment/api/SampleTypeUpdateServiceDI.java @@ -44,6 +44,7 @@ import org.labkey.api.data.MultiValuedForeignKey; import org.labkey.api.data.NameGenerator; import org.labkey.api.data.NameGeneratorState; +import org.labkey.api.data.RemapCache; import org.labkey.api.data.RuntimeSQLException; import org.labkey.api.data.SimpleFilter; import org.labkey.api.data.Table; @@ -133,6 +134,7 @@ import static org.labkey.api.dataiterator.DetailedAuditLogDataIterator.AuditConfigs; import static org.labkey.api.dataiterator.SampleUpdateAddColumnsDataIterator.CURRENT_SAMPLE_STATUS_COLUMN_NAME; import static org.labkey.api.exp.api.ExpRunItem.PARENT_IMPORT_ALIAS_MAP_PROP; +import static org.labkey.api.exp.api.ExperimentService.QueryOptions.SkipBulkRemapCache; import static org.labkey.api.exp.api.SampleTypeDomainKind.ALIQUOT_ROLLUP_FIELD_LABELS; import static org.labkey.api.exp.api.SampleTypeService.ConfigParameters.SkipAliquotRollup; import static org.labkey.api.exp.api.SampleTypeService.ConfigParameters.SkipMaxSampleCounterFunction; @@ -1506,7 +1508,7 @@ public DataIterator getDataIterator(DataIteratorContext context) DataIterator c = LoggingDataIterator.wrap(new _SamplesCoerceDataIterator(source, context, sampleType, materialTable)); context.setWithLookupRemapping(false); SimpleTranslator addColumns = new SimpleTranslator(c, context); - addColumns.setDebugName("add genId and other requried columns"); + addColumns.setDebugName("add genId and other required columns"); Set idColNames = Sets.newCaseInsensitiveHashSet("genId"); materialTable.getColumns().stream().filter(ColumnInfo::isUniqueIdField).forEach(columnInfo -> idColNames.add(columnInfo.getName())); addColumns.selectAll(idColNames); @@ -1531,14 +1533,7 @@ public DataIterator getDataIterator(DataIteratorContext context) // sampleset.createSampleNames() + generate lsid // TODO: does not handle insertIgnore - DataIterator names = new _GenerateNamesDataIterator(sampleType, container, user, DataIteratorUtil.wrapMap(dataIterator, false), context, batchSize) - .setAllowUserSpecifiedNames(NameExpressionOptionService.get().getAllowUserSpecificNamesValue(container)) - .addExtraPropsFn(() -> { - if (container != null) - return Map.of(NameExpressionOptionService.FOLDER_PREFIX_TOKEN, StringUtils.trimToEmpty(NameExpressionOptionService.get().getExpressionPrefix(container))); - else - return Collections.emptyMap(); - }); + DataIterator names = new _GenerateNamesDataIterator(sampleType, container, user, DataIteratorUtil.wrapMap(dataIterator, false), context, batchSize); return LoggingDataIterator.wrap(names); } @@ -1613,76 +1608,69 @@ private static boolean isAliquotRollupHeader(String name) static class _GenerateNamesDataIterator extends SimpleTranslator { - final ExpSampleTypeImpl sampleType; - final SampleNameGeneratorState nameState; + final boolean _allowUserSpecifiedNames; // whether manual names specification is allowed or only name expression generation + final int _batchSize; + final RemapCache _cache; + final Container _container; + final List>> _extraPropsFns; + final Map _materialCache; + final SampleNameGeneratorState _nameState; final Lsid.LsidBuilder lsidBuilder; final DbSequence _lsidDbSeq; - final Container _container; - final int _batchSize; - boolean first = true; - Map importAliasMap = null; - boolean _allowUserSpecifiedNames = true; // whether manual names specification is allowed or only name expression generation - Set _existingNames = null; - List>> _extraPropsFns = new ArrayList<>(); + final ExpSampleTypeImpl _sampleType; + final User _user; + Set _existingNames = null; String generatedName = null; - _GenerateNamesDataIterator(ExpSampleTypeImpl sampleType, Container dataContainer, User user, MapDataIterator source, DataIteratorContext context, int batchSize) + _GenerateNamesDataIterator(ExpSampleTypeImpl sampleType, Container container, User user, MapDataIterator source, DataIteratorContext context, int batchSize) { super(source, context); - this.sampleType = sampleType; + _allowUserSpecifiedNames = NameExpressionOptionService.get().getAllowUserSpecificNamesValue(container); + _cache = new RemapCache(!context.getConfigParameterBoolean(SkipBulkRemapCache)); + _container = container; + _extraPropsFns = new ArrayList<>(); + _materialCache = new HashMap<>(); + _sampleType = sampleType; + _user = user; + try { - this.importAliasMap = sampleType.getImportAliases(); - _extraPropsFns.add(() -> { - if (this.importAliasMap != null) - return Map.of(PARENT_IMPORT_ALIAS_MAP_PROP, this.importAliasMap); - else - return Collections.emptyMap(); - }); + Map importAliasMap = sampleType.getImportAliases(); + _extraPropsFns.add(() -> Map.of(PARENT_IMPORT_ALIAS_MAP_PROP, importAliasMap)); } catch (IOException e) { // do nothing } + + _extraPropsFns.add(() -> { + if (_container == null) + return Collections.emptyMap(); + return Map.of(NameExpressionOptionService.FOLDER_PREFIX_TOKEN, StringUtils.trimToEmpty(NameExpressionOptionService.get().getExpressionPrefix(_container))); + }); + boolean skipDuplicateCheck = context.getConfigParameterBoolean(SkipMaxSampleCounterFunction); - nameState = sampleType.getNameGenState(skipDuplicateCheck, true, dataContainer, user); + _nameState = sampleType.getNameGenState(skipDuplicateCheck, true, _container, user); lsidBuilder = sampleType.generateSampleLSID(); - _container = sampleType.getContainer(); _batchSize = batchSize; - if (context.getConfigParameterBoolean(ExperimentService.QueryOptions.UseLsidForUpdate)) + boolean useLsidForUpdate = context.getConfigParameterBoolean(ExperimentService.QueryOptions.UseLsidForUpdate); + if (useLsidForUpdate) selectAll(CaseInsensitiveHashSet.of(Name.name(), RootMaterialRowId.name())); else selectAll(CaseInsensitiveHashSet.of(Name.name(), LSID.name(), RootMaterialRowId.name())); - _lsidDbSeq = sampleType.getSampleLsidDbSeq(_batchSize, _container); + _lsidDbSeq = sampleType.getSampleLsidDbSeq(_batchSize, sampleType.getContainer()); - addColumn(new BaseColumnInfo("name", JdbcType.VARCHAR), (Supplier)() -> generatedName); - if (!context.getConfigParameterBoolean(ExperimentService.QueryOptions.UseLsidForUpdate)) - addColumn(new BaseColumnInfo("lsid", JdbcType.VARCHAR), (Supplier)() -> lsidBuilder.setObjectId(String.valueOf(_lsidDbSeq.next())).toString()); + addColumn(new BaseColumnInfo("name", JdbcType.VARCHAR), (Supplier)() -> generatedName); + if (!useLsidForUpdate) + addColumn(new BaseColumnInfo("lsid", JdbcType.VARCHAR), (Supplier)() -> lsidBuilder.setObjectId(String.valueOf(_lsidDbSeq.next())).toString()); // Ensure we have a cpasType column and it is of the right value addColumn(new BaseColumnInfo("cpasType", JdbcType.VARCHAR), new SimpleTranslator.ConstantColumn(sampleType.getLSID())); addColumn(new BaseColumnInfo("materialSourceId", JdbcType.INTEGER), new SimpleTranslator.ConstantColumn(sampleType.getRowId())); } - _GenerateNamesDataIterator setAllowUserSpecifiedNames(boolean allowUserSpecifiedNames) - { - _allowUserSpecifiedNames = allowUserSpecifiedNames; - return this; - } - - _GenerateNamesDataIterator addExtraPropsFn(Supplier> extraProps) - { - _extraPropsFns.add(extraProps); - return this; - } - - void onFirst() - { - first = false; - } - @Override protected void processNextInput() { @@ -1726,7 +1714,7 @@ else if (aliquotedFromObj instanceof Number) } } - generatedName = nameState.nextName(map, _extraPropsFns); + generatedName = _nameState.nextName(map, _extraPropsFns); } catch (NameGenerator.DuplicateNameException dup) @@ -1738,13 +1726,13 @@ else if (aliquotedFromObj instanceof Number) // Failed to generate a name due to some part of the expression not in the row if (isAliquot) { - addRowError("Failed to generate name for aliquot on row " + e.getRowNumber() + " using aliquot naming pattern " + sampleType.getAliquotNameExpression() + ". Check the syntax of the aliquot naming pattern and the data values for the aliquot."); + addRowError("Failed to generate name for aliquot on row " + e.getRowNumber() + " using aliquot naming pattern " + _sampleType.getAliquotNameExpression() + ". Check the syntax of the aliquot naming pattern and the data values for the aliquot."); } else { - if (sampleType.hasNameExpression()) - addRowError("Failed to generate name for sample on row " + e.getRowNumber() + " using naming pattern " + sampleType.getNameExpression() + ". Check the syntax of the naming pattern and the data values for the sample."); - else if (sampleType.hasNameAsIdCol()) + if (_sampleType.hasNameExpression()) + addRowError("Failed to generate name for sample on row " + e.getRowNumber() + " using naming pattern " + _sampleType.getNameExpression() + ". Check the syntax of the naming pattern and the data values for the sample."); + else if (_sampleType.hasNameAsIdCol()) addRowError("SampleID or Name is required for sample on row " + e.getRowNumber()); else addRowError("All id columns are required for sample on row " + e.getRowNumber()); @@ -1755,16 +1743,12 @@ else if (sampleType.hasNameAsIdCol()) @Override public boolean next() throws BatchValidationException { - // consider add onFirst() as callback from SimpleTranslator - if (first) - onFirst(); - // calls processNextInput() boolean next = super.next(); if (!next) { - if (null != nameState) - nameState.cleanUp(); + if (null != _nameState) + _nameState.cleanUp(); } return next; } @@ -1773,8 +1757,8 @@ public boolean next() throws BatchValidationException public void close() throws IOException { super.close(); - if (null != nameState) - nameState.close(); + if (null != _nameState) + _nameState.close(); } private boolean rowExists(String name) @@ -1782,8 +1766,8 @@ private boolean rowExists(String name) if (_existingNames == null) { _existingNames = new HashSet<>(); - SamplesSchema schema = new SamplesSchema(User.getSearchUser(), _container); - TableSelector ts = new TableSelector(schema.getTable(sampleType, null), Collections.singleton("Name")).setMaxRows(1_000_000); + SamplesSchema schema = new SamplesSchema(User.getSearchUser(), _sampleType.getContainer()); + TableSelector ts = new TableSelector(schema.getTable(_sampleType, null), Collections.singleton("Name")).setMaxRows(1_000_000); ts.fillSet(_existingNames); } return _existingNames.contains(name); @@ -2002,11 +1986,13 @@ else if (aliquotedFrom instanceof Number) public static class SampleNameGeneratorState extends NameGeneratorState { private final NameGenerator aliquotNameGenerator; + private final ExpSampleTypeImpl sampleType; - public SampleNameGeneratorState(@NotNull NameGenerator nameGenerator, boolean incrementSampleCounts, @Nullable NameGenerator aliquotNameGenerator) + public SampleNameGeneratorState(@NotNull ExpSampleTypeImpl sampleType, @NotNull NameGenerator nameGenerator, boolean incrementSampleCounts, @Nullable NameGenerator aliquotNameGenerator) { super(nameGenerator, incrementSampleCounts, nameGenerator.getExpressionSummary() == null ? null : nameGenerator.getExpressionSummary().sampleSummary()); this.aliquotNameGenerator = aliquotNameGenerator; + this.sampleType = sampleType; } public String nextName(Map map, @Nullable List>> _extraPropsFns) throws NameGenerator.NameGenerationException @@ -2019,32 +2005,60 @@ public String nextName(Map map, @Nullable Set parentSamples, @Nullable List>> _extraPropsFns) throws NameGenerator.NameGenerationException { - String aliquotedFrom = null; + String aliquotedFrom = resolveAliquotParentName(map); + boolean isAliquot = !StringUtils.isEmpty(aliquotedFrom); + + String generatedName = null; + if (isAliquot && aliquotNameGenerator != null) + generatedName = nextName(map, parentDatas, parentSamples, _extraPropsFns, aliquotNameGenerator); + else if (!isAliquot) + generatedName = nextName(map, parentDatas, parentSamples, _extraPropsFns, null); + + return generatedName; + } + + private @Nullable ExpMaterial findAliquotParent(Object sampleIdentifier) throws NameGenerator.NameGenerationException + { + try + { + return ExperimentService.get().findExpMaterial(_container, _user, sampleIdentifier, sampleType, renameCache, materialCache); + } + catch (ValidationException e) + { + throw new NameGenerator.NameGenerationException(_rowNumber, e); + } + } + + private @Nullable String resolveAliquotParentName(Map map) throws NameGenerator.NameGenerationException + { Object aliquotedFromObj = map.get(ExpMaterial.ALIQUOTED_FROM_INPUT); - if (aliquotedFromObj != null) + if (aliquotedFromObj == null) + return null; + + String aliquotedFrom = null; + + if (aliquotedFromObj instanceof String aliquotStr) { - if (aliquotedFromObj instanceof String) + // Issue 45563: Unquote and trim the string to find the parent properly + aliquotedFrom = StringUtilsLabKey.unquoteString(aliquotStr).trim(); + } + else if (aliquotedFromObj instanceof Number) + { + // Issue 53153: support "RowId" as value for "AliquotedFrom" + ExpMaterial aliquotParent = findAliquotParent(aliquotedFromObj); + + if (aliquotParent != null) { - // Issue 45563: We need the AliquotedFrom name to be quoted so we can properly find the parent, - // but we don't want to include the quotes in the name we generate using AliquotedFrom - aliquotedFrom = StringUtilsLabKey.unquoteString((String) aliquotedFromObj).trim(); + aliquotedFrom = aliquotParent.getName(); map.put(ExpMaterial.ALIQUOTED_FROM_INPUT, aliquotedFrom); } - else if (aliquotedFromObj instanceof Number) + else { aliquotedFrom = aliquotedFromObj.toString(); } } - boolean isAliquot = !StringUtils.isEmpty(aliquotedFrom); - - String generatedName = null; - if (isAliquot && aliquotNameGenerator != null) - generatedName = nextName(map, parentDatas, parentSamples, _extraPropsFns, aliquotNameGenerator); - else if (!isAliquot) - generatedName = nextName(map, parentDatas, parentSamples, _extraPropsFns, null); - - return generatedName; + return aliquotedFrom; } } } From 71e8076401ae0021ee8349b38dd203299c817693 Mon Sep 17 00:00:00 2001 From: labkey-nicka Date: Mon, 2 Jun 2025 16:15:26 -0700 Subject: [PATCH 06/22] Resolve number names by name, then rowId --- .../experiment/api/SampleTypeUpdateServiceDI.java | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/experiment/src/org/labkey/experiment/api/SampleTypeUpdateServiceDI.java b/experiment/src/org/labkey/experiment/api/SampleTypeUpdateServiceDI.java index e340c96c716..8a96ae37dbb 100644 --- a/experiment/src/org/labkey/experiment/api/SampleTypeUpdateServiceDI.java +++ b/experiment/src/org/labkey/experiment/api/SampleTypeUpdateServiceDI.java @@ -2039,23 +2039,22 @@ else if (!isAliquot) if (aliquotedFromObj instanceof String aliquotStr) { - // Issue 45563: Unquote and trim the string to find the parent properly + // Issue 45563: We need the AliquotedFrom name to be quoted so we can properly find the parent, + // but we don't want to include the quotes in the name we generate using AliquotedFrom aliquotedFrom = StringUtilsLabKey.unquoteString(aliquotStr).trim(); + map.put(ExpMaterial.ALIQUOTED_FROM_INPUT, aliquotedFrom); } else if (aliquotedFromObj instanceof Number) { - // Issue 53153: support "RowId" as value for "AliquotedFrom" - ExpMaterial aliquotParent = findAliquotParent(aliquotedFromObj); + aliquotedFrom = aliquotedFromObj.toString(); + // Issue 53153: support "RowId" as value for "AliquotedFrom" + ExpMaterial aliquotParent = findAliquotParent(aliquotedFrom); if (aliquotParent != null) { aliquotedFrom = aliquotParent.getName(); map.put(ExpMaterial.ALIQUOTED_FROM_INPUT, aliquotedFrom); } - else - { - aliquotedFrom = aliquotedFromObj.toString(); - } } return aliquotedFrom; From 9e5ac8ee428d0068e90ce45015f8a363e34cecd3 Mon Sep 17 00:00:00 2001 From: labkey-nicka Date: Tue, 3 Jun 2025 08:50:59 -0700 Subject: [PATCH 07/22] Always lookup --- .../org/labkey/experiment/api/SampleTypeUpdateServiceDI.java | 3 +++ 1 file changed, 3 insertions(+) diff --git a/experiment/src/org/labkey/experiment/api/SampleTypeUpdateServiceDI.java b/experiment/src/org/labkey/experiment/api/SampleTypeUpdateServiceDI.java index 8a96ae37dbb..76dff2b51fa 100644 --- a/experiment/src/org/labkey/experiment/api/SampleTypeUpdateServiceDI.java +++ b/experiment/src/org/labkey/experiment/api/SampleTypeUpdateServiceDI.java @@ -2047,7 +2047,10 @@ else if (!isAliquot) else if (aliquotedFromObj instanceof Number) { aliquotedFrom = aliquotedFromObj.toString(); + } + if (aliquotedFrom != null) + { // Issue 53153: support "RowId" as value for "AliquotedFrom" ExpMaterial aliquotParent = findAliquotParent(aliquotedFrom); if (aliquotParent != null) From 46e73d7624417a56e4174da956af80aa2c874518 Mon Sep 17 00:00:00 2001 From: labkey-nicka Date: Tue, 3 Jun 2025 15:56:17 -0700 Subject: [PATCH 08/22] AliquotResolutionDataIterator --- .../labkey/experiment/ExpDataIterators.java | 137 ++++++++++++++---- .../experiment/api/ExpSampleTypeImpl.java | 2 +- .../api/SampleTypeUpdateServiceDI.java | 87 ++--------- 3 files changed, 122 insertions(+), 104 deletions(-) diff --git a/experiment/src/org/labkey/experiment/ExpDataIterators.java b/experiment/src/org/labkey/experiment/ExpDataIterators.java index 7063bda6197..f18000da556 100644 --- a/experiment/src/org/labkey/experiment/ExpDataIterators.java +++ b/experiment/src/org/labkey/experiment/ExpDataIterators.java @@ -25,6 +25,7 @@ import org.labkey.api.collections.CaseInsensitiveHashSet; import org.labkey.api.collections.Sets; import org.labkey.api.data.AbstractTableInfo; +import org.labkey.api.data.BaseColumnInfo; import org.labkey.api.data.ColumnInfo; import org.labkey.api.data.CompareType; import org.labkey.api.data.Container; @@ -32,6 +33,7 @@ import org.labkey.api.data.ContainerManager; import org.labkey.api.data.CounterDefinition; import org.labkey.api.data.DbScope; +import org.labkey.api.data.JdbcType; import org.labkey.api.data.NameGenerator; import org.labkey.api.data.RemapCache; import org.labkey.api.data.SimpleFilter; @@ -378,7 +380,7 @@ protected AliquotRollupDataIterator(DataIterator di, DataIteratorContext context _storedAmountCol = map.get(StoredAmount.name()); _unitsCol = map.get(Units.name()); _sampleStateCol = map.get(SampleState.name()); - _aliquotedFromCol = map.get(ALIQUOTED_FROM_INPUT); + _aliquotedFromCol = map.get(AliquotResolutionDataIterator.ALIQUOT_FROM_RESOLVED_NAME); _rootMaterialRowIdCol = map.get(RootMaterialRowId.name()); _rootIdToRecomputeCol = map.get(ROOT_RECOMPUTE_ROWID_COL); _parentNameToRecomputeCol = map.get(PARENT_RECOMPUTE_NAME_COL); @@ -984,12 +986,6 @@ protected Set> _getParentParts() return allParts; } - @Override - public boolean next() throws BatchValidationException - { - return super.next(); - } - protected void _processRun(ExpRunItem runItem, List runRecords, Set> parentNames, @@ -1105,24 +1101,13 @@ protected DerivationDataIterator(DataIterator di, DataIteratorContext context, C { super(di, context, container, user, currentDataType, isSample, false /* for insert/merge, required parents are always checked in StandardDataIteratorBuilder */); _skipAliquot = skipAliquot || context.getConfigParameterBoolean(SampleTypeService.ConfigParameters.DeferAliquotRuns); - - Map map = DataIteratorUtil.createColumnNameMap(di); _lsidNames = new HashMap<>(); _parentNames = new LinkedHashMap<>(); _aliquotParents = new LinkedHashMap<>(); _candidateAliquotNames = new ArrayList<>(); - Integer aliquotParentCol = -1; - for (Map.Entry entry : map.entrySet()) - { - String name = entry.getKey(); - if (isSample() && ALIQUOTED_FROM_INPUT.equalsIgnoreCase(name)) - { - aliquotParentCol = entry.getValue(); - } - } - - _aliquotParentCol = aliquotParentCol; + Map map = DataIteratorUtil.createColumnNameMap(di); + _aliquotParentCol = isSample() ? map.getOrDefault(ALIQUOTED_FROM_INPUT, -1) : -1; } @Override @@ -1284,17 +1269,7 @@ protected ImportWithUpdateDerivationDataIterator(DataIterator di, DataIteratorCo Map map = DataIteratorUtil.createColumnNameMap(di); _parentNames = new LinkedHashMap<>(); _aliquotParents = new LinkedHashMap<>(); - - Integer aliquotParentCol = -1; - for (Map.Entry entry : map.entrySet()) - { - if (isSample() && "AliquotedFromLSID".equalsIgnoreCase(entry.getKey())) - { - aliquotParentCol = entry.getValue(); - } - } - - _aliquotParentCol = aliquotParentCol; + _aliquotParentCol = isSample() ? map.getOrDefault(AliquotedFromLSID.name(), -1) : -1; _useLsid = map.containsKey("lsid") && context.getConfigParameterBoolean(ExperimentService.QueryOptions.UseLsidForUpdate); } @@ -1334,7 +1309,7 @@ else if (o instanceof Number) } else { - getErrors().addRowError(new ValidationException("Expected string value for aliquot parent name: " + o, "AliquotedFromLSID")); + getErrors().addRowError(new ValidationException("Expected string value for aliquot parent name: " + o, AliquotedFromLSID.name())); } if (aliquotParentName != null) @@ -3207,4 +3182,102 @@ public boolean next() throws BatchValidationException return true; } } + + public static class AliquotResolutionDataIterator extends SimpleTranslator + { + public static final String ALIQUOT_FROM_IS_ALIQUOT = ALIQUOTED_FROM_INPUT + "#IsAliquot"; + public static final String ALIQUOT_FROM_RESOLVED_NAME = ALIQUOTED_FROM_INPUT + "#Name"; + public static final String ALIQUOT_FROM_RESOLVED_ROW_ID = ALIQUOTED_FROM_INPUT + "#RowId"; + + final Integer _aliquotFromCol; + final Integer _aliquotNameCol; + final Integer _aliquotRowIdCol; + final Container _container; + final Integer _isAliquotCol; + final Map _materialCache; + final RemapCache _remapCache; + final ExpSampleTypeImpl _sampleType; + final User _user; + + public AliquotResolutionDataIterator(DataIterator di, DataIteratorContext context, Container container, User user, ExpSampleTypeImpl sampleType) + { + super(di, context); + selectAll(); + + _container = container; + _sampleType = sampleType; + _user = user; + + var columnNameMap = getColumnNameMap(); + _aliquotFromCol = columnNameMap.get(ALIQUOTED_FROM_INPUT); + boolean hasAliquotFromCol = _aliquotFromCol != null; + + _materialCache = hasAliquotFromCol ? new HashMap<>() : null; + _remapCache = hasAliquotFromCol ? new RemapCache() : null; + _isAliquotCol = addColumn(new BaseColumnInfo(ALIQUOT_FROM_IS_ALIQUOT, JdbcType.BOOLEAN), (Supplier)() -> hasAliquotFromCol); + + if (hasAliquotFromCol) + { + _aliquotNameCol = addColumn(new BaseColumnInfo(ALIQUOT_FROM_RESOLVED_NAME, JdbcType.VARCHAR), (Supplier)() -> null); + _aliquotRowIdCol = addColumn(new BaseColumnInfo(ALIQUOT_FROM_RESOLVED_ROW_ID, JdbcType.INTEGER), (Supplier)() -> null); + } + else + { + _aliquotNameCol = null; + _aliquotRowIdCol = null; + } + } + + @Override + public boolean next() throws BatchValidationException + { + boolean hasNext = super.next(); + + if (hasNext && _aliquotFromCol != null) + { + Object aliquotedFromObj = get(_aliquotFromCol); + if (aliquotedFromObj != null) + { + String aliquotedFrom = null; + if (aliquotedFromObj instanceof String aliquotStr) + { + // Issue 45563: We need the AliquotedFrom name to be quoted so we can properly find the parent, + // but we don't want to include the quotes in the name we generate using AliquotedFrom + aliquotedFrom = StringUtilsLabKey.unquoteString(aliquotStr).trim(); + } + else if (aliquotedFromObj instanceof Number) + { + // Issue 53153: support "RowId" as value for "AliquotedFrom" + aliquotedFrom = aliquotedFromObj.toString(); + } + + boolean isAliquot = !StringUtils.isEmpty(aliquotedFrom); + _row[_isAliquotCol] = isAliquot; + + if (isAliquot) + { + try + { + ExpMaterial aliquotParent = ExperimentService.get().findExpMaterial(_container, _user, aliquotedFrom, _sampleType, _remapCache, _materialCache); + if (aliquotParent != null) + { + _row[_aliquotNameCol] = aliquotParent.getName(); + _row[_aliquotRowIdCol] = aliquotParent.getRowId(); + } + } + catch (ValidationException e) + { + addRowError(e.getMessage()); + } + } + } + else + { + _row[_isAliquotCol] = false; + } + } + + return hasNext; + } + } } diff --git a/experiment/src/org/labkey/experiment/api/ExpSampleTypeImpl.java b/experiment/src/org/labkey/experiment/api/ExpSampleTypeImpl.java index 9829a81b78a..289dcdea767 100644 --- a/experiment/src/org/labkey/experiment/api/ExpSampleTypeImpl.java +++ b/experiment/src/org/labkey/experiment/api/ExpSampleTypeImpl.java @@ -655,7 +655,7 @@ public SampleTypeUpdateServiceDI.SampleNameGeneratorState getNameGenState(boolea primaryGen.setExpressionSummary(new NameGenerator.ExpressionSummary(sampleNameExpressionSummary, expressionSummary.hasDateBasedSampleCounter(), expressionSummary.hasParentInputs(), expressionSummary.hasParentLookup(), expressionSummary.hasAncestorSearch())); } - return new SampleTypeUpdateServiceDI.SampleNameGeneratorState(this, primaryGen, incrementSampleCounts, aliquotNameGen); + return new SampleTypeUpdateServiceDI.SampleNameGeneratorState(primaryGen, incrementSampleCounts, aliquotNameGen); } private Container getGenIdSequenceContainer() diff --git a/experiment/src/org/labkey/experiment/api/SampleTypeUpdateServiceDI.java b/experiment/src/org/labkey/experiment/api/SampleTypeUpdateServiceDI.java index 76dff2b51fa..962d57087ef 100644 --- a/experiment/src/org/labkey/experiment/api/SampleTypeUpdateServiceDI.java +++ b/experiment/src/org/labkey/experiment/api/SampleTypeUpdateServiceDI.java @@ -151,6 +151,8 @@ import static org.labkey.api.exp.query.ExpMaterialTable.Column.StoredAmount; import static org.labkey.api.exp.query.ExpMaterialTable.Column.Units; import static org.labkey.api.exp.query.SamplesSchema.SCHEMA_SAMPLES; +import static org.labkey.experiment.ExpDataIterators.AliquotResolutionDataIterator.ALIQUOT_FROM_IS_ALIQUOT; +import static org.labkey.experiment.ExpDataIterators.AliquotResolutionDataIterator.ALIQUOT_FROM_RESOLVED_NAME; import static org.labkey.experiment.ExpDataIterators.incrementCounts; import static org.labkey.experiment.api.SampleTypeServiceImpl.SampleChangeType.insert; import static org.labkey.experiment.api.SampleTypeServiceImpl.SampleChangeType.rollup; @@ -1531,6 +1533,9 @@ public DataIterator getDataIterator(DataIteratorContext context) DataIteratorBuilder dib = ExpDataIterators.CounterDataIteratorBuilder.create(dataIterator, sampleType.getContainer(), materialTable, ExpSampleType.SEQUENCE_PREFIX, sampleType.getRowId()); dataIterator = dib.getDataIterator(context); + // AliquotedFrom resolution + dataIterator = LoggingDataIterator.wrap(new ExpDataIterators.AliquotResolutionDataIterator(dataIterator, context, container, user, sampleType)); + // sampleset.createSampleNames() + generate lsid // TODO: does not handle insertIgnore DataIterator names = new _GenerateNamesDataIterator(sampleType, container, user, DataIteratorUtil.wrapMap(dataIterator, false), context, batchSize); @@ -1676,25 +1681,6 @@ protected void processNextInput() { Map map = new CaseInsensitiveHashMap<>(((MapDataIterator)getInput()).getMap()); - String aliquotedFrom = null; - Object aliquotedFromObj = map.get(ExpMaterial.ALIQUOTED_FROM_INPUT); - if (aliquotedFromObj != null) - { - if (aliquotedFromObj instanceof String) - { - // Issue 45563: We need the AliquotedFrom name to be quoted so we can properly find the parent, - // but we don't want to include the quotes in the name we generate using AliquotedFrom - aliquotedFrom = StringUtilsLabKey.unquoteString((String) aliquotedFromObj).trim(); - map.put(ExpMaterial.ALIQUOTED_FROM_INPUT, aliquotedFrom); - } - else if (aliquotedFromObj instanceof Number) - { - aliquotedFrom = aliquotedFromObj.toString(); - } - } - - boolean isAliquot = !StringUtils.isEmpty(aliquotedFrom); - try { Object currNameObj = map.get("Name"); @@ -1724,7 +1710,7 @@ else if (aliquotedFromObj instanceof Number) catch (NameGenerator.NameGenerationException e) { // Failed to generate a name due to some part of the expression not in the row - if (isAliquot) + if ((boolean) map.getOrDefault(ALIQUOT_FROM_IS_ALIQUOT, false)) { addRowError("Failed to generate name for aliquot on row " + e.getRowNumber() + " using aliquot naming pattern " + _sampleType.getAliquotNameExpression() + ". Check the syntax of the aliquot naming pattern and the data values for the aliquot."); } @@ -1986,13 +1972,11 @@ else if (aliquotedFrom instanceof Number) public static class SampleNameGeneratorState extends NameGeneratorState { private final NameGenerator aliquotNameGenerator; - private final ExpSampleTypeImpl sampleType; - public SampleNameGeneratorState(@NotNull ExpSampleTypeImpl sampleType, @NotNull NameGenerator nameGenerator, boolean incrementSampleCounts, @Nullable NameGenerator aliquotNameGenerator) + public SampleNameGeneratorState(@NotNull NameGenerator nameGenerator, boolean incrementSampleCounts, @Nullable NameGenerator aliquotNameGenerator) { super(nameGenerator, incrementSampleCounts, nameGenerator.getExpressionSummary() == null ? null : nameGenerator.getExpressionSummary().sampleSummary()); this.aliquotNameGenerator = aliquotNameGenerator; - this.sampleType = sampleType; } public String nextName(Map map, @Nullable List>> _extraPropsFns) throws NameGenerator.NameGenerationException @@ -2005,8 +1989,15 @@ public String nextName(Map map, @Nullable Set parentSamples, @Nullable List>> _extraPropsFns) throws NameGenerator.NameGenerationException { - String aliquotedFrom = resolveAliquotParentName(map); - boolean isAliquot = !StringUtils.isEmpty(aliquotedFrom); + boolean isAliquot = (boolean) map.getOrDefault(ALIQUOT_FROM_IS_ALIQUOT, false); + if (isAliquot) + { + // Issue 53153: When a name has been resolved for the aliquot, + // then use that instead of the supplied value for AliquotedFrom. + Object name = map.get(ALIQUOT_FROM_RESOLVED_NAME); + if (name instanceof String aliquotParentName) + map.put(ExpMaterial.ALIQUOTED_FROM_INPUT, aliquotParentName); + } String generatedName = null; if (isAliquot && aliquotNameGenerator != null) @@ -2016,51 +2007,5 @@ else if (!isAliquot) return generatedName; } - - private @Nullable ExpMaterial findAliquotParent(Object sampleIdentifier) throws NameGenerator.NameGenerationException - { - try - { - return ExperimentService.get().findExpMaterial(_container, _user, sampleIdentifier, sampleType, renameCache, materialCache); - } - catch (ValidationException e) - { - throw new NameGenerator.NameGenerationException(_rowNumber, e); - } - } - - private @Nullable String resolveAliquotParentName(Map map) throws NameGenerator.NameGenerationException - { - Object aliquotedFromObj = map.get(ExpMaterial.ALIQUOTED_FROM_INPUT); - if (aliquotedFromObj == null) - return null; - - String aliquotedFrom = null; - - if (aliquotedFromObj instanceof String aliquotStr) - { - // Issue 45563: We need the AliquotedFrom name to be quoted so we can properly find the parent, - // but we don't want to include the quotes in the name we generate using AliquotedFrom - aliquotedFrom = StringUtilsLabKey.unquoteString(aliquotStr).trim(); - map.put(ExpMaterial.ALIQUOTED_FROM_INPUT, aliquotedFrom); - } - else if (aliquotedFromObj instanceof Number) - { - aliquotedFrom = aliquotedFromObj.toString(); - } - - if (aliquotedFrom != null) - { - // Issue 53153: support "RowId" as value for "AliquotedFrom" - ExpMaterial aliquotParent = findAliquotParent(aliquotedFrom); - if (aliquotParent != null) - { - aliquotedFrom = aliquotParent.getName(); - map.put(ExpMaterial.ALIQUOTED_FROM_INPUT, aliquotedFrom); - } - } - - return aliquotedFrom; - } } } From fb3f434769112eb582cdf7791caa5730845fc359 Mon Sep 17 00:00:00 2001 From: labkey-nicka Date: Thu, 5 Jun 2025 12:21:31 -0700 Subject: [PATCH 09/22] Use instance variables --- .../labkey/experiment/ExpDataIterators.java | 44 ++++++++----------- 1 file changed, 18 insertions(+), 26 deletions(-) diff --git a/experiment/src/org/labkey/experiment/ExpDataIterators.java b/experiment/src/org/labkey/experiment/ExpDataIterators.java index f18000da556..160613e08ed 100644 --- a/experiment/src/org/labkey/experiment/ExpDataIterators.java +++ b/experiment/src/org/labkey/experiment/ExpDataIterators.java @@ -1155,11 +1155,7 @@ else if (!_skipAliquot && _context.getInsertOption().mergeRows) { try { - boolean allowBulkLoadRemapCache = true; - if (_context.getConfigParameterBoolean(SkipBulkRemapCache)) - allowBulkLoadRemapCache = false; - - RemapCache cache = new RemapCache(allowBulkLoadRemapCache); + RemapCache cache = new RemapCache(!_context.getConfigParameterBoolean(SkipBulkRemapCache)); Map materialCache = new HashMap<>(); Map dataCache = new HashMap<>(); @@ -3185,13 +3181,11 @@ public boolean next() throws BatchValidationException public static class AliquotResolutionDataIterator extends SimpleTranslator { - public static final String ALIQUOT_FROM_IS_ALIQUOT = ALIQUOTED_FROM_INPUT + "#IsAliquot"; - public static final String ALIQUOT_FROM_RESOLVED_NAME = ALIQUOTED_FROM_INPUT + "#Name"; - public static final String ALIQUOT_FROM_RESOLVED_ROW_ID = ALIQUOTED_FROM_INPUT + "#RowId"; + public static final String ALIQUOT_FROM_IS_ALIQUOT = "_" + AliquotResolutionDataIterator.class.getName() + "#IsAliquot"; + public static final String ALIQUOT_FROM_RESOLVED_NAME = "_" + AliquotResolutionDataIterator.class.getName() + "#Name"; + public static final String ALIQUOT_FROM_RESOLVED_ROW_ID = "_" + AliquotResolutionDataIterator.class.getName() + "#RowId"; final Integer _aliquotFromCol; - final Integer _aliquotNameCol; - final Integer _aliquotRowIdCol; final Container _container; final Integer _isAliquotCol; final Map _materialCache; @@ -3199,6 +3193,10 @@ public static class AliquotResolutionDataIterator extends SimpleTranslator final ExpSampleTypeImpl _sampleType; final User _user; + String aliquotName = null; + Integer aliquotRowId = null; + boolean isAliquot = false; + public AliquotResolutionDataIterator(DataIterator di, DataIteratorContext context, Container container, User user, ExpSampleTypeImpl sampleType) { super(di, context); @@ -3214,17 +3212,12 @@ public AliquotResolutionDataIterator(DataIterator di, DataIteratorContext contex _materialCache = hasAliquotFromCol ? new HashMap<>() : null; _remapCache = hasAliquotFromCol ? new RemapCache() : null; - _isAliquotCol = addColumn(new BaseColumnInfo(ALIQUOT_FROM_IS_ALIQUOT, JdbcType.BOOLEAN), (Supplier)() -> hasAliquotFromCol); + _isAliquotCol = addColumn(new BaseColumnInfo(ALIQUOT_FROM_IS_ALIQUOT, JdbcType.BOOLEAN), (Supplier)() -> isAliquot); if (hasAliquotFromCol) { - _aliquotNameCol = addColumn(new BaseColumnInfo(ALIQUOT_FROM_RESOLVED_NAME, JdbcType.VARCHAR), (Supplier)() -> null); - _aliquotRowIdCol = addColumn(new BaseColumnInfo(ALIQUOT_FROM_RESOLVED_ROW_ID, JdbcType.INTEGER), (Supplier)() -> null); - } - else - { - _aliquotNameCol = null; - _aliquotRowIdCol = null; + addColumn(new BaseColumnInfo(ALIQUOT_FROM_RESOLVED_NAME, JdbcType.VARCHAR), (Supplier)() -> aliquotName); + addColumn(new BaseColumnInfo(ALIQUOT_FROM_RESOLVED_ROW_ID, JdbcType.INTEGER), (Supplier)() -> aliquotRowId); } } @@ -3235,6 +3228,10 @@ public boolean next() throws BatchValidationException if (hasNext && _aliquotFromCol != null) { + aliquotName = null; + aliquotRowId = null; + isAliquot = false; + Object aliquotedFromObj = get(_aliquotFromCol); if (aliquotedFromObj != null) { @@ -3251,8 +3248,7 @@ else if (aliquotedFromObj instanceof Number) aliquotedFrom = aliquotedFromObj.toString(); } - boolean isAliquot = !StringUtils.isEmpty(aliquotedFrom); - _row[_isAliquotCol] = isAliquot; + isAliquot = !StringUtils.isEmpty(aliquotedFrom); if (isAliquot) { @@ -3261,8 +3257,8 @@ else if (aliquotedFromObj instanceof Number) ExpMaterial aliquotParent = ExperimentService.get().findExpMaterial(_container, _user, aliquotedFrom, _sampleType, _remapCache, _materialCache); if (aliquotParent != null) { - _row[_aliquotNameCol] = aliquotParent.getName(); - _row[_aliquotRowIdCol] = aliquotParent.getRowId(); + aliquotName = aliquotParent.getName(); + aliquotRowId = aliquotParent.getRowId(); } } catch (ValidationException e) @@ -3271,10 +3267,6 @@ else if (aliquotedFromObj instanceof Number) } } } - else - { - _row[_isAliquotCol] = false; - } } return hasNext; From 1b989e063242ffe054e6a5e42669769a7836bb22 Mon Sep 17 00:00:00 2001 From: labkey-nicka Date: Thu, 5 Jun 2025 15:10:44 -0700 Subject: [PATCH 10/22] Revert to working paradigm --- .../labkey/experiment/ExpDataIterators.java | 32 +++++++++++-------- 1 file changed, 18 insertions(+), 14 deletions(-) diff --git a/experiment/src/org/labkey/experiment/ExpDataIterators.java b/experiment/src/org/labkey/experiment/ExpDataIterators.java index 160613e08ed..678dfa6a240 100644 --- a/experiment/src/org/labkey/experiment/ExpDataIterators.java +++ b/experiment/src/org/labkey/experiment/ExpDataIterators.java @@ -3186,6 +3186,8 @@ public static class AliquotResolutionDataIterator extends SimpleTranslator public static final String ALIQUOT_FROM_RESOLVED_ROW_ID = "_" + AliquotResolutionDataIterator.class.getName() + "#RowId"; final Integer _aliquotFromCol; + final Integer _aliquotNameCol; + final Integer _aliquotRowIdCol; final Container _container; final Integer _isAliquotCol; final Map _materialCache; @@ -3193,10 +3195,6 @@ public static class AliquotResolutionDataIterator extends SimpleTranslator final ExpSampleTypeImpl _sampleType; final User _user; - String aliquotName = null; - Integer aliquotRowId = null; - boolean isAliquot = false; - public AliquotResolutionDataIterator(DataIterator di, DataIteratorContext context, Container container, User user, ExpSampleTypeImpl sampleType) { super(di, context); @@ -3212,12 +3210,17 @@ public AliquotResolutionDataIterator(DataIterator di, DataIteratorContext contex _materialCache = hasAliquotFromCol ? new HashMap<>() : null; _remapCache = hasAliquotFromCol ? new RemapCache() : null; - _isAliquotCol = addColumn(new BaseColumnInfo(ALIQUOT_FROM_IS_ALIQUOT, JdbcType.BOOLEAN), (Supplier)() -> isAliquot); + _isAliquotCol = addColumn(new BaseColumnInfo(ALIQUOT_FROM_IS_ALIQUOT, JdbcType.BOOLEAN), (Supplier)() -> hasAliquotFromCol); if (hasAliquotFromCol) { - addColumn(new BaseColumnInfo(ALIQUOT_FROM_RESOLVED_NAME, JdbcType.VARCHAR), (Supplier)() -> aliquotName); - addColumn(new BaseColumnInfo(ALIQUOT_FROM_RESOLVED_ROW_ID, JdbcType.INTEGER), (Supplier)() -> aliquotRowId); + _aliquotNameCol = addColumn(new BaseColumnInfo(ALIQUOT_FROM_RESOLVED_NAME, JdbcType.VARCHAR), (Supplier)() -> null); + _aliquotRowIdCol = addColumn(new BaseColumnInfo(ALIQUOT_FROM_RESOLVED_ROW_ID, JdbcType.INTEGER), (Supplier)() -> null); + } + else + { + _aliquotNameCol = null; + _aliquotRowIdCol = null; } } @@ -3228,10 +3231,6 @@ public boolean next() throws BatchValidationException if (hasNext && _aliquotFromCol != null) { - aliquotName = null; - aliquotRowId = null; - isAliquot = false; - Object aliquotedFromObj = get(_aliquotFromCol); if (aliquotedFromObj != null) { @@ -3248,7 +3247,8 @@ else if (aliquotedFromObj instanceof Number) aliquotedFrom = aliquotedFromObj.toString(); } - isAliquot = !StringUtils.isEmpty(aliquotedFrom); + boolean isAliquot = !StringUtils.isEmpty(aliquotedFrom); + _row[_isAliquotCol] = isAliquot; if (isAliquot) { @@ -3257,8 +3257,8 @@ else if (aliquotedFromObj instanceof Number) ExpMaterial aliquotParent = ExperimentService.get().findExpMaterial(_container, _user, aliquotedFrom, _sampleType, _remapCache, _materialCache); if (aliquotParent != null) { - aliquotName = aliquotParent.getName(); - aliquotRowId = aliquotParent.getRowId(); + _row[_aliquotNameCol] = aliquotParent.getName(); + _row[_aliquotRowIdCol] = aliquotParent.getRowId(); } } catch (ValidationException e) @@ -3267,6 +3267,10 @@ else if (aliquotedFromObj instanceof Number) } } } + else + { + _row[_isAliquotCol] = false; + } } return hasNext; From 7d2d3a63c742c4151d03511f7d7d64a213010728 Mon Sep 17 00:00:00 2001 From: labkey-nicka Date: Mon, 23 Jun 2025 15:11:46 -0700 Subject: [PATCH 11/22] Put name in a list --- experiment/src/org/labkey/experiment/ExpDataIterators.java | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/experiment/src/org/labkey/experiment/ExpDataIterators.java b/experiment/src/org/labkey/experiment/ExpDataIterators.java index 678dfa6a240..209e5f199e8 100644 --- a/experiment/src/org/labkey/experiment/ExpDataIterators.java +++ b/experiment/src/org/labkey/experiment/ExpDataIterators.java @@ -3257,7 +3257,10 @@ else if (aliquotedFromObj instanceof Number) ExpMaterial aliquotParent = ExperimentService.get().findExpMaterial(_container, _user, aliquotedFrom, _sampleType, _remapCache, _materialCache); if (aliquotParent != null) { - _row[_aliquotNameCol] = aliquotParent.getName(); + // This is put into a collection to indicate that this is a processed name that does + // not need to be escaped with quotes, etc. which avoids the complexities of the + // default name string handling. + _row[_aliquotNameCol] = List.of(aliquotParent.getName()); _row[_aliquotRowIdCol] = aliquotParent.getRowId(); } } From 59da99ebf50660558e8ea97d77a1700b4a9acb63 Mon Sep 17 00:00:00 2001 From: labkey-nicka Date: Tue, 24 Jun 2025 09:22:52 -0700 Subject: [PATCH 12/22] Support not yet created aliquot parents --- experiment/src/org/labkey/experiment/ExpDataIterators.java | 5 +++++ .../org/labkey/experiment/api/SampleTypeUpdateServiceDI.java | 2 ++ 2 files changed, 7 insertions(+) diff --git a/experiment/src/org/labkey/experiment/ExpDataIterators.java b/experiment/src/org/labkey/experiment/ExpDataIterators.java index 209e5f199e8..f69900f38e1 100644 --- a/experiment/src/org/labkey/experiment/ExpDataIterators.java +++ b/experiment/src/org/labkey/experiment/ExpDataIterators.java @@ -3263,6 +3263,11 @@ else if (aliquotedFromObj instanceof Number) _row[_aliquotNameCol] = List.of(aliquotParent.getName()); _row[_aliquotRowIdCol] = aliquotParent.getRowId(); } + else + { + // The referenced sample may not exist yet (e.g. it may be created in the same import) + _row[_aliquotNameCol] = aliquotedFromObj.toString(); + } } catch (ValidationException e) { diff --git a/experiment/src/org/labkey/experiment/api/SampleTypeUpdateServiceDI.java b/experiment/src/org/labkey/experiment/api/SampleTypeUpdateServiceDI.java index 962d57087ef..39039634966 100644 --- a/experiment/src/org/labkey/experiment/api/SampleTypeUpdateServiceDI.java +++ b/experiment/src/org/labkey/experiment/api/SampleTypeUpdateServiceDI.java @@ -1997,6 +1997,8 @@ public String nextName(Map map, Object name = map.get(ALIQUOT_FROM_RESOLVED_NAME); if (name instanceof String aliquotParentName) map.put(ExpMaterial.ALIQUOTED_FROM_INPUT, aliquotParentName); + else if (name instanceof List nameList) + map.put(ExpMaterial.ALIQUOTED_FROM_INPUT, nameList.get(0)); } String generatedName = null; From 901fce5d09c0dc8d1438e7c44d4269f3b4e5c33e Mon Sep 17 00:00:00 2001 From: labkey-nicka Date: Wed, 2 Jul 2025 16:17:24 -0700 Subject: [PATCH 13/22] Revert SampleNameGeneratorState --- .../api/SampleTypeUpdateServiceDI.java | 25 ++++++++++++------- 1 file changed, 16 insertions(+), 9 deletions(-) diff --git a/experiment/src/org/labkey/experiment/api/SampleTypeUpdateServiceDI.java b/experiment/src/org/labkey/experiment/api/SampleTypeUpdateServiceDI.java index 39039634966..aaf0121293e 100644 --- a/experiment/src/org/labkey/experiment/api/SampleTypeUpdateServiceDI.java +++ b/experiment/src/org/labkey/experiment/api/SampleTypeUpdateServiceDI.java @@ -1989,18 +1989,25 @@ public String nextName(Map map, @Nullable Set parentSamples, @Nullable List>> _extraPropsFns) throws NameGenerator.NameGenerationException { - boolean isAliquot = (boolean) map.getOrDefault(ALIQUOT_FROM_IS_ALIQUOT, false); - if (isAliquot) + String aliquotedFrom = null; + Object aliquotedFromObj = map.get(ExpMaterial.ALIQUOTED_FROM_INPUT); + if (aliquotedFromObj != null) { - // Issue 53153: When a name has been resolved for the aliquot, - // then use that instead of the supplied value for AliquotedFrom. - Object name = map.get(ALIQUOT_FROM_RESOLVED_NAME); - if (name instanceof String aliquotParentName) - map.put(ExpMaterial.ALIQUOTED_FROM_INPUT, aliquotParentName); - else if (name instanceof List nameList) - map.put(ExpMaterial.ALIQUOTED_FROM_INPUT, nameList.get(0)); + if (aliquotedFromObj instanceof String) + { + // Issue 45563: We need the AliquotedFrom name to be quoted so we can properly find the parent, + // but we don't want to include the quotes in the name we generate using AliquotedFrom + aliquotedFrom = StringUtilsLabKey.unquoteString((String) aliquotedFromObj).trim(); + map.put(ExpMaterial.ALIQUOTED_FROM_INPUT, aliquotedFrom); + } + else if (aliquotedFromObj instanceof Number) + { + aliquotedFrom = aliquotedFromObj.toString(); + } } + boolean isAliquot = !StringUtils.isEmpty(aliquotedFrom); + String generatedName = null; if (isAliquot && aliquotNameGenerator != null) generatedName = nextName(map, parentDatas, parentSamples, _extraPropsFns, aliquotNameGenerator); From f292b5af42e66e7800a144e73f50eec44cb34877 Mon Sep 17 00:00:00 2001 From: labkey-nicka Date: Wed, 2 Jul 2025 16:34:15 -0700 Subject: [PATCH 14/22] Remove AliquotResolutionDataIterator --- .../labkey/experiment/ExpDataIterators.java | 108 +----------------- .../api/SampleTypeUpdateServiceDI.java | 43 ++++--- 2 files changed, 27 insertions(+), 124 deletions(-) diff --git a/experiment/src/org/labkey/experiment/ExpDataIterators.java b/experiment/src/org/labkey/experiment/ExpDataIterators.java index f69900f38e1..c2bb778375a 100644 --- a/experiment/src/org/labkey/experiment/ExpDataIterators.java +++ b/experiment/src/org/labkey/experiment/ExpDataIterators.java @@ -380,7 +380,7 @@ protected AliquotRollupDataIterator(DataIterator di, DataIteratorContext context _storedAmountCol = map.get(StoredAmount.name()); _unitsCol = map.get(Units.name()); _sampleStateCol = map.get(SampleState.name()); - _aliquotedFromCol = map.get(AliquotResolutionDataIterator.ALIQUOT_FROM_RESOLVED_NAME); + _aliquotedFromCol = map.get(ALIQUOTED_FROM_INPUT); _rootMaterialRowIdCol = map.get(RootMaterialRowId.name()); _rootIdToRecomputeCol = map.get(ROOT_RECOMPUTE_ROWID_COL); _parentNameToRecomputeCol = map.get(PARENT_RECOMPUTE_NAME_COL); @@ -3178,110 +3178,4 @@ public boolean next() throws BatchValidationException return true; } } - - public static class AliquotResolutionDataIterator extends SimpleTranslator - { - public static final String ALIQUOT_FROM_IS_ALIQUOT = "_" + AliquotResolutionDataIterator.class.getName() + "#IsAliquot"; - public static final String ALIQUOT_FROM_RESOLVED_NAME = "_" + AliquotResolutionDataIterator.class.getName() + "#Name"; - public static final String ALIQUOT_FROM_RESOLVED_ROW_ID = "_" + AliquotResolutionDataIterator.class.getName() + "#RowId"; - - final Integer _aliquotFromCol; - final Integer _aliquotNameCol; - final Integer _aliquotRowIdCol; - final Container _container; - final Integer _isAliquotCol; - final Map _materialCache; - final RemapCache _remapCache; - final ExpSampleTypeImpl _sampleType; - final User _user; - - public AliquotResolutionDataIterator(DataIterator di, DataIteratorContext context, Container container, User user, ExpSampleTypeImpl sampleType) - { - super(di, context); - selectAll(); - - _container = container; - _sampleType = sampleType; - _user = user; - - var columnNameMap = getColumnNameMap(); - _aliquotFromCol = columnNameMap.get(ALIQUOTED_FROM_INPUT); - boolean hasAliquotFromCol = _aliquotFromCol != null; - - _materialCache = hasAliquotFromCol ? new HashMap<>() : null; - _remapCache = hasAliquotFromCol ? new RemapCache() : null; - _isAliquotCol = addColumn(new BaseColumnInfo(ALIQUOT_FROM_IS_ALIQUOT, JdbcType.BOOLEAN), (Supplier)() -> hasAliquotFromCol); - - if (hasAliquotFromCol) - { - _aliquotNameCol = addColumn(new BaseColumnInfo(ALIQUOT_FROM_RESOLVED_NAME, JdbcType.VARCHAR), (Supplier)() -> null); - _aliquotRowIdCol = addColumn(new BaseColumnInfo(ALIQUOT_FROM_RESOLVED_ROW_ID, JdbcType.INTEGER), (Supplier)() -> null); - } - else - { - _aliquotNameCol = null; - _aliquotRowIdCol = null; - } - } - - @Override - public boolean next() throws BatchValidationException - { - boolean hasNext = super.next(); - - if (hasNext && _aliquotFromCol != null) - { - Object aliquotedFromObj = get(_aliquotFromCol); - if (aliquotedFromObj != null) - { - String aliquotedFrom = null; - if (aliquotedFromObj instanceof String aliquotStr) - { - // Issue 45563: We need the AliquotedFrom name to be quoted so we can properly find the parent, - // but we don't want to include the quotes in the name we generate using AliquotedFrom - aliquotedFrom = StringUtilsLabKey.unquoteString(aliquotStr).trim(); - } - else if (aliquotedFromObj instanceof Number) - { - // Issue 53153: support "RowId" as value for "AliquotedFrom" - aliquotedFrom = aliquotedFromObj.toString(); - } - - boolean isAliquot = !StringUtils.isEmpty(aliquotedFrom); - _row[_isAliquotCol] = isAliquot; - - if (isAliquot) - { - try - { - ExpMaterial aliquotParent = ExperimentService.get().findExpMaterial(_container, _user, aliquotedFrom, _sampleType, _remapCache, _materialCache); - if (aliquotParent != null) - { - // This is put into a collection to indicate that this is a processed name that does - // not need to be escaped with quotes, etc. which avoids the complexities of the - // default name string handling. - _row[_aliquotNameCol] = List.of(aliquotParent.getName()); - _row[_aliquotRowIdCol] = aliquotParent.getRowId(); - } - else - { - // The referenced sample may not exist yet (e.g. it may be created in the same import) - _row[_aliquotNameCol] = aliquotedFromObj.toString(); - } - } - catch (ValidationException e) - { - addRowError(e.getMessage()); - } - } - } - else - { - _row[_isAliquotCol] = false; - } - } - - return hasNext; - } - } } diff --git a/experiment/src/org/labkey/experiment/api/SampleTypeUpdateServiceDI.java b/experiment/src/org/labkey/experiment/api/SampleTypeUpdateServiceDI.java index aaf0121293e..cbdd367f5dd 100644 --- a/experiment/src/org/labkey/experiment/api/SampleTypeUpdateServiceDI.java +++ b/experiment/src/org/labkey/experiment/api/SampleTypeUpdateServiceDI.java @@ -151,8 +151,6 @@ import static org.labkey.api.exp.query.ExpMaterialTable.Column.StoredAmount; import static org.labkey.api.exp.query.ExpMaterialTable.Column.Units; import static org.labkey.api.exp.query.SamplesSchema.SCHEMA_SAMPLES; -import static org.labkey.experiment.ExpDataIterators.AliquotResolutionDataIterator.ALIQUOT_FROM_IS_ALIQUOT; -import static org.labkey.experiment.ExpDataIterators.AliquotResolutionDataIterator.ALIQUOT_FROM_RESOLVED_NAME; import static org.labkey.experiment.ExpDataIterators.incrementCounts; import static org.labkey.experiment.api.SampleTypeServiceImpl.SampleChangeType.insert; import static org.labkey.experiment.api.SampleTypeServiceImpl.SampleChangeType.rollup; @@ -1102,7 +1100,7 @@ private record ExistingRowSelect(TableInfo tableInfo, Set columns, boole remap = CaseInsensitiveHashMap.of(); // AliquotRollupDataIterator needs "samplestate", "storedamount", "rootmaterialrowId", "units" for MERGE option - Set includedColumns = new CaseInsensitiveHashSet("name", "lsid", "rowid", "samplestate", "storedamount", "rootmaterialrowId", "units"); + Set includedColumns = new CaseInsensitiveHashSet(Name.name(), LSID.name(), RowId.name(), SampleState.name(), StoredAmount.name(), RootMaterialRowId.name(), Units.name()); for (ColumnInfo column : getQueryTable().getColumns()) { if (dataColumns.contains(column.getColumnName())) @@ -1533,9 +1531,6 @@ public DataIterator getDataIterator(DataIteratorContext context) DataIteratorBuilder dib = ExpDataIterators.CounterDataIteratorBuilder.create(dataIterator, sampleType.getContainer(), materialTable, ExpSampleType.SEQUENCE_PREFIX, sampleType.getRowId()); dataIterator = dib.getDataIterator(context); - // AliquotedFrom resolution - dataIterator = LoggingDataIterator.wrap(new ExpDataIterators.AliquotResolutionDataIterator(dataIterator, context, container, user, sampleType)); - // sampleset.createSampleNames() + generate lsid // TODO: does not handle insertIgnore DataIterator names = new _GenerateNamesDataIterator(sampleType, container, user, DataIteratorUtil.wrapMap(dataIterator, false), context, batchSize); @@ -1681,6 +1676,25 @@ protected void processNextInput() { Map map = new CaseInsensitiveHashMap<>(((MapDataIterator)getInput()).getMap()); + String aliquotedFrom = null; + Object aliquotedFromObj = map.get(ExpMaterial.ALIQUOTED_FROM_INPUT); + if (aliquotedFromObj != null) + { + if (aliquotedFromObj instanceof String) + { + // Issue 45563: We need the AliquotedFrom name to be quoted so we can properly find the parent, + // but we don't want to include the quotes in the name we generate using AliquotedFrom + aliquotedFrom = StringUtilsLabKey.unquoteString((String) aliquotedFromObj).trim(); + map.put(ExpMaterial.ALIQUOTED_FROM_INPUT, aliquotedFrom); + } + else if (aliquotedFromObj instanceof Number) + { + aliquotedFrom = aliquotedFromObj.toString(); + } + } + + boolean isAliquot = !StringUtils.isEmpty(aliquotedFrom); + try { Object currNameObj = map.get("Name"); @@ -1710,19 +1724,14 @@ protected void processNextInput() catch (NameGenerator.NameGenerationException e) { // Failed to generate a name due to some part of the expression not in the row - if ((boolean) map.getOrDefault(ALIQUOT_FROM_IS_ALIQUOT, false)) - { + if (isAliquot) addRowError("Failed to generate name for aliquot on row " + e.getRowNumber() + " using aliquot naming pattern " + _sampleType.getAliquotNameExpression() + ". Check the syntax of the aliquot naming pattern and the data values for the aliquot."); - } + else if (_sampleType.hasNameExpression()) + addRowError("Failed to generate name for sample on row " + e.getRowNumber() + " using naming pattern " + _sampleType.getNameExpression() + ". Check the syntax of the naming pattern and the data values for the sample."); + else if (_sampleType.hasNameAsIdCol()) + addRowError("SampleID or Name is required for sample on row " + e.getRowNumber()); else - { - if (_sampleType.hasNameExpression()) - addRowError("Failed to generate name for sample on row " + e.getRowNumber() + " using naming pattern " + _sampleType.getNameExpression() + ". Check the syntax of the naming pattern and the data values for the sample."); - else if (_sampleType.hasNameAsIdCol()) - addRowError("SampleID or Name is required for sample on row " + e.getRowNumber()); - else - addRowError("All id columns are required for sample on row " + e.getRowNumber()); - } + addRowError("All id columns are required for sample on row " + e.getRowNumber()); } } From a7f519f85273725c44f6c2e4ffe30bd5f56591bf Mon Sep 17 00:00:00 2001 From: labkey-nicka Date: Thu, 3 Jul 2025 08:53:59 -0700 Subject: [PATCH 15/22] Unfurl FlagDataIterator logic --- .../labkey/experiment/ExpDataIterators.java | 53 +++++++++---------- 1 file changed, 26 insertions(+), 27 deletions(-) diff --git a/experiment/src/org/labkey/experiment/ExpDataIterators.java b/experiment/src/org/labkey/experiment/ExpDataIterators.java index c2bb778375a..59128f85131 100644 --- a/experiment/src/org/labkey/experiment/ExpDataIterators.java +++ b/experiment/src/org/labkey/experiment/ExpDataIterators.java @@ -771,39 +771,38 @@ public boolean next() throws BatchValidationException if (getErrors().hasErrors()) return true; - if (_lsidCol != null && _flagCol != null) + if (_flagCol == null) + return true; + + ExpObject expObject = null; + if (_nameCol != null && _context.getInsertOption().mergeRows) { - Object lsidValue = get(_lsidCol); - Object flagValue = get(_flagCol); + Object nameValue = get(_nameCol); + if (nameValue instanceof String name) + expObject = getExpObjectByName(name); + } + if (expObject == null && _lsidCol != null) + { + Object lsidValue = get(_lsidCol); if (lsidValue instanceof String lsid) - { - try - { - ExpObject expObject = null; - if (_context.getInsertOption().mergeRows && _nameCol != null) - { - Object nameValue = get(_nameCol); - if (nameValue instanceof String name) - expObject = getExpObjectByName(name); - } - - if (expObject == null) - expObject = getExpObjectByLsid(lsid); + expObject = getExpObjectByLsid(lsid); + } - if (expObject != null) - { - String flag = Objects.toString(flagValue, null); - expObject.setComment(_user, flag, false); - } - } - catch (ValidationException e) - { - throw new BatchValidationException(e); - } + if (expObject != null) + { + try + { + Object flagValue = get(_flagCol); + String flag = Objects.toString(flagValue, null); + expObject.setComment(_user, flag, false); + } + catch (ValidationException e) + { + throw new BatchValidationException(e); } - } + return true; } } From c3dc5949b7fed4b045212cb78d167164503c48aa Mon Sep 17 00:00:00 2001 From: labkey-nicka Date: Thu, 3 Jul 2025 09:19:05 -0700 Subject: [PATCH 16/22] Set flag in test --- .../src/org/labkey/experiment/api/ExpDataClassDataTestCase.jsp | 3 +++ 1 file changed, 3 insertions(+) diff --git a/experiment/src/org/labkey/experiment/api/ExpDataClassDataTestCase.jsp b/experiment/src/org/labkey/experiment/api/ExpDataClassDataTestCase.jsp index de0c926d9b0..bda97ad635f 100644 --- a/experiment/src/org/labkey/experiment/api/ExpDataClassDataTestCase.jsp +++ b/experiment/src/org/labkey/experiment/api/ExpDataClassDataTestCase.jsp @@ -301,6 +301,8 @@ private void testInsertIntoSubfolder(ExpDataClassImpl dataClass, TableInfo table Map row = new CaseInsensitiveHashMap<>(); row.put("aa", 30); row.put("bb", "bye"); + String expectedComment = "waving in the wind"; + row.put("flag", expectedComment); rows.add(row); List> ret; @@ -317,6 +319,7 @@ private void testInsertIntoSubfolder(ExpDataClassImpl dataClass, TableInfo table assertNotNull(data); assertEquals(sub, data.getContainer()); assertEquals(2, dataClass.getDatas().size()); + assertEquals(expectedComment, data.getComment()); } private void testInsertDuplicate(ExpDataClassImpl dataClass, TableInfo table) throws Exception From 08ff065af96b687b628010cc69e42d4bda3bdc00 Mon Sep 17 00:00:00 2001 From: labkey-nicka Date: Thu, 3 Jul 2025 12:01:02 -0700 Subject: [PATCH 17/22] null check --- .../org/labkey/experiment/api/ExpDataClassDataTableImpl.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/experiment/src/org/labkey/experiment/api/ExpDataClassDataTableImpl.java b/experiment/src/org/labkey/experiment/api/ExpDataClassDataTableImpl.java index 917d31981a1..e357d02181c 100644 --- a/experiment/src/org/labkey/experiment/api/ExpDataClassDataTableImpl.java +++ b/experiment/src/org/labkey/experiment/api/ExpDataClassDataTableImpl.java @@ -1375,7 +1375,8 @@ protected Map _update(User user, Container c, Map Date: Thu, 3 Jul 2025 16:37:50 -0700 Subject: [PATCH 18/22] Merge and update --- .../labkey/experiment/ExpDataIterators.java | 62 +++++++++---------- 1 file changed, 30 insertions(+), 32 deletions(-) diff --git a/experiment/src/org/labkey/experiment/ExpDataIterators.java b/experiment/src/org/labkey/experiment/ExpDataIterators.java index 59128f85131..628feb5231a 100644 --- a/experiment/src/org/labkey/experiment/ExpDataIterators.java +++ b/experiment/src/org/labkey/experiment/ExpDataIterators.java @@ -25,7 +25,6 @@ import org.labkey.api.collections.CaseInsensitiveHashSet; import org.labkey.api.collections.Sets; import org.labkey.api.data.AbstractTableInfo; -import org.labkey.api.data.BaseColumnInfo; import org.labkey.api.data.ColumnInfo; import org.labkey.api.data.CompareType; import org.labkey.api.data.Container; @@ -33,7 +32,6 @@ import org.labkey.api.data.ContainerManager; import org.labkey.api.data.CounterDefinition; import org.labkey.api.data.DbScope; -import org.labkey.api.data.JdbcType; import org.labkey.api.data.NameGenerator; import org.labkey.api.data.RemapCache; import org.labkey.api.data.SimpleFilter; @@ -497,12 +495,12 @@ public static class AliasDataIteratorBuilder implements DataIteratorBuilder private final boolean _isSample; private final ExpObject _dataType; - public AliasDataIteratorBuilder(@NotNull DataIteratorBuilder in, Container container, User user, TableInfo expTable, ExpObject dataType, boolean isSample) + public AliasDataIteratorBuilder(@NotNull DataIteratorBuilder in, Container container, User user, TableInfo expAliasTable, ExpObject dataType, boolean isSample) { _in = in; _container = container; _user = user; - _expAliasTable = expTable; + _expAliasTable = expAliasTable; _isSample = isSample; _dataType = dataType; } @@ -524,7 +522,7 @@ private static class AliasDataIterator extends ExpDataTypeDataIterator Map _lsidAliasMap = new HashMap<>(); private final TableInfo _expAliasTable; - protected AliasDataIterator(DataIterator di, DataIteratorContext context, Container container, User user, TableInfo expTable, ExpObject dataType, boolean isSample) + protected AliasDataIterator(DataIterator di, DataIteratorContext context, Container container, User user, TableInfo expAliasTable, ExpObject dataType, boolean isSample) { super(di, context, container, user, dataType, isSample); @@ -532,7 +530,7 @@ protected AliasDataIterator(DataIterator di, DataIteratorContext context, Contai _aliasCol = map.get(ALIASCOLUMNALIAS) == null ? null : di.getSupplier(map.get(ALIASCOLUMNALIAS)); _lsidCol = map.get("lsid") == null ? null : di.getSupplier(map.get("lsid")); _nameCol = map.get("name") == null ? null : di.getSupplier(map.get("name")); - _expAliasTable = expTable; + _expAliasTable = expAliasTable; } @Override @@ -540,41 +538,40 @@ public boolean next() throws BatchValidationException { boolean hasNext = super.next(); + // skip processing if aliases are not being modified + if (_aliasCol == null) + return hasNext; + // skip processing if there are errors upstream if (getErrors().hasErrors()) return hasNext; if (hasNext) { - // For each iteration, collect the lsid and alias col values. - if (_aliasCol != null) + // Collect alias values and map them by LSID + String lsid = null; + + if (_nameCol != null && (_context.getInsertOption().mergeRows || _context.getInsertOption().updateOnly)) { - if (_context.getInsertOption().mergeRows && _nameCol != null) + Object nameValue = _nameCol.get(); + if (nameValue instanceof String name) { - Object nameValue = _nameCol.get(); - if (nameValue instanceof String name) - { - ExpObject obj = getExpObjectByName(name); - if (obj != null) - { - String lsid = obj.getLSID(); - if (lsid != null && !lsid.isEmpty()) - _lsidAliasMap.put(lsid, _aliasCol.get()); - } - } + ExpObject obj = getExpObjectByName(name); + if (obj != null) + lsid = obj.getLSID(); } - else if (_lsidCol != null) - { - Object lsidValue = _lsidCol.get(); - Object aliasValue = _aliasCol.get(); + } - if (aliasValue != null && lsidValue instanceof String lsidString) - { - _lsidAliasMap.put(lsidString, aliasValue); - } - } + if (lsid == null && _lsidCol != null) + { + Object lsidValue = _lsidCol.get(); + if (lsidValue instanceof String lsidString) + lsid = lsidString; } + if (!StringUtils.isEmpty(lsid)) + _lsidAliasMap.put(lsid, _aliasCol.get()); + return true; } @@ -775,7 +772,7 @@ public boolean next() throws BatchValidationException return true; ExpObject expObject = null; - if (_nameCol != null && _context.getInsertOption().mergeRows) + if (_nameCol != null && (_context.getInsertOption().mergeRows || _context.getInsertOption().updateOnly)) { Object nameValue = get(_nameCol); if (nameValue instanceof String name) @@ -791,10 +788,11 @@ public boolean next() throws BatchValidationException if (expObject != null) { + Object flagValue = get(_flagCol); + String flag = Objects.toString(flagValue, null); + try { - Object flagValue = get(_flagCol); - String flag = Objects.toString(flagValue, null); expObject.setComment(_user, flag, false); } catch (ValidationException e) From da7caa4fe29e73f5e392f3896f4d21e550bc27b5 Mon Sep 17 00:00:00 2001 From: labkey-nicka Date: Thu, 3 Jul 2025 16:38:11 -0700 Subject: [PATCH 19/22] Tests --- .../api/ExpDataClassDataTestCase.jsp | 92 +++++++++++++++++-- 1 file changed, 85 insertions(+), 7 deletions(-) diff --git a/experiment/src/org/labkey/experiment/api/ExpDataClassDataTestCase.jsp b/experiment/src/org/labkey/experiment/api/ExpDataClassDataTestCase.jsp index bda97ad635f..ffce403d4b0 100644 --- a/experiment/src/org/labkey/experiment/api/ExpDataClassDataTestCase.jsp +++ b/experiment/src/org/labkey/experiment/api/ExpDataClassDataTestCase.jsp @@ -1044,9 +1044,9 @@ public void testInsertOptionUpdate() throws Exception ExperimentServiceImpl.get().createDataClass(c, user, dataClassName, null, props, emptyList(), null, null); List> rowsToAdd = new ArrayList<>(); - rowsToAdd.add(CaseInsensitiveHashMap.of("name", "D-1", "prop", "a", longFieldName, "Very")); - rowsToAdd.add(CaseInsensitiveHashMap.of("name", "D-1-d", "prop", "c", longFieldName, "Long")); - rowsToAdd.add(CaseInsensitiveHashMap.of("name", "D-2", "prop", "b", longFieldName, "Field")); + rowsToAdd.add(CaseInsensitiveHashMap.of("name", "D-1", "prop", "a", longFieldName, "Very", "alias", "Much", "flag", "c100")); + rowsToAdd.add(CaseInsensitiveHashMap.of("name", "D-1-d", "prop", "c", longFieldName, "Long", "alias", "Extended", "flag", "c200")); + rowsToAdd.add(CaseInsensitiveHashMap.of("name", "D-2", "prop", "b", longFieldName, "Field", "alias", "Paddock")); TableInfo table = getDataClassTable(dataClassName); QueryUpdateService qus = table.getUpdateService(); @@ -1069,9 +1069,9 @@ public void testInsertOptionUpdate() throws Exception // update regular properties as well as datainputs List> rowsToUpdate = new ArrayList<>(); - rowsToUpdate.add(CaseInsensitiveHashMap.of("name", "D-1", "prop", "a1", "DataInputs/DataClassWithImportOption", null)); + rowsToUpdate.add(CaseInsensitiveHashMap.of("name", "D-1", "prop", "a1", "DataInputs/DataClassWithImportOption", null, "alias", "A lot")); rowsToUpdate.add(CaseInsensitiveHashMap.of("name", "D-1-d", "prop", "c1", "DataInputs/DataClassWithImportOption", "D-1")); - rowsToUpdate.add(CaseInsensitiveHashMap.of("name", "D-2", "prop", "b1", "DataInputs/DataClassWithImportOption", null)); + rowsToUpdate.add(CaseInsensitiveHashMap.of("name", "D-2", "prop", "b1", "DataInputs/DataClassWithImportOption", null, "alias", "Grassland")); context.setInsertOption(QueryUpdateService.InsertOption.UPDATE); count = qus.loadRows(user, c, MapDataIterator.of(rowsToUpdate), context, null); @@ -1080,21 +1080,99 @@ public void testInsertOptionUpdate() throws Exception assertEquals(3, count); Set columnNames = new HashSet<>(); + columnNames.add("rowId"); + columnNames.add("lsid"); columnNames.add("Name"); columnNames.add("prop"); columnNames.add(longFieldName); + columnNames.add("alias"); + columnNames.add("flag"); - List> rows = Arrays.asList(new TableSelector(table, columnNames, null, new Sort("Name")).getMapArray()); + ts = new TableSelector(table, columnNames, null, new Sort("Name")); + ts.setForDisplay(true); + String aliasAlias = "alias$alias$name"; + String flagAlias = "flag$"; + + List> rows = Arrays.asList(ts.getMapArray()); + + assertEquals("D-1", rows.get(0).get("Name")); + assertEquals("D-1-d", rows.get(1).get("Name")); + assertEquals("D-2", rows.get(2).get("Name")); + int d2RowId = (Integer) rows.get(2).get("RowId"); + + // prop assertEquals("a1", rows.get(0).get("prop")); assertEquals("c1", rows.get(1).get("prop")); assertEquals("b1", rows.get(2).get("prop")); + + // long field assertEquals("Very", rows.get(0).get(longFieldAlias)); assertEquals("Long", rows.get(1).get(longFieldAlias)); assertEquals("Field", rows.get(2).get(longFieldAlias)); + // alias + assertEquals("A lot", rows.get(0).get(aliasAlias)); + assertNull(rows.get(1).get(aliasAlias)); + assertEquals("Grassland", rows.get(2).get(aliasAlias)); + + // flag + assertEquals("c100", rows.get(0).get(flagAlias)); + assertEquals("c200", rows.get(1).get(flagAlias)); + assertNull(rows.get(2).get(flagAlias)); + ts = new TableSelector(dataInputTInfo, filter, null); - int newInputCount = (int) ts.getRowCount(); + int newInputCount = (int) ts.getRowCount(); assertEquals(inputCount + 1, newInputCount); + + List> rowsToMerge = new ArrayList<>(); + rowsToMerge.add(CaseInsensitiveHashMap.of("name", "D-2", "prop", "gene", longFieldName, "Pasture", "alias", "Exceedingly", "flag", "cOne")); + rowsToMerge.add(CaseInsensitiveHashMap.of("name", "D-22", "prop", null, longFieldName, "Goal", "alias", "Immensely", "flag", "cEight")); + + context.setInsertOption(QueryUpdateService.InsertOption.MERGE); + count = qus.loadRows(user, c, MapDataIterator.of(rowsToMerge), context, null); + + assertFalse(context.getErrors().hasErrors()); + assertEquals(2, count); + + ts = new TableSelector(table, columnNames, null, new Sort("Name")); + ts.setForDisplay(true); + + rows = Arrays.asList(ts.getMapArray()); + assertEquals(4, rows.size()); + + assertEquals("D-1", rows.get(0).get("Name")); + assertEquals("D-1-d", rows.get(1).get("Name")); + assertEquals("D-2", rows.get(2).get("Name")); + assertEquals("D-22", rows.get(3).get("Name")); + + // verify merge retained existing row + assertEquals(d2RowId, rows.get(2).get("RowId")); + + // prop + assertEquals("a1", rows.get(0).get("prop")); + assertEquals("c1", rows.get(1).get("prop")); + assertEquals("gene", rows.get(2).get("prop")); + assertNull(rows.get(3).get("prop")); + + // long field + assertEquals("Very", rows.get(0).get(longFieldAlias)); + assertEquals("Long", rows.get(1).get(longFieldAlias)); + // TODO: This is not updating as I would expect. Verify what is expected here with regards to updating via merge. +// assertEquals("Pasture", rows.get(2).get(longFieldAlias)); + assertEquals("Goal", rows.get(3).get(longFieldAlias)); + + // alias + assertEquals("A lot", rows.get(0).get(aliasAlias)); + assertNull(rows.get(1).get(aliasAlias)); + assertEquals("Exceedingly", rows.get(2).get(aliasAlias)); + assertEquals("Immensely", rows.get(3).get(aliasAlias)); + + // flag + assertEquals("c100", rows.get(0).get(flagAlias)); + assertEquals("c200", rows.get(1).get(flagAlias)); + assertEquals("cOne", rows.get(2).get(flagAlias)); + assertEquals("cEight", rows.get(3).get(flagAlias)); + } private @NotNull TableInfo getDataClassTable(String dataClassName) From b17cee303a0bbb1e148ac84b9a751916ff807a38 Mon Sep 17 00:00:00 2001 From: labkey-nicka Date: Tue, 8 Jul 2025 13:18:37 -0700 Subject: [PATCH 20/22] Fix DataIteratorContext usage in tests --- .../experiment/api/ExpDataClassDataTestCase.jsp | 10 +++++----- .../labkey/experiment/api/ExpSampleTypeTestCase.jsp | 12 +++--------- 2 files changed, 8 insertions(+), 14 deletions(-) diff --git a/experiment/src/org/labkey/experiment/api/ExpDataClassDataTestCase.jsp b/experiment/src/org/labkey/experiment/api/ExpDataClassDataTestCase.jsp index ffce403d4b0..a5c5e1ec711 100644 --- a/experiment/src/org/labkey/experiment/api/ExpDataClassDataTestCase.jsp +++ b/experiment/src/org/labkey/experiment/api/ExpDataClassDataTestCase.jsp @@ -1073,6 +1073,7 @@ public void testInsertOptionUpdate() throws Exception rowsToUpdate.add(CaseInsensitiveHashMap.of("name", "D-1-d", "prop", "c1", "DataInputs/DataClassWithImportOption", "D-1")); rowsToUpdate.add(CaseInsensitiveHashMap.of("name", "D-2", "prop", "b1", "DataInputs/DataClassWithImportOption", null, "alias", "Grassland")); + context = new DataIteratorContext(); context.setInsertOption(QueryUpdateService.InsertOption.UPDATE); count = qus.loadRows(user, c, MapDataIterator.of(rowsToUpdate), context, null); @@ -1128,6 +1129,7 @@ public void testInsertOptionUpdate() throws Exception rowsToMerge.add(CaseInsensitiveHashMap.of("name", "D-2", "prop", "gene", longFieldName, "Pasture", "alias", "Exceedingly", "flag", "cOne")); rowsToMerge.add(CaseInsensitiveHashMap.of("name", "D-22", "prop", null, longFieldName, "Goal", "alias", "Immensely", "flag", "cEight")); + context = new DataIteratorContext(); context.setInsertOption(QueryUpdateService.InsertOption.MERGE); count = qus.loadRows(user, c, MapDataIterator.of(rowsToMerge), context, null); @@ -1157,8 +1159,7 @@ public void testInsertOptionUpdate() throws Exception // long field assertEquals("Very", rows.get(0).get(longFieldAlias)); assertEquals("Long", rows.get(1).get(longFieldAlias)); - // TODO: This is not updating as I would expect. Verify what is expected here with regards to updating via merge. -// assertEquals("Pasture", rows.get(2).get(longFieldAlias)); + assertEquals("Pasture", rows.get(2).get(longFieldAlias)); assertEquals("Goal", rows.get(3).get(longFieldAlias)); // alias @@ -1172,7 +1173,6 @@ public void testInsertOptionUpdate() throws Exception assertEquals("c200", rows.get(1).get(flagAlias)); assertEquals("cOne", rows.get(2).get(flagAlias)); assertEquals("cEight", rows.get(3).get(flagAlias)); - } private @NotNull TableInfo getDataClassTable(String dataClassName) @@ -1226,7 +1226,9 @@ public void testUpdateAuditForLongField() throws Exception List> rowsToUpdate = new ArrayList<>(); rowsToUpdate.add(CaseInsensitiveHashMap.of("name", "A-1", fieldName, "Updated")); + context = new DataIteratorContext(); context.setInsertOption(QueryUpdateService.InsertOption.UPDATE); + context.getConfigParameters().put(DetailedAuditLogDataIterator.AuditConfigs.AuditBehavior, AuditBehaviorType.DETAILED); count = qus.loadRows(user, c, MapDataIterator.of(rowsToUpdate), context, null); assertFalse("Unexpected errors from update", context.getErrors().hasErrors()); assertEquals("Number of rows updated not as expected", 1, count); @@ -1241,7 +1243,5 @@ public void testUpdateAuditForLongField() throws Exception String oldRecordMap = ((DetailedAuditTypeEvent) events.get(0)).getOldRecordMap(); assertTrue("Old record map (" + oldRecordMap + ") does not contain expected field", oldRecordMap.contains(encodeURIComponent(fieldName.toLowerCase()) + "=Initial")); } - } - %> diff --git a/experiment/src/org/labkey/experiment/api/ExpSampleTypeTestCase.jsp b/experiment/src/org/labkey/experiment/api/ExpSampleTypeTestCase.jsp index 37c86a68589..fcd8981bddf 100644 --- a/experiment/src/org/labkey/experiment/api/ExpSampleTypeTestCase.jsp +++ b/experiment/src/org/labkey/experiment/api/ExpSampleTypeTestCase.jsp @@ -17,8 +17,6 @@ <%@ page import="org.apache.commons.lang3.StringUtils" %> <%@ page import="org.junit.After" %> <%@ page import="org.junit.Before" %> - - <%@ page import="org.junit.Test" %> <%@ page import="org.labkey.api.audit.AuditLogService" %> <%@ page import="org.labkey.api.audit.SampleTimelineAuditEvent" %> @@ -51,7 +49,6 @@ <%@ page import="org.labkey.api.exp.query.ExpSchema" %> <%@ page import="org.labkey.api.exp.query.SamplesSchema" %> <%@ page import="org.labkey.api.gwt.client.AuditBehaviorType" %> - <%@ page import="org.labkey.api.gwt.client.model.GWTPropertyDescriptor" %> <%@ page import="org.labkey.api.query.BatchValidationException" %> <%@ page import="org.labkey.api.query.DefaultSchema" %> @@ -59,7 +56,6 @@ <%@ page import="org.labkey.api.query.QuerySchema" %> <%@ page import="org.labkey.api.query.QueryService" %> <%@ page import="org.labkey.api.query.QueryUpdateService" %> - <%@ page import="static org.hamcrest.CoreMatchers.hasItems" %> <%@ page import="static org.junit.Assert.*" %> <%@ page import="org.labkey.api.query.SchemaKey" %> @@ -93,14 +89,9 @@ <%@ page import="java.util.concurrent.TimeUnit" %> <%@ page import="org.jetbrains.annotations.NotNull" %> <%@ page import="org.labkey.api.dataiterator.MapDataIterator" %> - <%@ page extends="org.labkey.api.jsp.JspTest.BVT" %> <%! -/** - * User: kevink - * Date: 11/24/16 - */ private static final String PROJECT_NAME = "_testSampleType"; private final ExpProvisionedTableTestHelper helper = new ExpProvisionedTableTestHelper(); @@ -1205,6 +1196,7 @@ public void testInsertOptionUpdate() throws Exception rowsToUpdate.add(CaseInsensitiveHashMap.of("name", "S-1-1", "intVal", null)); rowsToUpdate.add(CaseInsensitiveHashMap.of("name", "S-2", "intVal", 200)); + context = new DataIteratorContext(); context.setInsertOption(QueryUpdateService.InsertOption.UPDATE); count = qus.loadRows(user, c, MapDataIterator.of(rowsToUpdate), context, null); @@ -1238,6 +1230,8 @@ public void testInsertOptionUpdate() throws Exception // update a sample that doesn't exist should throw error rowsToUpdate = new ArrayList<>(); rowsToUpdate.add(CaseInsensitiveHashMap.of("name", "S-1-absent", "intVal", 100)); + context = new DataIteratorContext(); + context.setInsertOption(QueryUpdateService.InsertOption.UPDATE); qus.loadRows(user, c, MapDataIterator.of(rowsToUpdate), context, null); assertTrue(context.getErrors().hasErrors()); String msg = !context.getErrors().getRowErrors().isEmpty() ? context.getErrors().getRowErrors().get(0).toString() : "no message"; From be7bb01b8f462a742d4f25f71837a79cd71ca273 Mon Sep 17 00:00:00 2001 From: labkey-nicka Date: Wed, 9 Jul 2025 09:45:58 -0700 Subject: [PATCH 21/22] @NotNull --- .../labkey/experiment/api/SampleTypeUpdateServiceDI.java | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/experiment/src/org/labkey/experiment/api/SampleTypeUpdateServiceDI.java b/experiment/src/org/labkey/experiment/api/SampleTypeUpdateServiceDI.java index cbdd367f5dd..36edcf87a17 100644 --- a/experiment/src/org/labkey/experiment/api/SampleTypeUpdateServiceDI.java +++ b/experiment/src/org/labkey/experiment/api/SampleTypeUpdateServiceDI.java @@ -1623,7 +1623,14 @@ static class _GenerateNamesDataIterator extends SimpleTranslator Set _existingNames = null; String generatedName = null; - _GenerateNamesDataIterator(ExpSampleTypeImpl sampleType, Container container, User user, MapDataIterator source, DataIteratorContext context, int batchSize) + _GenerateNamesDataIterator( + @NotNull ExpSampleTypeImpl sampleType, + Container container, + User user, + MapDataIterator source, + DataIteratorContext context, + int batchSize + ) { super(source, context); _allowUserSpecifiedNames = NameExpressionOptionService.get().getAllowUserSpecificNamesValue(container); From f31da995032ac40a66acaa674d20b1d2ce8ec112 Mon Sep 17 00:00:00 2001 From: labkey-matthewb Date: Wed, 9 Jul 2025 10:47:25 -0700 Subject: [PATCH 22/22] Short variable names --- api/src/org/labkey/api/data/StatementUtils.java | 4 +++- api/src/org/labkey/api/query/AliasManager.java | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/api/src/org/labkey/api/data/StatementUtils.java b/api/src/org/labkey/api/data/StatementUtils.java index 35da912196f..55ed6e6d7a8 100644 --- a/api/src/org/labkey/api/data/StatementUtils.java +++ b/api/src/org/labkey/api/data/StatementUtils.java @@ -304,7 +304,9 @@ int getPrecision() private final static String pgRowVarPrefix = "$1."; private String makeVariableName(String name) { - return (_dialect.isSqlServer() ? "@p" + (parameters.size()+1) : pgRowVarPrefix) + AliasManager.makeLegalName(name, _dialect); + String shortName = StringUtils.substring(name,0,32); // name is just for readability, make it short + String uniquePrefix = (_dialect.isSqlServer() ? "@" : pgRowVarPrefix) + ("p" + (parameters.size()+1) + "_"); + return uniquePrefix + AliasManager.makeLegalName(shortName, _dialect, true, uniquePrefix.length()); } private String makePgRowTypeName(String variableName) diff --git a/api/src/org/labkey/api/query/AliasManager.java b/api/src/org/labkey/api/query/AliasManager.java index 3ea56113e71..857913f8552 100644 --- a/api/src/org/labkey/api/query/AliasManager.java +++ b/api/src/org/labkey/api/query/AliasManager.java @@ -67,7 +67,7 @@ public static String makeLegalName(String str, @NotNull SqlDialect dialect, bool return makeLegalName(str, dialect, truncate, 0); } - private static String makeLegalName(String str, @NotNull SqlDialect dialect, boolean truncate, int reserveCount) + public static String makeLegalName(String str, @NotNull SqlDialect dialect, boolean truncate, int reserveCount) { return dialect.makeLegalName(str, truncate, reserveCount); }