From cdde35c3a0eeed4e44f7afd7a94cf4a5f38c9a42 Mon Sep 17 00:00:00 2001 From: labkey-nicka Date: Fri, 1 May 2026 09:40:52 -0700 Subject: [PATCH 1/2] TableInfo: ensure ExprColumn dependent columns are referenced --- .../labkey/api/data/AbstractTableInfo.java | 68 ++++++++++++------- api/src/org/labkey/api/query/ExprColumn.java | 21 +++--- .../org/labkey/query/QueryServiceImpl.java | 2 +- 3 files changed, 59 insertions(+), 32 deletions(-) diff --git a/api/src/org/labkey/api/data/AbstractTableInfo.java b/api/src/org/labkey/api/data/AbstractTableInfo.java index 8d1868df2fc..2146de8925c 100644 --- a/api/src/org/labkey/api/data/AbstractTableInfo.java +++ b/api/src/org/labkey/api/data/AbstractTableInfo.java @@ -43,6 +43,7 @@ import org.labkey.api.query.AggregateRowConfig; import org.labkey.api.query.BatchValidationException; import org.labkey.api.query.DetailsURL; +import org.labkey.api.query.ExprColumn; import org.labkey.api.query.FieldKey; import org.labkey.api.query.MetadataParseWarning; import org.labkey.api.query.QueryException; @@ -181,10 +182,10 @@ public enum MultiValuedFkType private final Map _counterDefinitionMap = new CaseInsensitiveHashMap<>(); // Really only 1 for now, but could be more in future /* If a subclass generates a non-trivial FROM clause in getFromSQL(), it may need to track dependencies - * for calculated columns. This is where we do that. This is setup in loadAllButCustomizerFromXML(), and + * for calculated columns. This is where we do that. This is set up in loadAllButCustomizerFromXML(), and * and used in getFromSQL(String alias, Set cols) */ - protected final HashMap> _referencedColumns = new HashMap<>(); + protected final Map> _referencedColumns = new HashMap<>(); private boolean _initialColumnsAreAdded = false; @@ -325,7 +326,6 @@ public void afterConstruct() } } - protected Map constructColumnMap() { if (isCaseSensitive()) @@ -399,15 +399,17 @@ protected Set expandColumns(Set columns) { if (null == columns || columns.isEmpty() || _referencedColumns.isEmpty()) return columns; - // We're not recursively expanding. However, if expressions can reference each other we'll have to. + + // We're not recursively expanding. However, if expressions can reference each other, we'll have to. HashSet expanded = new HashSet<>(); for (var fk : columns) { expanded.add(fk); - HashSet refs = _referencedColumns.get(fk); + Set refs = _referencedColumns.get(fk); if (null != refs) expanded.addAll(refs); } + return expanded; } @@ -439,8 +441,8 @@ protected SQLFragment getFromSQLExpanded(String alias, Set cols) List pkColumns = getPkColumns(); if (pkColumns.size() != 1) return new NamedObjectList(); - else - return getSelectList(pkColumns.get(0), Collections.emptyList(), maxRows, titleColumnInfo); + + return getSelectList(pkColumns.getFirst(), Collections.emptyList(), maxRows, titleColumnInfo); } ColumnInfo column = getColumn(columnName); @@ -460,12 +462,12 @@ protected SQLFragment getFromSQLExpanded(String alias, Set cols) final int titleIndex; if (!(firstColumn.equals(titleColumn))) { - cols = Arrays.asList(firstColumn, titleColumn); + cols = List.of(firstColumn, titleColumn); titleIndex = 2; } else { - cols = Arrays.asList(firstColumn); + cols = List.of(firstColumn); titleIndex = 1; } @@ -563,7 +565,7 @@ public String getTitleColumn() } } if (null == _titleColumn && !getColumns().isEmpty()) - _titleColumn = getColumns().get(0).getName(); + _titleColumn = getColumns().getFirst().getName(); } return _titleColumn; @@ -769,21 +771,39 @@ public void setDescription(String description) _description = description; } + // GitHub Issue #1118 + // ExprColumn may have dependent columns that need to be referenced so that getFromSQL() can include them + private void updateReferencedColumns(ColumnInfo column) + { + if (column instanceof ExprColumn exprColumn) + { + var fieldKey = exprColumn.getFieldKey(); + var dependentColumns = exprColumn.getDependentColumns(); + if (!dependentColumns.isEmpty()) + _referencedColumns.put(fieldKey, dependentColumns.stream().map(ColumnInfo::getFieldKey).collect(Collectors.toSet())); + } + } + public boolean removeColumn(ColumnInfo column) { checkLocked(); ensureInitialColumnsAreAdded(); - // Clear the cached resolved columns so we regenerate it if the shape of the table changes - _resolvedColumns.clear(); - return _columnMap.remove(column.getName()) != null; + + boolean removed = _columnMap.remove(column.getName()) != null; + if (removed) + { + // Clear the cached resolved columns so we regenerate it if the shape of the table changes + _resolvedColumns.clear(); + _referencedColumns.remove(column.getFieldKey()); + } + + return removed; } public MutableColumnInfo addColumn(MutableColumnInfo column) { checkLocked(); ensureInitialColumnsAreAdded(); - // Not true if this is a VirtualTableInfo - // assert column.getParentTable() == this; if (_columnMap.containsKey(column.getName())) { String message = "Column " + column.getName() + " already exists for table " + getName() + ". Full set of existing columns: " + _columnMap.keySet(); @@ -794,7 +814,9 @@ public MutableColumnInfo addColumn(MutableColumnInfo column) _columnMap.put(column.getName(), column); // Clear the cached resolved columns so we regenerate it if the shape of the table changes _resolvedColumns.clear(); + updateReferencedColumns(column); assert !(column instanceof BaseColumnInfo) || ((BaseColumnInfo)column).lockName(); + return column; } @@ -803,8 +825,6 @@ public MutableColumnInfo addColumn(MutableColumnInfo column) * This is usually only done in TableInfo.afterConstruct() to modify the behavior of a column. * Because the ColumnInfo implementation can change in afterConstruct(), TableInfo implementations * should hold onto columnInfo references by FieldKey, and not by reference. - - * during construction. */ public ColumnInfo replaceColumn(ColumnInfo updated, ColumnInfo existing) { @@ -821,21 +841,24 @@ public ColumnInfo replaceColumn(ColumnInfo updated, ColumnInfo existing) _columnMap.put(updated.getName(), updated); // Clear the cached resolved columns so we regenerate it if the shape of the table changes _resolvedColumns.clear(); + + _referencedColumns.remove(existing.getFieldKey()); + updateReferencedColumns(updated); + return updated; } - protected ColumnInfo transformColumn(MutableColumnInfo existing, @Nullable ColumnInfoTransformer t) { checkLocked(); existing.checkLocked(); if (null == t) return existing; + MutableColumnInfo updated = t.apply(existing); return replaceColumn(updated, existing); } - public void addCounterDefinition(@NotNull CounterDefinition counterDef) { boolean valid = true; @@ -1380,7 +1403,6 @@ protected void loadAllButCustomizerFromXML(QuerySchema schema, @Nullable TableTy if (xmlTable.isSetTableUrl()) _detailsURL = DetailsURL.fromXML(xmlTable.getTableUrl(), errors); - if (xmlTable.isSetCacheSize()) _cacheSize = xmlTable.getCacheSize(); @@ -1467,7 +1489,8 @@ else if (xmlColumn.isSetWrappedColumnName() && isNotBlank(xmlColumn.getWrappedCo var wrappedColumn = QueryService.get().createQueryExpressionColumn(this, FieldKey.fromParts(xmlColumn.getColumnName()), sql, xmlColumn); HashSet referencedColumns = new HashSet<>(); QueryService.get().bindQueryExpressionColumn(wrappedColumn, existingColumns, true, referencedColumns); - _referencedColumns.put(wrappedColumn.getFieldKey(), referencedColumns); + if (!referencedColumns.isEmpty()) + _referencedColumns.put(wrappedColumn.getFieldKey(), referencedColumns); calculatedFieldKeys.add(wrappedColumn.getFieldKey()); addColumn(wrappedColumn); } @@ -1503,7 +1526,6 @@ else if (!calculatedFieldKeys.isEmpty()) { warnings.add(new QueryParseWarning("Invalid AuditLogging: " + auditBehavior,null,0,0)); } - } _warnings.addAll(warnings); @@ -2140,7 +2162,7 @@ public FieldKey getAuditRowPk() { List pks = getPkColumnNames(); if (pks.size() == 1) - _auditRowPk = FieldKey.fromParts(pks.get(0)); + _auditRowPk = FieldKey.fromParts(pks.getFirst()); else if (getColumn(FieldKey.fromParts("EntityId")) != null) _auditRowPk = FieldKey.fromParts("EntityId"); else if (getColumn(FieldKey.fromParts("RowId")) != null) diff --git a/api/src/org/labkey/api/query/ExprColumn.java b/api/src/org/labkey/api/query/ExprColumn.java index 1e4f00f29a9..2e9730e0c64 100644 --- a/api/src/org/labkey/api/query/ExprColumn.java +++ b/api/src/org/labkey/api/query/ExprColumn.java @@ -16,7 +16,7 @@ package org.labkey.api.query; -import org.apache.commons.lang3.StringUtils; +import org.apache.commons.lang3.Strings; import org.labkey.api.data.BaseColumnInfo; import org.labkey.api.data.ColumnInfo; import org.labkey.api.data.JdbcType; @@ -24,6 +24,8 @@ import org.labkey.api.data.SQLFragment; import org.labkey.api.data.TableInfo; +import java.util.Collections; +import java.util.List; import java.util.Map; import java.util.function.Supplier; @@ -70,12 +72,11 @@ public ExprColumn(TableInfo parent, String name, SQLFragment sql, JdbcType type, this(parent, FieldKey.fromParts(name), sql, type, dependentColumns); } - public static MutableColumnInfo create(TableInfo parent, String name, JdbcType type, SQLFragment sqlf, ColumnInfo ... dependentColumns) + public static MutableColumnInfo create(TableInfo parent, String name, JdbcType type, SQLFragment sql, ColumnInfo ... dependentColumns) { - return new ExprColumn(parent, name, sqlf, type, dependentColumns); + return new ExprColumn(parent, name, sql, type, dependentColumns); } - /* * Use this 'constructor' to create an ExprColumn whose SQL is generated on demand. This is useful if generating * the SQL is at all expensive. Most TableInfo objects are not used for generating SQL and do not need to get this value. @@ -94,24 +95,21 @@ public SQLFragment getValueSql(String tableAlias) }; } - @Override public SQLFragment getValueSql(String tableAlias) { if (tableAlias.equals(STR_TABLE_ALIAS)) return _sql; SQLFragment ret = new SQLFragment(_sql); - ret.setSqlUnsafe(StringUtils.replace(_sql.getSQL(), STR_TABLE_ALIAS, tableAlias)); + ret.setSqlUnsafe(Strings.CS.replace(_sql.getSQL(), STR_TABLE_ALIAS, tableAlias)); return ret; } - public void setValueSQL(SQLFragment sql) { _sql = sql; } - @Override public void declareJoins(String parentAlias, Map map) { @@ -123,4 +121,11 @@ public void declareJoins(String parentAlias, Map map) } } } + + public List getDependentColumns() + { + if (_dependentColumns == null) + return Collections.emptyList(); + return List.of(_dependentColumns); + } } diff --git a/query/src/org/labkey/query/QueryServiceImpl.java b/query/src/org/labkey/query/QueryServiceImpl.java index fde24951ed3..77d62dbad87 100644 --- a/query/src/org/labkey/query/QueryServiceImpl.java +++ b/query/src/org/labkey/query/QueryServiceImpl.java @@ -3040,7 +3040,7 @@ public MutableColumnInfo createQueryExpressionColumn(TableInfo table, FieldKey k return ret; } - /** Compute and set the metadata for this column based on the source expressoin and the xml override */ + /** Compute and set the metadata for this column based on the source expression and the xml override */ @Override public void bindQueryExpressionColumn(ColumnInfo col, Map columns, boolean validateOnly, @Nullable Set referencedKeys) throws QueryParseException { From 896345a26769068d8bc97e2bb606e90ce0a4c813 Mon Sep 17 00:00:00 2001 From: labkey-nicka Date: Mon, 4 May 2026 10:51:04 -0700 Subject: [PATCH 2/2] ColumnInfo.getReferencedFieldKeys() --- .../labkey/api/data/AbstractTableInfo.java | 74 ++++++++++--------- api/src/org/labkey/api/data/ColumnInfo.java | 11 ++- api/src/org/labkey/api/query/ExprColumn.java | 17 +++-- .../query/sql/CalculatedExpressionColumn.java | 12 ++- 4 files changed, 69 insertions(+), 45 deletions(-) diff --git a/api/src/org/labkey/api/data/AbstractTableInfo.java b/api/src/org/labkey/api/data/AbstractTableInfo.java index 2146de8925c..d57ce93a665 100644 --- a/api/src/org/labkey/api/data/AbstractTableInfo.java +++ b/api/src/org/labkey/api/data/AbstractTableInfo.java @@ -43,7 +43,6 @@ import org.labkey.api.query.AggregateRowConfig; import org.labkey.api.query.BatchValidationException; import org.labkey.api.query.DetailsURL; -import org.labkey.api.query.ExprColumn; import org.labkey.api.query.FieldKey; import org.labkey.api.query.MetadataParseWarning; import org.labkey.api.query.QueryException; @@ -182,10 +181,10 @@ public enum MultiValuedFkType private final Map _counterDefinitionMap = new CaseInsensitiveHashMap<>(); // Really only 1 for now, but could be more in future /* If a subclass generates a non-trivial FROM clause in getFromSQL(), it may need to track dependencies - * for calculated columns. This is where we do that. This is set up in loadAllButCustomizerFromXML(), and - * and used in getFromSQL(String alias, Set cols) + * for calculated columns. Lazily built from each column's getReferencedFieldKeys() and used in + * getFromSQL(String alias, Set cols). Cleared whenever the column shape changes. */ - protected final Map> _referencedColumns = new HashMap<>(); + private volatile Map> _referencedColumns; private boolean _initialColumnsAreAdded = false; @@ -392,12 +391,23 @@ public SQLFragment getFromSQL(String alias) return new SQLFragment().append("(").append(getFromSQL()).append(") ").appendIdentifier(alias); } + // Clear the cached resolved/referenced columns so we regenerate them when the shape of the table changes + private void clearColumnReferences() + { + _resolvedColumns.clear(); + _referencedColumns = null; + } + /** When a table a) overrides (String alias, Set cols) b) has CalculatedColumns we need to make sure that * we include the dependent columns in the Set<>. */ protected Set expandColumns(Set columns) { - if (null == columns || columns.isEmpty() || _referencedColumns.isEmpty()) + if (null == columns || columns.isEmpty()) + return columns; + + var referenced = getReferencedColumns(); + if (referenced.isEmpty()) return columns; // We're not recursively expanding. However, if expressions can reference each other, we'll have to. @@ -405,7 +415,7 @@ protected Set expandColumns(Set columns) for (var fk : columns) { expanded.add(fk); - Set refs = _referencedColumns.get(fk); + Set refs = referenced.get(fk); if (null != refs) expanded.addAll(refs); } @@ -413,6 +423,24 @@ protected Set expandColumns(Set columns) return expanded; } + private Map> getReferencedColumns() + { + if (_referencedColumns == null) + { + var built = new HashMap>(); + for (var col : getColumns()) + { + var deps = col.getReferencedFieldKeys(); + if (!deps.isEmpty()) + built.put(col.getFieldKey(), deps); + } + + _referencedColumns = built.isEmpty() ? Map.of() : Collections.unmodifiableMap(built); + } + + return _referencedColumns; + } + @Override public final SQLFragment getFromSQL(String alias, Set cols) { @@ -771,19 +799,6 @@ public void setDescription(String description) _description = description; } - // GitHub Issue #1118 - // ExprColumn may have dependent columns that need to be referenced so that getFromSQL() can include them - private void updateReferencedColumns(ColumnInfo column) - { - if (column instanceof ExprColumn exprColumn) - { - var fieldKey = exprColumn.getFieldKey(); - var dependentColumns = exprColumn.getDependentColumns(); - if (!dependentColumns.isEmpty()) - _referencedColumns.put(fieldKey, dependentColumns.stream().map(ColumnInfo::getFieldKey).collect(Collectors.toSet())); - } - } - public boolean removeColumn(ColumnInfo column) { checkLocked(); @@ -791,11 +806,7 @@ public boolean removeColumn(ColumnInfo column) boolean removed = _columnMap.remove(column.getName()) != null; if (removed) - { - // Clear the cached resolved columns so we regenerate it if the shape of the table changes - _resolvedColumns.clear(); - _referencedColumns.remove(column.getFieldKey()); - } + clearColumnReferences(); return removed; } @@ -812,9 +823,7 @@ public MutableColumnInfo addColumn(MutableColumnInfo column) assert false : message; } _columnMap.put(column.getName(), column); - // Clear the cached resolved columns so we regenerate it if the shape of the table changes - _resolvedColumns.clear(); - updateReferencedColumns(column); + clearColumnReferences(); assert !(column instanceof BaseColumnInfo) || ((BaseColumnInfo)column).lockName(); return column; @@ -839,11 +848,7 @@ public ColumnInfo replaceColumn(ColumnInfo updated, ColumnInfo existing) throw new IllegalStateException("Column must have the same name"); _columnMap.put(updated.getName(), updated); - // Clear the cached resolved columns so we regenerate it if the shape of the table changes - _resolvedColumns.clear(); - - _referencedColumns.remove(existing.getFieldKey()); - updateReferencedColumns(updated); + clearColumnReferences(); return updated; } @@ -1487,10 +1492,7 @@ else if (xmlColumn.isSetWrappedColumnName() && isNotBlank(xmlColumn.getWrappedCo try { var wrappedColumn = QueryService.get().createQueryExpressionColumn(this, FieldKey.fromParts(xmlColumn.getColumnName()), sql, xmlColumn); - HashSet referencedColumns = new HashSet<>(); - QueryService.get().bindQueryExpressionColumn(wrappedColumn, existingColumns, true, referencedColumns); - if (!referencedColumns.isEmpty()) - _referencedColumns.put(wrappedColumn.getFieldKey(), referencedColumns); + QueryService.get().bindQueryExpressionColumn(wrappedColumn, existingColumns, true, null); calculatedFieldKeys.add(wrappedColumn.getFieldKey()); addColumn(wrappedColumn); } diff --git a/api/src/org/labkey/api/data/ColumnInfo.java b/api/src/org/labkey/api/data/ColumnInfo.java index 420f64f1b63..14fd5ba357a 100644 --- a/api/src/org/labkey/api/data/ColumnInfo.java +++ b/api/src/org/labkey/api/data/ColumnInfo.java @@ -35,8 +35,10 @@ import java.sql.ResultSet; import java.sql.SQLException; import java.util.ArrayList; +import java.util.Collections; import java.util.List; import java.util.Map; +import java.util.Set; public interface ColumnInfo extends ColumnRenderProperties { @@ -371,6 +373,12 @@ default boolean inferIsShownInInsertView() boolean isCalculated(); + /** FieldKeys this column depends on at SQL generation time. */ + default Set getReferencedFieldKeys() + { + return Collections.emptySet(); + } + default boolean isValueExpressionColumn() { return getValueExpression() != null && getWrappedColumnName() == null; @@ -405,6 +413,7 @@ static boolean booleanFromString(String str) { return BaseColumnInfo.booleanFromString(str); } + static boolean booleanFromObj(Object o) { return BaseColumnInfo.booleanFromObj(o); @@ -483,5 +492,3 @@ static SimpleConvert getDefaultConvertFn(ColumnInfo col) return ColumnRenderProperties.getDefaultConvertFn(col); } } - - diff --git a/api/src/org/labkey/api/query/ExprColumn.java b/api/src/org/labkey/api/query/ExprColumn.java index 2e9730e0c64..2de242c2e10 100644 --- a/api/src/org/labkey/api/query/ExprColumn.java +++ b/api/src/org/labkey/api/query/ExprColumn.java @@ -25,8 +25,9 @@ import org.labkey.api.data.TableInfo; import java.util.Collections; -import java.util.List; +import java.util.HashSet; import java.util.Map; +import java.util.Set; import java.util.function.Supplier; /** @@ -122,10 +123,16 @@ public void declareJoins(String parentAlias, Map map) } } - public List getDependentColumns() + @Override + public Set getReferencedFieldKeys() { - if (_dependentColumns == null) - return Collections.emptyList(); - return List.of(_dependentColumns); + if (_dependentColumns == null || _dependentColumns.length == 0) + return Collections.emptySet(); + + var keys = new HashSet(); + for (var c : _dependentColumns) + keys.add(c.getFieldKey()); + + return Collections.unmodifiableSet(keys); } } diff --git a/query/src/org/labkey/query/sql/CalculatedExpressionColumn.java b/query/src/org/labkey/query/sql/CalculatedExpressionColumn.java index 14d839177c7..cf05dafaf9f 100644 --- a/query/src/org/labkey/query/sql/CalculatedExpressionColumn.java +++ b/query/src/org/labkey/query/sql/CalculatedExpressionColumn.java @@ -23,8 +23,8 @@ import org.labkey.data.xml.ColumnType; import org.labkey.data.xml.TableType; -import java.sql.SQLException; import java.util.ArrayList; +import java.util.Collections; import java.util.HashSet; import java.util.List; import java.util.Map; @@ -97,6 +97,14 @@ public void validate(Map columnMap, @Nullable Set getReferencedFieldKeys() + { + if (_allFieldKeys == null) + return Collections.emptySet(); + + return Collections.unmodifiableSet(_allFieldKeys); + } public void computeMetaData(Map columns) { @@ -397,7 +405,7 @@ public _SchemaTableInfo(DbSchema schema, DatabaseTableType tableType, String tab } @Override - protected SchemaColumnMetaData createSchemaColumnMetaData() throws SQLException + protected SchemaColumnMetaData createSchemaColumnMetaData() { return new SchemaColumnMetaData(this, colsToAdd, xmlToApply); }