From 0a2aff13841581de59f86b7e660e64bd3d510a29 Mon Sep 17 00:00:00 2001 From: XingY Date: Tue, 15 Apr 2025 22:10:33 -0700 Subject: [PATCH 1/9] Issue 52728: LKSM: Update from file with duplicate rows is only updating once. --- .../ExistingRecordDataIterator.java | 21 ++++++++++++++++- .../api/query/AbstractQueryUpdateService.java | 8 +++++++ .../api/query/DefaultQueryUpdateService.java | 23 +++++++++++++++++++ .../labkey/experiment/ExpDataIterators.java | 9 ++++++-- 4 files changed, 58 insertions(+), 3 deletions(-) diff --git a/api/src/org/labkey/api/dataiterator/ExistingRecordDataIterator.java b/api/src/org/labkey/api/dataiterator/ExistingRecordDataIterator.java index f392a2ced84..a99da7986f9 100644 --- a/api/src/org/labkey/api/dataiterator/ExistingRecordDataIterator.java +++ b/api/src/org/labkey/api/dataiterator/ExistingRecordDataIterator.java @@ -35,6 +35,7 @@ import java.util.HashMap; import java.util.HashSet; import java.util.LinkedHashMap; +import java.util.List; import java.util.Map; import java.util.Set; import java.util.function.Supplier; @@ -64,6 +65,8 @@ public abstract class ExistingRecordDataIterator extends WrapperDataIterator final boolean _checkCrossFolderData; final boolean _verifyExisting; + final Set _pkKeysSeen = new HashSet<>(); + final DataIteratorContext _context; ExistingRecordDataIterator(DataIterator in, TableInfo target, @Nullable Set keys, boolean useMark, DataIteratorContext context, boolean detailed) @@ -255,12 +258,17 @@ private Pair> getSelectExistingSql(int rows) t sqlf.append(comma).append("(").appendValue(lastPrefetchRowNumber); comma = "\n,"; + List pkKeys = new ArrayList<>(); for (int p = 0; p < pkColumns.size(); p++) { sqlf.append(",?"); sqlf.add(pkSuppliers.get(p).get()); } sqlf.append(")"); + String pkKey = StringUtils.join(pkKeys, ", "); + if (_pkKeysSeen.contains(pkKey)) + _context.getErrors().addRowError(new ValidationException("Duplicate key provided: " + pkKey)); + _pkKeysSeen.add(pkKey); rowNumContainers.put(lastPrefetchRowNumber, container); } while (--rows > 0 && _delegate.next()); @@ -370,8 +378,19 @@ protected void prefetchExisting() throws BatchValidationException { lastPrefetchRowNumber = (Integer) _delegate.get(0); Map keyMap = CaseInsensitiveHashMap.of(); + List pkKeys = new ArrayList<>(); for (int p=0 ; p> updateRows(User user, Container container, List // TODO: Support update/delete without selecting the existing row -- unfortunately, we currently get the existing row to check its container matches the incoming container boolean streaming = false; //_queryTable.canStreamTriggers(container) && _queryTable.getAuditBehavior() != AuditBehaviorType.NONE; + Set updatedRows = new HashSet<>(); for (int i = 0; i < rows.size(); i++) { Map row = rows.get(i); @@ -805,6 +807,7 @@ public List> updateRows(User user, Container container, List { result.add(updatedRow); oldRows.add(oldRow); + checkDuplicateUpdate(updatedRows, updatedRow, container); } } catch (ValidationException vex) @@ -834,6 +837,11 @@ public List> updateRows(User user, Container container, List return result; } + protected void checkDuplicateUpdate(@NotNull Set updatedRows, @NotNull Map updatedRow, @NotNull Container container) throws InvalidKeyException + { + + } + @Override public Map moveRows(User user, Container container, Container targetContainer, List> rows, BatchValidationException errors, @Nullable Map configParameters, @Nullable Map extraScriptContext) throws InvalidKeyException, BatchValidationException, QueryUpdateServiceException, SQLException { diff --git a/api/src/org/labkey/api/query/DefaultQueryUpdateService.java b/api/src/org/labkey/api/query/DefaultQueryUpdateService.java index 0ec37705c81..3090c3aa8b1 100644 --- a/api/src/org/labkey/api/query/DefaultQueryUpdateService.java +++ b/api/src/org/labkey/api/query/DefaultQueryUpdateService.java @@ -17,6 +17,7 @@ import org.apache.commons.beanutils.ConversionException; import org.apache.commons.beanutils.ConvertUtils; +import org.apache.commons.lang3.StringUtils; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; import org.labkey.api.attachments.AttachmentFile; @@ -722,6 +723,28 @@ protected int truncateRows(User user, Container container) throws QueryUpdateSer return Table.delete(getDbTable()); } + @Override + protected void checkDuplicateUpdate(@NotNull Set updatedRows, @NotNull Map updatedRow, @NotNull Container container) throws InvalidKeyException + { + Object[] keysObj = getKeys(updatedRow, container); + if (keysObj == null) + return; + if (keysObj.length == 1) + { + if (updatedRows.contains(keysObj[0])) + throw new InvalidKeyException("Duplicate key provided: " + keysObj[0]); + updatedRows.add(keysObj[0]); + return; + } + + List keys = new ArrayList<>(); + for (Object key : keysObj) + keys.add(String.valueOf(key)); + if (updatedRows.contains(keys)) + throw new InvalidKeyException("Duplicate key provided: " + StringUtils.join(keys, ", ")); + updatedRows.add(keys); + } + protected Object[] getKeys(Map map, Container container) throws InvalidKeyException { //build an array of pk values based on the table info diff --git a/experiment/src/org/labkey/experiment/ExpDataIterators.java b/experiment/src/org/labkey/experiment/ExpDataIterators.java index 534a4b3b56c..041e2cad04a 100644 --- a/experiment/src/org/labkey/experiment/ExpDataIterators.java +++ b/experiment/src/org/labkey/experiment/ExpDataIterators.java @@ -145,6 +145,7 @@ import java.util.function.Function; import java.util.function.Supplier; import java.util.stream.Collectors; +import java.util.stream.IntStream; import java.util.stream.Stream; import static org.apache.commons.lang3.StringUtils.equalsIgnoreCase; @@ -3023,8 +3024,12 @@ private void writeRowsToFile(TypeData typeData) { String name = (String) row.get("name"); String dataContainer = (String) row.get("container"); - int dataRowId = typeData.dataIds.indexOf(name); - containerRows.computeIfAbsent(dataContainer, k -> new ArrayList<>()).add(dataRowId); + // could be updating the same data multiple times in a single import, the import will later be rejected + List dataRowIds = + IntStream.range(0, typeData.dataIds.size()).boxed() + .filter(i -> typeData.dataIds.get(i).equals(name)) + .toList(); + containerRows.computeIfAbsent(dataContainer, k -> new ArrayList<>()).addAll(dataRowIds); } for (String containerId : containerRows.keySet()) From 0bdb55107b6655e78960964f7b8306f753c96d2c Mon Sep 17 00:00:00 2001 From: XingY Date: Wed, 16 Apr 2025 09:31:27 -0700 Subject: [PATCH 2/9] fix --- .../labkey/api/dataiterator/ExistingRecordDataIterator.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/api/src/org/labkey/api/dataiterator/ExistingRecordDataIterator.java b/api/src/org/labkey/api/dataiterator/ExistingRecordDataIterator.java index a99da7986f9..827aca22b94 100644 --- a/api/src/org/labkey/api/dataiterator/ExistingRecordDataIterator.java +++ b/api/src/org/labkey/api/dataiterator/ExistingRecordDataIterator.java @@ -262,7 +262,9 @@ private Pair> getSelectExistingSql(int rows) t for (int p = 0; p < pkColumns.size(); p++) { sqlf.append(",?"); - sqlf.add(pkSuppliers.get(p).get()); + Object pkVal = pkSuppliers.get(p).get(); + sqlf.add(pkVal); + pkKeys.add(pkVal.toString()); } sqlf.append(")"); String pkKey = StringUtils.join(pkKeys, ", "); From 9cfe4bae725557f89e1140f0f19c3d87154e00b6 Mon Sep 17 00:00:00 2001 From: XingY Date: Thu, 17 Apr 2025 13:09:25 -0700 Subject: [PATCH 3/9] fix fail fast --- .../labkey/api/dataiterator/DataIteratorContext.java | 11 ++++++++--- .../api/dataiterator/ExistingRecordDataIterator.java | 2 +- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/api/src/org/labkey/api/dataiterator/DataIteratorContext.java b/api/src/org/labkey/api/dataiterator/DataIteratorContext.java index 3a3a71e9d14..90d2f28dbfc 100644 --- a/api/src/org/labkey/api/dataiterator/DataIteratorContext.java +++ b/api/src/org/labkey/api/dataiterator/DataIteratorContext.java @@ -238,12 +238,17 @@ public void addAlternateKeys(Set alternateKeys) _alternateKeys.addAll(alternateKeys); } + public boolean shouldCancel() + { + if (!_errors.hasErrors()) + return false; + return _failFast || _errors.getRowErrors().size() > _maxRowErrors; + } + /** if this etl should be killed, will execute throw _errors; */ public void checkShouldCancel() throws BatchValidationException { - if (!_errors.hasErrors()) - return; - if (_failFast || _errors.getRowErrors().size() > _maxRowErrors) + if (shouldCancel()) throw _errors; } diff --git a/api/src/org/labkey/api/dataiterator/ExistingRecordDataIterator.java b/api/src/org/labkey/api/dataiterator/ExistingRecordDataIterator.java index 827aca22b94..8315f01f7c8 100644 --- a/api/src/org/labkey/api/dataiterator/ExistingRecordDataIterator.java +++ b/api/src/org/labkey/api/dataiterator/ExistingRecordDataIterator.java @@ -173,7 +173,7 @@ public boolean next() throws BatchValidationException if (!_context.getErrors().hasErrors() && ret && !pkColumns.isEmpty()) { prefetchExisting(); - if (_context.getErrors().hasErrors()) + if (_context.shouldCancel()) return false; } return ret; From 98febdb72fd23b5ef3def16c9130584dd1886c48 Mon Sep 17 00:00:00 2001 From: XingY Date: Thu, 17 Apr 2025 15:37:23 -0700 Subject: [PATCH 4/9] check more data types --- .../query/AnnouncementSubscriptionTable.java | 4 +- .../query/ForumSubscriptionTable.java | 4 +- .../api/query/AbstractQueryUpdateService.java | 44 +++++++++++++++++-- .../api/query/DefaultQueryUpdateService.java | 24 +--------- .../experiment/api/ExpRunTableImpl.java | 1 + .../specimen/query/SpecimenUpdateService.java | 10 ++++- .../study/query/CohortUpdateService.java | 1 + .../study/query/DatasetUpdateService.java | 1 + 8 files changed, 61 insertions(+), 28 deletions(-) diff --git a/announcements/src/org/labkey/announcements/query/AnnouncementSubscriptionTable.java b/announcements/src/org/labkey/announcements/query/AnnouncementSubscriptionTable.java index 800ea3cb8c6..176d3c076c6 100644 --- a/announcements/src/org/labkey/announcements/query/AnnouncementSubscriptionTable.java +++ b/announcements/src/org/labkey/announcements/query/AnnouncementSubscriptionTable.java @@ -36,6 +36,7 @@ import org.labkey.api.query.LookupForeignKey; import org.labkey.api.query.QueryUpdateService; import org.labkey.api.query.QueryUpdateServiceException; +import org.labkey.api.query.ValidationException; import org.labkey.api.security.User; import org.labkey.api.util.Pair; @@ -179,7 +180,7 @@ private Map createDatabaseMap(Pair targ } @Override - protected Map updateRow(User user, Container container, Map row, @NotNull Map oldRow, @Nullable Map configParameters) throws InvalidKeyException + protected Map updateRow(User user, Container container, Map row, @NotNull Map oldRow, @Nullable Map configParameters) throws InvalidKeyException, ValidationException { Map existingRow = getRow(user, container, oldRow); if (existingRow != null) @@ -189,6 +190,7 @@ protected Map updateRow(User user, Container container, Map pks = new CaseInsensitiveHashMap<>(); pks.put("UserId", oldTargets.getKey().getUserId()); pks.put("MessageId", oldTargets.getValue().getRowId()); + checkDuplicateUpdate(pks); Table.update(user, getRealTable(), createDatabaseMap(newTargets), pks); } diff --git a/announcements/src/org/labkey/announcements/query/ForumSubscriptionTable.java b/announcements/src/org/labkey/announcements/query/ForumSubscriptionTable.java index 324dbf20e6c..1389c2f9112 100644 --- a/announcements/src/org/labkey/announcements/query/ForumSubscriptionTable.java +++ b/announcements/src/org/labkey/announcements/query/ForumSubscriptionTable.java @@ -35,6 +35,7 @@ import org.labkey.api.query.QueryUpdateService; import org.labkey.api.query.QueryUpdateServiceException; import org.labkey.api.query.UserIdQueryForeignKey; +import org.labkey.api.query.ValidationException; import org.labkey.api.security.User; import org.labkey.api.security.UserManager; import org.labkey.api.security.UserPrincipal; @@ -263,7 +264,7 @@ private Map createDatabaseMap(User user, Map row } @Override - protected Map updateRow(User user, Container container, Map row, @NotNull Map oldRow, @Nullable Map configParameters) throws InvalidKeyException + protected Map updateRow(User user, Container container, Map row, @NotNull Map oldRow, @Nullable Map configParameters) throws InvalidKeyException, ValidationException { Map existingRow = getRow(user, container, oldRow); if (existingRow != null) @@ -275,6 +276,7 @@ protected Map updateRow(User user, Container container, Map _previouslyUpdatedRows = new HashSet<>(); protected AbstractQueryUpdateService(TableInfo queryTable) { @@ -151,6 +154,11 @@ protected TableInfo getQueryTable() return _queryTable; } + public @NotNull Set getPreviouslyUpdatedRows() + { + return _previouslyUpdatedRows == null ? new HashSet<>() : _previouslyUpdatedRows; + } + @Override public boolean hasPermission(@NotNull UserPrincipal user, Class acl) { @@ -781,7 +789,6 @@ public List> updateRows(User user, Container container, List // TODO: Support update/delete without selecting the existing row -- unfortunately, we currently get the existing row to check its container matches the incoming container boolean streaming = false; //_queryTable.canStreamTriggers(container) && _queryTable.getAuditBehavior() != AuditBehaviorType.NONE; - Set updatedRows = new HashSet<>(); for (int i = 0; i < rows.size(); i++) { Map row = rows.get(i); @@ -807,7 +814,6 @@ public List> updateRows(User user, Container container, List { result.add(updatedRow); oldRows.add(oldRow); - checkDuplicateUpdate(updatedRows, updatedRow, container); } } catch (ValidationException vex) @@ -837,9 +843,41 @@ public List> updateRows(User user, Container container, List return result; } - protected void checkDuplicateUpdate(@NotNull Set updatedRows, @NotNull Map updatedRow, @NotNull Container container) throws InvalidKeyException + protected void checkDuplicateUpdate(Object pkVals) throws ValidationException { + if (pkVals == null) + return; + + Set updatedRows = getPreviouslyUpdatedRows(); + + Object[] keysObj; + if (pkVals.getClass().isArray()) + keysObj = (Object[]) pkVals; + else if (pkVals instanceof Map map) + { + List orderedKeyVals = new ArrayList<>(); + SortedSet sortedKeys = new TreeSet<>(map.keySet()); + for (String key : sortedKeys) + orderedKeyVals.add(map.get(key)); + keysObj = orderedKeyVals.toArray(); + } + else + keysObj = new Object[]{pkVals}; + + if (keysObj.length == 1) + { + if (updatedRows.contains(keysObj[0])) + throw new ValidationException("Duplicate key provided: " + keysObj[0]); + updatedRows.add(keysObj[0]); + return; + } + List keys = new ArrayList<>(); + for (Object key : keysObj) + keys.add(String.valueOf(key)); + if (updatedRows.contains(keys)) + throw new ValidationException("Duplicate key provided: " + StringUtils.join(keys, ", ")); + updatedRows.add(keys); } @Override diff --git a/api/src/org/labkey/api/query/DefaultQueryUpdateService.java b/api/src/org/labkey/api/query/DefaultQueryUpdateService.java index 3090c3aa8b1..fa6873f42ed 100644 --- a/api/src/org/labkey/api/query/DefaultQueryUpdateService.java +++ b/api/src/org/labkey/api/query/DefaultQueryUpdateService.java @@ -588,6 +588,8 @@ protected Map _update(User user, Container c, Map updatedRows, @NotNull Map updatedRow, @NotNull Container container) throws InvalidKeyException - { - Object[] keysObj = getKeys(updatedRow, container); - if (keysObj == null) - return; - if (keysObj.length == 1) - { - if (updatedRows.contains(keysObj[0])) - throw new InvalidKeyException("Duplicate key provided: " + keysObj[0]); - updatedRows.add(keysObj[0]); - return; - } - - List keys = new ArrayList<>(); - for (Object key : keysObj) - keys.add(String.valueOf(key)); - if (updatedRows.contains(keys)) - throw new InvalidKeyException("Duplicate key provided: " + StringUtils.join(keys, ", ")); - updatedRows.add(keys); - } - protected Object[] getKeys(Map map, Container container) throws InvalidKeyException { //build an array of pk values based on the table info diff --git a/experiment/src/org/labkey/experiment/api/ExpRunTableImpl.java b/experiment/src/org/labkey/experiment/api/ExpRunTableImpl.java index 5c17686677f..fc5e86aa57a 100644 --- a/experiment/src/org/labkey/experiment/api/ExpRunTableImpl.java +++ b/experiment/src/org/labkey/experiment/api/ExpRunTableImpl.java @@ -1066,6 +1066,7 @@ else if (Column.WorkflowTask.toString().equalsIgnoreCase(columnName)) } } + checkDuplicateUpdate(run.getRowId()); run.save(user); String auditUserComment = configParameters == null ? null : (String) configParameters.get(DetailedAuditLogDataIterator.AuditConfigs.AuditUserComment); diff --git a/specimen/src/org/labkey/specimen/query/SpecimenUpdateService.java b/specimen/src/org/labkey/specimen/query/SpecimenUpdateService.java index 7d3f968266f..56434fcd967 100644 --- a/specimen/src/org/labkey/specimen/query/SpecimenUpdateService.java +++ b/specimen/src/org/labkey/specimen/query/SpecimenUpdateService.java @@ -325,7 +325,7 @@ protected Map insertRow(User user, Container container, Map> updateRows(User user, Container container, List> rows, List> oldKeys, BatchValidationException errors, @Nullable Map configParameters, Map extraScriptContext) - throws InvalidKeyException, BatchValidationException, QueryUpdateServiceException, SQLException + throws InvalidKeyException, BatchValidationException, QueryUpdateServiceException, SQLException // TODO { if (oldKeys != null && rows.size() != oldKeys.size()) throw new IllegalArgumentException("rows and oldKeys are required to be the same length, but were " + rows.size() + " and " + oldKeys + " in length, respectively"); @@ -340,6 +340,14 @@ public List> updateRows(User user, Container container, List Map row = rows.get(i); Map keys = oldKeys == null ? row : oldKeys.get(i); long rowId = keyFromMap(keys); + try + { + checkDuplicateUpdate(rowId); + } + catch (ValidationException e) + { + throw new BatchValidationException(e); + } rowIds.add(rowId); uniqueRows.put(rowId, row); } diff --git a/study/src/org/labkey/study/query/CohortUpdateService.java b/study/src/org/labkey/study/query/CohortUpdateService.java index 1d0312578cf..6d3c084826b 100644 --- a/study/src/org/labkey/study/query/CohortUpdateService.java +++ b/study/src/org/labkey/study/query/CohortUpdateService.java @@ -114,6 +114,7 @@ protected Map updateRow(User user, Container container, Map updateRow(User user, Container container, Map Date: Mon, 21 Apr 2025 09:38:32 -0700 Subject: [PATCH 5/9] code review changes --- .../dataiterator/ExistingRecordDataIterator.java | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/api/src/org/labkey/api/dataiterator/ExistingRecordDataIterator.java b/api/src/org/labkey/api/dataiterator/ExistingRecordDataIterator.java index 8315f01f7c8..aad6b5fad29 100644 --- a/api/src/org/labkey/api/dataiterator/ExistingRecordDataIterator.java +++ b/api/src/org/labkey/api/dataiterator/ExistingRecordDataIterator.java @@ -65,7 +65,7 @@ public abstract class ExistingRecordDataIterator extends WrapperDataIterator final boolean _checkCrossFolderData; final boolean _verifyExisting; - final Set _pkKeysSeen = new HashSet<>(); + final Set _pkKeysSeen = new HashSet<>(); final DataIteratorContext _context; @@ -267,10 +267,9 @@ private Pair> getSelectExistingSql(int rows) t pkKeys.add(pkVal.toString()); } sqlf.append(")"); - String pkKey = StringUtils.join(pkKeys, ", "); - if (_pkKeysSeen.contains(pkKey)) - _context.getErrors().addRowError(new ValidationException("Duplicate key provided: " + pkKey)); - _pkKeysSeen.add(pkKey); + if (_pkKeysSeen.contains(pkKeys)) + _context.getErrors().addRowError(new ValidationException("Duplicate key provided: " + StringUtils.join(pkKeys, ", "))); + _pkKeysSeen.add(pkKeys); rowNumContainers.put(lastPrefetchRowNumber, container); } while (--rows > 0 && _delegate.next()); @@ -388,10 +387,9 @@ protected void prefetchExisting() throws BatchValidationException pkKeys.add(pkVal.toString()); } - String pkKey = StringUtils.join(pkKeys, ", "); - if (_pkKeysSeen.contains(pkKey)) - _context.getErrors().addRowError(new ValidationException("Duplicate key provided: " + pkKey)); - _pkKeysSeen.add(pkKey); + if (_pkKeysSeen.contains(pkKeys)) + _context.getErrors().addRowError(new ValidationException("Duplicate key provided: " + StringUtils.join(pkKeys, ", "))); + _pkKeysSeen.add(pkKeys); keysMap.put(lastPrefetchRowNumber, keyMap); existingRecords.put(lastPrefetchRowNumber, Map.of()); From e73a72be9d0f92fd1c5a9e1ed1567cc85e74f7b6 Mon Sep 17 00:00:00 2001 From: XingY Date: Mon, 21 Apr 2025 12:48:44 -0700 Subject: [PATCH 6/9] improve error msg --- .../ExistingRecordDataIterator.java | 45 +++++--- .../TriggerDataBuilderHelper.java | 2 +- .../test/integration/DataClassCrud.ispec.ts | 108 +++++++++++++++++- .../labkey/experiment/ExpDataIterators.java | 3 +- 4 files changed, 136 insertions(+), 22 deletions(-) diff --git a/api/src/org/labkey/api/dataiterator/ExistingRecordDataIterator.java b/api/src/org/labkey/api/dataiterator/ExistingRecordDataIterator.java index aad6b5fad29..f1a4328e0fb 100644 --- a/api/src/org/labkey/api/dataiterator/ExistingRecordDataIterator.java +++ b/api/src/org/labkey/api/dataiterator/ExistingRecordDataIterator.java @@ -65,11 +65,12 @@ public abstract class ExistingRecordDataIterator extends WrapperDataIterator final boolean _checkCrossFolderData; final boolean _verifyExisting; + final Set _sharedKeys = new CaseInsensitiveHashSet(); // common keys, such as data.classId, material.MaterialSourceId final Set _pkKeysSeen = new HashSet<>(); final DataIteratorContext _context; - ExistingRecordDataIterator(DataIterator in, TableInfo target, @Nullable Set keys, boolean useMark, DataIteratorContext context, boolean detailed) + ExistingRecordDataIterator(DataIterator in, TableInfo target, @Nullable Set keys, @Nullable Set sharedKeys, boolean useMark, DataIteratorContext context, boolean detailed) { super(in); _context = context; @@ -94,6 +95,9 @@ public abstract class ExistingRecordDataIterator extends WrapperDataIterator Collection keyNames = null==keys ? target.getPkColumnNames() : keys; + if (sharedKeys != null) + _sharedKeys.addAll(sharedKeys); + _dataColumnNames.addAll(detailed ? map.keySet() : keyNames); for (String name : keyNames) @@ -179,6 +183,13 @@ public boolean next() throws BatchValidationException return ret; } + protected void checkDuplicateKeys(List pkKeys) + { + Object pkKeysObj = pkKeys.size() == 1 ? pkKeys.get(0) : pkKeys; + if (_pkKeysSeen.contains(pkKeysObj)) + _context.getErrors().addRowError(new ValidationException("Duplicate key provided: " + StringUtils.join(pkKeys, ", "))); + _pkKeysSeen.add(pkKeysObj); + } @Override public boolean supportsGetExistingRecord() @@ -197,10 +208,10 @@ public Map getExistingRecord() public static DataIteratorBuilder createBuilder(DataIteratorBuilder dib, TableInfo target, @Nullable Set keys) { - return createBuilder(dib, target, keys, false); + return createBuilder(dib, target, keys, null, false); } - public static DataIteratorBuilder createBuilder(DataIteratorBuilder dib, TableInfo target, @Nullable Set keys, boolean useGetRows) + public static DataIteratorBuilder createBuilder(DataIteratorBuilder dib, TableInfo target, @Nullable Set keys, @Nullable Set sharedKeys, boolean useGetRows) { return context -> { @@ -218,9 +229,9 @@ public static DataIteratorBuilder createBuilder(DataIteratorBuilder dib, TableIn auditType = target.getAuditBehavior((AuditBehaviorType) context.getConfigParameter(DetailedAuditLogDataIterator.AuditConfigs.AuditBehavior)); boolean detailed = auditType == DETAILED; if (useGetRows) - return new ExistingDataIteratorsGetRows(new CachingDataIterator(di), target, keys, context, detailed); + return new ExistingDataIteratorsGetRows(new CachingDataIterator(di), target, keys, sharedKeys, context, detailed); else - return new ExistingDataIteratorsTableInfo(new CachingDataIterator(di), target, keys, context, detailed); + return new ExistingDataIteratorsTableInfo(new CachingDataIterator(di), target, keys, sharedKeys, context, detailed); } return di; }; @@ -232,9 +243,9 @@ static class ExistingDataIteratorsTableInfo extends ExistingRecordDataIterator { final Set allowedContainers = new HashSet<>(); - ExistingDataIteratorsTableInfo(CachingDataIterator in, TableInfo target, @Nullable Set keys, DataIteratorContext context, boolean detailed) + ExistingDataIteratorsTableInfo(CachingDataIterator in, TableInfo target, @Nullable Set keys, @Nullable Set sharedKeys, DataIteratorContext context, boolean detailed) { - super(in, target, keys, true, context, detailed); + super(in, target, keys, sharedKeys, true, context, detailed); if (c != null) allowedContainers.add(c.getId()); } @@ -264,12 +275,11 @@ private Pair> getSelectExistingSql(int rows) t sqlf.append(",?"); Object pkVal = pkSuppliers.get(p).get(); sqlf.add(pkVal); - pkKeys.add(pkVal.toString()); + if (!_sharedKeys.contains(pkColumns.get(p).getColumnName())) + pkKeys.add(pkVal.toString()); } sqlf.append(")"); - if (_pkKeysSeen.contains(pkKeys)) - _context.getErrors().addRowError(new ValidationException("Duplicate key provided: " + StringUtils.join(pkKeys, ", "))); - _pkKeysSeen.add(pkKeys); + checkDuplicateKeys(pkKeys); rowNumContainers.put(lastPrefetchRowNumber, container); } while (--rows > 0 && _delegate.next()); @@ -356,9 +366,9 @@ static class ExistingDataIteratorsGetRows extends ExistingRecordDataIterator { final QueryUpdateService qus; - ExistingDataIteratorsGetRows(CachingDataIterator in, TableInfo target, @Nullable Set keys, DataIteratorContext context, boolean detailed) + ExistingDataIteratorsGetRows(CachingDataIterator in, TableInfo target, @Nullable Set keys, @Nullable Set sharedKeys, DataIteratorContext context, boolean detailed) { - super(in, target, keys, true, context, detailed); + super(in, target, keys, sharedKeys, true, context, detailed); qus = target.getUpdateService(); } @@ -384,12 +394,11 @@ protected void prefetchExisting() throws BatchValidationException { Object pkVal = pkSuppliers.get(p).get(); keyMap.put(pkColumns.get(p).getColumnName(), pkVal); - pkKeys.add(pkVal.toString()); + if (!_sharedKeys.contains(pkColumns.get(p).getColumnName())) + pkKeys.add(pkVal.toString()); } - if (_pkKeysSeen.contains(pkKeys)) - _context.getErrors().addRowError(new ValidationException("Duplicate key provided: " + StringUtils.join(pkKeys, ", "))); - _pkKeysSeen.add(pkKeys); + checkDuplicateKeys(pkKeys); keysMap.put(lastPrefetchRowNumber, keyMap); existingRecords.put(lastPrefetchRowNumber, Map.of()); @@ -438,7 +447,7 @@ private DataIterator makeModulesDI() assertFalse(di.supportsGetExistingRecord()); var context = new DataIteratorContext(); context.setInsertOption(QueryUpdateService.InsertOption.INSERT); - DataIterator existing = new ExistingDataIteratorsTableInfo(new CachingDataIterator(di), CoreSchema.getInstance().getTableInfoModules(), null, context, true); + DataIterator existing = new ExistingDataIteratorsTableInfo(new CachingDataIterator(di), CoreSchema.getInstance().getTableInfoModules(), null, null, context, true); assertTrue(existing.supportsGetExistingRecord()); return existing; } diff --git a/api/src/org/labkey/api/dataiterator/TriggerDataBuilderHelper.java b/api/src/org/labkey/api/dataiterator/TriggerDataBuilderHelper.java index 98edee8e354..9ee30d5c45b 100644 --- a/api/src/org/labkey/api/dataiterator/TriggerDataBuilderHelper.java +++ b/api/src/org/labkey/api/dataiterator/TriggerDataBuilderHelper.java @@ -152,7 +152,7 @@ public DataIterator getDataIterator(DataIteratorContext context) else if (context.getInsertOption().mergeRows && !_target.supportsInsertOption(QueryUpdateService.InsertOption.MERGE)) return LoggingDataIterator.wrap(new BeforeIterator(coerce, context)); - coerce = ExistingRecordDataIterator.createBuilder(coerce, _target, mergeKeys, true).getDataIterator(context); + coerce = ExistingRecordDataIterator.createBuilder(coerce, _target, mergeKeys, null, true).getDataIterator(context); return LoggingDataIterator.wrap(new BeforeIterator(new CachingDataIterator(coerce), context)); } } diff --git a/experiment/src/client/test/integration/DataClassCrud.ispec.ts b/experiment/src/client/test/integration/DataClassCrud.ispec.ts index c244332b8c4..5d9cbf032e9 100644 --- a/experiment/src/client/test/integration/DataClassCrud.ispec.ts +++ b/experiment/src/client/test/integration/DataClassCrud.ispec.ts @@ -1,4 +1,4 @@ -import { hookServer, RequestOptions, successfulResponse } from '@labkey/test'; +import { ExperimentCRUDUtils, hookServer, RequestOptions, successfulResponse } from '@labkey/test'; import mock from 'mock-fs'; import { checkDomainName, @@ -9,7 +9,7 @@ import { initProject, verifyRequiredLineageInsertUpdate, } from './utils'; -import { DATA_CLASS_DESIGNER_ROLE } from '@labkey/components'; +import { caseInsensitive, DATA_CLASS_DESIGNER_ROLE } from '@labkey/components'; const server = hookServer(process.env); const PROJECT_NAME = 'DataClassCrudJestProject'; @@ -235,3 +235,107 @@ describe('Data Class - Required Lineage', () => { }); }); + +describe('CRUD actions', () => { + + it("Issue 52728: don't allow updating the same data twice", async () => { + const dataType = "TestIssue52728"; + const createPayload = { + kind: 'DataClass', + domainDesign: { name: dataType, fields: [{ name: 'Prop' }] }, + options: { + name: dataType, + } + }; + + await server.post('property', 'createDomain', createPayload, + {...topFolderOptions, ...designerReaderOptions}).expect(successfulResponse); + + let errorResp; + await server.post('query', 'insertRows', { + schemaName: 'exp.data', + queryName: dataType, + rows: [{ + name: 'duplicateShouldFail', + },{ + name: 'duplicateShouldFail', + }] + }, { ...topFolderOptions, ...editorUserOptions }).expect((result) => { + errorResp = JSON.parse(result.text); + expect(errorResp.exception.indexOf('duplicate key') > -1).toBeTruthy(); + }); + // import + errorResp = await ExperimentCRUDUtils.importData(server, "Name\tDescription\nduplicateShouldFail\tbad\nduplicateShouldFail\tbad", dataType, "IMPORT", topFolderOptions, editorUserOptions); + expect(errorResp.text.indexOf('duplicate key') > -1).toBeTruthy(); + + // merge + const duplicateKeyErrorPrefix = 'Duplicate key provided: '; + errorResp = await ExperimentCRUDUtils.importData(server, "Name\tDescription\nduplicateShouldFail\tbad\nduplicateShouldFail\tbad", dataType, "MERGE", topFolderOptions, editorUserOptions); + expect(errorResp.text.indexOf(duplicateKeyErrorPrefix + 'duplicateShouldFail') > -1).toBeTruthy(); + + const dataName1 = "up-dataId-1"; + const dataName2 = "up-dataId-2"; + const dataRows = await ExperimentCRUDUtils.insertRows(server, [{ + name: dataName1, + description: 'created' + },{ + name: dataName2, + description: 'created' + }], 'exp.data', dataType, topFolderOptions, editorUserOptions); + + const data1RowId = caseInsensitive(dataRows[0], 'rowId'); + const data1Lsid = caseInsensitive(dataRows[0], 'lsid'); + const data2RowId = caseInsensitive(dataRows[1], 'rowId'); + const data2Lsid = caseInsensitive(dataRows[1], 'lsid'); + + // update data2 twice using updateRows, using rowId + await server.post('query', 'updateRows', { + schemaName: 'exp.data', + queryName: dataType, + rows: [{ + description: 'update', + rowId: data1RowId + },{ + description: 'update', + rowId: data2RowId + },{ + description: 'update', + rowId: data2RowId + }] + }, { ...topFolderOptions, ...editorUserOptions }).expect((result) => { + errorResp = JSON.parse(result.text); + expect(errorResp['exception']).toBe('Duplicate key provided: ' + data2RowId); + }); + + // update data2 twice using updateRows, using lsid (data iterator) + await server.post('query', 'updateRows', { + schemaName: 'exp.data', + queryName: dataType, + rows: [{ + description: 'update', + lsid: data1Lsid + },{ + description: 'update', + lsid: data2Lsid + },{ + description: 'update', + lsid: data2Lsid + }] + }, { ...topFolderOptions, ...editorUserOptions }).expect((result) => { + errorResp = JSON.parse(result.text); + expect(errorResp['exception']).toBe('Duplicate key provided: ' + data2Lsid); + }); + + errorResp = await ExperimentCRUDUtils.importData(server, "Name\tDescription\n" + dataName1 + "\tupdate\n" + dataName2 + "\tupdate\n" + dataName2 + "\tupdate", dataType, "UPDATE", topFolderOptions, editorUserOptions); + expect(errorResp.text.indexOf('Duplicate key provided: ' + dataName2) > -1).toBeTruthy(); + + errorResp = await ExperimentCRUDUtils.importData(server, "Name\tDescription\n" + dataName1 + "\tupdate\n" + dataName2 + "\tupdate\n" + dataName2 + "\tupdate", dataType, "MERGE", topFolderOptions, editorUserOptions); + expect(errorResp.text.indexOf('Duplicate key provided: ' + dataName2) > -1).toBeTruthy(); + + // confirm rows are not updated + let dataResults = await ExperimentCRUDUtils.getRows(server, [data1RowId, data2RowId], 'exp.data', dataType, 'rowId,description', topFolderOptions, adminOptions); + expect(caseInsensitive(dataResults[0], 'description')).toBe('created'); + expect(caseInsensitive(dataResults[1], 'description')).toBe('created'); + + }); +}); \ No newline at end of file diff --git a/experiment/src/org/labkey/experiment/ExpDataIterators.java b/experiment/src/org/labkey/experiment/ExpDataIterators.java index c13996fb115..4c22b021fd4 100644 --- a/experiment/src/org/labkey/experiment/ExpDataIterators.java +++ b/experiment/src/org/labkey/experiment/ExpDataIterators.java @@ -78,6 +78,7 @@ import org.labkey.api.exp.property.PropertyService; import org.labkey.api.exp.query.AbstractExpSchema; import org.labkey.api.exp.query.DataClassUserSchema; +import org.labkey.api.exp.query.ExpDataClassDataTable; import org.labkey.api.exp.query.ExpDataTable; import org.labkey.api.exp.query.ExpMaterialTable; import org.labkey.api.exp.query.ExpSchema; @@ -2351,7 +2352,7 @@ else if (isMergeOrUpdate) // Since we support detailed audit logging add the ExistingRecordDataIterator here just before TableInsertDataIterator // this is a NOOP unless we are merging/updating and detailed logging is enabled - DataIteratorBuilder step2a = ExistingRecordDataIterator.createBuilder(step1, _expTable, keyColumns, true); + DataIteratorBuilder step2a = ExistingRecordDataIterator.createBuilder(step1, _expTable, keyColumns, Set.of(ExpMaterialTable.Column.MaterialSourceId.name(), ExpDataClassDataTable.Column.ClassId.name()), true); // Add RootMaterialRowId if it does not exist DataIteratorBuilder step2b = ctx -> { From d804586392ca05dd45fc435812dc608e77fba959 Mon Sep 17 00:00:00 2001 From: XingY Date: Mon, 21 Apr 2025 15:16:38 -0700 Subject: [PATCH 7/9] juit test for list --- .../api/query/AbstractQueryUpdateService.java | 38 +++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/api/src/org/labkey/api/query/AbstractQueryUpdateService.java b/api/src/org/labkey/api/query/AbstractQueryUpdateService.java index ec9dd9f7278..8b14f1c6878 100644 --- a/api/src/org/labkey/api/query/AbstractQueryUpdateService.java +++ b/api/src/org/labkey/api/query/AbstractQueryUpdateService.java @@ -1334,6 +1334,15 @@ public void addRowError(ValidationException vex) // test new row assertEquals("THREE", rows.get(3).get("s")); assertNull(rows.get(3).get("i")); + + // merge should fail if duplicate keys are provided + errors = new BatchValidationException(); + mergeRows = new ArrayList>(); + mergeRows.add(CaseInsensitiveHashMap.of(pkName,2,colName,"TWO-UP-2")); + mergeRows.add(CaseInsensitiveHashMap.of(pkName,2,colName,"TWO-UP-UP-2")); + qus.mergeRows(user, c, MapDataIterator.of(mergeRows.get(0).keySet(), mergeRows), errors, null, null); + assertTrue(errors.hasErrors()); + assertTrue("Duplicate key error: " + errors.getMessage(), errors.getMessage().contains("Duplicate key provided: 2")); } @Test @@ -1371,6 +1380,35 @@ public void UPDATE() throws Exception updateRows.add(CaseInsensitiveHashMap.of(pkName,2,colName,"TWO-UP-2")); count = qus.loadRows(user, c, MapDataIterator.of(updateRows.get(0).keySet(), updateRows), context, null); assertTrue(context.getErrors().hasErrors()); + + // Issue 52728: update should fail if duplicate key is provide + updateRows = new ArrayList>(); + updateRows.add(CaseInsensitiveHashMap.of(pkName,2,colName,"TWO-UP-2")); + updateRows.add(CaseInsensitiveHashMap.of(pkName,2,colName,"TWO-UP-UP-2")); + + // use DIB + context = new DataIteratorContext(); + context.setInsertOption(InsertOption.UPDATE); + qus.loadRows(user, c, MapDataIterator.of(updateRows.get(0).keySet(), updateRows), context, null); + assertTrue(context.getErrors().hasErrors()); + assertTrue("Duplicate key error: " + context.getErrors().getMessage(), context.getErrors().getMessage().contains("Duplicate key provided: 2")); + + // use updateRows + if (!_useAlias) // _update using alias is not supported + { + BatchValidationException errors = new BatchValidationException(); + try + { + qus.updateRows(user, c, updateRows, null, errors, null, null); + } + catch (Exception e) + { + + } + assertTrue(errors.hasErrors()); + assertTrue("Duplicate key error: " + errors.getMessage(), errors.getMessage().contains("Duplicate key provided: 2")); + + } } @Test From 4a6ea2e079325cff6a62d1b0e3860f094ec4f726 Mon Sep 17 00:00:00 2001 From: XingY Date: Mon, 21 Apr 2025 22:10:57 -0700 Subject: [PATCH 8/9] code review changes --- experiment/src/client/test/integration/DataClassCrud.ispec.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/experiment/src/client/test/integration/DataClassCrud.ispec.ts b/experiment/src/client/test/integration/DataClassCrud.ispec.ts index 5d9cbf032e9..8ca4164a0a0 100644 --- a/experiment/src/client/test/integration/DataClassCrud.ispec.ts +++ b/experiment/src/client/test/integration/DataClassCrud.ispec.ts @@ -236,7 +236,7 @@ describe('Data Class - Required Lineage', () => { }); -describe('CRUD actions', () => { +describe('Duplicate IDs', () => { it("Issue 52728: don't allow updating the same data twice", async () => { const dataType = "TestIssue52728"; From 6ff6832971e2636064879cdea02bbc2dc7945bac Mon Sep 17 00:00:00 2001 From: XingY Date: Tue, 22 Apr 2025 10:04:48 -0700 Subject: [PATCH 9/9] clean --- .../src/org/labkey/specimen/query/SpecimenUpdateService.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/specimen/src/org/labkey/specimen/query/SpecimenUpdateService.java b/specimen/src/org/labkey/specimen/query/SpecimenUpdateService.java index 56434fcd967..d7285c9a458 100644 --- a/specimen/src/org/labkey/specimen/query/SpecimenUpdateService.java +++ b/specimen/src/org/labkey/specimen/query/SpecimenUpdateService.java @@ -325,7 +325,7 @@ protected Map insertRow(User user, Container container, Map> updateRows(User user, Container container, List> rows, List> oldKeys, BatchValidationException errors, @Nullable Map configParameters, Map extraScriptContext) - throws InvalidKeyException, BatchValidationException, QueryUpdateServiceException, SQLException // TODO + throws InvalidKeyException, BatchValidationException, QueryUpdateServiceException, SQLException { if (oldKeys != null && rows.size() != oldKeys.size()) throw new IllegalArgumentException("rows and oldKeys are required to be the same length, but were " + rows.size() + " and " + oldKeys + " in length, respectively");