Issue 52657: LKSM: We shouldn't allow creating sample names that differ only in case.

api/src/org/labkey/api/query/AbstractQueryUpdateService.java

@@ -200,7 +200,9 @@ public Map<Integer, Map<String, Object>> getExistingRows(User user, Container co
         Map<Integer, Map<String, Object>> result = new LinkedHashMap<>();
         for (Map.Entry<Integer, Map<String, Object>> key : keys.entrySet())
         {
-            Map<String, Object> row = getRow(user, container, key.getValue(), verifyNoCrossFolderData);
+            Map<String, Object> keyValues = key.getValue();
+            Map<String, Object> row = getRow(user, container, keyValues, verifyNoCrossFolderData);
+            boolean hasValidExisting = false;
             if (row != null)
             {
                 result.put(key.getKey(), row);
@@ -212,9 +214,14 @@ public Map<Integer, Map<String, Object>> getExistingRows(User user, Container co
                     if (!container.getId().equals(dataContainer))
                         throw new InvalidKeyException("Data doesn't belong to folder '" + container.getName() + "': " + key.getValue().values());
                 }
+                // sql server will return case-insensitive match, check for exact match using equals
+                if (verifyExisting)
+                    hasValidExisting = !keyValues.containsKey("Name") || keyValues.get("Name").equals(row.get("Name"));
             }
-            else if (verifyExisting)
-                throw new InvalidKeyException("Data not found for " + key.getValue().values());
+
+            if (verifyExisting && !hasValidExisting)
+                throw new InvalidKeyException("Data not found: " + (keyValues.get("Name") != null ? keyValues.get("Name") : keyValues.values()) + ".");
+
         }
         return result;
     }

experiment/src/client/test/integration/DataClassCrud.ispec.ts

@@ -233,7 +233,7 @@ describe('Import with update / merge', () => {
    it ("Issue 52922: Blank sample id in the file are getting ignored in update from file", async () => {
        const BLANK_KEY_UPDATE_ERROR_NO_EXPRESSION = 'Missing value for required property: Name';
        const BLANK_KEY_UPDATE_ERROR_WITH_EXPRESSION = 'Name value not provided on row ';
-       const BOGUS_KEY_UPDATE_ERROR = 'Data not found for ';
+       const BOGUS_KEY_UPDATE_ERROR = 'Data not found: ';
        const CROSS_FOLDER_UPDATE_NOT_SUPPORTED_ERROR = "Data doesn't belong to folder ";
 
        const dataType = "NoExpressionNameRequired52922";
@@ -357,11 +357,11 @@ describe('Duplicate IDs', () => {
             }]
         }, { ...topFolderOptions, ...editorUserOptions }).expect((result) => {
             errorResp = JSON.parse(result.text);
-            expect(errorResp.exception.indexOf('duplicate key') > -1).toBeTruthy();
+            expect(errorResp.exception.indexOf('already exists') > -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();
+        expect(errorResp.text.indexOf('already exists') > -1).toBeTruthy();
 
         // merge
         const duplicateKeyErrorPrefix = 'Duplicate key provided: ';
@@ -433,4 +433,148 @@ describe('Duplicate IDs', () => {
         expect(caseInsensitive(dataResults[1], 'description')).toBe('created');
 
     });
+
+    it("Issue 52657: We shouldn't allow creating data names that differ only in case.", async () => {
+        const dataType = "Type Case Sensitive";
+        const createPayload = {
+            kind: 'DataClass',
+            domainDesign: { name: dataType, fields: [{ name: 'Prop' }] },
+            options: {
+                name: dataType,
+                nameExpression: 'Src-${Prop}'
+            }
+        };
+
+        await server.post('property', 'createDomain', createPayload,
+            {...topFolderOptions, ...designerReaderOptions}).expect(successfulResponse);
+
+        const NAME_EXIST_MSG = "The name '%%' already exists.";
+        const data1 = 'Src-case-dAta1';
+        const data2 = 'Src-case-dAta2';
+
+        let insertRows = [{
+            name: data1,
+        },{
+            name: data2,
+        }];
+        const dataRows = await ExperimentCRUDUtils.insertRows(server, insertRows, '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');
+
+        let expectedError = NAME_EXIST_MSG.replace('%%', 'Src-case-data1');
+        // import
+        let errorResp = await ExperimentCRUDUtils.importData(server, "Name\tDescription\nSrc-case-data1\tbad\nSrc-case-data2\tbad", dataType, "IMPORT", topFolderOptions, editorUserOptions);
+        expect(errorResp.text).toContain(expectedError);
+
+        // merge
+        let mergeError = 'The name \'Src-case-data1\' could not be resolved. Please check the casing of the provided name.';
+        errorResp = await ExperimentCRUDUtils.importData(server, "Name\tDescription\nSrc-case-data1\tbad\nSrc-case-data2\tbad", dataType, "MERGE", topFolderOptions, editorUserOptions);
+        expect(errorResp.text).toContain(mergeError);
+
+        // insert
+        await server.post('query', 'insertRows', {
+            schemaName: 'exp.data',
+            queryName: dataType,
+            rows: [{
+                name: 'Src-case-data1',
+            }]
+        }, { ...topFolderOptions, ...editorUserOptions }).expect((result) => {
+            const errorResp = JSON.parse(result.text);
+            expect(errorResp['exception']).toBe(expectedError);
+        });
+
+        // insert using naming expression to create case-insensitive name
+        await server.post('query', 'insertRows', {
+            schemaName: 'exp.data',
+            queryName: dataType,
+            rows: [{
+                prop: 'case-data1',
+            }]
+        }, { ...topFolderOptions, ...editorUserOptions }).expect((result) => {
+            const errorResp = JSON.parse(result.text);
+            expect(errorResp['exception']).toBe(expectedError);
+        });
+
+        // renaming data to another data's name, using rowId
+        await server.post('query', 'updateRows', {
+            schemaName: 'exp.data',
+            queryName: dataType,
+            rows: [{
+                name: 'Src-case-dAta2',
+                rowId: data1RowId
+            }]
+        }, { ...topFolderOptions, ...editorUserOptions }).expect((result) => {
+            errorResp = JSON.parse(result.text);
+            expect(errorResp['exception']).toBe(NAME_EXIST_MSG.replace('%%', 'Src-case-dAta2'));
+        });
+
+        // renaming data to another data's case-insensitive name, using rowId
+        await server.post('query', 'updateRows', {
+            schemaName: 'exp.data',
+            queryName: dataType,
+            rows: [{
+                name: 'Src-case-data2',
+                rowId: data1RowId
+            }]
+        }, { ...topFolderOptions, ...editorUserOptions }).expect((result) => {
+            errorResp = JSON.parse(result.text);
+            expect(errorResp['exception']).toBe(NAME_EXIST_MSG.replace('%%', 'Src-case-data2'));
+        });
+
+        // renaming data to another data's case-insensitive name, using lsid. Currently can only be done using api
+        await server.post('query', 'updateRows', {
+            schemaName: 'exp.data',
+            queryName: dataType,
+            rows: [{
+                name: 'Src-case-data2',
+                lsid: data1Lsid
+            }]
+        }, { ...topFolderOptions, ...editorUserOptions }).expect((result) => {
+            errorResp = JSON.parse(result.text);
+            expect(errorResp['exception']).toBe(NAME_EXIST_MSG.replace('%%', 'Src-case-data2'));
+        });
+
+        // swap names (fail)
+        await server.post('query', 'updateRows', {
+            schemaName: 'exp.data',
+            queryName: dataType,
+            rows: [{
+                name: 'Src-case-data2',
+                lsid: data1Lsid
+            }, {
+                name: 'Src-case-data1',
+                lsid: data2Lsid
+            }]
+        }, { ...topFolderOptions, ...editorUserOptions }).expect((result) => {
+            errorResp = JSON.parse(result.text);
+            expect(errorResp['exception']).toBe(NAME_EXIST_MSG.replace('%%', 'Src-case-data2'));
+        });
+
+        await server.post('query', 'updateRows', {
+            schemaName: 'exp.data',
+            queryName: dataType,
+            rows: [{
+                name: 'Src-case-data2',
+                rowId: data1RowId
+            }, {
+                name: 'Src-case-data1',
+                rowId: data2RowId
+            }]
+        }, { ...topFolderOptions, ...editorUserOptions }).expect((result) => {
+            errorResp = JSON.parse(result.text);
+            expect(errorResp['exception']).toBe(NAME_EXIST_MSG.replace('%%', 'Src-case-data2'));
+        });
+
+        // renaming data to its case-insensitive name, using rowId
+        let results = await ExperimentCRUDUtils.updateRows(server, [{name: 'SRC-CASE-data1', rowId: data1RowId}], 'exp.data', dataType, topFolderOptions, editorUserOptions);
+        expect(caseInsensitive(results[0], 'Name')).toBe('SRC-CASE-data1');
+
+        // renaming data to its case-insensitive name, using lsid
+        results = await ExperimentCRUDUtils.updateRows(server, [{name: 'src-case-DATA1', lsid: data1Lsid}], 'exp.data', dataType, topFolderOptions, editorUserOptions);
+        expect(caseInsensitive(results[0], 'Name')).toBe('src-case-DATA1');
+
+    });
+
 });

experiment/src/client/test/integration/SampleTypeCrud.ispec.ts

@@ -317,7 +317,7 @@ describe('Import with update / merge', () => {
     it ("Issue 52922: Blank sample id in the file are getting ignored in update from file", async () => {
         const BLANK_KEY_UPDATE_ERROR = 'Name value not provided on row ';
         const BLANK_KEY_MERGE_ERROR_NO_EXPRESSION = 'SampleID or Name is required for sample on row';
-        const BOGUS_KEY_UPDATE_ERROR = 'Sample does not exist: bogus.';
+        const BOGUS_KEY_UPDATE_ERROR = 'Sample not found: bogus.';
         const CROSS_FOLDER_UPDATE_NOT_SUPPORTED_ERROR = "Sample does not belong to ";
 
         const dataType = SAMPLE_ALIQUOT_IMPORT_NO_NAME_PATTERN_NAME;
@@ -715,19 +715,19 @@ describe('Aliquot crud', () => {
             importText = "Description\tAliquotedFrom\n";
             importText += aliquotDesc + "\t" + absentRootSample + "\n";
             let resp = await ExperimentCRUDUtils.importSample(server, importText, SAMPLE_ALIQUOT_IMPORT_TYPE_NAME, "IMPORT", topFolderOptions, editorUserOptions);
-            expect(resp.text.indexOf("Aliquot parent 'Absent_Root' not found.") > -1).toBeTruthy();
+            expect(resp.text).toContain("Aliquot parent 'Absent_Root' not found.");
             const invalidRootSample = "Not_This_Root";
             await ExperimentCRUDUtils.insertSamples(server, [{name: invalidRootSample}], SAMPLE_ALIQUOT_IMPORT_TYPE_NAME, topFolderOptions, editorUserOptions)
 
             importText = "Name\tDescription\tAliquotedFrom\n";
             importText += aliquot01 + "\t" + aliquotDesc + "\t" + invalidRootSample + "\n";
             // Validate that if the AliquotedFrom field has an invalid value the import fails.
             resp = await ExperimentCRUDUtils.importSample(server, importText, SAMPLE_ALIQUOT_IMPORT_TYPE_NAME, "IMPORT", topFolderOptions, editorUserOptions);
-            expect(resp.text.indexOf("duplicate key") > -1).toBeTruthy();
+            expect(resp.text).toContain("already exists");
 
             // Validate that the AliquotedFrom field of an aliquot cannot be updated.
             resp = await ExperimentCRUDUtils.importSample(server, importText, SAMPLE_ALIQUOT_IMPORT_TYPE_NAME, "MERGE", topFolderOptions, editorUserOptions);
-            expect(resp.text.indexOf("Aliquot parents cannot be updated for sample testInvalidImportCasesParent1-1.") > -1).toBeTruthy();
+            expect(resp.text).toContain("Aliquot parents cannot be updated for sample testInvalidImportCasesParent1-1.");
 
             // AliquotedFrom is ignored for UPDATE option
             resp = await ExperimentCRUDUtils.importSample(server, importText, SAMPLE_ALIQUOT_IMPORT_TYPE_NAME, "UPDATE", topFolderOptions, editorUserOptions);
@@ -746,7 +746,7 @@ describe('Aliquot crud', () => {
             importText = "Name\tAliquotedFrom\n";
             importText += invalidRootSample + "\t" + parentSampleName + "\n";
             resp = await ExperimentCRUDUtils.importSample(server, importText, SAMPLE_ALIQUOT_IMPORT_TYPE_NAME, "MERGE", topFolderOptions, editorUserOptions);
-            expect(resp.text.indexOf("Unable to change sample to aliquot Not_This_Root.") > -1).toBeTruthy();
+            expect(resp.text).toContain("Unable to change sample to aliquot Not_This_Root.");
         });
 
         /**
@@ -951,3 +951,135 @@ describe('Aliquot crud', () => {
 
 });
 
+describe('Duplicate IDs', () => {
+    it("Issue 52657: We shouldn't allow creating sample names that differ only in case.", async () => {
+        const sampleTypeName = 'Type Case Sensitive';
+        let field = { name: 'case', rangeURI: 'http://www.w3.org/2001/XMLSchema#string'};
+        const domainPayload = {
+            kind: 'SampleSet',
+            domainDesign: { name: sampleTypeName, fields: [{ name: 'Name' }, field]},
+            options: {
+                name: sampleTypeName,
+                nameExpression: 'S-${case}'
+            }
+        };
+        await server.post('property', 'createDomain', domainPayload, {...topFolderOptions, ...designerReaderOptions}).expect(successfulResponse);
+
+        const NAME_EXIST_MSG = "The name '%%' already exists.";
+        const sample1 = 'S-case-sAmple1';
+        const sample2 = 'S-case-sAmple2';
+
+        let insertRows = [{
+            name: sample1,
+        },{
+            name: sample2,
+        }];
+        const sampleRows = await ExperimentCRUDUtils.insertSamples(server, insertRows, sampleTypeName, topFolderOptions, editorUserOptions);
+        const sample1RowId = caseInsensitive(sampleRows[0], 'rowId');
+        const sample1Lsid = caseInsensitive(sampleRows[0], 'lsid');
+        const sample2RowId = caseInsensitive(sampleRows[1], 'rowId');
+        const sample2Lsid = caseInsensitive(sampleRows[1], 'lsid');
+
+        let expectedError = NAME_EXIST_MSG.replace('%%', 'S-case-sample1');
+        // import
+        let errorResp = await ExperimentCRUDUtils.importSample(server, "Name\tDescription\nS-case-sample1\tbad\ns-case-sample2\tbad", sampleTypeName, "IMPORT", topFolderOptions, editorUserOptions);
+        expect(errorResp.text).toContain(expectedError);
+
+        // merge
+        let mergeError = 'The name \'S-case-sample1\' could not be resolved. Please check the casing of the provided name.';
+        errorResp = await ExperimentCRUDUtils.importSample(server, "Name\tDescription\nS-case-sample1\tbad\ns-case-sample2\tbad", sampleTypeName, "MERGE", topFolderOptions, editorUserOptions);
+        expect(errorResp.text).toContain(mergeError);
+
+        // insert
+        await server.post('query', 'insertRows', {
+            schemaName: 'samples',
+            queryName: sampleTypeName,
+            rows: [{
+                name: 'S-case-sample1',
+            }]
+        }, { ...topFolderOptions, ...editorUserOptions }).expect((result) => {
+            const errorResp = JSON.parse(result.text);
+            expect(errorResp['exception']).toBe(expectedError);
+        });
+
+        // insert using naming expression to create case-insensitive name
+        await server.post('query', 'insertRows', {
+            schemaName: 'samples',
+            queryName: sampleTypeName,
+            rows: [{
+                case: 'case-sample1',
+            }]
+        }, { ...topFolderOptions, ...editorUserOptions }).expect((result) => {
+            const errorResp = JSON.parse(result.text);
+            expect(errorResp['exception']).toBe(expectedError);
+        });
+
+        // renaming sample to another sample's case-insensitive name, using rowId
+        await server.post('query', 'updateRows', {
+            schemaName: 'samples',
+            queryName: sampleTypeName,
+            rows: [{
+                name: 'S-case-sample2',
+                rowId: sample1RowId
+            }]
+        }, { ...topFolderOptions, ...editorUserOptions }).expect((result) => {
+            errorResp = JSON.parse(result.text);
+            expect(errorResp['exception']).toBe(NAME_EXIST_MSG.replace('%%', 'S-case-sample2'));
+        });
+
+        // renaming sample to another sample's case-insensitive name, using lsid. Currently can only be done using api
+        await server.post('query', 'updateRows', {
+            schemaName: 'samples',
+            queryName: sampleTypeName,
+            rows: [{
+                name: 'S-case-sample2',
+                lsid: sample1Lsid
+            }]
+        }, { ...topFolderOptions, ...editorUserOptions }).expect((result) => {
+            errorResp = JSON.parse(result.text);
+            expect(errorResp['exception']).toBe(NAME_EXIST_MSG.replace('%%', 'S-case-sample2'));
+        });
+
+        // swap names (fail)
+        await server.post('query', 'updateRows', {
+            schemaName: 'samples',
+            queryName: sampleTypeName,
+            rows: [{
+                name: 'S-case-sample2',
+                lsid: sample1Lsid
+            }, {
+                name: 'S-case-sample1',
+                lsid: sample2Lsid
+            }]
+        }, { ...topFolderOptions, ...editorUserOptions }).expect((result) => {
+            errorResp = JSON.parse(result.text);
+            expect(errorResp['exception']).toBe(NAME_EXIST_MSG.replace('%%', 'S-case-sample2'));
+        });
+
+        await server.post('query', 'updateRows', {
+            schemaName: 'samples',
+            queryName: sampleTypeName,
+            rows: [{
+                name: 'S-case-sample2',
+                rowId: sample1RowId
+            }, {
+                name: 'S-case-sample1',
+                rowId: sample2RowId
+            }]
+        }, { ...topFolderOptions, ...editorUserOptions }).expect((result) => {
+            errorResp = JSON.parse(result.text);
+            expect(errorResp['exception']).toBe(NAME_EXIST_MSG.replace('%%', 'S-case-sample2'));
+        });
+
+        // renaming current sample to case-insensitive name, using rowId
+        let results = await ExperimentCRUDUtils.updateSamples(server, [{name: 'S-CASE-sample1', rowId: sample1RowId}], sampleTypeName, topFolderOptions, editorUserOptions);
+        expect(caseInsensitive(results[0], 'Name')).toBe('S-CASE-sample1');
+
+        // renaming current sample to case-insensitive name, using lsid
+        results = await ExperimentCRUDUtils.updateSamples(server, [{name: 's-case-SAMPLE1', lsid: sample1Lsid}], sampleTypeName, topFolderOptions, editorUserOptions);
+        expect(caseInsensitive(results[0], 'Name')).toBe('s-case-SAMPLE1');
+
+    });
+
+})
+