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 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 f392a2ced84..f1a4328e0fb 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,9 +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; @@ -91,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) @@ -170,12 +177,19 @@ public boolean next() throws BatchValidationException if (!_context.getErrors().hasErrors() && ret && !pkColumns.isEmpty()) { prefetchExisting(); - if (_context.getErrors().hasErrors()) + if (_context.shouldCancel()) return false; } 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() @@ -194,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 -> { @@ -215,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; }; @@ -229,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()); } @@ -255,12 +269,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()); + Object pkVal = pkSuppliers.get(p).get(); + sqlf.add(pkVal); + if (!_sharedKeys.contains(pkColumns.get(p).getColumnName())) + pkKeys.add(pkVal.toString()); } sqlf.append(")"); + checkDuplicateKeys(pkKeys); rowNumContainers.put(lastPrefetchRowNumber, container); } while (--rows > 0 && _delegate.next()); @@ -347,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(); } @@ -370,8 +389,17 @@ protected void prefetchExisting() throws BatchValidationException { lastPrefetchRowNumber = (Integer) _delegate.get(0); Map keyMap = CaseInsensitiveHashMap.of(); + List pkKeys = new ArrayList<>(); for (int p=0 ; p _previouslyUpdatedRows = new HashSet<>(); protected AbstractQueryUpdateService(TableInfo queryTable) { @@ -150,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) { @@ -834,6 +843,43 @@ public List> updateRows(User user, Container container, List return result; } + 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 public Map moveRows(User user, Container container, Container targetContainer, List> rows, BatchValidationException errors, @Nullable Map configParameters, @Nullable Map extraScriptContext) throws InvalidKeyException, BatchValidationException, QueryUpdateServiceException, SQLException { @@ -1288,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 @@ -1325,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 diff --git a/api/src/org/labkey/api/query/DefaultQueryUpdateService.java b/api/src/org/labkey/api/query/DefaultQueryUpdateService.java index 0ec37705c81..fa6873f42ed 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; @@ -587,6 +588,8 @@ protected Map _update(User user, Container c, Map { }); }); + +describe('Duplicate IDs', () => { + + 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 be335169716..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; @@ -145,6 +146,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; @@ -2350,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 -> { @@ -3026,8 +3028,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()) 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..d7285c9a458 100644 --- a/specimen/src/org/labkey/specimen/query/SpecimenUpdateService.java +++ b/specimen/src/org/labkey/specimen/query/SpecimenUpdateService.java @@ -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