From 19fd44e3920dea17abf2a3505cf11ccb13e9d7ae Mon Sep 17 00:00:00 2001 From: Adam Rauch Date: Mon, 12 May 2025 12:31:43 -0700 Subject: [PATCH 1/4] Inspect cached objects for questionable members --- .../org/labkey/api/cache/CacheManager.java | 52 ++++++++++++++++--- .../org/labkey/api/util/ContainerContext.java | 7 +-- 2 files changed, 46 insertions(+), 13 deletions(-) diff --git a/api/src/org/labkey/api/cache/CacheManager.java b/api/src/org/labkey/api/cache/CacheManager.java index 4a0107dd0fb..ae7d71a9812 100644 --- a/api/src/org/labkey/api/cache/CacheManager.java +++ b/api/src/org/labkey/api/cache/CacheManager.java @@ -20,20 +20,23 @@ import org.jetbrains.annotations.Nullable; import org.labkey.api.cache.ehcache.EhCacheProvider; import org.labkey.api.collections.CollectionUtils; +import org.labkey.api.data.Container; +import org.labkey.api.data.Project; import org.labkey.api.mbean.LabKeyManagement; +import org.labkey.api.security.User; import org.labkey.api.util.logging.LogHelper; +import java.lang.reflect.Field; +import java.lang.reflect.Modifier; import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collections; +import java.util.HashSet; import java.util.LinkedList; import java.util.List; +import java.util.Set; import java.util.concurrent.CopyOnWriteArrayList; -/** - * User: adam - * Date: Jun 20, 2010 - * Time: 10:05:07 AM - */ - public class CacheManager { private static final Logger LOG = LogHelper.getLogger(CacheManager.class, "General cache information"); @@ -112,7 +115,7 @@ private static void addToKnownCaches(TrackingCache cache) { KNOWN_CACHES.add(cache); - LOG.debug("Known caches: " + KNOWN_CACHES.size()); + LOG.trace("Known caches: {}", KNOWN_CACHES.size()); } } @@ -165,6 +168,8 @@ public static void shutdown() PROVIDER.shutdown(); } + private static final Set MESSAGES = new HashSet<>(); + // Validate a cached value. For now, just log warnings for mutable collections. public static void validate(String debugName, @Nullable V value) { @@ -175,10 +180,41 @@ public static void validate(String debugName, @Nullable V value) if (null != description) { - LOG.warn(debugName + " attempted to cache " + description + ", which could be mutated by callers!"); + LOG.warn("{} 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() && value != null) + { + for (Field field : getAllInstanceFields(value.getClass())) + { + // TODO: Should also look for collections and arrays, and inspect the first element + Class type = field.getType(); + if (Container.class.isAssignableFrom(type) || User.class.isAssignableFrom(type) || Project.class.isAssignableFrom(type) || type.getName().contains("Reference")) + { + String message = String.format("%s: %s field %s: %s", debugName, value.getClass().getName(), field.getName(), field.getType().getName()); + if (MESSAGES.add(message)) + LOG.debug(message); + } + } } } + // Recurse up the class hierarchy + private static List getAllInstanceFields(Class clazz) + { + if (clazz == null) + { + return Collections.emptyList(); + } + + List result = new ArrayList<>(getAllInstanceFields(clazz.getSuperclass())); + List filteredFields = Arrays.stream(clazz.getDeclaredFields()) + .filter(field -> !Modifier.isStatic(field.getModifiers())) + .toList(); + result.addAll(filteredFields); + return result; + } /* This interface allows a Collection to declare itself immutable when being added to a cache */ public interface Sealable diff --git a/api/src/org/labkey/api/util/ContainerContext.java b/api/src/org/labkey/api/util/ContainerContext.java index d500779f359..b80525eb1c7 100644 --- a/api/src/org/labkey/api/util/ContainerContext.java +++ b/api/src/org/labkey/api/util/ContainerContext.java @@ -24,11 +24,8 @@ import java.util.Map; /** - * User: matthewb - * Date: Sep 27, 2009 - * - * This is a helper class for DetailsURL. Rather than needing to subclass DetailsURL to provide a - * container value, you may provide a ContainerContext instead. + * This is a helper class for DetailsURL. Rather than needing to subclass DetailsURL to provide a container value, + * you may provide a ContainerContext instead. */ public interface ContainerContext { From 4036c78f8555c99e5165404f2ccdb1afbcc1e800 Mon Sep 17 00:00:00 2001 From: Adam Rauch Date: Mon, 12 May 2025 13:20:29 -0700 Subject: [PATCH 2/4] PropertyDescriptor: stop holding Container objects --- .../org/labkey/api/cache/CacheManager.java | 2 +- .../org/labkey/api/exp/DomainDescriptor.java | 2 +- .../org/labkey/api/exp/OntologyManager.java | 2 -- .../labkey/api/exp/PropertyDescriptor.java | 26 ++++++++----------- .../api/assay/dilution/DilutionAssayRun.java | 1 - .../study/assay/StudyPublishManager.java | 1 - .../org/labkey/study/model/StudyManager.java | 1 - 7 files changed, 13 insertions(+), 22 deletions(-) diff --git a/api/src/org/labkey/api/cache/CacheManager.java b/api/src/org/labkey/api/cache/CacheManager.java index ae7d71a9812..3a5b1b4612b 100644 --- a/api/src/org/labkey/api/cache/CacheManager.java +++ b/api/src/org/labkey/api/cache/CacheManager.java @@ -188,7 +188,7 @@ public static void validate(String debugName, @Nullable V value) { for (Field field : getAllInstanceFields(value.getClass())) { - // TODO: Should also look for collections and arrays, and inspect the first element + // TODO: Should also look for collections and arrays, inspecting the first element Class type = field.getType(); if (Container.class.isAssignableFrom(type) || User.class.isAssignableFrom(type) || Project.class.isAssignableFrom(type) || type.getName().contains("Reference")) { diff --git a/api/src/org/labkey/api/exp/DomainDescriptor.java b/api/src/org/labkey/api/exp/DomainDescriptor.java index d7f3aa5ac64..2a9e0b0aca6 100644 --- a/api/src/org/labkey/api/exp/DomainDescriptor.java +++ b/api/src/org/labkey/api/exp/DomainDescriptor.java @@ -200,7 +200,7 @@ public String getDomainURI() public Container getProject() { var c = getContainer(); - return null==c ? null : c.getProject(); + return null == c ? null : c.getProject(); } public int getTitlePropertyId() diff --git a/api/src/org/labkey/api/exp/OntologyManager.java b/api/src/org/labkey/api/exp/OntologyManager.java index 9bb15813220..152597bb6b2 100644 --- a/api/src/org/labkey/api/exp/OntologyManager.java +++ b/api/src/org/labkey/api/exp/OntologyManager.java @@ -1455,7 +1455,6 @@ private static void copyDescriptors(final Container c, final Container project) PROP_DESCRIPTOR_CACHE.remove(getCacheKey(pd)); DOMAIN_PROPERTIES_CACHE.clear(); pd.setContainer(project); - pd.setProject(project); pd.setPropertyId(0); pd = ensurePropertyDescriptor(pd); } @@ -1640,7 +1639,6 @@ public static void moveContainer(@NotNull final Container c, @NotNull Container if (!pd.getContainer().equals(c) || !pd.getProject().equals(fNewProject)) { pd.setContainer(c); - pd.setProject(fNewProject); pd.setPropertyId(0); } diff --git a/api/src/org/labkey/api/exp/PropertyDescriptor.java b/api/src/org/labkey/api/exp/PropertyDescriptor.java index 6db6245f25b..20d55c932a9 100644 --- a/api/src/org/labkey/api/exp/PropertyDescriptor.java +++ b/api/src/org/labkey/api/exp/PropertyDescriptor.java @@ -15,7 +15,6 @@ */ package org.labkey.api.exp; -import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; @@ -42,9 +41,11 @@ import org.labkey.api.query.PdLookupForeignKey; import org.labkey.api.query.UserSchema; import org.labkey.api.security.User; +import org.labkey.api.util.GUID; import org.labkey.api.util.PageFlowUtil; import org.labkey.api.util.StringExpression; import org.labkey.api.util.UnexpectedException; +import org.labkey.api.util.logging.LogHelper; import java.io.Serializable; import java.util.Collection; @@ -64,8 +65,7 @@ public class PropertyDescriptor extends ColumnRenderPropertiesImpl implements Pa private String _name; private String _storageColumnName; private int _propertyId; - private Container _container; - private Container _project; + private GUID _containerId; private String _lookupContainer; private String _lookupSchema; private String _lookupQuery; @@ -73,7 +73,7 @@ public class PropertyDescriptor extends ColumnRenderPropertiesImpl implements Pa private String _mvIndicatorStorageColumnName; // only valid if mvEnabled private Object _databaseDefaultValue; - private static final Logger LOG = LogManager.getLogger(PropertyDescriptor.class); + private static final Logger LOG = LogHelper.getLogger(PropertyDescriptor.class, "Property type warnings"); @Override public void checkLocked() @@ -289,31 +289,28 @@ public void setDefaultValueType(String defaultValueTypeName) @Override public String toString() { - return _propertyURI + " name=" + _name + " project="+ (_project == null ? "null" : _project.getPath()) + " container="+ (_container ==null ? "null" : _container.getPath()) + " label=" + _label + " range=" + _rangeURI + " concept=" + _conceptURI; + return _propertyURI + " name=" + _name + " project="+ (getProject() == null ? "null" : getProject().getPath()) + " container="+ (getContainer() == null ? "null" : getContainer().getPath()) + " label=" + _label + " range=" + _rangeURI + " concept=" + _conceptURI; } public Container getContainer() { - return _container; + return ContainerManager.getForId(_containerId); } public void setContainer(Container container) { - _container = container; - if (null== _project) - _project =container.getProject(); - if (null== _project) - _project =container; + _containerId = container.getEntityId(); } public Container getProject() { - return _project; + var c = getContainer(); + return null == c ? null : c.getProject(); } public void setProject(Container proj) { - _project = proj; + // No-op - project is determined by _containerId } @NotNull @@ -437,8 +434,7 @@ public void copyTo(ColumnRenderPropertiesImpl to) if (to instanceof PropertyDescriptor) { PropertyDescriptor toPD = (PropertyDescriptor)to; - toPD._container = _container; // ? - toPD._project = _project; // ? + toPD._containerId = _containerId; // ? toPD._lookupContainer = _lookupContainer; toPD._lookupSchema = _lookupSchema; toPD._lookupQuery = _lookupQuery; diff --git a/assay/api-src/org/labkey/api/assay/dilution/DilutionAssayRun.java b/assay/api-src/org/labkey/api/assay/dilution/DilutionAssayRun.java index 6e33258c921..33267a27ce1 100644 --- a/assay/api-src/org/labkey/api/assay/dilution/DilutionAssayRun.java +++ b/assay/api-src/org/labkey/api/assay/dilution/DilutionAssayRun.java @@ -145,7 +145,6 @@ private Map getFieldKeys() pd.setLabel(runColumn.getLabel()); pd.setPropertyURI(runColumn.getPropertyURI()); pd.setContainer(_protocol.getContainer()); - pd.setProject(_protocol.getContainer().getProject()); fieldKeys.put(FieldKey.fromParts(runColumn.getName()), pd); } } diff --git a/study/src/org/labkey/study/assay/StudyPublishManager.java b/study/src/org/labkey/study/assay/StudyPublishManager.java index 41818beb073..694d2e80881 100644 --- a/study/src/org/labkey/study/assay/StudyPublishManager.java +++ b/study/src/org/labkey/study/assay/StudyPublishManager.java @@ -221,7 +221,6 @@ private List createTargetPropertyDescriptors(Dataset dataset targetPd.setPropertyURI(dataset.getTypeURI() + "#" + sourcePd.getName()); targetPd.setContainer(dataset.getContainer()); - targetPd.setProject(dataset.getContainer().getProject()); if (targetPd.getLookupQuery() != null) targetPd.setLookupContainer(sourcePd.getLookupContainer()); // set the ID to zero so it's clear that this is a new property descriptor: diff --git a/study/src/org/labkey/study/model/StudyManager.java b/study/src/org/labkey/study/model/StudyManager.java index 6f7e1094c5b..54aea179175 100644 --- a/study/src/org/labkey/study/model/StudyManager.java +++ b/study/src/org/labkey/study/model/StudyManager.java @@ -3776,7 +3776,6 @@ private void buildPropertySaveAndDeleteLists(Map { p.getPropertyDescriptor().setPropertyId(0); p.getPropertyDescriptor().setContainer(ipd.pd.getContainer()); - p.getPropertyDescriptor().setProject(ipd.pd.getProject()); } } else From 62ed5a91fa1c41f6f17ddfc40c3392e88951d152 Mon Sep 17 00:00:00 2001 From: Adam Rauch Date: Thu, 15 May 2025 21:22:35 -0700 Subject: [PATCH 3/4] Inspect maps, collections, arrays, and references. Recurse into members at a max depth. --- .../org/labkey/api/cache/CacheManager.java | 77 ++++++++++++++++--- 1 file changed, 68 insertions(+), 9 deletions(-) diff --git a/api/src/org/labkey/api/cache/CacheManager.java b/api/src/org/labkey/api/cache/CacheManager.java index 3a5b1b4612b..b1a9d2f9df2 100644 --- a/api/src/org/labkey/api/cache/CacheManager.java +++ b/api/src/org/labkey/api/cache/CacheManager.java @@ -26,14 +26,19 @@ import org.labkey.api.security.User; import org.labkey.api.util.logging.LogHelper; +import java.lang.ref.Reference; +import java.lang.reflect.Array; import java.lang.reflect.Field; +import java.lang.reflect.InaccessibleObjectException; import java.lang.reflect.Modifier; import java.util.ArrayList; import java.util.Arrays; +import java.util.Collection; import java.util.Collections; import java.util.HashSet; import java.util.LinkedList; import java.util.List; +import java.util.Map; import java.util.Set; import java.util.concurrent.CopyOnWriteArrayList; @@ -168,7 +173,7 @@ public static void shutdown() PROVIDER.shutdown(); } - private static final Set MESSAGES = new HashSet<>(); + private static final Set> CLASSES = new HashSet<>(); // Validate a cached value. For now, just log warnings for mutable collections. public static void validate(String debugName, @Nullable V value) @@ -184,17 +189,71 @@ public static void validate(String debugName, @Nullable V value) } // Log questionable members, but don't do the work if we're not going to log it - if (LOG.isDebugEnabled() && value != null) + if (LOG.isDebugEnabled()) { - for (Field field : getAllInstanceFields(value.getClass())) + analyzeValue(value, debugName, null, 1); + } + } + + private static final int MAX_DEPTH = 4; + + private static void analyzeValue(@Nullable Object value, String cacheName, @Nullable String fields, int depth) + { + if (value != null && depth < MAX_DEPTH) + { + Class clazz = value.getClass(); + + if (value instanceof Map m) + { + m.entrySet().stream() + .findFirst() + .ifPresent(entry -> { + analyzeValue(entry.getKey(), cacheName, fields, depth + 1); + analyzeValue(entry.getValue(), cacheName, fields, depth + 1); + }); + } + else if (value instanceof Collection coll) + { + coll.stream() + .findFirst() + .ifPresent(element -> analyzeValue(element, cacheName, fields, depth + 1)); + } + else if (clazz.isArray() && Array.getLength(value) > 0) + { + analyzeValue(Array.get(value, 0), cacheName, fields, depth + 1); + } + else if (value instanceof Reference ref) + { + analyzeValue(ref, cacheName, fields, depth + 1); + } + else if (CLASSES.add(clazz)) { - // TODO: Should also look for collections and arrays, inspecting the first element - Class type = field.getType(); - if (Container.class.isAssignableFrom(type) || User.class.isAssignableFrom(type) || Project.class.isAssignableFrom(type) || type.getName().contains("Reference")) + for (Field field : getAllInstanceFields(clazz)) { - String message = String.format("%s: %s field %s: %s", debugName, value.getClass().getName(), field.getName(), field.getType().getName()); - if (MESSAGES.add(message)) - LOG.debug(message); + String fieldPath = (fields != null ? fields + "." : "") + field.getName(); + Class type = field.getType(); + + if (Container.class.isAssignableFrom(type) || User.class.isAssignableFrom(type) || Project.class.isAssignableFrom(type)) + { + LOG.debug("{}: {} field {}: {}", cacheName, clazz.getName(), fieldPath, field.getType().getName()); + } + else + { + try + { + field.setAccessible(true); + Object val = field.get(value); + analyzeValue(val, cacheName, fieldPath, depth + 1); + } + catch (InaccessibleObjectException e) + { + LOG.debug("Inaccessible member {}", fieldPath); + } + catch (IllegalAccessException e) + { + LOG.debug("Exception attempting to access {}", fieldPath, e); + } + } } } } From de8557cb4865c7f989770c434a4780635dcbb470 Mon Sep 17 00:00:00 2001 From: Adam Rauch Date: Mon, 19 May 2025 15:34:08 -0700 Subject: [PATCH 4/4] Improve logging --- api/src/org/labkey/api/cache/CacheManager.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/api/src/org/labkey/api/cache/CacheManager.java b/api/src/org/labkey/api/cache/CacheManager.java index b1a9d2f9df2..53486329c38 100644 --- a/api/src/org/labkey/api/cache/CacheManager.java +++ b/api/src/org/labkey/api/cache/CacheManager.java @@ -235,7 +235,7 @@ else if (CLASSES.add(clazz)) if (Container.class.isAssignableFrom(type) || User.class.isAssignableFrom(type) || Project.class.isAssignableFrom(type)) { - LOG.debug("{}: {} field {}: {}", cacheName, clazz.getName(), fieldPath, field.getType().getName()); + LOG.debug("{}: {} field {} ({})", cacheName, clazz.getName(), fieldPath, field.getType().getName()); } else { @@ -247,11 +247,11 @@ else if (CLASSES.add(clazz)) } catch (InaccessibleObjectException e) { - LOG.debug("Inaccessible member {}", fieldPath); + LOG.debug("{}: {} field {} ({}): inaccessible member", cacheName, clazz.getName(), fieldPath, field.getType().getName()); } catch (IllegalAccessException e) { - LOG.debug("Exception attempting to access {}", fieldPath, e); + LOG.debug("{}: {} field {} ({}): exception attempting to access", cacheName, clazz.getName(), fieldPath, field.getType().getName(), e); } } }