From 96517f08b433146507f4bc1c17d34478a93a41a0 Mon Sep 17 00:00:00 2001 From: Adam Rauch Date: Tue, 26 Aug 2025 12:51:49 -0700 Subject: [PATCH 01/16] Issue 52913: Fix determination of existing index names when dropping --- .../api/audit/AbstractAuditTypeProvider.java | 53 ++----------------- .../audit/query/AbstractAuditDomainKind.java | 5 -- .../api/exp/api/StorageProvisioner.java | 8 +-- .../api/property/StorageProvisionerImpl.java | 53 ++++++++++++++++--- 4 files changed, 57 insertions(+), 62 deletions(-) diff --git a/api/src/org/labkey/api/audit/AbstractAuditTypeProvider.java b/api/src/org/labkey/api/audit/AbstractAuditTypeProvider.java index 366aa1484c1..7b7c97d2836 100644 --- a/api/src/org/labkey/api/audit/AbstractAuditTypeProvider.java +++ b/api/src/org/labkey/api/audit/AbstractAuditTypeProvider.java @@ -23,7 +23,6 @@ import org.labkey.api.audit.query.DefaultAuditTypeTable; import org.labkey.api.collections.CaseInsensitiveHashMap; import org.labkey.api.data.AbstractTableInfo; -import org.labkey.api.data.ColumnInfo; import org.labkey.api.data.Container; import org.labkey.api.data.ContainerFilter; import org.labkey.api.data.ContainerManager; @@ -31,11 +30,8 @@ import org.labkey.api.data.DbSchemaType; import org.labkey.api.data.DbScope; import org.labkey.api.data.MutableColumnInfo; -import org.labkey.api.data.PropertyStorageSpec; import org.labkey.api.data.SQLFragment; -import org.labkey.api.data.SchemaTableInfo; import org.labkey.api.data.SqlExecutor; -import org.labkey.api.data.TableChange; import org.labkey.api.data.TableInfo; import org.labkey.api.dataiterator.DataIterator; import org.labkey.api.dataiterator.ExistingRecordDataIterator; @@ -55,14 +51,12 @@ import org.labkey.api.security.User; import org.labkey.api.util.DateUtil; import org.labkey.api.util.PageFlowUtil; -import org.labkey.api.util.Pair; import org.labkey.api.view.ActionURL; import java.sql.Time; import java.util.Collection; import java.util.Collections; import java.util.Date; -import java.util.HashSet; import java.util.LinkedHashMap; import java.util.LinkedHashSet; import java.util.List; @@ -145,52 +139,15 @@ public void initializeProvider(User user) ensureProperties(user, domain); } + // TODO: eliminate this method... call StorageProvisioner directly private void updateIndices(Domain domain, AbstractAuditDomainKind domainKind) { - if (domain.getStorageTableName() == null) - return; - - // Issue 50059, acquiring the schema table info this way ensures that the domain fields are properly fixed up. See ProvisionedSchemaOptions. - SchemaTableInfo schemaTableInfo = StorageProvisioner.get().getSchemaTableInfo(domain); - if (schemaTableInfo != null) - { - Map>> existingIndices = schemaTableInfo.getAllIndices(); - Set newIndices = new HashSet<>(domainKind.getPropertyIndices(domain)); - Set toRemove = new HashSet<>(); - for (String name : existingIndices.keySet()) - { - if (existingIndices.get(name).first == TableInfo.IndexType.Primary) - continue; - Pair> columnIndex = existingIndices.get(name); - String[] columnNames = new String[columnIndex.second.size()]; - for (int i = 0; i < columnIndex.second.size(); i++) - { - columnNames[i] = columnIndex.second.get(i).getColumnName(); - } - PropertyStorageSpec.Index existingIndex = new PropertyStorageSpec.Index(columnIndex.first == TableInfo.IndexType.Unique, columnNames); - boolean foundIt = false; - for (PropertyStorageSpec.Index propertyIndex : newIndices) - { - if (PropertyStorageSpec.Index.isSameIndex(propertyIndex, existingIndex)) - { - foundIt = true; - newIndices.remove(propertyIndex); - break; - } - } - - if (!foundIt) - toRemove.add(name); - } - - if (!toRemove.isEmpty()) - StorageProvisioner.get().dropTableIndices(domain, toRemove); - if (!newIndices.isEmpty()) - StorageProvisioner.get().addTableIndices(domain, newIndices, TableChange.IndexSizeMode.Normal); - } + assert domain.getStorageTableName() != null; + assert domainKind != null; + assert domain.getDomainKind() != null; + StorageProvisioner.get().ensureTableIndices(domain, domainKind); } - // NOTE: Changing the name of an existing PropertyDescriptor will lose data! private void ensureProperties(User user, Domain domain) { diff --git a/api/src/org/labkey/api/audit/query/AbstractAuditDomainKind.java b/api/src/org/labkey/api/audit/query/AbstractAuditDomainKind.java index 6b48c6f29ac..f24f08f44ec 100644 --- a/api/src/org/labkey/api/audit/query/AbstractAuditDomainKind.java +++ b/api/src/org/labkey/api/audit/query/AbstractAuditDomainKind.java @@ -55,13 +55,8 @@ import java.util.LinkedHashSet; import java.util.Set; -/** - * User: klum - * Date: 7/8/13 - */ public abstract class AbstractAuditDomainKind extends DomainKind { - private static final String XAR_SUBSTITUTION_SCHEMA_NAME = "SchemaName"; private static final String XAR_SUBSTITUTION_TABLE_NAME = "TableName"; diff --git a/api/src/org/labkey/api/exp/api/StorageProvisioner.java b/api/src/org/labkey/api/exp/api/StorageProvisioner.java index 52a05ca79a0..56aab145733 100644 --- a/api/src/org/labkey/api/exp/api/StorageProvisioner.java +++ b/api/src/org/labkey/api/exp/api/StorageProvisioner.java @@ -22,7 +22,6 @@ import org.labkey.api.data.DbScope; import org.labkey.api.data.PropertyStorageSpec; import org.labkey.api.data.SchemaTableInfo; -import org.labkey.api.data.TableChange; import org.labkey.api.data.TableInfo; import org.labkey.api.exp.PropertyDescriptor; import org.labkey.api.exp.property.Domain; @@ -83,10 +82,13 @@ static TableInfo createTableInfo(@NotNull Domain domain) */ void createStorageTable(Domain domain, DomainKind kind, DbScope scope); + @Deprecated // Call ensureTableIndices() instead void dropNotRequiredIndices(Domain domain); + @Deprecated // Call ensureTableIndices() instead void addMissingRequiredIndices(Domain domain); - void addTableIndices(Domain domain, Set indices, TableChange.IndexSizeMode sizeMode); - void dropTableIndices(Domain domain, Set indexNames); + + // Passing in DomainKind because domains can't be trusted to know the correct DomainKind. We might fix this... + void ensureTableIndices(@NotNull Domain domain, @NotNull DomainKind kind); SchemaTableInfo getSchemaTableInfo(Domain domain); diff --git a/experiment/src/org/labkey/experiment/api/property/StorageProvisionerImpl.java b/experiment/src/org/labkey/experiment/api/property/StorageProvisionerImpl.java index 5e451c144da..e9be153f14a 100644 --- a/experiment/src/org/labkey/experiment/api/property/StorageProvisionerImpl.java +++ b/experiment/src/org/labkey/experiment/api/property/StorageProvisionerImpl.java @@ -1008,8 +1008,7 @@ private static void ensureIndexToBeAddedHasNoPrimaryKeys(SchemaTableInfo schemaT } } - @Override - public void addTableIndices(Domain domain, Set indices, TableChange.IndexSizeMode sizeMode) + private void addTableIndices(Domain domain, Set indices, TableChange.IndexSizeMode sizeMode) { DbScope scope = validateDomain(domain); @@ -1042,8 +1041,7 @@ private DbScope validateDomain(Domain domain) return scope; } - @Override - public void dropTableIndices(Domain domain, Set indexNames) + private void dropTableIndices(Domain domain, Set indexNames) { DbScope scope = validateDomain(domain); @@ -1060,6 +1058,49 @@ public void dropTableIndices(Domain domain, Set indexNames) } } + @Override + public void ensureTableIndices(@NotNull Domain domain, @NotNull DomainKind kind) + { + // Issue 50059, acquiring the schema table info this way ensures that the domain fields are properly fixed up. See ProvisionedSchemaOptions. + SchemaTableInfo schemaTableInfo = StorageProvisioner.get().getSchemaTableInfo(domain); + if (schemaTableInfo != null) + { + Map>> existingIndices = schemaTableInfo.getAllIndices(); + Set newIndices = new HashSet<>(kind.getPropertyIndices(domain)); + Set toRemove = new HashSet<>(); + for (String name : existingIndices.keySet()) + { + if (existingIndices.get(name).first == TableInfo.IndexType.Primary) + continue; + Pair> columnIndex = existingIndices.get(name); + String[] columnNames = new String[columnIndex.second.size()]; + for (int i = 0; i < columnIndex.second.size(); i++) + { + columnNames[i] = columnIndex.second.get(i).getColumnName(); + } + PropertyStorageSpec.Index existingIndex = new PropertyStorageSpec.Index(columnIndex.first == TableInfo.IndexType.Unique, columnNames); + boolean foundIt = false; + for (PropertyStorageSpec.Index propertyIndex : newIndices) + { + if (PropertyStorageSpec.Index.isSameIndex(propertyIndex, existingIndex)) + { + foundIt = true; + newIndices.remove(propertyIndex); + break; + } + } + + if (!foundIt) + toRemove.add(name); + } + + if (!toRemove.isEmpty()) + dropTableIndices(domain, toRemove); + if (!newIndices.isEmpty()) + addTableIndices(domain, newIndices, TableChange.IndexSizeMode.Normal); + } + } + private static DbScope getScope(Domain domain) { return getDomainKind(domain).getScope(); @@ -1828,8 +1869,8 @@ public Set getReservedPropertyNames(Domain domain, User user) { d = new DomainImpl(c, uri, "test", true) { - @Override @Nullable - public DomainKind getDomainKind() + @Override + public @NotNull DomainKind getDomainKind() { return k; } From f9215fcc1d74453fdc7f4c2d5800e530923c8948 Mon Sep 17 00:00:00 2001 From: Adam Rauch Date: Wed, 27 Aug 2025 11:21:27 -0700 Subject: [PATCH 02/16] Junit test that flags overlapping audit domain kind prefixes --- api/src/org/labkey/api/ApiModule.java | 2 + .../audit/query/AbstractAuditDomainKind.java | 37 +++++++++++++++++++ .../org/labkey/api/data/DbSchemaCache.java | 9 +---- .../SecurityEscalationAuditProvider.java | 11 ++---- .../StudySecurityEscalationAuditProvider.java | 8 ++-- .../org/labkey/audit/model/LogManager.java | 1 - 6 files changed, 48 insertions(+), 20 deletions(-) diff --git a/api/src/org/labkey/api/ApiModule.java b/api/src/org/labkey/api/ApiModule.java index 201e7fb564c..910b20e288f 100644 --- a/api/src/org/labkey/api/ApiModule.java +++ b/api/src/org/labkey/api/ApiModule.java @@ -32,6 +32,7 @@ import org.labkey.api.attachments.ImageServlet; import org.labkey.api.attachments.LookAndFeelResourceType; import org.labkey.api.attachments.SecureDocumentType; +import org.labkey.api.audit.query.AbstractAuditDomainKind; import org.labkey.api.cache.BlockingCache; import org.labkey.api.collections.ArrayListMap; import org.labkey.api.collections.CaseInsensitiveHashMap; @@ -463,6 +464,7 @@ public void registerServlets(ServletContext servletCtx) public @NotNull Set getIntegrationTests() { return Set.of( + AbstractAuditDomainKind.TestCase.class, AbstractForeignKey.TestCase.class, AbstractQueryUpdateService.TestCase.class, ActionURL.TestCase.class, diff --git a/api/src/org/labkey/api/audit/query/AbstractAuditDomainKind.java b/api/src/org/labkey/api/audit/query/AbstractAuditDomainKind.java index f24f08f44ec..79124f7c792 100644 --- a/api/src/org/labkey/api/audit/query/AbstractAuditDomainKind.java +++ b/api/src/org/labkey/api/audit/query/AbstractAuditDomainKind.java @@ -15,11 +15,15 @@ */ package org.labkey.api.audit.query; +import org.apache.commons.collections4.MultiValuedMap; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; import org.json.JSONObject; +import org.junit.Assert; +import org.junit.Test; import org.labkey.api.audit.AbstractAuditTypeProvider; import org.labkey.api.collections.CaseInsensitiveHashSet; +import org.labkey.api.collections.LabKeyCollectors; import org.labkey.api.data.Container; import org.labkey.api.data.ContainerManager; import org.labkey.api.data.DbSchema; @@ -38,6 +42,7 @@ import org.labkey.api.exp.property.Domain; import org.labkey.api.exp.property.DomainKind; import org.labkey.api.exp.property.DomainProperty; +import org.labkey.api.exp.property.PropertyService; import org.labkey.api.exp.xar.LsidUtils; import org.labkey.api.gwt.client.model.GWTDomain; import org.labkey.api.gwt.client.model.GWTPropertyDescriptor; @@ -50,9 +55,12 @@ import org.labkey.api.view.NavTree; import org.labkey.api.writer.ContainerUser; +import java.util.Collection; import java.util.Collections; import java.util.HashSet; import java.util.LinkedHashSet; +import java.util.List; +import java.util.Map; import java.util.Set; public abstract class AbstractAuditDomainKind extends DomainKind @@ -413,4 +421,33 @@ public boolean isUserCreatedType() { return false; } + + public static class TestCase extends Assert + { + @Test + public void flagDuplicateNamespacePrefixes() + { + MultiValuedMap mmap = PropertyService.get().getDomainKinds().stream() + .filter(AbstractAuditDomainKind.class::isInstance) + .map(AbstractAuditDomainKind.class::cast) + .collect(LabKeyCollectors.toMultiValuedMap(AbstractAuditDomainKind::getNamespacePrefix, dk -> dk.getClass().getSimpleName())); + + Map> map = mmap.asMap(); + map.entrySet().stream() + .filter(e -> e.getValue().size() > 1) + .forEach(e -> LOG.warn("{} share the same namespace prefix: \"{}\"!", e.getValue(), e.getKey())); + + for (Map.Entry> e1 : map.entrySet()) + { + String key1 = e1.getKey(); + List overlappers = map.entrySet().stream() + .filter(e2 -> !e1.equals(e2) && e2.getKey().startsWith(key1)) + .map(e2 -> "\"" + e2.getKey() + "\"") + .toList(); + + if (!overlappers.isEmpty()) + LOG.warn("Prefix \"{}\" ({}) overlaps with prefixes {})!", key1, e1.getValue(), overlappers); + } + } + } } diff --git a/api/src/org/labkey/api/data/DbSchemaCache.java b/api/src/org/labkey/api/data/DbSchemaCache.java index d26a6a7c11c..5657c72b989 100644 --- a/api/src/org/labkey/api/data/DbSchemaCache.java +++ b/api/src/org/labkey/api/data/DbSchemaCache.java @@ -26,12 +26,6 @@ import org.labkey.api.cache.CacheTimeChooser; import org.labkey.api.module.ModuleLoader; -/* -* User: adam -* Date: Mar 20, 2011 -* Time: 2:53:51 PM -*/ - // Every scope has its own cache of DbSchemas public class DbSchemaCache { @@ -42,7 +36,6 @@ public class DbSchemaCache // Ask the DbSchemaType how long to cache each schema private final CacheTimeChooser SCHEMA_CACHE_TIME_CHOOSER = (key, argument) -> { - @SuppressWarnings({"unchecked"}) SchemaDetails details = (SchemaDetails)argument; return details.getType().getCacheTimeToLive(); }; @@ -69,7 +62,7 @@ public DbSchemaCache(DbScope scope) void remove(String schemaName, DbSchemaType type) { - LOG.debug("remove " + type + " schema: " + schemaName); + LOG.debug("remove {} schema: {}", type, schemaName); _cache.removeUsingFilter(new Cache.StringPrefixFilter(getKey(schemaName, type))); } diff --git a/api/src/org/labkey/api/study/security/SecurityEscalationAuditProvider.java b/api/src/org/labkey/api/study/security/SecurityEscalationAuditProvider.java index 2f6a9bbb799..75e1f602f4e 100644 --- a/api/src/org/labkey/api/study/security/SecurityEscalationAuditProvider.java +++ b/api/src/org/labkey/api/study/security/SecurityEscalationAuditProvider.java @@ -15,8 +15,6 @@ */ package org.labkey.api.study.security; -import org.apache.logging.log4j.LogManager; -import org.apache.logging.log4j.Logger; import org.labkey.api.audit.AbstractAuditTypeProvider; import org.labkey.api.audit.AuditTypeEvent; import org.labkey.api.audit.AuditTypeProvider; @@ -45,7 +43,6 @@ /** * This is an abstract class that represents an Audit Log for a Security Escalator. To use it, you need to * extend it, and then register it with the AuditLogManager. - * * * // Register the Security Escalation Audit Log * AuditLogService.get().registerAuditType(new SecurityEscalatorAuditProvider()); @@ -53,8 +50,6 @@ */ public abstract class SecurityEscalationAuditProvider extends AbstractAuditTypeProvider implements AuditTypeProvider { - private static final Logger _log = LogManager.getLogger(SecurityEscalationAuditProvider.class); - protected SecurityEscalationAuditProvider() { super(); @@ -92,10 +87,10 @@ public enum CustomColumn { LEVEL ("level", "Level", PropertyType.INTEGER) ; - String displayName; - String columnName; + final String displayName; + final String columnName; boolean isHidden = false; - PropertyType type; + final PropertyType type; CustomColumn(String columnName, String displayName, PropertyType type) { this.displayName = displayName; diff --git a/api/src/org/labkey/api/study/security/StudySecurityEscalationAuditProvider.java b/api/src/org/labkey/api/study/security/StudySecurityEscalationAuditProvider.java index b15658ee964..03f3c0100a1 100644 --- a/api/src/org/labkey/api/study/security/StudySecurityEscalationAuditProvider.java +++ b/api/src/org/labkey/api/study/security/StudySecurityEscalationAuditProvider.java @@ -47,7 +47,7 @@ public String getAuditLogTitle() { public StudySecurityEscalationAuditProvider() { - super(new StudySecurityEscalationDomain()); + super(new StudySecurityEscalationDomainKind()); } public static class StudySecurityEscalationEvent extends SecurityEscalationEvent @@ -67,8 +67,10 @@ public String getEventType() { } } - public static class StudySecurityEscalationDomain extends SecurityEscalationAuditDomainKind { - public StudySecurityEscalationDomain() { + public static class StudySecurityEscalationDomainKind extends SecurityEscalationAuditDomainKind + { + public StudySecurityEscalationDomainKind() + { super(EVENT_TYPE, StudySecurityEscalationEvent.class.getName()); } } diff --git a/audit/src/org/labkey/audit/model/LogManager.java b/audit/src/org/labkey/audit/model/LogManager.java index 91984edc2a9..0f64bba8367 100644 --- a/audit/src/org/labkey/audit/model/LogManager.java +++ b/audit/src/org/labkey/audit/model/LogManager.java @@ -291,5 +291,4 @@ private K validateFields(@NotNull AuditTypeProvider p else return type; } - } From 4114dd696bd1624f91f23ce8354384a665d97536 Mon Sep 17 00:00:00 2001 From: Adam Rauch Date: Wed, 27 Aug 2025 19:03:35 -0700 Subject: [PATCH 03/16] Correct and uniquify some domain namespace prefixes --- .../audit/query/AbstractAuditDomainKind.java | 46 +++++++++++++------ .../SecurityEscalationAuditProvider.java | 8 +--- .../StudySecurityEscalationAuditProvider.java | 6 +++ .../postgresql/exp-25.007-25.008.sql | 10 ++++ .../dbscripts/sqlserver/exp-25.007-25.008.sql | 10 ++++ .../labkey/experiment/ExperimentModule.java | 3 +- .../experiment/api/ExperimentServiceImpl.java | 44 ++++++++++++++++-- .../query/audit/QueryExportAuditProvider.java | 5 +- 8 files changed, 102 insertions(+), 30 deletions(-) create mode 100644 experiment/resources/schemas/dbscripts/postgresql/exp-25.007-25.008.sql create mode 100644 experiment/resources/schemas/dbscripts/sqlserver/exp-25.007-25.008.sql diff --git a/api/src/org/labkey/api/audit/query/AbstractAuditDomainKind.java b/api/src/org/labkey/api/audit/query/AbstractAuditDomainKind.java index 79124f7c792..3cbcc5cc038 100644 --- a/api/src/org/labkey/api/audit/query/AbstractAuditDomainKind.java +++ b/api/src/org/labkey/api/audit/query/AbstractAuditDomainKind.java @@ -50,6 +50,7 @@ import org.labkey.api.query.QueryService; import org.labkey.api.query.ValidationException; import org.labkey.api.security.User; +import org.labkey.api.settings.OptionalFeatureService; import org.labkey.api.util.PageFlowUtil; import org.labkey.api.view.ActionURL; import org.labkey.api.view.NavTree; @@ -59,9 +60,14 @@ import java.util.Collections; import java.util.HashSet; import java.util.LinkedHashSet; +import java.util.LinkedList; import java.util.List; import java.util.Map; +import java.util.Objects; import java.util.Set; +import java.util.stream.Collectors; + +import static org.labkey.api.audit.query.DefaultAuditTypeTable.LEGACY_UNION_AUDIT_TABLE; public abstract class AbstractAuditDomainKind extends DomainKind { @@ -396,7 +402,9 @@ public Set getNonProvisionedTableNames() // omit the legacy auditlog table, this can be removed once the // table is dropped after migration Set tables = new CaseInsensitiveHashSet(); - tables.add("auditlog"); + + if (OptionalFeatureService.get().isFeatureEnabled(LEGACY_UNION_AUDIT_TABLE)) + tables.add("auditlog"); return tables; } @@ -424,30 +432,38 @@ public boolean isUserCreatedType() public static class TestCase extends Assert { + // Also see AuditDomainUriTest which tests the DomainURIs in the database @Test public void flagDuplicateNamespacePrefixes() { + // For now, simply log domain kinds that share the same prefix and those that are overlapping MultiValuedMap mmap = PropertyService.get().getDomainKinds().stream() .filter(AbstractAuditDomainKind.class::isInstance) .map(AbstractAuditDomainKind.class::cast) .collect(LabKeyCollectors.toMultiValuedMap(AbstractAuditDomainKind::getNamespacePrefix, dk -> dk.getClass().getSimpleName())); Map> map = mmap.asMap(); - map.entrySet().stream() + + List failures = map.entrySet().stream() .filter(e -> e.getValue().size() > 1) - .forEach(e -> LOG.warn("{} share the same namespace prefix: \"{}\"!", e.getValue(), e.getKey())); - - for (Map.Entry> e1 : map.entrySet()) - { - String key1 = e1.getKey(); - List overlappers = map.entrySet().stream() - .filter(e2 -> !e1.equals(e2) && e2.getKey().startsWith(key1)) - .map(e2 -> "\"" + e2.getKey() + "\"") - .toList(); - - if (!overlappers.isEmpty()) - LOG.warn("Prefix \"{}\" ({}) overlaps with prefixes {})!", key1, e1.getValue(), overlappers); - } + .map(e -> e.getValue() + " share the same namespace prefix: \"" + e.getKey() + "\"!") + .collect(Collectors.toCollection(LinkedList::new)); + + failures.addAll(map.entrySet().stream() + .map(e1 -> { + String key1 = e1.getKey(); + List overlappers = map.entrySet().stream() + .filter(e2 -> !e1.equals(e2) && e2.getKey().startsWith(key1)) + .map(e2 -> "\"" + e2.getKey() + "\"") + .toList(); + + return overlappers.isEmpty() ? null : "Prefix " + key1 + " (" + e1.getValue() + ") overlaps with prefixes " + overlappers + ")!"; + }) + .filter(Objects::nonNull) + .toList()); + + if (!failures.isEmpty()) + Assert.fail(failures.toString()); } } } diff --git a/api/src/org/labkey/api/study/security/SecurityEscalationAuditProvider.java b/api/src/org/labkey/api/study/security/SecurityEscalationAuditProvider.java index 75e1f602f4e..f15b48b5ad4 100644 --- a/api/src/org/labkey/api/study/security/SecurityEscalationAuditProvider.java +++ b/api/src/org/labkey/api/study/security/SecurityEscalationAuditProvider.java @@ -280,9 +280,10 @@ public void setStackTrace(String stackTrace) { */ public static abstract class SecurityEscalationAuditDomainKind extends AbstractAuditDomainKind { - private static final String NAMESPACE_PREFIX = "Audit-"; private static Set _fields = null; + protected static final String NAMESPACE_PREFIX = "Audit-"; + private final String _eventType; private final String _tableName; @@ -305,11 +306,6 @@ public String getKindName() { return _eventType; } - @Override - protected String getNamespacePrefix() { - return NAMESPACE_PREFIX; - } - /** * Returns a list of the custom columns to be recorded in the audit log. * diff --git a/api/src/org/labkey/api/study/security/StudySecurityEscalationAuditProvider.java b/api/src/org/labkey/api/study/security/StudySecurityEscalationAuditProvider.java index 03f3c0100a1..d3393974337 100644 --- a/api/src/org/labkey/api/study/security/StudySecurityEscalationAuditProvider.java +++ b/api/src/org/labkey/api/study/security/StudySecurityEscalationAuditProvider.java @@ -73,5 +73,11 @@ public StudySecurityEscalationDomainKind() { super(EVENT_TYPE, StudySecurityEscalationEvent.class.getName()); } + + @Override + protected String getNamespacePrefix() + { + return NAMESPACE_PREFIX + "StudySecurityEscalationDomain"; + } } } diff --git a/experiment/resources/schemas/dbscripts/postgresql/exp-25.007-25.008.sql b/experiment/resources/schemas/dbscripts/postgresql/exp-25.007-25.008.sql new file mode 100644 index 00000000000..5d3a3400d9a --- /dev/null +++ b/experiment/resources/schemas/dbscripts/postgresql/exp-25.007-25.008.sql @@ -0,0 +1,10 @@ +-- Update all "security escalation" domains with a proper namespace prefix +UPDATE exp.domainDescriptor SET DomainURI = REPLACE(DomainURI, '--', '-StudySecurityEscalationDomain-') WHERE StorageSchemaName = 'audit' AND (DomainURI ILIKE '%StudySecurityEscalationAuditProvider' OR DomainURI ILIKE '%StudySecurityEscalationEvent'); +UPDATE exp.domainDescriptor SET DomainURI = REPLACE(DomainURI, '--', '-EHRSecurityEscalationDomain-') WHERE StorageSchemaName = 'audit' AND DomainURI ILIKE '%EHRSecurityEscalationEvent'; + +-- Update LDAP sync domain with a unique namespace prefix so it doesn't overlap with the ONPRC version +UPDATE exp.domainDescriptor SET DomainURI = REPLACE(DomainURI, 'LdapSyncAuditDomain', 'PremiumLdapSyncAuditDomain') WHERE StorageSchemaName = 'audit' AND DomainURI ILIKE '%PremiumLdapAuditEvent'; + +-- Update query audit events that advertised the same generic namespace prefix +UPDATE exp.domainDescriptor SET DomainURI = REPLACE(DomainURI, 'QueryAuditDomain', 'QueryExportAuditDomain') WHERE StorageSchemaName = 'audit' AND DomainURI ILIKE '%QueryExportAuditEvent'; +UPDATE exp.domainDescriptor SET DomainURI = REPLACE(DomainURI, 'QueryAuditDomain', 'LoggedQueryAuditDomain') WHERE StorageSchemaName = 'audit' AND DomainURI ILIKE '%LoggedQuery'; \ No newline at end of file diff --git a/experiment/resources/schemas/dbscripts/sqlserver/exp-25.007-25.008.sql b/experiment/resources/schemas/dbscripts/sqlserver/exp-25.007-25.008.sql new file mode 100644 index 00000000000..dbfb724e1cf --- /dev/null +++ b/experiment/resources/schemas/dbscripts/sqlserver/exp-25.007-25.008.sql @@ -0,0 +1,10 @@ +-- Update all "security escalation" domains with a proper namespace prefix +UPDATE exp.domainDescriptor SET DomainURI = REPLACE(DomainURI, '--', '-StudySecurityEscalationDomain-') WHERE StorageSchemaName = 'audit' AND (DomainURI LIKE '%StudySecurityEscalationAuditProvider' OR DomainURI LIKE '%StudySecurityEscalationEvent'); +UPDATE exp.domainDescriptor SET DomainURI = REPLACE(DomainURI, '--', '-EHRSecurityEscalationDomain-') WHERE StorageSchemaName = 'audit' AND DomainURI LIKE '%EHRSecurityEscalationEvent'; + +-- Update LDAP sync domain with a unique namespace prefix so it doesn't overlap with the ONPRC version +UPDATE exp.domainDescriptor SET DomainURI = REPLACE(DomainURI, 'LdapSyncAuditDomain', 'PremiumLdapSyncAuditDomain') WHERE StorageSchemaName = 'audit' AND DomainURI LIKE '%PremiumLdapAuditEvent'; + +-- Update query audit events that advertised the same generic namespace prefix +UPDATE exp.domainDescriptor SET DomainURI = REPLACE(DomainURI, 'QueryAuditDomain', 'QueryExportAuditDomain') WHERE StorageSchemaName = 'audit' AND DomainURI LIKE '%QueryExportAuditEvent'; +UPDATE exp.domainDescriptor SET DomainURI = REPLACE(DomainURI, 'QueryAuditDomain', 'LoggedQueryAuditDomain') WHERE StorageSchemaName = 'audit' AND DomainURI LIKE '%LoggedQuery'; \ No newline at end of file diff --git a/experiment/src/org/labkey/experiment/ExperimentModule.java b/experiment/src/org/labkey/experiment/ExperimentModule.java index 34168ffa84b..3694ca50fb7 100644 --- a/experiment/src/org/labkey/experiment/ExperimentModule.java +++ b/experiment/src/org/labkey/experiment/ExperimentModule.java @@ -191,7 +191,7 @@ public String getName() @Override public Double getSchemaVersion() { - return 25.007; + return 25.008; } @Nullable @@ -1004,6 +1004,7 @@ public Set getIntegrationTests() DomainImpl.TestCase.class, DomainPropertyImpl.TestCase.class, ExpDataTableImpl.TestCase.class, + ExperimentServiceImpl.AuditDomainUriTest.class, ExperimentServiceImpl.LineageQueryTestCase.class, ExperimentServiceImpl.ParseInputOutputAliasTestCase.class, ExperimentServiceImpl.TestCase.class, diff --git a/experiment/src/org/labkey/experiment/api/ExperimentServiceImpl.java b/experiment/src/org/labkey/experiment/api/ExperimentServiceImpl.java index fd765db6570..52e0916627c 100644 --- a/experiment/src/org/labkey/experiment/api/ExperimentServiceImpl.java +++ b/experiment/src/org/labkey/experiment/api/ExperimentServiceImpl.java @@ -58,6 +58,7 @@ import org.labkey.api.cache.CacheManager; import org.labkey.api.collections.CaseInsensitiveHashMap; import org.labkey.api.collections.CaseInsensitiveHashSet; +import org.labkey.api.collections.CsvSet; import org.labkey.api.collections.LongArrayList; import org.labkey.api.collections.LongHashMap; import org.labkey.api.collections.Sets; @@ -74,6 +75,7 @@ import org.labkey.api.data.DbSchemaType; import org.labkey.api.data.DbScope; import org.labkey.api.data.DbSequenceManager; +import org.labkey.api.data.Filter; import org.labkey.api.data.JdbcType; import org.labkey.api.data.NameGenerator; import org.labkey.api.data.ObjectFactory; @@ -303,6 +305,7 @@ import static org.labkey.api.data.NameGenerator.EXPERIMENTAL_ALLOW_GAP_COUNTER; import static org.labkey.api.data.NameGenerator.EXPERIMENTAL_WITH_COUNTER; import static org.labkey.api.dataiterator.DataIteratorUtil.DUPLICATE_COLUMN_IN_DATA_ERROR; +import static org.labkey.api.exp.OntologyManager.getTinfoDomainDescriptor; import static org.labkey.api.exp.OntologyManager.getTinfoObject; import static org.labkey.api.exp.XarContext.XAR_JOB_ID_NAME; import static org.labkey.api.exp.api.ExpProtocol.ApplicationType.ExperimentRun; @@ -310,13 +313,11 @@ import static org.labkey.api.exp.api.ExpProtocol.ApplicationType.ProtocolApplication; import static org.labkey.api.exp.api.ExperimentJSONConverter.DATA_INPUTS_ALIAS_PREFIX; import static org.labkey.api.exp.api.ExperimentJSONConverter.MATERIAL_INPUTS_ALIAS_PREFIX; -import static org.labkey.api.util.IntegerUtils.asIntegerElseNull; -import static org.labkey.api.util.IntegerUtils.asLong; import static org.labkey.api.exp.api.NameExpressionOptionService.NAME_EXPRESSION_REQUIRED_MSG; import static org.labkey.api.exp.api.NameExpressionOptionService.NAME_EXPRESSION_REQUIRED_MSG_WITH_SUBFOLDERS; import static org.labkey.api.exp.api.ProvenanceService.PROVENANCE_PROTOCOL_LSID; -import static org.labkey.api.exp.query.ExpSchema.SCHEMA_EXP_DATA; -import static org.labkey.api.exp.query.SamplesSchema.SCHEMA_SAMPLES; +import static org.labkey.api.util.IntegerUtils.asIntegerElseNull; +import static org.labkey.api.util.IntegerUtils.asLong; import static org.labkey.api.util.IntegerUtils.asLongElseNull; import static org.labkey.experiment.api.SampleTypeServiceImpl.SampleChangeType.rollup; @@ -10308,6 +10309,41 @@ private int countMaterialInputObjects(Container c) } } + @SuppressWarnings("JUnitMalformedDeclaration") + public static class AuditDomainUriTest extends Assert + { + @Test + public void testAuditDomainUris() + { + // Each audit domain URI in the database should resolve to a unique DomainKind. A failure here indicates a + // problem with a DomainKind's namespace prefix or the resolution mechanism. Test lives here because it + // queries exp tables. See related test AbstractAuditDomainKind$TestCase.flagDuplicateNamespacePrefixes(). + TableInfo dd = getTinfoDomainDescriptor(); + Filter filter = new SimpleFilter(FieldKey.fromParts("StorageSchemaName"), "audit"); + PropertyService svc = PropertyService.get(); + Set> retrievedKinds = new HashSet<>(); + // This domain kind changed names at some point, so old deployments could have two rows that map to it -- tolerate + Set ignore = Set.of("StudySecurityEscalationEvent"); + List failures = new TableSelector(dd, new CsvSet("Name, DomainURI"), filter, new Sort("Name")).mapStream() + .map(map -> { + String name = (String)map.get("Name"); + String uri = (String)map.get("DomainURI"); + DomainKind kind = svc.getDomainKind(uri); + String ret = null; + if (null == kind) + LOG.info("{} ({}) did not resolve", uri, name); + else if (!ignore.contains(kind.getKindName()) && !retrievedKinds.add(kind)) + ret = name + ": " + uri + " ==> " + kind; + return ret; + }) + .filter(Objects::nonNull) + .toList(); + + if (!failures.isEmpty()) + Assert.fail(StringUtilsLabKey.pluralize(failures.size(), "duplicate domain kind") + "! " + failures); + } + } + public record edge(long to, int from) {} public static class InnerResult { diff --git a/query/src/org/labkey/query/audit/QueryExportAuditProvider.java b/query/src/org/labkey/query/audit/QueryExportAuditProvider.java index f878af01139..3af2648722a 100644 --- a/query/src/org/labkey/query/audit/QueryExportAuditProvider.java +++ b/query/src/org/labkey/query/audit/QueryExportAuditProvider.java @@ -41,9 +41,6 @@ import java.util.Set; /** - * User: klum - * Date: 7/21/13 - * * UNDONE: Fancy QueryAuditViewFactory.QueryDetailsColumn */ public class QueryExportAuditProvider extends AbstractAuditTypeProvider implements AuditTypeProvider @@ -219,7 +216,7 @@ public Map getAuditLogMessageElements() public static class QueryExportAuditDomainKind extends AbstractAuditDomainKind { - public static final String NAME = "QueryAuditDomain"; + public static final String NAME = "QueryExportAuditDomain"; public static String NAMESPACE_PREFIX = "Audit-" + NAME; private final Set _fields; From 91cafca2cbf4ef7468df80f28c28f0e3f8453afb Mon Sep 17 00:00:00 2001 From: Adam Rauch Date: Thu, 28 Aug 2025 07:17:26 -0700 Subject: [PATCH 04/16] Stricter checks on namespace prefixes --- .../api/audit/AbstractAuditTypeProvider.java | 27 ++++++++----------- .../audit/query/AbstractAuditDomainKind.java | 21 ++++++++++++++- .../api/exp/api/StorageProvisioner.java | 2 +- api/src/org/labkey/api/wiki/WikiService.java | 9 ++----- .../api/property/StorageProvisionerImpl.java | 5 +++- .../query/reports/ReportAuditProvider.java | 12 ++++----- .../audit/ParticipantGroupAuditProvider.java | 18 ++++++------- wiki/src/org/labkey/wiki/WikiManager.java | 10 +++---- 8 files changed, 55 insertions(+), 49 deletions(-) diff --git a/api/src/org/labkey/api/audit/AbstractAuditTypeProvider.java b/api/src/org/labkey/api/audit/AbstractAuditTypeProvider.java index 7b7c97d2836..116d3fc621b 100644 --- a/api/src/org/labkey/api/audit/AbstractAuditTypeProvider.java +++ b/api/src/org/labkey/api/audit/AbstractAuditTypeProvider.java @@ -40,7 +40,6 @@ import org.labkey.api.exp.api.ExperimentService; import org.labkey.api.exp.api.StorageProvisioner; import org.labkey.api.exp.property.Domain; -import org.labkey.api.exp.property.DomainKind; import org.labkey.api.exp.property.DomainProperty; import org.labkey.api.exp.property.PropertyService; import org.labkey.api.gwt.client.DefaultValueType; @@ -84,9 +83,9 @@ public abstract class AbstractAuditTypeProvider implements AuditTypeProvider public static final String COLUMN_NAME_TRANSACTION_ID = "TransactionID"; public static final String COLUMN_NAME_DATA_CHANGES = "DataChanges"; - final AbstractAuditDomainKind _domainKind; - + private final AbstractAuditDomainKind _domainKind; + @Deprecated // Call the other constructor and stop overriding getDomainKind() public AbstractAuditTypeProvider() { this(null); @@ -94,11 +93,12 @@ public AbstractAuditTypeProvider() public AbstractAuditTypeProvider(@NotNull AbstractAuditDomainKind domainKind) { - // TODO : consolidate domain kind initialization to either this constructor or to override - // getDomainKind. + // TODO: consolidate domain kind initialization to this constructor and stop overriding getDomainKind() _domainKind = domainKind; // Register the DomainKind PropertyService.get().registerDomainKind(getDomainKind()); + + getDomainKind().validate(); } protected AbstractAuditDomainKind getDomainKind() @@ -139,15 +139,6 @@ public void initializeProvider(User user) ensureProperties(user, domain); } - // TODO: eliminate this method... call StorageProvisioner directly - private void updateIndices(Domain domain, AbstractAuditDomainKind domainKind) - { - assert domain.getStorageTableName() != null; - assert domainKind != null; - assert domain.getDomainKind() != null; - StorageProvisioner.get().ensureTableIndices(domain, domainKind); - } - // NOTE: Changing the name of an existing PropertyDescriptor will lose data! private void ensureProperties(User user, Domain domain) { @@ -211,7 +202,11 @@ private void ensureProperties(User user, Domain domain) domain.save(user); } - updateIndices(domain, domainKind); + assert domain.getStorageTableName() != null; + assert domain.getDomainKind() != null; + assert domain.getDomainKind().getClass().equals(domainKind.getClass()); + + StorageProvisioner.get().ensureTableIndices(domain); transaction.commit(); } catch (ChangePropertyDescriptorException e) @@ -258,7 +253,7 @@ public final Domain getDomain() @Override public final Domain getDomain(boolean forUpdate) { - DomainKind domainKind = getDomainKind(); + AbstractAuditDomainKind domainKind = getDomainKind(); String domainURI = domainKind.generateDomainURI(QUERY_SCHEMA_NAME, getEventName(), getDomainContainer(), null); diff --git a/api/src/org/labkey/api/audit/query/AbstractAuditDomainKind.java b/api/src/org/labkey/api/audit/query/AbstractAuditDomainKind.java index 3cbcc5cc038..cd036037a30 100644 --- a/api/src/org/labkey/api/audit/query/AbstractAuditDomainKind.java +++ b/api/src/org/labkey/api/audit/query/AbstractAuditDomainKind.java @@ -430,13 +430,32 @@ public boolean isUserCreatedType() return false; } + private static final Set PREFIXES = new HashSet<>(); + + // Called once per AbstractAuditDomainKind at audit provider creation time + public void validate() + { + String prefix = getNamespacePrefix(); + if (prefix.length() < 12) + throw new IllegalStateException("Namespace prefix must be unique and longer than this: " + prefix); + + List overlappers = PREFIXES.stream() + .filter(p -> p.startsWith(prefix) || prefix.startsWith(p)) + .map(p -> "\"" + p + "\"") + .toList(); + + if (overlappers.isEmpty()) + PREFIXES.add(prefix); + else + throw new IllegalStateException("Namespace prefix \"" + prefix + "\" overlaps with " + overlappers); + } + public static class TestCase extends Assert { // Also see AuditDomainUriTest which tests the DomainURIs in the database @Test public void flagDuplicateNamespacePrefixes() { - // For now, simply log domain kinds that share the same prefix and those that are overlapping MultiValuedMap mmap = PropertyService.get().getDomainKinds().stream() .filter(AbstractAuditDomainKind.class::isInstance) .map(AbstractAuditDomainKind.class::cast) diff --git a/api/src/org/labkey/api/exp/api/StorageProvisioner.java b/api/src/org/labkey/api/exp/api/StorageProvisioner.java index 56aab145733..68ec8bd1549 100644 --- a/api/src/org/labkey/api/exp/api/StorageProvisioner.java +++ b/api/src/org/labkey/api/exp/api/StorageProvisioner.java @@ -88,7 +88,7 @@ static TableInfo createTableInfo(@NotNull Domain domain) void addMissingRequiredIndices(Domain domain); // Passing in DomainKind because domains can't be trusted to know the correct DomainKind. We might fix this... - void ensureTableIndices(@NotNull Domain domain, @NotNull DomainKind kind); + void ensureTableIndices(@NotNull Domain domain); SchemaTableInfo getSchemaTableInfo(Domain domain); diff --git a/api/src/org/labkey/api/wiki/WikiService.java b/api/src/org/labkey/api/wiki/WikiService.java index 7b04f3519d6..dfae120d75b 100644 --- a/api/src/org/labkey/api/wiki/WikiService.java +++ b/api/src/org/labkey/api/wiki/WikiService.java @@ -30,11 +30,6 @@ import java.sql.SQLException; import java.util.List; -/** - * User: Mark Igra - * Date: Jun 12, 2006 - * Time: 2:48:54 PM - */ public interface WikiService { static @Nullable WikiService get() @@ -47,8 +42,8 @@ static void setInstance(WikiService impl) ServiceRegistry.get().registerService(WikiService.class, impl); } - WebPartView getView(Container c, String name, boolean renderContentOnly); - WebPartView getHistoryView(Container c, String name); + WebPartView getView(Container c, String name, boolean renderContentOnly); + WebPartView getHistoryView(Container c, String name); HtmlString getHtml(Container c, String name); diff --git a/experiment/src/org/labkey/experiment/api/property/StorageProvisionerImpl.java b/experiment/src/org/labkey/experiment/api/property/StorageProvisionerImpl.java index e9be153f14a..4a29b16a859 100644 --- a/experiment/src/org/labkey/experiment/api/property/StorageProvisionerImpl.java +++ b/experiment/src/org/labkey/experiment/api/property/StorageProvisionerImpl.java @@ -1059,12 +1059,15 @@ private void dropTableIndices(Domain domain, Set indexNames) } @Override - public void ensureTableIndices(@NotNull Domain domain, @NotNull DomainKind kind) + public void ensureTableIndices(@NotNull Domain domain) { // Issue 50059, acquiring the schema table info this way ensures that the domain fields are properly fixed up. See ProvisionedSchemaOptions. SchemaTableInfo schemaTableInfo = StorageProvisioner.get().getSchemaTableInfo(domain); if (schemaTableInfo != null) { + DomainKind kind = domain.getDomainKind(); + if (null == kind) + throw new IllegalStateException("Domain kind is null!"); Map>> existingIndices = schemaTableInfo.getAllIndices(); Set newIndices = new HashSet<>(kind.getPropertyIndices(domain)); Set toRemove = new HashSet<>(); diff --git a/query/src/org/labkey/query/reports/ReportAuditProvider.java b/query/src/org/labkey/query/reports/ReportAuditProvider.java index 315c13f817d..762d6d4f5c5 100644 --- a/query/src/org/labkey/query/reports/ReportAuditProvider.java +++ b/query/src/org/labkey/query/reports/ReportAuditProvider.java @@ -25,11 +25,10 @@ public class ReportAuditProvider extends AbstractAuditTypeProvider private static final String COLUMN_NAME_REPORT_NAME = "ReportName"; private static final String COLUMN_NAME_REPORT_KEY = "ReportKey"; private static final String COLUMN_NAME_REPORT_TYPE = "ReportType"; + private static final List defaultVisibleColumns = new ArrayList<>(); - - static final List defaultVisibleColumns = new ArrayList<>(); - - static { + static + { defaultVisibleColumns.add(FieldKey.fromParts(COLUMN_NAME_CREATED)); defaultVisibleColumns.add(FieldKey.fromParts(COLUMN_NAME_CONTAINER)); defaultVisibleColumns.add(FieldKey.fromParts(COLUMN_NAME_CREATED_BY)); @@ -41,10 +40,9 @@ public class ReportAuditProvider extends AbstractAuditTypeProvider defaultVisibleColumns.add(FieldKey.fromParts(COLUMN_NAME_COMMENT)); } - @Override - protected AbstractAuditDomainKind getDomainKind() + public ReportAuditProvider() { - return new ReportAuditDomainKind(); + super(new ReportAuditDomainKind()); } @Override diff --git a/study/src/org/labkey/study/audit/ParticipantGroupAuditProvider.java b/study/src/org/labkey/study/audit/ParticipantGroupAuditProvider.java index 6d5f631c06a..953beb49d20 100644 --- a/study/src/org/labkey/study/audit/ParticipantGroupAuditProvider.java +++ b/study/src/org/labkey/study/audit/ParticipantGroupAuditProvider.java @@ -38,11 +38,10 @@ public class ParticipantGroupAuditProvider extends AbstractAuditTypeProvider imp private static final String EVENT_NAME = "ParticipantGroupEvent"; private static final String PARTICIPANT_CATEGORY_ID_COLUMN_NAME = "ParticipantCategory"; private static final String PARTICIPANT_GROUP_ID_COLUMN_NAME = "ParticipantGroup"; + private static final List defaultVisibleColumns = new ArrayList<>(); - - static final List defaultVisibleColumns = new ArrayList<>(); - - static { + static + { defaultVisibleColumns.add(FieldKey.fromParts(COLUMN_NAME_CREATED)); defaultVisibleColumns.add(FieldKey.fromParts(COLUMN_NAME_CONTAINER)); defaultVisibleColumns.add(FieldKey.fromParts(COLUMN_NAME_CREATED_BY)); @@ -53,6 +52,11 @@ public class ParticipantGroupAuditProvider extends AbstractAuditTypeProvider imp defaultVisibleColumns.add(FieldKey.fromParts(COLUMN_NAME_DATA_CHANGES)); } + public ParticipantGroupAuditProvider() + { + super(new ParticipantGroupAuditDomainKind()); + } + @Override public String getEventName() { @@ -112,12 +116,6 @@ else if (PARTICIPANT_GROUP_ID_COLUMN_NAME.equalsIgnoreCase(col.getName())) return table; } - @Override - protected AbstractAuditDomainKind getDomainKind() - { - return new ParticipantGroupAuditDomainKind(); - } - public static class ParticipantGroupAuditEvent extends DetailedAuditTypeEvent { private Integer _participantCategory; diff --git a/wiki/src/org/labkey/wiki/WikiManager.java b/wiki/src/org/labkey/wiki/WikiManager.java index c216d00f8fe..8a20fc586cc 100644 --- a/wiki/src/org/labkey/wiki/WikiManager.java +++ b/wiki/src/org/labkey/wiki/WikiManager.java @@ -900,7 +900,7 @@ public void insertWiki(User user, Container c, String name, String body, WikiRen } @Override - public WebPartView getView(Container c, String name, boolean contentOnly) + public WebPartView getView(Container c, String name, boolean contentOnly) { try { @@ -913,8 +913,7 @@ public WebPartView getView(Container c, String name, boolean contentOnly) if (null == wiki) return null; WikiVersion version = wiki.getLatestVersion(); - WikiView view = new WikiView(wiki, version, true); - return view; + return new WikiView(wiki, version, true); } catch (Exception x) { @@ -923,14 +922,13 @@ public WebPartView getView(Container c, String name, boolean contentOnly) } @Override - public WebPartView getHistoryView(Container c, String name) + public WikiVersionsGrid getHistoryView(Container c, String name) { Wiki wiki = WikiSelectManager.getWiki(c, name); if (null == wiki) return null; WikiVersion version = wiki.getLatestVersion(); - WikiVersionsGrid view = new WikiVersionsGrid(wiki, version, null); - return view; + return new WikiVersionsGrid(wiki, version, null); } @Override From ec606c54ac8aca6d42680d2c6fd43cb783d1d33c Mon Sep 17 00:00:00 2001 From: Adam Rauch Date: Thu, 28 Aug 2025 08:44:59 -0700 Subject: [PATCH 05/16] Ensure singleton --- .../labkey/api/exp/property/DomainAuditProvider.java | 10 ++++++++-- .../api/exp/property/DomainPropertyAuditProvider.java | 2 +- .../src/org/labkey/experiment/ExperimentModule.java | 2 +- 3 files changed, 10 insertions(+), 4 deletions(-) diff --git a/api/src/org/labkey/api/exp/property/DomainAuditProvider.java b/api/src/org/labkey/api/exp/property/DomainAuditProvider.java index 4f4634368b8..5d33d6579b7 100644 --- a/api/src/org/labkey/api/exp/property/DomainAuditProvider.java +++ b/api/src/org/labkey/api/exp/property/DomainAuditProvider.java @@ -50,11 +50,11 @@ public class DomainAuditProvider extends AbstractAuditTypeProvider implements AuditTypeProvider { public static final String EVENT_TYPE = "DomainAuditEvent"; - public static final String COLUMN_NAME_DOMAIN_URI = "DomainUri"; public static final String COLUMN_NAME_DOMAIN_NAME = "DomainName"; - static final List defaultVisibleColumns = new ArrayList<>(); + private static final List defaultVisibleColumns = new ArrayList<>(); + private static final DomainAuditProvider INSTANCE = new DomainAuditProvider(); static { @@ -67,6 +67,12 @@ public class DomainAuditProvider extends AbstractAuditTypeProvider implements Au defaultVisibleColumns.add(FieldKey.fromParts("UserComment")); } + // This provider is referenced in a lookup. Ensure it's a singleton so validate() is invoked only once on its domain kind. + public static DomainAuditProvider getInstance() + { + return INSTANCE; + } + public DomainAuditProvider() { super(new DomainAuditDomainKind()); diff --git a/api/src/org/labkey/api/exp/property/DomainPropertyAuditProvider.java b/api/src/org/labkey/api/exp/property/DomainPropertyAuditProvider.java index 0b48e114797..d254e68abed 100644 --- a/api/src/org/labkey/api/exp/property/DomainPropertyAuditProvider.java +++ b/api/src/org/labkey/api/exp/property/DomainPropertyAuditProvider.java @@ -124,7 +124,7 @@ protected void initColumn(MutableColumnInfo col) @Override public TableInfo getLookupTableInfo() { - DomainAuditProvider provider = new DomainAuditProvider(); + DomainAuditProvider provider = DomainAuditProvider.getInstance(); TableInfo table = provider.createTableInfo(getUserSchema(), ContainerFilter.getUnsafeEverythingFilter()); return table; } diff --git a/experiment/src/org/labkey/experiment/ExperimentModule.java b/experiment/src/org/labkey/experiment/ExperimentModule.java index 3694ca50fb7..dfe035c806a 100644 --- a/experiment/src/org/labkey/experiment/ExperimentModule.java +++ b/experiment/src/org/labkey/experiment/ExperimentModule.java @@ -520,7 +520,7 @@ protected void startupAfterSpringConfig(ModuleContext moduleContext) ExperimentService.get().registerExperimentRunTypeSource(container -> Collections.singleton(ExperimentRunType.ALL_RUNS_TYPE)); ExperimentService.get().registerDataType(new LogDataType()); - AuditLogService.get().registerAuditType(new DomainAuditProvider()); + AuditLogService.get().registerAuditType(DomainAuditProvider.getInstance()); AuditLogService.get().registerAuditType(new DomainPropertyAuditProvider()); AuditLogService.get().registerAuditType(new ExperimentAuditProvider()); AuditLogService.get().registerAuditType(new SampleTypeAuditProvider()); From 746197d60f570a1a945fdbeadbd90611f5644963 Mon Sep 17 00:00:00 2001 From: Adam Rauch Date: Thu, 28 Aug 2025 10:36:40 -0700 Subject: [PATCH 06/16] Switch to validating domain kinds at initialization() time instead of construction, which provides a bit more flexibility (e.g., allows construction of audit providers used in lookups) --- .../org/labkey/api/audit/AbstractAuditTypeProvider.java | 6 +++--- .../labkey/api/audit/query/AbstractAuditDomainKind.java | 2 +- .../org/labkey/api/exp/property/DomainAuditProvider.java | 7 ------- .../api/exp/property/DomainPropertyAuditProvider.java | 5 ++--- experiment/src/org/labkey/experiment/ExperimentModule.java | 2 +- 5 files changed, 7 insertions(+), 15 deletions(-) diff --git a/api/src/org/labkey/api/audit/AbstractAuditTypeProvider.java b/api/src/org/labkey/api/audit/AbstractAuditTypeProvider.java index 116d3fc621b..cf93ca71482 100644 --- a/api/src/org/labkey/api/audit/AbstractAuditTypeProvider.java +++ b/api/src/org/labkey/api/audit/AbstractAuditTypeProvider.java @@ -97,8 +97,6 @@ public AbstractAuditTypeProvider(@NotNull AbstractAuditDomainKind domainKind) _domainKind = domainKind; // Register the DomainKind PropertyService.get().registerDomainKind(getDomainKind()); - - getDomainKind().validate(); } protected AbstractAuditDomainKind getDomainKind() @@ -110,9 +108,11 @@ protected AbstractAuditDomainKind getDomainKind() } @Override - public void initializeProvider(User user) + public final void initializeProvider(User user) { AbstractAuditDomainKind domainKind = getDomainKind(); + domainKind.validate(); + Domain domain = getDomain(true); // if the domain doesn't exist, create it diff --git a/api/src/org/labkey/api/audit/query/AbstractAuditDomainKind.java b/api/src/org/labkey/api/audit/query/AbstractAuditDomainKind.java index cd036037a30..d4ed29da10e 100644 --- a/api/src/org/labkey/api/audit/query/AbstractAuditDomainKind.java +++ b/api/src/org/labkey/api/audit/query/AbstractAuditDomainKind.java @@ -432,7 +432,7 @@ public boolean isUserCreatedType() private static final Set PREFIXES = new HashSet<>(); - // Called once per AbstractAuditDomainKind at audit provider creation time + // Called at audit provider initialization time to forbid domain kinds with shared and overlapping namespace prefixes public void validate() { String prefix = getNamespacePrefix(); diff --git a/api/src/org/labkey/api/exp/property/DomainAuditProvider.java b/api/src/org/labkey/api/exp/property/DomainAuditProvider.java index 5d33d6579b7..d5ade35f243 100644 --- a/api/src/org/labkey/api/exp/property/DomainAuditProvider.java +++ b/api/src/org/labkey/api/exp/property/DomainAuditProvider.java @@ -54,7 +54,6 @@ public class DomainAuditProvider extends AbstractAuditTypeProvider implements Au public static final String COLUMN_NAME_DOMAIN_NAME = "DomainName"; private static final List defaultVisibleColumns = new ArrayList<>(); - private static final DomainAuditProvider INSTANCE = new DomainAuditProvider(); static { @@ -67,12 +66,6 @@ public class DomainAuditProvider extends AbstractAuditTypeProvider implements Au defaultVisibleColumns.add(FieldKey.fromParts("UserComment")); } - // This provider is referenced in a lookup. Ensure it's a singleton so validate() is invoked only once on its domain kind. - public static DomainAuditProvider getInstance() - { - return INSTANCE; - } - public DomainAuditProvider() { super(new DomainAuditDomainKind()); diff --git a/api/src/org/labkey/api/exp/property/DomainPropertyAuditProvider.java b/api/src/org/labkey/api/exp/property/DomainPropertyAuditProvider.java index d254e68abed..e4d3f2bd777 100644 --- a/api/src/org/labkey/api/exp/property/DomainPropertyAuditProvider.java +++ b/api/src/org/labkey/api/exp/property/DomainPropertyAuditProvider.java @@ -124,9 +124,8 @@ protected void initColumn(MutableColumnInfo col) @Override public TableInfo getLookupTableInfo() { - DomainAuditProvider provider = DomainAuditProvider.getInstance(); - TableInfo table = provider.createTableInfo(getUserSchema(), ContainerFilter.getUnsafeEverythingFilter()); - return table; + DomainAuditProvider provider = new DomainAuditProvider(); + return provider.createTableInfo(getUserSchema(), ContainerFilter.getUnsafeEverythingFilter()); } }); } diff --git a/experiment/src/org/labkey/experiment/ExperimentModule.java b/experiment/src/org/labkey/experiment/ExperimentModule.java index dfe035c806a..3694ca50fb7 100644 --- a/experiment/src/org/labkey/experiment/ExperimentModule.java +++ b/experiment/src/org/labkey/experiment/ExperimentModule.java @@ -520,7 +520,7 @@ protected void startupAfterSpringConfig(ModuleContext moduleContext) ExperimentService.get().registerExperimentRunTypeSource(container -> Collections.singleton(ExperimentRunType.ALL_RUNS_TYPE)); ExperimentService.get().registerDataType(new LogDataType()); - AuditLogService.get().registerAuditType(DomainAuditProvider.getInstance()); + AuditLogService.get().registerAuditType(new DomainAuditProvider()); AuditLogService.get().registerAuditType(new DomainPropertyAuditProvider()); AuditLogService.get().registerAuditType(new ExperimentAuditProvider()); AuditLogService.get().registerAuditType(new SampleTypeAuditProvider()); From 081b5357df837ad7b2e218c8b78f2e8585846726 Mon Sep 17 00:00:00 2001 From: Adam Rauch Date: Thu, 28 Aug 2025 20:39:43 -0700 Subject: [PATCH 07/16] Enumerate the audit providers to find the domain kinds to test --- .../api/audit/AbstractAuditTypeProvider.java | 6 ++++++ .../api/audit/query/AbstractAuditDomainKind.java | 14 ++++++-------- 2 files changed, 12 insertions(+), 8 deletions(-) diff --git a/api/src/org/labkey/api/audit/AbstractAuditTypeProvider.java b/api/src/org/labkey/api/audit/AbstractAuditTypeProvider.java index cf93ca71482..e5fc84d182f 100644 --- a/api/src/org/labkey/api/audit/AbstractAuditTypeProvider.java +++ b/api/src/org/labkey/api/audit/AbstractAuditTypeProvider.java @@ -107,6 +107,12 @@ protected AbstractAuditDomainKind getDomainKind() return _domainKind; } + // Expose the domain kind to AbstractAuditDomainKind$TestCase without touching every subclass + public AbstractAuditDomainKind getAuditDomainKind() + { + return getDomainKind(); + } + @Override public final void initializeProvider(User user) { diff --git a/api/src/org/labkey/api/audit/query/AbstractAuditDomainKind.java b/api/src/org/labkey/api/audit/query/AbstractAuditDomainKind.java index d4ed29da10e..8e1207a99db 100644 --- a/api/src/org/labkey/api/audit/query/AbstractAuditDomainKind.java +++ b/api/src/org/labkey/api/audit/query/AbstractAuditDomainKind.java @@ -15,13 +15,13 @@ */ package org.labkey.api.audit.query; -import org.apache.commons.collections4.MultiValuedMap; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; import org.json.JSONObject; import org.junit.Assert; import org.junit.Test; import org.labkey.api.audit.AbstractAuditTypeProvider; +import org.labkey.api.audit.AuditLogService; import org.labkey.api.collections.CaseInsensitiveHashSet; import org.labkey.api.collections.LabKeyCollectors; import org.labkey.api.data.Container; @@ -42,7 +42,6 @@ import org.labkey.api.exp.property.Domain; import org.labkey.api.exp.property.DomainKind; import org.labkey.api.exp.property.DomainProperty; -import org.labkey.api.exp.property.PropertyService; import org.labkey.api.exp.xar.LsidUtils; import org.labkey.api.gwt.client.model.GWTDomain; import org.labkey.api.gwt.client.model.GWTPropertyDescriptor; @@ -456,12 +455,11 @@ public static class TestCase extends Assert @Test public void flagDuplicateNamespacePrefixes() { - MultiValuedMap mmap = PropertyService.get().getDomainKinds().stream() - .filter(AbstractAuditDomainKind.class::isInstance) - .map(AbstractAuditDomainKind.class::cast) - .collect(LabKeyCollectors.toMultiValuedMap(AbstractAuditDomainKind::getNamespacePrefix, dk -> dk.getClass().getSimpleName())); - - Map> map = mmap.asMap(); + Map> map = AuditLogService.get().getAuditProviders().stream() + .filter(AbstractAuditTypeProvider.class::isInstance) + .map(p -> ((AbstractAuditTypeProvider)p).getAuditDomainKind()) + .collect(LabKeyCollectors.toMultiValuedMap(AbstractAuditDomainKind::getNamespacePrefix, dk -> dk.getClass().getSimpleName())) + .asMap(); List failures = map.entrySet().stream() .filter(e -> e.getValue().size() > 1) From dd31fe82964456428bf8a54bbeda0efdcb92cb76 Mon Sep 17 00:00:00 2001 From: Adam Rauch Date: Fri, 29 Aug 2025 10:17:43 -0700 Subject: [PATCH 08/16] Remove obsolete comment --- api/src/org/labkey/api/exp/api/StorageProvisioner.java | 1 - 1 file changed, 1 deletion(-) diff --git a/api/src/org/labkey/api/exp/api/StorageProvisioner.java b/api/src/org/labkey/api/exp/api/StorageProvisioner.java index 68ec8bd1549..df8c6a044fa 100644 --- a/api/src/org/labkey/api/exp/api/StorageProvisioner.java +++ b/api/src/org/labkey/api/exp/api/StorageProvisioner.java @@ -87,7 +87,6 @@ static TableInfo createTableInfo(@NotNull Domain domain) @Deprecated // Call ensureTableIndices() instead void addMissingRequiredIndices(Domain domain); - // Passing in DomainKind because domains can't be trusted to know the correct DomainKind. We might fix this... void ensureTableIndices(@NotNull Domain domain); SchemaTableInfo getSchemaTableInfo(Domain domain); From 0d9a966aa48d67d2d1826c5e98fe3c160922dc55 Mon Sep 17 00:00:00 2001 From: Adam Rauch Date: Sat, 30 Aug 2025 09:14:12 -0700 Subject: [PATCH 09/16] Compare old ensure indices code path with new --- .../labkey/api/exp/property/DomainUtil.java | 21 ++++++++- .../api/property/StorageProvisionerImpl.java | 45 +++++++++++-------- 2 files changed, 45 insertions(+), 21 deletions(-) diff --git a/api/src/org/labkey/api/exp/property/DomainUtil.java b/api/src/org/labkey/api/exp/property/DomainUtil.java index 6b83d805e70..966d4fedc2b 100644 --- a/api/src/org/labkey/api/exp/property/DomainUtil.java +++ b/api/src/org/labkey/api/exp/property/DomainUtil.java @@ -774,7 +774,7 @@ public static ValidationException updateDomainDescriptor(GWTDomain>> getExistingIndices(Domain domain) + { + SchemaTableInfo schemaTableInfo = StorageProvisioner.get().getSchemaTableInfo(domain); + if (schemaTableInfo != null) + { + DomainKind dk = domain.getDomainKind(); + if (null == dk) + throw new IllegalStateException("Domain kind is null!"); + return schemaTableInfo.getAllIndices(); + } + return Map.of(); + } + // Issue 51321: check reserved domain name: First, All public static @Nullable String validateReservedName(@NotNull String domainName, @NotNull String kindName) { diff --git a/experiment/src/org/labkey/experiment/api/property/StorageProvisionerImpl.java b/experiment/src/org/labkey/experiment/api/property/StorageProvisionerImpl.java index 4a29b16a859..74902c03a7b 100644 --- a/experiment/src/org/labkey/experiment/api/property/StorageProvisionerImpl.java +++ b/experiment/src/org/labkey/experiment/api/property/StorageProvisionerImpl.java @@ -45,6 +45,7 @@ import org.labkey.api.data.MVDisplayColumnFactory; import org.labkey.api.data.ParameterMapStatement; import org.labkey.api.data.PropertyStorageSpec; +import org.labkey.api.data.PropertyStorageSpec.Index; import org.labkey.api.data.RuntimeSQLException; import org.labkey.api.data.SQLFragment; import org.labkey.api.data.SchemaTableInfo; @@ -98,6 +99,7 @@ import java.sql.SQLException; import java.util.ArrayList; import java.util.Collection; +import java.util.Comparator; import java.util.HashMap; import java.util.HashSet; import java.util.LinkedHashSet; @@ -203,7 +205,7 @@ private String _create(DbScope scope, DomainKind kind, Domain domain, boolean } } - List indices = new ArrayList<>(); + List indices = new ArrayList<>(); indices.addAll(kind.getPropertyIndices(domain)); indices.addAll(domain.getPropertyIndices()); change.setIndexedColumns(domain, indices); @@ -858,7 +860,7 @@ enum RequiredIndicesAction Drop { @Override - public void doOperation(StorageProvisionerImpl provisioner, Domain domain, SchemaTableInfo schemaTableInfo, Map requiredIndicesMap) + public void doOperation(StorageProvisionerImpl provisioner, Domain domain, SchemaTableInfo schemaTableInfo, Map requiredIndicesMap) { provisioner.dropNotRequiredIndices(domain, schemaTableInfo, requiredIndicesMap); } @@ -866,13 +868,13 @@ public void doOperation(StorageProvisionerImpl provisioner, Domain domain, Schem Add { @Override - public void doOperation(StorageProvisionerImpl provisioner, Domain domain, SchemaTableInfo schemaTableInfo, Map requiredIndicesMap) + public void doOperation(StorageProvisionerImpl provisioner, Domain domain, SchemaTableInfo schemaTableInfo, Map requiredIndicesMap) { provisioner.addMissingRequiredIndices(domain, schemaTableInfo, requiredIndicesMap); } }; - public abstract void doOperation(StorageProvisionerImpl provisioner, Domain domain, SchemaTableInfo schemaTableInfo, Map requiredIndicesMap); + public abstract void doOperation(StorageProvisionerImpl provisioner, Domain domain, SchemaTableInfo schemaTableInfo, Map requiredIndicesMap); } @Override @@ -896,19 +898,19 @@ private void updateTableIndices(Domain domain, @NotNull RequiredIndicesAction re SqlDialect sqlDialect = getSqlDialect(domain); - Map requiredIndicesMap = getRequiredIndices(domain, sqlDialect); + Map requiredIndicesMap = getRequiredIndices(domain, sqlDialect); requiredIndicesAction.doOperation(this, domain, schemaTableInfo, requiredIndicesMap); } @NotNull - private Map getRequiredIndices(Domain domain, SqlDialect sqlDialect) + private Map getRequiredIndices(Domain domain, SqlDialect sqlDialect) { - Collection requiredIndices = new ArrayList<>(); + Collection requiredIndices = new ArrayList<>(); requiredIndices.addAll(domain.getDomainKind().getPropertyIndices(domain)); requiredIndices.addAll(domain.getPropertyIndices()); - Map requiredIndicesMap = new CaseInsensitiveMapWrapper<>(new HashMap<>()); + Map requiredIndicesMap = new CaseInsensitiveMapWrapper<>(new HashMap<>()); String storageTableName = domain.getStorageTableName(); @@ -917,7 +919,7 @@ private Map getRequiredIndices(Domain domain, storageTableName = makeTableName(getDomainKind(domain),domain); } - for (PropertyStorageSpec.Index index : requiredIndices) + for (Index index : requiredIndices) { // TODO: Bad!! Shouldn't be making up an index name here! Ideally, we use the AbstractAuditTypeProvider.updateIndices() approach instead. requiredIndicesMap.put(sqlDialect.nameIndex(storageTableName, index.columnNames), index); @@ -925,7 +927,7 @@ private Map getRequiredIndices(Domain domain, return requiredIndicesMap; } - private void dropNotRequiredIndices(Domain domain, SchemaTableInfo schemaTableInfo, Map requiredIndicesMap) + private void dropNotRequiredIndices(Domain domain, SchemaTableInfo schemaTableInfo, Map requiredIndicesMap) { Set indicesToDrop = new HashSet<>(); @@ -955,13 +957,13 @@ private void dropNotRequiredIndices(Domain domain, SchemaTableInfo schemaTableIn } } - private void addMissingRequiredIndices(Domain domain, SchemaTableInfo schemaTableInfo, Map requiredIndicesMap) + private void addMissingRequiredIndices(Domain domain, SchemaTableInfo schemaTableInfo, Map requiredIndicesMap) { - Set indicesToAdd = new HashSet<>(); + Set indicesToAdd = new HashSet<>(); // Build the list of indices to add CaseInsensitiveHashSet tableIndexNames = new CaseInsensitiveHashSet(schemaTableInfo.getAllIndices().keySet()); - for (Map.Entry requiredIndexEntry : requiredIndicesMap.entrySet()) + for (Map.Entry requiredIndexEntry : requiredIndicesMap.entrySet()) { boolean requiredIndexNotFoundInTable = !tableIndexNames.contains(requiredIndexEntry.getKey()); if (requiredIndexNotFoundInTable) @@ -993,7 +995,7 @@ private void addMissingRequiredIndices(Domain domain, SchemaTableInfo schemaTabl } } - private static void ensureIndexToBeAddedHasNoPrimaryKeys(SchemaTableInfo schemaTableInfo, Map.Entry requiredIndexEntry) + private static void ensureIndexToBeAddedHasNoPrimaryKeys(SchemaTableInfo schemaTableInfo, Map.Entry requiredIndexEntry) { for (String indexColumnName : requiredIndexEntry.getValue().columnNames) { @@ -1008,7 +1010,7 @@ private static void ensureIndexToBeAddedHasNoPrimaryKeys(SchemaTableInfo schemaT } } - private void addTableIndices(Domain domain, Set indices, TableChange.IndexSizeMode sizeMode) + private void addTableIndices(Domain domain, Set indices, TableChange.IndexSizeMode sizeMode) { DbScope scope = validateDomain(domain); @@ -1069,7 +1071,12 @@ public void ensureTableIndices(@NotNull Domain domain) if (null == kind) throw new IllegalStateException("Domain kind is null!"); Map>> existingIndices = schemaTableInfo.getAllIndices(); - Set newIndices = new HashSet<>(kind.getPropertyIndices(domain)); + // Determine the desired indexes. Note that the Domain and the DomainKind index lists may overlap, so we need + // to uniquify the list. Domain never specifies "clustered" but DomainKind does (e.g., DatasetDomainKind), + // so we compare using only column names and give preference to the DomainKind. + Set newIndices = new TreeSet<>(Comparator.comparing(index -> String.join("_", index.columnNames))); + newIndices.addAll(domain.getPropertyIndices()); + newIndices.addAll(kind.getPropertyIndices(domain)); Set toRemove = new HashSet<>(); for (String name : existingIndices.keySet()) { @@ -1081,11 +1088,11 @@ public void ensureTableIndices(@NotNull Domain domain) { columnNames[i] = columnIndex.second.get(i).getColumnName(); } - PropertyStorageSpec.Index existingIndex = new PropertyStorageSpec.Index(columnIndex.first == TableInfo.IndexType.Unique, columnNames); + Index existingIndex = new Index(columnIndex.first == TableInfo.IndexType.Unique, columnNames); boolean foundIt = false; - for (PropertyStorageSpec.Index propertyIndex : newIndices) + for (Index propertyIndex : newIndices) { - if (PropertyStorageSpec.Index.isSameIndex(propertyIndex, existingIndex)) + if (Index.isSameIndex(propertyIndex, existingIndex)) { foundIt = true; newIndices.remove(propertyIndex); From b9777ea18ede06038500ce4ba713b1be0475129e Mon Sep 17 00:00:00 2001 From: Adam Rauch Date: Sun, 31 Aug 2025 09:27:28 -0700 Subject: [PATCH 10/16] Use ensureTableIndices() in dataset import --- .../labkey/api/exp/property/DomainUtil.java | 6 ++-- .../org/labkey/study/model/StudyManager.java | 35 ++++++++++++++++--- 2 files changed, 35 insertions(+), 6 deletions(-) diff --git a/api/src/org/labkey/api/exp/property/DomainUtil.java b/api/src/org/labkey/api/exp/property/DomainUtil.java index 966d4fedc2b..520dac06dd2 100644 --- a/api/src/org/labkey/api/exp/property/DomainUtil.java +++ b/api/src/org/labkey/api/exp/property/DomainUtil.java @@ -977,10 +977,12 @@ public static ValidationException updateDomainDescriptor(GWTDomain orderedIds = reader.getDatasetOrder(); @@ -3764,6 +3773,24 @@ private void buildPropertySaveAndDeleteLists(Map } } + private void ensureRequiredIndices(SchemaReader reader, Map datasetDefEntryMap, Map domainChangeMap) + { + for (SchemaReader.DatasetImportInfo datasetImportInfo : reader.getDatasetInfo().values()) + { + DatasetDefinitionEntry datasetDefinitionEntry = datasetDefEntryMap.get(datasetImportInfo.name); + if (datasetDefinitionEntry.datasetDefinition.isShared()) + { + continue; + } + Domain domain = domainChangeMap.get(datasetDefinitionEntry.datasetDefinition.getTypeURI()).domain; + domain.setPropertyIndices(datasetImportInfo.indices); + var indices1 = DomainUtil.getExistingIndices(domain); + StorageProvisioner.get().ensureTableIndices(domain); + var indices2 = DomainUtil.getExistingIndices(domain); + assert indices1.equals(indices2); + } + } + private void addMissingRequiredIndices(SchemaReader reader, Map datasetDefEntryMap, Map domainChangeMap) { for (SchemaReader.DatasetImportInfo datasetImportInfo : reader.getDatasetInfo().values()) From b1f0202b615d8c1474d1d5c5280e2c36f640274f Mon Sep 17 00:00:00 2001 From: Adam Rauch Date: Sun, 31 Aug 2025 13:29:12 -0700 Subject: [PATCH 11/16] Test ensureTableIndices() in dataset domain kind --- .../api/property/StorageProvisionerImpl.java | 10 +++++----- .../src/org/labkey/study/model/DatasetDomainKind.java | 6 ++++++ 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/experiment/src/org/labkey/experiment/api/property/StorageProvisionerImpl.java b/experiment/src/org/labkey/experiment/api/property/StorageProvisionerImpl.java index 74902c03a7b..6f914ebe503 100644 --- a/experiment/src/org/labkey/experiment/api/property/StorageProvisionerImpl.java +++ b/experiment/src/org/labkey/experiment/api/property/StorageProvisionerImpl.java @@ -133,7 +133,7 @@ private StorageProvisionerImpl() } // #42641: Track recently created tables in a cache to limit size and duration - private static final Cache RECENTLY_CREATED_TABLES = CacheBuilder.newBuilder() + private static final Cache<@NotNull String, StackTraceElement @NotNull []> RECENTLY_CREATED_TABLES = CacheBuilder.newBuilder() .maximumSize(10000) .expireAfterWrite(1, TimeUnit.DAYS) .build(); @@ -1071,10 +1071,10 @@ public void ensureTableIndices(@NotNull Domain domain) if (null == kind) throw new IllegalStateException("Domain kind is null!"); Map>> existingIndices = schemaTableInfo.getAllIndices(); - // Determine the desired indexes. Note that the Domain and the DomainKind index lists may overlap, so we need - // to uniquify the list. Domain never specifies "clustered" but DomainKind does (e.g., DatasetDomainKind), - // so we compare using only column names and give preference to the DomainKind. - Set newIndices = new TreeSet<>(Comparator.comparing(index -> String.join("_", index.columnNames))); + // Determine the desired indexes. Note that the index lists provided by Domain and DomainKind may overlap, + // so we need to uniquify. Domain never specifies "clustered" but DomainKind does (e.g., DatasetDomainKind), + // so compare using only column names and give preference to DomainKind. + Set newIndices = new TreeSet<>(Comparator.comparing(index -> String.join("_", index.columnNames), String.CASE_INSENSITIVE_ORDER)); newIndices.addAll(domain.getPropertyIndices()); newIndices.addAll(kind.getPropertyIndices(domain)); Set toRemove = new HashSet<>(); diff --git a/study/src/org/labkey/study/model/DatasetDomainKind.java b/study/src/org/labkey/study/model/DatasetDomainKind.java index c67c068b278..b6906786887 100644 --- a/study/src/org/labkey/study/model/DatasetDomainKind.java +++ b/study/src/org/labkey/study/model/DatasetDomainKind.java @@ -522,6 +522,12 @@ else if (!timepointType.isVisitBased() && getKindName().equals(VisitDatasetDomai newDomain.setPropertyIndices(indices, lowerReservedNames); StorageProvisioner.get().addMissingRequiredIndices(newDomain); + // Calling ensureTableIndices(newDomain) should result in no changes. TODO: Remove addMissingRequiredIndices() call above. + var indices1 = DomainUtil.getExistingIndices(newDomain); + StorageProvisioner.get().ensureTableIndices(newDomain); + var indices2 = DomainUtil.getExistingIndices(newDomain); + assert indices1.equals(indices2); + QueryService.get().saveCalculatedFieldsMetadata("study", name, null, domain.getCalculatedFields(), false, user, container); } else From 3a5ac348bfe26c9d324327c951b686a028f19580 Mon Sep 17 00:00:00 2001 From: Adam Rauch Date: Mon, 1 Sep 2025 08:59:25 -0700 Subject: [PATCH 12/16] Prefer ensureTableIndices() over add/drop() --- .../labkey/api/exp/property/DomainUtil.java | 2 +- .../labkey/study/model/DatasetDomainKind.java | 6 ++--- .../org/labkey/study/model/StudyManager.java | 26 ++++++++++--------- 3 files changed, 18 insertions(+), 16 deletions(-) diff --git a/api/src/org/labkey/api/exp/property/DomainUtil.java b/api/src/org/labkey/api/exp/property/DomainUtil.java index 520dac06dd2..a71c883df74 100644 --- a/api/src/org/labkey/api/exp/property/DomainUtil.java +++ b/api/src/org/labkey/api/exp/property/DomainUtil.java @@ -979,7 +979,7 @@ public static ValidationException updateDomainDescriptor(GWTDomain indices = domain.getIndices(); newDomain.setPropertyIndices(indices, lowerReservedNames); - StorageProvisioner.get().addMissingRequiredIndices(newDomain); + StorageProvisioner.get().ensureTableIndices(newDomain); - // Calling ensureTableIndices(newDomain) should result in no changes. TODO: Remove addMissingRequiredIndices() call above. + // Calling addMissingRequiredIndices(newDomain) should result in no changes. TODO: Remove the below check. var indices1 = DomainUtil.getExistingIndices(newDomain); - StorageProvisioner.get().ensureTableIndices(newDomain); + StorageProvisioner.get().addMissingRequiredIndices(newDomain); var indices2 = DomainUtil.getExistingIndices(newDomain); assert indices1.equals(indices2); diff --git a/study/src/org/labkey/study/model/StudyManager.java b/study/src/org/labkey/study/model/StudyManager.java index 76cf181b895..a319216c914 100644 --- a/study/src/org/labkey/study/model/StudyManager.java +++ b/study/src/org/labkey/study/model/StudyManager.java @@ -3599,20 +3599,15 @@ else if (d.isModified) buildPropertySaveAndDeleteLists(datasetDefEntryMap, list, domainChangeMap, false); // now that we actually have datasets, create/update the domains - try - { - dropNotRequiredIndices(reader, datasetDefEntryMap, domainChangeMap); + ensureRequiredIndices(reader, datasetDefEntryMap, domainChangeMap); - if (!deleteAndSaveProperties(user, errors, domainChangeMap)) - return false; + // Test that the old drop/add methods result in no-ops. TODO: Remove + dropNotRequiredIndices(reader, datasetDefEntryMap, domainChangeMap); - addMissingRequiredIndices(reader, datasetDefEntryMap, domainChangeMap); - } - finally - { - // New method should succeed with no changes to indices. TODO: remove drop/add above - ensureRequiredIndices(reader, datasetDefEntryMap, domainChangeMap); - } + if (!deleteAndSaveProperties(user, errors, domainChangeMap)) + return false; + + addMissingRequiredIndices(reader, datasetDefEntryMap, domainChangeMap); } List orderedIds = reader.getDatasetOrder(); @@ -3791,6 +3786,7 @@ private void ensureRequiredIndices(SchemaReader reader, Map datasetDefEntryMap, Map domainChangeMap) { for (SchemaReader.DatasetImportInfo datasetImportInfo : reader.getDatasetInfo().values()) @@ -3802,10 +3798,16 @@ private void addMissingRequiredIndices(SchemaReader reader, Map datasetDefEntryMap, Map domainChangeMap) { for (SchemaReader.DatasetImportInfo datasetImportInfo : reader.getDatasetInfo().values()) From d0dcb337f9b915b29429c753250570ba359a89c9 Mon Sep 17 00:00:00 2001 From: Adam Rauch Date: Mon, 1 Sep 2025 11:00:43 -0700 Subject: [PATCH 13/16] Skip index creation if domain hasn't been provisioned yet --- .../org/labkey/api/exp/api/StorageProvisioner.java | 2 ++ .../api/property/StorageProvisionerImpl.java | 7 +++++-- study/src/org/labkey/study/model/StudyManager.java | 14 +++++++------- 3 files changed, 14 insertions(+), 9 deletions(-) diff --git a/api/src/org/labkey/api/exp/api/StorageProvisioner.java b/api/src/org/labkey/api/exp/api/StorageProvisioner.java index df8c6a044fa..cf05d7d167a 100644 --- a/api/src/org/labkey/api/exp/api/StorageProvisioner.java +++ b/api/src/org/labkey/api/exp/api/StorageProvisioner.java @@ -35,6 +35,7 @@ import java.util.HashSet; import java.util.List; import java.util.Set; +import java.util.function.Supplier; /** * Creates and maintains "hard" tables in the underlying database based on dynamically configured data types. @@ -88,6 +89,7 @@ static TableInfo createTableInfo(@NotNull Domain domain) void addMissingRequiredIndices(Domain domain); void ensureTableIndices(@NotNull Domain domain); +// void ensureTableIndices(@NotNull Domain domain, Supplier afterAddSupplier); SchemaTableInfo getSchemaTableInfo(Domain domain); diff --git a/experiment/src/org/labkey/experiment/api/property/StorageProvisionerImpl.java b/experiment/src/org/labkey/experiment/api/property/StorageProvisionerImpl.java index 6f914ebe503..2cad07f49a3 100644 --- a/experiment/src/org/labkey/experiment/api/property/StorageProvisionerImpl.java +++ b/experiment/src/org/labkey/experiment/api/property/StorageProvisionerImpl.java @@ -535,7 +535,7 @@ public void resizeProperty(Domain domain, DomainProperty prop, Integer oldScale) DomainKind kind = domain.getDomainKind(); DbScope scope = kind.getScope(); - // should be in a transaction with propertydescriptor changes + // should be in a transaction with property descriptor changes if (!scope.isTransactionActive()) throw new ChangePropertyDescriptorException("Unable to change property size. Transaction is not active within change scope"); @@ -676,7 +676,7 @@ public SQLFragment getValueSql(String tableAlias) @Override public String toString() { - // really shouldn't be doing this any more, use getSelectName()? + // really shouldn't be doing this anymore, use getSelectName()? return _inner.toString(); } @@ -1063,6 +1063,9 @@ private void dropTableIndices(Domain domain, Set indexNames) @Override public void ensureTableIndices(@NotNull Domain domain) { + if (!domain.isProvisioned()) + return; + // Issue 50059, acquiring the schema table info this way ensures that the domain fields are properly fixed up. See ProvisionedSchemaOptions. SchemaTableInfo schemaTableInfo = StorageProvisioner.get().getSchemaTableInfo(domain); if (schemaTableInfo != null) diff --git a/study/src/org/labkey/study/model/StudyManager.java b/study/src/org/labkey/study/model/StudyManager.java index a319216c914..9e9cefb943f 100644 --- a/study/src/org/labkey/study/model/StudyManager.java +++ b/study/src/org/labkey/study/model/StudyManager.java @@ -178,6 +178,7 @@ import org.labkey.api.util.Pair; import org.labkey.api.util.Path; import org.labkey.api.util.StringUtilsLabKey; +import org.labkey.api.util.logging.LogHelper; import org.labkey.api.view.ActionURL; import org.labkey.api.view.NavTree; import org.labkey.api.view.UnauthorizedException; @@ -222,6 +223,7 @@ import java.util.TreeMap; import java.util.WeakHashMap; import java.util.function.Consumer; +import java.util.function.Supplier; import java.util.stream.Collectors; import static org.labkey.api.action.SpringActionController.ERROR_MSG; @@ -237,7 +239,7 @@ public class StudyManager public static final SearchService.SearchCategory datasetCategory = new SearchService.SearchCategory("dataset", "Study Datasets"); public static final SearchService.SearchCategory subjectCategory = new SearchService.SearchCategory("subject", "Study Subjects"); - private static final Logger _log = LogManager.getLogger(StudyManager.class); + private static final Logger _log = LogHelper.getLogger(StudyManager.class, "Dataset operations"); private static final StudyManager _instance = new StudyManager(); private static final StudySchema SCHEMA = StudySchema.getInstance(); private static final String LSID_REQUIRED = "LSID_REQUIRED"; @@ -3598,8 +3600,9 @@ else if (d.isModified) domainChangeMap.clear(); buildPropertySaveAndDeleteLists(datasetDefEntryMap, list, domainChangeMap, false); - // now that we actually have datasets, create/update the domains - ensureRequiredIndices(reader, datasetDefEntryMap, domainChangeMap); + // now that we actually have datasets, create/update the domains. Supplier ensures that all domains and + // property changes are saved before adding indices. + ensureRequiredIndices(reader, datasetDefEntryMap, domainChangeMap, () -> deleteAndSaveProperties(user, errors, domainChangeMap)); // Test that the old drop/add methods result in no-ops. TODO: Remove dropNotRequiredIndices(reader, datasetDefEntryMap, domainChangeMap); @@ -3768,7 +3771,7 @@ private void buildPropertySaveAndDeleteLists(Map } } - private void ensureRequiredIndices(SchemaReader reader, Map datasetDefEntryMap, Map domainChangeMap) + private void ensureRequiredIndices(SchemaReader reader, Map datasetDefEntryMap, Map domainChangeMap, Supplier afterAddSupplier) { for (SchemaReader.DatasetImportInfo datasetImportInfo : reader.getDatasetInfo().values()) { @@ -3779,10 +3782,7 @@ private void ensureRequiredIndices(SchemaReader reader, Map Date: Mon, 1 Sep 2025 11:50:06 -0700 Subject: [PATCH 14/16] Handle provisioned vs. not-yet-provisioned dataset domains correctly --- .../api/exp/api/StorageProvisioner.java | 2 +- .../api/property/StorageProvisionerImpl.java | 12 +++- .../org/labkey/study/model/StudyManager.java | 68 +++++++++++-------- 3 files changed, 53 insertions(+), 29 deletions(-) diff --git a/api/src/org/labkey/api/exp/api/StorageProvisioner.java b/api/src/org/labkey/api/exp/api/StorageProvisioner.java index cf05d7d167a..a8c6c707a3f 100644 --- a/api/src/org/labkey/api/exp/api/StorageProvisioner.java +++ b/api/src/org/labkey/api/exp/api/StorageProvisioner.java @@ -89,7 +89,7 @@ static TableInfo createTableInfo(@NotNull Domain domain) void addMissingRequiredIndices(Domain domain); void ensureTableIndices(@NotNull Domain domain); -// void ensureTableIndices(@NotNull Domain domain, Supplier afterAddSupplier); + void ensureTableIndices(@NotNull Domain domain, Supplier afterAddSupplier); SchemaTableInfo getSchemaTableInfo(Domain domain); diff --git a/experiment/src/org/labkey/experiment/api/property/StorageProvisionerImpl.java b/experiment/src/org/labkey/experiment/api/property/StorageProvisionerImpl.java index 2cad07f49a3..822e4bd4a42 100644 --- a/experiment/src/org/labkey/experiment/api/property/StorageProvisionerImpl.java +++ b/experiment/src/org/labkey/experiment/api/property/StorageProvisionerImpl.java @@ -1063,6 +1063,13 @@ private void dropTableIndices(Domain domain, Set indexNames) @Override public void ensureTableIndices(@NotNull Domain domain) { + ensureTableIndices(domain, () -> true); + } + + @Override + public void ensureTableIndices(@NotNull Domain domain, Supplier afterAddSupplier) + { + // TODO: throw instead -- shouldn't be called on an unprovisioned domain if (!domain.isProvisioned()) return; @@ -1109,7 +1116,10 @@ public void ensureTableIndices(@NotNull Domain domain) if (!toRemove.isEmpty()) dropTableIndices(domain, toRemove); - if (!newIndices.isEmpty()) + + boolean successfulAdd = afterAddSupplier.get(); + + if (successfulAdd && !newIndices.isEmpty()) addTableIndices(domain, newIndices, TableChange.IndexSizeMode.Normal); } } diff --git a/study/src/org/labkey/study/model/StudyManager.java b/study/src/org/labkey/study/model/StudyManager.java index 9e9cefb943f..d28c62b2f4b 100644 --- a/study/src/org/labkey/study/model/StudyManager.java +++ b/study/src/org/labkey/study/model/StudyManager.java @@ -223,7 +223,6 @@ import java.util.TreeMap; import java.util.WeakHashMap; import java.util.function.Consumer; -import java.util.function.Supplier; import java.util.stream.Collectors; import static org.labkey.api.action.SpringActionController.ERROR_MSG; @@ -3596,20 +3595,19 @@ else if (d.isModified) // optional param to control whether field additions or deletions are permitted if (allowDomainUpdates) { - // generate dataset domain changes for new datasets + // Generate dataset domain changes for new datasets domainChangeMap.clear(); buildPropertySaveAndDeleteLists(datasetDefEntryMap, list, domainChangeMap, false); - // now that we actually have datasets, create/update the domains. Supplier ensures that all domains and + // Now that we actually have datasets, create or update the domains. This ensures that all domains and // property changes are saved before adding indices. - ensureRequiredIndices(reader, datasetDefEntryMap, domainChangeMap, () -> deleteAndSaveProperties(user, errors, domainChangeMap)); + ensurePropertiesAndRequiredIndices(reader, datasetDefEntryMap, domainChangeMap, user, errors); - // Test that the old drop/add methods result in no-ops. TODO: Remove - dropNotRequiredIndices(reader, datasetDefEntryMap, domainChangeMap); - - if (!deleteAndSaveProperties(user, errors, domainChangeMap)) + if (errors.hasErrors()) return false; + // Test that calling the old drop/add methods result in no-ops. TODO: Remove + dropNotRequiredIndices(reader, datasetDefEntryMap, domainChangeMap); addMissingRequiredIndices(reader, datasetDefEntryMap, domainChangeMap); } @@ -3623,25 +3621,23 @@ else if (d.isModified) return true; } - private boolean deleteAndSaveProperties(User user, BindException errors, Map domainChangeMap) + // see if we need to delete any columns from an existing domain and create the domain if it doesn't already exist + private boolean deleteAndSaveProperties(User user, BindException errors, _DatasetDomainChange domainChange) { - // see if we need to delete any columns from an existing domain - for (_DatasetDomainChange domainChange : domainChangeMap.values()) + // see if we need to delete any columns from the domain + for (DomainProperty p : domainChange.propsToDelete) { - for (DomainProperty p : domainChange.propsToDelete) - { - p.delete(); - } + p.delete(); + } - try - { - domainChange.domain.save(user); - } - catch (ChangePropertyDescriptorException ex) - { - errors.reject("importDatasetSchemas", ex.getMessage() == null ? ex.toString() : ex.getMessage()); - return false; - } + try + { + domainChange.domain.save(user); + } + catch (ChangePropertyDescriptorException ex) + { + errors.reject("importDatasetSchemas", ex.getMessage() == null ? ex.toString() : ex.getMessage()); + return false; } return true; } @@ -3771,7 +3767,7 @@ private void buildPropertySaveAndDeleteLists(Map } } - private void ensureRequiredIndices(SchemaReader reader, Map datasetDefEntryMap, Map domainChangeMap, Supplier afterAddSupplier) + private void ensurePropertiesAndRequiredIndices(SchemaReader reader, Map datasetDefEntryMap, Map domainChangeMap, User user, BindException errors) { for (SchemaReader.DatasetImportInfo datasetImportInfo : reader.getDatasetInfo().values()) { @@ -3780,9 +3776,22 @@ private void ensureRequiredIndices(SchemaReader reader, Map deleteAndSaveProperties(user, errors, domainChange)); + } + else + { + // deleteAndSaveProperties() calls domain.save() which ensures all the indices after provisioning the new domain + deleteAndSaveProperties(user, errors, domainChange); + } } } @@ -3819,7 +3828,12 @@ private void dropNotRequiredIndices(SchemaReader reader, Map Date: Mon, 1 Sep 2025 17:49:10 -0700 Subject: [PATCH 15/16] Remove old code --- .../labkey/api/data/PropertyStorageSpec.java | 7 +- api/src/org/labkey/api/data/TableChange.java | 7 +- .../data/dialect/BasePostgreSqlDialect.java | 17 +- .../api/exp/api/StorageProvisioner.java | 5 - .../labkey/api/exp/property/DomainUtil.java | 7 - .../api/property/StorageProvisionerImpl.java | 153 +----------------- .../labkey/study/model/DatasetDomainKind.java | 6 - .../org/labkey/study/model/StudyManager.java | 47 ------ 8 files changed, 21 insertions(+), 228 deletions(-) diff --git a/api/src/org/labkey/api/data/PropertyStorageSpec.java b/api/src/org/labkey/api/data/PropertyStorageSpec.java index 66a40909c0e..9003a17fae0 100644 --- a/api/src/org/labkey/api/data/PropertyStorageSpec.java +++ b/api/src/org/labkey/api/data/PropertyStorageSpec.java @@ -31,14 +31,9 @@ import java.util.Set; /** - * User: newton - * Date: Aug 24, 2010 - * Time: 4:08:39 PM - * * The reason that we have this class, instead of doing something like reusing PropertyDescriptor, is that we also need * a storage spec like this when there is not a full property descriptor such as for the base properties of DomainKinds. * ColumnInfo and SqlColumn are also mismatched for various reasons. - * */ public class PropertyStorageSpec { @@ -470,7 +465,7 @@ public Index(boolean unique, boolean clustered, String... columnNames) /** * Determines if two indices are the same modulo the isClustered setting. This is useful for updating indices - * when an audit domain type changes, for example. + * when a domain changes, for example. */ public static boolean isSameIndex(Index propertyIndex, Index tableIndex) { diff --git a/api/src/org/labkey/api/data/TableChange.java b/api/src/org/labkey/api/data/TableChange.java index f5924b02fe7..02e6c3fef92 100644 --- a/api/src/org/labkey/api/data/TableChange.java +++ b/api/src/org/labkey/api/data/TableChange.java @@ -234,7 +234,7 @@ public void addColumn(PropertyDescriptor prop) addColumn(new PropertyStorageSpec(prop)); } - /** Add the property to the set of columns tracked in this change along with it's old scale. */ + /** Add the property to the set of columns tracked in this change along with its old scale. */ public void addColumnResize(PropertyDescriptor prop, Integer oldScale) { addColumn(prop); @@ -248,7 +248,7 @@ public void addColumnRename(String oldName, String newName) /** * Index will be renamed using the columns listed in the Index. - * The columns used by the index won't be changed. We need to + * The columns used by the index won't be changed. We need to * pass the list of columns since the index name is created by the dialect. * * @param oldIndex Old index to be renamed. @@ -309,7 +309,8 @@ public void setIndexedColumns(Domain domain, Collection indices) _indices = indices.stream().map(i -> i.translateToStorageNames(domain)).toList(); } - public Set getIndicesToBeDroppedByName(){ + public Set getIndicesToBeDroppedByName() + { return _indicesToBeDroppedByName; } diff --git a/api/src/org/labkey/api/data/dialect/BasePostgreSqlDialect.java b/api/src/org/labkey/api/data/dialect/BasePostgreSqlDialect.java index 245fd01920b..41fbf509c43 100644 --- a/api/src/org/labkey/api/data/dialect/BasePostgreSqlDialect.java +++ b/api/src/org/labkey/api/data/dialect/BasePostgreSqlDialect.java @@ -1070,15 +1070,10 @@ private void addDropIndexByNameStatements(List statements, TableChange c { for (String indexName : change.getIndicesToBeDroppedByName()) { - statements.add(getDropIndexCommand(indexName, change)); + statements.add(getDropIndexCommand(change, indexName)); } } - private String getDropIndexCommand(String indexName, TableChange change) - { - return getDropIndexCommand(change,indexName); - } - private String getDropIndexCommand(TableChange change, String indexName) { return "DROP INDEX " + change.getSchemaName() + "." + indexName; @@ -1140,7 +1135,6 @@ else if (column.getJdbcType().isDecimal()) else { dbType = getSqlTypeName(column.getJdbcType()); - } //Postgres retains the existing null behavior @@ -1171,6 +1165,10 @@ private List getRenameColumnsStatement(TableChange change) } } + // TODO: This loop should not guess the name of the old indices; instead, it should look them up. + // TableChange.setIndexedColumns() could set _indexRenames providing the name, and then this code uses that info. + // Or maybe schemaTableInfo.getAllIndices() and then use Index.isSameIndex() to find names. + // Or maybe we don't bother renaming the indices since (on PostgreSQL) we always look them up when dropping them. for (Map.Entry oldToNew : change.getIndexRenames().entrySet()) { PropertyStorageSpec.Index oldIndex = oldToNew.getKey(); @@ -1327,16 +1325,17 @@ private void addCreateIndexStatements(List statements, TableChange chang { for (PropertyStorageSpec.Index index : change.getIndexedColumns()) { + String newIndexName = nameIndex(change.getTableName(), index.columnNames); statements.add(String.format("CREATE %sINDEX %s ON %s (%s);", index.isUnique ? "UNIQUE " : "", - nameIndex(change.getTableName(), index.columnNames), + newIndexName, makeTableIdentifier(change), makePropertyIdentifiers(index.columnNames))); if (index.isClustered) { statements.add(String.format("%s %s.%s USING %s", PropertyStorageSpec.CLUSTER_TYPE.CLUSTER, change.getSchemaName(), - change.getTableName(), nameIndex(change.getTableName(), index.columnNames))); + change.getTableName(), newIndexName)); } } } diff --git a/api/src/org/labkey/api/exp/api/StorageProvisioner.java b/api/src/org/labkey/api/exp/api/StorageProvisioner.java index a8c6c707a3f..b4a23ef71e5 100644 --- a/api/src/org/labkey/api/exp/api/StorageProvisioner.java +++ b/api/src/org/labkey/api/exp/api/StorageProvisioner.java @@ -83,11 +83,6 @@ static TableInfo createTableInfo(@NotNull Domain domain) */ void createStorageTable(Domain domain, DomainKind kind, DbScope scope); - @Deprecated // Call ensureTableIndices() instead - void dropNotRequiredIndices(Domain domain); - @Deprecated // Call ensureTableIndices() instead - void addMissingRequiredIndices(Domain domain); - void ensureTableIndices(@NotNull Domain domain); void ensureTableIndices(@NotNull Domain domain, Supplier afterAddSupplier); diff --git a/api/src/org/labkey/api/exp/property/DomainUtil.java b/api/src/org/labkey/api/exp/property/DomainUtil.java index a71c883df74..97656ef9b27 100644 --- a/api/src/org/labkey/api/exp/property/DomainUtil.java +++ b/api/src/org/labkey/api/exp/property/DomainUtil.java @@ -978,13 +978,6 @@ public static ValidationException updateDomainDescriptor(GWTDomain kind, DbScope scope) _create(scope, kind, domain, true); } - enum RequiredIndicesAction - { - Drop - { - @Override - public void doOperation(StorageProvisionerImpl provisioner, Domain domain, SchemaTableInfo schemaTableInfo, Map requiredIndicesMap) - { - provisioner.dropNotRequiredIndices(domain, schemaTableInfo, requiredIndicesMap); - } - }, - Add - { - @Override - public void doOperation(StorageProvisionerImpl provisioner, Domain domain, SchemaTableInfo schemaTableInfo, Map requiredIndicesMap) - { - provisioner.addMissingRequiredIndices(domain, schemaTableInfo, requiredIndicesMap); - } - }; - - public abstract void doOperation(StorageProvisionerImpl provisioner, Domain domain, SchemaTableInfo schemaTableInfo, Map requiredIndicesMap); - } - - @Override - public void dropNotRequiredIndices(Domain domain) - { - if (!domain.isProvisioned()){ - return; - } - updateTableIndices(domain, RequiredIndicesAction.Drop); - } - - @Override - public void addMissingRequiredIndices(Domain domain) - { - updateTableIndices(domain, RequiredIndicesAction.Add); - } - - private void updateTableIndices(Domain domain, @NotNull RequiredIndicesAction requiredIndicesAction) - { - SchemaTableInfo schemaTableInfo = getSchemaTableInfo(domain); - - SqlDialect sqlDialect = getSqlDialect(domain); - - Map requiredIndicesMap = getRequiredIndices(domain, sqlDialect); - - requiredIndicesAction.doOperation(this, domain, schemaTableInfo, requiredIndicesMap); - } - - @NotNull - private Map getRequiredIndices(Domain domain, SqlDialect sqlDialect) - { - Collection requiredIndices = new ArrayList<>(); - requiredIndices.addAll(domain.getDomainKind().getPropertyIndices(domain)); - requiredIndices.addAll(domain.getPropertyIndices()); - - Map requiredIndicesMap = new CaseInsensitiveMapWrapper<>(new HashMap<>()); - - String storageTableName = domain.getStorageTableName(); - - if (storageTableName == null) - { - storageTableName = makeTableName(getDomainKind(domain),domain); - } - - for (Index index : requiredIndices) - { - // TODO: Bad!! Shouldn't be making up an index name here! Ideally, we use the AbstractAuditTypeProvider.updateIndices() approach instead. - requiredIndicesMap.put(sqlDialect.nameIndex(storageTableName, index.columnNames), index); - } - return requiredIndicesMap; - } - - private void dropNotRequiredIndices(Domain domain, SchemaTableInfo schemaTableInfo, Map requiredIndicesMap) - { - Set indicesToDrop = new HashSet<>(); - - // Determine indices to drop - for (Map.Entry>> index : schemaTableInfo.getAllIndices().entrySet()) - { - boolean isPrimaryKey = index.getValue().getKey().equals(TableInfo.IndexType.Primary); - boolean tableIndexNameNotFoundInRequiredIndices = !requiredIndicesMap.containsKey(index.getKey().toLowerCase()); - - if (!isPrimaryKey && tableIndexNameNotFoundInRequiredIndices) - { - indicesToDrop.add(index.getKey()); - } - } - - if (!indicesToDrop.isEmpty()) - { - TableChange change = new TableChange(domain, ChangeType.DropIndicesByName); - - change.setIndicesToBeDroppedByName(indicesToDrop); - - try (Transaction transaction = getScope(domain).ensureTransaction()) - { - change.execute(); - transaction.commit(); - } - } - } - - private void addMissingRequiredIndices(Domain domain, SchemaTableInfo schemaTableInfo, Map requiredIndicesMap) - { - Set indicesToAdd = new HashSet<>(); - - // Build the list of indices to add - CaseInsensitiveHashSet tableIndexNames = new CaseInsensitiveHashSet(schemaTableInfo.getAllIndices().keySet()); - for (Map.Entry requiredIndexEntry : requiredIndicesMap.entrySet()) - { - boolean requiredIndexNotFoundInTable = !tableIndexNames.contains(requiredIndexEntry.getKey()); - if (requiredIndexNotFoundInTable) - { - ensureIndexToBeAddedHasNoPrimaryKeys(schemaTableInfo, requiredIndexEntry); - indicesToAdd.add(requiredIndexEntry.getValue()); - } - } - - if (!indicesToAdd.isEmpty()) - { - TableChange change; - if (domain.getStorageTableName() == null) - { - change = new TableChange(domain, ChangeType.AddIndices, makeTableName(getDomainKind(domain),domain)); - } - else - { - change = new TableChange(domain, ChangeType.AddIndices); - } - - change.getIndexedColumns().addAll(indicesToAdd); - - try (Transaction transaction = getScope(domain).ensureTransaction()) - { - change.execute(); - transaction.commit(); - } - } - } - + // The old addMissingRequiredIndices() method called this to ensure no index used any column in the PK. Why so + // strict? An index that *starts with* the same column(s) as another index or PK is likely redundant, but that's + // not unique to PKs and could be valid when a column in the middle of a composite PK is indexed. TODO: Decide + // if this should be removed or incorporated into ensureTableIndices(). private static void ensureIndexToBeAddedHasNoPrimaryKeys(SchemaTableInfo schemaTableInfo, Map.Entry requiredIndexEntry) { for (String indexColumnName : requiredIndexEntry.getValue().columnNames) @@ -1069,9 +933,8 @@ public void ensureTableIndices(@NotNull Domain domain) @Override public void ensureTableIndices(@NotNull Domain domain, Supplier afterAddSupplier) { - // TODO: throw instead -- shouldn't be called on an unprovisioned domain if (!domain.isProvisioned()) - return; + throw new IllegalStateException("Domain " + domain.getName() + " is not provisioned!"); // Issue 50059, acquiring the schema table info this way ensures that the domain fields are properly fixed up. See ProvisionedSchemaOptions. SchemaTableInfo schemaTableInfo = StorageProvisioner.get().getSchemaTableInfo(domain); @@ -1079,11 +942,11 @@ public void ensureTableIndices(@NotNull Domain domain, Supplier afterAd { DomainKind kind = domain.getDomainKind(); if (null == kind) - throw new IllegalStateException("Domain kind is null!"); + throw new IllegalStateException("Domain kind of " + domain.getName() + " is null!"); Map>> existingIndices = schemaTableInfo.getAllIndices(); // Determine the desired indexes. Note that the index lists provided by Domain and DomainKind may overlap, - // so we need to uniquify. Domain never specifies "clustered" but DomainKind does (e.g., DatasetDomainKind), - // so compare using only column names and give preference to DomainKind. + // so we need to uniquify. Domain indices never specify "clustered" but DomainKind indices may (e.g., + // DatasetDomainKind), so compare using only column names and give preference to DomainKind. Set newIndices = new TreeSet<>(Comparator.comparing(index -> String.join("_", index.columnNames), String.CASE_INSENSITIVE_ORDER)); newIndices.addAll(domain.getPropertyIndices()); newIndices.addAll(kind.getPropertyIndices(domain)); diff --git a/study/src/org/labkey/study/model/DatasetDomainKind.java b/study/src/org/labkey/study/model/DatasetDomainKind.java index 2d892b8f9e4..7210771238e 100644 --- a/study/src/org/labkey/study/model/DatasetDomainKind.java +++ b/study/src/org/labkey/study/model/DatasetDomainKind.java @@ -522,12 +522,6 @@ else if (!timepointType.isVisitBased() && getKindName().equals(VisitDatasetDomai newDomain.setPropertyIndices(indices, lowerReservedNames); StorageProvisioner.get().ensureTableIndices(newDomain); - // Calling addMissingRequiredIndices(newDomain) should result in no changes. TODO: Remove the below check. - var indices1 = DomainUtil.getExistingIndices(newDomain); - StorageProvisioner.get().addMissingRequiredIndices(newDomain); - var indices2 = DomainUtil.getExistingIndices(newDomain); - assert indices1.equals(indices2); - QueryService.get().saveCalculatedFieldsMetadata("study", name, null, domain.getCalculatedFields(), false, user, container); } else diff --git a/study/src/org/labkey/study/model/StudyManager.java b/study/src/org/labkey/study/model/StudyManager.java index d28c62b2f4b..d600d6aca94 100644 --- a/study/src/org/labkey/study/model/StudyManager.java +++ b/study/src/org/labkey/study/model/StudyManager.java @@ -3605,10 +3605,6 @@ else if (d.isModified) if (errors.hasErrors()) return false; - - // Test that calling the old drop/add methods result in no-ops. TODO: Remove - dropNotRequiredIndices(reader, datasetDefEntryMap, domainChangeMap); - addMissingRequiredIndices(reader, datasetDefEntryMap, domainChangeMap); } List orderedIds = reader.getDatasetOrder(); @@ -3795,49 +3791,6 @@ private void ensurePropertiesAndRequiredIndices(SchemaReader reader, Map datasetDefEntryMap, Map domainChangeMap) - { - for (SchemaReader.DatasetImportInfo datasetImportInfo : reader.getDatasetInfo().values()) - { - DatasetDefinitionEntry datasetDefinitionEntry = datasetDefEntryMap.get(datasetImportInfo.name); - if (datasetDefinitionEntry.datasetDefinition.isShared()) - { - continue; - } - Domain domain = domainChangeMap.get(datasetDefinitionEntry.datasetDefinition.getTypeURI()).domain; - domain.setPropertyIndices(datasetImportInfo.indices); - - // Test that the below is a no-op - var indices1 = DomainUtil.getExistingIndices(domain); - StorageProvisioner.get().addMissingRequiredIndices(domain); - var indices2 = DomainUtil.getExistingIndices(domain); - assert indices1.equals(indices2); - } - } - - @Deprecated - private void dropNotRequiredIndices(SchemaReader reader, Map datasetDefEntryMap, Map domainChangeMap) - { - for (SchemaReader.DatasetImportInfo datasetImportInfo : reader.getDatasetInfo().values()) - { - DatasetDefinitionEntry datasetDefinitionEntry = datasetDefEntryMap.get(datasetImportInfo.name); - if (datasetDefinitionEntry.datasetDefinition.isShared()) - { - continue; - } - Domain domain = domainChangeMap.get(datasetDefinitionEntry.datasetDefinition.getTypeURI()).domain; - domain.setPropertyIndices(datasetImportInfo.indices); - - // Test that the below is a no-op - var indices1 = DomainUtil.getExistingIndices(domain); - StorageProvisioner.get().dropNotRequiredIndices(domain); - var indices2 = DomainUtil.getExistingIndices(domain); - assert indices1.equals(indices2); - } - } - - public String getDomainURI(Container c, User u, Dataset def) { if (null == def) From f65c7bd1a597505f670e7405729cb94d68f72d4d Mon Sep 17 00:00:00 2001 From: Adam Rauch Date: Tue, 2 Sep 2025 08:05:56 -0700 Subject: [PATCH 16/16] Code review feedback --- .../audit/query/AbstractAuditDomainKind.java | 2 ++ .../data/dialect/BasePostgreSqlDialect.java | 14 ++++++------- .../labkey/api/exp/property/DomainUtil.java | 13 ------------ .../api/property/StorageProvisionerImpl.java | 20 ------------------- 4 files changed, 9 insertions(+), 40 deletions(-) diff --git a/api/src/org/labkey/api/audit/query/AbstractAuditDomainKind.java b/api/src/org/labkey/api/audit/query/AbstractAuditDomainKind.java index 8e1207a99db..c3dfbbb0b11 100644 --- a/api/src/org/labkey/api/audit/query/AbstractAuditDomainKind.java +++ b/api/src/org/labkey/api/audit/query/AbstractAuditDomainKind.java @@ -435,6 +435,8 @@ public boolean isUserCreatedType() public void validate() { String prefix = getNamespacePrefix(); + // This check for length >= 12 is arbitrary, meant to prevent "Audit-" and other trivial namespace prefixes that + // are likely to overlap and cause domain URIs to resolve to the wrong domain kinds. if (prefix.length() < 12) throw new IllegalStateException("Namespace prefix must be unique and longer than this: " + prefix); diff --git a/api/src/org/labkey/api/data/dialect/BasePostgreSqlDialect.java b/api/src/org/labkey/api/data/dialect/BasePostgreSqlDialect.java index 41fbf509c43..7f3d6d43367 100644 --- a/api/src/org/labkey/api/data/dialect/BasePostgreSqlDialect.java +++ b/api/src/org/labkey/api/data/dialect/BasePostgreSqlDialect.java @@ -38,6 +38,7 @@ import org.labkey.api.data.JdbcType; import org.labkey.api.data.MetadataSqlSelector; import org.labkey.api.data.PropertyStorageSpec; +import org.labkey.api.data.PropertyStorageSpec.Index; import org.labkey.api.data.RuntimeSQLException; import org.labkey.api.data.SQLFragment; import org.labkey.api.data.Selector; @@ -1167,13 +1168,12 @@ private List getRenameColumnsStatement(TableChange change) // TODO: This loop should not guess the name of the old indices; instead, it should look them up. // TableChange.setIndexedColumns() could set _indexRenames providing the name, and then this code uses that info. - // Or maybe schemaTableInfo.getAllIndices() and then use Index.isSameIndex() to find names. - // Or maybe we don't bother renaming the indices since (on PostgreSQL) we always look them up when dropping them. - for (Map.Entry oldToNew : change.getIndexRenames().entrySet()) + // Or maybe schemaTableInfo.getAllIndices() and then use Index.isSameIndex() to find names. Issue 53838. + for (Map.Entry oldToNew : change.getIndexRenames().entrySet()) { - PropertyStorageSpec.Index oldIndex = oldToNew.getKey(); - PropertyStorageSpec.Index newIndex = oldToNew.getValue(); - String oldName = nameIndex(change.getTableName(), oldIndex.columnNames); + Index oldIndex = oldToNew.getKey(); + Index newIndex = oldToNew.getValue(); + String oldName = nameIndex(change.getTableName(), oldIndex.columnNames); // TODO: Look up name String newName = nameIndex(change.getTableName(), newIndex.columnNames); if (!oldName.equals(newName)) { @@ -1323,7 +1323,7 @@ private List getCreateIndexStatements(TableChange change) private void addCreateIndexStatements(List statements, TableChange change) { - for (PropertyStorageSpec.Index index : change.getIndexedColumns()) + for (Index index : change.getIndexedColumns()) { String newIndexName = nameIndex(change.getTableName(), index.columnNames); statements.add(String.format("CREATE %sINDEX %s ON %s (%s);", diff --git a/api/src/org/labkey/api/exp/property/DomainUtil.java b/api/src/org/labkey/api/exp/property/DomainUtil.java index 97656ef9b27..e096f4189c8 100644 --- a/api/src/org/labkey/api/exp/property/DomainUtil.java +++ b/api/src/org/labkey/api/exp/property/DomainUtil.java @@ -989,19 +989,6 @@ public static ValidationException updateDomainDescriptor(GWTDomain>> getExistingIndices(Domain domain) - { - SchemaTableInfo schemaTableInfo = StorageProvisioner.get().getSchemaTableInfo(domain); - if (schemaTableInfo != null) - { - DomainKind dk = domain.getDomainKind(); - if (null == dk) - throw new IllegalStateException("Domain kind is null!"); - return schemaTableInfo.getAllIndices(); - } - return Map.of(); - } - // Issue 51321: check reserved domain name: First, All public static @Nullable String validateReservedName(@NotNull String domainName, @NotNull String kindName) { diff --git a/experiment/src/org/labkey/experiment/api/property/StorageProvisionerImpl.java b/experiment/src/org/labkey/experiment/api/property/StorageProvisionerImpl.java index 8f9a56971ed..75c01943dcd 100644 --- a/experiment/src/org/labkey/experiment/api/property/StorageProvisionerImpl.java +++ b/experiment/src/org/labkey/experiment/api/property/StorageProvisionerImpl.java @@ -28,7 +28,6 @@ import org.labkey.api.action.SpringActionController; import org.labkey.api.collections.CaseInsensitiveHashMap; import org.labkey.api.collections.CaseInsensitiveHashSet; -import org.labkey.api.collections.CaseInsensitiveMapWrapper; import org.labkey.api.collections.Sets; import org.labkey.api.data.BaseColumnInfo; import org.labkey.api.data.ColumnInfo; @@ -855,25 +854,6 @@ public void createStorageTable(Domain domain, DomainKind kind, DbScope scope) _create(scope, kind, domain, true); } - // The old addMissingRequiredIndices() method called this to ensure no index used any column in the PK. Why so - // strict? An index that *starts with* the same column(s) as another index or PK is likely redundant, but that's - // not unique to PKs and could be valid when a column in the middle of a composite PK is indexed. TODO: Decide - // if this should be removed or incorporated into ensureTableIndices(). - private static void ensureIndexToBeAddedHasNoPrimaryKeys(SchemaTableInfo schemaTableInfo, Map.Entry requiredIndexEntry) - { - for (String indexColumnName : requiredIndexEntry.getValue().columnNames) - { - for (String primaryKeyName : schemaTableInfo.getPkColumnNames()) - { - if (indexColumnName.equalsIgnoreCase(primaryKeyName)) - { - throw new UnsupportedOperationException( - "Adding an index with primary key columns is not supported. Primary keys are " + String.join(",", schemaTableInfo.getPkColumnNames())); - } - } - } - } - private void addTableIndices(Domain domain, Set indices, TableChange.IndexSizeMode sizeMode) { DbScope scope = validateDomain(domain);