From 68f06d95cef65cfa9b586f011640475b67df1037 Mon Sep 17 00:00:00 2001 From: labkey-nicka Date: Fri, 1 May 2026 09:40:52 -0700 Subject: [PATCH] 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 8019da5b6a0..98d68b91a23 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 {