Skip to content

GitHub Issue 1499: SpecialCharacterMetricsMaintenanceTask is throwing exceptions on legacy specimen tables #8005

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
XingY wants to merge 1 commit into
base: develop
Choose a base branch
from
+62 −15
Open

GitHub Issue 1499: SpecialCharacterMetricsMaintenanceTask is throwing exceptions on legacy specimen tables #8005

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
61 changes: 54 additions & 7 deletions experiment/src/org/labkey/experiment/SpecialCharacterMetricsMaintenanceTask.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,10 @@
import org.junit.Before;
import org.junit.Test;
import org.labkey.api.collections.CaseInsensitiveHashMap;
import org.labkey.api.collections.CaseInsensitiveHashSet;
import org.labkey.api.data.Container;
import org.labkey.api.data.DbSchema;
import org.labkey.api.data.DbSchemaType;
import org.labkey.api.data.DbScope;
import org.labkey.api.data.SQLFragment;
import org.labkey.api.data.SqlSelector;
Expand Down Expand Up @@ -54,6 +57,7 @@
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
import java.util.Set;

public class SpecialCharacterMetricsMaintenanceTask implements MaintenanceTask
{
Expand Down Expand Up @@ -182,7 +186,7 @@ private void collectTextFieldMetrics(DbScope scope, SqlDialect dialect, Map<Stri
{
// Enumerate provisioned Text/Multiline columns, excluding Text Choice fields.
SQLFragment enumSql = new SQLFragment(
"SELECT dd.storageschemaname, dd.storagetablename, pd.storagecolumnname, pd.rangeuri AS rangeuri\n" +
"SELECT dd.storageschemaname, dd.storagetablename, pd.storagecolumnname, pd.name, pd.rangeuri AS rangeuri\n" +
"FROM exp.propertydescriptor pd\n" +
"JOIN exp.propertydomain pdm ON pd.propertyid = pdm.propertyid\n" +
"JOIN exp.domaindescriptor dd ON pdm.domainid = dd.domainid\n" +
Expand All @@ -198,7 +202,7 @@ private void collectTextFieldMetrics(DbScope scope, SqlDialect dialect, Map<Stri
String rangeUri = rs.getString("rangeuri");
String fieldType = PropertyType.MULTI_LINE.getTypeUri().equals(rangeUri) ? TYPE_MULTILINE : TYPE_TEXT;
TableKey key = new TableKey(rs.getString("storageschemaname"), rs.getString("storagetablename"));
byTable.computeIfAbsent(key, k -> new ArrayList<>()).add(new Col(rs.getString("storagecolumnname"), fieldType));
byTable.computeIfAbsent(key, k -> new ArrayList<>()).add(new Col(rs.getString("storagecolumnname"), rs.getString("name"), fieldType));
});

// Sample type and data class provisioned tables have a base "name" column that is not a domain property (so it
Expand All @@ -224,8 +228,8 @@ private void addBaseNameColumns(DbScope scope, Map<TableKey, List<Col>> byTable)
new SqlSelector(scope, sql).forEach(rs -> {
TableKey key = new TableKey(rs.getString("storageschemaname"), rs.getString("storagetablename"));
List<Col> cols = byTable.computeIfAbsent(key, k -> new ArrayList<>());
if (cols.stream().noneMatch(col -> "name".equalsIgnoreCase(col.storageName())))
cols.add(new Col("name", TYPE_DATA_NAME));
if (cols.stream().noneMatch(col -> "name".equalsIgnoreCase(col.storageColumnName())))
cols.add(new Col("name", "name", TYPE_DATA_NAME));
});
}

Expand All @@ -234,9 +238,43 @@ private void scanTable(DbScope scope, SqlDialect dialect, TableKey table, List<C
SQLFragment sql = new SQLFragment("SELECT ");
Map<String, String[]> aliasMeta = new LinkedHashMap<>();
boolean first = true;

Map<String, String> remappedCols = new CaseInsensitiveHashMap<>();
boolean needsRemap = false;
for (Col col : cols)
{
// GitHub Issue 1449: A legacy descriptor can carry a storage name that is the field name uniquified with a numeric suffix (e.g. "Container1") but never became a physical column
String storageColumnName = col.storageColumnName();
if (isNumericSuffixOf(storageColumnName, col.colName()))
{
needsRemap = true;
break;
}
}

if (needsRemap)
{
TableInfo ti = DbSchema.get(table.schema(), DbSchemaType.Provisioned).getTable(table.table());
if (ti != null)
{
Set<String> columnNames = new CaseInsensitiveHashSet(ti.getColumnNameSet());
for (Col col : cols)
{
String storageColumnName = col.storageColumnName();
if (isNumericSuffixOf(storageColumnName, col.colName()) && !columnNames.contains(storageColumnName) && columnNames.contains(col.colName()))
remappedCols.put(storageColumnName, col.colName());
}
}
}

for (int i = 0; i < cols.size(); i++)
{
SQLFragment colRef = PropertyDescriptor.getLegalSelectNameFromStorageName(dialect, cols.get(i).storageName()).getSql();
Col col = cols.get(i);
String storageColumnName = col.storageColumnName();
if (remappedCols.containsKey(storageColumnName))
storageColumnName = remappedCols.get(storageColumnName);

SQLFragment colRef = PropertyDescriptor.getLegalSelectNameFromStorageName(dialect, storageColumnName).getSql();
for (String ck : CHAR_KEYS)
{
String alias = "c" + i + "_" + ck.toLowerCase();
Expand All @@ -246,7 +284,7 @@ private void scanTable(DbScope scope, SqlDialect dialect, TableKey table, List<C
sql.append("bool_or(");
appendBoolExpr(sql, ck, colRef);
sql.append(") AS ").append(alias);
aliasMeta.put(alias, new String[]{cols.get(i).fieldType(), ck});
aliasMeta.put(alias, new String[]{col.fieldType(), ck});
}
}
sql.append(" FROM ").appendIdentifier(PropertyDescriptor.getLegalSelectNameFromStorageName(dialect, table.schema())).append(".").appendIdentifier(PropertyDescriptor.getLegalSelectNameFromStorageName(dialect, table.table()));
Expand All @@ -272,6 +310,15 @@ private void scanTable(DbScope scope, SqlDialect dialect, TableKey table, List<C
}
}

// True when storageName is name followed by a positive-integer suffix, compared case-insensitively (e.g. "Container1" for "Container").
// Prior to https://www.labkey.org/home/Developer/issues/issues-details.view?issueId=29047, suffix (1, 2, etc.) would be added to field that matches base fields
private static boolean isNumericSuffixOf(String storageName, String name)
{
if (name == null || storageName.length() <= name.length() || !storageName.regionMatches(true, 0, name, 0, name.length()))
return false;
return storageName.substring(name.length()).chars().allMatch(Character::isDigit);
}

// Appends a boolean SQL expression detecting the given special character.
// The search patterns are bound as parameters so that no semicolons or quotes appear in the SQL text, which SQLFragment rejects.
// None of the target characters are LIKE wildcards, so they need no LIKE escaping.
Expand All @@ -289,7 +336,7 @@ private static void appendBoolExpr(SQLFragment sql, String key, SQLFragment colR

private record TableKey(String schema, String table) {}

private record Col(String storageName, String fieldType) {}
private record Col(String storageColumnName, String colName, String fieldType) {}

public static class TestCase extends Assert
{
Expand Down
12 changes: 6 additions & 6 deletions experiment/src/org/labkey/experiment/api/property/DomainImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -588,24 +588,24 @@ private void validatePropertyLookup(User user, DomainProperty dp) throws ChangeP

public void saveIfNotExists(User user) throws ChangePropertyDescriptorException
{
save(user, false, true, null, null, null, null, null, null);
save(user, true, null, null, null, null, null, null);
}

@Override
public void save(User user, @Nullable Map<String, Object> newRecordMap, @Nullable List<? extends GWTPropertyDescriptor> calculatedFields) throws ChangePropertyDescriptorException
{
save(user, false, false, null, null, null, newRecordMap, null, calculatedFields);
save(user, false, null, null, null, newRecordMap, null, calculatedFields);
}

@Override
public void save(User user, @Nullable String auditComment, @Nullable String auditUserComment,
@Nullable Map<String, Object> oldRecordMap, @Nullable Map<String, Object> newRecordMap,
@Nullable List<? extends GWTPropertyDescriptor> oldCalculatedFields, @Nullable List<? extends GWTPropertyDescriptor> newCalculatedFields) throws ChangePropertyDescriptorException
{
save(user, false, false, auditComment, auditUserComment, oldRecordMap, newRecordMap, oldCalculatedFields, newCalculatedFields);
save(user, false, auditComment, auditUserComment, oldRecordMap, newRecordMap, oldCalculatedFields, newCalculatedFields);
}

public void save(User user, boolean allowAddBaseProperty, boolean saveOnlyIfNotExists, @Nullable String auditComment, @Nullable String auditUserComment,
public void save(User user, boolean saveOnlyIfNotExists, @Nullable String auditComment, @Nullable String auditUserComment,
@Nullable Map<String, Object> oldRecordMap, @Nullable Map<String, Object> newRecordMap,
@Nullable List<? extends GWTPropertyDescriptor> oldCalculatedFields, @Nullable List<? extends GWTPropertyDescriptor> newCalculatedFields) throws ChangePropertyDescriptorException
{
Expand Down Expand Up @@ -718,7 +718,7 @@ public void save(User user, boolean allowAddBaseProperty, boolean saveOnlyIfNotE
// make sure all properties have storageColumnName
if (null == impl._pd.getStorageColumnName())
{
if (!allowAddBaseProperty && baseProperties.contains(newPropName))
if (baseProperties.contains(newPropName))
impl._pd.setStorageColumnName(newPropName); // Issue 29047: if we allow base property (like "date"), we're later going to use the base property name for storage
else
generateStorageColumnName(impl._pd);
Expand Down Expand Up @@ -845,7 +845,7 @@ else if (null != pdOld)
{
if (!propsAdded.isEmpty())
{
StorageProvisionerImpl.get().addProperties(this, propsAdded, allowAddBaseProperty);
StorageProvisionerImpl.get().addProperties(this, propsAdded);
try
{
ensureUniqueIdValues(propsAdded);
Expand Down
4 changes: 2 additions & 2 deletions experiment/src/org/labkey/experiment/api/property/StorageProvisionerImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -364,7 +364,7 @@ public void addStorageProperties(Domain domain, Collection<PropertyStorageSpec>
change.execute();
}

public void addProperties(Domain domain, Collection<DomainProperty> properties, boolean allowAddBaseProperty)
public void addProperties(Domain domain, Collection<DomainProperty> properties)
{
DomainKind<?> kind = domain.getDomainKind();
DbScope scope = kind.getScope();
Expand All @@ -389,7 +389,7 @@ public void addProperties(Domain domain, Collection<DomainProperty> properties,
if (prop.getName() == null || prop.getName().isEmpty())
throw new IllegalArgumentException("Can't add property with no name: " + prop.getPropertyURI());

if (!allowAddBaseProperty && base.contains(prop.getName()))
if (base.contains(prop.getName()))
{
// apparently this is a case where the domain allows a propertydescriptor to be defined with the same
// name as a built-in column. e.g. to allow setting overrides?
Expand Down