From c894c7149e8ebd38c31157b6ede726732b1e12fe Mon Sep 17 00:00:00 2001 From: Adam Rauch Date: Mon, 27 Apr 2026 17:38:44 -0700 Subject: [PATCH 01/12] Improve overlapping indices action --- .../org/labkey/devtools/DevtoolsModule.java | 7 ++- .../org/labkey/devtools/ToolsController.java | 50 ++++++++++++------- 2 files changed, 36 insertions(+), 21 deletions(-) diff --git a/devtools/src/org/labkey/devtools/DevtoolsModule.java b/devtools/src/org/labkey/devtools/DevtoolsModule.java index b918e6f01a7..dcedc1b7df3 100644 --- a/devtools/src/org/labkey/devtools/DevtoolsModule.java +++ b/devtools/src/org/labkey/devtools/DevtoolsModule.java @@ -61,10 +61,13 @@ protected void init() addController("testsso", TestSsoController.class); AuthenticationManager.registerProvider(new TestSsoProvider()); - OptionalFeatureService.get().addExperimentalFeatureFlag(Domain.EXPERIMENTAL_FUZZ_STORAGE_NAME, + OptionalFeatureService.get().addExperimentalFeatureFlag( + Domain.EXPERIMENTAL_FUZZ_STORAGE_NAME, "'fuzz' name of database columns used to back domain properties", "This is dev/test feature and not intended for any production usage.", - false, true); + false, + true + ); } @Override diff --git a/devtools/src/org/labkey/devtools/ToolsController.java b/devtools/src/org/labkey/devtools/ToolsController.java index b6971385135..1d32b2caa06 100644 --- a/devtools/src/org/labkey/devtools/ToolsController.java +++ b/devtools/src/org/labkey/devtools/ToolsController.java @@ -36,6 +36,7 @@ import org.labkey.api.util.Formats; import org.labkey.api.util.HtmlString; import org.labkey.api.util.HtmlStringBuilder; +import org.labkey.api.util.LinkBuilder; import org.labkey.api.util.PageFlowUtil; import org.labkey.api.util.StringUtilsLabKey; import org.labkey.api.util.URLHelper; @@ -556,7 +557,7 @@ public ModelAndView getView(Object o, BindException errors) throws IOException List actionIds = new LinkedList<>(); - // As of now, Crawler.java and the study tests are the only classes that specify crawler actions + // As of now, these are the only classes that specify crawler actions for (String path : List.of( sourcePath + "/../../clientModules/adjudication/test/src/org/labkey/test/tests/adjudication/AdjudicationAbstractBaseTest.java", sourcePath + "/../../ehrModules/ehr/test/src/org/labkey/test/tests/ehr/ComplianceTrainingTest.java", @@ -734,9 +735,9 @@ public void addNavTrail(NavTree root) public class OverlappingIndicesAction extends AbstractOverlappingIndicesAction { @Override - public ModelAndView getView(Object o, boolean reshow, BindException errors) + public ModelAndView getView(OverlappingIndicesForm form, boolean reshow, BindException errors) { - MultiValuedMap multiMap = getOverlappingIndices(); + MultiValuedMap multiMap = getOverlappingIndices(form); return new VBox( new HtmlView(DOM.createHtmlFragment( @@ -747,17 +748,18 @@ public ModelAndView getView(Object o, boolean reshow, BindException errors) DOM.TABLE( multiMap.get(type).stream() .map(overlap -> DOM.TR( - DOM.TD(at(style, "width:120px;"), overlap.schemaName()), + DOM.TD(at(style, "width:120px;"), LinkBuilder.simpleLink(overlap.schemaName(), new ActionURL(OverlappingIndicesAction.class, getContainer()).addParameter("schemaName", overlap.schemaName()))), DOM.TD(type.getMessage(overlap)), "\n" - )) + ) + ) ) ) ) )), new HtmlView(DOM.createHtmlFragment( BR(), - new ButtonBuilder("Create SQL Scripts That Drop Overlapping Indices").href(OverlappingIndicesAction.class, getContainer()).usePost()) + new ButtonBuilder("Create SQL Scripts That Drop Overlapping Indices").href(getViewContext().getActionURL()).usePost()) ) ); } @@ -770,9 +772,9 @@ public void addNavTrail(NavTree root) } @Override - public boolean handlePost(Object o, BindException errors) + public boolean handlePost(OverlappingIndicesForm form, BindException errors) { - MultiValuedMap multiMap = getOverlappingIndices(); + MultiValuedMap multiMap = getOverlappingIndices(form); try { @@ -896,26 +898,43 @@ private void closeAllContexts() } @Override - public void validateCommand(Object target, Errors errors) + public void validateCommand(OverlappingIndicesForm form, Errors errors) { } @Override - public URLHelper getSuccessURL(Object o) + public URLHelper getSuccessURL(OverlappingIndicesForm form) { return new ActionURL(BeginAction.class, getContainer()); } } - protected static abstract class AbstractOverlappingIndicesAction extends FormViewAction + public static class OverlappingIndicesForm { - protected MultiValuedMap getOverlappingIndices() + private @Nullable String _schemaName; + + public @Nullable String getSchemaName() + { + return _schemaName; + } + + @SuppressWarnings("unused") + public void setSchemaName(@Nullable String schemaName) + { + _schemaName = schemaName; + } + } + + protected static abstract class AbstractOverlappingIndicesAction extends FormViewAction + { + protected MultiValuedMap getOverlappingIndices(OverlappingIndicesForm form) { MultiValuedMap multiMap = new ArrayListValuedHashMap<>(); DbScope scope = DbScope.getLabKeyScope(); ModuleLoader.getInstance().getModules().stream() .flatMap(module -> module.getSchemaNames().stream().filter(name -> !module.getProvisionedSchemaNames().contains(name))) + .filter(schemaName -> form.getSchemaName() == null || schemaName.equals(form.getSchemaName())) .sorted(String.CASE_INSENSITIVE_ORDER) .map(name -> scope.getSchema(name, DbSchemaType.Module)) .flatMap(schema -> schema.getTableNames().stream().map(schema::getTable)) @@ -972,13 +991,6 @@ private String getKey(List cols) .map(col -> col.getName().toLowerCase()) .collect(Collectors.joining(delim)) + delim; } - - private List join(List cols) - { - return cols.stream() - .map(ColumnInfo::getName) - .toList(); - } } protected record Overlap(String schemaName, String tableName, IndexDefinition indexDef1, IndexDefinition indexDef2) {} From 382bf65ae2038ebbeaaf9f3000b176a5f8ce9117 Mon Sep 17 00:00:00 2001 From: Adam Rauch Date: Tue, 28 Apr 2026 08:40:24 -0700 Subject: [PATCH 02/12] Retrieve constraint for index --- .../org/labkey/devtools/ToolsController.java | 45 ++++++++++++++----- 1 file changed, 35 insertions(+), 10 deletions(-) diff --git a/devtools/src/org/labkey/devtools/ToolsController.java b/devtools/src/org/labkey/devtools/ToolsController.java index 1d32b2caa06..b5858ac534f 100644 --- a/devtools/src/org/labkey/devtools/ToolsController.java +++ b/devtools/src/org/labkey/devtools/ToolsController.java @@ -10,6 +10,8 @@ import org.labkey.api.action.SimpleErrorView; import org.labkey.api.action.SimpleViewAction; import org.labkey.api.action.SpringActionController; +import org.labkey.api.cache.Cache; +import org.labkey.api.cache.CacheManager; import org.labkey.api.collections.ArrayListValuedTreeMap; import org.labkey.api.collections.LabKeyCollectors; import org.labkey.api.data.BaseColumnInfo; @@ -18,7 +20,9 @@ import org.labkey.api.data.DbSchemaType; import org.labkey.api.data.DbScope; import org.labkey.api.data.FileSqlScriptProvider; +import org.labkey.api.data.SQLFragment; import org.labkey.api.data.SchemaTableInfo; +import org.labkey.api.data.SqlSelector; import org.labkey.api.data.TableInfo; import org.labkey.api.data.TableInfo.IndexDefinition; import org.labkey.api.data.TableInfo.IndexType; @@ -69,6 +73,7 @@ import java.sql.SQLException; import java.util.Arrays; import java.util.Collection; +import java.util.Collections; import java.util.Comparator; import java.util.HashMap; import java.util.HashSet; @@ -106,7 +111,7 @@ public class BeginAction extends SimpleViewAction @Override public ModelAndView getView(Object o, BindException errors) { - return new ActionListView(ToolsController.this, actionDescriptor->BeginAction.class != actionDescriptor.getActionClass()); + return new ActionListView(ToolsController.this, actionDescriptor -> BeginAction.class != actionDescriptor.getActionClass()); } @Override @@ -164,7 +169,7 @@ public void handle(Path gaPath, Stream stream) { out.println("Files listed in " + gaPath + " that don't exist:\n"); List missing = getMissingFiles(gaPath, stream); - missing.forEach(filename->out.println(filter(filename))); + missing.forEach(filename -> out.println(filter(filename))); if (!missing.isEmpty()) { out.println(); @@ -381,14 +386,14 @@ protected void renderInternal(Object model, PrintWriter out) Set copyOfJspFiles = new HashSet<>(jspFiles); jspFiles.removeAll(jspReferences); - jspFiles.forEach(path->out.println(filter(path))); + jspFiles.forEach(path -> out.println(filter(path))); out.println(); out.println("JSP references that couldn't be resolved to JSP files [plus any candidates for resolution]:"); out.println(); jspReferences.removeAll(copyOfJspFiles); - jspReferences.forEach(path-> { + jspReferences.forEach(path -> { List candidates = jspFiles.stream() .filter(s -> s.endsWith(path)) .toList(); @@ -411,7 +416,7 @@ protected void renderInternal(Object model, PrintWriter out) out.println(); out.println("The following " + (jspFiles.size() == 1 ? "JSP file is a strong candidate" : jspFiles.size() + " JSP files are strong candidates") + " for removal:"); out.println(); - jspFiles.forEach(path->out.println(filter(path))); + jspFiles.forEach(path -> out.println(filter(path))); } out.println(""); @@ -462,7 +467,8 @@ private Collection findJspReferences(Module module, PrintWriter out) String code = PageFlowUtil.getFileContentsAsString(file.toFile()); JavaScanner scanner = new JavaScanner(code); - scanner.scan(0, new Handler(){ + scanner.scan(0, new Handler() + { @Override public boolean string(int beginIndex, int endIndex) { @@ -604,7 +610,7 @@ public ModelAndView getView(Object o, BindException errors) throws IOException builder .append("The following " + (missingModuleActions.size() > 1 ? "actions' controllers" : "action's controller") + " could not be resolved to a module running in this deployment:") .unsafeAppend("

\n"); - missingModuleActions.forEach(id->builder.append(id.toString()).unsafeAppend("
\n")); + missingModuleActions.forEach(id -> builder.append(id.toString()).unsafeAppend("
\n")); builder.unsafeAppend("
\n"); builder.append("The associated module(s) might not support " + DbScope.getLabKeyScope().getDatabaseProductName() + "."); builder.unsafeAppend("

\n"); @@ -615,7 +621,7 @@ public ModelAndView getView(Object o, BindException errors) throws IOException builder .append("The following " + (missingActions.size() > 1 ? "actions were" : "action was") + " not found in the action's controller:") .unsafeAppend("

\n"); - missingActions.forEach(id->builder.append(id.toString()).unsafeAppend("
\n")); + missingActions.forEach(id -> builder.append(id.toString()).unsafeAppend("
\n")); } return new HtmlView(builder); @@ -751,8 +757,7 @@ public ModelAndView getView(OverlappingIndicesForm form, boolean reshow, BindExc DOM.TD(at(style, "width:120px;"), LinkBuilder.simpleLink(overlap.schemaName(), new ActionURL(OverlappingIndicesAction.class, getContainer()).addParameter("schemaName", overlap.schemaName()))), DOM.TD(type.getMessage(overlap)), "\n" - ) - ) + )) ) ) ) @@ -1115,6 +1120,26 @@ protected void dropIndex(Writer writer, String schemaName, String tableName, Str } } + private record IndexKey(String schemaName, String indexName) {} + + // Most, but not all, unique indexes are created by adding a unique constraint; in those cases, we need to drop + // the associated constraint. However, for explicitly created unique indexes, we need to drop the index instead. + // If this is a unique index associated with a constraint, return that constraint name. Otherwise, return null. + private static @Nullable String getConstraintForIndex(String schemaName, String indexName) + { + Cache> sharedCache = CacheManager.getSharedCache(); + var constraintMap = sharedCache.get("ConstraintForIndexMap", null, (_, _) -> Collections.unmodifiableMap( + new SqlSelector(DbScope.getLabKeyScope(), new SQLFragment(""" + SELECT NspName AS SchemaName, RelName AS IndexName, ConName AS ConstraintName FROM pg_index i + INNER JOIN pg_class cl ON cl.oid = i.indexrelid + INNER JOIN pg_namespace schema ON schema.oid = cl.relnamespace + INNER JOIN pg_constraint c ON ConNamespace = schema.oid AND ConIndId = cl.oid AND ConType = 'u' + WHERE IndIsUnique AND NOT NspName IN ('pg_toast', 'pg_catalog')""" + )).mapStream() + .collect(Collectors.toMap(map -> new IndexKey((String)map.get("SchemaName"), (String)map.get("IndexName")), map -> (String)map.get("ConstraintName"))))); + return constraintMap.get(new IndexKey(schemaName, indexName)); + } + @RequiresPermission(AdminPermission.class) public class ForeignKeysAction extends SimpleViewAction { From d6ed697f6d4a744e7a001d2347659859d74b545a Mon Sep 17 00:00:00 2001 From: Adam Rauch Date: Tue, 28 Apr 2026 14:22:38 -0700 Subject: [PATCH 03/12] Simple binding of action parameters to records --- .../org/labkey/api/action/BaseViewAction.java | 6 ++--- .../org/labkey/api/action/FormViewAction.java | 24 +++++++++++++++---- .../org/labkey/devtools/ToolsController.java | 18 ++------------ 3 files changed, 24 insertions(+), 24 deletions(-) diff --git a/api/src/org/labkey/api/action/BaseViewAction.java b/api/src/org/labkey/api/action/BaseViewAction.java index dfce3258d4f..f41e3523344 100644 --- a/api/src/org/labkey/api/action/BaseViewAction.java +++ b/api/src/org/labkey/api/action/BaseViewAction.java @@ -66,7 +66,6 @@ import org.springframework.web.bind.ServletRequestDataBinder; import org.springframework.web.bind.ServletRequestParameterPropertyValues; import org.springframework.web.multipart.MultipartFile; -import org.springframework.web.multipart.MultipartHttpServletRequest; import org.springframework.web.servlet.ModelAndView; import java.beans.PropertyDescriptor; @@ -76,7 +75,6 @@ import java.util.Collection; import java.util.Collections; import java.util.HashMap; -import java.util.LinkedHashMap; import java.util.List; import java.util.Map; import java.util.function.Predicate; @@ -432,13 +430,13 @@ static BindingErrorProcessor getBindingErrorProcessor(final BindingErrorProcesso return new BindingErrorProcessor() { @Override - public void processMissingFieldError(String missingField, BindingResult bindingResult) + public void processMissingFieldError(@NotNull String missingField, @NotNull BindingResult bindingResult) { defaultBEP.processMissingFieldError(missingField, bindingResult); } @Override - public void processPropertyAccessException(PropertyAccessException ex, BindingResult bindingResult) + public void processPropertyAccessException(@NotNull PropertyAccessException ex, @NotNull BindingResult bindingResult) { Object newValue = ex.getPropertyChangeEvent().getNewValue(); if (newValue instanceof String) diff --git a/api/src/org/labkey/api/action/FormViewAction.java b/api/src/org/labkey/api/action/FormViewAction.java index fa34304a341..c41895bd084 100644 --- a/api/src/org/labkey/api/action/FormViewAction.java +++ b/api/src/org/labkey/api/action/FormViewAction.java @@ -16,11 +16,14 @@ package org.labkey.api.action; +import org.jetbrains.annotations.NotNull; +import org.labkey.api.data.ObjectFactory; import org.labkey.api.miniprofiler.MiniProfiler; import org.labkey.api.miniprofiler.Timing; import org.labkey.api.util.ExceptionUtil; import org.labkey.api.util.URLHelper; import org.labkey.api.view.HttpView; +import org.springframework.beans.PropertyValue; import org.springframework.beans.PropertyValues; import org.springframework.validation.BindException; import org.springframework.validation.Errors; @@ -28,6 +31,8 @@ import org.springframework.web.servlet.ModelAndView; import java.util.List; +import java.util.Map; +import java.util.stream.Collectors; /** * Is this better than BaseCommandController? Probably not, but it understands TableViewForm. @@ -116,7 +121,7 @@ public ModelAndView handleRequest(FORM form, BindException errors) throws Except if (errors != null && errors.hasErrors()) { StringBuilder errorTextBuilder = new StringBuilder(); - String newLine = System.getProperty("line.separator"); + String newLine = System.lineSeparator(); List errorsList = errors.getAllErrors(); for (int i = 0; i < errorsList.size(); i++) @@ -136,7 +141,6 @@ public ModelAndView handleRequest(FORM form, BindException errors) throws Except } } - @Override protected String getCommandClassMethodName() { @@ -145,11 +149,23 @@ protected String getCommandClassMethodName() public BindException bindParameters(PropertyValues m) throws Exception { - return defaultBindParameters(getCommand(), m); + Class commandClass = getCommandClass(); + return commandClass.isRecord() ? defaultBindParametersToRecord(commandClass, m) : defaultBindParameters(getCommand(), m); + } + + // Very simple binding for Java records: no support for binding errors, arrays, lists, disallowed fields, etc. + private BindException defaultBindParametersToRecord(Class recordClass, PropertyValues m) + { + ObjectFactory factory = ObjectFactory.Registry.getFactory(recordClass); + Map map = m.stream() + .filter(pv -> pv.getValue() != null) + .collect(Collectors.toMap(PropertyValue::getName, PropertyValue::getValue)); + R record = factory.fromMap(map); + return new NullSafeBindException(record, "Form"); } @Override - public void validate(Object target, Errors errors) + public void validate(@NotNull Object target, @NotNull Errors errors) { if (target instanceof HasValidator) { diff --git a/devtools/src/org/labkey/devtools/ToolsController.java b/devtools/src/org/labkey/devtools/ToolsController.java index b5858ac534f..3d8f781d95f 100644 --- a/devtools/src/org/labkey/devtools/ToolsController.java +++ b/devtools/src/org/labkey/devtools/ToolsController.java @@ -914,21 +914,7 @@ public URLHelper getSuccessURL(OverlappingIndicesForm form) } } - public static class OverlappingIndicesForm - { - private @Nullable String _schemaName; - - public @Nullable String getSchemaName() - { - return _schemaName; - } - - @SuppressWarnings("unused") - public void setSchemaName(@Nullable String schemaName) - { - _schemaName = schemaName; - } - } + public record OverlappingIndicesForm(String schemaName) {} protected static abstract class AbstractOverlappingIndicesAction extends FormViewAction { @@ -939,7 +925,7 @@ protected MultiValuedMap getOverlappingIndices(Overlapping ModuleLoader.getInstance().getModules().stream() .flatMap(module -> module.getSchemaNames().stream().filter(name -> !module.getProvisionedSchemaNames().contains(name))) - .filter(schemaName -> form.getSchemaName() == null || schemaName.equals(form.getSchemaName())) + .filter(schemaName -> form.schemaName() == null || schemaName.equals(form.schemaName())) .sorted(String.CASE_INSENSITIVE_ORDER) .map(name -> scope.getSchema(name, DbSchemaType.Module)) .flatMap(schema -> schema.getTableNames().stream().map(schema::getTable)) From b5dfcbf9024b5f5d0df56b4d0a679e62fb4d4ce4 Mon Sep 17 00:00:00 2001 From: Adam Rauch Date: Thu, 30 Apr 2026 09:20:03 -0700 Subject: [PATCH 04/12] Much smarter detection of overlapping indices --- api/src/org/labkey/api/data/TableInfo.java | 6 + .../org/labkey/devtools/ToolsController.java | 208 ++++++++++++------ 2 files changed, 150 insertions(+), 64 deletions(-) diff --git a/api/src/org/labkey/api/data/TableInfo.java b/api/src/org/labkey/api/data/TableInfo.java index 15cde936ec4..607dd33a037 100644 --- a/api/src/org/labkey/api/data/TableInfo.java +++ b/api/src/org/labkey/api/data/TableInfo.java @@ -209,6 +209,12 @@ void addColumn(ColumnInfo column) { columns.add(column); } + + public String display() + { + String display = name + " " + columns.stream().map(ColumnInfo::getName).toList(); + return filterCondition == null ? display : display + " + " + filterCondition; + } } /** Get a list of columns that specifies a unique key, may return the same result as getPKColumns() diff --git a/devtools/src/org/labkey/devtools/ToolsController.java b/devtools/src/org/labkey/devtools/ToolsController.java index 3d8f781d95f..271e8b5f321 100644 --- a/devtools/src/org/labkey/devtools/ToolsController.java +++ b/devtools/src/org/labkey/devtools/ToolsController.java @@ -3,6 +3,7 @@ import org.apache.commons.collections4.MultiValuedMap; import org.apache.commons.collections4.multimap.ArrayListValuedHashMap; import org.apache.commons.lang3.StringUtils; +import org.apache.logging.log4j.Logger; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; import org.labkey.api.action.FormHandlerAction; @@ -44,6 +45,7 @@ import org.labkey.api.util.PageFlowUtil; import org.labkey.api.util.StringUtilsLabKey; import org.labkey.api.util.URLHelper; +import org.labkey.api.util.logging.LogHelper; import org.labkey.api.vcs.Vcs; import org.labkey.api.vcs.VcsService; import org.labkey.api.view.ActionURL; @@ -77,6 +79,7 @@ import java.util.Comparator; import java.util.HashMap; import java.util.HashSet; +import java.util.LinkedHashSet; import java.util.LinkedList; import java.util.List; import java.util.Map; @@ -88,6 +91,9 @@ import java.util.stream.Collectors; import java.util.stream.Stream; +import static org.labkey.api.data.TableInfo.IndexType.NonUnique; +import static org.labkey.api.data.TableInfo.IndexType.Primary; +import static org.labkey.api.data.TableInfo.IndexType.Unique; import static org.labkey.api.util.DOM.Attribute.style; import static org.labkey.api.util.DOM.BR; import static org.labkey.api.util.DOM.DIV; @@ -96,6 +102,7 @@ public class ToolsController extends SpringActionController { + private static final Logger LOG = LogHelper.getLogger(ToolsController.class, "Output from various tools"); private static final ActionResolver RESOLVER = new DefaultActionResolver(ToolsController.class); public static final String NAME = "tools"; @@ -915,6 +922,7 @@ public URLHelper getSuccessURL(OverlappingIndicesForm form) } public record OverlappingIndicesForm(String schemaName) {} + private record IndexChange(IndexDefinition index, ChangeType type, String description) {} protected static abstract class AbstractOverlappingIndicesAction extends FormViewAction { @@ -930,48 +938,73 @@ protected MultiValuedMap getOverlappingIndices(Overlapping .map(name -> scope.getSchema(name, DbSchemaType.Module)) .flatMap(schema -> schema.getTableNames().stream().map(schema::getTable)) .forEach(table -> { - var indices = table.getAllIndices(); - indices.forEach(indexDef1 -> indices.forEach(indexDef2 -> { - if (indexDef1 != indexDef2) - { - OverlapType type = overlap(indexDef1, indexDef2); + var indices = new LinkedHashSet<>(table.getAllIndices()); + + var changes = new LinkedList(); + + // Find the PK (if present), and drop all indexes that overlap with a smaller or equal column set + indices.stream() + .filter(ix -> ix.indexType() == Primary) + .findFirst() + .ifPresent(pk -> indices.stream() + .filter(index -> overlaps(pk, index)) + .forEach(index -> { + changes.add(new IndexChange(index, ChangeType.Drop, String.format("Dropping %s because it overlaps with %s", index.display(), pk.display()))); + }) + ); + + changes.forEach(change -> indices.remove(change.index())); + + Set convertedUniqueIndexes = new HashSet<>(); + + // For each unique index, switch it to a non-unique index if there's at least one UQ or PK that overlaps a smaller or equal column set + indices.stream() + .filter(index -> index.indexType() == Unique) + .forEach(uq -> indices.stream() + .filter(index -> index.indexType() == Primary || index.indexType() == Unique) + .filter(index -> overlaps(uq, index)) + .findFirst() + .ifPresent(index -> { + changes.add(new IndexChange(uq, ChangeType.Convert, String.format("Converting %s to an INDEX because %s overlaps it with a smaller column set", uq.display(), index.display()))); + convertedUniqueIndexes.add(uq); + }) + ); + + // For each IX and UQ, delete all other IXes that overlap with smaller or equal column set + indices.stream() + .filter(index -> index.indexType() != Primary) + .forEach(index -> indices.stream() + .filter(ix -> ix.indexType() == NonUnique || convertedUniqueIndexes.contains(ix)) + .filter(ix -> overlaps(index, ix)) + .forEach(ix -> { + String description = String.format("Dropping %s because it overlaps with %s", ix.display(), index.display()); + if (convertedUniqueIndexes.contains(ix)) + { + LOG.info("I should remove {} from the changes and update the description", ix.display()); + } + changes.add(new IndexChange(ix, ChangeType.Drop, description)); + }) + ); - if (type != null) - { - if (type != OverlapType.Identical || !alreadySeen(indexDef1.name(), indexDef2.name())) - multiMap.put(type, new Overlap(table.getSchema().getName(), table.getName(), indexDef1, indexDef2)); - } - } - })); + changes.forEach(change -> LOG.info("{}: {}", table.getSelectName(), change.description())); }); return multiMap; } - private final Set _alreadySeen = new HashSet<>(); - - // Keep track of the identical indexes we've seen so we don't repeat them for both directions - private boolean alreadySeen(String name1, String name2) + // Returns true if index2 has an overlapping column set that's equal to or smaller than index1's + private boolean overlaps(IndexDefinition index1, IndexDefinition index2) { - String key = name1.compareTo(name2) < 0 ? name1 + delim + name2 : name2 + delim + name1; - return !_alreadySeen.add(key); - } + boolean ret = false; - private @Nullable OverlapType overlap(IndexDefinition index1, IndexDefinition index2) - { - String key1 = getKey(index1.columns()); - String key2 = getKey(index2.columns()); - boolean sameFilterConditions = Objects.equals(index1.filterCondition(), index2.filterCondition()); - if (key1.equals(key2)) - return sameFilterConditions ? OverlapType.Identical : OverlapType.OverlappingWithDifferentFilter; - if (key2.startsWith(key1)) + if (!index1.equals(index2) && Objects.equals(index1.filterCondition(), index2.filterCondition())) { - if (index2.indexType() == IndexType.NonUnique && (index1.indexType() == IndexType.Primary || index1.indexType() == IndexType.Unique)) - return OverlapType.UniqueOverlappingNonUnique; - else - return sameFilterConditions ? OverlapType.Overlapping : OverlapType.OverlappingWithDifferentFilter; + String key1 = getKey(index1.columns()); + String key2 = getKey(index2.columns()); + ret = key1.startsWith(key2); } - return null; + + return ret; } private final String delim = Character.toString(31); // Non-printing character that's very unlikely to be in a column name @@ -986,6 +1019,12 @@ private String getKey(List cols) protected record Overlap(String schemaName, String tableName, IndexDefinition indexDef1, IndexDefinition indexDef2) {} + protected enum ChangeType + { + Drop, + Convert + } + protected enum OverlapType { UniqueOverlappingNonUnique("a column list that overlaps another index's column list at the start, but the first index is a unique constraint. These are likely valid") @@ -1011,29 +1050,29 @@ boolean writeScript(Writer writer, Overlap overlap) throws IOException { IndexType type1 = overlap.indexDef1.indexType(); IndexType type2 = overlap.indexDef2.indexType(); - String dropIndex = null; - String otherIndex = null; + IndexDefinition dropIndex = null; + IndexDefinition otherIndex = null; // Prefer to drop the non-PK, then prefer the non-unique, otherwise "drop" them both (let the human decide) - if (type1 == IndexType.Primary) + if (type1 == Primary) { - dropIndex = overlap.indexDef2.name(); - otherIndex = overlap.indexDef1.name(); + dropIndex = overlap.indexDef2; + otherIndex = overlap.indexDef1; } - else if (type2 == IndexType.Primary) + else if (type2 == Primary) { - dropIndex = overlap.indexDef1.name(); - otherIndex = overlap.indexDef2.name(); + dropIndex = overlap.indexDef1; + otherIndex = overlap.indexDef2; } - else if (type1 == IndexType.Unique && type2 == IndexType.NonUnique) + else if (type1 == Unique && type2 == IndexType.NonUnique) { - dropIndex = overlap.indexDef2.name(); - otherIndex = overlap.indexDef1.name(); + dropIndex = overlap.indexDef2; + otherIndex = overlap.indexDef1; } - else if (type2 == IndexType.Unique && type1 == IndexType.NonUnique) + else if (type2 == Unique && type1 == IndexType.NonUnique) { - dropIndex = overlap.indexDef1.name(); - otherIndex = overlap.indexDef2.name(); + dropIndex = overlap.indexDef1; + otherIndex = overlap.indexDef2; } if (dropIndex != null) @@ -1043,26 +1082,59 @@ else if (type2 == IndexType.Unique && type1 == IndexType.NonUnique) else { writer.write("TODO: Human, please help!! You should drop only one of the following, but I couldn't decide which one:\n"); - dropIndex(writer, overlap.schemaName, overlap.tableName, overlap.indexDef1.name(), overlap.indexDef2.name()); - dropIndex(writer, overlap.schemaName, overlap.tableName, overlap.indexDef2.name(), overlap.indexDef1.name()); + dropIndex(writer, overlap.schemaName, overlap.tableName, overlap.indexDef1, overlap.indexDef2); + dropIndex(writer, overlap.schemaName, overlap.tableName, overlap.indexDef2, overlap.indexDef1); writer.write('\n'); } return true; } - + }, + Overlapping("a column list that overlaps another index's column list at the start") + { @Override - String getMessage(Overlap overlap) + boolean writeScript(Writer writer, Overlap overlap) throws IOException { - return overlap.indexDef1.name() + " vs. " + overlap.indexDef2.name() + ": " + join(overlap.indexDef1.columns()); + dropIndex(writer, overlap.schemaName, overlap.tableName, overlap.indexDef1, overlap.indexDef2); + return true; } }, - Overlapping("a column list that overlaps another index's column list at the start") + UniqueOverlappingUnique("a column list that overlaps another index's column list at the start where both indexes are unique/pk") { @Override boolean writeScript(Writer writer, Overlap overlap) throws IOException { - dropIndex(writer, overlap.schemaName, overlap.tableName, overlap.indexDef1.name(), overlap.indexDef2.name()); + IndexType type1 = overlap.indexDef1.indexType(); + IndexType type2 = overlap.indexDef2.indexType(); + IndexDefinition dropIndex; + IndexDefinition otherIndex; + + // Prefer to drop the non-PK, then prefer the non-unique, otherwise "drop" them both (let the human decide) + if (type1 == Primary) + { + dropIndex = overlap.indexDef2; + otherIndex = overlap.indexDef1; + } + else if (type2 == Primary) + { + dropIndex = overlap.indexDef1; + otherIndex = overlap.indexDef2; + } + else + { + // Drop the unique index with fewer columns -- the extra columns are pointless + if (overlap.indexDef2.columns().size() >= overlap.indexDef1.columns().size()) + { + dropIndex = overlap.indexDef2; + otherIndex = overlap.indexDef1; + } + else + { + dropIndex = overlap.indexDef1; + otherIndex = overlap.indexDef2; + } + } + dropIndex(writer, overlap.schemaName, overlap.tableName, dropIndex, otherIndex); return true; } }; @@ -1084,25 +1156,33 @@ public String getDescription() String getMessage(Overlap overlap) { - return overlap.indexDef1.name() + " " + join(overlap.indexDef1.columns()) + " vs. " + overlap.indexDef2.name() + " " + join(overlap.indexDef2.columns()); + return overlap.indexDef1.display() + " vs. " + overlap.indexDef2.display(); } - protected List join(List cols) + protected void dropIndex(Writer writer, String schemaName, String tableName, IndexDefinition dropIndex, IndexDefinition otherIndex) throws IOException { - return cols.stream() - .map(ColumnInfo::getName) - .toList(); - } + if (dropIndex.indexType() == Primary) + throw new IllegalStateException("Should never drop a PK!"); - protected void dropIndex(Writer writer, String schemaName, String tableName, String dropIndex, String otherIndex) throws IOException - { SqlDialect dialect = DbScope.getLabKeyScope().getSqlDialect(); - writer.write("-- This index overlaps with " + otherIndex + "\n"); + writer.write("-- " + dropIndex.display() + " overlaps with " + otherIndex.display() + "\n"); if (dialect.isPostgreSQL()) - writer.write("DROP INDEX " + schemaName + "." + dropIndex + ";\n"); + { + if (dropIndex.indexType() == Unique) + { + String constraintName = getConstraintForIndex(schemaName, dropIndex.name()); + if (constraintName != null) + { + writer.write("ALTER TABLE " + schemaName + "." + tableName + " DROP CONSTRAINT " + constraintName + ";\n"); + return; + } + } + + writer.write("DROP INDEX " + schemaName + "." + dropIndex.name() + ";\n"); + } else - writer.write("DROP INDEX " + dropIndex + " ON " + schemaName + "." + tableName + ";\n"); + writer.write("DROP INDEX " + dropIndex.name() + " ON " + schemaName + "." + tableName + ";\n"); } } From a842b673b5c709ac2254bf2138d68481271c62b8 Mon Sep 17 00:00:00 2001 From: Adam Rauch Date: Thu, 30 Apr 2026 12:57:34 -0700 Subject: [PATCH 05/12] Update display of changes --- .../org/labkey/devtools/ToolsController.java | 100 ++++++++++++------ 1 file changed, 65 insertions(+), 35 deletions(-) diff --git a/devtools/src/org/labkey/devtools/ToolsController.java b/devtools/src/org/labkey/devtools/ToolsController.java index 271e8b5f321..50309de77f9 100644 --- a/devtools/src/org/labkey/devtools/ToolsController.java +++ b/devtools/src/org/labkey/devtools/ToolsController.java @@ -1,7 +1,7 @@ package org.labkey.devtools; import org.apache.commons.collections4.MultiValuedMap; -import org.apache.commons.collections4.multimap.ArrayListValuedHashMap; +import org.apache.commons.collections4.multimap.ArrayListValuedLinkedHashMap; import org.apache.commons.lang3.StringUtils; import org.apache.logging.log4j.Logger; import org.jetbrains.annotations.NotNull; @@ -73,7 +73,6 @@ import java.nio.file.SimpleFileVisitor; import java.nio.file.attribute.BasicFileAttributes; import java.sql.SQLException; -import java.util.Arrays; import java.util.Collection; import java.util.Collections; import java.util.Comparator; @@ -750,19 +749,28 @@ public class OverlappingIndicesAction extends AbstractOverlappingIndicesAction @Override public ModelAndView getView(OverlappingIndicesForm form, boolean reshow, BindException errors) { - MultiValuedMap multiMap = getOverlappingIndices(form); + ActionURL url = getViewContext().getActionURL().clone(); + + if (Boolean.TRUE.equals(form.clearCaches())) + { + CacheManager.clearAllKnownCaches(); + url.deleteParameter("clearCaches"); + } + + MultiValuedMap multiMap = getOverlappingIndices(form); return new VBox( new HtmlView(DOM.createHtmlFragment( - Arrays.stream(OverlapType.values()).flatMap(type -> + multiMap.keySet().stream().flatMap(schemaName -> Stream.of( - type != OverlapType.UniqueOverlappingNonUnique ? BR() : null, - DOM.STRONG(StringUtilsLabKey.pluralize(multiMap.get(type).size(), "index has ", "indices have ") + type.getDescription() + ":", BR()), + BR(), + DOM.STRONG("Schema ", LinkBuilder.simpleLink(schemaName, new ActionURL(OverlappingIndicesAction.class, getContainer()).addParameter("schemaName", schemaName)), " needs " + StringUtilsLabKey.pluralize(multiMap.get(schemaName).size(), "change") + ":", BR()), DOM.TABLE( - multiMap.get(type).stream() - .map(overlap -> DOM.TR( - DOM.TD(at(style, "width:120px;"), LinkBuilder.simpleLink(overlap.schemaName(), new ActionURL(OverlappingIndicesAction.class, getContainer()).addParameter("schemaName", overlap.schemaName()))), - DOM.TD(type.getMessage(overlap)), + multiMap.get(schemaName).stream() + .sorted(Comparator.comparing(change -> change.table().getName())) + .map(change -> DOM.TR( + DOM.TD(at(style, "width:200px;"), change.table().getName()), + DOM.TD(change.description()), "\n" )) ) @@ -771,11 +779,14 @@ public ModelAndView getView(OverlappingIndicesForm form, boolean reshow, BindExc )), new HtmlView(DOM.createHtmlFragment( BR(), - new ButtonBuilder("Create SQL Scripts That Drop Overlapping Indices").href(getViewContext().getActionURL()).usePost()) - ) + new ButtonBuilder("Create SQL Scripts That Drop Redundant Indices").href(url).usePost(), + " ", + new ButtonBuilder("Clear Caches and Refresh").href(url.addParameter("clearCaches", true)) + )) ); } + @Override public void addNavTrail(NavTree root) { @@ -786,17 +797,19 @@ public void addNavTrail(NavTree root) @Override public boolean handlePost(OverlappingIndicesForm form, BindException errors) { - MultiValuedMap multiMap = getOverlappingIndices(form); + MultiValuedMap multiMap = getOverlappingIndices(form); try { - Arrays.stream(OverlapType.values()).forEach(type -> multiMap.get(type).forEach(overlap -> { + multiMap.keySet() + .forEach(schemaName -> multiMap.get(schemaName).forEach(change -> { try { // All writers are closed below - WriterContext context = getWriterContext(overlap.schemaName()); - if (type.writeScript(context.getWriter(), overlap)) - context.setModified(); + WriterContext context = getWriterContext(schemaName); + // TODO: Write script!! +// if (type.writeScript(context.getWriter(), overlap)) +// context.setModified(); } catch (IOException e) { @@ -921,14 +934,14 @@ public URLHelper getSuccessURL(OverlappingIndicesForm form) } } - public record OverlappingIndicesForm(String schemaName) {} - private record IndexChange(IndexDefinition index, ChangeType type, String description) {} + public record OverlappingIndicesForm(String schemaName, Boolean clearCaches) {} + public record IndexChange(TableInfo table, IndexDefinition index, ChangeType type, String description) {} protected static abstract class AbstractOverlappingIndicesAction extends FormViewAction { - protected MultiValuedMap getOverlappingIndices(OverlappingIndicesForm form) + protected MultiValuedMap getOverlappingIndices(OverlappingIndicesForm form) { - MultiValuedMap multiMap = new ArrayListValuedHashMap<>(); + MultiValuedMap multiMap = new ArrayListValuedLinkedHashMap<>(); DbScope scope = DbScope.getLabKeyScope(); ModuleLoader.getInstance().getModules().stream() @@ -942,22 +955,25 @@ protected MultiValuedMap getOverlappingIndices(Overlapping var changes = new LinkedList(); - // Find the PK (if present), and drop all indexes that overlap with a smaller or equal column set + // Step #1: Find the PK (if present), and drop all indices that overlap with a smaller or equal + // column set. The third step below would take care of overlapping non-unique indices, so this is + // mainly to drop unique indices that overlap with the PK. indices.stream() .filter(ix -> ix.indexType() == Primary) .findFirst() .ifPresent(pk -> indices.stream() .filter(index -> overlaps(pk, index)) .forEach(index -> { - changes.add(new IndexChange(index, ChangeType.Drop, String.format("Dropping %s because it overlaps with %s", index.display(), pk.display()))); + changes.add(new IndexChange(table, index, ChangeType.Drop, String.format("Dropping %s because it overlaps with %s", index.display(), pk.display()))); }) ); changes.forEach(change -> indices.remove(change.index())); - Set convertedUniqueIndexes = new HashSet<>(); + Set convertedUniqueIndices = new HashSet<>(); - // For each unique index, switch it to a non-unique index if there's at least one UQ or PK that overlaps a smaller or equal column set + // Step #2: For each unique index, switch it to a non-unique index if there's any UQ or PK that + // overlaps with a smaller or equal column set. indices.stream() .filter(index -> index.indexType() == Unique) .forEach(uq -> indices.stream() @@ -965,31 +981,45 @@ protected MultiValuedMap getOverlappingIndices(Overlapping .filter(index -> overlaps(uq, index)) .findFirst() .ifPresent(index -> { - changes.add(new IndexChange(uq, ChangeType.Convert, String.format("Converting %s to an INDEX because %s overlaps it with a smaller column set", uq.display(), index.display()))); - convertedUniqueIndexes.add(uq); + changes.add(new IndexChange(table, uq, ChangeType.Convert, String.format("Converting %s to an INDEX because %s overlaps it with a smaller column set", uq.display(), index.display()))); + convertedUniqueIndices.add(uq); }) ); - // For each IX and UQ, delete all other IXes that overlap with smaller or equal column set + Set droppedIndices = new HashSet<>(); + + // Step #3: For each index (unique or non-unique), delete all other non-unique indices that overlap + // with a smaller or equal column set. indices.stream() .filter(index -> index.indexType() != Primary) .forEach(index -> indices.stream() - .filter(ix -> ix.indexType() == NonUnique || convertedUniqueIndexes.contains(ix)) + .filter(ix -> ix.indexType() == NonUnique || convertedUniqueIndices.contains(ix)) .filter(ix -> overlaps(index, ix)) .forEach(ix -> { String description = String.format("Dropping %s because it overlaps with %s", ix.display(), index.display()); - if (convertedUniqueIndexes.contains(ix)) + if (convertedUniqueIndices.contains(ix)) { LOG.info("I should remove {} from the changes and update the description", ix.display()); } - changes.add(new IndexChange(ix, ChangeType.Drop, description)); + changes.add(new IndexChange(table, ix, ChangeType.Drop, description)); + droppedIndices.add(ix); + // TODO: filter out droppedIndices }) ); - changes.forEach(change -> LOG.info("{}: {}", table.getSelectName(), change.description())); + changes.forEach(change -> multiMap.put(change.table().getSchema().getName(), change)); }); + + //LOG.info("{}: {}", change.table().getSelectName(), change.description()) return multiMap; + + // TODO: Combine abstract + action + // TODO: Filter out dropped indices + // TODO: Create scripts + // TODO: implement & test convert -> drop sequence + // TODO: junit test + // TODO: Delete old overlap code } // Returns true if index2 has an overlapping column set that's equal to or smaller than index1's @@ -1099,7 +1129,7 @@ boolean writeScript(Writer writer, Overlap overlap) throws IOException return true; } }, - UniqueOverlappingUnique("a column list that overlaps another index's column list at the start where both indexes are unique/pk") + UniqueOverlappingUnique("a column list that overlaps another index's column list at the start where both indices are unique/pk") { @Override boolean writeScript(Writer writer, Overlap overlap) throws IOException @@ -1188,8 +1218,8 @@ protected void dropIndex(Writer writer, String schemaName, String tableName, Ind private record IndexKey(String schemaName, String indexName) {} - // Most, but not all, unique indexes are created by adding a unique constraint; in those cases, we need to drop - // the associated constraint. However, for explicitly created unique indexes, we need to drop the index instead. + // Most, but not all, unique indices are created by adding a unique constraint; in those cases, we need to drop + // the associated constraint. However, for explicitly created unique indices, we need to drop the index instead. // If this is a unique index associated with a constraint, return that constraint name. Otherwise, return null. private static @Nullable String getConstraintForIndex(String schemaName, String indexName) { From b576c486354b1aa097b8735ac827b33208d9e50c Mon Sep 17 00:00:00 2001 From: Adam Rauch Date: Thu, 30 Apr 2026 13:33:07 -0700 Subject: [PATCH 06/12] Combine abstract and concrete action. Filter out dropped indices. --- .../org/labkey/devtools/ToolsController.java | 255 ++++++++++-------- 1 file changed, 139 insertions(+), 116 deletions(-) diff --git a/devtools/src/org/labkey/devtools/ToolsController.java b/devtools/src/org/labkey/devtools/ToolsController.java index 50309de77f9..76c68c95137 100644 --- a/devtools/src/org/labkey/devtools/ToolsController.java +++ b/devtools/src/org/labkey/devtools/ToolsController.java @@ -743,9 +743,14 @@ public void addNavTrail(NavTree root) } } + public record OverlappingIndicesForm(String schemaName, Boolean clearCaches) {} + public record IndexChange(TableInfo table, IndexDefinition index, ChangeType type, String description) {} + @RequiresPermission(AdminPermission.class) - public class OverlappingIndicesAction extends AbstractOverlappingIndicesAction + public class OverlappingIndicesAction extends FormViewAction { + private final String delim = Character.toString(31); // Non-printing character that's very unlikely to be in a column name + @Override public ModelAndView getView(OverlappingIndicesForm form, boolean reshow, BindException errors) { @@ -761,6 +766,8 @@ public ModelAndView getView(OverlappingIndicesForm form, boolean reshow, BindExc return new VBox( new HtmlView(DOM.createHtmlFragment( + "Total number of changes needed: " + multiMap.keys().size(), + BR(), multiMap.keySet().stream().flatMap(schemaName -> Stream.of( BR(), @@ -786,7 +793,6 @@ public ModelAndView getView(OverlappingIndicesForm form, boolean reshow, BindExc ); } - @Override public void addNavTrail(NavTree root) { @@ -825,6 +831,110 @@ public boolean handlePost(OverlappingIndicesForm form, BindException errors) return true; } + private MultiValuedMap getOverlappingIndices(OverlappingIndicesForm form) + { + MultiValuedMap multiMap = new ArrayListValuedLinkedHashMap<>(); + DbScope scope = DbScope.getLabKeyScope(); + + ModuleLoader.getInstance().getModules().stream() + .flatMap(module -> module.getSchemaNames().stream().filter(name -> !module.getProvisionedSchemaNames().contains(name))) + .filter(schemaName -> form.schemaName() == null || schemaName.equals(form.schemaName())) + .sorted(String.CASE_INSENSITIVE_ORDER) + .map(name -> scope.getSchema(name, DbSchemaType.Module)) + .flatMap(schema -> schema.getTableNames().stream().map(schema::getTable)) + .forEach(table -> { + var indices = new LinkedHashSet<>(table.getAllIndices()); + var changes = new LinkedList(); + Set droppedIndices = new HashSet<>(); + + // Step #1: Find the PK (if present), and drop all indices that overlap with a smaller or equal + // column set. The third step below would take care of overlapping non-unique indices, so this is + // mainly to drop unique indices that overlap with the PK. + indices.stream() + .filter(ix -> ix.indexType() == Primary) + .findFirst() + .ifPresent(pk -> indices.stream() + .filter(index -> overlaps(pk, index)) + .forEach(index -> { + changes.add(new IndexChange(table, index, ChangeType.Drop, String.format("Dropping %s because it overlaps with %s", index.display(), pk.display()))); + droppedIndices.add(index); + }) + ); + + Set convertedUniqueIndices = new HashSet<>(); + + // Step #2: For each unique index, switch it to a non-unique index if there's any UQ or PK that + // overlaps with a smaller or equal column set. + streamIndices(indices, droppedIndices) + .filter(index -> index.indexType() == Unique) + .forEach(uq -> streamIndices(indices, convertedUniqueIndices) + .filter(index -> index.indexType() == Primary || index.indexType() == Unique) + .filter(index -> overlaps(uq, index)) + .findFirst() + .ifPresent(index -> { + changes.add(new IndexChange(table, uq, ChangeType.Convert, String.format("Converting %s to an INDEX because %s overlaps it with a smaller column set", uq.display(), index.display()))); + convertedUniqueIndices.add(uq); + }) + ); + + // Step #3: For each index (unique or non-unique), delete all other non-unique indices that overlap + // with a smaller or equal column set. + streamIndices(indices, droppedIndices) + .filter(index -> index.indexType() != Primary) + .forEach(index -> streamIndices(indices, droppedIndices) + .filter(ix -> ix.indexType() == NonUnique || convertedUniqueIndices.contains(ix)) + .filter(ix -> overlaps(index, ix)) + .forEach(ix -> { + String description = String.format("Dropping %s because it overlaps with %s", ix.display(), index.display()); + if (convertedUniqueIndices.contains(ix)) + { + LOG.info("I should remove {} from the changes and update the description", ix.display()); + } + changes.add(new IndexChange(table, ix, ChangeType.Drop, description)); + droppedIndices.add(ix); + }) + ); + + changes.forEach(change -> multiMap.put(change.table().getSchema().getName(), change)); + }); + + return multiMap; + + // TODO: Create scripts + // TODO: implement & test convert -> drop sequence + // TODO: junit test + // TODO: Delete old overlap code + // TODO: Warn in script comment if column lists and types are identical + } + + // Helper that filters out the dropped indices + private Stream streamIndices(Set indices, Set droppedIndices) + { + return indices.stream().filter(index -> !droppedIndices.contains(index)); + } + + // Returns true if index2 has an overlapping column set that's equal to or smaller than index1's + private boolean overlaps(IndexDefinition index1, IndexDefinition index2) + { + boolean ret = false; + + if (!index1.equals(index2) && Objects.equals(index1.filterCondition(), index2.filterCondition())) + { + String key1 = getKey(index1.columns()); + String key2 = getKey(index2.columns()); + ret = key1.startsWith(key2); + } + + return ret; + } + + private String getKey(List cols) + { + return cols.stream() + .map(col -> col.getName().toLowerCase()) + .collect(Collectors.joining(delim)) + delim; + } + private static class WriterContext { private final File _scriptDirectory; @@ -934,125 +1044,38 @@ public URLHelper getSuccessURL(OverlappingIndicesForm form) } } - public record OverlappingIndicesForm(String schemaName, Boolean clearCaches) {} - public record IndexChange(TableInfo table, IndexDefinition index, ChangeType type, String description) {} - - protected static abstract class AbstractOverlappingIndicesAction extends FormViewAction - { - protected MultiValuedMap getOverlappingIndices(OverlappingIndicesForm form) - { - MultiValuedMap multiMap = new ArrayListValuedLinkedHashMap<>(); - DbScope scope = DbScope.getLabKeyScope(); - - ModuleLoader.getInstance().getModules().stream() - .flatMap(module -> module.getSchemaNames().stream().filter(name -> !module.getProvisionedSchemaNames().contains(name))) - .filter(schemaName -> form.schemaName() == null || schemaName.equals(form.schemaName())) - .sorted(String.CASE_INSENSITIVE_ORDER) - .map(name -> scope.getSchema(name, DbSchemaType.Module)) - .flatMap(schema -> schema.getTableNames().stream().map(schema::getTable)) - .forEach(table -> { - var indices = new LinkedHashSet<>(table.getAllIndices()); - - var changes = new LinkedList(); - - // Step #1: Find the PK (if present), and drop all indices that overlap with a smaller or equal - // column set. The third step below would take care of overlapping non-unique indices, so this is - // mainly to drop unique indices that overlap with the PK. - indices.stream() - .filter(ix -> ix.indexType() == Primary) - .findFirst() - .ifPresent(pk -> indices.stream() - .filter(index -> overlaps(pk, index)) - .forEach(index -> { - changes.add(new IndexChange(table, index, ChangeType.Drop, String.format("Dropping %s because it overlaps with %s", index.display(), pk.display()))); - }) - ); - - changes.forEach(change -> indices.remove(change.index())); - - Set convertedUniqueIndices = new HashSet<>(); - - // Step #2: For each unique index, switch it to a non-unique index if there's any UQ or PK that - // overlaps with a smaller or equal column set. - indices.stream() - .filter(index -> index.indexType() == Unique) - .forEach(uq -> indices.stream() - .filter(index -> index.indexType() == Primary || index.indexType() == Unique) - .filter(index -> overlaps(uq, index)) - .findFirst() - .ifPresent(index -> { - changes.add(new IndexChange(table, uq, ChangeType.Convert, String.format("Converting %s to an INDEX because %s overlaps it with a smaller column set", uq.display(), index.display()))); - convertedUniqueIndices.add(uq); - }) - ); - - Set droppedIndices = new HashSet<>(); - - // Step #3: For each index (unique or non-unique), delete all other non-unique indices that overlap - // with a smaller or equal column set. - indices.stream() - .filter(index -> index.indexType() != Primary) - .forEach(index -> indices.stream() - .filter(ix -> ix.indexType() == NonUnique || convertedUniqueIndices.contains(ix)) - .filter(ix -> overlaps(index, ix)) - .forEach(ix -> { - String description = String.format("Dropping %s because it overlaps with %s", ix.display(), index.display()); - if (convertedUniqueIndices.contains(ix)) - { - LOG.info("I should remove {} from the changes and update the description", ix.display()); - } - changes.add(new IndexChange(table, ix, ChangeType.Drop, description)); - droppedIndices.add(ix); - // TODO: filter out droppedIndices - }) - ); - - changes.forEach(change -> multiMap.put(change.table().getSchema().getName(), change)); - }); - - - //LOG.info("{}: {}", change.table().getSelectName(), change.description()) - return multiMap; - - // TODO: Combine abstract + action - // TODO: Filter out dropped indices - // TODO: Create scripts - // TODO: implement & test convert -> drop sequence - // TODO: junit test - // TODO: Delete old overlap code - } - - // Returns true if index2 has an overlapping column set that's equal to or smaller than index1's - private boolean overlaps(IndexDefinition index1, IndexDefinition index2) - { - boolean ret = false; - - if (!index1.equals(index2) && Objects.equals(index1.filterCondition(), index2.filterCondition())) - { - String key1 = getKey(index1.columns()); - String key2 = getKey(index2.columns()); - ret = key1.startsWith(key2); - } - - return ret; - } - - private final String delim = Character.toString(31); // Non-printing character that's very unlikely to be in a column name - - private String getKey(List cols) - { - return cols.stream() - .map(col -> col.getName().toLowerCase()) - .collect(Collectors.joining(delim)) + delim; - } - } - protected record Overlap(String schemaName, String tableName, IndexDefinition indexDef1, IndexDefinition indexDef2) {} protected enum ChangeType { Drop, - Convert + Convert; + +// abstract void writeScript(Writer writer, IndexDefinition index) throws IOException +// { +// if (dropIndex.indexType() == Primary) +// throw new IllegalStateException("Should never drop a PK!"); +// +// SqlDialect dialect = DbScope.getLabKeyScope().getSqlDialect(); +// writer.write("-- " + dropIndex.display() + " overlaps with " + otherIndex.display() + "\n"); +// +// if (dialect.isPostgreSQL()) +// { +// if (dropIndex.indexType() == Unique) +// { +// String constraintName = getConstraintForIndex(schemaName, dropIndex.name()); +// if (constraintName != null) +// { +// writer.write("ALTER TABLE " + schemaName + "." + tableName + " DROP CONSTRAINT " + constraintName + ";\n"); +// return; +// } +// } +// +// writer.write("DROP INDEX " + schemaName + "." + dropIndex.name() + ";\n"); +// } +// else +// writer.write("DROP INDEX " + dropIndex.name() + " ON " + schemaName + "." + tableName + ";\n"); +// } } protected enum OverlapType From c267b6e9848b3b76f21d088641fdea7fe07b39fe Mon Sep 17 00:00:00 2001 From: Adam Rauch Date: Thu, 30 Apr 2026 13:49:07 -0700 Subject: [PATCH 07/12] Create DROP scripts --- .../org/labkey/devtools/ToolsController.java | 232 +++--------------- 1 file changed, 40 insertions(+), 192 deletions(-) diff --git a/devtools/src/org/labkey/devtools/ToolsController.java b/devtools/src/org/labkey/devtools/ToolsController.java index 76c68c95137..d3002722901 100644 --- a/devtools/src/org/labkey/devtools/ToolsController.java +++ b/devtools/src/org/labkey/devtools/ToolsController.java @@ -26,7 +26,6 @@ import org.labkey.api.data.SqlSelector; import org.labkey.api.data.TableInfo; import org.labkey.api.data.TableInfo.IndexDefinition; -import org.labkey.api.data.TableInfo.IndexType; import org.labkey.api.data.dialect.SqlDialect; import org.labkey.api.module.Module; import org.labkey.api.module.ModuleLoader; @@ -809,19 +808,21 @@ public boolean handlePost(OverlappingIndicesForm form, BindException errors) { multiMap.keySet() .forEach(schemaName -> multiMap.get(schemaName).forEach(change -> { - try - { - // All writers are closed below - WriterContext context = getWriterContext(schemaName); - // TODO: Write script!! -// if (type.writeScript(context.getWriter(), overlap)) -// context.setModified(); - } - catch (IOException e) - { - throw new RuntimeException(e); - } - })); + try + { + if (change.index().indexType() == Primary) + throw new IllegalStateException("Should not be trying to modify a PK! (" + change + ")"); + + // All writers are closed below in the finally + WriterContext context = getWriterContext(schemaName); + change.type().writeScript(context.getWriter(), change); + context.setModified(); + } + catch (IOException e) + { + throw new RuntimeException(e); + } + })); } finally { @@ -900,11 +901,10 @@ private MultiValuedMap getOverlappingIndices(OverlappingInd return multiMap; - // TODO: Create scripts + // TODO: Create script to convert UQ -> IX // TODO: implement & test convert -> drop sequence // TODO: junit test - // TODO: Delete old overlap code - // TODO: Warn in script comment if column lists and types are identical + // TODO: Warn in description if column lists and types are identical } // Helper that filters out the dropped indices @@ -1044,199 +1044,47 @@ public URLHelper getSuccessURL(OverlappingIndicesForm form) } } - protected record Overlap(String schemaName, String tableName, IndexDefinition indexDef1, IndexDefinition indexDef2) {} - protected enum ChangeType { - Drop, - Convert; - -// abstract void writeScript(Writer writer, IndexDefinition index) throws IOException -// { -// if (dropIndex.indexType() == Primary) -// throw new IllegalStateException("Should never drop a PK!"); -// -// SqlDialect dialect = DbScope.getLabKeyScope().getSqlDialect(); -// writer.write("-- " + dropIndex.display() + " overlaps with " + otherIndex.display() + "\n"); -// -// if (dialect.isPostgreSQL()) -// { -// if (dropIndex.indexType() == Unique) -// { -// String constraintName = getConstraintForIndex(schemaName, dropIndex.name()); -// if (constraintName != null) -// { -// writer.write("ALTER TABLE " + schemaName + "." + tableName + " DROP CONSTRAINT " + constraintName + ";\n"); -// return; -// } -// } -// -// writer.write("DROP INDEX " + schemaName + "." + dropIndex.name() + ";\n"); -// } -// else -// writer.write("DROP INDEX " + dropIndex.name() + " ON " + schemaName + "." + tableName + ";\n"); -// } - } - - protected enum OverlapType - { - UniqueOverlappingNonUnique("a column list that overlaps another index's column list at the start, but the first index is a unique constraint. These are likely valid") + Drop { @Override - boolean writeScript(Writer writer, Overlap overlap) + void writeScript(Writer writer, IndexChange change) throws IOException { - return false; // Write nothing - } - }, - OverlappingWithDifferentFilter("a column list that overlaps another index's column list at the start, but with different filter conditions. These are likely valid") - { - @Override - boolean writeScript(Writer writer, Overlap overlap) - { - return false; // Write nothing - } - }, - Identical("a column list that's identical to another index's column list") - { - @Override - boolean writeScript(Writer writer, Overlap overlap) throws IOException - { - IndexType type1 = overlap.indexDef1.indexType(); - IndexType type2 = overlap.indexDef2.indexType(); - IndexDefinition dropIndex = null; - IndexDefinition otherIndex = null; + IndexDefinition dropIndex = change.index(); + String schemaName = change.table().getSchema().getName(); + String tableName = change.table().getName(); - // Prefer to drop the non-PK, then prefer the non-unique, otherwise "drop" them both (let the human decide) - if (type1 == Primary) - { - dropIndex = overlap.indexDef2; - otherIndex = overlap.indexDef1; - } - else if (type2 == Primary) - { - dropIndex = overlap.indexDef1; - otherIndex = overlap.indexDef2; - } - else if (type1 == Unique && type2 == IndexType.NonUnique) - { - dropIndex = overlap.indexDef2; - otherIndex = overlap.indexDef1; - } - else if (type2 == Unique && type1 == IndexType.NonUnique) - { - dropIndex = overlap.indexDef1; - otherIndex = overlap.indexDef2; - } + writer.write("-- " + change.description() + "\n"); - if (dropIndex != null) + if (DbScope.getLabKeyScope().getSqlDialect().isPostgreSQL()) { - dropIndex(writer, overlap.schemaName, overlap.tableName, dropIndex, otherIndex); + if (dropIndex.indexType() == Unique) + { + String constraintName = getConstraintForIndex(schemaName, dropIndex.name()); + if (constraintName != null) + { + writer.write("ALTER TABLE " + schemaName + "." + tableName + " DROP CONSTRAINT " + constraintName + ";\n"); + return; + } + } + + writer.write("DROP INDEX " + schemaName + "." + dropIndex.name() + ";\n"); } else - { - writer.write("TODO: Human, please help!! You should drop only one of the following, but I couldn't decide which one:\n"); - dropIndex(writer, overlap.schemaName, overlap.tableName, overlap.indexDef1, overlap.indexDef2); - dropIndex(writer, overlap.schemaName, overlap.tableName, overlap.indexDef2, overlap.indexDef1); - writer.write('\n'); - } - - return true; + writer.write("DROP INDEX " + dropIndex.name() + " ON " + schemaName + "." + tableName + ";\n"); } }, - Overlapping("a column list that overlaps another index's column list at the start") + Convert { @Override - boolean writeScript(Writer writer, Overlap overlap) throws IOException + void writeScript(Writer writer, IndexChange change) throws IOException { - dropIndex(writer, overlap.schemaName, overlap.tableName, overlap.indexDef1, overlap.indexDef2); - return true; - } - }, - UniqueOverlappingUnique("a column list that overlaps another index's column list at the start where both indices are unique/pk") - { - @Override - boolean writeScript(Writer writer, Overlap overlap) throws IOException - { - IndexType type1 = overlap.indexDef1.indexType(); - IndexType type2 = overlap.indexDef2.indexType(); - IndexDefinition dropIndex; - IndexDefinition otherIndex; - - // Prefer to drop the non-PK, then prefer the non-unique, otherwise "drop" them both (let the human decide) - if (type1 == Primary) - { - dropIndex = overlap.indexDef2; - otherIndex = overlap.indexDef1; - } - else if (type2 == Primary) - { - dropIndex = overlap.indexDef1; - otherIndex = overlap.indexDef2; - } - else - { - // Drop the unique index with fewer columns -- the extra columns are pointless - if (overlap.indexDef2.columns().size() >= overlap.indexDef1.columns().size()) - { - dropIndex = overlap.indexDef2; - otherIndex = overlap.indexDef1; - } - else - { - dropIndex = overlap.indexDef1; - otherIndex = overlap.indexDef2; - } - } - dropIndex(writer, overlap.schemaName, overlap.tableName, dropIndex, otherIndex); - return true; + writer.write("I don't know how to do this yet!!"); } }; - private final String _description; - - OverlapType(String description) - { - _description = description; - } - - public String getDescription() - { - return _description; - } - - // Return true if content has been written to the script file - abstract boolean writeScript(Writer writer, Overlap overlap) throws IOException; - - String getMessage(Overlap overlap) - { - return overlap.indexDef1.display() + " vs. " + overlap.indexDef2.display(); - } - - protected void dropIndex(Writer writer, String schemaName, String tableName, IndexDefinition dropIndex, IndexDefinition otherIndex) throws IOException - { - if (dropIndex.indexType() == Primary) - throw new IllegalStateException("Should never drop a PK!"); - - SqlDialect dialect = DbScope.getLabKeyScope().getSqlDialect(); - writer.write("-- " + dropIndex.display() + " overlaps with " + otherIndex.display() + "\n"); - - if (dialect.isPostgreSQL()) - { - if (dropIndex.indexType() == Unique) - { - String constraintName = getConstraintForIndex(schemaName, dropIndex.name()); - if (constraintName != null) - { - writer.write("ALTER TABLE " + schemaName + "." + tableName + " DROP CONSTRAINT " + constraintName + ";\n"); - return; - } - } - - writer.write("DROP INDEX " + schemaName + "." + dropIndex.name() + ";\n"); - } - else - writer.write("DROP INDEX " + dropIndex.name() + " ON " + schemaName + "." + tableName + ";\n"); - } + abstract void writeScript(Writer writer, IndexChange change) throws IOException; } private record IndexKey(String schemaName, String indexName) {} From 8496723b72ddd8e8820f39021f3cb0af3b2690dc Mon Sep 17 00:00:00 2001 From: Adam Rauch Date: Thu, 30 Apr 2026 16:19:07 -0700 Subject: [PATCH 08/12] Drop description. Improve script comments. --- .../org/labkey/devtools/ToolsController.java | 70 +++++++++++-------- 1 file changed, 40 insertions(+), 30 deletions(-) diff --git a/devtools/src/org/labkey/devtools/ToolsController.java b/devtools/src/org/labkey/devtools/ToolsController.java index d3002722901..ba3d26c19a0 100644 --- a/devtools/src/org/labkey/devtools/ToolsController.java +++ b/devtools/src/org/labkey/devtools/ToolsController.java @@ -810,9 +810,6 @@ public boolean handlePost(OverlappingIndicesForm form, BindException errors) .forEach(schemaName -> multiMap.get(schemaName).forEach(change -> { try { - if (change.index().indexType() == Primary) - throw new IllegalStateException("Should not be trying to modify a PK! (" + change + ")"); - // All writers are closed below in the finally WriterContext context = getWriterContext(schemaName); change.type().writeScript(context.getWriter(), change); @@ -832,6 +829,17 @@ public boolean handlePost(OverlappingIndicesForm form, BindException errors) return true; } + @Override + public void validateCommand(OverlappingIndicesForm form, Errors errors) + { + } + + @Override + public URLHelper getSuccessURL(OverlappingIndicesForm form) + { + return new ActionURL(OverlappingIndicesAction.class, getContainer()); + } + private MultiValuedMap getOverlappingIndices(OverlappingIndicesForm form) { MultiValuedMap multiMap = new ArrayListValuedLinkedHashMap<>(); @@ -839,7 +847,7 @@ private MultiValuedMap getOverlappingIndices(OverlappingInd ModuleLoader.getInstance().getModules().stream() .flatMap(module -> module.getSchemaNames().stream().filter(name -> !module.getProvisionedSchemaNames().contains(name))) - .filter(schemaName -> form.schemaName() == null || schemaName.equals(form.schemaName())) + .filter(schemaName -> form.schemaName() == null || schemaName.equalsIgnoreCase(form.schemaName())) .sorted(String.CASE_INSENSITIVE_ORDER) .map(name -> scope.getSchema(name, DbSchemaType.Module)) .flatMap(schema -> schema.getTableNames().stream().map(schema::getTable)) @@ -857,7 +865,7 @@ private MultiValuedMap getOverlappingIndices(OverlappingInd .ifPresent(pk -> indices.stream() .filter(index -> overlaps(pk, index)) .forEach(index -> { - changes.add(new IndexChange(table, index, ChangeType.Drop, String.format("Dropping %s because it overlaps with %s", index.display(), pk.display()))); + changes.add(new IndexChange(table, index, ChangeType.Drop, getDropDescription(index, pk))); droppedIndices.add(index); }) ); @@ -873,7 +881,7 @@ private MultiValuedMap getOverlappingIndices(OverlappingInd .filter(index -> overlaps(uq, index)) .findFirst() .ifPresent(index -> { - changes.add(new IndexChange(table, uq, ChangeType.Convert, String.format("Converting %s to an INDEX because %s overlaps it with a smaller column set", uq.display(), index.display()))); + changes.add(new IndexChange(table, uq, ChangeType.Convert, String.format("Converting %s from unique to non-unique index because %s overlaps it with a smaller column set", uq.display(), index.display()))); convertedUniqueIndices.add(uq); }) ); @@ -886,10 +894,10 @@ private MultiValuedMap getOverlappingIndices(OverlappingInd .filter(ix -> ix.indexType() == NonUnique || convertedUniqueIndices.contains(ix)) .filter(ix -> overlaps(index, ix)) .forEach(ix -> { - String description = String.format("Dropping %s because it overlaps with %s", ix.display(), index.display()); + String description = getDropDescription(ix, index); if (convertedUniqueIndices.contains(ix)) { - LOG.info("I should remove {} from the changes and update the description", ix.display()); + throw new IllegalStateException("Dropping an index that's been converted to non-unique is not yet supported"); } changes.add(new IndexChange(table, ix, ChangeType.Drop, description)); droppedIndices.add(ix); @@ -901,10 +909,7 @@ private MultiValuedMap getOverlappingIndices(OverlappingInd return multiMap; - // TODO: Create script to convert UQ -> IX - // TODO: implement & test convert -> drop sequence // TODO: junit test - // TODO: Warn in description if column lists and types are identical } // Helper that filters out the dropped indices @@ -913,6 +918,13 @@ private Stream streamIndices(Set indices, Set< return indices.stream().filter(index -> !droppedIndices.contains(index)); } + private String getDropDescription(IndexDefinition dropIndex, IndexDefinition otherIndex) + { + String warning = dropIndex.indexType() == otherIndex.indexType() && dropIndex.columns().size() == otherIndex.columns().size() ? + ". Note: You may want to drop " + otherIndex.display() + " instead!!" : ""; + return String.format("Dropping %s because it overlaps with %s", dropIndex.display(), otherIndex.display()) + warning; + } + // Returns true if index2 has an overlapping column set that's equal to or smaller than index1's private boolean overlaps(IndexDefinition index1, IndexDefinition index2) { @@ -1031,17 +1043,6 @@ private void closeAllContexts() } }); } - - @Override - public void validateCommand(OverlappingIndicesForm form, Errors errors) - { - } - - @Override - public URLHelper getSuccessURL(OverlappingIndicesForm form) - { - return new ActionURL(BeginAction.class, getContainer()); - } } protected enum ChangeType @@ -1049,12 +1050,8 @@ protected enum ChangeType Drop { @Override - void writeScript(Writer writer, IndexChange change) throws IOException + void writeScript(Writer writer, IndexChange change, String schemaName, String tableName, IndexDefinition dropIndex) throws IOException { - IndexDefinition dropIndex = change.index(); - String schemaName = change.table().getSchema().getName(); - String tableName = change.table().getName(); - writer.write("-- " + change.description() + "\n"); if (DbScope.getLabKeyScope().getSqlDialect().isPostgreSQL()) @@ -1078,13 +1075,26 @@ void writeScript(Writer writer, IndexChange change) throws IOException Convert { @Override - void writeScript(Writer writer, IndexChange change) throws IOException + void writeScript(Writer writer, IndexChange change, String schemaName, String tableName, IndexDefinition changeIndex) throws IOException { - writer.write("I don't know how to do this yet!!"); + Drop.writeScript(writer, change); + String indexName = changeIndex.name().replace("uq", "ix").replace("unique", "index"); + writer.write("CREATE INDEX " + indexName + " ON " + schemaName + "." + tableName + "(" + changeIndex.columns().stream().map(ColumnInfo::getName).collect(Collectors.joining(", ")) + ");\n"); } }; - abstract void writeScript(Writer writer, IndexChange change) throws IOException; + final void writeScript(Writer writer, IndexChange change) throws IOException + { + TableInfo table = change.table(); + IndexDefinition index = change.index(); + + if (index.indexType() == Primary) + throw new IllegalStateException("Should not be trying to modify a PK! (" + change + ")"); + + writeScript(writer, change, table.getSchema().getName(), table.getName(), index); + } + + abstract void writeScript(Writer writer, IndexChange change, String schemaName, String tableName, IndexDefinition index) throws IOException; } private record IndexKey(String schemaName, String indexName) {} From 22f0f3a9e9bf4f5138bc6d6450fbacefbdbfc8cd Mon Sep 17 00:00:00 2001 From: Adam Rauch Date: Fri, 1 May 2026 07:57:55 -0700 Subject: [PATCH 09/12] Junit test. Support index convert followed by drop. Fix filtering in Step #2. Generics in Junit management. --- .../labkey/core/junit/JunitController.java | 52 ++-- .../org/labkey/core/junit/JunitManager.java | 8 +- .../org/labkey/devtools/DevtoolsModule.java | 15 +- .../org/labkey/devtools/ToolsController.java | 255 ++++++++++-------- 4 files changed, 187 insertions(+), 143 deletions(-) diff --git a/core/src/org/labkey/core/junit/JunitController.java b/core/src/org/labkey/core/junit/JunitController.java index 4fec3407617..70645b658a9 100644 --- a/core/src/org/labkey/core/junit/JunitController.java +++ b/core/src/org/labkey/core/junit/JunitController.java @@ -91,10 +91,10 @@ public JunitController() public static class JUnitViewBean { - public final Map> testCases; + public final Map>> testCases; public final boolean showRunButtons; - JUnitViewBean(Map> tests, boolean buttons) + JUnitViewBean(Map>> tests, boolean buttons) { this.testCases = tests; this.showRunButtons = buttons; @@ -186,7 +186,7 @@ else if (!StringUtils.isEmpty(form.getTestCase())) // To allow performance tests to be selected change the scope to PERFORMANCE. form._scope = TestWhen.When.PERFORMANCE; - for (List list : JunitManager.getTestCases().values()) + for (List> list : JunitManager.getTestCases().values()) { list.stream() .filter((test) -> test.getName().equals(form.getTestCase())) @@ -237,14 +237,14 @@ public ModelAndView getView(TestForm form, BindException errors) throws Exceptio } else { - List testClasses = getTestClasses(form); + List> testClasses = getTestClasses(form); TestContext.setTestContext(getViewContext().getRequest(), getUser()); getPageConfig().setTemplate(PageConfig.Template.Dialog); results = new LinkedList<>(); HttpServletResponse response = getViewContext().getResponse(); response.setContentType("text/plain"); - for (Class testClass : testClasses) + for (Class testClass : testClasses) { // show status. this also stops the tests if the client goes away. response.getWriter().println(testClass.getName()); @@ -260,13 +260,13 @@ public ModelAndView getView(TestForm form, BindException errors) throws Exceptio return view; } - private List getTestClasses(TestForm form) + private List> getTestClasses(TestForm form) { String module = form.getModule(); if (null != module) { - List moduleTests = JunitManager.getTestCases().get(module); + List> moduleTests = JunitManager.getTestCases().get(module); if (moduleTests == null || moduleTests.isEmpty()) { throw new NotFoundException("No tests for module: " + module); @@ -274,18 +274,18 @@ private List getTestClasses(TestForm form) return moduleTests; } - Set allTestClasses = new LinkedHashSet<>(); + Set> allTestClasses = new LinkedHashSet<>(); JunitManager.getTestCases() - .values() - .forEach(moduleTests -> allTestClasses.addAll(moduleTests)); + .values() + .forEach(allTestClasses::addAll); final String testCase = form.getTestCase(); if (!StringUtils.isBlank(testCase)) { - Class specifiedTest = allTestClasses.parallelStream() - .filter(clazz -> testCase.equals(clazz.getName())) - .findAny() - .orElseThrow(() -> new NotFoundException("No such test: " + testCase)); + Class specifiedTest = allTestClasses.parallelStream() + .filter(clazz -> testCase.equals(clazz.getName())) + .findAny() + .orElseThrow(() -> new NotFoundException("No such test: " + testCase)); return Collections.singletonList(specifiedTest); } @@ -302,23 +302,23 @@ public void addNavTrail(NavTree root) @RequiresSiteAdmin public static class Run2Action extends StatusReportingRunnableAction { - private List getTestClasses(TestForm form) + private List> getTestClasses(TestForm form) { - Map> allTestClasses = JunitManager.getTestCases(); + Map>> allTestClasses = JunitManager.getTestCases(); String module = form.getModule(); if (null != module) return JunitManager.getTestCases().get(module); - List testClasses = new LinkedList<>(); + List> testClasses = new LinkedList<>(); String testCase = form.getTestCase(); if (null == testCase || !testCase.isEmpty()) { for (String m : allTestClasses.keySet()) { - for (Class clazz : allTestClasses.get(m)) + for (Class clazz : allTestClasses.get(m)) { // include test if (null == testCase || testCase.equals(clazz.getName())) @@ -333,7 +333,7 @@ private List getTestClasses(TestForm form) @Override protected StatusReportingRunnable newStatusReportingRunnable() { - List testClasses = getTestClasses(new TestForm()); + List> testClasses = getTestClasses(new TestForm()); List results = new LinkedList<>(); return new JunitRunnable(testClasses, results, getViewContext().getRequest(), getUser()); } @@ -344,11 +344,11 @@ private static class JunitRunnable implements StatusReportingRunnable { private final StatusAppender _appender; private final Logger _log; - private final List _testClasses; + private final List> _testClasses; private final List _results; private volatile boolean _running = true; - private JunitRunnable(List testClasses, List results, HttpServletRequest request, User user) // TODO: Make this a Callable instead? + private JunitRunnable(List> testClasses, List results, HttpServletRequest request, User user) // TODO: Make this a Callable instead? { _testClasses = testClasses; _results = results; @@ -395,14 +395,14 @@ public static class Testlist extends ReadOnlyApiAction @Override public ApiResponse execute(Object o, BindException errors) { - Map> testCases = JunitManager.getTestCases(); + Map>> testCases = JunitManager.getTestCases(); Map>> values = new HashMap<>(); for (String module : testCases.keySet()) { List> tests = new ArrayList<>(); values.put("Remote " + module, tests); - for (Class clazz : testCases.get(module)) + for (Class clazz : testCases.get(module)) { int timeout = TestTimeout.DEFAULT; // Check if the test has requested a non-standard timeout @@ -545,13 +545,13 @@ public void setWhen(String when) } - private static Class findTestClass(String testCase) + private static Class findTestClass(String testCase) { - Map> testCases = JunitManager.getTestCases(); + Map>> testCases = JunitManager.getTestCases(); for (String module : testCases.keySet()) { - for (Class clazz : testCases.get(module)) + for (Class clazz : testCases.get(module)) { if (null == testCase || testCase.equals(clazz.getName())) return clazz; diff --git a/core/src/org/labkey/core/junit/JunitManager.java b/core/src/org/labkey/core/junit/JunitManager.java index 8a8801a6271..4d71cd670f0 100644 --- a/core/src/org/labkey/core/junit/JunitManager.java +++ b/core/src/org/labkey/core/junit/JunitManager.java @@ -35,20 +35,20 @@ */ public class JunitManager { - public static synchronized Map> getTestCases() + public static synchronized Map>> getTestCases() { - Map> testCases = new TreeMap<>(); + Map>> testCases = new TreeMap<>(); for (Module module : ModuleLoader.getInstance().getModules()) { - Set moduleClazzes = new HashSet<>(); + Set> moduleClazzes = new HashSet<>(); module.getIntegrationTestFactories().forEach(f -> moduleClazzes.add(f.get())); moduleClazzes.addAll(module.getUnitTests()); if (!moduleClazzes.isEmpty()) { - List moduleClazzList = new ArrayList<>(moduleClazzes); + List> moduleClazzList = new ArrayList<>(moduleClazzes); moduleClazzList.sort(Comparator.comparing(Class::getName)); testCases.put(module.getName(), Collections.unmodifiableList(moduleClazzList)); diff --git a/devtools/src/org/labkey/devtools/DevtoolsModule.java b/devtools/src/org/labkey/devtools/DevtoolsModule.java index dcedc1b7df3..bdddd116dac 100644 --- a/devtools/src/org/labkey/devtools/DevtoolsModule.java +++ b/devtools/src/org/labkey/devtools/DevtoolsModule.java @@ -29,8 +29,11 @@ import org.labkey.devtools.authentication.TestSsoController; import org.labkey.devtools.authentication.TestSsoProvider; +import java.util.ArrayList; import java.util.Collection; import java.util.Collections; +import java.util.List; +import java.util.Set; import java.util.function.Supplier; public class DevtoolsModule extends CodeOnlyModule @@ -78,6 +81,16 @@ public void doStartup(ModuleContext moduleContext) @Override public @NotNull Collection>> getIntegrationTestFactories() { - return Collections.singletonList(new JspTestCase("/org/labkey/devtools/test/JspTestCaseTest.jsp")); + List>> list = new ArrayList<>(super.getIntegrationTestFactories()); + list.add(new JspTestCase("/org/labkey/devtools/test/JspTestCaseTest.jsp")); + return list; + } + + @Override + public @NotNull Set> getIntegrationTests() + { + return Set.of( + ToolsController.TestCase.class + ); } } \ No newline at end of file diff --git a/devtools/src/org/labkey/devtools/ToolsController.java b/devtools/src/org/labkey/devtools/ToolsController.java index ba3d26c19a0..68310b16daf 100644 --- a/devtools/src/org/labkey/devtools/ToolsController.java +++ b/devtools/src/org/labkey/devtools/ToolsController.java @@ -6,6 +6,8 @@ import org.apache.logging.log4j.Logger; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; +import org.junit.Assert; +import org.junit.Test; import org.labkey.api.action.FormHandlerAction; import org.labkey.api.action.FormViewAction; import org.labkey.api.action.SimpleErrorView; @@ -33,6 +35,7 @@ import org.labkey.api.reader.Readers; import org.labkey.api.security.RequiresPermission; import org.labkey.api.security.permissions.AdminPermission; +import org.labkey.api.test.TestWhen; import org.labkey.api.util.BaseScanner.Handler; import org.labkey.api.util.ButtonBuilder; import org.labkey.api.util.DOM; @@ -748,8 +751,6 @@ public record IndexChange(TableInfo table, IndexDefinition index, ChangeType typ @RequiresPermission(AdminPermission.class) public class OverlappingIndicesAction extends FormViewAction { - private final String delim = Character.toString(31); // Non-printing character that's very unlikely to be in a column name - @Override public ModelAndView getView(OverlappingIndicesForm form, boolean reshow, BindException errors) { @@ -761,7 +762,7 @@ public ModelAndView getView(OverlappingIndicesForm form, boolean reshow, BindExc url.deleteParameter("clearCaches"); } - MultiValuedMap multiMap = getOverlappingIndices(form); + MultiValuedMap multiMap = new OverlappingIndicesAnalyzer().getOverlappingIndices(form.schemaName()); return new VBox( new HtmlView(DOM.createHtmlFragment( @@ -802,7 +803,7 @@ public void addNavTrail(NavTree root) @Override public boolean handlePost(OverlappingIndicesForm form, BindException errors) { - MultiValuedMap multiMap = getOverlappingIndices(form); + MultiValuedMap multiMap = new OverlappingIndicesAnalyzer().getOverlappingIndices(form.schemaName()); try { @@ -840,113 +841,6 @@ public URLHelper getSuccessURL(OverlappingIndicesForm form) return new ActionURL(OverlappingIndicesAction.class, getContainer()); } - private MultiValuedMap getOverlappingIndices(OverlappingIndicesForm form) - { - MultiValuedMap multiMap = new ArrayListValuedLinkedHashMap<>(); - DbScope scope = DbScope.getLabKeyScope(); - - ModuleLoader.getInstance().getModules().stream() - .flatMap(module -> module.getSchemaNames().stream().filter(name -> !module.getProvisionedSchemaNames().contains(name))) - .filter(schemaName -> form.schemaName() == null || schemaName.equalsIgnoreCase(form.schemaName())) - .sorted(String.CASE_INSENSITIVE_ORDER) - .map(name -> scope.getSchema(name, DbSchemaType.Module)) - .flatMap(schema -> schema.getTableNames().stream().map(schema::getTable)) - .forEach(table -> { - var indices = new LinkedHashSet<>(table.getAllIndices()); - var changes = new LinkedList(); - Set droppedIndices = new HashSet<>(); - - // Step #1: Find the PK (if present), and drop all indices that overlap with a smaller or equal - // column set. The third step below would take care of overlapping non-unique indices, so this is - // mainly to drop unique indices that overlap with the PK. - indices.stream() - .filter(ix -> ix.indexType() == Primary) - .findFirst() - .ifPresent(pk -> indices.stream() - .filter(index -> overlaps(pk, index)) - .forEach(index -> { - changes.add(new IndexChange(table, index, ChangeType.Drop, getDropDescription(index, pk))); - droppedIndices.add(index); - }) - ); - - Set convertedUniqueIndices = new HashSet<>(); - - // Step #2: For each unique index, switch it to a non-unique index if there's any UQ or PK that - // overlaps with a smaller or equal column set. - streamIndices(indices, droppedIndices) - .filter(index -> index.indexType() == Unique) - .forEach(uq -> streamIndices(indices, convertedUniqueIndices) - .filter(index -> index.indexType() == Primary || index.indexType() == Unique) - .filter(index -> overlaps(uq, index)) - .findFirst() - .ifPresent(index -> { - changes.add(new IndexChange(table, uq, ChangeType.Convert, String.format("Converting %s from unique to non-unique index because %s overlaps it with a smaller column set", uq.display(), index.display()))); - convertedUniqueIndices.add(uq); - }) - ); - - // Step #3: For each index (unique or non-unique), delete all other non-unique indices that overlap - // with a smaller or equal column set. - streamIndices(indices, droppedIndices) - .filter(index -> index.indexType() != Primary) - .forEach(index -> streamIndices(indices, droppedIndices) - .filter(ix -> ix.indexType() == NonUnique || convertedUniqueIndices.contains(ix)) - .filter(ix -> overlaps(index, ix)) - .forEach(ix -> { - String description = getDropDescription(ix, index); - if (convertedUniqueIndices.contains(ix)) - { - throw new IllegalStateException("Dropping an index that's been converted to non-unique is not yet supported"); - } - changes.add(new IndexChange(table, ix, ChangeType.Drop, description)); - droppedIndices.add(ix); - }) - ); - - changes.forEach(change -> multiMap.put(change.table().getSchema().getName(), change)); - }); - - return multiMap; - - // TODO: junit test - } - - // Helper that filters out the dropped indices - private Stream streamIndices(Set indices, Set droppedIndices) - { - return indices.stream().filter(index -> !droppedIndices.contains(index)); - } - - private String getDropDescription(IndexDefinition dropIndex, IndexDefinition otherIndex) - { - String warning = dropIndex.indexType() == otherIndex.indexType() && dropIndex.columns().size() == otherIndex.columns().size() ? - ". Note: You may want to drop " + otherIndex.display() + " instead!!" : ""; - return String.format("Dropping %s because it overlaps with %s", dropIndex.display(), otherIndex.display()) + warning; - } - - // Returns true if index2 has an overlapping column set that's equal to or smaller than index1's - private boolean overlaps(IndexDefinition index1, IndexDefinition index2) - { - boolean ret = false; - - if (!index1.equals(index2) && Objects.equals(index1.filterCondition(), index2.filterCondition())) - { - String key1 = getKey(index1.columns()); - String key2 = getKey(index2.columns()); - ret = key1.startsWith(key2); - } - - return ret; - } - - private String getKey(List cols) - { - return cols.stream() - .map(col -> col.getName().toLowerCase()) - .collect(Collectors.joining(delim)) + delim; - } - private static class WriterContext { private final File _scriptDirectory; @@ -1045,7 +939,144 @@ private void closeAllContexts() } } - protected enum ChangeType + private static class OverlappingIndicesAnalyzer + { + private final String delim = Character.toString(31); // Non-printing character that's very unlikely to be in a column name + + private MultiValuedMap getOverlappingIndices(@Nullable String schemaName) + { + MultiValuedMap multiMap = new ArrayListValuedLinkedHashMap<>(); + DbScope scope = DbScope.getLabKeyScope(); + + ModuleLoader.getInstance().getModules().stream() + .flatMap(module -> module.getSchemaNames().stream().filter(name -> !module.getProvisionedSchemaNames().contains(name))) + .filter(name -> schemaName == null || name.equalsIgnoreCase(schemaName)) + .sorted(String.CASE_INSENSITIVE_ORDER) + .map(name -> scope.getSchema(name, DbSchemaType.Module)) + .flatMap(schema -> schema.getTableNames().stream().map(schema::getTable)) + .forEach(table -> { + var indices = new LinkedHashSet<>(table.getAllIndices()); + var changes = new LinkedList(); + Set droppedIndices = new HashSet<>(); + + // Step #1: Find the PK (if present), and drop all indices that overlap with a smaller or equal + // column set. The third step below would take care of overlapping non-unique indices, so this is + // mainly to drop unique indices that overlap with the PK. + indices.stream() + .filter(ix -> ix.indexType() == Primary) + .findFirst() + .ifPresent(pk -> indices.stream() + .filter(index -> overlaps(pk, index)) + .forEach(index -> { + changes.add(new IndexChange(table, index, ChangeType.Drop, getDropDescription(index, pk))); + droppedIndices.add(index); + }) + ); + + Set convertedUniqueIndices = new HashSet<>(); + + // Step #2: For each unique index, switch it to a non-unique index if there's any UQ or PK that + // overlaps with a smaller or equal column set. + streamIndices(indices, droppedIndices) + .filter(index -> index.indexType() == Unique) + .filter(index -> !convertedUniqueIndices.contains(index)) + .forEach(uq -> streamIndices(indices, droppedIndices) + .filter(index -> index.indexType() == Primary || index.indexType() == Unique) + .filter(index -> !convertedUniqueIndices.contains(index)) + .filter(index -> overlaps(uq, index)) + .findFirst() + .ifPresent(index -> { + changes.add(new IndexChange(table, uq, ChangeType.Convert, String.format("Converting %s from unique to non-unique index because %s overlaps it with a smaller column set", uq.display(), index.display()))); + convertedUniqueIndices.add(uq); + }) + ); + + // Step #3: For each index (unique or non-unique), delete all other non-unique indices that overlap + // with a smaller or equal column set. + streamIndices(indices, droppedIndices) + .filter(index -> index.indexType() != Primary) + .forEach(index -> streamIndices(indices, droppedIndices) + .filter(ix -> ix.indexType() == NonUnique || convertedUniqueIndices.contains(ix)) + .filter(ix -> overlaps(index, ix)) + .forEach(ix -> { + String description = getDropDescription(ix, index); + if (convertedUniqueIndices.contains(ix)) + { + // Index was converted to non-unique but now needs to be dropped. Adjust changes, description, etc. + IndexChange convert = changes.stream() + .filter(change -> change.index().equals(ix)) + .findFirst() + .orElseThrow(); + description = description + (description.contains("!!") ? "" : ".") + " Prior to this drop, the index was converted: " + convert.description(); + changes.remove(convert); + convertedUniqueIndices.remove(ix); + } + changes.add(new IndexChange(table, ix, ChangeType.Drop, description)); + droppedIndices.add(ix); + }) + ); + + changes.forEach(change -> multiMap.put(change.table().getSchema().getName(), change)); + }); + + return multiMap; + } + + // Helper that filters out the dropped indices + private Stream streamIndices(Set indices, Set droppedIndices) + { + return indices.stream().filter(index -> !droppedIndices.contains(index)); + } + + private String getDropDescription(IndexDefinition dropIndex, IndexDefinition otherIndex) + { + String warning = dropIndex.indexType() == otherIndex.indexType() && dropIndex.columns().size() == otherIndex.columns().size() ? + ". Note: You may want to drop " + otherIndex.display() + " instead!!" : ""; + return String.format("Dropping %s because it overlaps with %s", dropIndex.display(), otherIndex.display()) + warning; + } + + // Returns true if index2 has an overlapping column set that's equal to or smaller than index1's + private boolean overlaps(IndexDefinition index1, IndexDefinition index2) + { + boolean ret = false; + + if (!index1.equals(index2) && Objects.equals(index1.filterCondition(), index2.filterCondition())) + { + String key1 = getKey(index1.columns()); + String key2 = getKey(index2.columns()); + ret = key1.startsWith(key2); + } + + return ret; + } + + private String getKey(List cols) + { + return cols.stream() + .map(col -> col.getName().toLowerCase()) + .collect(Collectors.joining(delim)) + delim; + } + } + + @TestWhen(TestWhen.When.BVT) + public static class TestCase extends Assert + { + @Test + public void testOverlappingIndices() + { + if (DbScope.getLabKeyScope().getSqlDialect().isPostgreSQL()) + { + var map = new OverlappingIndicesAnalyzer().getOverlappingIndices(null); + var keys = map.keys(); + if (!keys.isEmpty()) + fail(StringUtilsLabKey.pluralize(keys.size(), "redundant index", "redundant indices") + " detected: " + map); + } + } + + // TODO: Test convert -> drop sequence, etc. + } + + private enum ChangeType { Drop { From 715b5aa70df7f82a4df9cf8a012d96fb1f49f85f Mon Sep 17 00:00:00 2001 From: Adam Rauch Date: Fri, 1 May 2026 08:10:29 -0700 Subject: [PATCH 10/12] Address minor Claude feedback --- devtools/src/org/labkey/devtools/ToolsController.java | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/devtools/src/org/labkey/devtools/ToolsController.java b/devtools/src/org/labkey/devtools/ToolsController.java index 68310b16daf..dd5aa182695 100644 --- a/devtools/src/org/labkey/devtools/ToolsController.java +++ b/devtools/src/org/labkey/devtools/ToolsController.java @@ -979,7 +979,6 @@ private MultiValuedMap getOverlappingIndices(@Nullable Stri // overlaps with a smaller or equal column set. streamIndices(indices, droppedIndices) .filter(index -> index.indexType() == Unique) - .filter(index -> !convertedUniqueIndices.contains(index)) .forEach(uq -> streamIndices(indices, droppedIndices) .filter(index -> index.indexType() == Primary || index.indexType() == Unique) .filter(index -> !convertedUniqueIndices.contains(index)) @@ -1007,7 +1006,7 @@ private MultiValuedMap getOverlappingIndices(@Nullable Stri .filter(change -> change.index().equals(ix)) .findFirst() .orElseThrow(); - description = description + (description.contains("!!") ? "" : ".") + " Prior to this drop, the index was converted: " + convert.description(); + description = description + (!description.endsWith(".") ? "." : "") + " Prior to this drop, the index was converted: " + convert.description(); changes.remove(convert); convertedUniqueIndices.remove(ix); } @@ -1031,7 +1030,7 @@ private Stream streamIndices(Set indices, Set< private String getDropDescription(IndexDefinition dropIndex, IndexDefinition otherIndex) { String warning = dropIndex.indexType() == otherIndex.indexType() && dropIndex.columns().size() == otherIndex.columns().size() ? - ". Note: You may want to drop " + otherIndex.display() + " instead!!" : ""; + ". Note: You may want to drop " + otherIndex.display() + " instead." : ""; return String.format("Dropping %s because it overlaps with %s", dropIndex.display(), otherIndex.display()) + warning; } @@ -1136,7 +1135,7 @@ private record IndexKey(String schemaName, String indexName) {} private static @Nullable String getConstraintForIndex(String schemaName, String indexName) { Cache> sharedCache = CacheManager.getSharedCache(); - var constraintMap = sharedCache.get("ConstraintForIndexMap", null, (_, _) -> Collections.unmodifiableMap( + var constraintMap = sharedCache.get(OverlappingIndicesAction.class.getName() + "/ConstraintForIndexMap", null, (_, _) -> Collections.unmodifiableMap( new SqlSelector(DbScope.getLabKeyScope(), new SQLFragment(""" SELECT NspName AS SchemaName, RelName AS IndexName, ConName AS ConstraintName FROM pg_index i INNER JOIN pg_class cl ON cl.oid = i.indexrelid From 81d6b67a1fd4b56c3e8a11d160b91ec101e4bd78 Mon Sep 17 00:00:00 2001 From: Adam Rauch Date: Fri, 1 May 2026 12:05:28 -0700 Subject: [PATCH 11/12] Skip junit on SQL Server. Small refactor. --- .../org/labkey/devtools/ToolsController.java | 67 ++++++++++++------- 1 file changed, 42 insertions(+), 25 deletions(-) diff --git a/devtools/src/org/labkey/devtools/ToolsController.java b/devtools/src/org/labkey/devtools/ToolsController.java index dd5aa182695..c52f68361fd 100644 --- a/devtools/src/org/labkey/devtools/ToolsController.java +++ b/devtools/src/org/labkey/devtools/ToolsController.java @@ -7,6 +7,8 @@ import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; import org.junit.Assert; +import org.junit.Assume; +import org.junit.BeforeClass; import org.junit.Test; import org.labkey.api.action.FormHandlerAction; import org.labkey.api.action.FormViewAction; @@ -87,6 +89,7 @@ import java.util.Objects; import java.util.Set; import java.util.TreeSet; +import java.util.function.Consumer; import java.util.regex.Matcher; import java.util.regex.Pattern; import java.util.stream.Collectors; @@ -762,7 +765,7 @@ public ModelAndView getView(OverlappingIndicesForm form, boolean reshow, BindExc url.deleteParameter("clearCaches"); } - MultiValuedMap multiMap = new OverlappingIndicesAnalyzer().getOverlappingIndices(form.schemaName()); + MultiValuedMap multiMap = new OverlappingIndicesAnalyzer().getChanges(form.schemaName()); return new VBox( new HtmlView(DOM.createHtmlFragment( @@ -803,7 +806,7 @@ public void addNavTrail(NavTree root) @Override public boolean handlePost(OverlappingIndicesForm form, BindException errors) { - MultiValuedMap multiMap = new OverlappingIndicesAnalyzer().getOverlappingIndices(form.schemaName()); + MultiValuedMap multiMap = new OverlappingIndicesAnalyzer().getChanges(form.schemaName()); try { @@ -943,18 +946,22 @@ private static class OverlappingIndicesAnalyzer { private final String delim = Character.toString(31); // Non-printing character that's very unlikely to be in a column name - private MultiValuedMap getOverlappingIndices(@Nullable String schemaName) + private void enumerateTables(@Nullable String schemaName, Consumer tableConsumer) + { + ModuleLoader.getInstance().getModules().stream() + .flatMap(module -> module.getSchemaNames().stream().filter(name -> !module.getProvisionedSchemaNames().contains(name))) + .filter(name -> schemaName == null || name.equalsIgnoreCase(schemaName)) + .sorted(String.CASE_INSENSITIVE_ORDER) + .map(name -> DbScope.getLabKeyScope().getSchema(name, DbSchemaType.Module)) + .flatMap(schema -> schema.getTableNames().stream().map(schema::getTable)) + .forEach(tableConsumer); + } + + private MultiValuedMap getChanges(@Nullable String schemaName) { MultiValuedMap multiMap = new ArrayListValuedLinkedHashMap<>(); - DbScope scope = DbScope.getLabKeyScope(); - ModuleLoader.getInstance().getModules().stream() - .flatMap(module -> module.getSchemaNames().stream().filter(name -> !module.getProvisionedSchemaNames().contains(name))) - .filter(name -> schemaName == null || name.equalsIgnoreCase(schemaName)) - .sorted(String.CASE_INSENSITIVE_ORDER) - .map(name -> scope.getSchema(name, DbSchemaType.Module)) - .flatMap(schema -> schema.getTableNames().stream().map(schema::getTable)) - .forEach(table -> { + enumerateTables(schemaName, table -> { var indices = new LinkedHashSet<>(table.getAllIndices()); var changes = new LinkedList(); Set droppedIndices = new HashSet<>(); @@ -966,7 +973,7 @@ private MultiValuedMap getOverlappingIndices(@Nullable Stri .filter(ix -> ix.indexType() == Primary) .findFirst() .ifPresent(pk -> indices.stream() - .filter(index -> overlaps(pk, index)) + .filter(index -> isOverlap(pk, index)) .forEach(index -> { changes.add(new IndexChange(table, index, ChangeType.Drop, getDropDescription(index, pk))); droppedIndices.add(index); @@ -982,7 +989,7 @@ private MultiValuedMap getOverlappingIndices(@Nullable Stri .forEach(uq -> streamIndices(indices, droppedIndices) .filter(index -> index.indexType() == Primary || index.indexType() == Unique) .filter(index -> !convertedUniqueIndices.contains(index)) - .filter(index -> overlaps(uq, index)) + .filter(index -> isOverlap(uq, index)) .findFirst() .ifPresent(index -> { changes.add(new IndexChange(table, uq, ChangeType.Convert, String.format("Converting %s from unique to non-unique index because %s overlaps it with a smaller column set", uq.display(), index.display()))); @@ -996,7 +1003,7 @@ private MultiValuedMap getOverlappingIndices(@Nullable Stri .filter(index -> index.indexType() != Primary) .forEach(index -> streamIndices(indices, droppedIndices) .filter(ix -> ix.indexType() == NonUnique || convertedUniqueIndices.contains(ix)) - .filter(ix -> overlaps(index, ix)) + .filter(ix -> isOverlap(index, ix)) .forEach(ix -> { String description = getDropDescription(ix, index); if (convertedUniqueIndices.contains(ix)) @@ -1035,11 +1042,11 @@ private String getDropDescription(IndexDefinition dropIndex, IndexDefinition oth } // Returns true if index2 has an overlapping column set that's equal to or smaller than index1's - private boolean overlaps(IndexDefinition index1, IndexDefinition index2) + private boolean isSimpleOverlap(IndexDefinition index1, IndexDefinition index2) { boolean ret = false; - if (!index1.equals(index2) && Objects.equals(index1.filterCondition(), index2.filterCondition())) + if (!index1.equals(index2)) { String key1 = getKey(index1.columns()); String key2 = getKey(index2.columns()); @@ -1049,6 +1056,12 @@ private boolean overlaps(IndexDefinition index1, IndexDefinition index2) return ret; } + // Returns true if index2 overlaps index1, and they have the same filter condition + private boolean isOverlap(IndexDefinition index1, IndexDefinition index2) + { + return Objects.equals(index1.filterCondition(), index2.filterCondition()) && isSimpleOverlap(index1, index2); + } + private String getKey(List cols) { return cols.stream() @@ -1060,19 +1073,23 @@ private String getKey(List cols) @TestWhen(TestWhen.When.BVT) public static class TestCase extends Assert { + @BeforeClass + public static void checkDatabase() + { + Assume.assumeTrue("Skipping because this server is not running on PostgreSQL", DbScope.getLabKeyScope().getSqlDialect().isPostgreSQL()); + } + @Test public void testOverlappingIndices() { - if (DbScope.getLabKeyScope().getSqlDialect().isPostgreSQL()) - { - var map = new OverlappingIndicesAnalyzer().getOverlappingIndices(null); - var keys = map.keys(); - if (!keys.isEmpty()) - fail(StringUtilsLabKey.pluralize(keys.size(), "redundant index", "redundant indices") + " detected: " + map); - } + var map = new OverlappingIndicesAnalyzer().getChanges(null); + var keys = map.keys(); + if (!keys.isEmpty()) + fail(StringUtilsLabKey.pluralize(keys.size(), "redundant index", "redundant indices") + " detected: " + map); } // TODO: Test convert -> drop sequence, etc. + // TODO: Display all overlapping indices that don't require changes } private enum ChangeType @@ -1119,7 +1136,7 @@ final void writeScript(Writer writer, IndexChange change) throws IOException IndexDefinition index = change.index(); if (index.indexType() == Primary) - throw new IllegalStateException("Should not be trying to modify a PK! (" + change + ")"); + throw new IllegalStateException("Should not modify a PK! (" + change + ")"); writeScript(writer, change, table.getSchema().getName(), table.getName(), index); } @@ -1129,9 +1146,9 @@ final void writeScript(Writer writer, IndexChange change) throws IOException private record IndexKey(String schemaName, String indexName) {} + // If this is a unique index associated with a constraint, return that constraint name. Otherwise, return null. // Most, but not all, unique indices are created by adding a unique constraint; in those cases, we need to drop // the associated constraint. However, for explicitly created unique indices, we need to drop the index instead. - // If this is a unique index associated with a constraint, return that constraint name. Otherwise, return null. private static @Nullable String getConstraintForIndex(String schemaName, String indexName) { Cache> sharedCache = CacheManager.getSharedCache(); From d2ef1da7130f4a0bfd11811bdd9c743a1a16c60d Mon Sep 17 00:00:00 2001 From: labkey-jeckels Date: Fri, 1 May 2026 17:24:04 -0700 Subject: [PATCH 12/12] Fix issue with PK vs narrower unique index --- .../org/labkey/devtools/ToolsController.java | 287 +++++++++++++----- 1 file changed, 212 insertions(+), 75 deletions(-) diff --git a/devtools/src/org/labkey/devtools/ToolsController.java b/devtools/src/org/labkey/devtools/ToolsController.java index c52f68361fd..7e700844f5b 100644 --- a/devtools/src/org/labkey/devtools/ToolsController.java +++ b/devtools/src/org/labkey/devtools/ToolsController.java @@ -2,13 +2,13 @@ import org.apache.commons.collections4.MultiValuedMap; import org.apache.commons.collections4.multimap.ArrayListValuedLinkedHashMap; +import org.labkey.api.data.JdbcType; import org.apache.commons.lang3.StringUtils; import org.apache.logging.log4j.Logger; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; import org.junit.Assert; import org.junit.Assume; -import org.junit.BeforeClass; import org.junit.Test; import org.labkey.api.action.FormHandlerAction; import org.labkey.api.action.FormViewAction; @@ -49,7 +49,6 @@ import org.labkey.api.util.PageFlowUtil; import org.labkey.api.util.StringUtilsLabKey; import org.labkey.api.util.URLHelper; -import org.labkey.api.util.logging.LogHelper; import org.labkey.api.vcs.Vcs; import org.labkey.api.vcs.VcsService; import org.labkey.api.view.ActionURL; @@ -79,6 +78,8 @@ import java.sql.SQLException; import java.util.Collection; import java.util.Collections; +import java.util.ArrayList; +import java.util.Arrays; import java.util.Comparator; import java.util.HashMap; import java.util.HashSet; @@ -106,7 +107,6 @@ public class ToolsController extends SpringActionController { - private static final Logger LOG = LogHelper.getLogger(ToolsController.class, "Output from various tools"); private static final ActionResolver RESOLVER = new DefaultActionResolver(ToolsController.class); public static final String NAME = "tools"; @@ -895,7 +895,7 @@ public void close() throws IOException private WriterContext getWriterContext(String schemaName) throws IOException { - return _writerContextMap.computeIfAbsent(schemaName, n -> { + return _writerContextMap.computeIfAbsent(schemaName, _ -> { DbSchema schema = DbSchema.get(schemaName, DbSchemaType.Module); Module module = schema.getModule(); @@ -960,72 +960,77 @@ private void enumerateTables(@Nullable String schemaName, Consumer ta private MultiValuedMap getChanges(@Nullable String schemaName) { MultiValuedMap multiMap = new ArrayListValuedLinkedHashMap<>(); + enumerateTables(schemaName, table -> + analyzeTable(table, new LinkedHashSet<>(table.getAllIndices())) + .forEach(change -> multiMap.put(change.table().getSchema().getName(), change))); + return multiMap; + } - enumerateTables(schemaName, table -> { - var indices = new LinkedHashSet<>(table.getAllIndices()); - var changes = new LinkedList(); - Set droppedIndices = new HashSet<>(); - - // Step #1: Find the PK (if present), and drop all indices that overlap with a smaller or equal - // column set. The third step below would take care of overlapping non-unique indices, so this is - // mainly to drop unique indices that overlap with the PK. - indices.stream() - .filter(ix -> ix.indexType() == Primary) + // Package-visible for testing. Pass null for table only in unit tests that don't inspect change.table(). + List analyzeTable(@Nullable TableInfo table, LinkedHashSet indices) + { + var changes = new LinkedList(); + Set droppedIndices = new HashSet<>(); + + // Step #1: Find the PK (if present), and drop non-unique indices whose columns are a prefix of the + // PK, plus unique indices that cover the exact same column set as the PK. A unique index with FEWER + // columns than the PK enforces a strictly stronger uniqueness guarantee (the PK cannot replace it), + // so it is left for Step #2 to evaluate. + indices.stream() + .filter(ix -> ix.indexType() == Primary) + .findFirst() + .ifPresent(pk -> indices.stream() + .filter(index -> isOverlap(pk, index)) + .filter(index -> index.indexType() != Unique || index.columns().size() == pk.columns().size()) + .forEach(index -> { + changes.add(new IndexChange(table, index, ChangeType.Drop, getDropDescription(index, pk))); + droppedIndices.add(index); + }) + ); + + Set convertedUniqueIndices = new HashSet<>(); + + // Step #2: For each unique index, switch it to a non-unique index if there's any UQ or PK that + // overlaps with a smaller or equal column set. + streamIndices(indices, droppedIndices) + .filter(index -> index.indexType() == Unique) + .forEach(uq -> streamIndices(indices, droppedIndices) + .filter(index -> index.indexType() == Primary || index.indexType() == Unique) + .filter(index -> !convertedUniqueIndices.contains(index)) + .filter(index -> isOverlap(uq, index)) .findFirst() - .ifPresent(pk -> indices.stream() - .filter(index -> isOverlap(pk, index)) - .forEach(index -> { - changes.add(new IndexChange(table, index, ChangeType.Drop, getDropDescription(index, pk))); - droppedIndices.add(index); - }) - ); - - Set convertedUniqueIndices = new HashSet<>(); - - // Step #2: For each unique index, switch it to a non-unique index if there's any UQ or PK that - // overlaps with a smaller or equal column set. - streamIndices(indices, droppedIndices) - .filter(index -> index.indexType() == Unique) - .forEach(uq -> streamIndices(indices, droppedIndices) - .filter(index -> index.indexType() == Primary || index.indexType() == Unique) - .filter(index -> !convertedUniqueIndices.contains(index)) - .filter(index -> isOverlap(uq, index)) - .findFirst() - .ifPresent(index -> { - changes.add(new IndexChange(table, uq, ChangeType.Convert, String.format("Converting %s from unique to non-unique index because %s overlaps it with a smaller column set", uq.display(), index.display()))); - convertedUniqueIndices.add(uq); - }) - ); - - // Step #3: For each index (unique or non-unique), delete all other non-unique indices that overlap - // with a smaller or equal column set. - streamIndices(indices, droppedIndices) - .filter(index -> index.indexType() != Primary) - .forEach(index -> streamIndices(indices, droppedIndices) - .filter(ix -> ix.indexType() == NonUnique || convertedUniqueIndices.contains(ix)) - .filter(ix -> isOverlap(index, ix)) - .forEach(ix -> { - String description = getDropDescription(ix, index); - if (convertedUniqueIndices.contains(ix)) - { - // Index was converted to non-unique but now needs to be dropped. Adjust changes, description, etc. - IndexChange convert = changes.stream() - .filter(change -> change.index().equals(ix)) - .findFirst() - .orElseThrow(); - description = description + (!description.endsWith(".") ? "." : "") + " Prior to this drop, the index was converted: " + convert.description(); - changes.remove(convert); - convertedUniqueIndices.remove(ix); - } - changes.add(new IndexChange(table, ix, ChangeType.Drop, description)); - droppedIndices.add(ix); - }) - ); - - changes.forEach(change -> multiMap.put(change.table().getSchema().getName(), change)); - }); + .ifPresent(index -> { + changes.add(new IndexChange(table, uq, ChangeType.Convert, String.format("Converting %s from unique to non-unique index because %s overlaps it with a smaller column set", uq.display(), index.display()))); + convertedUniqueIndices.add(uq); + }) + ); + + // Step #3: For each index (unique or non-unique), delete all other non-unique indices that overlap + // with a smaller or equal column set. + streamIndices(indices, droppedIndices) + .filter(index -> index.indexType() != Primary) + .forEach(index -> streamIndices(indices, droppedIndices) + .filter(ix -> ix.indexType() == NonUnique || convertedUniqueIndices.contains(ix)) + .filter(ix -> isOverlap(index, ix)) + .forEach(ix -> { + String description = getDropDescription(ix, index); + if (convertedUniqueIndices.contains(ix)) + { + // Index was converted to non-unique but now needs to be dropped. Adjust changes, description, etc. + IndexChange convert = changes.stream() + .filter(change -> change.index().equals(ix)) + .findFirst() + .orElseThrow(); + description = description + (!description.endsWith(".") ? "." : "") + " Prior to this drop, the index was converted: " + convert.description(); + changes.remove(convert); + convertedUniqueIndices.remove(ix); + } + changes.add(new IndexChange(table, ix, ChangeType.Drop, description)); + droppedIndices.add(ix); + }) + ); - return multiMap; + return changes; } // Helper that filters out the dropped indices @@ -1073,23 +1078,155 @@ private String getKey(List cols) @TestWhen(TestWhen.When.BVT) public static class TestCase extends Assert { - @BeforeClass - public static void checkDatabase() - { - Assume.assumeTrue("Skipping because this server is not running on PostgreSQL", DbScope.getLabKeyScope().getSqlDialect().isPostgreSQL()); - } - @Test public void testOverlappingIndices() { + Assume.assumeTrue("Skipping because this server is not running on PostgreSQL", DbScope.getLabKeyScope().getSqlDialect().isPostgreSQL()); var map = new OverlappingIndicesAnalyzer().getChanges(null); var keys = map.keys(); if (!keys.isEmpty()) fail(StringUtilsLabKey.pluralize(keys.size(), "redundant index", "redundant indices") + " detected: " + map); } - // TODO: Test convert -> drop sequence, etc. - // TODO: Display all overlapping indices that don't require changes + // Helper to build an IndexDefinition with named columns in order + private static IndexDefinition idx(String name, TableInfo.IndexType type, String... columnNames) + { + var cols = Arrays.stream(columnNames) + .map(n -> (ColumnInfo) new BaseColumnInfo(n, JdbcType.VARCHAR)) + .collect(Collectors.toCollection(ArrayList::new)); + return new IndexDefinition(name, type, cols, null); + } + + private static List analyze(IndexDefinition... indexDefs) + { + return new OverlappingIndicesAnalyzer().analyzeTable(null, new LinkedHashSet<>(Arrays.asList(indexDefs))); + } + + @Test + public void testNonUniqueIndexIsRedundantWithPk() + { + // A non-unique index on (A) is redundant when the PK is on (A, B): the PK B-tree satisfies + // all the same prefix queries. This should be dropped. + var pk = idx("pk_ab", Primary, "A", "B"); + var ix = idx("ix_a", NonUnique, "A"); + + var changes = analyze(pk, ix); + + boolean ixDropped = changes.stream().anyMatch(c -> c.index().equals(ix) && c.type() == ChangeType.Drop); + assertTrue("ix_a (non-unique on A) should be dropped when PK is on (A, B)", ixDropped); + } + + @Test + public void testIdenticalUniqueIndexIsRedundantWithPk() + { + // A unique index on exactly the same columns as the PK is a true duplicate: the PK already + // enforces the same uniqueness guarantee, so the separate index should be removed. + var pk = idx("pk_ab", Primary, "A", "B"); + var uq = idx("uq_ab", Unique, "A", "B"); + + var changes = analyze(pk, uq); + + boolean uqActedOn = changes.stream() + .anyMatch(c -> c.index().equals(uq) && (c.type() == ChangeType.Drop || c.type() == ChangeType.Convert)); + assertTrue("uq_ab (unique on A, B) should be dropped or converted when PK is also on (A, B)", uqActedOn); + } + + @Test + public void testUniqueNotDroppedByLongerPk() + { + // PK(A,B,C) allows rows (A=1,B=1,C=1) and (A=1,B=1,C=2); Unique(A,B) does not. + // Dropping it would silently relax the uniqueness guarantee. + var pk = idx("pk_abc", Primary, "A", "B", "C"); + var uq = idx("uq_ab", Unique, "A", "B"); + + var changes = analyze(pk, uq); + + boolean uqDropped = changes.stream().anyMatch(c -> c.index().equals(uq) && c.type() == ChangeType.Drop); + assertFalse("uq_ab (unique on A,B) must not be dropped when pk is on (A,B,C). Changes: " + changes, uqDropped); + } + + @Test + public void testStep2UniqueConvertedWhenPrefixUniqueExists() + { + // If Unique(A) exists, the (A,B) pair is already guaranteed unique by the A constraint alone, + // so Unique(A,B) provides no additional uniqueness and should be converted to non-unique. + // The narrower Unique(A) must not be touched. + var uqA = idx("uq_a", Unique, "A"); + var uqAB = idx("uq_ab", Unique, "A", "B"); + + var changes = analyze(uqA, uqAB); + + boolean uqABConverted = changes.stream().anyMatch(c -> c.index().equals(uqAB) && c.type() == ChangeType.Convert); + boolean uqAConverted = changes.stream().anyMatch(c -> c.index().equals(uqA) && c.type() == ChangeType.Convert); + assertTrue("uq_ab (unique on A,B) should be converted when uq_a (unique on A) exists. Changes: " + changes, uqABConverted); + assertFalse("uq_a (unique on A) should not be converted. Changes: " + changes, uqAConverted); + } + + @Test + public void testStep3NonUniqueDroppedByWiderNonUnique() + { + // A B-tree index on (A,B,C) can serve all prefix queries on (A,B), making a separate + // NonUnique(A,B) index redundant. The narrower one should be dropped; the wider one kept. + var ixABC = idx("ix_abc", NonUnique, "A", "B", "C"); + var ixAB = idx("ix_ab", NonUnique, "A", "B"); + + var changes = analyze(ixABC, ixAB); + + boolean ixABDropped = changes.stream().anyMatch(c -> c.index().equals(ixAB) && c.type() == ChangeType.Drop); + boolean ixABCDropped = changes.stream().anyMatch(c -> c.index().equals(ixABC) && c.type() == ChangeType.Drop); + assertTrue("ix_ab (non-unique on A,B) should be dropped when ix_abc (non-unique on A,B,C) exists. Changes: " + changes, ixABDropped); + assertFalse("ix_abc (non-unique on A,B,C) should not be dropped. Changes: " + changes, ixABCDropped); + } + + @Test + public void testStep2And3ConvertThenDrop() + { + // Step 2 converts Unique(A,B) because Unique(A) makes its uniqueness redundant. + // Step 3 then drops the (now non-unique) Unique(A,B) because NonUnique(A,B,C) covers it. + // The final changes list must show a single Drop for uq_ab — no separate Convert entry. + var uqA = idx("uq_a", Unique, "A"); + var uqAB = idx("uq_ab", Unique, "A", "B"); + var ixABC = idx("ix_abc", NonUnique, "A", "B", "C"); + + var changes = analyze(uqA, uqAB, ixABC); + + boolean uqABDropped = changes.stream().anyMatch(c -> c.index().equals(uqAB) && c.type() == ChangeType.Drop); + boolean uqABConverted = changes.stream().anyMatch(c -> c.index().equals(uqAB) && c.type() == ChangeType.Convert); + assertTrue("uq_ab should appear as Drop (convert folded in). Changes: " + changes, uqABDropped); + assertFalse("uq_ab should not have a separate Convert entry. Changes: " + changes, uqABConverted); + } + + @Test + public void testNoChangesForDisjointIndices() + { + // Indices on completely different columns have no prefix relationship; nothing should change. + var ixA = idx("ix_a", NonUnique, "A"); + var ixB = idx("ix_b", NonUnique, "B"); + var uqC = idx("uq_c", Unique, "C"); + + var changes = analyze(ixA, ixB, uqC); + + assertTrue("Disjoint indices should produce no changes. Changes: " + changes, changes.isEmpty()); + } + + @Test + public void testFilteredIndexNotDroppedByFullIndex() + { + // A partial (filtered) index covers only a subset of rows. Even when its column set is a + // prefix of the PK, the filter condition means the two indices are not interchangeable. + var pk = idx("pk_ab", Primary, "A", "B"); + var cols = Arrays.stream(new String[]{"A"}) + .map(n -> (ColumnInfo) new BaseColumnInfo(n, JdbcType.VARCHAR)) + .collect(Collectors.toCollection(ArrayList::new)); + var filteredIx = new IndexDefinition("ix_a_partial", NonUnique, cols, "active = 1"); + + var changes = analyze(pk, filteredIx); + + boolean filteredDropped = changes.stream().anyMatch(c -> c.index().equals(filteredIx) && c.type() == ChangeType.Drop); + assertFalse( + "ix_a_partial (filtered non-unique on A) must not be dropped by pk_ab: different filter conditions. Changes: " + changes, + filteredDropped); + } } private enum ChangeType