diff --git a/api/src/org/labkey/api/audit/AuditHandler.java b/api/src/org/labkey/api/audit/AuditHandler.java index 47eb678980b..a09036300f4 100644 --- a/api/src/org/labkey/api/audit/AuditHandler.java +++ b/api/src/org/labkey/api/audit/AuditHandler.java @@ -13,14 +13,15 @@ import org.labkey.api.exp.api.ExpMaterial; import org.labkey.api.exp.api.ExperimentService; import org.labkey.api.gwt.client.AuditBehaviorType; -import org.labkey.api.query.QueryKey; import org.labkey.api.query.QueryService; import org.labkey.api.security.User; import org.labkey.api.util.Pair; +import java.io.IOException; import java.text.NumberFormat; import java.text.ParseException; import java.text.SimpleDateFormat; +import java.util.Arrays; import java.util.Collections; import java.util.Date; import java.util.HashMap; @@ -28,6 +29,7 @@ import java.util.Map; import java.util.Objects; import java.util.Set; +import java.util.stream.Collectors; public interface AuditHandler @@ -104,7 +106,7 @@ static Pair, Map> getOldAndNewRecordForMerge } String lcName = nameFromAlias.toLowerCase(); // Preserve casing of inputs so we can show the names properly - boolean isExpInput = false; + boolean isExpInput = false; // TODO: extract lineage handling out of this generic method String encodedInputColumn = ExperimentService.getEncodedLineageKey(lcName); if (encodedInputColumn != null) { @@ -172,6 +174,21 @@ else if (!Objects.equals(oldValue, newValue) || isExtraAuditField) } else { + if (isExpInput && oldValue != null && newValue != null) + { + // For parent inputs, the order of the values does not matter, so compare as sets + try + { + Set oldSet = Arrays.stream(ExperimentService.getParentValues(oldValue.toString())).collect(Collectors.toSet()); + Set newSet = Arrays.stream(ExperimentService.getParentValues(newValue.toString())).collect(Collectors.toSet()); + if (oldSet.equals(newSet) && !isExtraAuditField) + continue; + } + catch (IOException ignore) + { + } + } + originalRow.put(nameFromAlias, oldValue); modifiedRow.put(nameFromAlias, newValue); } diff --git a/api/src/org/labkey/api/data/NameGenerator.java b/api/src/org/labkey/api/data/NameGenerator.java index aad531327cb..48030ff9467 100644 --- a/api/src/org/labkey/api/data/NameGenerator.java +++ b/api/src/org/labkey/api/data/NameGenerator.java @@ -745,26 +745,16 @@ public static Stream parentNames(Object value, String parentColName, TSV // using TabLoader instead of just splitting on the comma. boolean likelyAlreadyQuoted = valueStr.contains(",") || valueStr.contains("\n") || valueStr.contains("\r") || (valueStr.startsWith("\"") && valueStr.endsWith("\"")); String quotedStr = likelyAlreadyQuoted ? valueStr : tsvWriter.quoteValue(valueStr); // if value contains comma, no need to quote again - try (TabLoader tabLoader = new TabLoader(quotedStr)) - { - tabLoader.setDelimiterCharacter(','); - tabLoader.setUnescapeBackslashes(false); - // Issue 50924: LKSM: Importing samples using naming expression referencing parent inputs with # result in error - tabLoader.setIncludeComments(true); - // Issue 51056 Samples with single double quotes in the name will not resolve if added as parent samples. - tabLoader.setParseEnclosedQuotes(true); - try - { - String[][] parsedValues = tabLoader.getFirstNLines(1); - values = Arrays.stream(parsedValues[0]); - } - catch (IOException e) - { - if (errors != null) - errors.addRowError(new ValidationException("Unable to parse parent names from " + value, parentColName)); - else - throw new IllegalStateException("Unable to parse parent names from " + valueStr, e); - } + try + { + values = Arrays.stream(ExperimentService.getParentValues(quotedStr)); + } + catch (IOException e) + { + if (errors != null) + errors.addRowError(new ValidationException("Unable to parse parent names from " + value, parentColName)); + else + throw new IllegalStateException("Unable to parse parent names from " + valueStr, e); } } else if (value instanceof Collection coll) diff --git a/api/src/org/labkey/api/exp/api/ExperimentService.java b/api/src/org/labkey/api/exp/api/ExperimentService.java index 101bb909a1e..5c6a0141513 100644 --- a/api/src/org/labkey/api/exp/api/ExperimentService.java +++ b/api/src/org/labkey/api/exp/api/ExperimentService.java @@ -75,6 +75,7 @@ import org.labkey.api.query.QueryViewProvider; import org.labkey.api.query.UserSchema; import org.labkey.api.query.ValidationException; +import org.labkey.api.reader.TabLoader; import org.labkey.api.security.User; import org.labkey.api.services.ServiceRegistry; import org.labkey.api.util.Pair; @@ -521,6 +522,23 @@ static boolean isInputOutputColumn(String columnName) return null; } + static @NotNull String[] getParentValues(String valueStr) throws IOException + { + if (StringUtils.isEmpty(valueStr.trim())) + return new String[0]; + try (TabLoader tabLoader = new TabLoader(valueStr)) + { + tabLoader.setDelimiterCharacter(','); + tabLoader.setUnescapeBackslashes(false); + // Issue 50924: LKSM: Importing samples using naming expression referencing parent inputs with # result in error + tabLoader.setIncludeComments(true); + // Issue 51056 Samples with single double quotes in the name will not resolve if added as parent samples. + tabLoader.setParseEnclosedQuotes(true); + String[][] parsedValues = tabLoader.getFirstNLines(1); + return parsedValues[0]; + } + } + static Pair parseInputOutputAlias(String columnName) { if (!isInputOutputColumn(columnName)) @@ -593,33 +611,6 @@ static void validateParentAlias(Map aliasMap, Set reserv * ignoring any sample children derived from ExpData children. */ Set getRelatedChildSamples(Container c, User user, ExpData start); - - /** - * Find the ExpData objects, if any, that are parents of the start ExpMaterial. - */ - @NotNull - Set getParentDatas(Container c, User user, ExpMaterial start); - - /** - * Find the ExpMaterial objects, if any, that are parents of the start ExpMaterial. - */ - @NotNull - Set getParentMaterials(Container c, User user, ExpMaterial start); - - /** - * Find all parent ExpData that are parents of the start ExpMaterial, - * stopping at the first parent generation (no grandparents.) - */ - @NotNull - Set getNearestParentDatas(Container c, User user, ExpMaterial start); - - /** - * Find all parent ExpMaterial that are parents of the start ExpMaterial, - * stopping at the first parent generation (no grandparents.) - */ - @NotNull - Set getNearestParentMaterials(Container c, User user, ExpMaterial start); - /** * Get the lineage for the seed Identifiable object. Typically, the seed object is a ExpMaterial, * a ExpData (in a DataClass), or an ExpRun. diff --git a/experiment/src/org/labkey/experiment/api/ExpDataClassDataTableImpl.java b/experiment/src/org/labkey/experiment/api/ExpDataClassDataTableImpl.java index ddb2cafe159..d16adc98aaa 100644 --- a/experiment/src/org/labkey/experiment/api/ExpDataClassDataTableImpl.java +++ b/experiment/src/org/labkey/experiment/api/ExpDataClassDataTableImpl.java @@ -20,6 +20,7 @@ import org.apache.commons.collections4.MapUtils; import org.apache.commons.collections4.ListUtils; import org.apache.commons.lang3.StringUtils; +import org.apache.commons.lang3.Strings; import org.apache.logging.log4j.Level; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; @@ -1231,12 +1232,66 @@ public List> insertRows(User user, Container container, List return super._insertRowsUsingDIB(user, container, rows, getDataIteratorContext(errors, InsertOption.INSERT, configParameters), extraScriptContext); } + @Override + public Map> getExistingRows(User user, Container container, Map> keys, boolean verifyNoCrossFolderData, boolean verifyExisting, @Nullable Set columns) throws SQLException, QueryUpdateServiceException, InvalidKeyException + { + Map> dataRows = super.getExistingRows(user, container, keys, verifyNoCrossFolderData, verifyExisting, columns); + boolean hasParentInput = false; + if (_dataClass != null && columns != null && !columns.isEmpty()) + { + try + { + Map importAliases = _dataClass.getImportAliases(); + for (String col : columns) + { + if (ExperimentService.isInputOutputColumn(col) || Strings.CI.equals("parent",col) || importAliases.containsKey(col)) + { + hasParentInput = true; + break; + } + } + } + catch (IOException ignored) + { + } + } + + if (!hasParentInput) + return dataRows; + + Set lsids = new HashSet<>(); + for (Map dataRow : dataRows.values()) + lsids.add((String) dataRow.get("lsid")); + List seeds = ExperimentServiceImpl.get().getExpDatasByLSID(lsids); + + ExperimentServiceImpl.get().addRowsParentsFields(new HashSet<>(seeds), dataRows, user, container); + + return dataRows; + } + @Override protected Map getRow(User user, Container container, Map keys) throws InvalidKeyException, SQLException { return getRow(user, container, keys, false); } + @Override + public List> getRows(User user, Container container, List> keys) + throws SQLException + { + if (!hasPermission(user, ReadPermission.class)) + throw new UnauthorizedException("You do not have permission to read data from this table."); + + List> result = new ArrayList<>(keys.size()); + for (Map row : keys) + { + Map materialMap = getDataMap(row, user, container, true, false); + if (materialMap != null) + result.add(materialMap); + } + return result; + } + /* This class overrides getRow() in order to support getRow() using "rowid" or "lsid" */ @Override protected Map getRow(User user, Container container, Map keys, boolean allowCrossContainer) throws InvalidKeyException, SQLException @@ -1246,15 +1301,11 @@ protected Map getRow(User user, Container container, Map _select(Container container, Object[] keys) throws throw new IllegalStateException(); } - protected Map _select(Container container, Long rowId, String lsid, String name, Long classId, boolean allowCrossContainer) throws SQLException + @Nullable protected Map getDataMap(Map keys, User user, Container container, boolean allowCrossContainer, boolean addInputs) throws SQLException { - if (null == rowId && null == lsid && (null == name || null == classId)) + Long rowId = (Long)JdbcType.BIGINT.convert(keys.get(Column.RowId.name())); + String lsid = (String)JdbcType.VARCHAR.convert(keys.get(Column.LSID.name())); + String name = (String)JdbcType.VARCHAR.convert(keys.get(Name.name())); + Long classId = (Long)JdbcType.BIGINT.convert(keys.get(Column.ClassId.name())); + + if (classId == null) + classId = _dataClass.getRowId(); + + if (null == rowId && null == lsid && null == name) return null; // Issue 52886: Use queryTable here, not raw database table, so the rows are from the user schema with names @@ -1284,13 +1343,31 @@ else if (null != lsid) TableInfo queryTable = getQueryTable(); TableSelector selector = new TableSelector(queryTable, filter, null); + Map dataRow = null; try (var results = selector.getResults()) { if (results.next()) { - return FieldKeyRowMap.toNameMap(results.getFieldKeyRowMap()); + dataRow = FieldKeyRowMap.toNameMap(results.getFieldKeyRowMap()); } } - return null; + + if (!addInputs) + return dataRow; + + ExperimentService experimentService = ExperimentService.get(); + ExpData seed = null; + if (lsid != null && (rowId == null || rowId <= 0)) + seed = experimentService.getExpData(lsid); + else if (rowId != null && rowId > 0) + seed = experimentService.getExpData(_dataClass, rowId); + else if (name != null) + seed = experimentService.getExpData(_dataClass, name); + if (null == seed) + return dataRow; + + ExperimentServiceImpl.get().addParentsFields(seed, dataRow, user, container); + + return dataRow; } @Override diff --git a/experiment/src/org/labkey/experiment/api/ExperimentServiceImpl.java b/experiment/src/org/labkey/experiment/api/ExperimentServiceImpl.java index 5d8eeb3f056..d1441d648b5 100644 --- a/experiment/src/org/labkey/experiment/api/ExperimentServiceImpl.java +++ b/experiment/src/org/labkey/experiment/api/ExperimentServiceImpl.java @@ -2491,9 +2491,11 @@ public Set getRelatedChildSamples(Container c, User user, ExpData s return lineage.findRelatedChildSamples(start); } - @Override + /** + * Find the Identifiable objects, if any, that are parents of the start Identifiable. + */ @NotNull - public Set getParentDatas(Container c, User user, ExpMaterial start) + public Set getParentDatas(Container c, User user, Identifiable start) { ExpLineageOptions options = new ExpLineageOptions(); options.setChildren(false); @@ -2503,9 +2505,11 @@ public Set getParentDatas(Container c, User user, ExpMaterial start) return lineage.findNearestParentDatas(start); } - @Override + /** + * Find the Identifiable objects, if any, that are parents of the start Identifiable. + */ @NotNull - public Set getParentMaterials(Container c, User user, ExpMaterial start) + public Set getParentMaterials(Container c, User user, Identifiable start) { ExpLineageOptions options = new ExpLineageOptions(); options.setChildren(false); @@ -2515,6 +2519,73 @@ public Set getParentMaterials(Container c, User user, ExpMaterial s return lineage.findNearestParentMaterials(start); } + public void addParentsFields(T seed, Map dataRow, User user, Container container) + { + Set parentSamples = getParentMaterials(container, user, seed); + if (!parentSamples.isEmpty()) + addParentFields(dataRow, parentSamples, ExpMaterial.MATERIAL_INPUT_PARENT + "/", user); + Set parentDatas = getParentDatas(container, user, seed); + if (!parentDatas.isEmpty()) + addParentFields(dataRow, parentDatas, ExpData.DATA_INPUT_PARENT + "/", user); + } + + public void addParentFields(Map sampleRow, Set parents, String parentPrefix, User user) + { + Map> parentByType = new HashMap<>(); + for (ExpRunItem parent : parents) + { + String type = ""; + if (parent instanceof ExpData dataParent) + { + ExpDataClass dataClass = dataParent.getDataClass(user); + if (dataClass == null) + continue; + type = dataClass.getName(); + } + else if (parent instanceof ExpMaterial materialParent) + { + ExpSampleType sampleType = materialParent.getSampleType(); + if (sampleType == null) + continue; + type = sampleType.getName(); + } + + parentByType.computeIfAbsent(type, k -> new ArrayList<>()); + String parentName = parent.getName(); + if (parentName.contains(",")) + parentName = "\"" + parentName + "\""; + parentByType.get(type).add(parentName); + } + + for (String type : parentByType.keySet()) + { + String key = parentPrefix + type; + List parentValues = parentByType.get(type); + String value = String.join(",", parentValues); + sampleRow.put(key, value); + } + } + + public void addRowsParentsFields(Set seeds, Map> dataRows, User user, Container container) + { + Map, Set>> parents = ExperimentServiceImpl.get().getParentMaterialAndDataMap(container, user, seeds); + + for (Map dataRow : dataRows.values()) + { + String lsidKey = (String) dataRow.get("lsid"); + + if (!parents.containsKey(lsidKey)) + continue; + + Pair, Set> sampleParents = parents.get(lsidKey); + + if (!sampleParents.first.isEmpty()) + addParentFields(dataRow, sampleParents.first, ExpMaterial.MATERIAL_INPUT_PARENT + "/", user); + if (!sampleParents.second.isEmpty()) + addParentFields(dataRow, sampleParents.second, ExpData.DATA_INPUT_PARENT + "/", user); + } + } + public Map, Set>> getParentMaterialAndDataMap(Container c, User user, Set seeds) { Map, Set>> results = new HashMap<>(); @@ -2544,28 +2615,6 @@ else if (item instanceof ExpData dItem) return results; } - @Override - @NotNull - public Set getNearestParentDatas(Container c, User user, ExpMaterial start) - { - ExpLineageOptions options = new ExpLineageOptions(); - options.setChildren(false); - - ExpLineage lineage = getLineage(c, user, start, options); - return lineage.findNearestParentDatas(start); - } - - @Override - @NotNull - public Set getNearestParentMaterials(Container c, User user, ExpMaterial start) - { - ExpLineageOptions options = new ExpLineageOptions(); - options.setChildren(false); - - ExpLineage lineage = getLineage(c, user, start, options); - return lineage.findNearestParentMaterials(start); - } - // Get list of ExpRun LSIDs for the start Data or Material @Override public List collectRunsToInvestigate(ExpRunItem start, ExpLineageOptions options) diff --git a/experiment/src/org/labkey/experiment/api/SampleTypeUpdateServiceDI.java b/experiment/src/org/labkey/experiment/api/SampleTypeUpdateServiceDI.java index 43f0ea73d7d..92001fe2d5e 100644 --- a/experiment/src/org/labkey/experiment/api/SampleTypeUpdateServiceDI.java +++ b/experiment/src/org/labkey/experiment/api/SampleTypeUpdateServiceDI.java @@ -77,9 +77,7 @@ import org.labkey.api.exp.Lsid; import org.labkey.api.exp.PropertyType; import org.labkey.api.exp.api.ExpData; -import org.labkey.api.exp.api.ExpDataClass; import org.labkey.api.exp.api.ExpMaterial; -import org.labkey.api.exp.api.ExpRunItem; import org.labkey.api.exp.api.ExpSampleType; import org.labkey.api.exp.api.ExperimentService; import org.labkey.api.exp.api.NameExpressionOptionService; @@ -109,6 +107,7 @@ import org.labkey.api.search.SearchService; import org.labkey.api.security.User; import org.labkey.api.security.permissions.MoveEntitiesPermission; +import org.labkey.api.security.permissions.ReadPermission; import org.labkey.api.study.publish.StudyPublishService; import org.labkey.api.usageMetrics.SimpleMetricsService; import org.labkey.api.util.JobRunner; @@ -1162,57 +1161,10 @@ else if (lsid != null) ExpMaterial seed = rowId != null ? experimentService.getExpMaterial(rowId) : experimentService.getExpMaterial(lsid); if (null == seed) return sampleRow; - Set parentSamples = experimentService.getParentMaterials(container, user, seed); - if (!parentSamples.isEmpty()) - addParentFields(sampleRow, parentSamples, ExpMaterial.MATERIAL_INPUT_PARENT + "/", user); - Set parentDatas = experimentService.getParentDatas(container, user, seed); - if (!parentDatas.isEmpty()) - addParentFields(sampleRow, parentDatas, ExpData.DATA_INPUT_PARENT + "/", user); - return sampleRow; - } - - private void addParentFields(Map sampleRow, Set parents, String parentPrefix, User user) - { - Map> parentByType = new HashMap<>(); - for (ExpRunItem parent : parents) - { - String type = ""; - if (parent instanceof ExpData dataParent) - { - ExpDataClass dataClass = dataParent.getDataClass(user); - if (dataClass == null) - continue; - type = dataClass.getName(); - } - else if (parent instanceof ExpMaterial materialParent) - { - ExpSampleType sampleType = materialParent.getSampleType(); - if (sampleType == null) - continue; - type = sampleType.getName(); - } + ExperimentServiceImpl.get().addParentsFields(seed, sampleRow, user, container); - parentByType.computeIfAbsent(type, k -> new ArrayList<>()); - String parentName = parent.getName(); - if (parentName.contains(",")) - parentName = "\"" + parentName + "\""; - parentByType.get(type).add(parentName); - } - - for (String type : parentByType.keySet()) - { - String key = parentPrefix + type; - String value = String.join(",", parentByType.get(type)); - sampleRow.put(key, value); - } - } - - @Override - public List> getRows(User user, Container container, List> keys) - throws QueryUpdateServiceException - { - return getRows(user, container, keys, false /*skip addInputs for insertRows*/); + return sampleRow; } @Override @@ -1238,13 +1190,6 @@ public boolean hasExistingRowsInOtherContainers(Container container, Map> getExistingRows(User user, Container container, Map> keys, boolean verifyNoCrossFolderData, boolean verifyExisting, @Nullable Set columns) - throws InvalidKeyException, QueryUpdateServiceException - { - return getMaterialMapsWithInput(keys, user, container, verifyNoCrossFolderData, verifyExisting, columns); - } - private record ExistingRowSelect(TableInfo tableInfo, Set columns, boolean includeParent) {} private @NotNull ExistingRowSelect getExistingRowSelect(@Nullable Set dataColumns) @@ -1310,8 +1255,9 @@ else if (dataColumns.contains(remap.get(column.getColumnName()))) return new ExistingRowSelect(selectTable, includedColumns, hasParentInput); } - private Map> getMaterialMapsWithInput(Map> keys, User user, Container container, boolean checkCrossFolderData, boolean verifyExisting, @Nullable Set columns) - throws QueryUpdateServiceException, InvalidKeyException + @Override + public Map> getExistingRows(User user, Container container, Map> keys, boolean verifyNoCrossFolderData, boolean verifyExisting, @Nullable Set columns) + throws InvalidKeyException, QueryUpdateServiceException { ExistingRowSelect existingRowSelect = getExistingRowSelect(columns); TableInfo queryTableInfo = existingRowSelect.tableInfo; @@ -1405,7 +1351,7 @@ else if (name != null && materialSourceId != null) } } - if (checkCrossFolderData && !allKeys.isEmpty()) + if (verifyNoCrossFolderData && !allKeys.isEmpty()) { // 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 @@ -1445,37 +1391,27 @@ else if (name != null && materialSourceId != null) if (!existingRowSelect.includeParent) return sampleRows; - List materials = ExperimentServiceImpl.get().getExpMaterialsByLsid(rowNumLsid.values()); + Set lsids = new HashSet<>(); + for (Map sampleRow : sampleRows.values()) + lsids.add((String) sampleRow.get("lsid")); + List seeds = ExperimentServiceImpl.get().getExpMaterialsByLsid(lsids); - Map, Set>> parents = ExperimentServiceImpl.get().getParentMaterialAndDataMap(container, user, new HashSet<>(materials)); - - for (Map.Entry> rowNumSampleRow : sampleRows.entrySet()) - { - Integer rowNum = rowNumSampleRow.getKey(); - String lsidKey = rowNumLsid.get(rowNum); - Map sampleRow = rowNumSampleRow.getValue(); - - if (!parents.containsKey(lsidKey)) - continue; - - Pair, Set> sampleParents = parents.get(lsidKey); - - if (!sampleParents.first.isEmpty()) - addParentFields(sampleRow, sampleParents.first, ExpMaterial.MATERIAL_INPUT_PARENT + "/", user); - if (!sampleParents.second.isEmpty()) - addParentFields(sampleRow, sampleParents.second, ExpData.DATA_INPUT_PARENT + "/", user); - } + ExperimentServiceImpl.get().addRowsParentsFields(new HashSet<>(seeds), sampleRows, user, container); return sampleRows; } - public List> getRows(User user, Container container, List> keys, boolean addInputs) + @Override + public List> getRows(User user, Container container, List> keys) throws QueryUpdateServiceException { + if (!hasPermission(user, ReadPermission.class)) + throw new UnauthorizedException("You do not have permission to read data from this table."); + List> result = new ArrayList<>(keys.size()); for (Map k : keys) { - Map materialMap = getMaterialMap(getMaterialRowId(k), getMaterialLsid(k), user, container, addInputs); + Map materialMap = getMaterialMap(getMaterialRowId(k), getMaterialLsid(k), user, container, false); if (materialMap != null) result.add(materialMap); }