From 2890a76ad1131fa11debe6993b23cb71c911b258 Mon Sep 17 00:00:00 2001 From: labkey-susanh Date: Mon, 27 Jul 2026 13:43:53 -0700 Subject: [PATCH 1/8] GH Issue 1257: Check for duplicates among field names, import aliases, and parent import aliases --- .../labkey/api/exp/api/ExperimentService.java | 15 +++++++-- .../labkey/api/exp/property/DomainKind.java | 33 +++++++++++++++++++ .../api/gwt/client/model/GWTDomain.java | 12 +++++++ 3 files changed, 58 insertions(+), 2 deletions(-) diff --git a/api/src/org/labkey/api/exp/api/ExperimentService.java b/api/src/org/labkey/api/exp/api/ExperimentService.java index df052bbe503..c9ba0e89055 100644 --- a/api/src/org/labkey/api/exp/api/ExperimentService.java +++ b/api/src/org/labkey/api/exp/api/ExperimentService.java @@ -613,9 +613,20 @@ static void validateParentAlias(Map aliasMap, Set reserv throw new IllegalArgumentException(String.format("Parent alias header is reserved: %1$s", trimmedKey)); } - if (updatedDomainDesign != null && !existingAliases.contains(trimmedKey) && updatedDomainDesign.getFieldByName(trimmedKey) != null) + if (updatedDomainDesign != null && !existingAliases.contains(trimmedKey)) { - throw new IllegalArgumentException(String.format("An existing " + dataTypeNoun + " property conflicts with parent alias header: %1$s", trimmedKey)); + var field = updatedDomainDesign.getFieldByName(trimmedKey); + if (field != null) + { + throw new IllegalArgumentException(String.format("An existing %1s property conflicts with parent alias header: %2$s", dataTypeNoun, trimmedKey)); + } + + // GH Issue 1257 + field = updatedDomainDesign.getFieldByImportAlias(trimmedKey); + if (field != null) + { + throw new IllegalArgumentException(String.format("Field %1$s has an import alias %2$s that conflicts with a parent alias header.", field.getName(), trimmedKey)); + } } if (!dupes.add(trimmedKey)) diff --git a/api/src/org/labkey/api/exp/property/DomainKind.java b/api/src/org/labkey/api/exp/property/DomainKind.java index 99a1b9717ff..b0eacdc53f3 100644 --- a/api/src/org/labkey/api/exp/property/DomainKind.java +++ b/api/src/org/labkey/api/exp/property/DomainKind.java @@ -16,10 +16,12 @@ package org.labkey.api.exp.property; +import org.apache.commons.lang3.StringUtils; import org.apache.logging.log4j.Logger; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; import org.labkey.api.collections.CaseInsensitiveHashSet; +import org.labkey.api.data.ColumnRenderPropertiesImpl; import org.labkey.api.data.Container; import org.labkey.api.data.ContainerFilter; import org.labkey.api.data.DbSchemaType; @@ -48,7 +50,9 @@ import org.labkey.api.writer.ContainerUser; import org.labkey.data.xml.domainTemplate.DomainTemplateType; +import java.util.ArrayList; import java.util.Collections; +import java.util.HashMap; import java.util.LinkedHashSet; import java.util.List; import java.util.Map; @@ -443,6 +447,11 @@ public void validateOptions(Container container, User user, T options, String na if (isUpdate && !canEditDefinition(user, domain)) throw new UnauthorizedException("You don't have permission to edit this domain"); + + String validationMsg = validateFieldImportAliases(updatedDomainDesign.getFields()); + + if (validationMsg != null) + throw new IllegalArgumentException(validationMsg); } public NameExpressionValidationResult validateNameExpressions(T options, GWTDomain domainDesign, Container container) @@ -450,6 +459,30 @@ public NameExpressionValidationResult validateNameExpressions(T options, GWTDoma return null; } + // GH Issue 1257: Check for duplicate aliases and conflicts with field names + public String validateFieldImportAliases(List properties) + { + Map> aliasesMap = new HashMap<>(); + Map propNames = properties.stream().collect(Collectors.toMap(GWTPropertyDescriptor::getName, p -> p)); + properties.forEach(pd -> { + Set aliasSet = ColumnRenderPropertiesImpl.convertToSet(pd.getImportAliases()); + aliasSet.forEach(alias -> { + aliasesMap.computeIfAbsent(alias, k -> new ArrayList<>()).add(pd.getName()); + }); + }); + List fieldMessages = new ArrayList<>(); + aliasesMap.forEach((alias, fields) -> { + if (fields.size() > 1) + fieldMessages.add("Duplicate import alias " + alias + " for fields " + fields.stream().sorted().collect(Collectors.joining(", ")) + "."); + }); + aliasesMap.forEach((alias, fields) -> { + if (propNames.containsKey(alias)) + fieldMessages.add("Import alias " + alias + " on field" + (fields.size() == 1 ? " " : "s ") + fields.stream().sorted().collect(Collectors.joining(", ")) + " conflicts with a field name."); + }); + if (!fieldMessages.isEmpty()) + return StringUtils.join(fieldMessages, " "); + return null; + } /** * @return Return preview name(s) based on the name expression configured for the designer. For DataClass, * up to one preview names is returned. For samples, up to 2 names can be returned, with the 1st one being diff --git a/api/src/org/labkey/api/gwt/client/model/GWTDomain.java b/api/src/org/labkey/api/gwt/client/model/GWTDomain.java index a0bc62bb98a..c4fb488e360 100644 --- a/api/src/org/labkey/api/gwt/client/model/GWTDomain.java +++ b/api/src/org/labkey/api/gwt/client/model/GWTDomain.java @@ -19,6 +19,7 @@ import com.fasterxml.jackson.annotation.JsonIgnore; import lombok.Getter; import lombok.Setter; +import org.labkey.api.data.ColumnRenderPropertiesImpl; import org.labkey.api.gwt.client.DefaultValueType; import java.util.ArrayList; @@ -197,6 +198,17 @@ public FieldType getFieldByName(String name) return null; } + public FieldType getFieldByImportAlias(String name) + { + for (FieldType field : getFields(true)) + { + Set importAliases = ColumnRenderPropertiesImpl.convertToSet(field.getImportAliases()); + if (importAliases.contains(name)) + return field; + } + return null; + } + /** * @return Indicates that the property can't be removed from the domain. The property may or may not be nullable. */ From 8515fb02c87c83cfa3a73733649e65c2a459f659 Mon Sep 17 00:00:00 2001 From: labkey-susanh Date: Mon, 27 Jul 2026 14:21:34 -0700 Subject: [PATCH 2/8] Case-sensitivity checks and more deterministic error reporting --- .../labkey/api/exp/api/ExperimentService.java | 13 +++++++----- .../labkey/api/exp/property/DomainKind.java | 21 +++++++++++-------- 2 files changed, 20 insertions(+), 14 deletions(-) diff --git a/api/src/org/labkey/api/exp/api/ExperimentService.java b/api/src/org/labkey/api/exp/api/ExperimentService.java index c9ba0e89055..2f8d047c833 100644 --- a/api/src/org/labkey/api/exp/api/ExperimentService.java +++ b/api/src/org/labkey/api/exp/api/ExperimentService.java @@ -613,16 +613,19 @@ static void validateParentAlias(Map aliasMap, Set reserv throw new IllegalArgumentException(String.format("Parent alias header is reserved: %1$s", trimmedKey)); } - if (updatedDomainDesign != null && !existingAliases.contains(trimmedKey)) + if (updatedDomainDesign != null) { - var field = updatedDomainDesign.getFieldByName(trimmedKey); - if (field != null) + if (!existingAliases.contains(trimmedKey)) { - throw new IllegalArgumentException(String.format("An existing %1s property conflicts with parent alias header: %2$s", dataTypeNoun, trimmedKey)); + var field = updatedDomainDesign.getFieldByName(trimmedKey); + if (field != null) + { + throw new IllegalArgumentException(String.format("An existing %1$s property conflicts with parent alias header: %2$s", dataTypeNoun, trimmedKey)); + } } // GH Issue 1257 - field = updatedDomainDesign.getFieldByImportAlias(trimmedKey); + var field = updatedDomainDesign.getFieldByImportAlias(trimmedKey); if (field != null) { throw new IllegalArgumentException(String.format("Field %1$s has an import alias %2$s that conflicts with a parent alias header.", field.getName(), trimmedKey)); diff --git a/api/src/org/labkey/api/exp/property/DomainKind.java b/api/src/org/labkey/api/exp/property/DomainKind.java index b0eacdc53f3..9f95d177fe3 100644 --- a/api/src/org/labkey/api/exp/property/DomainKind.java +++ b/api/src/org/labkey/api/exp/property/DomainKind.java @@ -21,6 +21,7 @@ import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; import org.labkey.api.collections.CaseInsensitiveHashSet; +import org.labkey.api.collections.CaseInsensitiveLinkedHashMap; import org.labkey.api.data.ColumnRenderPropertiesImpl; import org.labkey.api.data.Container; import org.labkey.api.data.ContainerFilter; @@ -52,10 +53,11 @@ import java.util.ArrayList; import java.util.Collections; -import java.util.HashMap; +import java.util.HashSet; import java.util.LinkedHashSet; import java.util.List; import java.util.Map; +import java.util.Objects; import java.util.Set; import java.util.stream.Collectors; @@ -448,10 +450,13 @@ public void validateOptions(Container container, User user, T options, String na if (isUpdate && !canEditDefinition(user, domain)) throw new UnauthorizedException("You don't have permission to edit this domain"); - String validationMsg = validateFieldImportAliases(updatedDomainDesign.getFields()); + if (updatedDomainDesign != null) + { + String validationMsg = validateFieldImportAliases(updatedDomainDesign.getFields(true)); - if (validationMsg != null) - throw new IllegalArgumentException(validationMsg); + if (validationMsg != null) + throw new IllegalArgumentException(validationMsg); + } } public NameExpressionValidationResult validateNameExpressions(T options, GWTDomain domainDesign, Container container) @@ -462,8 +467,8 @@ public NameExpressionValidationResult validateNameExpressions(T options, GWTDoma // GH Issue 1257: Check for duplicate aliases and conflicts with field names public String validateFieldImportAliases(List properties) { - Map> aliasesMap = new HashMap<>(); - Map propNames = properties.stream().collect(Collectors.toMap(GWTPropertyDescriptor::getName, p -> p)); + Map> aliasesMap = new CaseInsensitiveLinkedHashMap<>(); + HashSet propNames = properties.stream().map(GWTPropertyDescriptor::getName).filter(Objects::nonNull).collect(Collectors.toCollection(CaseInsensitiveHashSet::new)); properties.forEach(pd -> { Set aliasSet = ColumnRenderPropertiesImpl.convertToSet(pd.getImportAliases()); aliasSet.forEach(alias -> { @@ -474,9 +479,7 @@ public String validateFieldImportAliases(List p aliasesMap.forEach((alias, fields) -> { if (fields.size() > 1) fieldMessages.add("Duplicate import alias " + alias + " for fields " + fields.stream().sorted().collect(Collectors.joining(", ")) + "."); - }); - aliasesMap.forEach((alias, fields) -> { - if (propNames.containsKey(alias)) + if (propNames.contains(alias)) fieldMessages.add("Import alias " + alias + " on field" + (fields.size() == 1 ? " " : "s ") + fields.stream().sorted().collect(Collectors.joining(", ")) + " conflicts with a field name."); }); if (!fieldMessages.isEmpty()) From dcafca281a755208873c21b7a8edbc3a762e1dba Mon Sep 17 00:00:00 2001 From: labkey-susanh Date: Mon, 27 Jul 2026 15:47:40 -0700 Subject: [PATCH 3/8] Move field validation to DomainUtil.validateProperties to be applicable to all domains and tap into domain designer error messaging --- .../labkey/api/exp/api/ExperimentService.java | 13 +++---- .../labkey/api/exp/property/DomainKind.java | 36 ------------------- .../labkey/api/exp/property/DomainUtil.java | 32 +++++++++++++++++ .../api/gwt/client/model/GWTDomain.java | 5 +-- 4 files changed, 42 insertions(+), 44 deletions(-) diff --git a/api/src/org/labkey/api/exp/api/ExperimentService.java b/api/src/org/labkey/api/exp/api/ExperimentService.java index 2f8d047c833..46cd8d4fe6a 100644 --- a/api/src/org/labkey/api/exp/api/ExperimentService.java +++ b/api/src/org/labkey/api/exp/api/ExperimentService.java @@ -615,6 +615,7 @@ static void validateParentAlias(Map aliasMap, Set reserv if (updatedDomainDesign != null) { + // This allows existing domains to save with conflicting aliases. Should it? if (!existingAliases.contains(trimmedKey)) { var field = updatedDomainDesign.getFieldByName(trimmedKey); @@ -622,13 +623,13 @@ static void validateParentAlias(Map aliasMap, Set reserv { throw new IllegalArgumentException(String.format("An existing %1$s property conflicts with parent alias header: %2$s", dataTypeNoun, trimmedKey)); } - } - // GH Issue 1257 - var field = updatedDomainDesign.getFieldByImportAlias(trimmedKey); - if (field != null) - { - throw new IllegalArgumentException(String.format("Field %1$s has an import alias %2$s that conflicts with a parent alias header.", field.getName(), trimmedKey)); + // GH Issue 1257 + field = updatedDomainDesign.getFieldByImportAlias(trimmedKey); + if (field != null) + { + throw new IllegalArgumentException(String.format("Field %1$s has an import alias %2$s that conflicts with a parent alias header.", field.getName(), trimmedKey)); + } } } diff --git a/api/src/org/labkey/api/exp/property/DomainKind.java b/api/src/org/labkey/api/exp/property/DomainKind.java index 9f95d177fe3..99a1b9717ff 100644 --- a/api/src/org/labkey/api/exp/property/DomainKind.java +++ b/api/src/org/labkey/api/exp/property/DomainKind.java @@ -16,13 +16,10 @@ package org.labkey.api.exp.property; -import org.apache.commons.lang3.StringUtils; import org.apache.logging.log4j.Logger; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; import org.labkey.api.collections.CaseInsensitiveHashSet; -import org.labkey.api.collections.CaseInsensitiveLinkedHashMap; -import org.labkey.api.data.ColumnRenderPropertiesImpl; import org.labkey.api.data.Container; import org.labkey.api.data.ContainerFilter; import org.labkey.api.data.DbSchemaType; @@ -51,13 +48,10 @@ import org.labkey.api.writer.ContainerUser; import org.labkey.data.xml.domainTemplate.DomainTemplateType; -import java.util.ArrayList; import java.util.Collections; -import java.util.HashSet; import java.util.LinkedHashSet; import java.util.List; import java.util.Map; -import java.util.Objects; import java.util.Set; import java.util.stream.Collectors; @@ -449,14 +443,6 @@ public void validateOptions(Container container, User user, T options, String na if (isUpdate && !canEditDefinition(user, domain)) throw new UnauthorizedException("You don't have permission to edit this domain"); - - if (updatedDomainDesign != null) - { - String validationMsg = validateFieldImportAliases(updatedDomainDesign.getFields(true)); - - if (validationMsg != null) - throw new IllegalArgumentException(validationMsg); - } } public NameExpressionValidationResult validateNameExpressions(T options, GWTDomain domainDesign, Container container) @@ -464,28 +450,6 @@ public NameExpressionValidationResult validateNameExpressions(T options, GWTDoma return null; } - // GH Issue 1257: Check for duplicate aliases and conflicts with field names - public String validateFieldImportAliases(List properties) - { - Map> aliasesMap = new CaseInsensitiveLinkedHashMap<>(); - HashSet propNames = properties.stream().map(GWTPropertyDescriptor::getName).filter(Objects::nonNull).collect(Collectors.toCollection(CaseInsensitiveHashSet::new)); - properties.forEach(pd -> { - Set aliasSet = ColumnRenderPropertiesImpl.convertToSet(pd.getImportAliases()); - aliasSet.forEach(alias -> { - aliasesMap.computeIfAbsent(alias, k -> new ArrayList<>()).add(pd.getName()); - }); - }); - List fieldMessages = new ArrayList<>(); - aliasesMap.forEach((alias, fields) -> { - if (fields.size() > 1) - fieldMessages.add("Duplicate import alias " + alias + " for fields " + fields.stream().sorted().collect(Collectors.joining(", ")) + "."); - if (propNames.contains(alias)) - fieldMessages.add("Import alias " + alias + " on field" + (fields.size() == 1 ? " " : "s ") + fields.stream().sorted().collect(Collectors.joining(", ")) + " conflicts with a field name."); - }); - if (!fieldMessages.isEmpty()) - return StringUtils.join(fieldMessages, " "); - return null; - } /** * @return Return preview name(s) based on the name expression configured for the designer. For DataClass, * up to one preview names is returned. For samples, up to 2 names can be returned, with the 1st one being diff --git a/api/src/org/labkey/api/exp/property/DomainUtil.java b/api/src/org/labkey/api/exp/property/DomainUtil.java index d10d3d5a854..2f443345214 100644 --- a/api/src/org/labkey/api/exp/property/DomainUtil.java +++ b/api/src/org/labkey/api/exp/property/DomainUtil.java @@ -26,6 +26,7 @@ import org.labkey.api.assay.AbstractAssayProvider; import org.labkey.api.collections.CaseInsensitiveHashMap; import org.labkey.api.collections.CaseInsensitiveHashSet; +import org.labkey.api.collections.CaseInsensitiveLinkedHashMap; import org.labkey.api.collections.IntHashMap; import org.labkey.api.collections.LongHashMap; import org.labkey.api.data.ColumnInfo; @@ -1563,6 +1564,9 @@ public static ValidationException validateProperties(@Nullable Domain domain, @N Set reservedPrefixes = (null != domain && null != domainKind) ? domainKind.getReservedPropertyNamePrefixes() : updates.getReservedFieldNamePrefixes(); Map namePropertyIdMap = new CaseInsensitiveHashMap<>(); Map altNameMap = new CaseInsensitiveHashMap<>(); + // GH Issue 1257: import alias -> the fields declaring it. + Map> importAliasMap = new CaseInsensitiveLinkedHashMap<>(); + Set fieldNames = new CaseInsensitiveHashSet(); ValidationException exception = new ValidationException(); Map propertyIdNameMap = getOriginalFieldPropertyIdNameMap(orig);//key: orig property id, value : orig field name @@ -1577,6 +1581,15 @@ public static ValidationException validateProperties(@Nullable Domain domain, @N continue; } + fieldNames.add(name); + + // GH Issue 1257: Collect alias to field list mapping + for (String alias : new CaseInsensitiveHashSet(ColumnRenderPropertiesImpl.convertToSet(field.getImportAliases()))) + { + if (!alias.equalsIgnoreCase(name)) // an alias that repeats the field's name is redundant but not ambiguous, so skip it + importAliasMap.computeIfAbsent(alias, k -> new ArrayList<>()).add(field); + } + if (ILLEGAL_PROPERTY_NAMES.contains(name.trim())) { exception.addError(new SimpleValidationError(getDomainErrorMessage(updates, "The field name '" + name + "' is not allowed."))); @@ -1674,9 +1687,28 @@ public static ValidationException validateProperties(@Nullable Domain domain, @N } } + // GH Issue 1257: import aliases must be unique across the domain and must not collide with a field name, since + // ImportAliasable.Helper.createImportMap() resolves names, labels, and aliases into one case-insensitive map. + importAliasMap.forEach((alias, fields) -> { + String names = fields.stream().map(GWTPropertyDescriptor::getName).sorted().collect(Collectors.joining(", ")); + + if (fields.size() > 1) + addImportAliasErrors(exception, updates, fields, "Duplicate import alias " + alias + " for fields " + names + "."); + + if (fieldNames.contains(alias)) + addImportAliasErrors(exception, updates, fields, "Import alias " + alias + " on field" + (fields.size() == 1 ? " " : "s ") + names + " conflicts with a field name."); + }); + return exception; } + /** Anchor an import alias error on every field that declares the offending alias so the designer can highlight them. */ + private static void addImportAliasErrors(ValidationException exception, GWTDomain updates, List fields, String message) + { + for (GWTPropertyDescriptor field : fields) + exception.addError(new PropertyValidationError(getDomainErrorMessage(updates, message), field.getName(), field.getPropertyId())); + } + @Nullable private static Map getOriginalFieldPropertyIdNameMap(@Nullable GWTDomain orig) { diff --git a/api/src/org/labkey/api/gwt/client/model/GWTDomain.java b/api/src/org/labkey/api/gwt/client/model/GWTDomain.java index c4fb488e360..58eebeb2b1a 100644 --- a/api/src/org/labkey/api/gwt/client/model/GWTDomain.java +++ b/api/src/org/labkey/api/gwt/client/model/GWTDomain.java @@ -19,6 +19,7 @@ import com.fasterxml.jackson.annotation.JsonIgnore; import lombok.Getter; import lombok.Setter; +import org.labkey.api.collections.CaseInsensitiveHashSet; import org.labkey.api.data.ColumnRenderPropertiesImpl; import org.labkey.api.gwt.client.DefaultValueType; @@ -198,12 +199,12 @@ public FieldType getFieldByName(String name) return null; } - public FieldType getFieldByImportAlias(String name) + public FieldType getFieldByImportAlias(String alias) { for (FieldType field : getFields(true)) { Set importAliases = ColumnRenderPropertiesImpl.convertToSet(field.getImportAliases()); - if (importAliases.contains(name)) + if (new CaseInsensitiveHashSet(importAliases).contains(alias)) return field; } return null; From cd4e96468009c269507001c87404b28d0787bebc Mon Sep 17 00:00:00 2001 From: labkey-susanh Date: Mon, 27 Jul 2026 16:33:55 -0700 Subject: [PATCH 4/8] Rearrange and add comment --- .../org/labkey/api/exp/api/ExperimentService.java | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/api/src/org/labkey/api/exp/api/ExperimentService.java b/api/src/org/labkey/api/exp/api/ExperimentService.java index 46cd8d4fe6a..e775fd6d834 100644 --- a/api/src/org/labkey/api/exp/api/ExperimentService.java +++ b/api/src/org/labkey/api/exp/api/ExperimentService.java @@ -615,7 +615,6 @@ static void validateParentAlias(Map aliasMap, Set reserv if (updatedDomainDesign != null) { - // This allows existing domains to save with conflicting aliases. Should it? if (!existingAliases.contains(trimmedKey)) { var field = updatedDomainDesign.getFieldByName(trimmedKey); @@ -623,14 +622,14 @@ static void validateParentAlias(Map aliasMap, Set reserv { throw new IllegalArgumentException(String.format("An existing %1$s property conflicts with parent alias header: %2$s", dataTypeNoun, trimmedKey)); } - - // GH Issue 1257 - field = updatedDomainDesign.getFieldByImportAlias(trimmedKey); - if (field != null) - { - throw new IllegalArgumentException(String.format("Field %1$s has an import alias %2$s that conflicts with a parent alias header.", field.getName(), trimmedKey)); - } } + // GH Issue 1257: If there are conflicts with import aliases, this should be an error since it produces ambiguity during import + var field = updatedDomainDesign.getFieldByImportAlias(trimmedKey); + if (field != null) + { + throw new IllegalArgumentException(String.format("Field %1$s has an import alias %2$s that conflicts with a parent alias header.", field.getName(), trimmedKey)); + } + } if (!dupes.add(trimmedKey)) From a9396ae306397d7ca019b3f1a4b02c5f5fc6b477 Mon Sep 17 00:00:00 2001 From: labkey-susanh Date: Tue, 28 Jul 2026 06:48:10 -0700 Subject: [PATCH 5/8] More quotes for fun and profit. --- api/src/org/labkey/api/exp/api/ExperimentService.java | 2 +- api/src/org/labkey/api/exp/property/DomainUtil.java | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/api/src/org/labkey/api/exp/api/ExperimentService.java b/api/src/org/labkey/api/exp/api/ExperimentService.java index 73efe6461b5..76a566c6a40 100644 --- a/api/src/org/labkey/api/exp/api/ExperimentService.java +++ b/api/src/org/labkey/api/exp/api/ExperimentService.java @@ -629,7 +629,7 @@ static void validateParentAlias(Map aliasMap, Set reserv var field = updatedDomainDesign.getFieldByImportAlias(trimmedKey); if (field != null) { - throw new IllegalArgumentException(String.format("Field %1$s has an import alias %2$s that conflicts with a parent alias header.", field.getName(), trimmedKey)); + throw new IllegalArgumentException(String.format("Field '%1$s' has an import alias '%2$s' that conflicts with a parent alias header.", field.getName(), trimmedKey)); } } diff --git a/api/src/org/labkey/api/exp/property/DomainUtil.java b/api/src/org/labkey/api/exp/property/DomainUtil.java index 2f443345214..84d50d057b4 100644 --- a/api/src/org/labkey/api/exp/property/DomainUtil.java +++ b/api/src/org/labkey/api/exp/property/DomainUtil.java @@ -1690,13 +1690,13 @@ public static ValidationException validateProperties(@Nullable Domain domain, @N // GH Issue 1257: import aliases must be unique across the domain and must not collide with a field name, since // ImportAliasable.Helper.createImportMap() resolves names, labels, and aliases into one case-insensitive map. importAliasMap.forEach((alias, fields) -> { - String names = fields.stream().map(GWTPropertyDescriptor::getName).sorted().collect(Collectors.joining(", ")); + String names = "'" + fields.stream().map(GWTPropertyDescriptor::getName).sorted().collect(Collectors.joining("', '")) + "'"; if (fields.size() > 1) - addImportAliasErrors(exception, updates, fields, "Duplicate import alias " + alias + " for fields " + names + "."); + addImportAliasErrors(exception, updates, fields, "Duplicate import alias '" + alias + "' for fields " + names + "."); if (fieldNames.contains(alias)) - addImportAliasErrors(exception, updates, fields, "Import alias " + alias + " on field" + (fields.size() == 1 ? " " : "s ") + names + " conflicts with a field name."); + addImportAliasErrors(exception, updates, fields, "Import alias '" + alias + "' on field" + (fields.size() == 1 ? " " : "s ") + names + " conflicts with a field name."); }); return exception; From 11639d7a2f88c068afc7648f9b242abf71d36c87 Mon Sep 17 00:00:00 2001 From: labkey-susanh Date: Wed, 29 Jul 2026 20:12:04 -0700 Subject: [PATCH 6/8] Remove protection against existing parent aliases colliding field names --- api/src/org/labkey/api/exp/api/ExperimentService.java | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/api/src/org/labkey/api/exp/api/ExperimentService.java b/api/src/org/labkey/api/exp/api/ExperimentService.java index 76a566c6a40..fd0b9fce06f 100644 --- a/api/src/org/labkey/api/exp/api/ExperimentService.java +++ b/api/src/org/labkey/api/exp/api/ExperimentService.java @@ -617,16 +617,13 @@ static void validateParentAlias(Map aliasMap, Set reserv if (updatedDomainDesign != null) { - if (!existingAliases.contains(trimmedKey)) + var field = updatedDomainDesign.getFieldByName(trimmedKey); + if (field != null) { - var field = updatedDomainDesign.getFieldByName(trimmedKey); - if (field != null) - { - throw new IllegalArgumentException(String.format("An existing %1$s property conflicts with parent alias header: %2$s", dataTypeNoun, trimmedKey)); - } + throw new IllegalArgumentException(String.format("An existing %1$s property conflicts with parent alias header: %2$s", dataTypeNoun, trimmedKey)); } // GH Issue 1257: If there are conflicts with import aliases, this should be an error since it produces ambiguity during import - var field = updatedDomainDesign.getFieldByImportAlias(trimmedKey); + field = updatedDomainDesign.getFieldByImportAlias(trimmedKey); if (field != null) { throw new IllegalArgumentException(String.format("Field '%1$s' has an import alias '%2$s' that conflicts with a parent alias header.", field.getName(), trimmedKey)); From 6bfb6f83d605592451d6a16a90f5d5faf2463e8b Mon Sep 17 00:00:00 2001 From: labkey-susanh Date: Thu, 30 Jul 2026 09:43:40 -0700 Subject: [PATCH 7/8] Add Unit tests and one designer test for overlapping aliases --- .../labkey/api/exp/property/DomainUtil.java | 144 +++++++++++++++++- .../labkey/experiment/ExperimentModule.java | 2 + 2 files changed, 143 insertions(+), 3 deletions(-) diff --git a/api/src/org/labkey/api/exp/property/DomainUtil.java b/api/src/org/labkey/api/exp/property/DomainUtil.java index 84d50d057b4..821a9ace6de 100644 --- a/api/src/org/labkey/api/exp/property/DomainUtil.java +++ b/api/src/org/labkey/api/exp/property/DomainUtil.java @@ -23,6 +23,8 @@ import org.apache.logging.log4j.Logger; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; +import org.junit.Assert; +import org.junit.Test; import org.labkey.api.assay.AbstractAssayProvider; import org.labkey.api.collections.CaseInsensitiveHashMap; import org.labkey.api.collections.CaseInsensitiveHashSet; @@ -100,6 +102,7 @@ import java.lang.reflect.InvocationTargetException; import java.util.ArrayList; +import java.util.Arrays; import java.util.Collection; import java.util.Collections; import java.util.Date; @@ -1583,10 +1586,14 @@ public static ValidationException validateProperties(@Nullable Domain domain, @N fieldNames.add(name); - // GH Issue 1257: Collect alias to field list mapping - for (String alias : new CaseInsensitiveHashSet(ColumnRenderPropertiesImpl.convertToSet(field.getImportAliases()))) + // GH Issue 1257: Collect alias to field list mapping. convertToSet() is case-sensitive, so dedupe the field's + // own aliases here to keep case variants ("abc, ABC") from looking like two fields claiming one alias. Iterate + // the parsed values rather than a CaseInsensitiveHashSet so the alias keeps the casing the user entered. + Set fieldAliases = new CaseInsensitiveHashSet(); + for (String alias : ColumnRenderPropertiesImpl.convertToSet(field.getImportAliases())) { - if (!alias.equalsIgnoreCase(name)) // an alias that repeats the field's name is redundant but not ambiguous, so skip it + // an alias that repeats the field's name is redundant but not ambiguous, so skip it + if (!alias.equalsIgnoreCase(name) && fieldAliases.add(alias)) importAliasMap.computeIfAbsent(alias, k -> new ArrayList<>()).add(field); } @@ -1747,4 +1754,135 @@ public static Set getNamesAndLabels(Collection names) } return values; } + + /** GH Issue 1257: import alias validation in {@link #validateProperties}. */ + public static class ImportAliasTestCase extends Assert + { + private static final String FIELD_ONE = "AliasFieldOne"; + private static final String FIELD_TWO = "AliasFieldTwo"; + private static final String FIELD_THREE = "AliasFieldThree"; + + private static GWTPropertyDescriptor field(String name, @Nullable String importAliases) + { + GWTPropertyDescriptor pd = new GWTPropertyDescriptor(name, PropertyType.STRING.getTypeUri()); + pd.setImportAliases(importAliases); + return pd; + } + + /** + * Validate with a null DomainKind and no original domain so the surrounding name/reserved-name checks stay out of + * the way, and with no domain name so messages carry no "<domainName> -- " prefix. + */ + private static ValidationException validate(GWTPropertyDescriptor... fields) + { + GWTDomain domain = new GWTDomain<>(); + domain.setFields(Arrays.asList(fields)); + return validateProperties(null, domain, null, null, null); + } + + @Test + public void aliasesWithNoOverlapAreAllowed() + { + ValidationException errors = validate( + field(FIELD_ONE, "one, uno"), + field(FIELD_TWO, "two"), + field(FIELD_THREE, null)); + assertFalse("Non-overlapping import aliases should validate: " + errors.getAllErrors(), errors.hasErrors()); + } + + @Test + public void aliasRepeatingItsOwnFieldNameIsAllowed() + { + // Redundant, but resolves to the same field, so there is nothing ambiguous to reject + ValidationException errors = validate(field(FIELD_ONE, FIELD_ONE), field(FIELD_TWO, FIELD_TWO.toLowerCase())); + assertFalse("A field's import alias may repeat its own name: " + errors.getAllErrors(), errors.hasErrors()); + } + + @Test + public void caseVariantsWithinOneFieldCountOnce() + { + // convertToSet() is case-sensitive, so "dupe, DUPE" parses to two aliases that collapse to one for this check + ValidationException errors = validate(field(FIELD_ONE, "dupe, DUPE"), field(FIELD_TWO, null)); + assertFalse("Case variants of one alias on a single field are not a duplicate: " + errors.getAllErrors(), errors.hasErrors()); + } + + @Test + public void messageEchoesTheAliasAsEntered() + { + // Reported back to the user, so it must not be normalized to the lower case the comparison uses + ValidationException errors = validate(field(FIELD_ONE, "MixedCaseAlias"), field(FIELD_TWO, "mixedcasealias")); + assertEquals(List.of("Duplicate import alias 'MixedCaseAlias' for fields '" + FIELD_ONE + "', '" + FIELD_TWO + "'."), + errors.getFieldErrors(FIELD_ONE)); + } + + @Test + public void duplicateAliasAcrossFieldsIsRejected() + { + ValidationException errors = validate(field(FIELD_TWO, "shared"), field(FIELD_ONE, "shared")); + // Field names in the message are sorted, so they do not depend on the order the fields were declared in + String expected = "Duplicate import alias 'shared' for fields '" + FIELD_ONE + "', '" + FIELD_TWO + "'."; + assertEquals("Error should be anchored on the first field", List.of(expected), errors.getFieldErrors(FIELD_TWO)); + assertEquals("Error should be anchored on the second field", List.of(expected), errors.getFieldErrors(FIELD_ONE)); + } + + @Test + public void duplicateAliasAcrossFieldsIgnoresCase() + { + ValidationException errors = validate(field(FIELD_ONE, "shared"), field(FIELD_TWO, "SHARED")); + // The message reports the alias as first encountered, which is the earlier field's spelling + String expected = "Duplicate import alias 'shared' for fields '" + FIELD_ONE + "', '" + FIELD_TWO + "'."; + assertEquals(List.of(expected), errors.getFieldErrors(FIELD_ONE)); + assertEquals(List.of(expected), errors.getFieldErrors(FIELD_TWO)); + } + + @Test + public void aliasConflictingWithFieldNameIsRejected() + { + ValidationException errors = validate(field(FIELD_ONE, FIELD_TWO), field(FIELD_TWO, null)); + assertEquals(List.of("Import alias '" + FIELD_TWO + "' on field '" + FIELD_ONE + "' conflicts with a field name."), + errors.getFieldErrors(FIELD_ONE)); + assertTrue("The conflict belongs to the field declaring the alias, not the field being named", + errors.getFieldErrors(FIELD_TWO).isEmpty()); + } + + @Test + public void aliasConflictingWithFieldNameIgnoresCase() + { + ValidationException errors = validate(field(FIELD_ONE, FIELD_TWO.toLowerCase()), field(FIELD_TWO, null)); + assertEquals(List.of("Import alias '" + FIELD_TWO.toLowerCase() + "' on field '" + FIELD_ONE + "' conflicts with a field name."), + errors.getFieldErrors(FIELD_ONE)); + } + + @Test + public void aliasConflictingWithALaterFieldNameIsRejected() + { + // The alias is declared before the field it collides with, so the check cannot run field-by-field + ValidationException errors = validate(field(FIELD_ONE, FIELD_THREE), field(FIELD_TWO, null), field(FIELD_THREE, null)); + assertEquals(List.of("Import alias '" + FIELD_THREE + "' on field '" + FIELD_ONE + "' conflicts with a field name."), + errors.getFieldErrors(FIELD_ONE)); + } + + @Test + public void aliasThatIsBothDuplicatedAndAFieldNameReportsBoth() + { + ValidationException errors = validate(field(FIELD_ONE, FIELD_THREE), field(FIELD_TWO, FIELD_THREE), field(FIELD_THREE, null)); + List expected = List.of( + "Duplicate import alias '" + FIELD_THREE + "' for fields '" + FIELD_ONE + "', '" + FIELD_TWO + "'.", + "Import alias '" + FIELD_THREE + "' on fields '" + FIELD_ONE + "', '" + FIELD_TWO + "' conflicts with a field name."); + assertEquals(expected, errors.getFieldErrors(FIELD_ONE)); + assertEquals(expected, errors.getFieldErrors(FIELD_TWO)); + } + + @Test + public void blankFieldNameDoesNotBreakAliasChecking() + { + // A nameless field is reported on its own and must not reach the alias map, where it would have no name to report + ValidationException errors = validate(field(null, "shared"), field(" ", "shared"), field(FIELD_ONE, "shared")); + assertEquals("Each nameless field should be reported once", + List.of("Please provide a name for each field.", "Please provide a name for each field."), + errors.getGlobalErrorStrings()); + assertTrue("Only one named field declares the alias, so it is not a duplicate: " + errors.getAllErrors(), + errors.getFieldErrors(FIELD_ONE).isEmpty()); + } + } } diff --git a/experiment/src/org/labkey/experiment/ExperimentModule.java b/experiment/src/org/labkey/experiment/ExperimentModule.java index 0226497abab..87a4d2bd9a7 100644 --- a/experiment/src/org/labkey/experiment/ExperimentModule.java +++ b/experiment/src/org/labkey/experiment/ExperimentModule.java @@ -65,6 +65,7 @@ import org.labkey.api.exp.api.SampleTypeService; import org.labkey.api.exp.api.StorageProvisioner; import org.labkey.api.exp.property.DomainAuditProvider; +import org.labkey.api.exp.property.DomainUtil; import org.labkey.api.exp.property.DomainPropertyAuditProvider; import org.labkey.api.exp.property.ExperimentProperty; import org.labkey.api.exp.property.PropertyService; @@ -1192,6 +1193,7 @@ public Collection getSummary(Container c) public @NotNull Set> getUnitTests() { return Set.of( + DomainUtil.ImportAliasTestCase.class, GraphAlgorithms.TestCase.class, LSIDRelativizer.TestCase.class, Lsid.TestCase.class, From a666f2a4c2b94cf25bc3b551970a44640c900603 Mon Sep 17 00:00:00 2001 From: labkey-susanh Date: Thu, 30 Jul 2026 13:11:34 -0700 Subject: [PATCH 8/8] Add test for multiple errors and with tricky characters --- .../labkey/api/exp/property/DomainUtil.java | 21 +++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/api/src/org/labkey/api/exp/property/DomainUtil.java b/api/src/org/labkey/api/exp/property/DomainUtil.java index 821a9ace6de..93e19bfd681 100644 --- a/api/src/org/labkey/api/exp/property/DomainUtil.java +++ b/api/src/org/labkey/api/exp/property/DomainUtil.java @@ -1873,6 +1873,27 @@ public void aliasThatIsBothDuplicatedAndAFieldNameReportsBoth() assertEquals(expected, errors.getFieldErrors(FIELD_TWO)); } + @Test + public void multipleAliasesPerFieldWithDuplicatesAndTrickyChars() + { + String aliasWithBlanks = "with blanks"; + String aliasWithComma = "with, comma"; + String trickyChars = "\u00C5\u00E4"; // Angstrom + a-umlaut + ValidationException errors = validate(field(FIELD_ONE, trickyChars + "," + "\"" + aliasWithComma + "\"" + "," + aliasWithBlanks), + field(FIELD_TWO, "\"" + aliasWithComma + "\"" ), field(FIELD_THREE, trickyChars + " \"" + aliasWithComma + "\"")); + List expected = List.of( + "Duplicate import alias '" + trickyChars + "' for fields '" + FIELD_ONE + "', '" + FIELD_THREE + "'.", + "Duplicate import alias '" + aliasWithComma + "' for fields '" + FIELD_ONE + "', '" + FIELD_THREE + "', '" + FIELD_TWO + "'."); + assertEquals(expected, errors.getFieldErrors(FIELD_ONE)); + expected = List.of( + "Duplicate import alias '" + aliasWithComma + "' for fields '" + FIELD_ONE + "', '" + FIELD_THREE + "', '" + FIELD_TWO + "'."); + assertEquals(expected, errors.getFieldErrors(FIELD_TWO)); + expected = List.of( + "Duplicate import alias '" + trickyChars + "' for fields '" + FIELD_ONE + "', '" + FIELD_THREE + "'.", + "Duplicate import alias '" + aliasWithComma + "' for fields '" + FIELD_ONE + "', '" + FIELD_THREE + "', '" + FIELD_TWO + "'."); + assertEquals(expected, errors.getFieldErrors(FIELD_THREE)); + } + @Test public void blankFieldNameDoesNotBreakAliasChecking() {