Skip to content

Log error if we attempt to cache a "bad" value #6794

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 6 commits into from
Jun 28, 2025
Merged

Log error if we attempt to cache a "bad" value #6794

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
2afc5cc
Throw if we attempt to cache a "bad" value
labkey-adam Jun 25, 2025
d4714c5
Log error instead of throwing
labkey-adam Jun 25, 2025
3f82960
Hold GUID instead of Container
labkey-adam Jun 26, 2025
5b48c80
Merge remote-tracking branch 'origin/develop' into fb_throw_on_bad_ca…
labkey-adam Jun 26, 2025
d56047f
Remove stashed User from DilutionAssayRun
labkey-adam Jun 27, 2025
a618463
Update comment
labkey-adam Jun 28, 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
17 changes: 6 additions & 11 deletions api/src/org/labkey/api/cache/CacheManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ public class CacheManager
public static final int DEFAULT_CACHE_SIZE = 5000;

// Set useCache = false to completely disable all caching... and slow your server to a near halt. Possibly useful for
// reproducing CacheLoader re-entrancy problems, but not much else.
// reproducing CacheLoader reentrancy problems, but not much else.
private static final boolean useCache = true;
private static final CacheProvider PROVIDER = useCache ? EhCacheProvider.getInstance() : new NoopCacheProvider();

Expand Down Expand Up @@ -176,7 +176,7 @@ public static void shutdown()

private static final Set<Class<?>> CLASSES = new HashSet<>();

// Validate a cached value. For now, just log warnings for mutable collections.
// Validate a cached value. Log errors for mutable collections/arrays and for values holding Container or User objects.
public static <V> void validate(String debugName, @Nullable V value)
{
if (value instanceof Wrapper<?>)
Expand All @@ -186,14 +186,11 @@ public static <V> void validate(String debugName, @Nullable V value)

if (null != description)
{
LOG.warn("{} attempted to cache {}, which could be mutated by callers!", debugName, description);
LOG.error("{} attempted to cache {}, which could be mutated by callers!", debugName, description);
}

// Log questionable members, but don't do the work if we're not going to log it
if (LOG.isDebugEnabled())
{
analyzeValue(value, debugName, null, 1);
}
// Flag values that hold a Container or User object
analyzeValue(value, debugName, null, 1);
}

private static final int MAX_DEPTH = 4;
Expand Down Expand Up @@ -236,9 +233,7 @@ else if (CLASSES.add(clazz))

if (Container.class.isAssignableFrom(type) || User.class.isAssignableFrom(type) || Project.class.isAssignableFrom(type))
{
// String message = cacheName + ": " + clazz.getName() + " field " + newFieldPath + " (" + field.getType().getName() + ")";
// throw new IllegalStateException(message);
LOG.debug("{}: {} field {} ({})", cacheName, clazz.getName(), newFieldPath, field.getType().getName());
LOG.error("Cached value holds an unsafe object - {}: {} field {} ({})", cacheName, clazz.getName(), newFieldPath, field.getType().getName());
}
else
{
Expand Down
14 changes: 14 additions & 0 deletions api/src/org/labkey/api/security/ElevatedUser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

import org.labkey.api.audit.permissions.CanSeeAuditLogPermission;
import org.labkey.api.data.Container;
import org.labkey.api.security.impersonation.ImpersonationContext;
import org.labkey.api.security.impersonation.WrappedImpersonationContext;
import org.labkey.api.security.permissions.Permission;
import org.labkey.api.security.roles.CanSeeAuditLogRole;
Expand All @@ -26,6 +27,11 @@ private ElevatedUser(User user, Set<Role> rolesToAdd)
super(user, new WrappedImpersonationContext(user.getImpersonationContext(), rolesToAdd));
}

private ElevatedUser(User user, ImpersonationContext ctx)
{
super(user, ctx);
}

/**
* Wrap the supplied user and unconditionally add the supplied role(s). Always returns an ElevatedUser.
*/
Expand All @@ -43,6 +49,14 @@ public static ElevatedUser getElevatedUser(User user, Collection<Class<? extends
return new ElevatedUser(user, getRoles(roleClassesToAdd));
}

/**
* Used to reconstitute an ElevatedUser from its component parts
*/
public static ElevatedUser getElevatedUser(User user, ImpersonationContext ctx)
{
return new ElevatedUser(user, ctx);
}

/**
* Ensure the supplied user has the supplied permissions. If so, return that user. If not, wrap the user with
* ElevatedUser and, for each pair of permission + role, add the role if the user doesn't have the corresponding
Expand Down
80 changes: 47 additions & 33 deletions assay/api-src/org/labkey/api/assay/dilution/DilutionAssayRun.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,16 @@
package org.labkey.api.assay.dilution;

import org.jetbrains.annotations.Nullable;
import org.labkey.api.assay.AbstractAssayProvider;
import org.labkey.api.assay.AssayProtocolSchema;
import org.labkey.api.assay.AssayService;
import org.labkey.api.assay.dilution.query.DilutionProviderSchema;
import org.labkey.api.assay.nab.Luc5Assay;
import org.labkey.api.assay.nab.NabGraph;
import org.labkey.api.assay.nab.NabSpecimen;
import org.labkey.api.assay.nab.RenderAssayBean;
import org.labkey.api.assay.nab.view.RunDetailOptions;
import org.labkey.api.assay.plate.WellGroup;
import org.labkey.api.collections.CaseInsensitiveHashMap;
import org.labkey.api.data.ColumnInfo;
import org.labkey.api.data.Container;
Expand All @@ -42,11 +46,10 @@
import org.labkey.api.query.CustomView;
import org.labkey.api.query.FieldKey;
import org.labkey.api.query.QueryService;
import org.labkey.api.security.ElevatedUser;
import org.labkey.api.security.User;
import org.labkey.api.assay.plate.WellGroup;
import org.labkey.api.assay.AbstractAssayProvider;
import org.labkey.api.assay.AssayProtocolSchema;
import org.labkey.api.assay.AssayService;
import org.labkey.api.security.UserManager;
import org.labkey.api.security.impersonation.ImpersonationContext;
import org.labkey.api.view.NotFoundException;
import org.labkey.api.view.ViewContext;

Expand All @@ -64,35 +67,43 @@
import java.util.Set;
import java.util.TreeMap;

/**
* User: klum
* Date: 5/8/13
*/
public abstract class DilutionAssayRun extends Luc5Assay
{
protected ExpProtocol _protocol;
protected DilutionAssayProvider _provider;
protected DilutionAssayProvider<?> _provider;
protected Map<PropertyDescriptor, Object> _runProperties;
protected Map<PropertyDescriptor, Object> _runDisplayProperties;
protected List<SampleResult> _sampleResults;
protected Map<String, Object> _virusNames;
protected ExpRun _run;
// Be extremely careful to not leak this user out in any objects (e.g, via schemas or tables) as it may have elevated permissions.
protected User _user;
protected StatsService.CurveFitType _savedCurveFitType = null;
protected Map<ExpMaterial, List<WellGroup>> _materialWellGroupMapping;
protected Map<WellGroup, ExpMaterial> _wellGroupMaterialMapping;

public DilutionAssayRun(DilutionAssayProvider provider, ExpRun run,
// Objects of this class are cached (held by NAbRunWrapper), so we don't want to hold onto a User object. The
// passed in user might have elevated permissions, so stash the impersonation context along with the user ID.
private final int _userId;
private final ImpersonationContext _impersonationContext;
private final Map<FieldKey, PropertyDescriptor> _fieldKeys;

public DilutionAssayRun(DilutionAssayProvider<?> provider, ExpRun run,
User user, List<Integer> cutoffs, StatsService.CurveFitType renderCurveFitType)
{
super(run.getRowId(), cutoffs, renderCurveFitType);
_run = run;
_user = user;
_userId = user.getUserId();
_impersonationContext = user instanceof ElevatedUser eu ? eu.getImpersonationContext() : null;
_protocol = run.getProtocol();
_provider = provider;
_fieldKeys = getFieldKeys(user);

for (Map.Entry<PropertyDescriptor, Object> property : getRunProperties().entrySet())
TableInfo runTable = AssayService.get().createRunTable(_protocol, _provider, user, _run.getContainer(), null);
Map<FieldKey, ColumnInfo> cols = QueryService.get().getColumns(runTable, _fieldKeys.keySet());
Map<PropertyDescriptor, Object> runProperties = new TreeMap<>(new PropertyDescriptorComparator());
runProperties.putAll(getRunProperties(runTable, _fieldKeys, cols));
_runProperties = Collections.unmodifiableMap(runProperties);

for (Map.Entry<PropertyDescriptor, Object> property : _runProperties.entrySet())
{
if (DilutionAssayProvider.CURVE_FIT_METHOD_PROPERTY_NAME.equals(property.getKey().getName()))
{
Expand All @@ -102,7 +113,13 @@ public DilutionAssayRun(DilutionAssayProvider provider, ExpRun run,
}
}

public DilutionAssayProvider getProvider()
protected User getUser()
{
User user = UserManager.getUser(_userId);
return _impersonationContext != null ? ElevatedUser.getElevatedUser(user, _impersonationContext) : user;
}

public DilutionAssayProvider<?> getProvider()
{
return _provider;
}
Expand All @@ -119,6 +136,11 @@ public String getRunName()
}

private Map<FieldKey, PropertyDescriptor> getFieldKeys()
{
return _fieldKeys;
}

private Map<FieldKey, PropertyDescriptor> getFieldKeys(User user)
{
Map<FieldKey, PropertyDescriptor> fieldKeys = new HashMap<>();
for (DomainProperty property : _provider.getBatchDomain(_protocol).getProperties())
Expand All @@ -127,7 +149,7 @@ private Map<FieldKey, PropertyDescriptor> getFieldKeys()
fieldKeys.put(FieldKey.fromParts(property.getName()), property.getPropertyDescriptor());

// Add all of the hard columns to the set of properties we can show
TableInfo runTableInfo = AssayService.get().createRunTable(_protocol, _provider, _user, _run.getContainer(), null);
TableInfo runTableInfo = AssayService.get().createRunTable(_protocol, _provider, user, _run.getContainer(), null);
for (ColumnInfo runColumn : runTableInfo.getColumns())
{
// These columns cause an UnauthorizedException if the user has permission to see the dataset
Expand All @@ -149,7 +171,7 @@ private Map<FieldKey, PropertyDescriptor> getFieldKeys()
}
}

return fieldKeys;
return Collections.unmodifiableMap(fieldKeys);
}

public StatsService.CurveFitType getSavedCurveFitType()
Expand Down Expand Up @@ -196,7 +218,7 @@ public Map<PropertyDescriptor, Object> getRunDisplayProperties(ViewContext conte
if (_runDisplayProperties == null)
{
Map<FieldKey, PropertyDescriptor> fieldKeys = getFieldKeys();
TableInfo runTable = AssayService.get().createRunTable(_protocol, _provider, _user, _run.getContainer(), null);
TableInfo runTable = AssayService.get().createRunTable(_protocol, _provider, getUser(), _run.getContainer(), null);

CustomView runView = getRunsCustomView(context);
Collection<FieldKey> fieldKeysToShow;
Expand All @@ -211,7 +233,7 @@ public Map<PropertyDescriptor, Object> getRunDisplayProperties(ViewContext conte
fieldKeysToShow = new ArrayList<>(runTable.getDefaultVisibleColumns());
}
// The list of available columns is reduced from the default set because the user may not have
// permission to join to all of the lookups. Remove any columns that aren't part of the acceptable set,
// permission to join to all the lookups. Remove any columns that aren't part of the acceptable set,
// which is built up by getFieldKeys()
List<FieldKey> newFieldKeysToShow = new ArrayList<>();
for (FieldKey fieldKey : fieldKeysToShow)
Expand All @@ -229,9 +251,9 @@ public Map<PropertyDescriptor, Object> getRunDisplayProperties(ViewContext conte
}

Map<FieldKey, ColumnInfo> selectCols = QueryService.get().getColumns(runTable, newFieldKeysToShow);
_runDisplayProperties = getRunProperties(runTable, fieldKeys, selectCols);
_runDisplayProperties = Collections.unmodifiableMap(getRunProperties(runTable, fieldKeys, selectCols));
}
return Collections.unmodifiableMap(_runDisplayProperties);
return _runDisplayProperties;
}

protected Map<String, Map<PropertyDescriptor, Object>> getSampleProperties()
Expand All @@ -256,7 +278,7 @@ protected Map<String, DilutionResultProperties> getSampleProperties(ExpData outp
{
Map<String, DilutionResultProperties> dilutionResultPropertiesMap = new HashMap<>();

AssayProtocolSchema schema = _provider.createProtocolSchema(_user, _run.getContainer(), _protocol, null);
AssayProtocolSchema schema = _provider.createProtocolSchema(getUser(), _run.getContainer(), _protocol, null);
TableInfo virusTable = schema.createTable(DilutionManager.VIRUS_TABLE_NAME, null);

// Do a query to get all the info we need to do the linkage
Expand Down Expand Up @@ -316,23 +338,15 @@ protected CustomView getRunsCustomView(ViewContext context)

public Map<PropertyDescriptor, Object> getRunProperties()
{
if (_runProperties == null)
{
Map<FieldKey, PropertyDescriptor> fieldKeys = getFieldKeys();
TableInfo runTable = AssayService.get().createRunTable(_protocol, _provider, _user, _run.getContainer(), null);
Map<FieldKey, ColumnInfo> cols = QueryService.get().getColumns(runTable, fieldKeys.keySet());
_runProperties = new TreeMap<>(new PropertyDescriptorComparator());
_runProperties.putAll(getRunProperties(runTable, fieldKeys, cols));
}
return Collections.unmodifiableMap(_runProperties);
return _runProperties;
}

public abstract List<SampleResult> getSampleResults();

public Map<String, Object> getVirusNames()
{
if (_virusNames == null)
_virusNames = Collections.EMPTY_MAP;
_virusNames = Collections.emptyMap();
return _virusNames;
}

Expand Down Expand Up @@ -377,7 +391,7 @@ public static class SampleResult
private boolean _longCaptions = false;
private final DilutionManager _mgr = new DilutionManager();

public SampleResult(DilutionAssayProvider provider, ExpData data, DilutionSummary dilutionSummary, DilutionMaterialKey materialKey,
public SampleResult(DilutionAssayProvider<?> provider, ExpData data, DilutionSummary dilutionSummary, DilutionMaterialKey materialKey,
Map<PropertyDescriptor, Object> sampleProperties, DilutionResultProperties dilutionResultProperties)
{
_dilutionSummary = dilutionSummary;
Expand Down
11 changes: 6 additions & 5 deletions issues/src/org/labkey/issue/model/IssueListDef.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
import org.labkey.api.security.Group;
import org.labkey.api.security.SecurityManager;
import org.labkey.api.security.User;
import org.labkey.api.util.GUID;
import org.labkey.api.util.UnexpectedException;
import org.labkey.issue.query.IssueDefDomainKind;

Expand All @@ -55,7 +56,7 @@ public class IssueListDef extends Entity
private String _name;
private String _label;
private String _kind;
private Container _domainContainer;
private GUID _domainContainerId;

public int getRowId()
{
Expand Down Expand Up @@ -111,7 +112,7 @@ public TableInfo createTable(User user)
@Nullable
public Container getDomainContainer(User user)
{
if (_domainContainer == null)
if (_domainContainerId == null)
{
String id = getContainerId();
if (id != null)
Expand All @@ -125,16 +126,16 @@ public Container getDomainContainer(User user)
// create the domain in the current container
if (domain != null)
{
_domainContainer = domain.getContainer();
_domainContainerId = domain.getContainer().getEntityId();
}
else
{
_domainContainer = container;
_domainContainerId = container.getEntityId();
}
}
}
}
return _domainContainer;
return ContainerManager.getForId(_domainContainerId);
}

public Domain getDomain(User user)
Expand Down