Issue 52913: Fix determination of existing index names when dropping

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<Class> getIntegrationTests()
     {
         return Set.of(
+            AbstractAuditDomainKind.TestCase.class,
             AbstractForeignKey.TestCase.class,
             AbstractQueryUpdateService.TestCase.class,
             ActionURL.TestCase.class,

api/src/org/labkey/api/audit/AbstractAuditTypeProvider.java

@@ -23,19 +23,15 @@
 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;
 import org.labkey.api.data.DbSchema;
 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;
@@ -44,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;
@@ -55,14 +50,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;
@@ -90,18 +83,17 @@ 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);
     }
 
     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());
@@ -115,10 +107,18 @@ protected AbstractAuditDomainKind getDomainKind()
         return _domainKind;
     }
 
+    // Expose the domain kind to AbstractAuditDomainKind$TestCase without touching every subclass
+    public AbstractAuditDomainKind getAuditDomainKind()
+    {
+        return 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
@@ -145,52 +145,6 @@ public void initializeProvider(User user)
         ensureProperties(user, domain);
     }
 
-    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<String, Pair<TableInfo.IndexType, List<ColumnInfo>>> existingIndices = schemaTableInfo.getAllIndices();
-            Set<PropertyStorageSpec.Index> newIndices = new HashSet<>(domainKind.getPropertyIndices(domain));
-            Set<String> toRemove = new HashSet<>();
-            for (String name : existingIndices.keySet())
-            {
-                if (existingIndices.get(name).first == TableInfo.IndexType.Primary)
-                    continue;
-                Pair<TableInfo.IndexType, List<ColumnInfo>> 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);
-        }
-    }
-
-
     // NOTE: Changing the name of an existing PropertyDescriptor will lose data!
     private void ensureProperties(User user, Domain domain)
     {
@@ -254,7 +208,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)
@@ -301,7 +259,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);
 

api/src/org/labkey/api/audit/query/AbstractAuditDomainKind.java

@@ -18,8 +18,12 @@
 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;
 import org.labkey.api.data.ContainerManager;
 import org.labkey.api.data.DbSchema;
@@ -45,23 +49,27 @@
 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;
 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.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;
 
-/**
- * User: klum
- * Date: 7/8/13
- */
 public abstract class AbstractAuditDomainKind extends DomainKind<JSONObject>
 {
-
     private static final String XAR_SUBSTITUTION_SCHEMA_NAME = "SchemaName";
     private static final String XAR_SUBSTITUTION_TABLE_NAME = "TableName";
 
@@ -393,7 +401,9 @@ public Set<String> getNonProvisionedTableNames()
         // omit the legacy auditlog table, this can be removed once the
         // table is dropped after migration
         Set<String> tables = new CaseInsensitiveHashSet();
-        tables.add("auditlog");
+
+        if (OptionalFeatureService.get().isFeatureEnabled(LEGACY_UNION_AUDIT_TABLE))
+            tables.add("auditlog");
 
         return tables;
     }
@@ -418,4 +428,61 @@ public boolean isUserCreatedType()
     {
         return false;
     }
+
+    private static final Set<String> PREFIXES = new HashSet<>();
+
+    // Called at audit provider initialization time to forbid domain kinds with shared and overlapping namespace prefixes
+    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);
+
+        List<String> 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()
+        {
+            Map<String, Collection<String>> 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<String> failures = map.entrySet().stream()
+                .filter(e -> e.getValue().size() > 1)
+                .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<String> 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());
+        }
+    }
 }

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
 {
@@ -68,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)));
     }
 

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)
         {

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<Index> indices)
         _indices = indices.stream().map(i -> i.translateToStorageNames(domain)).toList();
     }
 
-    public Set<String> getIndicesToBeDroppedByName(){
+    public Set<String> getIndicesToBeDroppedByName()
+    {
         return _indicesToBeDroppedByName;
     }