Skip to content

Commit ed8a8aa

Browse files
XingYXingY
authored
Revert support for case insensitive sample/data names (#6868)
1 parent 79ae10c commit ed8a8aa

File tree

9 files changed

+17
-439
lines changed

9 files changed

+17
-439
lines changed

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

Lines changed: 3 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -201,9 +201,7 @@ public Map<Integer, Map<String, Object>> getExistingRows(User user, Container co
201201
Map<Integer, Map<String, Object>> result = new LinkedHashMap<>();
202202
for (Map.Entry<Integer, Map<String, Object>> key : keys.entrySet())
203203
{
204-
Map<String, Object> keyValues = key.getValue();
205-
Map<String, Object> row = getRow(user, container, keyValues, verifyNoCrossFolderData);
206-
boolean hasValidExisting = false;
204+
Map<String, Object> row = getRow(user, container, key.getValue(), verifyNoCrossFolderData);
207205
if (row != null && !row.isEmpty())
208206
{
209207
result.put(key.getKey(), row);
@@ -215,14 +213,9 @@ public Map<Integer, Map<String, Object>> getExistingRows(User user, Container co
215213
if (!container.getId().equals(dataContainer))
216214
throw new InvalidKeyException("Data doesn't belong to folder '" + container.getName() + "': " + key.getValue().values());
217215
}
218-
// sql server will return case-insensitive match, check for exact match using equals
219-
if (verifyExisting)
220-
hasValidExisting = !keyValues.containsKey("Name") || keyValues.get("Name").equals(row.get("Name"));
221216
}
222-
223-
if (verifyExisting && !hasValidExisting)
224-
throw new InvalidKeyException("Data not found: " + (keyValues.get("Name") != null ? keyValues.get("Name") : keyValues.values()) + ".");
225-
217+
else if (verifyExisting)
218+
throw new InvalidKeyException("Data not found for " + key.getValue().values());
226219
}
227220
return result;
228221
}

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

Lines changed: 3 additions & 147 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: ';
236+
const BOGUS_KEY_UPDATE_ERROR = 'Data not found for ';
237237
const DUPLICATE_KEY_ERROR = 'duplicate key value';
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('already exists') > -1).toBeTruthy();
360+
expect(errorResp.exception.indexOf('duplicate key') > -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('already exists') > -1).toBeTruthy();
364+
expect(errorResp.text.indexOf('duplicate key') > -1).toBeTruthy();
365365

366366
// merge
367367
const duplicateKeyErrorPrefix = 'Duplicate key provided: ';
@@ -433,148 +433,4 @@ 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-
580436
});

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

Lines changed: 5 additions & 138 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 not found: bogus.';
320+
const BOGUS_KEY_UPDATE_ERROR = 'Sample does not exist: 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;
@@ -394,7 +394,6 @@ describe('Aliquot crud', () => {
394394
const aliquotQueryCols = 'name, rowid, lsid, description, str, int, isAliquot, AliquotedFromLsid/name, rootmaterialrowid, Myparentcol, Myaliquotcol, Myindependentcol';
395395

396396
async function verifyImportingWithNameValue(parentSampleName: string, sampleType: string) {
397-
console.log('Selected parentSampleName: ' + parentSampleName);
398397
const parentInsertRow = {
399398
name: parentSampleName,
400399
description: 'testImportingWithNameValue parent'
@@ -721,19 +720,19 @@ describe('Aliquot crud', () => {
721720
importText = "Description\tAliquotedFrom\n";
722721
importText += aliquotDesc + "\t" + absentRootSample + "\n";
723722
let resp = await ExperimentCRUDUtils.importSample(server, importText, SAMPLE_ALIQUOT_IMPORT_TYPE_NAME, "IMPORT", topFolderOptions, editorUserOptions);
724-
expect(resp.text).toContain("Aliquot parent 'Absent_Root' not found.");
723+
expect(resp.text.indexOf("Aliquot parent 'Absent_Root' not found.") > -1).toBeTruthy();
725724
const invalidRootSample = "Not_This_Root";
726725
await ExperimentCRUDUtils.insertSamples(server, [{name: invalidRootSample}], SAMPLE_ALIQUOT_IMPORT_TYPE_NAME, topFolderOptions, editorUserOptions)
727726

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

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

738737
// AliquotedFrom is ignored for UPDATE option
739738
resp = await ExperimentCRUDUtils.importSample(server, importText, SAMPLE_ALIQUOT_IMPORT_TYPE_NAME, "UPDATE", topFolderOptions, editorUserOptions);
@@ -752,7 +751,7 @@ describe('Aliquot crud', () => {
752751
importText = "Name\tAliquotedFrom\n";
753752
importText += invalidRootSample + "\t" + parentSampleName + "\n";
754753
resp = await ExperimentCRUDUtils.importSample(server, importText, SAMPLE_ALIQUOT_IMPORT_TYPE_NAME, "MERGE", topFolderOptions, editorUserOptions);
755-
expect(resp.text).toContain("Unable to change sample to aliquot Not_This_Root.");
754+
expect(resp.text.indexOf("Unable to change sample to aliquot Not_This_Root.") > -1).toBeTruthy();
756755
});
757756

758757
/**
@@ -957,135 +956,3 @@ describe('Aliquot crud', () => {
957956

958957
});
959958

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

0 commit comments

Comments
 (0)