From 40a64ac8c60e5fcc3039ce293beaa10f1080d573 Mon Sep 17 00:00:00 2001 From: XingY Date: Fri, 6 Dec 2024 14:41:45 -0800 Subject: [PATCH 01/16] Validate domain names for special characters or reserved patterns during create and update --- .../org/labkey/api/data/NameGenerator.java | 23 ++++++++ .../api/exp/api/SampleTypeDomainKind.java | 6 ++ .../labkey/api/exp/property/DomainKind.java | 5 ++ .../labkey/api/exp/property/DomainUtil.java | 57 +++++++++++++++++-- api/src/org/labkey/api/util/FileUtil.java | 15 +---- .../labkey/api/util/StringUtilsLabKey.java | 21 +++++++ .../experiment/api/DataClassDomainKind.java | 6 ++ 7 files changed, 113 insertions(+), 20 deletions(-) diff --git a/api/src/org/labkey/api/data/NameGenerator.java b/api/src/org/labkey/api/data/NameGenerator.java index 61f21d93e01..b055d4451b3 100644 --- a/api/src/org/labkey/api/data/NameGenerator.java +++ b/api/src/org/labkey/api/data/NameGenerator.java @@ -445,6 +445,29 @@ static List getFieldMissingBracesWarnings(@NotNull String nameExpression } + public static String validateFieldKeyConflict(String fieldKey) + { + String fieldKeyLc = fieldKey.toLowerCase(); + + Set formatNames = new HashSet<>(SubstitutionFormat.getFormatNames()); + formatNames.add(SubstitutionValue.withCounter.name()); + for (String formatName : formatNames) + { + String lcFormatName = ":" + formatName.toLowerCase(); + int matchInd = fieldKeyLc.indexOf(lcFormatName); + if (matchInd > -1) + return "'" + fieldKey.substring(matchInd, matchInd + lcFormatName.length()) + "' is a reserved pattern."; + } + + for (SubstitutionValue subValue : SubstitutionValue.values()) + { + if (subValue.getKey().equalsIgnoreCase(fieldKey)) + return fieldKey + " is a reserved name."; + } + + return null; + } + static Pair, List> getReservedFieldValidationResults(String nameExpression) { // For each substitution format, find its location in the string diff --git a/api/src/org/labkey/api/exp/api/SampleTypeDomainKind.java b/api/src/org/labkey/api/exp/api/SampleTypeDomainKind.java index 14e9e3daa39..2477041e20c 100644 --- a/api/src/org/labkey/api/exp/api/SampleTypeDomainKind.java +++ b/api/src/org/labkey/api/exp/api/SampleTypeDomainKind.java @@ -671,4 +671,10 @@ public boolean supportsPhiLevel() { return ComplianceService.get().isComplianceSupported(); } + + @Override + public boolean supportNamingPattern() + { + return true; + } } diff --git a/api/src/org/labkey/api/exp/property/DomainKind.java b/api/src/org/labkey/api/exp/property/DomainKind.java index d9d1d6f8d6a..d17488b99ff 100644 --- a/api/src/org/labkey/api/exp/property/DomainKind.java +++ b/api/src/org/labkey/api/exp/property/DomainKind.java @@ -410,4 +410,9 @@ public boolean supportsPhiLevel() { return false; } + + public boolean supportNamingPattern() + { + return false; + } } diff --git a/api/src/org/labkey/api/exp/property/DomainUtil.java b/api/src/org/labkey/api/exp/property/DomainUtil.java index a86adb1f197..028e726fa0b 100644 --- a/api/src/org/labkey/api/exp/property/DomainUtil.java +++ b/api/src/org/labkey/api/exp/property/DomainUtil.java @@ -32,6 +32,7 @@ import org.labkey.api.data.ContainerFilter; import org.labkey.api.data.ContainerManager; import org.labkey.api.data.ContainerService; +import org.labkey.api.data.NameGenerator; import org.labkey.api.data.PHI; import org.labkey.api.data.SchemaTableInfo; import org.labkey.api.data.SimpleFilter; @@ -76,6 +77,7 @@ import org.labkey.api.util.JsonUtil; import org.labkey.api.util.Pair; import org.labkey.api.util.StringExpression; +import org.labkey.api.util.StringUtilsLabKey; import org.labkey.api.view.UnauthorizedException; import org.labkey.data.xml.ColumnType; import org.labkey.data.xml.ConditionalFormatFilterType; @@ -104,6 +106,8 @@ public class DomainUtil { private static final Logger LOG = LogManager.getLogger(DomainUtil.class); + private static final String ILLEGAL_CHARSET = "<>$[]{};,`\"~!@#$%^*=|?\\"; + private DomainUtil() { } @@ -688,10 +692,6 @@ public static Domain createDomain( { // Create a copy of the GWTDomain to ensure the template's Domain is not modified domain = new GWTDomain(domain); - if (domainName != null) - { - domain.setName(domainName); - } DomainKind kind = PropertyService.get().getDomainKindByName(kindName); if (kind == null) @@ -700,6 +700,17 @@ public static Domain createDomain( if (!kind.canCreateDefinition(user, container)) throw new UnauthorizedException("You don't have permission to create a new domain"); + + if (domainName != null) + domain.setName(domainName); + + if (domain.getName() != null) + { + String domainNameError = validateDomainName(domain.getName(), kind.supportNamingPattern()); + if (!StringUtils.isEmpty(domainNameError)) + throw new IllegalArgumentException(domainNameError); + } + // Issue 48810: if not creating from templateInfo, validate reserved field names based on domainKind boolean strictFieldValidation = arguments != null ? (Boolean) arguments.getOrDefault("strictFieldValidation", true) : true; DomainKind validateDomainKind = templateInfo == null && strictFieldValidation ? kind : null; @@ -754,10 +765,17 @@ public static ValidationException updateDomainDescriptor(GWTDomain updatedFields, List origFields) { @@ -1333,6 +1368,16 @@ public static ValidationException validateProperties(@Nullable Domain domain, @N continue; } + if (domainKind != null && domainKind.supportNamingPattern()) + { + String nameSyntaxError = NameGenerator.validateFieldKeyConflict(name); + if (nameSyntaxError != null) + { + exception.addFieldError(name, nameSyntaxError); + continue; + } + } + if (namePropertyIdMap.containsKey(name)) { String errorMsg = getDomainErrorMessage(updates,"The field name '" + name + "' is already taken. Please provide a unique name for each field."); diff --git a/api/src/org/labkey/api/util/FileUtil.java b/api/src/org/labkey/api/util/FileUtil.java index 2b4645a090e..ec8642446b6 100644 --- a/api/src/org/labkey/api/util/FileUtil.java +++ b/api/src/org/labkey/api/util/FileUtil.java @@ -334,20 +334,7 @@ static String isAllowedFileName(String s, boolean checkFileExtension, AppProps a private static @Nullable String validateFileName(String s) { - if (StringUtils.isBlank(s)) - return "Filename must not be blank"; - if (!ViewServlet.validChars(s)) - return "Filename must contain only valid unicode characters."; - if (StringUtils.containsAny(s, restrictedPrintable)) - return "Filename may not contain any of these characters: " + restrictedPrintable; - if (StringUtils.containsAny(s, "\t\n\r")) - return "Filename may not contain 'tab', 'new line', or 'return' characters."; - if (StringUtils.contains("-$", s.charAt(0))) - return "Filename may not begin with any of these characters: -$"; - if (Pattern.matches("(.*\\s--[^ ].*)|(.*\\s-[^- ].*)", s)) - return "Filename may not contain space followed by dash."; - - return null; + return StringUtilsLabKey.validateLegalNames(s, restrictedPrintable, "Filename"); } private static String checkExtension(String filename, AppProps appProps) diff --git a/api/src/org/labkey/api/util/StringUtilsLabKey.java b/api/src/org/labkey/api/util/StringUtilsLabKey.java index 89b5284c23b..9c3d740c248 100644 --- a/api/src/org/labkey/api/util/StringUtilsLabKey.java +++ b/api/src/org/labkey/api/util/StringUtilsLabKey.java @@ -25,6 +25,7 @@ import org.junit.Test; import org.labkey.api.data.Container; import org.labkey.api.exp.Identifiable; +import org.labkey.api.view.ViewServlet; import java.nio.charset.Charset; import java.nio.charset.StandardCharsets; @@ -52,6 +53,26 @@ public class StringUtilsLabKey private static final Random RANDOM = new Random(); private static final int MAX_LONG_LENGTH = String.valueOf(Long.MAX_VALUE).length() - 1; + + + public static @Nullable String validateLegalNames(String s, @NotNull String illegalCharset, String type) + { + if (StringUtils.isBlank(s)) + return type + " must not be blank"; + if (!ViewServlet.validChars(s)) + return type + " must contain only valid unicode characters."; + if (StringUtils.containsAny(s, illegalCharset)) + return type + " may not contain any of these characters: " + illegalCharset; + if (StringUtils.containsAny(s, "\t\n\r")) + return type + " may not contain 'tab', 'new line', or 'return' characters."; + if (StringUtils.contains("-$", s.charAt(0))) + return type + " may not begin with any of these characters: -$"; + if (Pattern.matches("(.*\\s--[^ ].*)|(.*\\s-[^- ].*)", s)) + return type + " may not contain space followed by dash."; + + return null; + } + public static void append(StringBuilder sb, @Nullable Identifiable identifiable) { if (null != identifiable) diff --git a/experiment/src/org/labkey/experiment/api/DataClassDomainKind.java b/experiment/src/org/labkey/experiment/api/DataClassDomainKind.java index 96e9bf4b150..6293cecad7b 100644 --- a/experiment/src/org/labkey/experiment/api/DataClassDomainKind.java +++ b/experiment/src/org/labkey/experiment/api/DataClassDomainKind.java @@ -494,4 +494,10 @@ public boolean supportsPhiLevel() { return ComplianceService.get().isComplianceSupported(); } + + @Override + public boolean supportNamingPattern() + { + return true; + } } From 86bc15f76bb68fc088326f37f2a6d5cc3e8080df Mon Sep 17 00:00:00 2001 From: XingY Date: Mon, 9 Dec 2024 18:45:35 -0800 Subject: [PATCH 02/16] Add restrictions on names of domains --- .../org/labkey/api/data/NameGenerator.java | 109 +++++++++++------- .../labkey/api/exp/api/ExperimentService.java | 12 ++ .../labkey/api/exp/property/DomainUtil.java | 23 ++-- 3 files changed, 96 insertions(+), 48 deletions(-) diff --git a/api/src/org/labkey/api/data/NameGenerator.java b/api/src/org/labkey/api/data/NameGenerator.java index b055d4451b3..28d13a0a914 100644 --- a/api/src/org/labkey/api/data/NameGenerator.java +++ b/api/src/org/labkey/api/data/NameGenerator.java @@ -823,7 +823,7 @@ public static boolean isParentInputWithDataType(String[] parts, @Nullable String return false; inputToken = inputToken.substring(1); // remove leading ~ } - String dataType = parts[1]; + String dataType = QueryKey.decodePart(parts[1]); // If data type contains special characters boolean isInput = INPUT_PARENT.equalsIgnoreCase(inputToken); boolean isData = ExpData.DATA_INPUT_PARENT.equalsIgnoreCase(inputToken); @@ -847,8 +847,20 @@ public static boolean isParentInputWithDataType(String[] parts, @Nullable String return ExperimentService.get().getDataClass(container, user, dataType) != null; } + static String getEncodedDataTypeInExpression(String expression) + { + return expression.replaceAll("/", "\\$S"); + } + + static String getDecodedDataTypeInExpression(String expression) + { + return QueryKey.decodePart(expression); + } + private Object getParentLookupTokenPreview(String currentDataType, FieldKey fkTok, String inputPrefix, @Nullable String inputDataType, @Nullable NameExpressionAncestorPartOption ancestorPartOption, String lookupField, User user, Map dataClassNames, Map sampleTypeNames) { + if (inputDataType != null) + inputDataType = getDecodedDataTypeInExpression(inputDataType); String inputPrefixLc = inputPrefix.toLowerCase(); boolean isMaterial = inputPrefixLc.startsWith("materialinputs") || inputPrefixLc.startsWith("inputs"); boolean isData = inputPrefixLc.startsWith("datainputs") || inputPrefixLc.startsWith("inputs"); @@ -1063,13 +1075,14 @@ private void initialize(@Nullable Map importAliases) hasDateBasedSampleCounterFormat = true; } - String sTok = QueryKey.decodePart(token.toString()).toLowerCase(); + String sTok = QueryKey.decodePart(token.toString()); if (isParentInput(sTok, importAliases, _currentDataTypeName, _container, user)) { isParentPart = true; hasParentInputs = true; } + String sTokLc = sTok.toLowerCase(); if (token instanceof FieldKey fkTok) { int previousErrorCount = _syntaxErrors.size(); @@ -1211,8 +1224,8 @@ else if (fieldParts.size() > 2) if (isParentPart) { - boolean isInput = sTok.startsWith(INPUT_PARENT.toLowerCase()); - boolean isMaterial = sTok.startsWith(ExpMaterial.MATERIAL_INPUT_PARENT.toLowerCase()); + boolean isInput = sTokLc.startsWith(INPUT_PARENT.toLowerCase()); + boolean isMaterial = sTokLc.startsWith(ExpMaterial.MATERIAL_INPUT_PARENT.toLowerCase()); Object preview = isInput ? SubstitutionValue.Inputs.getPreviewValue() : (isMaterial ? SubstitutionValue.MaterialInputs.getPreviewValue() : SubstitutionValue.DataInputs.getPreviewValue()); previewCtx.put(fkTok.toString(), preview); } @@ -1381,7 +1394,7 @@ private List processFieldParts(FieldKey fkTok, Map fieldNames = new ArrayList<>(); + Set fieldNames = new HashSet<>(); if (_expParentLookupFields.containsKey(inputCol)) fieldNames.addAll(_expParentLookupFields.get(inputCol)); + if (_expParentLookupFields.containsKey(inputColEncoded)) + fieldNames.addAll(_expParentLookupFields.get(inputColEncoded)); if (_expParentLookupFields.containsKey(inputType)) fieldNames.addAll(_expParentLookupFields.get(inputType)); if (_expParentLookupFields.containsKey(INPUT_PARENT)) @@ -1939,6 +1955,8 @@ private void addParentLookupValues(String parentTypeName, inputLookupValues.computeIfAbsent(inputType + "/" + fieldName, (s) -> new ArrayList<>()).add(lookupValue); // add to Inputs// inputLookupValues.computeIfAbsent(inputCol + "/" + fieldName, (s) -> new ArrayList<>()).add(lookupValue); + if (!inputColEncoded.equalsIgnoreCase(inputCol)) + inputLookupValues.computeIfAbsent(inputColEncoded + "/" + fieldName, (s) -> new ArrayList<>()).add(lookupValue); } } @@ -2030,7 +2048,7 @@ else if (parentObject instanceof ExpData) } - private void addParentLookupContext(String parentTypeName, + private void addParentLookupContext(String parentTypeName/* already decoded */, String parentName, boolean isMaterialParent, @Nullable Map parentImportAliases, @@ -2043,12 +2061,15 @@ private void addParentLookupContext(String parentTypeName, if (!hasTypeLookup) { + String parentTypeNameEncoded = getEncodedDataTypeInExpression(parentTypeName); if (isMaterialParent) { if (_expParentLookupFields.containsKey(ExpMaterial.MATERIAL_INPUT_PARENT)) hasTypeLookup = true; else if (_expParentLookupFields.containsKey(ExpMaterial.MATERIAL_INPUT_PARENT + "/" + parentTypeName)) hasTypeLookup = true; + else if (_expParentLookupFields.containsKey(ExpMaterial.MATERIAL_INPUT_PARENT + "/" + parentTypeNameEncoded)) + hasTypeLookup = true; } else { @@ -2056,6 +2077,8 @@ else if (_expParentLookupFields.containsKey(ExpMaterial.MATERIAL_INPUT_PARENT + hasTypeLookup = true; else if (_expParentLookupFields.containsKey(ExpData.DATA_INPUT_PARENT + "/" + parentTypeName)) hasTypeLookup = true; + else if (_expParentLookupFields.containsKey(ExpData.DATA_INPUT_PARENT + "/" + parentTypeNameEncoded)) + hasTypeLookup = true; } } @@ -2171,24 +2194,10 @@ private Map additionalContext(@NotNull Map rowMa if (value == null) continue; - String[] parts = colName.split("/", 2); - - if (parts.length == 2) - { - if (_expressionSummary.hasParentInputs) - addInputs(parts, colName, value, inputs, parentImportAliases); - if (_expressionSummary.hasParentLookup) - addParentLookupInput(parts, colName, value, parentImportAliases, inputLookupValues); - } - else if (parentImportAliases != null && parentImportAliases.containsKey(colName)) - { - String colNameForAlias = parentImportAliases.get(colName); - parts = colNameForAlias.split("/", 2); - if (_expressionSummary.hasParentInputs) - addInputs(parts, colNameForAlias, value, inputs, parentImportAliases); - if (_expressionSummary.hasParentLookup) - addParentLookupInput(parts, colName, value, parentImportAliases, inputLookupValues); - } + if (_expressionSummary.hasParentInputs) + addInputs(colName, value, inputs, parentImportAliases); + if (_expressionSummary.hasParentLookup) + addParentLookupInput(colName, value, parentImportAliases, inputLookupValues); } // if a single input or lookup is found, return the object, not the list @@ -2277,18 +2286,24 @@ else if (value.isEmpty()) return ctx; } - private void addParentLookupInput(String[] parts, - String colName, + private void addParentLookupInput(String colName, Object value, @Nullable Map parentImportAliases, Map> inputLookupValues) { - boolean isMaterialParent = parts[0].equalsIgnoreCase(ExpMaterial.MATERIAL_INPUT_PARENT); - boolean isDataParent = parts[0].equalsIgnoreCase(ExpData.DATA_INPUT_PARENT); - if (isMaterialParent || isDataParent) + String[] parts = colName.split("/", 2); + if (parentImportAliases != null && parentImportAliases.containsKey(colName)) + parts = parentImportAliases.get(colName).split("/", 2); + + if (parts.length == 2) { - for (String parent : parentNames(value, colName)) - addParentLookupContext(QueryKey.decodePart(parts[1]), parent, isMaterialParent, parentImportAliases, inputLookupValues); + boolean isMaterialParent = parts[0].equalsIgnoreCase(ExpMaterial.MATERIAL_INPUT_PARENT); + boolean isDataParent = parts[0].equalsIgnoreCase(ExpData.DATA_INPUT_PARENT); + if (isMaterialParent || isDataParent) + { + for (String parent : parentNames(value, colName)) + addParentLookupContext(QueryKey.decodePart(parts[1]), parent, isMaterialParent, parentImportAliases, inputLookupValues); + } } } @@ -2297,12 +2312,19 @@ private Collection parentNames(Object value, String parentColName) return NameGenerator.parentNames(value, parentColName).collect(Collectors.toList()); } - private void addInputs(String[] parts, - String colName, + private void addInputs(String colName, Object value, Map> inputs, @Nullable Map parentImportAliases) { + String[] parts = colName.split("/", 2); + boolean isParentAliasInput = false; + if (parts.length == 1 && parentImportAliases != null && parentImportAliases.containsKey(colName)) + { + isParentAliasInput = true; + parts = parentImportAliases.get(colName).split("/", 2); + } + if (parts.length == 2) { String inputsCategory = null; @@ -2312,21 +2334,30 @@ else if (parts[0].equalsIgnoreCase(ExpMaterial.MATERIAL_INPUT_PARENT)) inputsCategory = ExpMaterial.MATERIAL_INPUT_PARENT; if (inputsCategory != null) { + String dataType = parts[1]; + String decodedDataType = QueryKey.decodePart(dataType); // data might come in as encoded or decoded Collection parents = parentNames(value, colName); inputs.get(INPUT_PARENT).addAll(parents); - inputs.computeIfAbsent(INPUT_PARENT + "/" + parts[1], (s) -> new LinkedHashSet<>()).addAll(parents); // add Inputs/SampleType1 inputs.get(inputsCategory).addAll(parents); + + Set dataTypeAltNames = new HashSet<>(); + dataTypeAltNames.add(decodedDataType); + dataTypeAltNames.add(getEncodedDataTypeInExpression(decodedDataType)); + for (String dataTypeAltName : dataTypeAltNames) + { + inputs.computeIfAbsent(INPUT_PARENT + "/" + dataTypeAltName, (s) -> new LinkedHashSet<>()).addAll(parents); // add Inputs/SampleType1 + if (!parents.isEmpty()) // convert "parent1,parent2" to [parent1, parent2] + inputs.computeIfAbsent(parts[0] + "/" + dataTypeAltName, (s) -> new LinkedHashSet<>()).addAll(parents); + } + // if import aliases are defined, also add in the inputs under the aliases in case those are used in the name expression - if (parentImportAliases != null) + if (parentImportAliases != null && !isParentAliasInput) { Optional> aliasEntry = parentImportAliases.entrySet().stream().filter(entry -> entry.getValue().equalsIgnoreCase(colName)).findFirst(); aliasEntry.ifPresent(entry -> { inputs.computeIfAbsent(entry.getKey(), (s) -> new LinkedHashSet<>()).addAll(parents); }); } - - if (!parents.isEmpty()) // convert "parent1,parent2" to [parent1, parent2] - inputs.computeIfAbsent(parts[0] + "/" + parts[1], (s) -> new LinkedHashSet<>()).addAll(parents); } } } diff --git a/api/src/org/labkey/api/exp/api/ExperimentService.java b/api/src/org/labkey/api/exp/api/ExperimentService.java index 1cc9a354b23..5b4f5aa189c 100644 --- a/api/src/org/labkey/api/exp/api/ExperimentService.java +++ b/api/src/org/labkey/api/exp/api/ExperimentService.java @@ -26,6 +26,7 @@ import org.labkey.api.data.ContainerFilter; import org.labkey.api.data.DbSchema; import org.labkey.api.data.DbScope; +import org.labkey.api.data.NameGenerator; import org.labkey.api.data.RemapCache; import org.labkey.api.data.SQLFragment; import org.labkey.api.data.TableInfo; @@ -75,6 +76,7 @@ import org.labkey.api.security.User; import org.labkey.api.services.ServiceRegistry; import org.labkey.api.util.Pair; +import org.labkey.api.util.StringUtilsLabKey; import org.labkey.api.view.HttpView; import org.labkey.api.view.NotFoundException; import org.labkey.api.view.ViewBackgroundInfo; @@ -118,6 +120,8 @@ public interface ExperimentService extends ExperimentRunTypeSource String LINEAGE_DEFAULT_MAXIMUM_DEPTH_PROPERTY_NAME = "lineageDefaultMaximumDepth"; + String ILLEGAL_PARENT_ALIAS_CHARSET = "/:<>$[]{};,`\"~!@#$%^*=|?\\"; + int SIMPLE_PROTOCOL_FIRST_STEP_SEQUENCE = 1; int SIMPLE_PROTOCOL_CORE_STEP_SEQUENCE = 10; int SIMPLE_PROTOCOL_EXTRA_STEP_SEQUENCE = 15; @@ -539,6 +543,14 @@ static void validateParentAlias(Map aliasMap, Set reserv throw new IllegalArgumentException(String.format("Duplicate parent alias header found: %1$s", trimmedKey)); } + String legalCharacterCheck = StringUtilsLabKey.validateLegalNames(trimmedKey, ILLEGAL_PARENT_ALIAS_CHARSET, "Parent alias"); + if (legalCharacterCheck != null) + throw new IllegalArgumentException(legalCharacterCheck); + + String nameSyntaxError = NameGenerator.validateFieldKeyConflict(trimmedKey); + if (nameSyntaxError != null) + throw new IllegalArgumentException("Invalid Parent alias '" + trimmedKey + "'. " + nameSyntaxError); + //Check if parent alias has correct format MaterialInput/ or NEW_SAMPLE_TYPE_ALIAS_VALUE, or DataInput/ or NEW_DATA_CLASS_ALIAS_VALUE if (!ExperimentService.parentAliasHasCorrectFormat(trimmedValue)) throw new IllegalArgumentException(String.format("Invalid parent alias header: %1$s", trimmedValue)); diff --git a/api/src/org/labkey/api/exp/property/DomainUtil.java b/api/src/org/labkey/api/exp/property/DomainUtil.java index 028e726fa0b..82d33cda018 100644 --- a/api/src/org/labkey/api/exp/property/DomainUtil.java +++ b/api/src/org/labkey/api/exp/property/DomainUtil.java @@ -106,7 +106,7 @@ public class DomainUtil { private static final Logger LOG = LogManager.getLogger(DomainUtil.class); - private static final String ILLEGAL_CHARSET = "<>$[]{};,`\"~!@#$%^*=|?\\"; + private static final String ILLEGAL_CHARSET = "<>[]{};,`\"~!@#$%^*=|?\\"; private DomainUtil() { @@ -706,7 +706,7 @@ public static Domain createDomain( if (domain.getName() != null) { - String domainNameError = validateDomainName(domain.getName(), kind.supportNamingPattern()); + String domainNameError = validateDomainName(domain.getName(), kind); if (!StringUtils.isEmpty(domainNameError)) throw new IllegalArgumentException(domainNameError); } @@ -768,7 +768,7 @@ public static ValidationException updateDomainDescriptor(GWTDomain Date: Tue, 10 Dec 2024 14:50:12 -0800 Subject: [PATCH 03/16] api tests --- .../org/labkey/api/data/NameGenerator.java | 2 +- .../labkey/api/exp/property/DomainUtil.java | 24 +-- .../labkey/assay/AssayDomainServiceImpl.java | 10 ++ .../test/integration/AssayDesignCrud.ispec.ts | 161 +++++++++++++----- .../test/integration/DataClassCrud.ispec.ts | 7 +- .../test/integration/SampleTypeCrud.ispec.ts | 7 +- .../src/client/test/integration/utils.ts | 106 +++++++++++- .../property/PropertyController.java | 7 + .../labkey/study/model/DatasetDomainKind.java | 4 + 9 files changed, 269 insertions(+), 59 deletions(-) diff --git a/api/src/org/labkey/api/data/NameGenerator.java b/api/src/org/labkey/api/data/NameGenerator.java index 28d13a0a914..7dac53aff33 100644 --- a/api/src/org/labkey/api/data/NameGenerator.java +++ b/api/src/org/labkey/api/data/NameGenerator.java @@ -462,7 +462,7 @@ public static String validateFieldKeyConflict(String fieldKey) for (SubstitutionValue subValue : SubstitutionValue.values()) { if (subValue.getKey().equalsIgnoreCase(fieldKey)) - return fieldKey + " is a reserved name."; + return "'" + fieldKey + "' is a reserved name."; } return null; diff --git a/api/src/org/labkey/api/exp/property/DomainUtil.java b/api/src/org/labkey/api/exp/property/DomainUtil.java index 82d33cda018..860ffbbf24a 100644 --- a/api/src/org/labkey/api/exp/property/DomainUtil.java +++ b/api/src/org/labkey/api/exp/property/DomainUtil.java @@ -106,7 +106,7 @@ public class DomainUtil { private static final Logger LOG = LogManager.getLogger(DomainUtil.class); - private static final String ILLEGAL_CHARSET = "<>[]{};,`\"~!@#$%^*=|?\\"; + public static final String ILLEGAL_DOMAIN_NAME_CHARSET = "<>[]{};,`\"~!@#$%^*=|?\\"; private DomainUtil() { @@ -706,7 +706,7 @@ public static Domain createDomain( if (domain.getName() != null) { - String domainNameError = validateDomainName(domain.getName(), kind); + String domainNameError = validateDomainName(domain.getName(), kind.getKindName(), kind.supportNamingPattern()); if (!StringUtils.isEmpty(domainNameError)) throw new IllegalArgumentException(domainNameError); } @@ -768,7 +768,7 @@ public static ValidationException updateDomainDescriptor(GWTDomain { }); + async function verifyAssayDesignCreateFailure(badDomainName: string, error: string) { + let exception = null; + const badDomainNameResp = await server.post( + 'assay', + 'saveProtocol.api', + getAssayDesignPayload(badDomainName, [], []), + {...topFolderOptions, ...designerReaderOptions} + ).expect((resp) => { + exception = JSON.parse(resp.text).exception; + }) + + if (exception !== error) + console.log(badDomainName); + + expect(exception).toBe(error); + } + + async function verifyAssayDesignUpdateFailure(payload: any, badDomainName: string, error: string) { + let exception = null; + payload['name'] = badDomainName; + const badDomainNameResp = await server.post( + 'assay', + 'saveProtocol.api', + payload, + {...topFolderOptions, ...designerReaderOptions} + ).expect((resp) => { + exception = JSON.parse(resp.text).exception; + }) + + if (exception !== error) + console.log(badDomainName); + + expect(exception).toBe(error); + } + + it('Assay design name validation', async () => { + const badNames = { + '': 'Domain name must not be blank', + ' ': 'Domain name must not be blank', + 'with\0nullCharacter': `Invalid Assay Design name. Domain name must contain only valid unicode characters.`, + 'with\tnewLines': `Invalid Assay Design name. Domain name may not contain 'tab', 'new line', or 'return' characters.`, + '.startWithDot': `Invalid Assay Design name. Domain name must start with a letter or a number character.`, + ['c' + selectRandomN(ILLEGAL_DOMAIN_CHARSET.split(''), 2).join('')]: `Invalid Assay Design name. Domain name may not contain any of these characters: ` + ILLEGAL_DOMAIN_CHARSET, + 'a -b': `Invalid Assay Design name. Domain name may not contain space followed by dash.` + }; + + let badNameKeys = Object.keys(badNames); + for (let i = 0; i < badNameKeys.length; i++) + await verifyAssayDesignCreateFailure(badNameKeys[i], badNames[badNameKeys[i]]); + + let assayDesignPayload = getAssayDesignPayload('good', [], []); + + await server.post( + 'assay', + 'saveProtocol.api', + assayDesignPayload, + {...topFolderOptions, ...designerReaderOptions} + ).expect((res) => { + const result = JSON.parse(res.text); + assayDesignPayload.protocolId = result.data.protocolId; + const domains = result.data.domains; + assayDesignPayload.domains[0].domainId = domains[0].domainId; + assayDesignPayload.domains[0].domainURI = domains[0].domainURI; + assayDesignPayload.domains[1].domainId = domains[1].domainId; + assayDesignPayload.domains[1].domainURI = domains[1].domainURI; + assayDesignPayload.domains[2].domainId = domains[2].domainId; + assayDesignPayload.domains[2].domainURI = domains[2].domainURI; + return true; + }); + + for (let i = 0; i < badNameKeys.length; i++) + await verifyAssayDesignUpdateFailure(assayDesignPayload, badNameKeys[i], badNames[badNameKeys[i]]); + + }); + describe('Create/update/delete designs', () => { it('Designer can create, update and delete empty design, reader and editors cannot create/update/delete design.', async () => { const dataType = "ToBeDeleted"; - const assayDesignPaylod = getAssayDesignPayload(dataType, [], []); + const assayDesignPayload = getAssayDesignPayload(dataType, [], []); await server.post( 'assay', 'saveProtocol.api', - assayDesignPaylod, + assayDesignPayload, {...topFolderOptions, ...readerUserOptions} ).expect(403); await server.post( 'assay', 'saveProtocol.api', - assayDesignPaylod, + assayDesignPayload, {...topFolderOptions, ...editorUserOptions} ).expect(403); await server.post( 'assay', 'saveProtocol.api', - assayDesignPaylod, + assayDesignPayload, {...topFolderOptions, ...designerReaderOptions} ).expect((res) => { const result = JSON.parse(res.text); - assayDesignPaylod.protocolId = result.data.protocolId; + assayDesignPayload.protocolId = result.data.protocolId; const domains = result.data.domains; - assayDesignPaylod.domains[0].domainId = domains[0].domainId; - assayDesignPaylod.domains[0].domainURI = domains[0].domainURI; - assayDesignPaylod.domains[1].domainId = domains[1].domainId; - assayDesignPaylod.domains[1].domainURI = domains[1].domainURI; - assayDesignPaylod.domains[2].domainId = domains[2].domainId; - assayDesignPaylod.domains[2].domainURI = domains[2].domainURI; + assayDesignPayload.domains[0].domainId = domains[0].domainId; + assayDesignPayload.domains[0].domainURI = domains[0].domainURI; + assayDesignPayload.domains[1].domainId = domains[1].domainId; + assayDesignPayload.domains[1].domainURI = domains[1].domainURI; + assayDesignPayload.domains[2].domainId = domains[2].domainId; + assayDesignPayload.domains[2].domainURI = domains[2].domainURI; return true; }); - assayDesignPaylod.domains[2].fields = [resultPropField]; + assayDesignPayload.domains[2].fields = [resultPropField]; await server.post( 'assay', 'saveProtocol.api', - assayDesignPaylod, + assayDesignPayload, {...topFolderOptions, ...readerUserOptions} ).expect(403); await server.post( 'assay', 'saveProtocol.api', - assayDesignPaylod, + assayDesignPayload, {...topFolderOptions, ...editorUserOptions} ).expect(403); await server.post( 'assay', 'saveProtocol.api', - assayDesignPaylod, + assayDesignPayload, {...topFolderOptions, ...designerReaderOptions} ).expect(successfulResponse); const dataTypeRowId = await getAssayDesignRowIdByName(server, dataType, topFolderOptions); - expect(dataTypeRowId).toBe(assayDesignPaylod.protocolId); + expect(dataTypeRowId).toBe(assayDesignPayload.protocolId); - let deleteResult = await deleteAssayDesign(server, assayDesignPaylod.protocolId, topFolderOptions, readerUserOptions); + let deleteResult = await deleteAssayDesign(server, assayDesignPayload.protocolId, topFolderOptions, readerUserOptions); expect(deleteResult.status).toEqual(403); - deleteResult = await deleteAssayDesign(server, assayDesignPaylod.protocolId, topFolderOptions, editorUserOptions); + deleteResult = await deleteAssayDesign(server, assayDesignPayload.protocolId, topFolderOptions, editorUserOptions); expect(deleteResult.status).toEqual(403); - deleteResult = await deleteAssayDesign(server, assayDesignPaylod.protocolId, topFolderOptions, designerReaderOptions); + deleteResult = await deleteAssayDesign(server, assayDesignPayload.protocolId, topFolderOptions, designerReaderOptions); expect(deleteResult.status).toEqual(200); const removeddataType = await getAssayDesignRowIdByName(server, dataType, topFolderOptions); @@ -168,7 +245,7 @@ describe('Assay Designer - Permissions', () => { // it('Designer can update non-empty design but cannot delete non-empty design, admin can delete non-empty design', async () => { const dataType = "FailedDelete"; - const assayDesignPaylod = getAssayDesignPayload(dataType, [], []); + const assayDesignPayload = getAssayDesignPayload(dataType, [], []); await server.post( 'assay', 'saveProtocol.api', @@ -176,57 +253,57 @@ describe('Assay Designer - Permissions', () => { {...topFolderOptions, ...designerReaderOptions} ).expect((res) => { const result = JSON.parse(res.text); - assayDesignPaylod.protocolId = result.data.protocolId; + assayDesignPayload.protocolId = result.data.protocolId; const domains = result.data.domains; - assayDesignPaylod.domains[0].domainId = domains[0].domainId; - assayDesignPaylod.domains[0].domainURI = domains[0].domainURI; - assayDesignPaylod.domains[1].domainId = domains[1].domainId; - assayDesignPaylod.domains[1].domainURI = domains[1].domainURI; - assayDesignPaylod.domains[2].domainId = domains[2].domainId; - assayDesignPaylod.domains[2].domainURI = domains[2].domainURI; + assayDesignPayload.domains[0].domainId = domains[0].domainId; + assayDesignPayload.domains[0].domainURI = domains[0].domainURI; + assayDesignPayload.domains[1].domainId = domains[1].domainId; + assayDesignPayload.domains[1].domainURI = domains[1].domainURI; + assayDesignPayload.domains[2].domainId = domains[2].domainId; + assayDesignPayload.domains[2].domainURI = domains[2].domainURI; return true; }); - assayDesignPaylod.domains[2].fields = [resultPropField]; + assayDesignPayload.domains[2].fields = [resultPropField]; // create run in child folder - const { runId } = await importRun(server, assayDesignPaylod.protocolId, 'ChildRun', [{"Prop":"ABC"}], subfolder1Options, editorUserOptions); + const { runId } = await importRun(server, assayDesignPayload.protocolId, 'ChildRun', [{"Prop":"ABC"}], subfolder1Options, editorUserOptions); expect(runId > 0).toBeTruthy(); await server.post( 'assay', 'saveProtocol.api', - assayDesignPaylod, + assayDesignPayload, {...topFolderOptions, ...designerReaderOptions} ).expect(successfulResponse); // verify data exist in child prevent designer from delete design - let deleteResult = await deleteAssayDesign(server, assayDesignPaylod.protocolId, topFolderOptions, designerReaderOptions); + let deleteResult = await deleteAssayDesign(server, assayDesignPayload.protocolId, topFolderOptions, designerReaderOptions); expect(deleteResult.status).toEqual(403); - deleteResult = await deleteAssayDesign(server, assayDesignPaylod.protocolId, topFolderOptions, readerUserOptions); + deleteResult = await deleteAssayDesign(server, assayDesignPayload.protocolId, topFolderOptions, readerUserOptions); expect(deleteResult.status).toEqual(403); - deleteResult = await deleteAssayDesign(server, assayDesignPaylod.protocolId, topFolderOptions, editorUserOptions); + deleteResult = await deleteAssayDesign(server, assayDesignPayload.protocolId, topFolderOptions, editorUserOptions); expect(deleteResult.status).toEqual(403); let failedRemoveddataType = await getAssayDesignRowIdByName(server, dataType, topFolderOptions); - expect(failedRemoveddataType).toEqual(assayDesignPaylod.protocolId); + expect(failedRemoveddataType).toEqual(assayDesignPayload.protocolId); // create another run in top folder - await importRun(server, assayDesignPaylod.protocolId, 'TopRun', [{"Prop":"EFG"}], topFolderOptions, editorUserOptions); + await importRun(server, assayDesignPayload.protocolId, 'TopRun', [{"Prop":"EFG"}], topFolderOptions, editorUserOptions); // verify data exist in Top prevent designer from delete design - deleteResult = await deleteAssayDesign(server, assayDesignPaylod.protocolId, topFolderOptions, designerEditorOptions); + deleteResult = await deleteAssayDesign(server, assayDesignPayload.protocolId, topFolderOptions, designerEditorOptions); expect(deleteResult.status).toEqual(403); - deleteResult = await deleteAssayDesign(server, assayDesignPaylod.protocolId, topFolderOptions, readerUserOptions); + deleteResult = await deleteAssayDesign(server, assayDesignPayload.protocolId, topFolderOptions, readerUserOptions); expect(deleteResult.status).toEqual(403); - deleteResult = await deleteAssayDesign(server, assayDesignPaylod.protocolId, topFolderOptions, editorUserOptions); + deleteResult = await deleteAssayDesign(server, assayDesignPayload.protocolId, topFolderOptions, editorUserOptions); expect(deleteResult.status).toEqual(403); failedRemoveddataType = await getAssayDesignRowIdByName(server, dataType, topFolderOptions); - expect(failedRemoveddataType).toEqual(assayDesignPaylod.protocolId); + expect(failedRemoveddataType).toEqual(assayDesignPayload.protocolId); //admin can delete design with data - deleteResult = await deleteAssayDesign(server, assayDesignPaylod.protocolId, topFolderOptions, adminOptions); + deleteResult = await deleteAssayDesign(server, assayDesignPayload.protocolId, topFolderOptions, adminOptions); expect(deleteResult.status).toEqual(200); const removedDataType = await getAssayDesignRowIdByName(server, dataType, topFolderOptions); diff --git a/experiment/src/client/test/integration/DataClassCrud.ispec.ts b/experiment/src/client/test/integration/DataClassCrud.ispec.ts index 4b9a46497a4..c244332b8c4 100644 --- a/experiment/src/client/test/integration/DataClassCrud.ispec.ts +++ b/experiment/src/client/test/integration/DataClassCrud.ispec.ts @@ -1,6 +1,7 @@ import { hookServer, RequestOptions, successfulResponse } from '@labkey/test'; import mock from 'mock-fs'; import { + checkDomainName, checkLackDesignerOrReaderPerm, createSource, deleteSourceType, @@ -68,11 +69,15 @@ afterEach(() => { mock.restore(); }); -describe('Data Class Designer - Permissions', () => { +describe('Data Class Designer', () => { it('Lack designer or Reader permission', async () => { await checkLackDesignerOrReaderPerm(server, 'DataClass', topFolderOptions, readerUserOptions, editorUserOptions, designerOptions); }); + it('Data class name validation', async () => { + await checkDomainName(server, 'DataClass', true, topFolderOptions, designerReaderOptions); + }); + describe('Create/update/delete designs', () => { it('Designer can create, update and delete empty design, reader and editors cannot create/update/delete design', async () => { const dataType = "ToDelete"; diff --git a/experiment/src/client/test/integration/SampleTypeCrud.ispec.ts b/experiment/src/client/test/integration/SampleTypeCrud.ispec.ts index 8bc14116386..bf8846227d1 100644 --- a/experiment/src/client/test/integration/SampleTypeCrud.ispec.ts +++ b/experiment/src/client/test/integration/SampleTypeCrud.ispec.ts @@ -1,6 +1,7 @@ import { ExperimentCRUDUtils, hookServer, RequestOptions, successfulResponse } from '@labkey/test'; import mock from 'mock-fs'; import { + checkDomainName, checkLackDesignerOrReaderPerm, createSample, deleteSampleType, @@ -136,12 +137,16 @@ afterEach(() => { mock.restore(); }); -describe('Sample Type Designer - Permissions', () => { +describe('Sample Type Designer', () => { it('Lack designer or Reader permission', async () => { await checkLackDesignerOrReaderPerm(server, 'SampleSet', topFolderOptions, readerUserOptions, editorUserOptions, designerOptions); }); describe('Create/update/delete designs', () => { + it('Sample type name validation', async () => { + await checkDomainName(server, 'SampleSet', true, topFolderOptions, designerReaderOptions); + }); + it('Designer can create, update and delete empty design, reader and editors cannot create/update/delete design', async () => { const sampleType = "ToDelete"; let domainId = -1, domainURI = ''; diff --git a/experiment/src/client/test/integration/utils.ts b/experiment/src/client/test/integration/utils.ts index f4b63c6a389..7628cddaaf0 100644 --- a/experiment/src/client/test/integration/utils.ts +++ b/experiment/src/client/test/integration/utils.ts @@ -1,4 +1,10 @@ -import { ExperimentCRUDUtils, IntegrationTestServer, RequestOptions, successfulResponse } from '@labkey/test'; +import { + ExperimentCRUDUtils, + IntegrationTestServer, + RequestOptions, + selectRandomN, + successfulResponse +} from '@labkey/test'; import { caseInsensitive, SAMPLE_TYPE_DESIGNER_ROLE } from '@labkey/components'; import { PermissionRoles } from '@labkey/api'; @@ -299,6 +305,98 @@ export async function initProject(server: IntegrationTestServer, projectName: st } } +async function verifyDomainCreateFailure(server: IntegrationTestServer, domainType: string, badDomainName: string, error: string, folderOptions: RequestOptions, userOptions: RequestOptions) { + const field = domainType === 'SampleSet' ? { name: 'Name' } : { name: 'Prop' }; + const badDomainNameResp = await server.post('property', 'createDomain', { + kind: domainType, + domainDesign: { name: badDomainName, fields: [field] }, + options: { + name: badDomainName, + } + }, {...folderOptions, ...userOptions}); + + expect(badDomainNameResp['body']['success']).toBeFalsy(); + expect(badDomainNameResp['body']['exception']).toBe(error); +} + +async function verifyDomainUpdateFailure(server: IntegrationTestServer, domainId: number, domainURI: string, dataTypeRowId/*needed for updating dataclass*/: number, badDomainName: string, error: string, folderOptions: RequestOptions, userOptions: RequestOptions) { + const options = { + name: badDomainName + } + if (dataTypeRowId) + options['rowId'] = dataTypeRowId; + const updatedDomainPayload = { + domainId, + domainDesign: {name: badDomainName, domainId, domainURI}, + options + }; + + const badDomainNameResp = await server.post('property', 'saveDomain', updatedDomainPayload, {...folderOptions, ...userOptions}); + + expect(badDomainNameResp['body']['success']).toBeFalsy(); + expect(badDomainNameResp['body']['exception']).toBe(error); +} + +async function verifyDomainCreateSuccess(server: IntegrationTestServer, domainType: string, domainName: string, folderOptions: RequestOptions, userOptions: RequestOptions) { + let domainId, domainURI; + const field = domainType === 'SampleSet' ? { name: 'Name' } : { name: 'Prop' }; + await server.post('property', 'createDomain', { + kind: domainType, + domainDesign: { name: domainName, fields: [field] }, + options: { + name: domainName, + } + }, {...folderOptions, ...userOptions}).expect((result) => { + const domain = JSON.parse(result.text); + domainId = domain.domainId; + domainURI = domain.domainURI; + return true; + }); + return {domainId, domainURI}; +} + +const EMPTY_DOMAIN_NAME_MSG = 'Domain name must not be blank'; +export const ILLEGAL_DOMAIN_CHARSET = "<>[]{};,`\"~!@#$%^*=|?\\"; +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 = { + '': EMPTY_DOMAIN_NAME_MSG, + ' ': EMPTY_DOMAIN_NAME_MSG, + 'with\0nullCharacter': `Invalid ${domainType} name. Domain name must contain only valid unicode characters.`, + 'with\tnewLines': `Invalid ${domainType} name. Domain name may not contain 'tab', 'new line', or 'return' characters.`, + '.startWithDot': `Invalid ${domainType} name. Domain name must start with a letter or a number character.`, + ' startWithSpace': `Invalid ${domainType} name. Domain name must start with a letter or a number character.`, + ['c' + selectRandomN(ILLEGAL_DOMAIN_CHARSET.split(''), 2).join('')]: `Invalid ${domainType} name. Domain name may not contain any of these characters: ` + ILLEGAL_DOMAIN_CHARSET, + 'a -b': `Invalid ${domainType} name. Domain name may not contain space followed by dash.` + }; + if (supportNameExpression) { + badNames['withCounter'] = `Invalid ${domainType} name. 'withCounter' is a reserved name.`; + badNames['int:withCounter'] = `Invalid ${domainType} name. ':withCounter' is a reserved pattern.`; + badNames['drawdate:first'] = `Invalid ${domainType} name. ':first' is a reserved pattern.`; + } + + let badNameKeys = Object.keys(badNames); + for (let i = 0; i < badNameKeys.length; i++) + await verifyDomainCreateFailure(server, domainType, badNameKeys[i], badNames[badNameKeys[i]], folderOptions, userOptions); + + if (!supportNameExpression) + { + await verifyDomainCreateSuccess(server, domainType, 'withCounter', folderOptions, userOptions); + } + + const domainName = selectRandomN(alphaNumeric, 2).join('') + selectRandomN(LEGAL_CHARSET, 5).join(''); + const { domainId, domainURI } = await verifyDomainCreateSuccess(server, domainType, domainName, folderOptions, userOptions); + + let dataTypeRowId = 0; + if (domainType !== 'SampleSet') + dataTypeRowId = await getDataClassRowIdByName(server, domainName, folderOptions); + for (let i = 0; i < badNameKeys.length; i++){ + await verifyDomainUpdateFailure(server, domainId, domainURI, dataTypeRowId, badNameKeys[i], badNames[badNameKeys[i]], folderOptions, userOptions); + } + +} + export async function checkLackDesignerOrReaderPerm(server: IntegrationTestServer, domainType: string, topFolderOptions: RequestOptions, readerUserOptions: RequestOptions, editorUserOptions: RequestOptions, designerOptions: RequestOptions) { await server.post('property', 'createDomain', { kind: domainType, @@ -582,7 +680,7 @@ export const getAssayDesignPayload = (name: string, runFields: any[], resultFiel "domains": [ { "name": "Batch Fields", - "domainURI": "urn:lsid:${LSIDAuthority}:AssayDomain-Batch.Folder-${Container.RowId}:${AssayName}", + "domainURI": "urn:lsid:${LSIDAuthority}:AssayDomain-Batch.Folder-${Container.RowId}:${GpatAssayDBSeq}", "domainId": 0, "fields": [], "indices": [], @@ -591,7 +689,7 @@ export const getAssayDesignPayload = (name: string, runFields: any[], resultFiel }, { "name": "Run Fields", - "domainURI": "urn:lsid:${LSIDAuthority}:AssayDomain-Run.Folder-${Container.RowId}:${AssayName}", + "domainURI": "urn:lsid:${LSIDAuthority}:AssayDomain-Run.Folder-${Container.RowId}:${GpatAssayDBSeq}", "domainId": 0, "fields": runFields, "indices": [], @@ -600,7 +698,7 @@ export const getAssayDesignPayload = (name: string, runFields: any[], resultFiel }, { "name": "Data Fields", - "domainURI": "urn:lsid:${LSIDAuthority}:AssayDomain-Data.Folder-${Container.RowId}:${AssayName}", + "domainURI": "urn:lsid:${LSIDAuthority}:AssayDomain-Data.Folder-${Container.RowId}:${GpatAssayDBSeq}", "domainId": 0, "fields": resultFields, "indices": [], diff --git a/experiment/src/org/labkey/experiment/controllers/property/PropertyController.java b/experiment/src/org/labkey/experiment/controllers/property/PropertyController.java index 9800204d644..ac05e518834 100644 --- a/experiment/src/org/labkey/experiment/controllers/property/PropertyController.java +++ b/experiment/src/org/labkey/experiment/controllers/property/PropertyController.java @@ -1475,6 +1475,13 @@ private static ValidationException updateDomain(GWTDomain Date: Tue, 10 Dec 2024 18:00:07 -0800 Subject: [PATCH 04/16] Add restrictions on names of domains - automated tests --- .../test/tests/study/StudyDatasetsTest.java | 50 +++++++++++++------ 1 file changed, 36 insertions(+), 14 deletions(-) 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 e69569c7a22..fd61b931959 100644 --- a/study/test/src/org/labkey/test/tests/study/StudyDatasetsTest.java +++ b/study/test/src/org/labkey/test/tests/study/StudyDatasetsTest.java @@ -37,6 +37,7 @@ 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.openqa.selenium.WebElement; import java.util.ArrayList; @@ -135,18 +136,26 @@ public void preTest() @Test public void testDatasets() { - createDataset("A"); - renameDataset("A", "Original A", "A", "Original A", "XTest", "YTest", "ZTest"); - createDataset("A"); - deleteFields("A"); + createDataset(TestDataGenerator.randomInvalidDomainName(5), "Invalid StudyDatasetVisit name. Domain name must start with a letter or a number character."); - checkFieldsPresent("Original A", "YTest", "ZTest"); + String datasetA = TestDataGenerator.randomDomainName(); + createDataset(datasetA, null); + + String badDataSetName = TestDataGenerator.randomInvalidDomainName(5); + renameDataset("Invalid StudyDatasetVisit name. Domain name must start with a letter or a number character.", datasetA, badDataSetName, datasetA, badDataSetName, "XTest", "YTest", "ZTest"); + + String datasetAUpdated = TestDataGenerator.randomDomainName(); + renameDataset(null, datasetA, datasetAUpdated, datasetA, datasetAUpdated, "XTest", "YTest", "ZTest"); + createDataset(datasetA, null); + deleteFields(datasetA); + + checkFieldsPresent(datasetAUpdated, "YTest", "ZTest"); verifySideFilter(); verifyReportAndViewDatasetReferences(); - createDataset("B"); + createDataset("B", null); importDatasetData("B", DATASET_HEADER, DATASET_B_DATA, "All data"); checkDataElementsPresent("B", DATASET_B_DATA.split("\t|\n")); @@ -199,7 +208,7 @@ public void testDatasetSubjectId() } @LogMethod - protected void createDataset(@LoggedParam String name) + protected void createDataset(@LoggedParam String name, @Nullable String error) { DatasetDesignerPage definitionPage = _studyHelper.goToManageDatasets() .clickCreateNewDataset() @@ -209,11 +218,17 @@ protected void createDataset(@LoggedParam String name) panel.manuallyDefineFields("XTest"); panel.addField("YTest"); panel.addField("ZTest"); - definitionPage.clickSave(); + if (error == null) + definitionPage.clickSave(); + else + { + definitionPage.saveExpectFail(error); + definitionPage.clickCancel(); + } } @LogMethod - protected void renameDataset(String orgName, String newName, String orgLabel, String newLabel, String... fieldNames) + protected void renameDataset(String error, String orgName, String newName, String orgLabel, String newLabel, String... fieldNames) { DatasetDesignerPage editDatasetPage = _studyHelper.goToManageDatasets() .selectDatasetByName(orgName) @@ -228,7 +243,14 @@ protected void renameDataset(String orgName, String newName, String orgLabel, St assertTextPresent(fieldName); } - editDatasetPage.clickSave(); + if (error == null) + editDatasetPage.clickSave(); + else + { + editDatasetPage.saveExpectFail(error); + editDatasetPage.clickCancel(); + return; + } // fix dataset label references in report and view mappings for (Map.Entry entry : EXPECTED_REPORTS.entrySet()) @@ -442,10 +464,10 @@ private void verifyReportAndViewDatasetReferences() verifyCustomViewWithDatasetJoins("CPS-1: Screening Chemistry Panel", CUSTOM_VIEW_PRIVATE, false, false, "DataSets/DEM-1/DEMbdt", "DataSets/DEM-1/DEMsex"); // rename and relabel the datasets related to these reports and views - renameDataset("DEM-1", "demo", "DEM-1: Demographics", "Demographics"); - renameDataset("APX-1", "abbrphy", "APX-1: Abbreviated Physical Exam", "Abbreviated Physical Exam"); - renameDataset("ECI-1", "eligcrit", "ECI-1: Eligibility Criteria", "Eligibility Criteria"); - renameDataset("CPS-1", "scrchem", "CPS-1: Screening Chemistry Panel", "Screening Chemistry Panel"); + renameDataset(null, "DEM-1", "demo", "DEM-1: Demographics", "Demographics"); + renameDataset(null, "APX-1", "abbrphy", "APX-1: Abbreviated Physical Exam", "Abbreviated Physical Exam"); + renameDataset(null, "ECI-1", "eligcrit", "ECI-1: Eligibility Criteria", "Eligibility Criteria"); + renameDataset(null, "CPS-1", "scrchem", "CPS-1: Screening Chemistry Panel", "Screening Chemistry Panel"); // verify the reports and views dataset label/name references after dataset rename and relabel //verifyExpectedReportsAndViewsExist(); From a40e9ee00e85b631f05daa1c288b60caea8a7410 Mon Sep 17 00:00:00 2001 From: XingY Date: Tue, 10 Dec 2024 18:06:32 -0800 Subject: [PATCH 05/16] update version --- core/package-lock.json | 8 ++++---- core/package.json | 2 +- experiment/package-lock.json | 8 ++++---- experiment/package.json | 2 +- 4 files changed, 10 insertions(+), 10 deletions(-) diff --git a/core/package-lock.json b/core/package-lock.json index 2068c2a43a3..b1bc71449d7 100644 --- a/core/package-lock.json +++ b/core/package-lock.json @@ -8,7 +8,7 @@ "name": "labkey-core", "version": "0.0.0", "dependencies": { - "@labkey/components": "6.4.0", + "@labkey/components": "6.5.1-fb-domainNameValidation.1", "@labkey/themes": "1.4.0" }, "devDependencies": { @@ -3058,9 +3058,9 @@ } }, "node_modules/@labkey/components": { - "version": "6.4.0", - "resolved": "https://labkey.jfrog.io/artifactory/api/npm/libs-client/@labkey/components/-/@labkey/components-6.4.0.tgz", - "integrity": "sha512-S3HX8kc7yL9PXNmeAbn6hWrz/6Jl1xGNwdqpFrmvCvb/aQ8g+ydZFSBQd73EcxZsCsPUlewZbrOQ7gqRTADH7g==", + "version": "6.5.1-fb-domainNameValidation.1", + "resolved": "https://labkey.jfrog.io/artifactory/api/npm/libs-client/@labkey/components/-/@labkey/components-6.5.1-fb-domainNameValidation.1.tgz", + "integrity": "sha512-BuoM0l57Giy8JjjA4iRvJh3hdN/nkz2eB6a1Wkweyj+XDuG0+6IPfUiCM5TOqbbCUNvfbJE09YcY9ON9EUoBmQ==", "dependencies": { "@hello-pangea/dnd": "17.0.0", "@labkey/api": "1.36.0", diff --git a/core/package.json b/core/package.json index 90e691d74c3..8faec5086e5 100644 --- a/core/package.json +++ b/core/package.json @@ -54,7 +54,7 @@ } }, "dependencies": { - "@labkey/components": "6.4.0", + "@labkey/components": "6.5.1-fb-domainNameValidation.1", "@labkey/themes": "1.4.0" }, "devDependencies": { diff --git a/experiment/package-lock.json b/experiment/package-lock.json index fb9fbd96c56..89f07723d9e 100644 --- a/experiment/package-lock.json +++ b/experiment/package-lock.json @@ -8,7 +8,7 @@ "name": "experiment", "version": "0.0.0", "dependencies": { - "@labkey/components": "6.4.0" + "@labkey/components": "6.5.1-fb-domainNameValidation.1" }, "devDependencies": { "@labkey/build": "8.3.0", @@ -2864,9 +2864,9 @@ } }, "node_modules/@labkey/components": { - "version": "6.4.0", - "resolved": "https://labkey.jfrog.io/artifactory/api/npm/libs-client/@labkey/components/-/@labkey/components-6.4.0.tgz", - "integrity": "sha512-S3HX8kc7yL9PXNmeAbn6hWrz/6Jl1xGNwdqpFrmvCvb/aQ8g+ydZFSBQd73EcxZsCsPUlewZbrOQ7gqRTADH7g==", + "version": "6.5.1-fb-domainNameValidation.1", + "resolved": "https://labkey.jfrog.io/artifactory/api/npm/libs-client/@labkey/components/-/@labkey/components-6.5.1-fb-domainNameValidation.1.tgz", + "integrity": "sha512-BuoM0l57Giy8JjjA4iRvJh3hdN/nkz2eB6a1Wkweyj+XDuG0+6IPfUiCM5TOqbbCUNvfbJE09YcY9ON9EUoBmQ==", "dependencies": { "@hello-pangea/dnd": "17.0.0", "@labkey/api": "1.36.0", diff --git a/experiment/package.json b/experiment/package.json index 89cdb61b338..e3674c75815 100644 --- a/experiment/package.json +++ b/experiment/package.json @@ -13,7 +13,7 @@ "test-integration": "cross-env NODE_ENV=test jest --ci --runInBand -c test/js/jest.config.integration.js" }, "dependencies": { - "@labkey/components": "6.4.0" + "@labkey/components": "6.5.1-fb-domainNameValidation.1" }, "devDependencies": { "@labkey/build": "8.3.0", From b59f10ce8ad47b1a3fd2a4be8054144435e36897 Mon Sep 17 00:00:00 2001 From: XingY Date: Wed, 11 Dec 2024 12:10:56 -0800 Subject: [PATCH 06/16] fix name expression --- api/src/org/labkey/api/data/NameGenerator.java | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/api/src/org/labkey/api/data/NameGenerator.java b/api/src/org/labkey/api/data/NameGenerator.java index 7dac53aff33..f956d364ece 100644 --- a/api/src/org/labkey/api/data/NameGenerator.java +++ b/api/src/org/labkey/api/data/NameGenerator.java @@ -2318,12 +2318,8 @@ private void addInputs(String colName, @Nullable Map parentImportAliases) { String[] parts = colName.split("/", 2); - boolean isParentAliasInput = false; if (parts.length == 1 && parentImportAliases != null && parentImportAliases.containsKey(colName)) - { - isParentAliasInput = true; parts = parentImportAliases.get(colName).split("/", 2); - } if (parts.length == 2) { @@ -2339,6 +2335,7 @@ else if (parts[0].equalsIgnoreCase(ExpMaterial.MATERIAL_INPUT_PARENT)) Collection parents = parentNames(value, colName); inputs.get(INPUT_PARENT).addAll(parents); inputs.get(inputsCategory).addAll(parents); + // TODO, parents broken Set dataTypeAltNames = new HashSet<>(); dataTypeAltNames.add(decodedDataType); @@ -2351,7 +2348,7 @@ else if (parts[0].equalsIgnoreCase(ExpMaterial.MATERIAL_INPUT_PARENT)) } // if import aliases are defined, also add in the inputs under the aliases in case those are used in the name expression - if (parentImportAliases != null && !isParentAliasInput) + if (parentImportAliases != null) { Optional> aliasEntry = parentImportAliases.entrySet().stream().filter(entry -> entry.getValue().equalsIgnoreCase(colName)).findFirst(); aliasEntry.ifPresent(entry -> { From 931b5f612a74351ca62433b1158b87765c4c0354 Mon Sep 17 00:00:00 2001 From: XingY Date: Wed, 11 Dec 2024 15:05:48 -0800 Subject: [PATCH 07/16] fix import using parent alias --- api/src/org/labkey/api/data/NameGenerator.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/api/src/org/labkey/api/data/NameGenerator.java b/api/src/org/labkey/api/data/NameGenerator.java index f956d364ece..ab1e7e55697 100644 --- a/api/src/org/labkey/api/data/NameGenerator.java +++ b/api/src/org/labkey/api/data/NameGenerator.java @@ -2350,6 +2350,8 @@ else if (parts[0].equalsIgnoreCase(ExpMaterial.MATERIAL_INPUT_PARENT)) // if import aliases are defined, also add in the inputs under the aliases in case those are used in the name expression if (parentImportAliases != null) { + if (parentImportAliases.containsKey(colName)) + inputs.computeIfAbsent(colName, (s) -> new LinkedHashSet<>()).addAll(parents); Optional> aliasEntry = parentImportAliases.entrySet().stream().filter(entry -> entry.getValue().equalsIgnoreCase(colName)).findFirst(); aliasEntry.ifPresent(entry -> { inputs.computeIfAbsent(entry.getKey(), (s) -> new LinkedHashSet<>()).addAll(parents); From 26539f7d5b3dc2e750f120a2bbae78b30a7e1e26 Mon Sep 17 00:00:00 2001 From: XingY Date: Wed, 11 Dec 2024 17:04:11 -0800 Subject: [PATCH 08/16] fix api usage --- .../experiment/controllers/property/PropertyController.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/experiment/src/org/labkey/experiment/controllers/property/PropertyController.java b/experiment/src/org/labkey/experiment/controllers/property/PropertyController.java index ac05e518834..b0146afdbce 100644 --- a/experiment/src/org/labkey/experiment/controllers/property/PropertyController.java +++ b/experiment/src/org/labkey/experiment/controllers/property/PropertyController.java @@ -1475,7 +1475,7 @@ private static ValidationException updateDomain(GWTDomain Date: Wed, 11 Dec 2024 18:34:18 -0800 Subject: [PATCH 09/16] code review changes --- .../org/labkey/api/data/NameGenerator.java | 1 - .../api/exp/api/SampleTypeDomainKind.java | 2 +- .../labkey/api/exp/property/DomainKind.java | 2 +- .../labkey/api/exp/property/DomainUtil.java | 19 +++++++------ .../labkey/api/util/StringUtilsLabKey.java | 2 +- .../test/integration/AssayDesignCrud.ispec.ts | 18 ++++++------- .../src/client/test/integration/utils.ts | 27 +++++++++---------- .../experiment/api/DataClassDomainKind.java | 2 +- .../property/PropertyController.java | 2 +- .../test/tests/study/StudyDatasetsTest.java | 8 +++--- 10 files changed, 40 insertions(+), 43 deletions(-) diff --git a/api/src/org/labkey/api/data/NameGenerator.java b/api/src/org/labkey/api/data/NameGenerator.java index ab1e7e55697..9e929bf925d 100644 --- a/api/src/org/labkey/api/data/NameGenerator.java +++ b/api/src/org/labkey/api/data/NameGenerator.java @@ -2335,7 +2335,6 @@ else if (parts[0].equalsIgnoreCase(ExpMaterial.MATERIAL_INPUT_PARENT)) Collection parents = parentNames(value, colName); inputs.get(INPUT_PARENT).addAll(parents); inputs.get(inputsCategory).addAll(parents); - // TODO, parents broken Set dataTypeAltNames = new HashSet<>(); dataTypeAltNames.add(decodedDataType); diff --git a/api/src/org/labkey/api/exp/api/SampleTypeDomainKind.java b/api/src/org/labkey/api/exp/api/SampleTypeDomainKind.java index 2477041e20c..dd13480e4c1 100644 --- a/api/src/org/labkey/api/exp/api/SampleTypeDomainKind.java +++ b/api/src/org/labkey/api/exp/api/SampleTypeDomainKind.java @@ -673,7 +673,7 @@ public boolean supportsPhiLevel() } @Override - public boolean supportNamingPattern() + public boolean supportsNamingPattern() { return true; } diff --git a/api/src/org/labkey/api/exp/property/DomainKind.java b/api/src/org/labkey/api/exp/property/DomainKind.java index d17488b99ff..d21efba81d2 100644 --- a/api/src/org/labkey/api/exp/property/DomainKind.java +++ b/api/src/org/labkey/api/exp/property/DomainKind.java @@ -411,7 +411,7 @@ public boolean supportsPhiLevel() return false; } - public boolean supportNamingPattern() + public boolean supportsNamingPattern() { return false; } diff --git a/api/src/org/labkey/api/exp/property/DomainUtil.java b/api/src/org/labkey/api/exp/property/DomainUtil.java index 860ffbbf24a..fb8ec6a4506 100644 --- a/api/src/org/labkey/api/exp/property/DomainUtil.java +++ b/api/src/org/labkey/api/exp/property/DomainUtil.java @@ -706,7 +706,7 @@ public static Domain createDomain( if (domain.getName() != null) { - String domainNameError = validateDomainName(domain.getName(), kind.getKindName(), kind.supportNamingPattern()); + String domainNameError = validateDomainName(domain.getName(), kind.getKindName(), kind.supportsNamingPattern()); if (!StringUtils.isEmpty(domainNameError)) throw new IllegalArgumentException(domainNameError); } @@ -768,7 +768,7 @@ public static ValidationException updateDomainDescriptor(GWTDomain { if (exception !== error) console.log(badDomainName); - expect(exception).toBe(error); + expect(exception).toBe(error.replace("REPLACE", badDomainName)); } async function verifyAssayDesignUpdateFailure(payload: any, badDomainName: string, error: string) { @@ -125,18 +125,18 @@ describe('Assay Designer - Permissions', () => { if (exception !== error) console.log(badDomainName); - expect(exception).toBe(error); + expect(exception).toBe(error.replace("REPLACE", badDomainName)); } it('Assay design name validation', async () => { const badNames = { - '': 'Domain name must not be blank', - ' ': 'Domain name must not be blank', - 'with\0nullCharacter': `Invalid Assay Design name. Domain name must contain only valid unicode characters.`, - 'with\tnewLines': `Invalid Assay Design name. Domain name may not contain 'tab', 'new line', or 'return' characters.`, - '.startWithDot': `Invalid Assay Design name. Domain name must start with a letter or a number character.`, - ['c' + selectRandomN(ILLEGAL_DOMAIN_CHARSET.split(''), 2).join('')]: `Invalid Assay Design name. Domain name may not contain any of these characters: ` + ILLEGAL_DOMAIN_CHARSET, - 'a -b': `Invalid Assay Design name. Domain name may not contain space followed by dash.` + '': 'Assay Design name must not be blank', + ' ': 'Assay Design name must not be blank', + 'with\0nullCharacter': `Invalid Assay Design name "REPLACE". Assay Design name must contain only valid unicode characters.`, + 'with\tnewLines': `Invalid Assay Design name "REPLACE". Assay Design name may not contain 'tab', 'new line', or 'return' characters.`, + '.startWithDot': `Invalid Assay Design name "REPLACE". Assay Design name must start with a letter or a number.`, + ['c' + selectRandomN(ILLEGAL_DOMAIN_CHARSET.split(''), 2).join('')]: `Invalid Assay Design name "REPLACE". Assay Design name may not contain any of these characters: ` + ILLEGAL_DOMAIN_CHARSET, + 'a -b': `Invalid Assay Design name "REPLACE". Assay Design name may not contain space followed by dash.` }; let badNameKeys = Object.keys(badNames); diff --git a/experiment/src/client/test/integration/utils.ts b/experiment/src/client/test/integration/utils.ts index 7628cddaaf0..f7b00b12c05 100644 --- a/experiment/src/client/test/integration/utils.ts +++ b/experiment/src/client/test/integration/utils.ts @@ -316,7 +316,7 @@ async function verifyDomainCreateFailure(server: IntegrationTestServer, domainTy }, {...folderOptions, ...userOptions}); expect(badDomainNameResp['body']['success']).toBeFalsy(); - expect(badDomainNameResp['body']['exception']).toBe(error); + expect(badDomainNameResp['body']['exception']).toBe(error.replace("REPLACE", badDomainName)); } async function verifyDomainUpdateFailure(server: IntegrationTestServer, domainId: number, domainURI: string, dataTypeRowId/*needed for updating dataclass*/: number, badDomainName: string, error: string, folderOptions: RequestOptions, userOptions: RequestOptions) { @@ -334,7 +334,7 @@ async function verifyDomainUpdateFailure(server: IntegrationTestServer, domainId const badDomainNameResp = await server.post('property', 'saveDomain', updatedDomainPayload, {...folderOptions, ...userOptions}); expect(badDomainNameResp['body']['success']).toBeFalsy(); - expect(badDomainNameResp['body']['exception']).toBe(error); + expect(badDomainNameResp['body']['exception']).toBe(error.replace("REPLACE", badDomainName)); } async function verifyDomainCreateSuccess(server: IntegrationTestServer, domainType: string, domainName: string, folderOptions: RequestOptions, userOptions: RequestOptions) { @@ -355,25 +355,24 @@ async function verifyDomainCreateSuccess(server: IntegrationTestServer, domainTy return {domainId, domainURI}; } -const EMPTY_DOMAIN_NAME_MSG = 'Domain name must not be blank'; export const ILLEGAL_DOMAIN_CHARSET = "<>[]{};,`\"~!@#$%^*=|?\\"; 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 = { - '': EMPTY_DOMAIN_NAME_MSG, - ' ': EMPTY_DOMAIN_NAME_MSG, - 'with\0nullCharacter': `Invalid ${domainType} name. Domain name must contain only valid unicode characters.`, - 'with\tnewLines': `Invalid ${domainType} name. Domain name may not contain 'tab', 'new line', or 'return' characters.`, - '.startWithDot': `Invalid ${domainType} name. Domain name must start with a letter or a number character.`, - ' startWithSpace': `Invalid ${domainType} name. Domain name must start with a letter or a number character.`, - ['c' + selectRandomN(ILLEGAL_DOMAIN_CHARSET.split(''), 2).join('')]: `Invalid ${domainType} name. Domain name may not contain any of these characters: ` + ILLEGAL_DOMAIN_CHARSET, - 'a -b': `Invalid ${domainType} name. Domain name may not contain space followed by dash.` + '': `${domainType} name must not be blank.`, + ' ': `${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.`, + ' startWithSpace': `Invalid ${domainType} name "REPLACE". ${domainType} name must start with a letter or a number.`, + ['c' + selectRandomN(ILLEGAL_DOMAIN_CHARSET.split(''), 2).join('')]: `Invalid ${domainType} name "REPLACE". ${domainType} name may not contain any of these characters: ` + ILLEGAL_DOMAIN_CHARSET, + 'a -b': `Invalid ${domainType} name "REPLACE". ${domainType} name may not contain space followed by dash.` }; if (supportNameExpression) { - badNames['withCounter'] = `Invalid ${domainType} name. 'withCounter' is a reserved name.`; - badNames['int:withCounter'] = `Invalid ${domainType} name. ':withCounter' is a reserved pattern.`; - badNames['drawdate:first'] = `Invalid ${domainType} name. ':first' is a reserved pattern.`; + badNames['withCounter'] = `Invalid ${domainType} name "REPLACE". 'withCounter' is a reserved name.`; + badNames['int:withCounter'] = `Invalid ${domainType} name "REPLACE". ':withCounter' is a reserved pattern.`; + badNames['drawdate:first'] = `Invalid ${domainType} name "REPLACE". ':first' is a reserved pattern.`; } let badNameKeys = Object.keys(badNames); diff --git a/experiment/src/org/labkey/experiment/api/DataClassDomainKind.java b/experiment/src/org/labkey/experiment/api/DataClassDomainKind.java index 6293cecad7b..ef22af86c2e 100644 --- a/experiment/src/org/labkey/experiment/api/DataClassDomainKind.java +++ b/experiment/src/org/labkey/experiment/api/DataClassDomainKind.java @@ -496,7 +496,7 @@ public boolean supportsPhiLevel() } @Override - public boolean supportNamingPattern() + public boolean supportsNamingPattern() { return true; } diff --git a/experiment/src/org/labkey/experiment/controllers/property/PropertyController.java b/experiment/src/org/labkey/experiment/controllers/property/PropertyController.java index b0146afdbce..9e0ac2b3f6d 100644 --- a/experiment/src/org/labkey/experiment/controllers/property/PropertyController.java +++ b/experiment/src/org/labkey/experiment/controllers/property/PropertyController.java @@ -1477,7 +1477,7 @@ private static ValidationException updateDomain(GWTDomain Date: Wed, 11 Dec 2024 18:35:19 -0800 Subject: [PATCH 10/16] code review changes --- core/package-lock.json | 8 ++++---- core/package.json | 2 +- experiment/package-lock.json | 8 ++++---- experiment/package.json | 2 +- 4 files changed, 10 insertions(+), 10 deletions(-) diff --git a/core/package-lock.json b/core/package-lock.json index b1bc71449d7..e597fe4afcc 100644 --- a/core/package-lock.json +++ b/core/package-lock.json @@ -8,7 +8,7 @@ "name": "labkey-core", "version": "0.0.0", "dependencies": { - "@labkey/components": "6.5.1-fb-domainNameValidation.1", + "@labkey/components": "6.5.2-fb-domainNameValidation.1", "@labkey/themes": "1.4.0" }, "devDependencies": { @@ -3058,9 +3058,9 @@ } }, "node_modules/@labkey/components": { - "version": "6.5.1-fb-domainNameValidation.1", - "resolved": "https://labkey.jfrog.io/artifactory/api/npm/libs-client/@labkey/components/-/@labkey/components-6.5.1-fb-domainNameValidation.1.tgz", - "integrity": "sha512-BuoM0l57Giy8JjjA4iRvJh3hdN/nkz2eB6a1Wkweyj+XDuG0+6IPfUiCM5TOqbbCUNvfbJE09YcY9ON9EUoBmQ==", + "version": "6.5.2-fb-domainNameValidation.1", + "resolved": "https://labkey.jfrog.io/artifactory/api/npm/libs-client/@labkey/components/-/@labkey/components-6.5.2-fb-domainNameValidation.1.tgz", + "integrity": "sha512-ttc2hu8RcYroBRfQbgy1mFGXGsX71wgtENfpwevwd2YY4ciaHLpRk/IC7iyo/HW1Rwu1lpcuX615ETUu2gOdWA==", "dependencies": { "@hello-pangea/dnd": "17.0.0", "@labkey/api": "1.36.0", diff --git a/core/package.json b/core/package.json index 8faec5086e5..9737939abc8 100644 --- a/core/package.json +++ b/core/package.json @@ -54,7 +54,7 @@ } }, "dependencies": { - "@labkey/components": "6.5.1-fb-domainNameValidation.1", + "@labkey/components": "6.5.2-fb-domainNameValidation.1", "@labkey/themes": "1.4.0" }, "devDependencies": { diff --git a/experiment/package-lock.json b/experiment/package-lock.json index 89f07723d9e..50c3dc15a80 100644 --- a/experiment/package-lock.json +++ b/experiment/package-lock.json @@ -8,7 +8,7 @@ "name": "experiment", "version": "0.0.0", "dependencies": { - "@labkey/components": "6.5.1-fb-domainNameValidation.1" + "@labkey/components": "6.5.2-fb-domainNameValidation.1" }, "devDependencies": { "@labkey/build": "8.3.0", @@ -2864,9 +2864,9 @@ } }, "node_modules/@labkey/components": { - "version": "6.5.1-fb-domainNameValidation.1", - "resolved": "https://labkey.jfrog.io/artifactory/api/npm/libs-client/@labkey/components/-/@labkey/components-6.5.1-fb-domainNameValidation.1.tgz", - "integrity": "sha512-BuoM0l57Giy8JjjA4iRvJh3hdN/nkz2eB6a1Wkweyj+XDuG0+6IPfUiCM5TOqbbCUNvfbJE09YcY9ON9EUoBmQ==", + "version": "6.5.2-fb-domainNameValidation.1", + "resolved": "https://labkey.jfrog.io/artifactory/api/npm/libs-client/@labkey/components/-/@labkey/components-6.5.2-fb-domainNameValidation.1.tgz", + "integrity": "sha512-ttc2hu8RcYroBRfQbgy1mFGXGsX71wgtENfpwevwd2YY4ciaHLpRk/IC7iyo/HW1Rwu1lpcuX615ETUu2gOdWA==", "dependencies": { "@hello-pangea/dnd": "17.0.0", "@labkey/api": "1.36.0", diff --git a/experiment/package.json b/experiment/package.json index e3674c75815..c60cb8568a1 100644 --- a/experiment/package.json +++ b/experiment/package.json @@ -13,7 +13,7 @@ "test-integration": "cross-env NODE_ENV=test jest --ci --runInBand -c test/js/jest.config.integration.js" }, "dependencies": { - "@labkey/components": "6.5.1-fb-domainNameValidation.1" + "@labkey/components": "6.5.2-fb-domainNameValidation.1" }, "devDependencies": { "@labkey/build": "8.3.0", From 1063885b2d851dd3dbd59ce894dda3cb46d69448 Mon Sep 17 00:00:00 2001 From: XingY Date: Thu, 12 Dec 2024 15:39:46 -0800 Subject: [PATCH 11/16] fix naming expression --- api/src/org/labkey/api/data/NameGenerator.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/api/src/org/labkey/api/data/NameGenerator.java b/api/src/org/labkey/api/data/NameGenerator.java index 9e929bf925d..863df7d14e1 100644 --- a/api/src/org/labkey/api/data/NameGenerator.java +++ b/api/src/org/labkey/api/data/NameGenerator.java @@ -747,7 +747,7 @@ else if (value instanceof JSONArray jsonArray) public static boolean isParentInput(Object token, @Nullable Map importAliases, @Nullable String currentDataTypeName, Container container, User user) { - return isParentInputToken(token, importAliases, false) || isParentInputWithDataType(token.toString().split("/"), currentDataTypeName, false, container, user); + return isParentInputToken(token, importAliases, false) || isParentInputWithDataType(token.toString().split("/", 2), currentDataTypeName, false, container, user); } public static boolean isParentLookup(List fieldParts, @Nullable Map importAliases, @Nullable String currentDataTypeName, Container container, User user) @@ -1167,7 +1167,7 @@ else if (pt != null) if (!isAncestorSearch) parentLookupFields.computeIfAbsent(dataTypeToken, (s) -> new ArrayList<>()).add(fieldParts.get(1)); - String[] inputParts = dataTypeToken.split("/"); + String[] inputParts = dataTypeToken.split("/", 2); lookupValuePreview = getParentLookupTokenPreview(_currentDataTypeName, fkTok, inputParts[0], inputParts[1], ancestorPartOption, lookupField, user, dataClassNames, sampleTypeNames); } else if (!isParentAlias && fieldParts.size() <= 3) From f610bde015ed622045003b58c2c2ad2e768ff41b Mon Sep 17 00:00:00 2001 From: XingY Date: Thu, 12 Dec 2024 15:43:36 -0800 Subject: [PATCH 12/16] fix test --- .../src/client/test/integration/AssayDesignCrud.ispec.ts | 7 ++----- experiment/src/client/test/integration/utils.ts | 6 ++++-- 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/experiment/src/client/test/integration/AssayDesignCrud.ispec.ts b/experiment/src/client/test/integration/AssayDesignCrud.ispec.ts index df58b69a4f1..ed526db1678 100644 --- a/experiment/src/client/test/integration/AssayDesignCrud.ispec.ts +++ b/experiment/src/client/test/integration/AssayDesignCrud.ispec.ts @@ -122,16 +122,13 @@ describe('Assay Designer - Permissions', () => { exception = JSON.parse(resp.text).exception; }) - if (exception !== error) - console.log(badDomainName); - expect(exception).toBe(error.replace("REPLACE", badDomainName)); } it('Assay design name validation', async () => { const badNames = { - '': 'Assay Design name must not be blank', - ' ': 'Assay Design name must not be blank', + '': 'Assay Design name must not be blank.', + ' ': 'Assay Design name must not be blank.', 'with\0nullCharacter': `Invalid Assay Design name "REPLACE". Assay Design name must contain only valid unicode characters.`, 'with\tnewLines': `Invalid Assay Design name "REPLACE". Assay Design name may not contain 'tab', 'new line', or 'return' characters.`, '.startWithDot': `Invalid Assay Design name "REPLACE". Assay Design name must start with a letter or a number.`, diff --git a/experiment/src/client/test/integration/utils.ts b/experiment/src/client/test/integration/utils.ts index f7b00b12c05..ed5a09f524a 100644 --- a/experiment/src/client/test/integration/utils.ts +++ b/experiment/src/client/test/integration/utils.ts @@ -360,8 +360,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} name must not be blank.`, - ' ': `${domainType} name must not be blank.`, + '': 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.`, '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.`, @@ -390,6 +390,8 @@ export async function checkDomainName(server: IntegrationTestServer, domainType: let dataTypeRowId = 0; if (domainType !== 'SampleSet') dataTypeRowId = await getDataClassRowIdByName(server, domainName, folderOptions); + badNames[''] = `${domainType} name must not be blank.`; + badNames[' '] = `${domainType} name must not be blank.`; for (let i = 0; i < badNameKeys.length; i++){ await verifyDomainUpdateFailure(server, domainId, domainURI, dataTypeRowId, badNameKeys[i], badNames[badNameKeys[i]], folderOptions, userOptions); } From 750270fbf8f25be664aee4e3fa9f1fe51f3f068c Mon Sep 17 00:00:00 2001 From: XingY Date: Thu, 12 Dec 2024 16:36:02 -0800 Subject: [PATCH 13/16] fix cross type import --- experiment/src/org/labkey/experiment/ExpDataIterators.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/experiment/src/org/labkey/experiment/ExpDataIterators.java b/experiment/src/org/labkey/experiment/ExpDataIterators.java index a4efc998fbf..0017d04f7b2 100644 --- a/experiment/src/org/labkey/experiment/ExpDataIterators.java +++ b/experiment/src/org/labkey/experiment/ExpDataIterators.java @@ -2962,7 +2962,7 @@ private TypeData createDataClassHeaderRow(ExpDataClass dataClass, Container cont header.add(name); } - File dataFile = FileUtil.createTempFile("~importSplit-", container.getRowId() + dataClass.getName() + ".tsv"); + File dataFile = FileUtil.createTempFile("~importSplit-", container.getRowId() + FileUtil.makeLegalName(dataClass.getName()) + ".tsv"); List dataRows = new ArrayList(); dataRows.add(StringUtils.join(header, "\t")); @@ -2980,7 +2980,7 @@ private TypeData createSampleHeaderRow(ExpSampleTypeImpl sampleType, Container c _context.getErrors().addRowError(new ValidationException("Table for sample type '" + sampleType.getName() + "' not found.")); return null; } - File dataFile = FileUtil.createTempFile("~importSplit-", container.getRowId() + sampleType.getName() + ".tsv"); + File dataFile = FileUtil.createTempFile("~importSplit-", container.getRowId() + FileUtil.makeLegalName(sampleType.getName()) + ".tsv"); Set validFields = new CaseInsensitiveHashSet(); samplesTable.getColumns().forEach(column -> { if (!IGNORED_FIELD_NAMES.contains(column.getName())) From e2695161f57f04e3f6d89011c25e7a800e07b5d7 Mon Sep 17 00:00:00 2001 From: XingY Date: Thu, 12 Dec 2024 18:03:08 -0800 Subject: [PATCH 14/16] merge from develop --- core/package-lock.json | 8 ++++---- core/package.json | 2 +- experiment/package-lock.json | 8 ++++---- experiment/package.json | 2 +- 4 files changed, 10 insertions(+), 10 deletions(-) diff --git a/core/package-lock.json b/core/package-lock.json index e597fe4afcc..5edcbff5a6c 100644 --- a/core/package-lock.json +++ b/core/package-lock.json @@ -8,7 +8,7 @@ "name": "labkey-core", "version": "0.0.0", "dependencies": { - "@labkey/components": "6.5.2-fb-domainNameValidation.1", + "@labkey/components": "6.5.3-fb-domainNameValidation.1", "@labkey/themes": "1.4.0" }, "devDependencies": { @@ -3058,9 +3058,9 @@ } }, "node_modules/@labkey/components": { - "version": "6.5.2-fb-domainNameValidation.1", - "resolved": "https://labkey.jfrog.io/artifactory/api/npm/libs-client/@labkey/components/-/@labkey/components-6.5.2-fb-domainNameValidation.1.tgz", - "integrity": "sha512-ttc2hu8RcYroBRfQbgy1mFGXGsX71wgtENfpwevwd2YY4ciaHLpRk/IC7iyo/HW1Rwu1lpcuX615ETUu2gOdWA==", + "version": "6.5.3-fb-domainNameValidation.1", + "resolved": "https://labkey.jfrog.io/artifactory/api/npm/libs-client/@labkey/components/-/@labkey/components-6.5.3-fb-domainNameValidation.1.tgz", + "integrity": "sha512-UUuJYcBlzTrzo3faghKz6lk/zT6xFaSC2pxymIMrpOiXLJVNtlL0BoWZafboU/JUNU8Ac5JI5MDf873n04iCLw==", "dependencies": { "@hello-pangea/dnd": "17.0.0", "@labkey/api": "1.36.0", diff --git a/core/package.json b/core/package.json index 9737939abc8..abeb84ab0a4 100644 --- a/core/package.json +++ b/core/package.json @@ -54,7 +54,7 @@ } }, "dependencies": { - "@labkey/components": "6.5.2-fb-domainNameValidation.1", + "@labkey/components": "6.5.3-fb-domainNameValidation.1", "@labkey/themes": "1.4.0" }, "devDependencies": { diff --git a/experiment/package-lock.json b/experiment/package-lock.json index 50c3dc15a80..09f57269825 100644 --- a/experiment/package-lock.json +++ b/experiment/package-lock.json @@ -8,7 +8,7 @@ "name": "experiment", "version": "0.0.0", "dependencies": { - "@labkey/components": "6.5.2-fb-domainNameValidation.1" + "@labkey/components": "6.5.3-fb-domainNameValidation.1" }, "devDependencies": { "@labkey/build": "8.3.0", @@ -2864,9 +2864,9 @@ } }, "node_modules/@labkey/components": { - "version": "6.5.2-fb-domainNameValidation.1", - "resolved": "https://labkey.jfrog.io/artifactory/api/npm/libs-client/@labkey/components/-/@labkey/components-6.5.2-fb-domainNameValidation.1.tgz", - "integrity": "sha512-ttc2hu8RcYroBRfQbgy1mFGXGsX71wgtENfpwevwd2YY4ciaHLpRk/IC7iyo/HW1Rwu1lpcuX615ETUu2gOdWA==", + "version": "6.5.3-fb-domainNameValidation.1", + "resolved": "https://labkey.jfrog.io/artifactory/api/npm/libs-client/@labkey/components/-/@labkey/components-6.5.3-fb-domainNameValidation.1.tgz", + "integrity": "sha512-UUuJYcBlzTrzo3faghKz6lk/zT6xFaSC2pxymIMrpOiXLJVNtlL0BoWZafboU/JUNU8Ac5JI5MDf873n04iCLw==", "dependencies": { "@hello-pangea/dnd": "17.0.0", "@labkey/api": "1.36.0", diff --git a/experiment/package.json b/experiment/package.json index c60cb8568a1..a88c9cee628 100644 --- a/experiment/package.json +++ b/experiment/package.json @@ -13,7 +13,7 @@ "test-integration": "cross-env NODE_ENV=test jest --ci --runInBand -c test/js/jest.config.integration.js" }, "dependencies": { - "@labkey/components": "6.5.2-fb-domainNameValidation.1" + "@labkey/components": "6.5.3-fb-domainNameValidation.1" }, "devDependencies": { "@labkey/build": "8.3.0", From b021d3857b9baffa00cdbe2c7ea5755e2fde3cfb Mon Sep 17 00:00:00 2001 From: XingY Date: Sun, 15 Dec 2024 15:52:03 -0800 Subject: [PATCH 15/16] fix timeline event --- api/src/org/labkey/api/audit/AuditHandler.java | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/api/src/org/labkey/api/audit/AuditHandler.java b/api/src/org/labkey/api/audit/AuditHandler.java index 44e4ea922e4..154c58029b1 100644 --- a/api/src/org/labkey/api/audit/AuditHandler.java +++ b/api/src/org/labkey/api/audit/AuditHandler.java @@ -11,6 +11,7 @@ import org.labkey.api.exp.api.ExpMaterial; import org.labkey.api.exp.api.ExperimentService; import org.labkey.api.gwt.client.AuditBehaviorType; +import org.labkey.api.query.QueryKey; import org.labkey.api.query.QueryService; import org.labkey.api.security.User; import org.labkey.api.util.Pair; @@ -72,11 +73,21 @@ static Pair, Map> getOldAndNewRecordForMerge .orElse(key); String lcName = nameFromAlias.toLowerCase(); // Preserve casing of inputs so we can show the names properly - if (!lcName.startsWith(ExpData.DATA_INPUT_PARENT.toLowerCase()) && !lcName.startsWith(ExpMaterial.MATERIAL_INPUT_PARENT.toLowerCase())) + boolean isExpInput = false; + if (lcName.startsWith(ExpData.DATA_INPUTS_PREFIX.toLowerCase()) || lcName.startsWith(ExpMaterial.MATERIAL_INPUTS_PREFIX.toLowerCase())) + { + String[] parts = nameFromAlias.split("/", 2); + String prefix = parts[0]; + String dataType = parts[1]; + // MaterialInputs/datypeTypeEncoded + if (row.containsKey(prefix + "/" + QueryKey.encodePart(dataType))) + isExpInput = true; + } + else nameFromAlias = lcName; boolean isExtraAuditField = extraFieldsToInclude != null && extraFieldsToInclude.contains(nameFromAlias); - if (!excludedFromDetailDiff.contains(nameFromAlias) && row.containsKey(nameFromAlias)) + if (!excludedFromDetailDiff.contains(nameFromAlias) && (row.containsKey(nameFromAlias) || isExpInput)) { Object oldValue = entry.getValue(); Object newValue = row.get(nameFromAlias); From f328f9b5f7796ccd17ae81dc89863f258c780f49 Mon Sep 17 00:00:00 2001 From: XingY Date: Mon, 16 Dec 2024 06:41:57 -0800 Subject: [PATCH 16/16] publish --- core/package-lock.json | 8 ++++---- core/package.json | 2 +- experiment/package-lock.json | 8 ++++---- experiment/package.json | 2 +- 4 files changed, 10 insertions(+), 10 deletions(-) diff --git a/core/package-lock.json b/core/package-lock.json index 5edcbff5a6c..e01d6ab6754 100644 --- a/core/package-lock.json +++ b/core/package-lock.json @@ -8,7 +8,7 @@ "name": "labkey-core", "version": "0.0.0", "dependencies": { - "@labkey/components": "6.5.3-fb-domainNameValidation.1", + "@labkey/components": "6.5.3", "@labkey/themes": "1.4.0" }, "devDependencies": { @@ -3058,9 +3058,9 @@ } }, "node_modules/@labkey/components": { - "version": "6.5.3-fb-domainNameValidation.1", - "resolved": "https://labkey.jfrog.io/artifactory/api/npm/libs-client/@labkey/components/-/@labkey/components-6.5.3-fb-domainNameValidation.1.tgz", - "integrity": "sha512-UUuJYcBlzTrzo3faghKz6lk/zT6xFaSC2pxymIMrpOiXLJVNtlL0BoWZafboU/JUNU8Ac5JI5MDf873n04iCLw==", + "version": "6.5.3", + "resolved": "https://labkey.jfrog.io/artifactory/api/npm/libs-client/@labkey/components/-/@labkey/components-6.5.3.tgz", + "integrity": "sha512-7kQ9VegwEysB2BFJ4W8sKim0Ld9mXLZq3PC23gcqWEKvhok/jL0aId6x2uQUxhu+8q1BX+6shfBj0j8RLA9uLQ==", "dependencies": { "@hello-pangea/dnd": "17.0.0", "@labkey/api": "1.36.0", diff --git a/core/package.json b/core/package.json index abeb84ab0a4..6942d30d0f0 100644 --- a/core/package.json +++ b/core/package.json @@ -54,7 +54,7 @@ } }, "dependencies": { - "@labkey/components": "6.5.3-fb-domainNameValidation.1", + "@labkey/components": "6.5.3", "@labkey/themes": "1.4.0" }, "devDependencies": { diff --git a/experiment/package-lock.json b/experiment/package-lock.json index 09f57269825..35a1dab8c76 100644 --- a/experiment/package-lock.json +++ b/experiment/package-lock.json @@ -8,7 +8,7 @@ "name": "experiment", "version": "0.0.0", "dependencies": { - "@labkey/components": "6.5.3-fb-domainNameValidation.1" + "@labkey/components": "6.5.3" }, "devDependencies": { "@labkey/build": "8.3.0", @@ -2864,9 +2864,9 @@ } }, "node_modules/@labkey/components": { - "version": "6.5.3-fb-domainNameValidation.1", - "resolved": "https://labkey.jfrog.io/artifactory/api/npm/libs-client/@labkey/components/-/@labkey/components-6.5.3-fb-domainNameValidation.1.tgz", - "integrity": "sha512-UUuJYcBlzTrzo3faghKz6lk/zT6xFaSC2pxymIMrpOiXLJVNtlL0BoWZafboU/JUNU8Ac5JI5MDf873n04iCLw==", + "version": "6.5.3", + "resolved": "https://labkey.jfrog.io/artifactory/api/npm/libs-client/@labkey/components/-/@labkey/components-6.5.3.tgz", + "integrity": "sha512-7kQ9VegwEysB2BFJ4W8sKim0Ld9mXLZq3PC23gcqWEKvhok/jL0aId6x2uQUxhu+8q1BX+6shfBj0j8RLA9uLQ==", "dependencies": { "@hello-pangea/dnd": "17.0.0", "@labkey/api": "1.36.0", diff --git a/experiment/package.json b/experiment/package.json index a88c9cee628..f5636b6089b 100644 --- a/experiment/package.json +++ b/experiment/package.json @@ -13,7 +13,7 @@ "test-integration": "cross-env NODE_ENV=test jest --ci --runInBand -c test/js/jest.config.integration.js" }, "dependencies": { - "@labkey/components": "6.5.3-fb-domainNameValidation.1" + "@labkey/components": "6.5.3" }, "devDependencies": { "@labkey/build": "8.3.0",