From 8e13fd2d4ded633b1f487bf1d5b0048b6e08f2d8 Mon Sep 17 00:00:00 2001 From: Adam Rauch Date: Fri, 11 Sep 2026 07:49:45 -0700 Subject: [PATCH 1/3] Optimize full-text-search SecurityQuery --- .../org/labkey/api/search/SearchService.java | 19 +-- assay/src/org/labkey/assay/AssayManager.java | 13 +- .../org/labkey/assay/plate/PlateManager.java | 17 +- .../experiment/api/ExpDataClassImpl.java | 9 +- .../labkey/experiment/api/ExpDataImpl.java | 8 +- .../experiment/api/ExpMaterialImpl.java | 5 +- .../experiment/api/ExpSampleTypeImpl.java | 5 +- .../src/org/labkey/search/SearchModule.java | 3 +- .../labkey/search/model/SecurityQuery.java | 157 +++++++++++++++++- 9 files changed, 185 insertions(+), 51 deletions(-) diff --git a/api/src/org/labkey/api/search/SearchService.java b/api/src/org/labkey/api/search/SearchService.java index 874b22cbe3f..f8cfce69be8 100644 --- a/api/src/org/labkey/api/search/SearchService.java +++ b/api/src/org/labkey/api/search/SearchService.java @@ -305,19 +305,14 @@ public String toString() return _name; } - protected Set getPermittedContainerIds(User user, Map containers, @NotNull Class perm) - { - Set containerIds = new HashSet<>(); - containers.forEach((id, container) -> { - if (container.hasPermission(user, perm)) - containerIds.add(id); - }); - return containerIds.size() == containers.size() ? containers.keySet() : containerIds; - } - - public Set getPermittedContainerIds(User user, Map containers) + /** + * Permission required, beyond base container Read (which every searchable container already has), for this + * category's documents to be visible. Return null if base Read is sufficient. + */ + @Nullable + public Class getRequiredPermission() { - return containers.keySet(); + return null; } public boolean isShowInAdvancedSearch() diff --git a/assay/src/org/labkey/assay/AssayManager.java b/assay/src/org/labkey/assay/AssayManager.java index 9219ae1f5c8..9d3eb6ba5a2 100644 --- a/assay/src/org/labkey/assay/AssayManager.java +++ b/assay/src/org/labkey/assay/AssayManager.java @@ -76,6 +76,7 @@ import org.labkey.api.security.User; import org.labkey.api.security.permissions.AssayReadPermission; import org.labkey.api.security.permissions.InsertPermission; +import org.labkey.api.security.permissions.Permission; import org.labkey.api.settings.AppProps; import org.labkey.api.study.assay.ParticipantVisitResolver; import org.labkey.api.study.assay.ParticipantVisitResolverType; @@ -120,23 +121,23 @@ public class AssayManager implements AssayService { SearchService.SearchCategory ASSAY_CATEGORY = new SearchService.SearchCategory("assay", "Assays") { @Override - public Set getPermittedContainerIds(User user, Map containers) + public Class getRequiredPermission() { - return getPermittedContainerIds(user, containers, AssayReadPermission.class); + return AssayReadPermission.class; } }; SearchService.SearchCategory ASSAY_BATCH_CATEGORY = new SearchService.SearchCategory("assayBatch", "Assay Batches", false) { @Override - public Set getPermittedContainerIds(User user, Map containers) + public Class getRequiredPermission() { - return getPermittedContainerIds(user, containers, AssayReadPermission.class); + return AssayReadPermission.class; } }; SearchService.SearchCategory ASSAY_RUN_CATEGORY = new SearchService.SearchCategory("assayRun", "Assay Runs", false) { @Override - public Set getPermittedContainerIds(User user, Map containers) + public Class getRequiredPermission() { - return getPermittedContainerIds(user, containers, AssayReadPermission.class); + return AssayReadPermission.class; } }; diff --git a/assay/src/org/labkey/assay/plate/PlateManager.java b/assay/src/org/labkey/assay/plate/PlateManager.java index 39c29f1c289..913675120e6 100644 --- a/assay/src/org/labkey/assay/plate/PlateManager.java +++ b/assay/src/org/labkey/assay/plate/PlateManager.java @@ -220,21 +220,10 @@ public class PlateManager implements PlateService, AssayListener, ExperimentList // when those calls are being made for a plate save operation. public static final String PLATE_SAVE_FLAG = ".plateSave"; - public SearchService.SearchCategory PLATE_CATEGORY = new SearchService.SearchCategory("plate", "Assay Plates", false) { - @Override - public Set getPermittedContainerIds(User user, Map containers) - { - return getPermittedContainerIds(user, containers, ReadPermission.class); - } - }; + // No override needed: base container Read (already required to reach this category's containers) is sufficient. + public SearchService.SearchCategory PLATE_CATEGORY = new SearchService.SearchCategory("plate", "Assay Plates", false); - public SearchService.SearchCategory PLATE_SET_CATEGORY = new SearchService.SearchCategory("plateSet", "Assay Plate Sets", false) { - @Override - public Set getPermittedContainerIds(User user, Map containers) - { - return getPermittedContainerIds(user, containers, ReadPermission.class); - } - }; + public SearchService.SearchCategory PLATE_SET_CATEGORY = new SearchService.SearchCategory("plateSet", "Assay Plate Sets", false); public static PlateManager get() { diff --git a/experiment/src/org/labkey/experiment/api/ExpDataClassImpl.java b/experiment/src/org/labkey/experiment/api/ExpDataClassImpl.java index b1984980873..c19d2456587 100644 --- a/experiment/src/org/labkey/experiment/api/ExpDataClassImpl.java +++ b/experiment/src/org/labkey/experiment/api/ExpDataClassImpl.java @@ -48,6 +48,7 @@ import org.labkey.api.security.User; import org.labkey.api.security.permissions.DataClassReadPermission; import org.labkey.api.security.permissions.MediaReadPermission; +import org.labkey.api.security.permissions.Permission; import org.labkey.api.util.PageFlowUtil; import org.labkey.api.util.Path; import org.labkey.api.util.UnexpectedException; @@ -76,16 +77,16 @@ public class ExpDataClassImpl extends ExpIdentifiableEntityImpl imple private static final String MEDIA_SEARCH_CATEGORY_NAME = "media"; public static final SearchService.SearchCategory SEARCH_CATEGORY = new SearchService.SearchCategory(SEARCH_CATEGORY_NAME, "Collections of data objects", false) { @Override - public Set getPermittedContainerIds(User user, Map containers) + public Class getRequiredPermission() { - return getPermittedContainerIds(user, containers, DataClassReadPermission.class); + return DataClassReadPermission.class; } }; public static final SearchService.SearchCategory MEDIA_SEARCH_CATEGORY = new SearchService.SearchCategory(MEDIA_SEARCH_CATEGORY_NAME, "Collections of media data and samples", false) { @Override - public Set getPermittedContainerIds(User user, Map containers) + public Class getRequiredPermission() { - return getPermittedContainerIds(user, containers, MediaReadPermission.class); + return MediaReadPermission.class; } }; diff --git a/experiment/src/org/labkey/experiment/api/ExpDataImpl.java b/experiment/src/org/labkey/experiment/api/ExpDataImpl.java index eb38273ddc2..b8306ad25cc 100644 --- a/experiment/src/org/labkey/experiment/api/ExpDataImpl.java +++ b/experiment/src/org/labkey/experiment/api/ExpDataImpl.java @@ -133,16 +133,16 @@ public Class getPermissionClass() public static final SearchService.SearchCategory expDataCategory = new SearchService.SearchCategory("data", "ExpData", false) { @Override - public Set getPermittedContainerIds(User user, Map containers) + public Class getRequiredPermission() { - return getPermittedContainerIds(user, containers, DataClassReadPermission.class); + return DataClassReadPermission.class; } }; public static final SearchService.SearchCategory expMediaDataCategory = new SearchService.SearchCategory("mediaData", "ExpData for media objects", false) { @Override - public Set getPermittedContainerIds(User user, Map containers) + public Class getRequiredPermission() { - return getPermittedContainerIds(user, containers, MediaReadPermission.class); + return MediaReadPermission.class; } }; diff --git a/experiment/src/org/labkey/experiment/api/ExpMaterialImpl.java b/experiment/src/org/labkey/experiment/api/ExpMaterialImpl.java index 152dd004add..192683c66fc 100644 --- a/experiment/src/org/labkey/experiment/api/ExpMaterialImpl.java +++ b/experiment/src/org/labkey/experiment/api/ExpMaterialImpl.java @@ -59,6 +59,7 @@ import org.labkey.api.search.SearchService; import org.labkey.api.security.User; import org.labkey.api.security.permissions.MediaReadPermission; +import org.labkey.api.security.permissions.Permission; import org.labkey.api.study.StudyService; import org.labkey.api.util.JobRunner; import org.labkey.api.util.PageFlowUtil; @@ -88,9 +89,9 @@ public class ExpMaterialImpl extends AbstractRunItemImpl implements Ex public static final SearchService.SearchCategory searchCategory = new SearchService.SearchCategory("material", "Materials/Samples", false); public static final SearchService.SearchCategory mediaSearchCategory = new SearchService.SearchCategory("media", "Media Samples", false){ @Override - public Set getPermittedContainerIds(User user, Map containers) + public Class getRequiredPermission() { - return getPermittedContainerIds(user, containers, MediaReadPermission.class); + return MediaReadPermission.class; } }; diff --git a/experiment/src/org/labkey/experiment/api/ExpSampleTypeImpl.java b/experiment/src/org/labkey/experiment/api/ExpSampleTypeImpl.java index 1638e121f84..36acf398e88 100644 --- a/experiment/src/org/labkey/experiment/api/ExpSampleTypeImpl.java +++ b/experiment/src/org/labkey/experiment/api/ExpSampleTypeImpl.java @@ -63,6 +63,7 @@ import org.labkey.api.search.SearchService; import org.labkey.api.security.User; import org.labkey.api.security.permissions.MediaReadPermission; +import org.labkey.api.security.permissions.Permission; import org.labkey.api.study.StudyService; import org.labkey.api.util.PageFlowUtil; import org.labkey.api.util.Path; @@ -97,9 +98,9 @@ public class ExpSampleTypeImpl extends ExpIdentifiableEntityImpl public static final SearchService.SearchCategory searchCategory = new SearchService.SearchCategory(categoryName, "Sample Types", false); public static final SearchService.SearchCategory mediaSearchCategory = new SearchService.SearchCategory(mediaCategoryName, "Media Sample Types", false) { @Override - public Set getPermittedContainerIds(User user, Map containers) + public Class getRequiredPermission() { - return getPermittedContainerIds(user, containers, MediaReadPermission.class); + return MediaReadPermission.class; } }; diff --git a/search/src/org/labkey/search/SearchModule.java b/search/src/org/labkey/search/SearchModule.java index 69e652a08a1..529e7abdfb9 100644 --- a/search/src/org/labkey/search/SearchModule.java +++ b/search/src/org/labkey/search/SearchModule.java @@ -58,6 +58,7 @@ import org.labkey.search.model.PlainTextDocumentParser; import org.labkey.search.model.SearchSchema; import org.labkey.search.model.SearchStartupProperties; +import org.labkey.search.model.SecurityQuery; import org.labkey.search.view.SearchWebPartFactory; import javax.management.StandardMBean; @@ -259,7 +260,7 @@ private void reindexIfNeeded(@NotNull SearchService ss) @Override public @NotNull Set> getUnitTests() { - return Set.of(AbstractSearchService.TestCase.class); + return Set.of(AbstractSearchService.TestCase.class, SecurityQuery.TestCase.class); } @Override diff --git a/search/src/org/labkey/search/model/SecurityQuery.java b/search/src/org/labkey/search/model/SecurityQuery.java index 074fda60af5..20fc953eab2 100644 --- a/search/src/org/labkey/search/model/SecurityQuery.java +++ b/search/src/org/labkey/search/model/SecurityQuery.java @@ -16,6 +16,8 @@ package org.labkey.search.model; +import org.apache.commons.collections4.MultiValuedMap; +import org.apache.commons.collections4.multimap.ArrayListValuedHashMap; import org.apache.commons.lang3.StringUtils; import org.apache.lucene.index.BinaryDocValues; import org.apache.lucene.index.LeafReader; @@ -34,25 +36,33 @@ import org.apache.lucene.util.FixedBitSet; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; +import org.junit.Assert; +import org.junit.Test; import org.labkey.api.data.Container; import org.labkey.api.data.ContainerManager; import org.labkey.api.module.Module; import org.labkey.api.search.SearchScope; import org.labkey.api.search.SearchService; import org.labkey.api.security.SecurableResource; +import org.labkey.api.security.SecurityManager; import org.labkey.api.security.User; +import org.labkey.api.security.permissions.DeletePermission; +import org.labkey.api.security.permissions.InsertPermission; +import org.labkey.api.security.permissions.Permission; import org.labkey.api.security.permissions.ReadPermission; import org.labkey.api.util.MultiPhaseCPUTimer.InvocationTimer; import org.labkey.search.model.LuceneSearchServiceImpl.FIELD_NAME; import java.io.IOException; +import java.util.Collection; import java.util.HashMap; +import java.util.HashSet; import java.util.List; import java.util.Set; import static org.apache.lucene.search.DocIdSetIterator.NO_MORE_DOCS; -class SecurityQuery extends Query +public class SecurityQuery extends Query { private final User _user; private final Container _currentContainer; @@ -75,11 +85,72 @@ class SecurityQuery extends Query _containerIds = searchScope.getSearchableContainers(user, currentContainer); - SearchService.get().getSearchCategories().forEach( - category -> { - _categoryContainers.put(category.getName(), category.getPermittedContainerIds(user, _containerIds)); - } - ); + // Categories that require only base container Read (already guaranteed for every container above) are + // resolved directly; the rest are grouped by required permission so multiple categories that require the + // same permission (e.g., the three assay categories all require AssayReadPermission) share a single + // O(containers) assembly pass below instead of each redoing it. + Set baseReadCategoryNames = new HashSet<>(); + MultiValuedMap, SearchService.SearchCategory> categoriesByPermission = + groupCategoriesByRequiredPermission(SearchService.get().getSearchCategories(), baseReadCategoryNames); + + for (String categoryName : baseReadCategoryNames) + _categoryContainers.put(categoryName, _containerIds.keySet()); + + // Containers that inherit their policy (e.g., workbooks, which typically don't have their own explicit + // policy) share the exact same SecurityPolicy object as their nearest ancestor with one. Role resolution + // (SecurityManager.getPermissions()) is therefore identical for every container backed by the same policy, + // so compute it once per distinct policy instead of once per container per category. A user's full granted + // permission set can be large (100+ for a site admin), but categories only ever ask about a handful of + // permission classes, so retain just those instead of holding the full set for every distinct policy. + Set> requiredPermissions = categoriesByPermission.keySet(); + HashMap>> permissionsByPolicy = new HashMap<>(); + + if (!requiredPermissions.isEmpty()) + { + for (Container c : _containerIds.values()) + { + permissionsByPolicy.computeIfAbsent(c.getPolicy().getResourceId(), id -> { + Set> permitted = new HashSet<>(requiredPermissions); + permitted.retainAll(SecurityManager.getPermissions(c, user, null)); + return permitted; + }); + } + } + + categoriesByPermission.asMap().forEach((requiredPermission, categories) -> { + Set permittedContainerIds = new HashSet<>(); + + for (var entry : _containerIds.entrySet()) + { + if (permissionsByPolicy.get(entry.getValue().getPolicy().getResourceId()).contains(requiredPermission)) + permittedContainerIds.add(entry.getKey()); + } + + for (SearchService.SearchCategory category : categories) + _categoryContainers.put(category.getName(), permittedContainerIds); + }); + } + + /** + * Splits categories into those requiring only base container Read (their names are added to baseReadCategoryNames) + * and those requiring a specific permission, which are grouped by that permission class. + */ + static MultiValuedMap, SearchService.SearchCategory> groupCategoriesByRequiredPermission( + Collection categories, Set baseReadCategoryNames) + { + MultiValuedMap, SearchService.SearchCategory> categoriesByPermission = new ArrayListValuedHashMap<>(); + + for (SearchService.SearchCategory category : categories) + { + Class requiredPermission = category.getRequiredPermission(); + + if (null == requiredPermission) + baseReadCategoryNames.add(category.getName()); + else + categoriesByPermission.put(requiredPermission, category); + } + + return categoriesByPermission; } @Override @@ -314,4 +385,78 @@ public boolean mayInheritPolicy() return false; } } + + public static class TestCase extends Assert + { + private static SearchService.SearchCategory categoryRequiring(String name, Class requiredPermission) + { + return new SearchService.SearchCategory(name, name, false) + { + @Override + public Class getRequiredPermission() + { + return requiredPermission; + } + }; + } + + @Test + public void testCategoryWithNoRequiredPermissionGoesToBaseRead() + { + SearchService.SearchCategory wiki = new SearchService.SearchCategory("wiki", "Wiki Pages"); + Set baseReadCategoryNames = new HashSet<>(); + + MultiValuedMap, SearchService.SearchCategory> categoriesByPermission = + SecurityQuery.groupCategoriesByRequiredPermission(List.of(wiki), baseReadCategoryNames); + + assertEquals(Set.of("wiki"), baseReadCategoryNames); + assertTrue(categoriesByPermission.isEmpty()); + } + + @Test + public void testCategoriesSharingAPermissionAreGroupedTogether() + { + // Mirrors the real assay/assayBatch/assayRun categories, which all require the same permission. + SearchService.SearchCategory assay = categoryRequiring("assay", InsertPermission.class); + SearchService.SearchCategory assayBatch = categoryRequiring("assayBatch", InsertPermission.class); + SearchService.SearchCategory assayRun = categoryRequiring("assayRun", InsertPermission.class); + Set baseReadCategoryNames = new HashSet<>(); + + MultiValuedMap, SearchService.SearchCategory> categoriesByPermission = + SecurityQuery.groupCategoriesByRequiredPermission(List.of(assay, assayBatch, assayRun), baseReadCategoryNames); + + assertTrue(baseReadCategoryNames.isEmpty()); + assertEquals(Set.of(InsertPermission.class), categoriesByPermission.keySet()); + assertEquals(Set.of(assay, assayBatch, assayRun), Set.copyOf(categoriesByPermission.get(InsertPermission.class))); + } + + @Test + public void testCategoriesWithDifferentPermissionsAreNotGroupedTogether() + { + SearchService.SearchCategory data = categoryRequiring("data", InsertPermission.class); + SearchService.SearchCategory media = categoryRequiring("media", DeletePermission.class); + Set baseReadCategoryNames = new HashSet<>(); + + MultiValuedMap, SearchService.SearchCategory> categoriesByPermission = + SecurityQuery.groupCategoriesByRequiredPermission(List.of(data, media), baseReadCategoryNames); + + assertEquals(Set.of(InsertPermission.class, DeletePermission.class), categoriesByPermission.keySet()); + assertEquals(List.of(data), categoriesByPermission.get(InsertPermission.class)); + assertEquals(List.of(media), categoriesByPermission.get(DeletePermission.class)); + } + + @Test + public void testMixOfBaseReadAndPermissionRequiringCategories() + { + SearchService.SearchCategory wiki = new SearchService.SearchCategory("wiki", "Wiki Pages"); + SearchService.SearchCategory data = categoryRequiring("data", InsertPermission.class); + Set baseReadCategoryNames = new HashSet<>(); + + MultiValuedMap, SearchService.SearchCategory> categoriesByPermission = + SecurityQuery.groupCategoriesByRequiredPermission(List.of(wiki, data), baseReadCategoryNames); + + assertEquals(Set.of("wiki"), baseReadCategoryNames); + assertEquals(List.of(data), categoriesByPermission.get(InsertPermission.class)); + } + } } From fae9a9652cdb0820219fa270bb9b9e887fc20bea Mon Sep 17 00:00:00 2001 From: Adam Rauch Date: Fri, 11 Sep 2026 08:30:46 -0700 Subject: [PATCH 2/3] Restore old way temporarily for comparisons --- .../org/labkey/api/search/SearchService.java | 19 ++++++- .../labkey/api/util/MultiPhaseCPUTimer.java | 17 ++++++ assay/src/org/labkey/assay/AssayManager.java | 18 ++++++ .../org/labkey/assay/plate/PlateManager.java | 20 ++++++- .../experiment/api/ExpDataClassImpl.java | 12 ++++ .../labkey/experiment/api/ExpDataImpl.java | 12 ++++ .../experiment/api/ExpMaterialImpl.java | 6 ++ .../experiment/api/ExpSampleTypeImpl.java | 6 ++ .../search/model/LuceneSearchServiceImpl.java | 1 + .../labkey/search/model/SecurityQuery.java | 55 ++++++++++++------- 10 files changed, 142 insertions(+), 24 deletions(-) diff --git a/api/src/org/labkey/api/search/SearchService.java b/api/src/org/labkey/api/search/SearchService.java index f8cfce69be8..cc606b2faf7 100644 --- a/api/src/org/labkey/api/search/SearchService.java +++ b/api/src/org/labkey/api/search/SearchService.java @@ -187,7 +187,7 @@ public String toString() } } - enum SEARCH_PHASE {createQuery, buildSecurityFilter, search, applySecurityFilter, processHits} + enum SEARCH_PHASE {createQuery, buildSecurityFilterOld, buildSecurityFilter, search, applySecurityFilter, processHits} interface TaskListener { @@ -315,6 +315,23 @@ public Class getRequiredPermission() return null; } + @Deprecated // TODO: Remove after testing + protected Set getPermittedContainerIds(User user, Map containers, @NotNull Class perm) + { + Set containerIds = new HashSet<>(); + containers.forEach((id, container) -> { + if (container.hasPermission(user, perm)) + containerIds.add(id); + }); + return containerIds.size() == containers.size() ? containers.keySet() : containerIds; + } + + @Deprecated // TODO: Remove after testing + public Set getPermittedContainerIds(User user, Map containers) + { + return containers.keySet(); + } + public boolean isShowInAdvancedSearch() { return _showInAdvancedSearch; diff --git a/api/src/org/labkey/api/util/MultiPhaseCPUTimer.java b/api/src/org/labkey/api/util/MultiPhaseCPUTimer.java index 7852356db7a..b22a79de283 100644 --- a/api/src/org/labkey/api/util/MultiPhaseCPUTimer.java +++ b/api/src/org/labkey/api/util/MultiPhaseCPUTimer.java @@ -87,6 +87,23 @@ public Map getTimes() return map; } + public void clearTimes() + { + synchronized (_accumulationMap) + { + _accumulationMap.values().forEach(v -> v.setValue(0)); + } + } + + public void clearTimesIfFirstInvocation() + { + synchronized (_accumulationMap) + { + if (_count == 1) + clearTimes(); + } + } + // Create an enum map and populate it with MutableLongs for each value private static > Map getEnumMap(Class clazz, ENUM[] values) { diff --git a/assay/src/org/labkey/assay/AssayManager.java b/assay/src/org/labkey/assay/AssayManager.java index 9d3eb6ba5a2..0e06ec69887 100644 --- a/assay/src/org/labkey/assay/AssayManager.java +++ b/assay/src/org/labkey/assay/AssayManager.java @@ -125,6 +125,12 @@ public Class getRequiredPermission() { return AssayReadPermission.class; } + + @Override + public Set getPermittedContainerIds(User user, Map containers) + { + return getPermittedContainerIds(user, containers, AssayReadPermission.class); + } }; SearchService.SearchCategory ASSAY_BATCH_CATEGORY = new SearchService.SearchCategory("assayBatch", "Assay Batches", false) { @Override @@ -132,6 +138,12 @@ public Class getRequiredPermission() { return AssayReadPermission.class; } + + @Override + public Set getPermittedContainerIds(User user, Map containers) + { + return getPermittedContainerIds(user, containers, AssayReadPermission.class); + } }; SearchService.SearchCategory ASSAY_RUN_CATEGORY = new SearchService.SearchCategory("assayRun", "Assay Runs", false) { @Override @@ -139,6 +151,12 @@ public Class getRequiredPermission() { return AssayReadPermission.class; } + + @Override + public Set getPermittedContainerIds(User user, Map containers) + { + return getPermittedContainerIds(user, containers, AssayReadPermission.class); + } }; /** Cache the protocols defined in a given container, which we can quickly compose to get the protocols in scope */ diff --git a/assay/src/org/labkey/assay/plate/PlateManager.java b/assay/src/org/labkey/assay/plate/PlateManager.java index 913675120e6..9893c8e9fd7 100644 --- a/assay/src/org/labkey/assay/plate/PlateManager.java +++ b/assay/src/org/labkey/assay/plate/PlateManager.java @@ -220,10 +220,24 @@ public class PlateManager implements PlateService, AssayListener, ExperimentList // when those calls are being made for a plate save operation. public static final String PLATE_SAVE_FLAG = ".plateSave"; - // No override needed: base container Read (already required to reach this category's containers) is sufficient. - public SearchService.SearchCategory PLATE_CATEGORY = new SearchService.SearchCategory("plate", "Assay Plates", false); + // No getRequiredPermission() override needed: base container Read (already required to reach this category's + // containers) is sufficient. The old getPermittedContainerIds() override below is kept temporarily for + // old-vs-new comparison testing even though it's a redundant re-check of Read permission. + public SearchService.SearchCategory PLATE_CATEGORY = new SearchService.SearchCategory("plate", "Assay Plates", false) { + @Override + public Set getPermittedContainerIds(User user, Map containers) + { + return getPermittedContainerIds(user, containers, ReadPermission.class); + } + }; - public SearchService.SearchCategory PLATE_SET_CATEGORY = new SearchService.SearchCategory("plateSet", "Assay Plate Sets", false); + public SearchService.SearchCategory PLATE_SET_CATEGORY = new SearchService.SearchCategory("plateSet", "Assay Plate Sets", false) { + @Override + public Set getPermittedContainerIds(User user, Map containers) + { + return getPermittedContainerIds(user, containers, ReadPermission.class); + } + }; public static PlateManager get() { diff --git a/experiment/src/org/labkey/experiment/api/ExpDataClassImpl.java b/experiment/src/org/labkey/experiment/api/ExpDataClassImpl.java index c19d2456587..346cb035d6c 100644 --- a/experiment/src/org/labkey/experiment/api/ExpDataClassImpl.java +++ b/experiment/src/org/labkey/experiment/api/ExpDataClassImpl.java @@ -81,6 +81,12 @@ public Class getRequiredPermission() { return DataClassReadPermission.class; } + + @Override + public Set getPermittedContainerIds(User user, Map containers) + { + return getPermittedContainerIds(user, containers, DataClassReadPermission.class); + } }; public static final SearchService.SearchCategory MEDIA_SEARCH_CATEGORY = new SearchService.SearchCategory(MEDIA_SEARCH_CATEGORY_NAME, "Collections of media data and samples", false) { @Override @@ -88,6 +94,12 @@ public Class getRequiredPermission() { return MediaReadPermission.class; } + + @Override + public Set getPermittedContainerIds(User user, Map containers) + { + return getPermittedContainerIds(user, containers, MediaReadPermission.class); + } }; private Domain _domain; diff --git a/experiment/src/org/labkey/experiment/api/ExpDataImpl.java b/experiment/src/org/labkey/experiment/api/ExpDataImpl.java index b8306ad25cc..cd05e90a669 100644 --- a/experiment/src/org/labkey/experiment/api/ExpDataImpl.java +++ b/experiment/src/org/labkey/experiment/api/ExpDataImpl.java @@ -137,6 +137,12 @@ public Class getRequiredPermission() { return DataClassReadPermission.class; } + + @Override + public Set getPermittedContainerIds(User user, Map containers) + { + return getPermittedContainerIds(user, containers, DataClassReadPermission.class); + } }; public static final SearchService.SearchCategory expMediaDataCategory = new SearchService.SearchCategory("mediaData", "ExpData for media objects", false) { @Override @@ -144,6 +150,12 @@ public Class getRequiredPermission() { return MediaReadPermission.class; } + + @Override + public Set getPermittedContainerIds(User user, Map containers) + { + return getPermittedContainerIds(user, containers, MediaReadPermission.class); + } }; /** Cache this because it can be expensive to recompute */ diff --git a/experiment/src/org/labkey/experiment/api/ExpMaterialImpl.java b/experiment/src/org/labkey/experiment/api/ExpMaterialImpl.java index 192683c66fc..c36c9e4b1bc 100644 --- a/experiment/src/org/labkey/experiment/api/ExpMaterialImpl.java +++ b/experiment/src/org/labkey/experiment/api/ExpMaterialImpl.java @@ -93,6 +93,12 @@ public Class getRequiredPermission() { return MediaReadPermission.class; } + + @Override + public Set getPermittedContainerIds(User user, Map containers) + { + return getPermittedContainerIds(user, containers, MediaReadPermission.class); + } }; static public List fromMaterials(Collection materials) diff --git a/experiment/src/org/labkey/experiment/api/ExpSampleTypeImpl.java b/experiment/src/org/labkey/experiment/api/ExpSampleTypeImpl.java index 36acf398e88..83c4a6b2a18 100644 --- a/experiment/src/org/labkey/experiment/api/ExpSampleTypeImpl.java +++ b/experiment/src/org/labkey/experiment/api/ExpSampleTypeImpl.java @@ -102,6 +102,12 @@ public Class getRequiredPermission() { return MediaReadPermission.class; } + + @Override + public Set getPermittedContainerIds(User user, Map containers) + { + return getPermittedContainerIds(user, containers, MediaReadPermission.class); + } }; public static final String ALIQUOT_NAME_EXPRESSION = "${" + ALIQUOTED_FROM_EXPRESSION + "-:withCounter}"; diff --git a/search/src/org/labkey/search/model/LuceneSearchServiceImpl.java b/search/src/org/labkey/search/model/LuceneSearchServiceImpl.java index c75670eec36..56c28c380aa 100644 --- a/search/src/org/labkey/search/model/LuceneSearchServiceImpl.java +++ b/search/src/org/labkey/search/model/LuceneSearchServiceImpl.java @@ -1824,6 +1824,7 @@ else if (options.sortField.equals(FIELD_NAME.container.name())) finally { TIMER.releaseInvocationTimer(iTimer); + TIMER.clearTimesIfFirstInvocation(); // Toss the very first invocation since it likely had to warm the caches, etc. } } diff --git a/search/src/org/labkey/search/model/SecurityQuery.java b/search/src/org/labkey/search/model/SecurityQuery.java index 20fc953eab2..b47d12d4997 100644 --- a/search/src/org/labkey/search/model/SecurityQuery.java +++ b/search/src/org/labkey/search/model/SecurityQuery.java @@ -43,6 +43,7 @@ import org.labkey.api.module.Module; import org.labkey.api.search.SearchScope; import org.labkey.api.search.SearchService; +import org.labkey.api.search.SearchService.SearchCategory; import org.labkey.api.security.SecurableResource; import org.labkey.api.security.SecurityManager; import org.labkey.api.security.User; @@ -83,14 +84,26 @@ public class SecurityQuery extends Query _recursive = searchScope.isRecursive(); _iTimer = iTimer; + // For now, perform the permission checking twice, old way and new way. This allows us to verify the results + // are identical and evaluate the performance benefit. TODO: Remove the block below and comparison asserts before merging. + iTimer.setPhase(SearchService.SEARCH_PHASE.buildSecurityFilterOld); + HashMap oldContainerIds = searchScope.getSearchableContainers(user, currentContainer); + HashMap> categoryContainers = new HashMap<>(); + SearchService.get().getSearchCategories().forEach( + category -> categoryContainers.put(category.getName(), category.getPermittedContainerIds(user, oldContainerIds)) + ); + iTimer.setPhase(SearchService.SEARCH_PHASE.buildSecurityFilter); + _containerIds = searchScope.getSearchableContainers(user, currentContainer); + assert oldContainerIds.equals(_containerIds); + // Categories that require only base container Read (already guaranteed for every container above) are // resolved directly; the rest are grouped by required permission so multiple categories that require the // same permission (e.g., the three assay categories all require AssayReadPermission) share a single // O(containers) assembly pass below instead of each redoing it. Set baseReadCategoryNames = new HashSet<>(); - MultiValuedMap, SearchService.SearchCategory> categoriesByPermission = + MultiValuedMap, SearchCategory> categoriesByPermission = groupCategoriesByRequiredPermission(SearchService.get().getSearchCategories(), baseReadCategoryNames); for (String categoryName : baseReadCategoryNames) @@ -126,21 +139,23 @@ public class SecurityQuery extends Query permittedContainerIds.add(entry.getKey()); } - for (SearchService.SearchCategory category : categories) + for (SearchCategory category : categories) _categoryContainers.put(category.getName(), permittedContainerIds); }); + + assert categoryContainers.equals(_categoryContainers); } /** * Splits categories into those requiring only base container Read (their names are added to baseReadCategoryNames) * and those requiring a specific permission, which are grouped by that permission class. */ - static MultiValuedMap, SearchService.SearchCategory> groupCategoriesByRequiredPermission( - Collection categories, Set baseReadCategoryNames) + static MultiValuedMap, SearchCategory> groupCategoriesByRequiredPermission( + Collection categories, Set baseReadCategoryNames) { - MultiValuedMap, SearchService.SearchCategory> categoriesByPermission = new ArrayListValuedHashMap<>(); + MultiValuedMap, SearchCategory> categoriesByPermission = new ArrayListValuedHashMap<>(); - for (SearchService.SearchCategory category : categories) + for (SearchCategory category : categories) { Class requiredPermission = category.getRequiredPermission(); @@ -388,9 +403,9 @@ public boolean mayInheritPolicy() public static class TestCase extends Assert { - private static SearchService.SearchCategory categoryRequiring(String name, Class requiredPermission) + private static SearchCategory categoryRequiring(String name, Class requiredPermission) { - return new SearchService.SearchCategory(name, name, false) + return new SearchCategory(name, name, false) { @Override public Class getRequiredPermission() @@ -403,10 +418,10 @@ public Class getRequiredPermission() @Test public void testCategoryWithNoRequiredPermissionGoesToBaseRead() { - SearchService.SearchCategory wiki = new SearchService.SearchCategory("wiki", "Wiki Pages"); + SearchCategory wiki = new SearchCategory("wiki", "Wiki Pages"); Set baseReadCategoryNames = new HashSet<>(); - MultiValuedMap, SearchService.SearchCategory> categoriesByPermission = + MultiValuedMap, SearchCategory> categoriesByPermission = SecurityQuery.groupCategoriesByRequiredPermission(List.of(wiki), baseReadCategoryNames); assertEquals(Set.of("wiki"), baseReadCategoryNames); @@ -417,12 +432,12 @@ public void testCategoryWithNoRequiredPermissionGoesToBaseRead() public void testCategoriesSharingAPermissionAreGroupedTogether() { // Mirrors the real assay/assayBatch/assayRun categories, which all require the same permission. - SearchService.SearchCategory assay = categoryRequiring("assay", InsertPermission.class); - SearchService.SearchCategory assayBatch = categoryRequiring("assayBatch", InsertPermission.class); - SearchService.SearchCategory assayRun = categoryRequiring("assayRun", InsertPermission.class); + SearchCategory assay = categoryRequiring("assay", InsertPermission.class); + SearchCategory assayBatch = categoryRequiring("assayBatch", InsertPermission.class); + SearchCategory assayRun = categoryRequiring("assayRun", InsertPermission.class); Set baseReadCategoryNames = new HashSet<>(); - MultiValuedMap, SearchService.SearchCategory> categoriesByPermission = + MultiValuedMap, SearchCategory> categoriesByPermission = SecurityQuery.groupCategoriesByRequiredPermission(List.of(assay, assayBatch, assayRun), baseReadCategoryNames); assertTrue(baseReadCategoryNames.isEmpty()); @@ -433,11 +448,11 @@ public void testCategoriesSharingAPermissionAreGroupedTogether() @Test public void testCategoriesWithDifferentPermissionsAreNotGroupedTogether() { - SearchService.SearchCategory data = categoryRequiring("data", InsertPermission.class); - SearchService.SearchCategory media = categoryRequiring("media", DeletePermission.class); + SearchCategory data = categoryRequiring("data", InsertPermission.class); + SearchCategory media = categoryRequiring("media", DeletePermission.class); Set baseReadCategoryNames = new HashSet<>(); - MultiValuedMap, SearchService.SearchCategory> categoriesByPermission = + MultiValuedMap, SearchCategory> categoriesByPermission = SecurityQuery.groupCategoriesByRequiredPermission(List.of(data, media), baseReadCategoryNames); assertEquals(Set.of(InsertPermission.class, DeletePermission.class), categoriesByPermission.keySet()); @@ -448,11 +463,11 @@ public void testCategoriesWithDifferentPermissionsAreNotGroupedTogether() @Test public void testMixOfBaseReadAndPermissionRequiringCategories() { - SearchService.SearchCategory wiki = new SearchService.SearchCategory("wiki", "Wiki Pages"); - SearchService.SearchCategory data = categoryRequiring("data", InsertPermission.class); + SearchCategory wiki = new SearchCategory("wiki", "Wiki Pages"); + SearchCategory data = categoryRequiring("data", InsertPermission.class); Set baseReadCategoryNames = new HashSet<>(); - MultiValuedMap, SearchService.SearchCategory> categoriesByPermission = + MultiValuedMap, SearchCategory> categoriesByPermission = SecurityQuery.groupCategoriesByRequiredPermission(List.of(wiki, data), baseReadCategoryNames); assertEquals(Set.of("wiki"), baseReadCategoryNames); From 847e58135528903c43593bc5305744d871d53ad6 Mon Sep 17 00:00:00 2001 From: Adam Rauch Date: Fri, 11 Sep 2026 08:49:41 -0700 Subject: [PATCH 3/3] Clear count as well --- api/src/org/labkey/api/util/MultiPhaseCPUTimer.java | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/api/src/org/labkey/api/util/MultiPhaseCPUTimer.java b/api/src/org/labkey/api/util/MultiPhaseCPUTimer.java index b22a79de283..f823b2bf5f2 100644 --- a/api/src/org/labkey/api/util/MultiPhaseCPUTimer.java +++ b/api/src/org/labkey/api/util/MultiPhaseCPUTimer.java @@ -38,6 +38,7 @@ public class MultiPhaseCPUTimer> private final K[] _values; private long _count = 0; + private boolean _clearedFirstInvocation = false; public MultiPhaseCPUTimer(Class clazz, K[] values) { @@ -92,6 +93,7 @@ public void clearTimes() synchronized (_accumulationMap) { _accumulationMap.values().forEach(v -> v.setValue(0)); + _count = 0; } } @@ -99,8 +101,11 @@ public void clearTimesIfFirstInvocation() { synchronized (_accumulationMap) { - if (_count == 1) + if (!_clearedFirstInvocation) + { + _clearedFirstInvocation = true; clearTimes(); + } } }