Skip to content

TableInfo: ensure ExprColumn dependent columns are referenced #7638

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

Open
labkey-nicka wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

TableInfo: ensure ExprColumn dependent columns are referenced #7638

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
68 changes: 45 additions & 23 deletions api/src/org/labkey/api/data/AbstractTableInfo.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -181,10 +182,10 @@ public enum MultiValuedFkType
private final Map<String, CounterDefinition> _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<FieldKey> cols)
*/
protected final HashMap<FieldKey, HashSet<FieldKey>> _referencedColumns = new HashMap<>();
protected final Map<FieldKey, Set<FieldKey>> _referencedColumns = new HashMap<>();

private boolean _initialColumnsAreAdded = false;

Expand Down Expand Up @@ -325,7 +326,6 @@ public void afterConstruct()
}
}


protected Map<String, ColumnInfo> constructColumnMap()
{
if (isCaseSensitive())
Expand Down Expand Up @@ -399,15 +399,17 @@ protected Set<FieldKey> expandColumns(Set<FieldKey> 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<FieldKey> expanded = new HashSet<>();
for (var fk : columns)
{
expanded.add(fk);
HashSet<FieldKey> refs = _referencedColumns.get(fk);
Set<FieldKey> refs = _referencedColumns.get(fk);
if (null != refs)
expanded.addAll(refs);
}

return expanded;
}

Expand Down Expand Up @@ -439,8 +441,8 @@ protected SQLFragment getFromSQLExpanded(String alias, Set<FieldKey> cols)
List<ColumnInfo> 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);
Expand All @@ -460,12 +462,12 @@ protected SQLFragment getFromSQLExpanded(String alias, Set<FieldKey> 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;
}

Expand Down Expand Up @@ -563,7 +565,7 @@ public String getTitleColumn()
}
}
if (null == _titleColumn && !getColumns().isEmpty())
_titleColumn = getColumns().get(0).getName();
_titleColumn = getColumns().getFirst().getName();
}

return _titleColumn;
Expand Down Expand Up @@ -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();
Expand All @@ -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;
}

Expand All @@ -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)
{
Expand All @@ -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;
Expand Down Expand Up @@ -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();

Expand Down Expand Up @@ -1467,7 +1489,8 @@ else if (xmlColumn.isSetWrappedColumnName() && isNotBlank(xmlColumn.getWrappedCo
var wrappedColumn = QueryService.get().createQueryExpressionColumn(this, FieldKey.fromParts(xmlColumn.getColumnName()), sql, xmlColumn);
HashSet<FieldKey> 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);
}
Expand Down Expand Up @@ -1503,7 +1526,6 @@ else if (!calculatedFieldKeys.isEmpty())
{
warnings.add(new QueryParseWarning("Invalid AuditLogging: " + auditBehavior,null,0,0));
}

}

_warnings.addAll(warnings);
Expand Down Expand Up @@ -2140,7 +2162,7 @@ public FieldKey getAuditRowPk()
{
List<String> 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)
Expand Down
21 changes: 13 additions & 8 deletions api/src/org/labkey/api/query/ExprColumn.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,14 +16,16 @@

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;
import org.labkey.api.data.MutableColumnInfo;
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;

Expand Down Expand Up @@ -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.
Expand All @@ -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<String, SQLFragment> map)
{
Expand All @@ -123,4 +121,11 @@ public void declareJoins(String parentAlias, Map<String, SQLFragment> map)
}
}
}

public List<ColumnInfo> getDependentColumns()
{
if (_dependentColumns == null)
return Collections.emptyList();
return List.of(_dependentColumns);
}
}
2 changes: 1 addition & 1 deletion query/src/org/labkey/query/QueryServiceImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<FieldKey,ColumnInfo> columns, boolean validateOnly, @Nullable Set<FieldKey> referencedKeys) throws QueryParseException
{
Expand Down
Loading