diff --git a/api/src/org/labkey/api/exp/api/ExperimentService.java b/api/src/org/labkey/api/exp/api/ExperimentService.java index 5e369154827..4ce4dc646c5 100644 --- a/api/src/org/labkey/api/exp/api/ExperimentService.java +++ b/api/src/org/labkey/api/exp/api/ExperimentService.java @@ -285,6 +285,8 @@ ValidationException updateDataClass( @Nullable String auditUserComment ); + void validateDataClassName(@NotNull Container c, @NotNull User u, String name, boolean skipExisting); + /** * Get all DataClass definitions in the container. If includeOtherContainers is true, * a user must be provided to check for read permission of the containers in scope. diff --git a/api/src/org/labkey/api/exp/api/SampleTypeDomainKind.java b/api/src/org/labkey/api/exp/api/SampleTypeDomainKind.java index e3c55d3286c..c2a4d1fb42a 100644 --- a/api/src/org/labkey/api/exp/api/SampleTypeDomainKind.java +++ b/api/src/org/labkey/api/exp/api/SampleTypeDomainKind.java @@ -444,39 +444,23 @@ public NameExpressionValidationResult validateNameExpressions(SampleTypeDomainKi return errors; } + @Override + public void validateDomainName(Container container, User user, @Nullable Domain domain, String name) + { + SampleTypeService.get().validateSampleTypeName(container, user, name, domain != null); + } + @Override public void validateOptions(Container container, User user, SampleTypeDomainKindProperties options, String name, Domain domain, GWTDomain updatedDomainDesign) { super.validateOptions(container, user, options, name, domain, updatedDomainDesign); - // verify and NameExpression values - TableInfo materialSourceTI = ExperimentService.get().getTinfoSampleType(); - - boolean isUpdate = domain != null; - if (!isUpdate) - { - if (name == null) - { - throw new IllegalArgumentException("You must supply a name for the sample type."); - } - else - { - ExpSampleType st = SampleTypeService.get().getSampleType(container, user, name); - if (st != null) - throw new IllegalArgumentException("A Sample Type with that name already exists."); - } - } - - // verify the length of the Name - int nameMax = materialSourceTI.getColumn("Name").getScale(); - if (name != null && name.length() >= nameMax) - throw new IllegalArgumentException("Value for Name field may not exceed " + nameMax + " characters."); + validateDomainName(container, user, domain, name); if (options == null) - { return; - } + TableInfo materialSourceTI = ExperimentService.get().getTinfoSampleType(); int nameExpMax = materialSourceTI.getColumn("NameExpression").getScale(); if (StringUtils.isNotBlank(options.getNameExpression()) && options.getNameExpression().length() > nameExpMax) throw new IllegalArgumentException("Value for Name Expression field may not exceed " + nameExpMax + " characters."); diff --git a/api/src/org/labkey/api/exp/api/SampleTypeService.java b/api/src/org/labkey/api/exp/api/SampleTypeService.java index 2d3bc26ecff..8fa9ebd9b06 100644 --- a/api/src/org/labkey/api/exp/api/SampleTypeService.java +++ b/api/src/org/labkey/api/exp/api/SampleTypeService.java @@ -219,6 +219,8 @@ default Map incrementSampleCounts(@Nullable Date counterDate) */ Function,Map> getSampleCountsFunction(@Nullable Date counterDate); + void validateSampleTypeName(Container container, User user, String name, boolean skipExistingCheck); + void deleteSampleType(int rowId, Container c, User user, @Nullable String auditUserComment) throws ExperimentException; // used by DomainKind.invalidate() diff --git a/api/src/org/labkey/api/exp/property/DomainKind.java b/api/src/org/labkey/api/exp/property/DomainKind.java index ad6cd4ef74e..508ef06f025 100644 --- a/api/src/org/labkey/api/exp/property/DomainKind.java +++ b/api/src/org/labkey/api/exp/property/DomainKind.java @@ -368,6 +368,12 @@ public UpdateableTableInfo.ObjectUriType getObjectUriColumn() return null; } + /** + * Overridable validity check for domain name. Base implementation does nothing. + */ + public void validateDomainName(Container container, User user, @Nullable Domain domain, String name) + {} + /** * Overridable validity check. Base only executes canCreateDefinition check. * NOTE: Due to historical limitations throws runtime exceptions instead of validation errors diff --git a/experiment/src/client/test/integration/utils.ts b/experiment/src/client/test/integration/utils.ts index c3bbdf83b1b..25a526e4435 100644 --- a/experiment/src/client/test/integration/utils.ts +++ b/experiment/src/client/test/integration/utils.ts @@ -16,6 +16,8 @@ export const SOURCE_TYPE_NAME_1 = 'SourceType1'; export const SOURCE_TYPE_NAME_2 = 'SourceType2'; export const ATTACHMENT_FIELD_1_NAME = 'SourceFile1'; export const ATTACHMENT_FIELD_2_NAME = 'SourceFile2'; +const SAMPLE_TYPE_DOMAIN_KIND = 'SampleSet'; +const DATA_CLASS_DOMAIN_KIND = 'DataClass'; // TODO move getSourceDataByName to ExperimentCrudUtils export async function getSourceDataByName(server: IntegrationTestServer, sourceName: string, queryName: string, columns: string = 'Name, RowId', folderOptions: RequestOptions , userOptions: RequestOptions, debug?: boolean) : Promise { @@ -306,7 +308,7 @@ export async function initProject(server: IntegrationTestServer, projectName: st } async function verifyDomainCreateFailure(server: IntegrationTestServer, domainType: string, badDomainName: string, error: string, folderOptions: RequestOptions, userOptions: RequestOptions, domainFields?: any[]) { - const field : Record = domainType === 'SampleSet' ? { name: 'Name' } : { name: 'Prop' }; + const field : Record = domainType === SAMPLE_TYPE_DOMAIN_KIND ? { name: 'Name' } : { name: 'Prop' }; const fields = [field]; if (domainFields) fields.push(...domainFields); @@ -349,7 +351,7 @@ async function verifyDomainUpdateFailure(server: IntegrationTestServer, domainId async function verifyDomainCreateSuccess(server: IntegrationTestServer, domainType: string, domainName: string, folderOptions: RequestOptions, userOptions: RequestOptions) { let domainId, domainURI; - const field = domainType === 'SampleSet' ? { name: 'Name' } : { name: 'Prop' }; + const field = domainType === SAMPLE_TYPE_DOMAIN_KIND ? { name: 'Name' } : { name: 'Prop' }; await server.post('property', 'createDomain', { kind: domainType, domainDesign: { name: domainName, fields: [field] }, @@ -370,8 +372,8 @@ const LEGAL_CHARSET = [' ', '+', '-', '_', '.', ':', '', '&', '(', ')', '/']; const alphaNumeric = ['a', 'A', '1', '0']; export async function checkDomainName(server: IntegrationTestServer, domainType: string, supportNameExpression: boolean, folderOptions: RequestOptions, userOptions: RequestOptions) { const badNames = { - '': domainType === 'SampleSet' ? 'You must supply a name for the sample type.' : `${domainType} name must not be blank.`, - ' ': domainType === 'SampleSet' ? 'You must supply a name for the sample type.' : `${domainType} name must not be blank.`, + '': domainType === SAMPLE_TYPE_DOMAIN_KIND ? 'Sample Type name is required.' : `${domainType} name must not be blank.`, + ' ': domainType === SAMPLE_TYPE_DOMAIN_KIND ? 'Sample Type name is required.' : `${domainType} name must not be blank.`, 'with\0nullCharacter': `Invalid ${domainType} name 'REPLACE'. ${domainType} name must contain only valid unicode characters.`, 'with\tnewLines': `Invalid ${domainType} name 'REPLACE'. ${domainType} name may not contain 'tab', 'new line', or 'return' characters.`, '.startWithDot': `Invalid ${domainType} name 'REPLACE'. ${domainType} name must start with a letter or a number.`, @@ -406,9 +408,9 @@ export async function checkDomainName(server: IntegrationTestServer, domainType: const { domainId, domainURI } = await verifyDomainCreateSuccess(server, domainType, domainName, folderOptions, userOptions); let dataTypeRowId = 0; - if (domainType !== 'SampleSet') + if (domainType !== SAMPLE_TYPE_DOMAIN_KIND) dataTypeRowId = await getDataClassRowIdByName(server, domainName, folderOptions); - const requireMsg = `${domainType} name must not be blank.` + const requireMsg = domainType == SAMPLE_TYPE_DOMAIN_KIND ? 'Sample Type name is required.' : `${domainType} name must not be blank.`; badNames[''] = requireMsg; badNames[' '] = requireMsg; for (let i = 0; i < badNameKeys.length; i++){ @@ -447,7 +449,7 @@ export async function checkLackDesignerOrReaderPerm(server: IntegrationTestServe export async function verifyRequiredLineageInsertUpdate(server: IntegrationTestServer, isParentSample: boolean, isChildSample: boolean, topFolderOptions: RequestOptions, subfolder1Options: RequestOptions, designerReaderOptions: RequestOptions, readerUserOptions: RequestOptions, editorUserOptions: RequestOptions, adminUserOptions: RequestOptions) { const parentDataType = isParentSample ? "ParentSampleType" : "ParentDataType"; await server.post('property', 'createDomain', { - kind: isParentSample ? 'SampleSet' : 'DataClass', + kind: isParentSample ? SAMPLE_TYPE_DOMAIN_KIND : DATA_CLASS_DOMAIN_KIND, domainDesign: { name: parentDataType, fields: [{ name: isParentSample ? 'Name' : 'Prop' }] }, options: { name: parentDataType, @@ -470,7 +472,7 @@ export async function verifyRequiredLineageInsertUpdate(server: IntegrationTestS const parentInput = (isParentSample ? (useLowerCase ? 'materialInputs/' : 'MaterialInputs/') : (useLowerCase ? 'dataInputs/' : 'DataInputs/')) + parentDataType; console.log("Selected alias input type: " + parentInput); await server.post('property', 'createDomain', { - kind: isChildSample ? 'SampleSet' : 'DataClass', + kind: isChildSample ? SAMPLE_TYPE_DOMAIN_KIND : DATA_CLASS_DOMAIN_KIND, domainDesign: { name: dataType, fields: [{ name: isChildSample ? 'Name' : 'Prop' }]}, options: { name: dataType, diff --git a/experiment/src/org/labkey/experiment/api/DataClassDomainKind.java b/experiment/src/org/labkey/experiment/api/DataClassDomainKind.java index fc87dd45df5..7efec176896 100644 --- a/experiment/src/org/labkey/experiment/api/DataClassDomainKind.java +++ b/experiment/src/org/labkey/experiment/api/DataClassDomainKind.java @@ -368,6 +368,13 @@ public void validateOptions(Container container, User user, DataClassDomainKindP } + @Override + public void validateDomainName(Container container, User user, @Nullable Domain domain, String name) + { + // will skipExisting here, that check will be made again during createDataClass / updateDataClass + ExperimentService.get().validateDataClassName(container, user, name, true); + } + @Override public Domain createDomain(GWTDomain domain, DataClassDomainKindProperties options, Container container, User user, @Nullable TemplateInfo templateInfo, boolean forUpdate) { diff --git a/experiment/src/org/labkey/experiment/api/ExperimentServiceImpl.java b/experiment/src/org/labkey/experiment/api/ExperimentServiceImpl.java index 0409a0d1fd3..6613ca20d06 100644 --- a/experiment/src/org/labkey/experiment/api/ExperimentServiceImpl.java +++ b/experiment/src/org/labkey/experiment/api/ExperimentServiceImpl.java @@ -8060,7 +8060,8 @@ public ValidationException updateDataClass(@NotNull Container c, @NotNull User u return errors; } - private void validateDataClassName(@NotNull Container c, @NotNull User u, String name, boolean skipExisting) throws IllegalArgumentException + @Override + public void validateDataClassName(@NotNull Container c, @NotNull User u, String name, boolean skipExisting) { if (name == null) throw new ApiUsageException("DataClass name is required."); diff --git a/experiment/src/org/labkey/experiment/api/SampleTypeServiceImpl.java b/experiment/src/org/labkey/experiment/api/SampleTypeServiceImpl.java index 10abe5ad1e6..03ccd6a1e5e 100644 --- a/experiment/src/org/labkey/experiment/api/SampleTypeServiceImpl.java +++ b/experiment/src/org/labkey/experiment/api/SampleTypeServiceImpl.java @@ -993,7 +993,8 @@ public Function,Map> getSampleCountsFunction(@Null }; } - private void validateSampleTypeName(Container container, User user, String name, boolean skipExistingCheck) + @Override + public void validateSampleTypeName(Container container, User user, String name, boolean skipExistingCheck) { if (name == null || StringUtils.isBlank(name)) throw new ApiUsageException("Sample Type name is required."); diff --git a/experiment/src/org/labkey/experiment/controllers/property/PropertyController.java b/experiment/src/org/labkey/experiment/controllers/property/PropertyController.java index c1ee2680a67..1b0324ecbbd 100644 --- a/experiment/src/org/labkey/experiment/controllers/property/PropertyController.java +++ b/experiment/src/org/labkey/experiment/controllers/property/PropertyController.java @@ -501,6 +501,72 @@ public Object execute(DomainApiForm form, BindException errors) } } + @Marshal(Marshaller.Jackson) + @RequiresPermission(ReadPermission.class) + public static class ValidateDomainAndFieldNamesAction extends MutatingApiAction + { + @Override + protected ObjectMapper createRequestObjectMapper() + { + ObjectMapper mapper = JsonUtil.DEFAULT_MAPPER.copy(); + _propertyService.configureObjectMapper(mapper, null); + return mapper; + } + + @Override + public Object execute(DomainApiForm form, BindException errors) + { + GWTDomain domainDesign = form.getDomainDesign(); + String kindName = form.getKind() == null ? form.getDomainKind() : form.getKind(); + DomainKind kind = PropertyService.get().getDomainKindByName(kindName); + if (kind == null) + throw new IllegalArgumentException("No domain kind matches name '" + kindName + "'"); + String typeURI = "urn:lsid:labkey.com:" + kindName + ".Folder-1000.1001:1002"; // note using a fake lsid here, since not all domain kinds override generateDomainURI + Domain domain = PropertyService.get().createDomain(getContainer(), typeURI, "test"); + + boolean checkFieldNames = !domainDesign.getFields().isEmpty(); + boolean checkDomainName = domainDesign.getName() != null; + + ApiSimpleResponse resp = new ApiSimpleResponse(); + ValidationException results = new ValidationException(); + if (checkFieldNames) + { + results = DomainUtil.validateProperties(null, domainDesign, kind, null, getUser()); + for (GWTPropertyDescriptor field : domainDesign.getFields()) + { + try + { + DomainProperty dp = DomainUtil.addProperty(domain, field, new HashMap<>(), new HashSet<>(), results); + OntologyManager.validatePropertyDescriptor(dp.getPropertyDescriptor()); + } + catch (ChangePropertyDescriptorException e) + { + results.addFieldError(field.getName(), e.getMessage()); + } + } + } + if (checkDomainName) + { + try + { + kind.validateDomainName(getContainer(), getUser(), null, domainDesign.getName()); + String nameError = DomainUtil.validateDomainName(domainDesign.getName(), kindName, kind.supportsNamingPattern()); + if (nameError != null) + results.addGlobalError(nameError); + } + catch (Exception e) + { + results.addGlobalError(e.getMessage()); + } + } + + resp.put("success", !results.hasErrors()); + if (results.hasErrors()) + resp.put("errors", results.getErrors()); + return resp; + } + } + @Marshal(Marshaller.Jackson) @RequiresPermission(ReadPermission.class) //Real permissions will be enforced later on by the DomainKind public static class SaveDomainAction extends MutatingApiAction diff --git a/list/src/org/labkey/list/model/ListDomainKind.java b/list/src/org/labkey/list/model/ListDomainKind.java index 327dafecf5a..c04fb2c1640 100644 --- a/list/src/org/labkey/list/model/ListDomainKind.java +++ b/list/src/org/labkey/list/model/ListDomainKind.java @@ -358,17 +358,23 @@ public Class getTypeClass() } @Override - public Domain createDomain(GWTDomain domain, ListDomainKindProperties listProperties, Container container, User user, @Nullable TemplateInfo templateInfo, boolean forUpdate) + public void validateDomainName(Container container, User user, @Nullable Domain domain, String name) { - String name = StringUtils.trimToEmpty(domain.getName()); - String keyName = listProperties.getKeyName(); - if (StringUtils.isEmpty(name)) - throw new ApiUsageException("List name is required"); + throw new ApiUsageException("List name is required."); if (name.length() > MAX_NAME_LENGTH) - throw new ApiUsageException("List name cannot be longer than " + MAX_NAME_LENGTH + " characters"); - if (ListService.get().getList(container, name, true) != null) + throw new ApiUsageException("List name cannot be longer than " + MAX_NAME_LENGTH + " characters."); + if (ListService.get().getList(container, name, domain == null) != null) throw new ApiUsageException("The name '" + name + "' is already in use."); + } + + @Override + public Domain createDomain(GWTDomain domain, ListDomainKindProperties listProperties, Container container, User user, @Nullable TemplateInfo templateInfo, boolean forUpdate) + { + String name = StringUtils.trimToEmpty(domain.getName()); + validateDomainName(container, user, null, name); + + String keyName = listProperties.getKeyName(); if (StringUtils.isEmpty(keyName)) throw new ApiUsageException("List keyName is required"); @@ -497,14 +503,7 @@ else if (!key.getName().equalsIgnoreCase(newKey.getName())) boolean hasNameChange = !original.getName().equals(updatedName); if (hasNameChange) { - if (updatedName.length() > MAX_NAME_LENGTH) - { - return exception.addGlobalError("List name cannot be longer than " + MAX_NAME_LENGTH + " characters."); - } - else if (ListService.get().getList(container, updatedName, false) != null) - { - return exception.addGlobalError("The name '" + updatedName + "' is already in use."); - } + validateDomainName(container, user, domain, updatedName); changeDetails.append("The name of the list domain '" + original.getName() + "' was changed to '" + updatedName + "'."); } diff --git a/query/test/src/org/labkey/test/tests/query/CrossFolderDataClassTest.java b/query/test/src/org/labkey/test/tests/query/CrossFolderDataClassTest.java index 65ca1a09580..ba58c16657f 100644 --- a/query/test/src/org/labkey/test/tests/query/CrossFolderDataClassTest.java +++ b/query/test/src/org/labkey/test/tests/query/CrossFolderDataClassTest.java @@ -8,7 +8,9 @@ import org.labkey.test.categories.Daily; import org.labkey.test.pages.query.ExecuteQueryPage; import org.labkey.test.params.FieldDefinition; +import org.labkey.test.params.FieldInfo; import org.labkey.test.params.experiment.DataClassDefinition; +import org.labkey.test.util.DomainUtils; import org.labkey.test.util.exp.DataClassAPIHelper; import java.util.Arrays; @@ -54,15 +56,16 @@ private void doSetup() * Issue 45664: addresses the problem where DataClass metadata wasn't available in query when querying cross-folder */ @Test - public void testIssue454644() throws Exception + public void testIssue45664() throws Exception { String dataClass = "TopFolderDataClass"; var fields = Arrays.asList( - new FieldDefinition("intColumn", FieldDefinition.ColumnType.Integer), - new FieldDefinition("decimalColumn", FieldDefinition.ColumnType.Decimal), - new FieldDefinition("stringColumn", FieldDefinition.ColumnType.String), - new FieldDefinition("sampleDate", FieldDefinition.ColumnType.DateAndTime), - new FieldDefinition("boolColumn", FieldDefinition.ColumnType.Boolean)); + FieldInfo.random("intColumn", FieldDefinition.ColumnType.Integer, DomainUtils.DomainKind.DataClass).getFieldDefinition(), + FieldInfo.random("decimalColumn", FieldDefinition.ColumnType.Decimal, DomainUtils.DomainKind.DataClass).getFieldDefinition(), + FieldInfo.random("stringColumn", FieldDefinition.ColumnType.String, DomainUtils.DomainKind.DataClass).getFieldDefinition(), + FieldInfo.random("sampleDate", FieldDefinition.ColumnType.DateAndTime, DomainUtils.DomainKind.DataClass).getFieldDefinition(), + FieldInfo.random("boolColumn", FieldDefinition.ColumnType.Boolean, DomainUtils.DomainKind.DataClass).getFieldDefinition() + ); // make a dataclass in the top folder, give it some data DataClassDefinition testType = new DataClassDefinition(dataClass).setFields(fields); var dGen = DataClassAPIHelper.createEmptyDataClass(getProjectName(), testType); @@ -81,9 +84,9 @@ public void testIssue454644() throws Exception // now insert a record into the dataclass, in the subfolder subfolderQueryPage.getDataRegion().clickInsertNewRow() .setField("Name", "Jeff") - .setField("intColumn", "5") - .setField("decimalColumn", "6.7") - .setField("stringColumn", "hey") + .setField(testType.getFieldByNamePart("intColumn").getName(), "5") + .setField(testType.getFieldByNamePart("decimalColumn").getName(), "6.7") + .setField(testType.getFieldByNamePart("stringColumn").getName(), "hey") .submit(); // gather the data from the view; should only see Jeff diff --git a/study/test/src/org/labkey/test/tests/study/StudyDatasetsTest.java b/study/test/src/org/labkey/test/tests/study/StudyDatasetsTest.java index b5ec3451669..149aecbc520 100644 --- a/study/test/src/org/labkey/test/tests/study/StudyDatasetsTest.java +++ b/study/test/src/org/labkey/test/tests/study/StudyDatasetsTest.java @@ -33,13 +33,16 @@ import org.labkey.test.pages.ImportDataPage; import org.labkey.test.pages.TimeChartWizard; import org.labkey.test.pages.study.DatasetDesignerPage; +import org.labkey.test.params.FieldDefinition; import org.labkey.test.params.FieldInfo; import org.labkey.test.util.AuditLogHelper; import org.labkey.test.util.DataRegionTable; +import org.labkey.test.util.DomainUtils; import org.labkey.test.util.LogMethod; import org.labkey.test.util.LoggedParam; import org.labkey.test.util.PortalHelper; import org.labkey.test.util.TestDataGenerator; +import org.labkey.test.util.data.TestDataUtils; import org.openqa.selenium.WebElement; import java.io.File; @@ -219,14 +222,20 @@ public void testDatasetRoundTripWithSpecialChars() // Issue 53431 { goToManageStudy(); String datasetName = "Issue 53431"; - FieldInfo fieldInfo = FieldInfo.random("test,./field"); + FieldInfo fieldInfo = FieldInfo.random("test,./field", FieldDefinition.ColumnType.String, DomainUtils.DomainKind.StudyDatasetVisit); DatasetDesignerPage definitionPage = _studyHelper.goToManageDatasets() .clickCreateNewDataset() .setName(datasetName); DomainFormPanel panel = definitionPage.getFieldsPanel(); panel.manuallyDefineFields(fieldInfo.getFieldDefinition()); definitionPage.clickSave(); - importDatasetData(datasetName, "mouseId\tsequenceNum\t\"" + fieldInfo.getName() + "\"\n", "a1\t1\ttest123", "All data"); + importDatasetData(datasetName, "", TestDataUtils.tsvStringFromRowMaps( + List.of(Map.of( + "mouseId", "a1", + "sequenceNum", "1", + fieldInfo.getName(), "test123" + )), List.of("mouseId", "sequenceNum", fieldInfo.getName()), true + ), "All data"); File exportedFolder = exportFolderAsZip(null, false, false, false, false); deleteStudy();