Skip to content

Add InventoryFilter. Clear orphaned issues. #7165

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
labkey-adam merged 5 commits into from
Oct 31, 2025
Merged

Add InventoryFilter. Clear orphaned issues. #7165

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2fbcb73
Add InventoryFilter
labkey-adam Oct 30, 2025
2871d4b
Share more code across provisioned and extensible table filtering
labkey-adam Oct 30, 2025
85fd135
Delete orphaned issues rows
labkey-adam Oct 30, 2025
8c9b76c
Thousands separators in data class & sample type counts
labkey-adam Oct 30, 2025
b95845e
Delete RelatedIssues based on RelatedIssueId as well
labkey-adam Oct 31, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
135 changes: 97 additions & 38 deletions api/src/org/labkey/api/data/DatabaseMigrationService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,12 @@
import org.labkey.api.data.SimpleFilter.FilterClause;
import org.labkey.api.data.SimpleFilter.InClause;
import org.labkey.api.data.SimpleFilter.OrClause;
import org.labkey.api.data.SimpleFilter.SQLClause;
import org.labkey.api.query.FieldKey;
import org.labkey.api.query.SchemaKey;
import org.labkey.api.query.TableSorter;
import org.labkey.api.services.ServiceRegistry;
import org.labkey.api.util.ConfigurationException;
import org.labkey.api.util.GUID;
import org.labkey.api.util.logging.LogHelper;
import org.labkey.vfs.FileLike;
Expand All @@ -30,7 +32,7 @@ public interface DatabaseMigrationService
{
Logger LOG = LogHelper.getLogger(DatabaseMigrationService.class, "Information about database migration");

record DomainFilter(Set<GUID> containers, String column, FilterClause condition) {}
record DataFilter(Set<GUID> containers, String column, FilterClause condition) {}

static @NotNull DatabaseMigrationService get()
{
Expand Down Expand Up @@ -84,15 +86,25 @@ record Sequence(String schemaName, String tableName, String columnName, long las

List<TableInfo> getTablesToCopy();

// Create a filter clause that selects from all specified containers and (in some overrides) applies table-specific filters
FilterClause getTableFilter(TableInfo sourceTable, FieldKey containerFieldKey, Set<GUID> containers);

// Create a filter clause that selects from all specified containers
FilterClause getContainerClause(TableInfo sourceTable, FieldKey containerFieldKey, Set<GUID> containers);

// Return the FieldKey that can be used to filter this table by container. Special values SITE_WIDE_TABLE and
// DUMMY_FIELD_KEY can be returned for special behaviors. DUMMY_FIELD_KEY ensures that the handler's custom
// getContainerClause() is always called. SITE_WIDE_TABLE is used to select all rows.
@Nullable FieldKey getContainerFieldKey(TableInfo sourceTable);

void addDomainDataFilter(OrClause orClause, DomainFilter filter, TableInfo sourceTable, FieldKey fKey, Set<String> selectColumnNames);
// Create a filter clause that selects all rows from unfiltered containers plus filtered rows from the filtered containers
FilterClause getDomainDataFilter(Set<GUID> copyContainers, Set<GUID> filteredContainers, List<DataFilter> domainFilters, TableInfo sourceTable, FieldKey containerFieldKey, Set<String> selectColumnNames);

void addDomainDataFilter(OrClause orClause, DataFilter filter, TableInfo sourceTable, FieldKey fKey, Set<String> selectColumnNames);

// Do any necessary clean up after the target table has been populated. notCopiedFilter selects all rows in the
// source table that were NOT copied to the target table. (For example, they were filtered out due to container
// and/or domain data filtering.)
// source table that were NOT copied to the target table. (For example, rows in a global table not copied due to
// container filtering or rows in a provisioned table not copied due to domain data filtering.)
void afterTable(TableInfo sourceTable, TableInfo targetTable, SimpleFilter notCopiedFilter);

void afterSchema(DatabaseMigrationConfiguration configuration, DbSchema sourceSchema, DbSchema targetSchema, Map<String, Map<String, Sequence>> sequenceMap);
Expand Down Expand Up @@ -144,9 +156,18 @@ public List<TableInfo> getTablesToCopy()
.collect(Collectors.toCollection(ArrayList::new)); // Ensure mutable
}

@Override
public FilterClause getTableFilter(TableInfo sourceTable, FieldKey containerFieldKey, Set<GUID> containers)
{
return getContainerClause(sourceTable, containerFieldKey, containers);
}

@Override
public FilterClause getContainerClause(TableInfo sourceTable, FieldKey containerFieldKey, Set<GUID> containers)
{
if (containerFieldKey == SITE_WIDE_TABLE || containerFieldKey == DUMMY_FIELD_KEY)
throw new IllegalStateException("Should not be supplying " + containerFieldKey + " to the default getContainerClause() method");

return new InClause(containerFieldKey, containers);
}

Expand Down Expand Up @@ -192,14 +213,39 @@ public FilterClause getContainerClause(TableInfo sourceTable, FieldKey container
}

@Override
public void addDomainDataFilter(OrClause orClause, DomainFilter filter, TableInfo sourceTable, FieldKey fKey, Set<String> selectColumnNames)
public FilterClause getDomainDataFilter(Set<GUID> copyContainers, Set<GUID> filteredContainers, List<DataFilter> domainFilters, TableInfo sourceTable, FieldKey fKey, Set<String> selectColumnNames)
{
addDomainDataStandardFilter(orClause, filter, sourceTable, fKey, selectColumnNames);
// Filtered case: remove the filtered containers from the unconditional container set
Set<GUID> otherContainers = new HashSet<>(copyContainers);
otherContainers.removeAll(filteredContainers);
FilterClause ret = getContainerClause(sourceTable, fKey, otherContainers);

OrClause orClause = new OrClause();

// Delegate to the MigrationSchemaHandler to add domain-filtered containers back with their special filter applied
domainFilters.forEach(filter -> addDomainDataFilter(orClause, filter, sourceTable, fKey, selectColumnNames));

if (!orClause.getClauses().isEmpty())
{
orClause.addClause(ret);
ret = orClause;
}

return ret;
}

@Override
public void addDomainDataFilter(OrClause orClause, DataFilter filter, TableInfo sourceTable, FieldKey fKey, Set<String> selectColumnNames)
{
addDataFilter(orClause, filter, sourceTable, fKey, selectColumnNames);
}

protected void addDomainDataStandardFilter(OrClause orClause, DomainFilter filter, TableInfo sourceTable, FieldKey fKey, Set<String> selectColumnNames)
// Add a filter and return true if the column exists directly on the table
protected boolean addDataFilter(OrClause orClause, DataFilter filter, TableInfo sourceTable, FieldKey fKey, Set<String> selectColumnNames)
{
if (selectColumnNames.contains(filter.column()))
boolean columnExists = selectColumnNames.contains(filter.column());

if (columnExists)
{
// Select all rows in this domain-filtered container that meet its criteria
orClause.addClause(
Expand All @@ -209,37 +255,44 @@ protected void addDomainDataStandardFilter(OrClause orClause, DomainFilter filte
)
);
}

return columnExists;
}

// Special domain data filter method for provisioned tables that have a built-in Flag field
protected void addDomainDataFlagFilter(OrClause orClause, DomainFilter filter, TableInfo sourceTable, FieldKey fKey, Set<String> selectColumnNames)
// Add a filter to select all rows where the object property with <propertyId> equals the filter value
protected void addObjectPropertyFilter(OrClause orClause, DataFilter filter, TableInfo sourceTable, FieldKey fKey, int propertyId)
{
SQLFragment flagWhere = new SQLFragment("lsid IN (SELECT ObjectURI FROM exp.Object WHERE ObjectId IN (SELECT ObjectId FROM exp.ObjectProperty WHERE StringValue = ? AND PropertyId = ?))", filter.condition().getParamVals()[0], propertyId);
Copy link
Contributor

@labkey-matthewb labkey-matthewb Oct 31, 2025

Choose a reason for hiding this comment

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

Since most exp tables have an object it, it might be worth having a parameter that indicates whether to use "lsid" or "objectid"

Copy link
Contributor

@labkey-matthewb labkey-matthewb Oct 31, 2025

Choose a reason for hiding this comment

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

This runs the risk of the database actually evaluating ((SELECT ObjectURI FROM exp.Object WHERE ObjectId IN (SELECT ObjectId FROM exp.ObjectProperty WHERE StringValue = ? AND PropertyId = ?)) in a somewhat expensive way. It's index are optimized for objectid=? and propertyid=? joins.
Iff there is a performance issue this might be worth trying

lsid:: WHERE ? = (SELECT StringValue FROM (O WHERE objecturi=lsid) JOIN OP ON objectid WHERE PropertyId=?)
or
objectid:: WHERE ? = (SELECT StringValue FROM OP WHERE OP.ObjectId=.objectid PropertyId=?)

Copy link
Contributor Author

@labkey-adam labkey-adam Nov 1, 2025

Choose a reason for hiding this comment

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

Since most exp tables have an object it, it might be worth having a parameter that indicates whether to use "lsid" or "objectid"

This is called to filter provisioned tables, none of which have ObjectId (that I've seen). Sample Type tables do have RowId (a material RowId) and that handler generates a custom join that takes advantage. I don't think we can generalize this method to handle all the ways we join into exp.ObjectProperty... in my current FB, I've improved the comments to make it clear this is for cases where LSID is the only option.

Copy link
Contributor Author

@labkey-adam labkey-adam Nov 3, 2025

Choose a reason for hiding this comment

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

This runs the risk of the database actually evaluating ((SELECT ObjectURI FROM exp.Object WHERE ObjectId IN (SELECT ObjectId FROM exp.ObjectProperty WHERE StringValue = ? AND PropertyId = ?)) in a somewhat expensive way. It's index are optimized for objectid=? and propertyid=? joins. Iff there is a performance issue this might be worth trying

lsid:: WHERE ? = (SELECT StringValue FROM (O WHERE objecturi=lsid) JOIN OP ON objectid WHERE PropertyId=?) or objectid:: WHERE ? = (SELECT StringValue FROM OP WHERE OP.ObjectId=.objectid PropertyId=?)

@labkey-matthewb Here's how I've rewritten the full data class provisioned table SELECT statement (with hard-coded values instead of placeholders). Any further suggestions? (Note that because we're constructing and passing around FilterClauses, the first join has to be a sub-select.):

SELECT * FROM expdataclass.c1563d4300_protsequence dc WHERE lsid IN (SELECT ObjectURI FROM exp.Object o INNER JOIN exp.ObjectProperty op ON o.ObjectId = op.ObjectId WHERE StringValue = 'J.POD2' AND PropertyId = 70)

Copy link
Contributor

@labkey-matthewb labkey-matthewb Nov 3, 2025

Choose a reason for hiding this comment

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

I see. That makes sense. Simpler is usually better (or not worse) so the rewrite “looks” better to me (does that count as vibe coding)

Copy link
Contributor Author

@labkey-adam labkey-adam Nov 3, 2025

Choose a reason for hiding this comment

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

Or maybe you're saying this is better:

SELECT * FROM expdataclass.c1563d4300_protsequence dc WHERE 'J.POD2' = (
	SELECT StringValue FROM exp.ObjectProperty op INNER JOIN exp.Object o ON op.ObjectId = o.ObjectId AND ObjectURI = dc.LSID WHERE PropertyId = 70
)

Copy link
Contributor

@labkey-matthewb labkey-matthewb Nov 3, 2025

Choose a reason for hiding this comment

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

That’s how I would have written it by hand as a first guess (that kinda mirrors what we do for PropertyColumn), but if you have to stuff it into a filter clause, I can see why your rewrite makes sense. I would just keep these ideas in your back pocket if the performance is a problem.


orClause.addClause(
new AndClause(
getContainerClause(sourceTable, fKey, filter.containers()),
new SQLClause(flagWhere)
)
);
}

// Special domain data filter method for provisioned tables that have a built-in Flag field (currently used by data classes)
protected void addDomainDataFlagFilter(OrClause orClause, DataFilter filter, TableInfo sourceTable, FieldKey fKey, Set<String> selectColumnNames)
{
if (filter.column().equalsIgnoreCase("Flag"))
{
SQLFragment flagWhere = new SQLFragment("lsid IN (SELECT ObjectURI FROM exp.Object WHERE ObjectId IN (SELECT ObjectId FROM exp.ObjectProperty WHERE StringValue = ? AND PropertyId = ?))", filter.condition().getParamVals()[0], getCommentPropertyId(sourceTable));

// Select all rows where the built-in flag column equals the filter value
orClause.addClause(
new AndClause(
getContainerClause(sourceTable, fKey, filter.containers()),
new SimpleFilter.SQLClause(flagWhere)
)
);
addObjectPropertyFilter(orClause, filter, sourceTable, fKey, getCommentPropertyId(sourceTable.getSchema().getScope()));
}
else
{
addDomainDataStandardFilter(orClause, filter, sourceTable, fKey, selectColumnNames);
addDataFilter(orClause, filter, sourceTable, fKey, selectColumnNames);
}
}

private Integer _commentPropertyId = null;

protected synchronized int getCommentPropertyId(TableInfo sourceTable)
protected synchronized int getCommentPropertyId(DbScope scope)
{
if (_commentPropertyId == null)
{
// Get the exp.PropertyDescriptor table from the source scope
TableInfo propertyDescriptor = sourceTable.getSchema().getScope().getSchema("exp", DbSchemaType.Migration).getTable("PropertyDescriptor");
TableInfo propertyDescriptor = scope.getSchema("exp", DbSchemaType.Migration).getTable("PropertyDescriptor");
// Select the PropertyId associated with built-in Flag fields ("urn:exp.labkey.org/#Comment")
Integer propertyId = new TableSelector(propertyDescriptor, Collections.singleton("PropertyId"), new SimpleFilter(FieldKey.fromParts("PropertyURI"), "urn:exp.labkey.org/#Comment"), null).getObject(Integer.class);
if (propertyId == null)
Expand Down Expand Up @@ -274,22 +327,6 @@ interface MigrationTableHandler
FilterClause getAdditionalFilterClause(Set<GUID> containers);
}

abstract class DefaultMigrationTableHandler implements MigrationTableHandler
{
private final TableInfo _tableInfo;

public DefaultMigrationTableHandler(TableInfo tableInfo)
{
_tableInfo = tableInfo;
}

@Override
public TableInfo getTableInfo()
{
return _tableInfo;
}
}

/**
* A MigrationFilter adds support for the named filter property in the migration configuration file. If present,
* saveFilter() is called with the container guid and property value. Modules can register these to present
Expand All @@ -301,4 +338,26 @@ interface MigrationFilter
// Implementations should validate guid nullity
void saveFilter(@Nullable GUID guid, String value);
}

// Helper method that parses a data filter then adds it and its container to the provided collections, coalescing
// cases where multiple containers specify the same filter
static void addDataFilter(String filterName, List<DataFilter> dataFilters, Set<GUID> filteredContainers, GUID guid, String filter)
Copy link
Contributor

@labkey-matthewb labkey-matthewb Oct 31, 2025

Choose a reason for hiding this comment

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

Too bad filter isn't a FilterClause (but I don't know how we get to this point)

{
String[] filterParts = filter.split("=");
if (filterParts.length != 2)
throw new ConfigurationException("Bad " + filterName + " value; expected <columnName>=<value>: " + filter);

if (!filteredContainers.add(guid))
throw new ConfigurationException("Duplicate " + filterName + " entry for container " + guid);

String column = filterParts[0];
String value = filterParts[1];
FilterClause clause = CompareType.EQUAL.createFilterClause(new FieldKey(null, column), value);
// If another container is already using this filter clause, then simply add this guid to that domain filter.
// Otherwise, add a new domain filter to the list.
dataFilters.stream()
.filter(df -> df.column().equals(column) && df.condition().equals(clause))
.findFirst()
.ifPresentOrElse(df -> df.containers().add(guid), () -> dataFilters.add(new DataFilter(new HashSet<>(Set.of(guid)), filterParts[0], clause)));
}
}
1 change: 0 additions & 1 deletion api/src/org/labkey/api/issues/AbstractIssuesListDefDomainKind.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,6 @@
import java.util.Collection;
import java.util.Collections;
import java.util.HashMap;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Set;
Expand Down
8 changes: 2 additions & 6 deletions api/src/org/labkey/api/issues/IssuesSchema.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,14 +17,10 @@
package org.labkey.api.issues;

import org.labkey.api.data.DbSchema;
import org.labkey.api.data.DbSchemaType;
import org.labkey.api.data.TableInfo;
import org.labkey.api.data.dialect.SqlDialect;

/**
* User: Tamra Myers
* Date: Sep 29, 2006
* Time: 12:44:32 PM
*/
public class IssuesSchema
{
private static final IssuesSchema instance = new IssuesSchema();
Expand All @@ -48,7 +44,7 @@ public String getSchemaName()

public DbSchema getSchema()
{
return DbSchema.get(SCHEMA_NAME);
return DbSchema.get(SCHEMA_NAME, DbSchemaType.Module);
}

public SqlDialect getSqlDialect()
Expand Down
39 changes: 27 additions & 12 deletions core/src/org/labkey/core/CoreMigrationSchemaHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,10 @@
import org.labkey.api.data.DbScope;
import org.labkey.api.data.PropertySchema;
import org.labkey.api.data.SQLFragment;
import org.labkey.api.data.SimpleFilter;
import org.labkey.api.data.SimpleFilter.AndClause;
import org.labkey.api.data.SimpleFilter.FilterClause;
import org.labkey.api.data.SimpleFilter.OrClause;
import org.labkey.api.data.SimpleFilter.SQLClause;
import org.labkey.api.data.SqlExecutor;
import org.labkey.api.data.Table;
import org.labkey.api.data.TableInfo;
Expand Down Expand Up @@ -113,19 +116,13 @@ public List<TableInfo> getTablesToCopy()
}

@Override
public SimpleFilter.FilterClause getContainerClause(TableInfo sourceTable, FieldKey containerFieldKey, Set<GUID> containers)
public FilterClause getTableFilter(TableInfo sourceTable, FieldKey containerFieldKey, Set<GUID> containers)
{
SimpleFilter.FilterClause containerClause = super.getContainerClause(sourceTable, containerFieldKey, containers);
FilterClause filterClause = getContainerClause(sourceTable, containerFieldKey, containers);
String tableName = sourceTable.getName();

// Users and root groups have container == null, so add that as an OR clause
if ("Principals".equals(tableName) || "Members".equals(tableName))
{
SimpleFilter.OrClause orClause = new SimpleFilter.OrClause();
orClause.addClause(containerClause);
orClause.addClause(new CompareType.CompareClause(containerFieldKey, CompareType.ISBLANK, null));
containerClause = orClause;

if (_groupFilterCondition != null)
{
SQLFragment groupFilterFragment = new SQLFragment();
Expand All @@ -144,7 +141,7 @@ public SimpleFilter.FilterClause getContainerClause(TableInfo sourceTable, Field
.append(_groupFilterCondition);
}

containerClause = new SimpleFilter.AndClause(containerClause, new SimpleFilter.SQLClause(groupFilterFragment));
filterClause = new AndClause(filterClause, new SQLClause(groupFilterFragment));
}
}

Expand All @@ -153,7 +150,25 @@ public SimpleFilter.FilterClause getContainerClause(TableInfo sourceTable, Field
SQLFragment groupFilterFragment = new SQLFragment("UserId IN (SELECT UserId FROM core.Principals WHERE Type <> 'g' OR (type = 'g' AND UserId ")
.append(_groupFilterCondition)
.append("))");
containerClause = new SimpleFilter.AndClause(containerClause, new SimpleFilter.SQLClause(groupFilterFragment));
filterClause = new AndClause(filterClause, new SQLClause(groupFilterFragment));
}

return filterClause;
}

@Override
public FilterClause getContainerClause(TableInfo sourceTable, FieldKey containerFieldKey, Set<GUID> containers)
{
FilterClause containerClause = super.getContainerClause(sourceTable, containerFieldKey, containers);
String tableName = sourceTable.getName();

if ("Principals".equals(tableName) || "Members".equals(tableName))
{
// Users and root groups have container == null, so add that as an OR clause
OrClause orClause = new OrClause();
orClause.addClause(containerClause);
orClause.addClause(new CompareType.CompareClause(containerFieldKey, CompareType.ISBLANK, null));
containerClause = orClause;
}

return containerClause;
Expand All @@ -180,7 +195,7 @@ public String getName()
public void saveFilter(@Nullable GUID guid, String groupFilter)
{
if (guid != null)
throw new ConfigurationException("GUID should not be provided to GroupFilter");
throw new ConfigurationException("GroupFilter is applied globally; you cannot specify a GUID");

_groupFilterCondition = new SQLFragment(groupFilter);
}
Expand Down
Loading