Skip to content

Commit aeead5c

Browse files
XingYXingY
authored
Issue 52657: LKSM: We shouldn't allow creating sample names that differ only in case. (#6820)
1 parent 36966dc commit aeead5c

File tree

10 files changed

+441
-19
lines changed

10 files changed

+441
-19
lines changed

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

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -200,7 +200,9 @@ public Map<Integer, Map<String, Object>> getExistingRows(User user, Container co
200200
Map<Integer, Map<String, Object>> result = new LinkedHashMap<>();
201201
for (Map.Entry<Integer, Map<String, Object>> key : keys.entrySet())
202202
{
203-
Map<String, Object> row = getRow(user, container, key.getValue(), verifyNoCrossFolderData);
203+
Map<String, Object> keyValues = key.getValue();
204+
Map<String, Object> row = getRow(user, container, keyValues, verifyNoCrossFolderData);
205+
boolean hasValidExisting = false;
204206
if (row != null)
205207
{
206208
result.put(key.getKey(), row);
@@ -212,9 +214,14 @@ public Map<Integer, Map<String, Object>> getExistingRows(User user, Container co
212214
if (!container.getId().equals(dataContainer))
213215
throw new InvalidKeyException("Data doesn't belong to folder '" + container.getName() + "': " + key.getValue().values());
214216
}
217+
// sql server will return case-insensitive match, check for exact match using equals
218+
if (verifyExisting)
219+
hasValidExisting = !keyValues.containsKey("Name") || keyValues.get("Name").equals(row.get("Name"));
215220
}
216-
else if (verifyExisting)
217-
throw new InvalidKeyException("Data not found for " + key.getValue().values());
221+
222+
if (verifyExisting && !hasValidExisting)
223+
throw new InvalidKeyException("Data not found: " + (keyValues.get("Name") != null ? keyValues.get("Name") : keyValues.values()) + ".");
224+
218225
}
219226
return result;
220227
}

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

Lines changed: 147 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -233,7 +233,7 @@ describe('Import with update / merge', () => {
233233
it ("Issue 52922: Blank sample id in the file are getting ignored in update from file", async () => {
234234
const BLANK_KEY_UPDATE_ERROR_NO_EXPRESSION = 'Missing value for required property: Name';
235235
const BLANK_KEY_UPDATE_ERROR_WITH_EXPRESSION = 'Name value not provided on row ';
236-
const BOGUS_KEY_UPDATE_ERROR = 'Data not found for ';
236+
const BOGUS_KEY_UPDATE_ERROR = 'Data not found: ';
237237
const CROSS_FOLDER_UPDATE_NOT_SUPPORTED_ERROR = "Data doesn't belong to folder ";
238238

239239
const dataType = "NoExpressionNameRequired52922";
@@ -357,11 +357,11 @@ describe('Duplicate IDs', () => {
357357
}]
358358
}, { ...topFolderOptions, ...editorUserOptions }).expect((result) => {
359359
errorResp = JSON.parse(result.text);
360-
expect(errorResp.exception.indexOf('duplicate key') > -1).toBeTruthy();
360+
expect(errorResp.exception.indexOf('already exists') > -1).toBeTruthy();
361361
});
362362
// import
363363
errorResp = await ExperimentCRUDUtils.importData(server, "Name\tDescription\nduplicateShouldFail\tbad\nduplicateShouldFail\tbad", dataType, "IMPORT", topFolderOptions, editorUserOptions);
364-
expect(errorResp.text.indexOf('duplicate key') > -1).toBeTruthy();
364+
expect(errorResp.text.indexOf('already exists') > -1).toBeTruthy();
365365

366366
// merge
367367
const duplicateKeyErrorPrefix = 'Duplicate key provided: ';
@@ -433,4 +433,148 @@ describe('Duplicate IDs', () => {
433433
expect(caseInsensitive(dataResults[1], 'description')).toBe('created');
434434

435435
});
436+
437+
it("Issue 52657: We shouldn't allow creating data names that differ only in case.", async () => {
438+
const dataType = "Type Case Sensitive";
439+
const createPayload = {
440+
kind: 'DataClass',
441+
domainDesign: { name: dataType, fields: [{ name: 'Prop' }] },
442+
options: {
443+
name: dataType,
444+
nameExpression: 'Src-${Prop}'
445+
}
446+
};
447+
448+
await server.post('property', 'createDomain', createPayload,
449+
{...topFolderOptions, ...designerReaderOptions}).expect(successfulResponse);
450+
451+
const NAME_EXIST_MSG = "The name '%%' already exists.";
452+
const data1 = 'Src-case-dAta1';
453+
const data2 = 'Src-case-dAta2';
454+
455+
let insertRows = [{
456+
name: data1,
457+
},{
458+
name: data2,
459+
}];
460+
const dataRows = await ExperimentCRUDUtils.insertRows(server, insertRows, 'exp.data', dataType, topFolderOptions, editorUserOptions);
461+
const data1RowId = caseInsensitive(dataRows[0], 'rowId');
462+
const data1Lsid = caseInsensitive(dataRows[0], 'lsid');
463+
const data2RowId = caseInsensitive(dataRows[1], 'rowId');
464+
const data2Lsid = caseInsensitive(dataRows[1], 'lsid');
465+
466+
let expectedError = NAME_EXIST_MSG.replace('%%', 'Src-case-data1');
467+
// import
468+
let errorResp = await ExperimentCRUDUtils.importData(server, "Name\tDescription\nSrc-case-data1\tbad\nSrc-case-data2\tbad", dataType, "IMPORT", topFolderOptions, editorUserOptions);
469+
expect(errorResp.text).toContain(expectedError);
470+
471+
// merge
472+
let mergeError = 'The name \'Src-case-data1\' could not be resolved. Please check the casing of the provided name.';
473+
errorResp = await ExperimentCRUDUtils.importData(server, "Name\tDescription\nSrc-case-data1\tbad\nSrc-case-data2\tbad", dataType, "MERGE", topFolderOptions, editorUserOptions);
474+
expect(errorResp.text).toContain(mergeError);
475+
476+
// insert
477+
await server.post('query', 'insertRows', {
478+
schemaName: 'exp.data',
479+
queryName: dataType,
480+
rows: [{
481+
name: 'Src-case-data1',
482+
}]
483+
}, { ...topFolderOptions, ...editorUserOptions }).expect((result) => {
484+
const errorResp = JSON.parse(result.text);
485+
expect(errorResp['exception']).toBe(expectedError);
486+
});
487+
488+
// insert using naming expression to create case-insensitive name
489+
await server.post('query', 'insertRows', {
490+
schemaName: 'exp.data',
491+
queryName: dataType,
492+
rows: [{
493+
prop: 'case-data1',
494+
}]
495+
}, { ...topFolderOptions, ...editorUserOptions }).expect((result) => {
496+
const errorResp = JSON.parse(result.text);
497+
expect(errorResp['exception']).toBe(expectedError);
498+
});
499+
500+
// renaming data to another data's name, using rowId
501+
await server.post('query', 'updateRows', {
502+
schemaName: 'exp.data',
503+
queryName: dataType,
504+
rows: [{
505+
name: 'Src-case-dAta2',
506+
rowId: data1RowId
507+
}]
508+
}, { ...topFolderOptions, ...editorUserOptions }).expect((result) => {
509+
errorResp = JSON.parse(result.text);
510+
expect(errorResp['exception']).toBe(NAME_EXIST_MSG.replace('%%', 'Src-case-dAta2'));
511+
});
512+
513+
// renaming data to another data's case-insensitive name, using rowId
514+
await server.post('query', 'updateRows', {
515+
schemaName: 'exp.data',
516+
queryName: dataType,
517+
rows: [{
518+
name: 'Src-case-data2',
519+
rowId: data1RowId
520+
}]
521+
}, { ...topFolderOptions, ...editorUserOptions }).expect((result) => {
522+
errorResp = JSON.parse(result.text);
523+
expect(errorResp['exception']).toBe(NAME_EXIST_MSG.replace('%%', 'Src-case-data2'));
524+
});
525+
526+
// renaming data to another data's case-insensitive name, using lsid. Currently can only be done using api
527+
await server.post('query', 'updateRows', {
528+
schemaName: 'exp.data',
529+
queryName: dataType,
530+
rows: [{
531+
name: 'Src-case-data2',
532+
lsid: data1Lsid
533+
}]
534+
}, { ...topFolderOptions, ...editorUserOptions }).expect((result) => {
535+
errorResp = JSON.parse(result.text);
536+
expect(errorResp['exception']).toBe(NAME_EXIST_MSG.replace('%%', 'Src-case-data2'));
537+
});
538+
539+
// swap names (fail)
540+
await server.post('query', 'updateRows', {
541+
schemaName: 'exp.data',
542+
queryName: dataType,
543+
rows: [{
544+
name: 'Src-case-data2',
545+
lsid: data1Lsid
546+
}, {
547+
name: 'Src-case-data1',
548+
lsid: data2Lsid
549+
}]
550+
}, { ...topFolderOptions, ...editorUserOptions }).expect((result) => {
551+
errorResp = JSON.parse(result.text);
552+
expect(errorResp['exception']).toBe(NAME_EXIST_MSG.replace('%%', 'Src-case-data2'));
553+
});
554+
555+
await server.post('query', 'updateRows', {
556+
schemaName: 'exp.data',
557+
queryName: dataType,
558+
rows: [{
559+
name: 'Src-case-data2',
560+
rowId: data1RowId
561+
}, {
562+
name: 'Src-case-data1',
563+
rowId: data2RowId
564+
}]
565+
}, { ...topFolderOptions, ...editorUserOptions }).expect((result) => {
566+
errorResp = JSON.parse(result.text);
567+
expect(errorResp['exception']).toBe(NAME_EXIST_MSG.replace('%%', 'Src-case-data2'));
568+
});
569+
570+
// renaming data to its case-insensitive name, using rowId
571+
let results = await ExperimentCRUDUtils.updateRows(server, [{name: 'SRC-CASE-data1', rowId: data1RowId}], 'exp.data', dataType, topFolderOptions, editorUserOptions);
572+
expect(caseInsensitive(results[0], 'Name')).toBe('SRC-CASE-data1');
573+
574+
// renaming data to its case-insensitive name, using lsid
575+
results = await ExperimentCRUDUtils.updateRows(server, [{name: 'src-case-DATA1', lsid: data1Lsid}], 'exp.data', dataType, topFolderOptions, editorUserOptions);
576+
expect(caseInsensitive(results[0], 'Name')).toBe('src-case-DATA1');
577+
578+
});
579+
436580
});

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

Lines changed: 137 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -317,7 +317,7 @@ describe('Import with update / merge', () => {
317317
it ("Issue 52922: Blank sample id in the file are getting ignored in update from file", async () => {
318318
const BLANK_KEY_UPDATE_ERROR = 'Name value not provided on row ';
319319
const BLANK_KEY_MERGE_ERROR_NO_EXPRESSION = 'SampleID or Name is required for sample on row';
320-
const BOGUS_KEY_UPDATE_ERROR = 'Sample does not exist: bogus.';
320+
const BOGUS_KEY_UPDATE_ERROR = 'Sample not found: bogus.';
321321
const CROSS_FOLDER_UPDATE_NOT_SUPPORTED_ERROR = "Sample does not belong to ";
322322

323323
const dataType = SAMPLE_ALIQUOT_IMPORT_NO_NAME_PATTERN_NAME;
@@ -715,19 +715,19 @@ describe('Aliquot crud', () => {
715715
importText = "Description\tAliquotedFrom\n";
716716
importText += aliquotDesc + "\t" + absentRootSample + "\n";
717717
let resp = await ExperimentCRUDUtils.importSample(server, importText, SAMPLE_ALIQUOT_IMPORT_TYPE_NAME, "IMPORT", topFolderOptions, editorUserOptions);
718-
expect(resp.text.indexOf("Aliquot parent 'Absent_Root' not found.") > -1).toBeTruthy();
718+
expect(resp.text).toContain("Aliquot parent 'Absent_Root' not found.");
719719
const invalidRootSample = "Not_This_Root";
720720
await ExperimentCRUDUtils.insertSamples(server, [{name: invalidRootSample}], SAMPLE_ALIQUOT_IMPORT_TYPE_NAME, topFolderOptions, editorUserOptions)
721721

722722
importText = "Name\tDescription\tAliquotedFrom\n";
723723
importText += aliquot01 + "\t" + aliquotDesc + "\t" + invalidRootSample + "\n";
724724
// Validate that if the AliquotedFrom field has an invalid value the import fails.
725725
resp = await ExperimentCRUDUtils.importSample(server, importText, SAMPLE_ALIQUOT_IMPORT_TYPE_NAME, "IMPORT", topFolderOptions, editorUserOptions);
726-
expect(resp.text.indexOf("duplicate key") > -1).toBeTruthy();
726+
expect(resp.text).toContain("already exists");
727727

728728
// Validate that the AliquotedFrom field of an aliquot cannot be updated.
729729
resp = await ExperimentCRUDUtils.importSample(server, importText, SAMPLE_ALIQUOT_IMPORT_TYPE_NAME, "MERGE", topFolderOptions, editorUserOptions);
730-
expect(resp.text.indexOf("Aliquot parents cannot be updated for sample testInvalidImportCasesParent1-1.") > -1).toBeTruthy();
730+
expect(resp.text).toContain("Aliquot parents cannot be updated for sample testInvalidImportCasesParent1-1.");
731731

732732
// AliquotedFrom is ignored for UPDATE option
733733
resp = await ExperimentCRUDUtils.importSample(server, importText, SAMPLE_ALIQUOT_IMPORT_TYPE_NAME, "UPDATE", topFolderOptions, editorUserOptions);
@@ -746,7 +746,7 @@ describe('Aliquot crud', () => {
746746
importText = "Name\tAliquotedFrom\n";
747747
importText += invalidRootSample + "\t" + parentSampleName + "\n";
748748
resp = await ExperimentCRUDUtils.importSample(server, importText, SAMPLE_ALIQUOT_IMPORT_TYPE_NAME, "MERGE", topFolderOptions, editorUserOptions);
749-
expect(resp.text.indexOf("Unable to change sample to aliquot Not_This_Root.") > -1).toBeTruthy();
749+
expect(resp.text).toContain("Unable to change sample to aliquot Not_This_Root.");
750750
});
751751

752752
/**
@@ -951,3 +951,135 @@ describe('Aliquot crud', () => {
951951

952952
});
953953

954+
describe('Duplicate IDs', () => {
955+
it("Issue 52657: We shouldn't allow creating sample names that differ only in case.", async () => {
956+
const sampleTypeName = 'Type Case Sensitive';
957+
let field = { name: 'case', rangeURI: 'http://www.w3.org/2001/XMLSchema#string'};
958+
const domainPayload = {
959+
kind: 'SampleSet',
960+
domainDesign: { name: sampleTypeName, fields: [{ name: 'Name' }, field]},
961+
options: {
962+
name: sampleTypeName,
963+
nameExpression: 'S-${case}'
964+
}
965+
};
966+
await server.post('property', 'createDomain', domainPayload, {...topFolderOptions, ...designerReaderOptions}).expect(successfulResponse);
967+
968+
const NAME_EXIST_MSG = "The name '%%' already exists.";
969+
const sample1 = 'S-case-sAmple1';
970+
const sample2 = 'S-case-sAmple2';
971+
972+
let insertRows = [{
973+
name: sample1,
974+
},{
975+
name: sample2,
976+
}];
977+
const sampleRows = await ExperimentCRUDUtils.insertSamples(server, insertRows, sampleTypeName, topFolderOptions, editorUserOptions);
978+
const sample1RowId = caseInsensitive(sampleRows[0], 'rowId');
979+
const sample1Lsid = caseInsensitive(sampleRows[0], 'lsid');
980+
const sample2RowId = caseInsensitive(sampleRows[1], 'rowId');
981+
const sample2Lsid = caseInsensitive(sampleRows[1], 'lsid');
982+
983+
let expectedError = NAME_EXIST_MSG.replace('%%', 'S-case-sample1');
984+
// import
985+
let errorResp = await ExperimentCRUDUtils.importSample(server, "Name\tDescription\nS-case-sample1\tbad\ns-case-sample2\tbad", sampleTypeName, "IMPORT", topFolderOptions, editorUserOptions);
986+
expect(errorResp.text).toContain(expectedError);
987+
988+
// merge
989+
let mergeError = 'The name \'S-case-sample1\' could not be resolved. Please check the casing of the provided name.';
990+
errorResp = await ExperimentCRUDUtils.importSample(server, "Name\tDescription\nS-case-sample1\tbad\ns-case-sample2\tbad", sampleTypeName, "MERGE", topFolderOptions, editorUserOptions);
991+
expect(errorResp.text).toContain(mergeError);
992+
993+
// insert
994+
await server.post('query', 'insertRows', {
995+
schemaName: 'samples',
996+
queryName: sampleTypeName,
997+
rows: [{
998+
name: 'S-case-sample1',
999+
}]
1000+
}, { ...topFolderOptions, ...editorUserOptions }).expect((result) => {
1001+
const errorResp = JSON.parse(result.text);
1002+
expect(errorResp['exception']).toBe(expectedError);
1003+
});
1004+
1005+
// insert using naming expression to create case-insensitive name
1006+
await server.post('query', 'insertRows', {
1007+
schemaName: 'samples',
1008+
queryName: sampleTypeName,
1009+
rows: [{
1010+
case: 'case-sample1',
1011+
}]
1012+
}, { ...topFolderOptions, ...editorUserOptions }).expect((result) => {
1013+
const errorResp = JSON.parse(result.text);
1014+
expect(errorResp['exception']).toBe(expectedError);
1015+
});
1016+
1017+
// renaming sample to another sample's case-insensitive name, using rowId
1018+
await server.post('query', 'updateRows', {
1019+
schemaName: 'samples',
1020+
queryName: sampleTypeName,
1021+
rows: [{
1022+
name: 'S-case-sample2',
1023+
rowId: sample1RowId
1024+
}]
1025+
}, { ...topFolderOptions, ...editorUserOptions }).expect((result) => {
1026+
errorResp = JSON.parse(result.text);
1027+
expect(errorResp['exception']).toBe(NAME_EXIST_MSG.replace('%%', 'S-case-sample2'));
1028+
});
1029+
1030+
// renaming sample to another sample's case-insensitive name, using lsid. Currently can only be done using api
1031+
await server.post('query', 'updateRows', {
1032+
schemaName: 'samples',
1033+
queryName: sampleTypeName,
1034+
rows: [{
1035+
name: 'S-case-sample2',
1036+
lsid: sample1Lsid
1037+
}]
1038+
}, { ...topFolderOptions, ...editorUserOptions }).expect((result) => {
1039+
errorResp = JSON.parse(result.text);
1040+
expect(errorResp['exception']).toBe(NAME_EXIST_MSG.replace('%%', 'S-case-sample2'));
1041+
});
1042+
1043+
// swap names (fail)
1044+
await server.post('query', 'updateRows', {
1045+
schemaName: 'samples',
1046+
queryName: sampleTypeName,
1047+
rows: [{
1048+
name: 'S-case-sample2',
1049+
lsid: sample1Lsid
1050+
}, {
1051+
name: 'S-case-sample1',
1052+
lsid: sample2Lsid
1053+
}]
1054+
}, { ...topFolderOptions, ...editorUserOptions }).expect((result) => {
1055+
errorResp = JSON.parse(result.text);
1056+
expect(errorResp['exception']).toBe(NAME_EXIST_MSG.replace('%%', 'S-case-sample2'));
1057+
});
1058+
1059+
await server.post('query', 'updateRows', {
1060+
schemaName: 'samples',
1061+
queryName: sampleTypeName,
1062+
rows: [{
1063+
name: 'S-case-sample2',
1064+
rowId: sample1RowId
1065+
}, {
1066+
name: 'S-case-sample1',
1067+
rowId: sample2RowId
1068+
}]
1069+
}, { ...topFolderOptions, ...editorUserOptions }).expect((result) => {
1070+
errorResp = JSON.parse(result.text);
1071+
expect(errorResp['exception']).toBe(NAME_EXIST_MSG.replace('%%', 'S-case-sample2'));
1072+
});
1073+
1074+
// renaming current sample to case-insensitive name, using rowId
1075+
let results = await ExperimentCRUDUtils.updateSamples(server, [{name: 'S-CASE-sample1', rowId: sample1RowId}], sampleTypeName, topFolderOptions, editorUserOptions);
1076+
expect(caseInsensitive(results[0], 'Name')).toBe('S-CASE-sample1');
1077+
1078+
// renaming current sample to case-insensitive name, using lsid
1079+
results = await ExperimentCRUDUtils.updateSamples(server, [{name: 's-case-SAMPLE1', lsid: sample1Lsid}], sampleTypeName, topFolderOptions, editorUserOptions);
1080+
expect(caseInsensitive(results[0], 'Name')).toBe('s-case-SAMPLE1');
1081+
1082+
});
1083+
1084+
})
1085+

0 commit comments

Comments
 (0)