Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 1 addition & 4 deletions api/src/org/labkey/api/exp/api/ExperimentService.java
Original file line number Diff line number Diff line change
Expand Up @@ -119,9 +119,6 @@ public interface ExperimentService extends ExperimentRunTypeSource
String SAMPLE_ALIQUOT_PROTOCOL_LSID = "urn:lsid:labkey.org:Protocol:SampleAliquotProtocol";
String SAMPLE_ALIQUOT_PROTOCOL_NAME = "Sample Aliquot Protocol";

String SAMPLE_MANAGEMENT_JOB_PROTOCOL_PREFIX = "SampleManagement.JobProtocol";
String SAMPLE_MANAGEMENT_TASK_PROTOCOL_PREFIX = "SampleManagement.TaskProtocol";

// Constant used by ExpDataIterators.AliasDataIterator
String ALIASCOLUMNALIAS = "org.labkey.experiment.ExpDataIterators$AliasDataIterator#ALIAS";

Expand Down Expand Up @@ -609,7 +606,7 @@ static boolean parentAliasHasCorrectFormat(String parentAlias)
return true;
}

static void validateParentAlias(Map<String, String> aliasMap, Set<String> reservedNames, Set<String> existingAliases, GWTDomain updatedDomainDesign, String dataTypeNoun)
static void validateParentAlias(Map<String, String> aliasMap, Set<String> reservedNames, GWTDomain updatedDomainDesign, String dataTypeNoun)
{
Set<String> dupes = new CaseInsensitiveHashSet();
aliasMap.forEach((key, value) -> {
Expand Down
48 changes: 6 additions & 42 deletions api/src/org/labkey/api/exp/api/SampleTypeDomainKind.java
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@

import com.google.common.collect.Sets;
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;
Expand Down Expand Up @@ -62,15 +61,12 @@
import org.labkey.api.security.permissions.DesignSampleTypePermission;
import org.labkey.api.util.PageFlowUtil;
import org.labkey.api.util.StringUtilsLabKey;
import org.labkey.api.util.UnexpectedException;
import org.labkey.api.util.logging.LogHelper;
import org.labkey.api.view.ActionURL;
import org.labkey.api.view.NotFoundException;
import org.labkey.api.writer.ContainerUser;
import org.labkey.data.xml.domainTemplate.DomainTemplateType;
import org.labkey.data.xml.domainTemplate.SampleSetTemplateType;

import java.io.IOException;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
Expand All @@ -81,7 +77,6 @@

public class SampleTypeDomainKind extends AbstractDomainKind<SampleTypeDomainKindProperties>
{
private static final Logger logger = LogHelper.getLogger(SampleTypeDomainKind.class, "Sample type domain kind");
public static final String NAME = "SampleSet";
public static final String PROVISIONED_SCHEMA_NAME = "expsampleset";
public static final String SAMPLE_TYPE_FILE_DIRECTORY_NAME = "sampletype";
Expand Down Expand Up @@ -266,30 +261,12 @@ public Set<PropertyStorageSpec> getBaseProperties(Domain domain)
{
Set<String> reserved = new CaseInsensitiveHashSet(RESERVED_NAMES);

if (domain == null)
{
// Issue 48810: See SampleTypeService.createSampleType hasNameProperty. We are adding in a "name" col on the
// client side right before calling the saveDomain API, so we need to "exclude" it from the reserved set in
// this case.
if (forCreate)
reserved.remove("Name");

return reserved;
}
// Issue 48810: See SampleTypeService.createSampleType hasNameProperty. We are adding in a "name" col on the
// client side right before calling the saveDomain API, so we need to "exclude" it from the reserved set in
// this case.
if (domain == null && forCreate)
reserved.remove("Name");

ExpSampleType st = getSampleType(domain);
if (st == null)
return reserved;

try
{
Map<String, String> aliases = st.getImportAliases();
reserved.addAll(aliases.keySet());
}
catch (IOException e)
{
logger.error("Failed to parse SampleType parent aliases for [{}]", st.getRowId(), e);
}
return reserved;
}

Expand Down Expand Up @@ -474,20 +451,7 @@ public void validateOptions(Container container, User user, SampleTypeDomainKind
Domain stDomain = sampleType != null ? sampleType.getDomain() : null;
Set<String> reservedNames = new CaseInsensitiveHashSet(this.getReservedPropertyNames(stDomain, user));
reservedNames.add("Parent"); // Issue 52627: Don't allow "Parent" to be used for lineage parent import alias
Set<String> existingAliases = new CaseInsensitiveHashSet();

try
{
if (sampleType != null)
existingAliases = new CaseInsensitiveHashSet(sampleType.getImportAliases().keySet());
}
catch (IOException e)
{
throw UnexpectedException.wrap(e);
}

final Set<String> finalExistingAliases = existingAliases;
ExperimentService.validateParentAlias(aliasMap, reservedNames, finalExistingAliases, updatedDomainDesign, "sample type");
ExperimentService.validateParentAlias(aliasMap, reservedNames, updatedDomainDesign, "sample type");
}

List<? extends GWTPropertyDescriptor> properties = updatedDomainDesign.getFields();
Expand Down
4 changes: 2 additions & 2 deletions api/src/org/labkey/api/exp/property/AbstractDomainKind.java
Original file line number Diff line number Diff line change
Expand Up @@ -282,9 +282,9 @@ public Set<String> getMandatoryPropertyNames(Domain domain)
}

@Nullable
protected DomainTemplate getDomainTemplate(Domain domain)
protected DomainTemplate getDomainTemplate(@Nullable Domain domain)
{
return DomainTemplate.findTemplate(domain.getTemplateInfo(), getKindName());
return domain == null ? null : DomainTemplate.findTemplate(domain.getTemplateInfo(), getKindName());
}

@Override
Expand Down
14 changes: 14 additions & 0 deletions api/src/org/labkey/api/exp/property/DomainKind.java
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,20 @@ public Set<String> getReservedPropertyNamePrefixes()
return Collections.emptySet();
}

/**
* Names this kind creates as real domain properties itself, even though it also reserves them, so they must not be
* reported as illegal user-chosen names. Override when the kind materializes its own required properties into every
* domain -- see PlateMetadataDomainKind.ensureDomainProperties and IssueListDef.ensureDomainProperties.
*
* GH Issue 1589. Deliberately takes no Domain: it is called by the nightly name scan, which has no Domain and must
* not build one, so a kind that could only answer from a live Domain simply does not participate.
*/
@NotNull
public Set<String> getReservedNameExemptions()
{
return Collections.emptySet();
}

/**
* Return the set of names that are always required and cannot subsequently
* be deleted.
Expand Down
122 changes: 102 additions & 20 deletions api/src/org/labkey/api/exp/property/DomainUtil.java
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,7 @@
import java.util.Objects;
import java.util.Optional;
import java.util.Set;
import java.util.function.BiConsumer;
import java.util.regex.Matcher;
import java.util.regex.Pattern;
import java.util.stream.Collectors;
Expand All @@ -136,6 +137,25 @@ public class DomainUtil
public static final String ILLEGAL_DOMAIN_NAME_CHARSET = "<>[]{};,`\"~!@#$%^*=|?\\";
public static final Set<String> ILLEGAL_PROPERTY_NAMES = Set.of("*"); // Issue 53416

/** The field-naming rules enforced by {@link #validateProperties}, named so callers can tally them. GH Issue 1589. */
public enum FieldNameRule

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ran this locally against a database with a few projects. Results:

{
  "domainsSkipped": 84,
  "Run time": "2026-10-29 11:33:36.517",
  "labelVsReservedName": {
    "StudyDatasetDate": 12,
    "IntList": 44
  },
  "domainsScanned": 3096,
  "reservedName": {
    "SampleSet": 4,
    "IssueDefinition": 4,
    "PlateMetadata": 180
  }
}

Noting these here since they inform my feedback.

{
starName,
substitutionToken,
newlineInName,
reservedPrefix,
reservedName,
namingPatternConflict,
duplicateName,
duplicateImportAlias,
importAliasVsFieldName,
importAliasVsReservedName,
duplicateLabel,
labelVsFieldName,
labelVsImportAlias,
labelVsReservedName
}

private DomainUtil()
{
}
Expand Down Expand Up @@ -1590,18 +1610,35 @@ private static String getDomainErrorMessage(@Nullable GWTDomain<?> domain, Strin
*/
public static ValidationException validateProperties(@Nullable Domain domain, @NotNull GWTDomain<?> updates, @Nullable DomainKind domainKind, @Nullable GWTDomain<?> orig, @Nullable User user)
{
Set<String> reservedNames = null != domainKind ? new CaseInsensitiveHashSet(domainKind.getReservedPropertyNames(domain, user, domain == null))
: new CaseInsensitiveHashSet(updates.getReservedFieldNames());
Set<String> reservedNames = null != domainKind ? domainKind.getReservedPropertyNames(domain, user, domain == null)
: updates.getReservedFieldNames();
Set<String> reservedPrefixes = (null != domain && null != domainKind) ? domainKind.getReservedPropertyNamePrefixes() : updates.getReservedFieldNamePrefixes();
boolean allowMultiChoice = domainKind != null ? allowMultiChoice(domainKind) : updates.isAllowMultiChoiceProperties();
boolean supportsNamingPattern = domainKind != null && domainKind.supportsNamingPattern();

return validateProperties(updates, reservedNames, reservedPrefixes, allowMultiChoice, supportsNamingPattern,
getOriginalFieldPropertyIdNameMap(orig), null);
}

/**
* Applies the field-naming rules to an already-resolved rule set, so a caller that has no {@link Domain} can run
* them. GH Issue 1589: the nightly scan over stored domains resolves the reserved names itself and passes a
* {@code tally} to count violations by {@link FieldNameRule}; {@code tally} is null on the save path.
*/
public static ValidationException validateProperties(@NotNull GWTDomain<?> updates, @NotNull Set<String> reservedNameSet,
@NotNull Set<String> reservedPrefixes, boolean allowMultiChoice, boolean supportsNamingPattern,
@Nullable Map<Integer, String> propertyIdNameMap, @Nullable BiConsumer<FieldNameRule, GWTPropertyDescriptor> tally)
{
Set<String> reservedNames = new CaseInsensitiveHashSet(reservedNameSet);
Map<String, Integer> namePropertyIdMap = new CaseInsensitiveHashMap<>();
Map<String, String> altNameMap = new CaseInsensitiveHashMap<>();
// GH Issue 1257: import alias -> the fields declaring it.
Map<String, List<GWTPropertyDescriptor>> importAliasMap = new CaseInsensitiveLinkedHashMap<>();
// GH Issue 1589: explicit label -> the fields carrying it. Only populated when tallying, since label collisions
// are counted but not rejected.
Map<String, List<GWTPropertyDescriptor>> labelMap = new CaseInsensitiveLinkedHashMap<>();
Set<String> fieldNames = new CaseInsensitiveHashSet();
ValidationException exception = new ValidationException();
Map<Integer, String> propertyIdNameMap = getOriginalFieldPropertyIdNameMap(orig);//key: orig property id, value : orig field name

boolean allowMultiChoice = domainKind != null ? allowMultiChoice(domainKind) : updates.isAllowMultiChoiceProperties();
for (GWTPropertyDescriptor field : updates.getFields(true))
{
String name = field.getName();
Expand All @@ -1625,8 +1662,19 @@ public static ValidationException validateProperties(@Nullable Domain domain, @N
importAliasMap.computeIfAbsent(alias, k -> new ArrayList<>()).add(field);
}

if (tally != null)
{
// Only an explicitly set label counts. A label derived from the field name is not something anyone
// chose, and the only way to resolve a collision on one is to rename the field.
String label = StringUtils.trimToNull(field.getLabel());
// A label repeating its own field's name is redundant but not ambiguous.
if (label != null && !label.equalsIgnoreCase(name))
labelMap.computeIfAbsent(label, k -> new ArrayList<>()).add(field);
}

if (ILLEGAL_PROPERTY_NAMES.contains(name.trim()))
{
tally(tally, FieldNameRule.starName, field);
exception.addError(new SimpleValidationError(getDomainErrorMessage(updates, "The field name '" + name + "' is not allowed.")));
}

Expand All @@ -1649,13 +1697,15 @@ public static ValidationException validateProperties(@Nullable Domain domain, @N
Matcher expMatcher = SUBSTITUTION_EXP_PATTERN.matcher(name);
if (expMatcher.find())
{
tally(tally, FieldNameRule.substitutionToken, field);
exception.addFieldError(name, getDomainErrorMessage(updates, "The '${}' substitution token is reserved for system use."));
continue;
}

// Issue 52746: SM Import: Newlines in field names on import
if (StringUtils.containsAny(name, "\t\n\r"))
{
tally(tally, FieldNameRule.newlineInName, field);
exception.addFieldError(name, getDomainErrorMessage(updates, "Field name may not contain 'tab', 'new line', or 'return' characters."));
continue;
}
Expand All @@ -1666,6 +1716,7 @@ public static ValidationException validateProperties(@Nullable Domain domain, @N
Optional<String> reservedPrefix = reservedPrefixes.stream().filter(prefix -> lcName.startsWith(prefix.toLowerCase())).findAny();
if (reservedPrefix.isPresent())
{
tally(tally, FieldNameRule.reservedPrefix, field);
exception.addFieldError(name, getDomainErrorMessage(updates, "The prefix '" + reservedPrefix.get() + "' is reserved for system use."));
continue;
}
Expand All @@ -1677,44 +1728,33 @@ public static ValidationException validateProperties(@Nullable Domain domain, @N
String origFieldName = (null != propertyIdNameMap ? propertyIdNameMap.get(field.getPropertyId()) : null);
if (field.getPropertyId() <= 0 || !name.equalsIgnoreCase(origFieldName))
{
tally(tally, FieldNameRule.reservedName, field);
exception.addFieldError(name, getDomainErrorMessage(updates, ("'" + name + "' is a reserved field name in '" + updates.getName() + "'.")));
}
continue;
}

if (domainKind != null && domainKind.supportsNamingPattern())
if (supportsNamingPattern)
{
String nameSyntaxError = NameGenerator.validateFieldKeyConflict(name);
if (nameSyntaxError != null)
{
tally(tally, FieldNameRule.namingPatternConflict, field);
exception.addFieldError(name, nameSyntaxError);
continue;
}
}

if (namePropertyIdMap.containsKey(name))
{
tally(tally, FieldNameRule.duplicateName, field);
String errorMsg = getDomainErrorMessage(updates, "The field name '" + name + "' is already taken. Please provide a unique name for each field.");
PropertyValidationError propertyValidationError = new PropertyValidationError(errorMsg, name, field.getPropertyId());
exception.addError(propertyValidationError);
}
else
{
namePropertyIdMap.put(name, field.getPropertyId());

// Issue 52827: File/attachment fields with special characters
if (altNameMap.containsKey(name))
{
String errorMsg = getDomainErrorMessage(updates, "The field name '" + name + "' cannot be used with another field '" + altNameMap.get(name) + "'. Please provide a different name for the field.");
PropertyValidationError propertyValidationError = new PropertyValidationError(errorMsg, name, field.getPropertyId());
exception.addError(propertyValidationError);
}
else
{
altNameMap.put(name, name);
}
}

if (field.getValueExpression() != null && field.getValueExpression().trim().length() > 4000)
{
exception.addFieldError(name, getDomainErrorMessage(updates, "The value expression for '" + name
Expand All @@ -1728,19 +1768,55 @@ public static ValidationException validateProperties(@Nullable Domain domain, @N
String names = "'" + fields.stream().map(GWTPropertyDescriptor::getName).sorted().collect(Collectors.joining("', '")) + "'";

if (fields.size() > 1)
{
fields.forEach(f -> tally(tally, FieldNameRule.duplicateImportAlias, f));
addImportAliasErrors(exception, updates, fields, "Duplicate import alias '" + alias + "' for fields " + names + ".");
}

if (fieldNames.contains(alias))
{
fields.forEach(f -> tally(tally, FieldNameRule.importAliasVsFieldName, f));
addImportAliasErrors(exception, updates, fields, "Import alias '" + alias + "' on field" + (fields.size() == 1 ? " " : "s ") + names + " conflicts with a field name.");
}

// GH Issue 1474: 'Container' should not be allowed as valid Import Alias
if (reservedNames.contains(alias))
{
fields.forEach(f -> tally(tally, FieldNameRule.importAliasVsReservedName, f));
addImportAliasErrors(exception, updates, fields, "Import alias '" + alias + "' on field" + (fields.size() == 1 ? " " : "s ") + names + " conflicts with a reserved field name.");
}
});

// GH Issue 1589: tally-only. ImportAliasable.Helper.createImportMap resolves propertyURI, name, label and import
// alias into one case-insensitive map at precedence propertyURI < propName < alias < label < name, so a label
// that ties with any of them silently loses the token or steals it. Counted, not rejected: these configurations
// save successfully today and blocking them would break existing domains.
labelMap.forEach((label, fields) -> {
if (fields.size() > 1)
fields.forEach(f -> tally(tally, FieldNameRule.duplicateLabel, f));

// A name outranks a label, so these fields' labels are unreachable at import.
if (fieldNames.contains(label))
fields.forEach(f -> tally(tally, FieldNameRule.labelVsFieldName, f));

// A label outranks an alias, so here the colliding alias is the unreachable one. One field owning both its
// own label and the matching alias resolves to that field either way, so it is redundant, not ambiguous.
List<GWTPropertyDescriptor> aliasFields = importAliasMap.get(label);
if (aliasFields != null && !isSameSingleField(fields, aliasFields))
fields.forEach(f -> tally(tally, FieldNameRule.labelVsImportAlias, f));

if (reservedNames.contains(label))
fields.forEach(f -> tally(tally, FieldNameRule.labelVsReservedName, f));
});

return exception;
}

private static boolean isSameSingleField(List<GWTPropertyDescriptor> a, List<GWTPropertyDescriptor> b)
{
return a.size() == 1 && b.size() == 1 && a.get(0) == b.get(0);
}

private static DbScope getDomainScope(DomainKind<?> kind)
{
return Objects.requireNonNullElse(kind.getScope(), DbScope.getLabKeyScope());
Expand Down Expand Up @@ -1772,6 +1848,12 @@ private static boolean isLookupChanged(GWTPropertyDescriptor field, @Nullable GW
|| !Strings.CI.equals(field.getLookupQuery(), origField.getLookupQuery());
}

private static void tally(@Nullable BiConsumer<FieldNameRule, GWTPropertyDescriptor> tally, FieldNameRule rule, GWTPropertyDescriptor field)
{
if (tally != null)
tally.accept(rule, field);
}

/** 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<GWTPropertyDescriptor> fields, String message)
{
Expand Down
Loading
Loading