From 2ed92d8b375469b8769a600e686c2c36a24944da Mon Sep 17 00:00:00 2001 From: XingY Date: Wed, 30 Sep 2026 18:52:40 -0700 Subject: [PATCH 01/13] GH Issue 1512: user defined queries causes performance issues with lookup field query listing --- .../org/labkey/query/QueryDefinitionImpl.java | 33 ++++++++++++++ .../query/controllers/QueryController.java | 43 ++++++++++++++++--- 2 files changed, 70 insertions(+), 6 deletions(-) diff --git a/query/src/org/labkey/query/QueryDefinitionImpl.java b/query/src/org/labkey/query/QueryDefinitionImpl.java index 02033b9333a..7a0dd4b027a 100644 --- a/query/src/org/labkey/query/QueryDefinitionImpl.java +++ b/query/src/org/labkey/query/QueryDefinitionImpl.java @@ -27,6 +27,8 @@ import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; import org.labkey.api.action.SpringActionController; +import org.labkey.api.cache.Cache; +import org.labkey.api.cache.CacheManager; import org.labkey.api.collections.CaseInsensitiveLinkedHashMap; import org.labkey.api.data.AbstractTableInfo; import org.labkey.api.data.ColumnInfo; @@ -77,6 +79,7 @@ import java.util.ArrayList; import java.util.Collection; import java.util.Collections; +import java.util.Date; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -106,6 +109,11 @@ public abstract class QueryDefinitionImpl implements QueryDefinition // private static Map, TableInfo> _cache = new HashMap<>(); private final Map, TableInfo> _cache = new HashMap<>(); + // GH Issue 1512: PK presence is structural and user-independent, so (unlike a resolved TableInfo) it's safe to share + // across requests, letting lookup-target enumeration skip re-resolving known no-PK queries. Keyed on Modified so an + // edit busts the entry; the DAY TTL bounds the one stale case, a source table's PK changing with no query edit. + private static final Cache HAS_PK_COLUMN_CACHE = CacheManager.getCache(CacheManager.UNLIMITED, CacheManager.DAY, "Query has-PK-column flags"); + private Map _metadataTableMap = null; public QueryDefinitionImpl(User user, Container container, QueryDef queryDef) @@ -814,6 +822,31 @@ public boolean isIncludedForLookups() return _includedForLookups; } + // GH Issue 1512: null key (new/unsaved def with no Modified stamp) means "don't cache" + @Nullable + private String getHasPkColumnCacheKey() + { + Date modified = _queryDef.getModified(); + if (null == modified || null == _queryDef.getContainerId() || null == getName()) + return null; + return _queryDef.getContainerId() + "/" + getSchemaPath() + "/" + getName() + "/" + modified.getTime(); + } + + /** @return cached PK-presence for this query, or null if not cached */ + @Nullable + public Boolean getCachedHasPkColumn() + { + String key = getHasPkColumnCacheKey(); + return null == key ? null : HAS_PK_COLUMN_CACHE.get(key); + } + + public void cacheHasPkColumn(boolean hasPkColumn) + { + String key = getHasPkColumnCacheKey(); + if (null != key) + HAS_PK_COLUMN_CACHE.put(key, hasPkColumn); + } + @Override public void setIsIncludedForLookups(boolean included) { diff --git a/query/src/org/labkey/query/controllers/QueryController.java b/query/src/org/labkey/query/controllers/QueryController.java index 8e71be82b2d..3438127a525 100644 --- a/query/src/org/labkey/query/controllers/QueryController.java +++ b/query/src/org/labkey/query/controllers/QueryController.java @@ -283,6 +283,7 @@ import org.labkey.query.LinkedTableInfo; import org.labkey.query.MetadataTableJSON; import org.labkey.query.ModuleCustomQueryDefinition; +import org.labkey.query.QueryDefinitionImpl; import org.labkey.query.ModuleCustomView; import org.labkey.query.QueryServiceImpl; import org.labkey.query.QueryServiceImpl.CalculatedColumnParseResult; @@ -6849,6 +6850,7 @@ public static class GetQueriesForm { private String _schemaName; private boolean _includeUserQueries = true; + private boolean _includeUserQueriesForLookups = false; private boolean _includeSystemQueries = true; private boolean _includeColumns = true; private boolean _includeViewDataUrl = true; @@ -6875,6 +6877,16 @@ public void setIncludeUserQueries(boolean includeUserQueries) _includeUserQueries = includeUserQueries; } + public boolean isIncludeUserQueriesForLookups() + { + return _includeUserQueriesForLookups; + } + + public void setIncludeUserQueriesForLookups(boolean includeUserQueriesForLookups) + { + _includeUserQueriesForLookups = includeUserQueriesForLookups; + } + public boolean isIncludeSystemQueries() { return _includeSystemQueries; @@ -6948,14 +6960,25 @@ public ApiResponse execute(GetQueriesForm form, BindException errors) List> qinfos = new ArrayList<>(); //user-defined queries - if (form.isIncludeUserQueries()) + if (form.isIncludeUserQueries() || form.isIncludeUserQueriesForLookups()) { + // GH Issue 1512: includeUserQueries returns them all; includeUserQueriesForLookups (only when the former is off) + // restricts to queries that expose a primary key + boolean requirePk = form.isIncludeUserQueriesForLookups() && !form.isIncludeUserQueries(); for (QueryDefinition qdef : uschema.getQueryDefs().values()) { if (!qdef.isTemporary()) { + if (requirePk) + { + QueryDefinitionImpl impl = qdef instanceof QueryDefinitionImpl q ? q : null; + if (impl != null && Boolean.FALSE.equals(impl.getCachedHasPkColumn())) + continue; + } ActionURL viewDataUrl = form.isIncludeViewDataUrl() ? uschema.urlFor(QueryAction.executeQuery, qdef) : null; - qinfos.add(getQueryProps(qdef, viewDataUrl, true, uschema, form.isIncludeColumns(), form.isQueryDetailColumns(), form.isIncludeTitle())); + Map props = getQueryProps(qdef, viewDataUrl, true, uschema, form.isIncludeColumns(), form.isQueryDetailColumns(), form.isIncludeTitle(), requirePk); + if (props != null) + qinfos.add(props); } } } @@ -6971,7 +6994,7 @@ public ApiResponse execute(GetQueriesForm form, BindException errors) if (qdef != null) { ActionURL viewDataUrl = form.isIncludeViewDataUrl() ? uschema.urlFor(QueryAction.executeQuery, qdef) : null; - qinfos.add(getQueryProps(qdef, viewDataUrl, false, uschema, form.isIncludeColumns(), form.isQueryDetailColumns(), form.isIncludeTitle())); + qinfos.add(getQueryProps(qdef, viewDataUrl, false, uschema, form.isIncludeColumns(), form.isQueryDetailColumns(), form.isIncludeTitle(), false)); } } } @@ -6980,8 +7003,10 @@ public ApiResponse execute(GetQueriesForm form, BindException errors) return response; } - protected Map getQueryProps(QueryDefinition qdef, ActionURL viewDataUrl, boolean isUserDefined, UserSchema schema, boolean includeColumns, boolean useQueryDetailColumns, boolean includeTitle) + private Map getQueryProps(QueryDefinition qdef, ActionURL viewDataUrl, boolean isUserDefined, UserSchema schema, boolean includeColumns, boolean useQueryDetailColumns, boolean includeTitle, boolean requirePk) { + QueryDefinitionImpl impl = qdef instanceof QueryDefinitionImpl q ? q : null; + Map qinfo = new HashMap<>(); qinfo.put("hidden", qdef.isHidden()); qinfo.put("snapshot", qdef.isSnapshot()); @@ -7010,13 +7035,19 @@ protected Map getQueryProps(QueryDefinition qdef, ActionURL view String name = qdef.getName(); try { - // get the TableInfo if the user requested column info or title, otherwise skip (it can be expensive) - if (includeColumns || includeTitle) + // get the TableInfo if the user requested column info or title or a PK filter, otherwise skip (it can be expensive) + if (includeColumns || includeTitle || requirePk) { TableInfo table = qdef.getTable(schema, null, true); if (null != table) { + boolean hasPk = table.getPkColumns().stream().anyMatch(col -> !col.isAdditionalQueryColumn()); + if (isUserDefined && impl != null) + impl.cacheHasPkColumn(hasPk); + if (requirePk && !hasPk) + return null; + if (includeColumns) { Collection> columns; From 4078e373d5067e28231ead44e88daadb74d4c580 Mon Sep 17 00:00:00 2001 From: XingY Date: Wed, 30 Sep 2026 19:12:33 -0700 Subject: [PATCH 02/13] CC review --- .../src/org/labkey/query/controllers/QueryController.java | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/query/src/org/labkey/query/controllers/QueryController.java b/query/src/org/labkey/query/controllers/QueryController.java index 3438127a525..39a7602032d 100644 --- a/query/src/org/labkey/query/controllers/QueryController.java +++ b/query/src/org/labkey/query/controllers/QueryController.java @@ -7033,6 +7033,7 @@ private Map getQueryProps(QueryDefinition qdef, ActionURL viewDa String title = qdef.getName(); String name = qdef.getName(); + boolean hasPk = false; try { // get the TableInfo if the user requested column info or title or a PK filter, otherwise skip (it can be expensive) @@ -7042,7 +7043,7 @@ private Map getQueryProps(QueryDefinition qdef, ActionURL viewDa if (null != table) { - boolean hasPk = table.getPkColumns().stream().anyMatch(col -> !col.isAdditionalQueryColumn()); + hasPk = table.getPkColumns().stream().anyMatch(col -> !col.isAdditionalQueryColumn()); if (isUserDefined && impl != null) impl.cacheHasPkColumn(hasPk); if (requirePk && !hasPk) @@ -7093,6 +7094,10 @@ private Map getQueryProps(QueryDefinition qdef, ActionURL viewDa //may happen due to query failing parse } + // GH Issue 1512: a query that didn't resolve (null table or parse failure) can't be confirmed as a lookup target + if (requirePk && !hasPk) + return null; + qinfo.put("title", title); qinfo.put("name", name); return qinfo; From 4b7ba5efbd12e422088fa9a192d9900d2c3bd6b3 Mon Sep 17 00:00:00 2001 From: XingY Date: Mon, 5 Oct 2026 10:01:43 -0700 Subject: [PATCH 03/13] code review changes --- .../org/labkey/query/QueryDefinitionImpl.java | 18 ++++++++++++------ .../labkey/query/persist/QueryDefCache.java | 2 ++ .../org/labkey/query/persist/QueryManager.java | 4 ++++ 3 files changed, 18 insertions(+), 6 deletions(-) diff --git a/query/src/org/labkey/query/QueryDefinitionImpl.java b/query/src/org/labkey/query/QueryDefinitionImpl.java index 7a0dd4b027a..f08554d3e98 100644 --- a/query/src/org/labkey/query/QueryDefinitionImpl.java +++ b/query/src/org/labkey/query/QueryDefinitionImpl.java @@ -109,9 +109,9 @@ public abstract class QueryDefinitionImpl implements QueryDefinition // private static Map, TableInfo> _cache = new HashMap<>(); private final Map, TableInfo> _cache = new HashMap<>(); - // GH Issue 1512: PK presence is structural and user-independent, so (unlike a resolved TableInfo) it's safe to share - // across requests, letting lookup-target enumeration skip re-resolving known no-PK queries. Keyed on Modified so an - // edit busts the entry; the DAY TTL bounds the one stale case, a source table's PK changing with no query edit. + // GH Issue 1512: does this query expose a PK? Lets lookup-target enumeration skip re-resolving known no-PK queries. + // Keyed by resolving container + schema path + name + Modified; cleared on any QueryDef/schema change (a query's PK + // can shift without its own row changing, via chained queries, source metadata, or schema reloads). private static final Cache HAS_PK_COLUMN_CACHE = CacheManager.getCache(CacheManager.UNLIMITED, CacheManager.DAY, "Query has-PK-column flags"); private Map _metadataTableMap = null; @@ -822,14 +822,20 @@ public boolean isIncludedForLookups() return _includedForLookups; } - // GH Issue 1512: null key (new/unsaved def with no Modified stamp) means "don't cache" + // GH Issue 1512: key on the resolving container, not the defining one: an inheritable/shared query compiles to a + // different table, and PK, per folder. Null key (unsaved def, no Modified) means "don't cache". @Nullable private String getHasPkColumnCacheKey() { Date modified = _queryDef.getModified(); - if (null == modified || null == _queryDef.getContainerId() || null == getName()) + if (null == modified || null == getContainer() || null == getName()) return null; - return _queryDef.getContainerId() + "/" + getSchemaPath() + "/" + getName() + "/" + modified.getTime(); + return getContainer().getId() + "/" + getSchemaPath() + "/" + getName() + "/" + modified.getTime(); + } + + public static void clearHasPkColumnCache() + { + HAS_PK_COLUMN_CACHE.clear(); } /** @return cached PK-presence for this query, or null if not cached */ diff --git a/query/src/org/labkey/query/persist/QueryDefCache.java b/query/src/org/labkey/query/persist/QueryDefCache.java index c4acb06d0d7..21ac9651711 100644 --- a/query/src/org/labkey/query/persist/QueryDefCache.java +++ b/query/src/org/labkey/query/persist/QueryDefCache.java @@ -25,6 +25,7 @@ import org.labkey.api.data.Container; import org.labkey.api.data.SimpleFilter; import org.labkey.api.data.TableSelector; +import org.labkey.query.QueryDefinitionImpl; import java.util.ArrayList; import java.util.Collection; @@ -168,5 +169,6 @@ QueryDef getQueryDefById(Container container, int queryDefId) public static void uncache(Container c) { QUERY_DEF_DB_CACHE.remove(c); + QueryDefinitionImpl.clearHasPkColumnCache(); } } \ No newline at end of file diff --git a/query/src/org/labkey/query/persist/QueryManager.java b/query/src/org/labkey/query/persist/QueryManager.java index 0c11951341c..332def0a7b3 100644 --- a/query/src/org/labkey/query/persist/QueryManager.java +++ b/query/src/org/labkey/query/persist/QueryManager.java @@ -67,6 +67,7 @@ import org.labkey.api.view.NotFoundException; import org.labkey.query.ExternalSchema; import org.labkey.query.ExternalSchemaDocumentProvider; +import org.labkey.query.QueryDefinitionImpl; import org.labkey.query.audit.GridViewAuditProvider; import org.labkey.query.audit.GridViewAuditProvider.GridViewAuditEvent; import org.labkey.query.audit.QueryExportAuditProvider; @@ -433,6 +434,8 @@ public void updateExternalSchemas(Container c) { ExternalSchemaDefCache.uncache(c); ExternalSchemaDocumentProvider.getInstance().enumerateDocuments(SearchService.get().defaultTask().getQueue(c, SearchService.PRIORITY.modified), null); + // GH Issue 1512: a schema reload can change a source-table PK without touching the query row + QueryDefinitionImpl.clearHasPkColumnCache(); } } @@ -444,6 +447,7 @@ public void reloadAllExternalSchemas(Container c) public void reloadExternalSchema(ExternalSchemaDef def) { ExternalSchema.uncache(def); + QueryDefinitionImpl.clearHasPkColumnCache(); } public boolean canInherit(int flag) From fd57f1757b777f201a1412fa5eab9296f8c4ba1d Mon Sep 17 00:00:00 2001 From: XingY Date: Mon, 5 Oct 2026 10:15:17 -0700 Subject: [PATCH 04/13] code review changes - module query --- .../query/ModuleCustomQueryDefinition.java | 9 +++++++++ .../org/labkey/query/QueryDefinitionImpl.java | 17 +++++++++++++---- 2 files changed, 22 insertions(+), 4 deletions(-) diff --git a/query/src/org/labkey/query/ModuleCustomQueryDefinition.java b/query/src/org/labkey/query/ModuleCustomQueryDefinition.java index 54c919ac8fe..c4be3934890 100644 --- a/query/src/org/labkey/query/ModuleCustomQueryDefinition.java +++ b/query/src/org/labkey/query/ModuleCustomQueryDefinition.java @@ -16,6 +16,7 @@ package org.labkey.query; import org.apache.commons.io.IOUtils; +import org.jetbrains.annotations.Nullable; import org.labkey.api.data.Container; import org.labkey.api.module.ModuleLoader; import org.labkey.api.moduleeditor.api.ModuleEditorService; @@ -88,6 +89,14 @@ public File getSqlFile() return _resourceSqlFile; } + // GH Issue 1512: file-based module queries carry no QueryDef.Modified, so bust the has-PK cache on the .sql mtime + @Nullable + @Override + protected String getHasPkCacheVersion() + { + return null != _resourceSqlFile ? "module:" + _resourceSqlFile.lastModified() : null; + } + public File getModuleXmlFile() { return _resourceQueryXmlFile; diff --git a/query/src/org/labkey/query/QueryDefinitionImpl.java b/query/src/org/labkey/query/QueryDefinitionImpl.java index f08554d3e98..b038b39e397 100644 --- a/query/src/org/labkey/query/QueryDefinitionImpl.java +++ b/query/src/org/labkey/query/QueryDefinitionImpl.java @@ -822,15 +822,24 @@ public boolean isIncludedForLookups() return _includedForLookups; } + // GH Issue 1512: cache-busting token for the has-PK cache. DB queries use Modified; subclasses override (e.g. a + // file-based module query uses its .sql mtime). Null means "don't cache" (unsaved/transient def). + @Nullable + protected String getHasPkCacheVersion() + { + Date modified = _queryDef.getModified(); + return null == modified ? null : String.valueOf(modified.getTime()); + } + // GH Issue 1512: key on the resolving container, not the defining one: an inheritable/shared query compiles to a - // different table, and PK, per folder. Null key (unsaved def, no Modified) means "don't cache". + // different table, and PK, per folder. @Nullable private String getHasPkColumnCacheKey() { - Date modified = _queryDef.getModified(); - if (null == modified || null == getContainer() || null == getName()) + String version = getHasPkCacheVersion(); + if (null == version || null == getContainer() || null == getName()) return null; - return getContainer().getId() + "/" + getSchemaPath() + "/" + getName() + "/" + modified.getTime(); + return getContainer().getId() + "/" + getSchemaPath() + "/" + getName() + "/" + version; } public static void clearHasPkColumnCache() From c5c4d542b05a0ba80fd0bb1c468e79dc686d4e4c Mon Sep 17 00:00:00 2001 From: labkey-nicka Date: Tue, 6 Oct 2026 11:31:31 -0700 Subject: [PATCH 05/13] Skip non-lookup queries; cache module query PK flags - Skip user queries excluded from lookups before resolving them - Cache module queries' has-PK flag outside dev mode; refresh source modules on .sql and .query.xml changes --- .../query/ModuleCustomQueryDefinition.java | 11 +++-- .../query/controllers/QueryController.java | 40 ++++++++++--------- 2 files changed, 28 insertions(+), 23 deletions(-) diff --git a/query/src/org/labkey/query/ModuleCustomQueryDefinition.java b/query/src/org/labkey/query/ModuleCustomQueryDefinition.java index c4be3934890..9bb2cce2dae 100644 --- a/query/src/org/labkey/query/ModuleCustomQueryDefinition.java +++ b/query/src/org/labkey/query/ModuleCustomQueryDefinition.java @@ -25,6 +25,7 @@ import org.labkey.api.query.SchemaKey; import org.labkey.api.security.User; import org.labkey.api.security.permissions.EditModuleResourcesPermission; +import org.labkey.api.settings.AppProps; import org.labkey.api.util.UnexpectedException; import org.labkey.query.persist.QueryDef; @@ -89,12 +90,14 @@ public File getSqlFile() return _resourceSqlFile; } - // GH Issue 1512: file-based module queries carry no QueryDef.Modified, so bust the has-PK cache on the .sql mtime - @Nullable + // GH Issue 1512: module queries carry no QueryDef.Modified. Source files (dev mode only) version on + // .sql + .query.xml mtimes; otherwise resources are fixed until restart. @Override - protected String getHasPkCacheVersion() + protected @Nullable String getHasPkCacheVersion() { - return null != _resourceSqlFile ? "module:" + _resourceSqlFile.lastModified() : null; + if (null != _resourceSqlFile) + return "module:" + _resourceSqlFile.lastModified() + ":" + _resourceQueryXmlFile.lastModified(); + return AppProps.getInstance().isDevMode() ? null : "module:" + _moduleName; } public File getModuleXmlFile() diff --git a/query/src/org/labkey/query/controllers/QueryController.java b/query/src/org/labkey/query/controllers/QueryController.java index 39a7602032d..fe41b7904ae 100644 --- a/query/src/org/labkey/query/controllers/QueryController.java +++ b/query/src/org/labkey/query/controllers/QueryController.java @@ -6963,23 +6963,25 @@ public ApiResponse execute(GetQueriesForm form, BindException errors) if (form.isIncludeUserQueries() || form.isIncludeUserQueriesForLookups()) { // GH Issue 1512: includeUserQueries returns them all; includeUserQueriesForLookups (only when the former is off) - // restricts to queries that expose a primary key + // restricts to lookup-eligible queries that expose a primary key boolean requirePk = form.isIncludeUserQueriesForLookups() && !form.isIncludeUserQueries(); for (QueryDefinition qdef : uschema.getQueryDefs().values()) { - if (!qdef.isTemporary()) + if (qdef.isTemporary()) + continue; + + if (requirePk) { - if (requirePk) - { - QueryDefinitionImpl impl = qdef instanceof QueryDefinitionImpl q ? q : null; - if (impl != null && Boolean.FALSE.equals(impl.getCachedHasPkColumn())) - continue; - } - ActionURL viewDataUrl = form.isIncludeViewDataUrl() ? uschema.urlFor(QueryAction.executeQuery, qdef) : null; - Map props = getQueryProps(qdef, viewDataUrl, true, uschema, form.isIncludeColumns(), form.isQueryDetailColumns(), form.isIncludeTitle(), requirePk); - if (props != null) - qinfos.add(props); + if (!qdef.isIncludedForLookups()) + continue; + if (qdef instanceof QueryDefinitionImpl q && Boolean.FALSE.equals(q.getCachedHasPkColumn())) + continue; } + + ActionURL viewDataUrl = form.isIncludeViewDataUrl() ? uschema.urlFor(QueryAction.executeQuery, qdef) : null; + Map props = getQueryProps(qdef, viewDataUrl, true, uschema, form.isIncludeColumns(), form.isQueryDetailColumns(), form.isIncludeTitle(), requirePk); + if (props != null) + qinfos.add(props); } } @@ -6994,7 +6996,9 @@ public ApiResponse execute(GetQueriesForm form, BindException errors) if (qdef != null) { ActionURL viewDataUrl = form.isIncludeViewDataUrl() ? uschema.urlFor(QueryAction.executeQuery, qdef) : null; - qinfos.add(getQueryProps(qdef, viewDataUrl, false, uschema, form.isIncludeColumns(), form.isQueryDetailColumns(), form.isIncludeTitle(), false)); + Map props = getQueryProps(qdef, viewDataUrl, false, uschema, form.isIncludeColumns(), form.isQueryDetailColumns(), form.isIncludeTitle(), false); + if (props != null) + qinfos.add(props); } } } @@ -7003,10 +7007,8 @@ public ApiResponse execute(GetQueriesForm form, BindException errors) return response; } - private Map getQueryProps(QueryDefinition qdef, ActionURL viewDataUrl, boolean isUserDefined, UserSchema schema, boolean includeColumns, boolean useQueryDetailColumns, boolean includeTitle, boolean requirePk) + private @Nullable Map getQueryProps(QueryDefinition qdef, ActionURL viewDataUrl, boolean isUserDefined, UserSchema schema, boolean includeColumns, boolean useQueryDetailColumns, boolean includeTitle, boolean requirePk) { - QueryDefinitionImpl impl = qdef instanceof QueryDefinitionImpl q ? q : null; - Map qinfo = new HashMap<>(); qinfo.put("hidden", qdef.isHidden()); qinfo.put("snapshot", qdef.isSnapshot()); @@ -7044,8 +7046,8 @@ private Map getQueryProps(QueryDefinition qdef, ActionURL viewDa if (null != table) { hasPk = table.getPkColumns().stream().anyMatch(col -> !col.isAdditionalQueryColumn()); - if (isUserDefined && impl != null) - impl.cacheHasPkColumn(hasPk); + if (isUserDefined && qdef instanceof QueryDefinitionImpl q) + q.cacheHasPkColumn(hasPk); if (requirePk && !hasPk) return null; @@ -7089,7 +7091,7 @@ private Map getQueryProps(QueryDefinition qdef, ActionURL viewDa } } } - catch(Exception e) + catch (Exception e) { //may happen due to query failing parse } From 71dfc5d4f8bb9355516aaedf33ec88476b1827fe Mon Sep 17 00:00:00 2001 From: labkey-nicka Date: Tue, 6 Oct 2026 11:32:20 -0700 Subject: [PATCH 06/13] CacheManager.MONTH --- query/src/org/labkey/query/QueryDefinitionImpl.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/query/src/org/labkey/query/QueryDefinitionImpl.java b/query/src/org/labkey/query/QueryDefinitionImpl.java index b038b39e397..b6ad8283ac9 100644 --- a/query/src/org/labkey/query/QueryDefinitionImpl.java +++ b/query/src/org/labkey/query/QueryDefinitionImpl.java @@ -112,7 +112,7 @@ public abstract class QueryDefinitionImpl implements QueryDefinition // GH Issue 1512: does this query expose a PK? Lets lookup-target enumeration skip re-resolving known no-PK queries. // Keyed by resolving container + schema path + name + Modified; cleared on any QueryDef/schema change (a query's PK // can shift without its own row changing, via chained queries, source metadata, or schema reloads). - private static final Cache HAS_PK_COLUMN_CACHE = CacheManager.getCache(CacheManager.UNLIMITED, CacheManager.DAY, "Query has-PK-column flags"); + private static final Cache HAS_PK_COLUMN_CACHE = CacheManager.getCache(CacheManager.UNLIMITED, CacheManager.MONTH, "Query has-PK-column flags"); private Map _metadataTableMap = null; From 6f74b36a32f2da1f3e34f1c7deab7a907d6a5573 Mon Sep 17 00:00:00 2001 From: labkey-nicka Date: Tue, 6 Oct 2026 12:12:57 -0700 Subject: [PATCH 07/13] Clear ontology caches after query rename updates lookups - Renaming a query rewrote lookup targets in the database directly, leaving cached property descriptors pointing at the old name --- .../PropertyQueryChangeListener.java | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/experiment/src/org/labkey/experiment/PropertyQueryChangeListener.java b/experiment/src/org/labkey/experiment/PropertyQueryChangeListener.java index 00bc2122399..586e48456ca 100644 --- a/experiment/src/org/labkey/experiment/PropertyQueryChangeListener.java +++ b/experiment/src/org/labkey/experiment/PropertyQueryChangeListener.java @@ -38,7 +38,7 @@ public void queryCreated(User user, Container container, ContainerFilter scope, { } - private void updateLookupQuery(String newValue, SchemaKey schema, String oldQuery, Container container) + private int updateLookupQuery(String newValue, SchemaKey schema, String oldQuery, Container container) { String fieldName = "lookupquery"; TableInfo pdTable = OntologyManager.getTinfoPropertyDescriptor(); @@ -53,11 +53,10 @@ private void updateLookupQuery(String newValue, SchemaKey schema, String oldQuer .add(container) .add(container); - new SqlExecutor(pdTable.getSchema()).execute(updateSql); - + return new SqlExecutor(pdTable.getSchema()).execute(updateSql); } - private void updateLookupSchema(String newValue, String oldSchema, Container container) + private int updateLookupSchema(String newValue, String oldSchema, Container container) { String fieldName = "lookupschema"; TableInfo pdTable = OntologyManager.getTinfoPropertyDescriptor(); @@ -71,8 +70,7 @@ private void updateLookupSchema(String newValue, String oldSchema, Container con .add(container) .add(container); - new SqlExecutor(pdTable.getSchema()).execute(updateSql); - + return new SqlExecutor(pdTable.getSchema()).execute(updateSql); } @Override @@ -93,14 +91,19 @@ public void queryChanged(User user, Container container, ContainerFilter scope, queryNameChangeMap.put(oldVal, newVal); } + int updated = 0; for (String oldValue : queryNameChangeMap.keySet()) { String newValue = queryNameChangeMap.get(oldValue); if (isSchemaChange) - updateLookupSchema(newValue, oldValue, container); + updated += updateLookupSchema(newValue, oldValue, container); else - updateLookupQuery(newValue, schema, oldValue, container); + updated += updateLookupQuery(newValue, schema, oldValue, container); } + + // GH Issue 1512: the direct SQL updates bypass OntologyManager, so its cached property descriptors still hold the old lookup target + if (updated > 0) + OntologyManager.clearCaches(); } @Override From 22a3dcf3534a53b6fd8febb28d96ad254ca02520 Mon Sep 17 00:00:00 2001 From: Adam Rauch Date: Mon, 5 Oct 2026 12:01:11 -0700 Subject: [PATCH 08/13] API key auth optional feature flag (#8103) ## Rationale Authenticating via an API key provided as a URL parameter is not a security best practice, so we want to put this under an optional feature flag. https://github.com/LabKey/internal-issues/issues/1617 ## Related Pull Requests - https://github.com/LabKey/clientModules/pull/143 - https://github.com/LabKey/snprcEHRModules/pull/1016 - https://github.com/LabKey/onprcEHRModules/pull/1909 --------- Co-authored-by: Marty Pradere --- .gitattributes | 1 - .../org/labkey/api/security/AuthFilter.java | 6 ++ .../labkey/api/security/SecurityManager.java | 76 +++++++++++++++---- assay/src/org/labkey/assay/AssayModule.java | 3 + core/src/org/labkey/core/CoreModule.java | 6 ++ 5 files changed, 76 insertions(+), 16 deletions(-) diff --git a/.gitattributes b/.gitattributes index fe89a3ba5dd..3d1bc142acb 100644 --- a/.gitattributes +++ b/.gitattributes @@ -2742,7 +2742,6 @@ study/test/src/org/labkey/test/tests/study/AssayTest.java -text study/test/src/org/labkey/test/tests/study/CohortTest.java -text study/test/src/org/labkey/test/tests/study/ExtraKeyStudyTest.java -text study/test/src/org/labkey/test/tests/study/QuerySnapshotTest.java -text -study/test/src/org/labkey/test/tests/study/SpecimenReplaceTest.java -text study/test/src/org/labkey/test/tests/study/StudyCohortExportTest.java -text study/test/src/org/labkey/test/tests/study/StudyContinuousTest.java -text study/test/src/org/labkey/test/tests/study/StudyDateBasedTest.java -text diff --git a/api/src/org/labkey/api/security/AuthFilter.java b/api/src/org/labkey/api/security/AuthFilter.java index 94d5aa87fef..972843fac55 100644 --- a/api/src/org/labkey/api/security/AuthFilter.java +++ b/api/src/org/labkey/api/security/AuthFilter.java @@ -40,6 +40,7 @@ import org.labkey.api.util.GUID; import org.labkey.api.util.HttpUtil; import org.labkey.api.util.HttpsUtil; +import org.labkey.api.view.BadRequestException; import org.labkey.api.view.UnauthorizedException; import org.labkey.api.view.ViewServlet; @@ -189,6 +190,11 @@ else if (!AppProps.getInstance().isDevMode()) resp.sendError(HttpServletResponse.SC_BAD_REQUEST, uee.getMessage()); return; } + catch (BadRequestException bre) + { + resp.sendError(bre.getStatus(), bre.getMessage()); + return; + } catch (UnauthorizedException ue) { ExceptionUtil.handleException(req, resp, ue, ue.getMessage(), false); diff --git a/api/src/org/labkey/api/security/SecurityManager.java b/api/src/org/labkey/api/security/SecurityManager.java index e0238f36858..c0db6f86104 100644 --- a/api/src/org/labkey/api/security/SecurityManager.java +++ b/api/src/org/labkey/api/security/SecurityManager.java @@ -44,6 +44,8 @@ import org.labkey.api.audit.AuditLogService; import org.labkey.api.audit.permissions.CanSeeAuditLogPermission; import org.labkey.api.audit.provider.GroupAuditProvider; +import org.labkey.api.cache.CacheManager; +import org.labkey.api.cache.Throttle; import org.labkey.api.data.Container; import org.labkey.api.data.ContainerManager; import org.labkey.api.data.CoreSchema; @@ -111,6 +113,7 @@ import org.labkey.api.util.emailTemplate.UserOriginatedEmailTemplate; import org.labkey.api.util.logging.LogHelper; import org.labkey.api.view.ActionURL; +import org.labkey.api.view.BadRequestException; import org.labkey.api.view.HasHttpRequest; import org.labkey.api.view.HttpView; import org.labkey.api.view.NotFoundException; @@ -176,6 +179,8 @@ public class SecurityManager public static final String TRANSFORM_SESSION_ID = "LabKeyTransformSessionId"; // issue 19748 /** GH Issue 1489: gates acceptance of the deprecated TRANSFORM_SESSION_ID cookie; default off */ public static final String FEATUREFLAG_ALLOW_TRANSFORM_SESSION_ID = "AllowTransformSessionIdAuth"; + public static final String FEATURE_FLAG_ALLOW_APIKEY_PARAMETER = "AllowApiKeyParameter"; + public static final String FEATURE_FLAG_ALLOW_APIKEY_PARAMETER_DESCRIPTION = "Allow authentication via 'apikey' URL parameter"; public static final String API_KEY = "apikey"; public static final String USER_ID_KEY = User.class.getName() + "$userId"; @@ -419,7 +424,13 @@ public void userAccountDisabled(User user) } } - private record Credentials(String username, String password) {} + private record Credentials(String username, String password, boolean shouldSetSessionCookie) + { + Credentials(String username, String password) + { + this(username, password, true); + } + } private static @Nullable Credentials getBasicCredentials(HttpServletRequest request) { @@ -568,12 +579,16 @@ public record AuthenticationAttempt(User user, HttpServletRequest request) {} String sessionId = PageFlowUtil.getCookieValue(request.getCookies(), JSESSIONID, null); if (!session.getId().equals(sessionId)) { - Cookie sessionCookie = new Cookie(JSESSIONID, session.getId()); - sessionCookie.setPath("/"); - sessionCookie.setHttpOnly(true); - if (AppProps.getInstance().isSSLRequired() || request.isSecure()) - sessionCookie.setSecure(true); - response.addCookie(sessionCookie); + // A URL can be planted in a victim's browser, so a URL parameter key must not set the session cookie + if (basicCredentials.shouldSetSessionCookie()) + { + Cookie sessionCookie = new Cookie(JSESSIONID, session.getId()); + sessionCookie.setPath("/"); + sessionCookie.setHttpOnly(true); + if (AppProps.getInstance().isSSLRequired() || request.isSecure()) + sessionCookie.setSecure(true); + response.addCookie(sessionCookie); + } request = new SessionReplacingRequest(request, session); } } @@ -670,8 +685,8 @@ public record AuthenticationAttempt(User user, HttpServletRequest request) {} /** * Determine if an API key is present, checking the "apikey" header first, then the deprecated * "LabKeyTransformSessionId" cookie (gated behind {@link #FEATUREFLAG_ALLOW_TRANSFORM_SESSION_ID}), and finally - * the "LabKeyTransformSessionId" GET parameter (supported permanently, since SSRS can't be made to use the header - * or a cookie). Return the credentials if an API key is present via any of these; otherwise return null. + * the "apikey" GET parameter (supported since SSRS can't be made to use a header, but only if the optional feature + * flag is enabled). Return the credentials if an API key is present via any of these; otherwise return null. * @param request Current request * @return First API key found or null if an apikey is not present. */ @@ -680,6 +695,7 @@ public record AuthenticationAttempt(User user, HttpServletRequest request) {} // Passing via the "apikey" HTTP header is our preferred approach and used by most LabKey client API // implementations String apiKey = request.getHeader(API_KEY); + boolean shouldSetSessionCookie = true; if (null == apiKey) { @@ -710,24 +726,54 @@ public record AuthenticationAttempt(User user, HttpServletRequest request) {} } else { - // Continue to support "LabKeyTransformSessionId" as a GET parameter, to support authentication through - // SSRS which can't be made to use BasicAuth, pass cookies, or other HTTP headers. Do not use - // request.getParameter() since that will consume the POST body, #32711. + // Continue to support "apikey" as a GET parameter only if the optional feature flag is enabled. This + // supports authentication through SSRS, which can't be made to use BasicAuth, pass cookies, or use HTTP + // headers. + Map params; try { - Map params = PageFlowUtil.mapFromQueryString(request.getQueryString()); - apiKey = params.get(TRANSFORM_SESSION_ID); + // Do not use request.getParameter() since that will consume the POST body, #32711. + params = PageFlowUtil.mapFromQueryString(request.getQueryString()); } catch (IllegalArgumentException e) { + // URLDecoder throws on malformed escapes; AuthFilter maps this to a 400 throw new UnsupportedEncodingException(e.getMessage()); } + + String apiKeyParameter = params.get(API_KEY); + + if (apiKeyParameter != null) + { + if (AppProps.getInstance().isOptionalFeatureEnabled(FEATURE_FLAG_ALLOW_APIKEY_PARAMETER)) + { + apiKey = apiKeyParameter; + shouldSetSessionCookie = false; + } + else + { + API_KEY_PARAMETER_WARNING_THROTTLE.execute("Rejected \"" + API_KEY + "\" parameter; " + + "enable the \"" + FEATURE_FLAG_ALLOW_APIKEY_PARAMETER_DESCRIPTION + "\" optional feature " + + "flag or authenticate via a different approach."); + } + } + else if (params.get(TRANSFORM_SESSION_ID) != null) + { + String message = "Rejected \"" + TRANSFORM_SESSION_ID + "\" parameter because it's no longer " + + "supported. Enable the \"" + FEATURE_FLAG_ALLOW_APIKEY_PARAMETER_DESCRIPTION + "\" optional " + + "feature flag and use the \"" + API_KEY + "\" parameter instead."; + API_KEY_PARAMETER_WARNING_THROTTLE.execute(message); + throw new BadRequestException(message); + } } } - return null != apiKey ? new Credentials(API_KEY, apiKey) : null; + return null != apiKey ? new Credentials(API_KEY, apiKey, shouldSetSessionCookie) : null; } + // Unauthenticated callers can trigger these warnings on every request + private static final Throttle API_KEY_PARAMETER_WARNING_THROTTLE = new Throttle<>("apikey parameter warnings", 10, CacheManager.HOUR, AUTH_LOG::warn); + public static final int SECONDS_PER_DAY = 60*60*24; public static abstract class TransformSession implements Closeable diff --git a/assay/src/org/labkey/assay/AssayModule.java b/assay/src/org/labkey/assay/AssayModule.java index 062512e22d9..8605523d7bf 100644 --- a/assay/src/org/labkey/assay/AssayModule.java +++ b/assay/src/org/labkey/assay/AssayModule.java @@ -119,6 +119,7 @@ import static org.labkey.api.assay.transform.DataTransformService.LEGACY_SESSION_COOKIE_NAME_REPLACEMENT; import static org.labkey.api.assay.transform.DataTransformService.LEGACY_SESSION_ID_REPLACEMENT; +import static org.labkey.api.assay.transform.DataTransformService.R_SESSIONID_REPLACEMENT; public class AssayModule extends SpringModule { @@ -184,6 +185,8 @@ protected void init() ParamReplacementSvc.get().registerDeprecated(LEGACY_SESSION_COOKIE_NAME_REPLACEMENT, ValidationException.SEVERITY.WARN, "Use '" + SecurityManager.API_KEY + "' instead"); ParamReplacementSvc.get().registerDeprecated(LEGACY_SESSION_ID_REPLACEMENT, ValidationException.SEVERITY.WARN, "Use '" + SecurityManager.API_KEY + "' instead"); + ParamReplacementSvc.get().registerDeprecated(R_SESSIONID_REPLACEMENT, ValidationException.SEVERITY.WARN, "Use '" + SecurityManager.API_KEY + "' instead"); + ParamReplacementSvc.get().registerDeprecated(SecurityManager.TRANSFORM_SESSION_ID, ValidationException.SEVERITY.WARN, "Use '" + SecurityManager.API_KEY + "' instead"); RoleManager.registerRole(new AssayDesignerRole()); diff --git a/core/src/org/labkey/core/CoreModule.java b/core/src/org/labkey/core/CoreModule.java index a96ce5ad774..7c7a1846424 100644 --- a/core/src/org/labkey/core/CoreModule.java +++ b/core/src/org/labkey/core/CoreModule.java @@ -373,6 +373,7 @@ import java.util.stream.Stream; import static org.labkey.api.mcp.McpService.VECTOR_SCHEMA; +import static org.labkey.api.security.SecurityManager.FEATURE_FLAG_ALLOW_APIKEY_PARAMETER_DESCRIPTION; import static org.labkey.api.settings.StashedStartupProperties.homeProjectFolderType; import static org.labkey.api.settings.StashedStartupProperties.homeProjectResetPermissions; import static org.labkey.api.settings.StashedStartupProperties.homeProjectWebparts; @@ -543,6 +544,11 @@ public QuerySchema createSchema(DefaultSchema schema, Module module) "Allow script authentication via legacy substitution parameters", "Allows pipeline/transform scripts to authenticate via legacy approaches ('LabKeyTransformSessionId', 'rLabkeySessionId', 'httpSessionId', and 'sessionCookieName' substitution parameters) instead of 'apikey' header authentication. This option will be removed in a future release of LabKey Server.", false, false, FeatureType.Deprecated)); + OptionalFeatureService.get().addFeatureFlag(new OptionalFeatureFlag(SecurityManager.FEATURE_FLAG_ALLOW_APIKEY_PARAMETER, + FEATURE_FLAG_ALLOW_APIKEY_PARAMETER_DESCRIPTION, + "Allows tools such as SSRS to authenticate by providing an API key via an 'apikey' parameter. Providing " + + "a credential via a URL parameter is not generally recommended, but in some cases this is the only option.", + false, false, FeatureType.Optional)); OptionalFeatureService.get().addExperimentalFeatureFlag(PageTemplate.EXPERIMENTAL_SHORT_CIRCUIT_ROBOTS, "Short-circuit robots", "Save resources by not rendering pages marked as 'noindex' for robots. This is experimental as not all robots are search engines.", From 442877661463a8178b1b5cd25b4a23f9b2ce419b Mon Sep 17 00:00:00 2001 From: Adam Rauch Date: Mon, 5 Oct 2026 13:40:22 -0700 Subject: [PATCH 09/13] Remove unused methods (#8136) ## Rationale Methods in several classes were deprecated and are now unused. ## Changes - Remove unused methods in `SecurityManager`, `ServiceRegistry`, and `ViewContext` - Format the row count in the large collection warning with commas - Spell "participant" correctly --- .../labkey/api/data/SqlExecutingSelector.java | 6 ++-- .../org/labkey/api/reader/ExcelLoader.java | 4 +-- .../labkey/api/security/SecurityManager.java | 14 +--------- .../labkey/api/services/ServiceRegistry.java | 28 ++++--------------- api/src/org/labkey/api/view/ViewContext.java | 6 ---- .../labkey/core/admin/AdminController.java | 2 +- .../study/controllers/StudyController.java | 11 ++++---- 7 files changed, 17 insertions(+), 54 deletions(-) diff --git a/api/src/org/labkey/api/data/SqlExecutingSelector.java b/api/src/org/labkey/api/data/SqlExecutingSelector.java index d77f90c83a1..b2e5b0fccd1 100644 --- a/api/src/org/labkey/api/data/SqlExecutingSelector.java +++ b/api/src/org/labkey/api/data/SqlExecutingSelector.java @@ -61,8 +61,8 @@ public abstract class SqlExecutingSelector LARGE_RESULT_WARNING_THROTTLE = new Throttle<>("SqlSelector large result warnings", 1000, CacheManager.DAY, - w -> LOGGER.warn("{} {} rows loaded into a collection via {}. Consider switching to streaming variants to reduce memory usage. SQL: {}", - w.rowCount, w.elementClass, w.selectorClass, w.sql, w.stackTrace)); + w -> LOGGER.warn("{} {} rows loaded into a collection via {}. Consider switching to streaming variants to reduce memory usage. SQL: {}", + String.format("%,d", w.rowCount), w.elementClass, w.selectorClass, w.sql, w.stackTrace)); int _maxRows = Table.ALL_ROWS; protected long _offset = Table.NO_OFFSET; @@ -191,7 +191,7 @@ public SELECTOR setJdbcCaching(boolean cache) // Log the parameterized SQL only so bound parameter values stay out of the log SQLFragment sql = getSqlFactory(false).getSql(); LARGE_RESULT_WARNING_THROTTLE.execute(new LargeResultWarning(getStackKey(stackTrace), result.size(), - clazz.getSimpleName(), getClass().getSimpleName(), sql == null ? null : sql.getSQL(), stackTrace)); + clazz.getSimpleName(), getClass().getSimpleName(), sql == null ? null : sql.getSQL(), stackTrace)); } return result; diff --git a/api/src/org/labkey/api/reader/ExcelLoader.java b/api/src/org/labkey/api/reader/ExcelLoader.java index 3c4611ac127..36c06471a8d 100644 --- a/api/src/org/labkey/api/reader/ExcelLoader.java +++ b/api/src/org/labkey/api/reader/ExcelLoader.java @@ -1321,9 +1321,9 @@ else if (isDateFormat) } else { - // Excel auto-converts lots of things that are not numbers, such particpantids and sometimes dates + // Excel auto-converts lots of things that are not numbers, such participant ids and sometimes dates // If the value is not explicitly formatted as a number then use Excel's stored string representation and let DataLoader sort it out - // NOTE: if we it is formatted as a number we generate our own string representation, + // NOTE: if it is formatted as a number, we generate our own string representation // This helps when targeting a string column // a) to avoid Excel's trailing 0000001 and 9999999 format // b) avoid scientific notation if possible diff --git a/api/src/org/labkey/api/security/SecurityManager.java b/api/src/org/labkey/api/security/SecurityManager.java index c0db6f86104..f5f7f8e01c4 100644 --- a/api/src/org/labkey/api/security/SecurityManager.java +++ b/api/src/org/labkey/api/security/SecurityManager.java @@ -3035,15 +3035,9 @@ public static Stream> getPermissions(SecurableResour return getPermissionsWithoutCheckingForbiddenProjects(resource, principal, contextualRoles); } - @Deprecated // Left behind temporarily so we don't immediately break existing ehrModules FBs. TODO: Remove - public static Set> streamPermissions(SecurableResource resource, UserPrincipal principal, Set contextualRoles) - { - return getPermissions(resource, principal, contextualRoles).collect(Collectors.toSet()); - } - /** * This method exists to allow isForbiddenProject() to check permissions on the project without reentrancy loops. - * Do not call this method unless you're isForbiddenProject(). + * isForbiddenProject() and getPermissions() are the only methods that should be calling this. */ public static Stream> getPermissionsWithoutCheckingForbiddenProjects(@NotNull SecurableResource resource, @NotNull UserPrincipal principal, @NotNull Set contextualRoles) { @@ -3061,12 +3055,6 @@ public static Stream> getPermissionsWithoutCheckingF return permissions; } - @Deprecated // Left behind temporarily so we don't immediately break existing premiumModules FBs. TODO: Remove - public static Stream> streamPermissionsWithoutCheckingForbiddenProjects(@NotNull SecurableResource resource, @NotNull UserPrincipal principal, Set contextualRoles) - { - return getPermissionsWithoutCheckingForbiddenProjects(resource, principal, contextualRoles); - } - @NotNull public static Set getPermissionNames(SecurableResource resource, @NotNull UserPrincipal principal) { diff --git a/api/src/org/labkey/api/services/ServiceRegistry.java b/api/src/org/labkey/api/services/ServiceRegistry.java index d91d71af62a..0474b367e42 100644 --- a/api/src/org/labkey/api/services/ServiceRegistry.java +++ b/api/src/org/labkey/api/services/ServiceRegistry.java @@ -29,19 +29,10 @@ import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.ConcurrentMap; -/* -* User: Dave -* Date: Nov 19, 2008 -* Time: 10:50:17 AM -*/ - /** - * Provides a central registry for service interface implementations. - * Modules that supply cross-module services should register their service - * instances at startup by calling {@link #registerService}. - * Other modules can then request that service at - * runtime by calling {@link #getService(Class)}, specifying the - * class of the service interface. + * Provides a central registry for service interface implementations. Modules that supply cross-module services should + * register their service instances at startup by calling {@link #registerService}. Other modules can then request that + * service at runtime by calling {@link #getService(Class)}, specifying the class of the service interface. */ public class ServiceRegistry { @@ -63,12 +54,12 @@ else if (name.endsWith("$I")) } final String shortName; final String longName; - final Class cls; + final Class cls; final Object instance; } private static final ServiceRegistry _instance = new ServiceRegistry(); - private final ConcurrentMap _servicesByClass = new ConcurrentHashMap<>(); + private final ConcurrentMap, _ServiceDef> _servicesByClass = new ConcurrentHashMap<>(); static {ServiceRegistry._instance.registerService(ServiceRegistry.class, _instance);} @@ -104,15 +95,6 @@ public boolean hasService(Class type) return getService(type) != null; } - - /** Returns a service implementation for a given service interface. */ - @Deprecated // Use ServiceRegistry.get().getService() instead - public static T get(Class type) - { - return get().getService(type); - } - - /** * Registers a service implementation. Modules that expose services should call this method * at load time, passing the service interface class and the implementation instance. diff --git a/api/src/org/labkey/api/view/ViewContext.java b/api/src/org/labkey/api/view/ViewContext.java index 19247f921af..a2a44eddfe7 100644 --- a/api/src/org/labkey/api/view/ViewContext.java +++ b/api/src/org/labkey/api/view/ViewContext.java @@ -219,12 +219,6 @@ public Object get(String key) return _map.get(key); } - @Deprecated // Left behind so not every module needs to be recompiled immediately. TODO: Remove - public Object get(Object key) - { - return _map.get(key); - } - /* * Safer and more convenient than using get() with a String cast. Returns _map.get(key) if it's null or a String. * Otherwise, throws BadRequestException. See GH Issue 1631. diff --git a/core/src/org/labkey/core/admin/AdminController.java b/core/src/org/labkey/core/admin/AdminController.java index 6a59699f699..5ebb97817ff 100644 --- a/core/src/org/labkey/core/admin/AdminController.java +++ b/core/src/org/labkey/core/admin/AdminController.java @@ -12376,7 +12376,7 @@ public class ViewUsageStatisticsAction extends SimpleViewAction @Override public ModelAndView getView(Object o, BindException errors) { - return ModuleHtmlView.get(ModuleLoader.getInstance().getModule("core"), ModuleHtmlView.getGeneratedViewPath("ViewUsageStatistics")); + return ModuleHtmlView.get(ModuleLoader.getInstance().getCoreModule(), ModuleHtmlView.getGeneratedViewPath("ViewUsageStatistics")); } @Override diff --git a/study/src/org/labkey/study/controllers/StudyController.java b/study/src/org/labkey/study/controllers/StudyController.java index 8e1a4ec86ca..df2b78abba2 100644 --- a/study/src/org/labkey/study/controllers/StudyController.java +++ b/study/src/org/labkey/study/controllers/StudyController.java @@ -1096,19 +1096,18 @@ public void addNavTrail(NavTree root) } } - - Participant findParticipant(Study study, String particpantId) throws StudyManager.ParticipantNotUniqueException + Participant findParticipant(Study study, String participantId) throws StudyManager.ParticipantNotUniqueException { - Participant participant = StudyManager.getInstance().getParticipant(study, particpantId); + Participant participant = StudyManager.getInstance().getParticipant(study, participantId); if (participant == null) { if (study.isDataspaceStudy()) { - Container c = StudyManager.getInstance().findParticipant(study, particpantId); + Container c = StudyManager.getInstance().findParticipant(study, participantId); Study s = null == c ? null : StudyManager.getInstance().getStudy(c); if (null != s && c.hasPermission(getUser(), ReadPermission.class)) { - participant = StudyManager.getInstance().getParticipant(s, particpantId); + participant = StudyManager.getInstance().getParticipant(s, participantId); } } } @@ -1217,7 +1216,7 @@ public class Participant2Action extends SimpleViewAction // TODO participant list support? cohortfilter support? // TODO define participant context // { -// particpantId:"", +// participantId:"", // participantGroup:"" // demoMode:false // } From 34c48df2d2cddac3c7f39251d68fa7743a7c41e5 Mon Sep 17 00:00:00 2001 From: Adam Rauch Date: Mon, 5 Oct 2026 15:04:29 -0700 Subject: [PATCH 10/13] Further optimize full-text search SecurityQuery (#8132) ## Rationale Make full-text searches faster, especially with many containers and workbooks. Last step for [GitHub Issue #819](https://github.com/LabKey/internal-issues/issues/819). ## Related Pull Requests - https://github.com/LabKey/platform/pull/8041 - https://github.com/LabKey/platform/pull/8063 ## Changes - Switch to a single pass through the candidate containers to find read and additional permissions - Check each security policy once and stash the permissions found - Use streams and bitmasks instead of sets ## Tasks - [x] Development - [x] Claude Code Review - [x] Test Automation - added junit tests - [x] Code Review - [x] Manual Testing - [ ] TeamCity and Merge ## User Education - On deployments with thousands of folders and workbooks, full-text searches are now up to 20x faster --------- Co-authored-by: labkey-jeckels --- api/src/org/labkey/api/ApiModule.java | 2 + .../org/labkey/api/search/SearchScope.java | 385 ++++++++++++++++-- .../src/org/labkey/search/SearchModule.java | 3 +- .../labkey/search/model/SecurityQuery.java | 262 ++++++++++-- 4 files changed, 578 insertions(+), 74 deletions(-) diff --git a/api/src/org/labkey/api/ApiModule.java b/api/src/org/labkey/api/ApiModule.java index d484ce6e7ee..4b8d7579c09 100644 --- a/api/src/org/labkey/api/ApiModule.java +++ b/api/src/org/labkey/api/ApiModule.java @@ -130,6 +130,7 @@ import org.labkey.api.reports.model.ViewCategoryManager; import org.labkey.api.reports.report.ReportType; import org.labkey.api.reports.report.r.RReport; +import org.labkey.api.search.SearchScope; import org.labkey.api.security.ApiKeyManager; import org.labkey.api.security.ApiKeyManager.ApiKeyMaintenanceTask; import org.labkey.api.security.AuthenticationConfiguration; @@ -580,6 +581,7 @@ public void registerServlets(ServletContext servletCtx) ResultSetSelectorTestCase.class, RoleSet.TestCase.class, RowTrackingResultSetWrapper.TestCase.class, + SearchScope.TestCase.class, SecurityManager.TestCase.class, SimpleQueryUpdateService.TestCase.class, SimpleTranslator.TranslateTestCase.class, diff --git a/api/src/org/labkey/api/search/SearchScope.java b/api/src/org/labkey/api/search/SearchScope.java index 674e1d0794f..438a35734c5 100644 --- a/api/src/org/labkey/api/search/SearchScope.java +++ b/api/src/org/labkey/api/search/SearchScope.java @@ -15,20 +15,48 @@ */ package org.labkey.api.search; +import org.junit.AfterClass; +import org.junit.Assert; +import org.junit.BeforeClass; +import org.junit.Test; import org.labkey.api.data.Container; +import org.labkey.api.data.Container.LockState; import org.labkey.api.data.ContainerManager; import org.labkey.api.data.ContainerType; +import org.labkey.api.data.WorkbookContainerType; +import org.labkey.api.security.ClonedUser; +import org.labkey.api.security.Group; +import org.labkey.api.security.MutableSecurityPolicy; +import org.labkey.api.security.PermissionsContext; +import org.labkey.api.security.SecurityManager; +import org.labkey.api.security.SecurityPolicyManager; import org.labkey.api.security.User; +import org.labkey.api.security.UserManager; +import org.labkey.api.security.ValidEmail; +import org.labkey.api.security.impersonation.RoleImpersonationContextFactory; +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.security.roles.EditorRole; +import org.labkey.api.security.roles.ReaderRole; +import org.labkey.api.security.roles.RoleManager; +import org.labkey.api.test.TestTimeout; import org.labkey.api.util.SafeToRenderEnum; +import org.labkey.api.util.TestContext; import java.util.HashMap; +import java.util.HashSet; +import java.util.Iterator; +import java.util.LinkedHashMap; import java.util.List; +import java.util.Map; +import java.util.Set; +import java.util.stream.Collectors; +import java.util.stream.Stream; /** * Options for how widely or narrowly to search on the server, based on the number of containers to include. - * User: adam - * Date: 2/18/12 */ public enum SearchScope implements SafeToRenderEnum { @@ -86,15 +114,9 @@ public Container getRoot(Container c) } @Override - protected HashMap _getSearchableContainers(User user, Container currentContainer) + protected Stream getCandidateContainers(User user, Container searchRoot, Container currentContainer) { - HashMap containers = Folder._getSearchableContainers(user, currentContainer); - - Container project = Project.getRoot(currentContainer); - if (project.hasPermission(user, ReadPermission.class)) - containers.put(project.getId(), project); - - return containers; + return Stream.of(searchRoot, Project.getRoot(searchRoot)); } }, FolderAndProjectAndShared(false, true) { @@ -105,9 +127,9 @@ public Container getRoot(Container c) } @Override - protected HashMap _getSearchableContainers(User user, Container currentContainer) + protected Stream getCandidateContainers(User user, Container searchRoot, Container currentContainer) { - return FolderAndProject._getSearchableContainers(user, currentContainer); + return FolderAndProject.getCandidateContainers(user, searchRoot, currentContainer); } }; @@ -132,49 +154,336 @@ public boolean includeShared() return _includeShared; } - public HashMap getSearchableContainers(User user, Container currentContainer) + /** + * @param readable Containers in scope where the user has read permission, keyed by container ID + * @param containerIdsByPermission For each requested additional permission, the IDs of readable containers where the user also holds it + */ + public record SearchableContainers(Map readable, Map, Set> containerIdsByPermission) {} + + public SearchableContainers getSearchableContainers(User user, Container currentContainer, Set> additionalPermissions) { - Container searchRoot = this.getRoot(currentContainer); - HashMap containers = this.isRecursive() ? - getRecursiveContainers(user, searchRoot, currentContainer): - _getSearchableContainers(user, searchRoot); + Stream candidates = getCandidateContainers(user, getRoot(currentContainer), currentContainer); - Container shared = ContainerManager.getSharedContainer(); - if (this.includeShared() && shared.hasPermission(user, ReadPermission.class)) - { - containers.put(shared.getId(), shared); - } + if (includeShared()) + candidates = Stream.concat(candidates, Stream.of(ContainerManager.getSharedContainer())); + + return resolvePermissions(user, candidates, additionalPermissions); + } + + /** Containers to consider for this scope, prior to any permission check */ + protected Stream getCandidateContainers(User user, Container searchRoot, Container currentContainer) + { + if (!isRecursive()) + return Stream.of(searchRoot); + + // Root plus all children, including workbooks & tabs + return ContainerManager.getAllChildren(searchRoot).stream() + .filter(c -> (c.isSearchable() || c.equals(currentContainer)) && (c.isContainerFor(ContainerType.DataType.search) || c.shouldDisplay(user))); + } + + /** + * Resolves the user's permissions once per distinct policy instead of once per container, since containers that + * inherit their policy (e.g., workbooks) resolve identically to the ancestor that holds it. + */ + static SearchableContainers resolvePermissions(User user, Stream candidates, Set> additionalPermissions) + { + Map, Long> bitByPermission = new HashMap<>(); + bitByPermission.put(ReadPermission.class, READ_BIT); + additionalPermissions.forEach(permission -> bitByPermission.putIfAbsent(permission, 1L << bitByPermission.size())); - return containers; + if (bitByPermission.size() > Long.SIZE) + throw new IllegalStateException("Too many additional permissions to track: " + bitByPermission.size()); + + long allBits = bitByPermission.values().stream().reduce(0L, (a, b) -> a | b); + + Map grantedByPolicy = new HashMap<>(); + Map readable = new HashMap<>(); + Map, Set> containerIdsByPermission = additionalPermissions.stream() + .collect(Collectors.toMap(permission -> permission, _ -> new HashSet<>())); + + candidates.forEach(c -> { + long granted = grantedByPolicy.computeIfAbsent(getPolicyKey(c), _ -> getGrantedBits(c, user, bitByPermission, allBits)); + + if ((granted & READ_BIT) != 0) + { + readable.put(c.getId(), c); + + containerIdsByPermission.forEach((permission, containerIds) -> { + if ((granted & bitByPermission.get(permission)) != 0) + containerIds.add(c.getId()); + }); + } + }); + + return new SearchableContainers(readable, containerIdsByPermission); } - protected HashMap _getSearchableContainers(User user, Container searchRoot) + private static final long READ_BIT = 1L; + + // Stops consuming the stream once every tracked permission is found, since some users are granted 100+ permissions + private static long getGrantedBits(Container c, User user, Map, Long> bitByPermission, long allBits) { - HashMap containers = new HashMap<>(); + long granted = 0; + Iterator> permissions = SecurityManager.getPermissions(c, user, Set.of()).iterator(); - if (searchRoot.hasPermission(user, ReadPermission.class)) - containers.put(searchRoot.getId(), searchRoot); + while (granted != allBits && permissions.hasNext()) + { + Long bit = bitByPermission.get(permissions.next()); + if (null != bit) + granted |= bit; + } - return containers; + return granted; } - protected HashMap getRecursiveContainers(User user, Container searchRoot, Container currentContainer) + // Permissions also depend on root-ness and project (locked/impersonation checks), and a project without a policy inherits root's + private static String getPolicyKey(Container c) { - // Returns root plus all children (including workbooks & tabs) where user has read permissions - List containers = ContainerManager.getAllChildren(searchRoot, user); - HashMap containerIds = new HashMap<>(containers.size() * 2); + Container project = c.getProject(); + return c.getPolicy().getResourceId() + "|" + (null == project ? "" : project.getId()); + } + + // Container deletion dominates; each test container takes several seconds to delete + @TestTimeout(240) + public static class TestCase extends Assert + { + private static final String PROJECT_NAME = "SearchScopeTestProject"; + private static final String OTHER_PROJECT_NAME = "SearchScopeTestOtherProject"; + private static final String ROOT_POLICY_PROJECT_NAME = "SearchScopeTestRootPolicyProject"; + private static final String EMAIL = "search_scope_test@test.com"; + private static final String GROUP_MEMBER_EMAIL = "search_scope_group_test@test.com"; + private static final Set> EXTRA_PERMISSIONS = Set.of(InsertPermission.class, DeletePermission.class); + + private static User _admin; + private static User _user; + private static User _groupMember; + private static Container _project; + private static Container _inherited; + private static Container _workbook; + private static Container _editable; + private static Container _restricted; + private static Container _unsearchable; + private static Container _otherProject; + private static Container _rootPolicyProject; + + @BeforeClass + public static void setUp() throws Exception + { + cleanup(); + _admin = TestContext.get().getUser(); + _user = SecurityManager.addUser(new ValidEmail(EMAIL), null).getUser(); + _groupMember = SecurityManager.addUser(new ValidEmail(GROUP_MEMBER_EMAIL), null).getUser(); + + _project = ContainerManager.createContainer(ContainerManager.getRoot(), PROJECT_NAME, _admin); + Group readers = SecurityManager.createGroup(_project, "Readers", _admin); + SecurityManager.addMember(readers, _groupMember); + MutableSecurityPolicy projectPolicy = new MutableSecurityPolicy(_project.getPolicy()); + projectPolicy.addRoleAssignment(_user, ReaderRole.class); + projectPolicy.addRoleAssignment(readers, ReaderRole.class); + SecurityPolicyManager.savePolicyForTests(projectPolicy, _admin); + + // Subfolders created by an admin get an admin-only policy, so explicitly inherit + _inherited = createInheritingFolder(_project, "Inherited"); + _workbook = ContainerManager.createContainer(_inherited, null, "Workbook", null, WorkbookContainerType.NAME, _admin); + assertEquals(_project.getPolicy().getResourceId(), _workbook.getPolicy().getResourceId()); + + _editable = ContainerManager.createContainer(_project, "Editable", _admin); + MutableSecurityPolicy editablePolicy = new MutableSecurityPolicy(_editable); + editablePolicy.addRoleAssignment(_user, EditorRole.class); + SecurityPolicyManager.savePolicyForTests(editablePolicy, _admin); + + _restricted = ContainerManager.createContainer(_project, "Restricted", _admin); + SecurityPolicyManager.savePolicyForTests(new MutableSecurityPolicy(_restricted), _admin); + + _unsearchable = createInheritingFolder(_project, "Unsearchable"); + ContainerManager.updateSearchable(_unsearchable, false, _admin); + _unsearchable = ContainerManager.getForId(_unsearchable.getId()); + + _otherProject = ContainerManager.createContainer(ContainerManager.getRoot(), OTHER_PROJECT_NAME, _admin); + MutableSecurityPolicy otherPolicy = new MutableSecurityPolicy(_otherProject.getPolicy()); + otherPolicy.addRoleAssignment(_user, ReaderRole.class); + SecurityPolicyManager.savePolicyForTests(otherPolicy, _admin); + + // A project with no policy of its own resolves to root's policy, sharing its resource ID with root and every other such project + _rootPolicyProject = ContainerManager.createContainer(ContainerManager.getRoot(), ROOT_POLICY_PROJECT_NAME, _admin); + SecurityPolicyManager.deletePolicy(_rootPolicyProject); + assertEquals(ContainerManager.getRoot().getPolicy().getResourceId(), _rootPolicyProject.getPolicy().getResourceId()); + } + + private static Container createInheritingFolder(Container parent, String name) + { + Container c = ContainerManager.createContainer(parent, name, _admin); + SecurityManager.setInheritPermissions(c); + return c; + } + + @AfterClass + public static void cleanup() throws Exception + { + for (String name : List.of(PROJECT_NAME, OTHER_PROJECT_NAME, ROOT_POLICY_PROJECT_NAME)) + { + Container project = ContainerManager.getForPath(name); + if (null != project) + ContainerManager.deleteAll(project, TestContext.get().getUser()); + } + + for (String email : List.of(EMAIL, GROUP_MEMBER_EMAIL)) + { + User user = UserManager.getUser(new ValidEmail(email)); + if (null != user) + UserManager.deleteUser(user.getUserId()); + } + } + + @Test + public void testResolvedPermissionsMatchPerContainerChecks() + { + Set candidates = ContainerManager.getAllChildren(_project); + SearchableContainers result = resolvePermissions(_user, candidates.stream(), Set.of(InsertPermission.class)); + Set insertable = result.containerIdsByPermission().get(InsertPermission.class); + + for (Container c : candidates) + { + boolean canRead = c.hasPermission(_user, ReadPermission.class); + assertEquals(c.getPath(), canRead, result.readable().containsKey(c.getId())); + assertEquals(c.getPath(), canRead && c.hasPermission(_user, InsertPermission.class), insertable.contains(c.getId())); + } + + assertEquals(Set.of(_project.getId(), _inherited.getId(), _workbook.getId(), _editable.getId(), _unsearchable.getId()), result.readable().keySet()); + assertEquals(Set.of(_editable.getId()), insertable); + } + + @Test + public void testScopes() + { + assertEquals(Set.of(_inherited.getId()), Folder.getSearchableContainers(_user, _inherited, Set.of()).readable().keySet()); + assertEquals(Set.of(), Folder.getSearchableContainers(_user, _restricted, Set.of()).readable().keySet()); + assertEquals(Set.of(_editable.getId(), _project.getId()), FolderAndProject.getSearchableContainers(_user, _editable, Set.of()).readable().keySet()); + + Set recursive = FolderAndSubfolders.getSearchableContainers(_user, _project, Set.of()).readable().keySet(); + assertTrue(recursive.containsAll(Set.of(_project.getId(), _inherited.getId(), _editable.getId()))); + assertFalse(recursive.contains(_restricted.getId())); + assertFalse(recursive.contains(_unsearchable.getId())); + + // Searching from within an unsearchable folder still includes it + assertTrue(FolderAndSubfolders.getSearchableContainers(_user, _unsearchable, Set.of()).readable().containsKey(_unsearchable.getId())); + } + + @Test + public void testEveryScopeMatchesPerContainerChecks() + { + assertMatchesPerContainerChecks(getUsers(), List.of(_project, _inherited, _workbook, _editable, _restricted, _unsearchable, _otherProject, _rootPolicyProject)); + } - for (Container c : containers) + @Test + public void testImpersonationRestrictedToProject() { - //Read permission is already checked in the 'getAllChildren' method - boolean searchable = (c.isSearchable() || c.equals(currentContainer)) && (c.isContainerFor(ContainerType.DataType.search) || c.shouldDisplay(user)); + User impersonator = getProjectImpersonator(); + assertEquals(Set.of(), Folder.getSearchableContainers(impersonator, _otherProject, Set.of()).readable().keySet()); + + Set all = All.getSearchableContainers(impersonator, _project, Set.of()).readable().keySet(); + assertTrue(all.contains(_project.getId())); + assertFalse(all.contains(_otherProject.getId())); + assertFalse(all.contains(_rootPolicyProject.getId())); + } - if (searchable) + @Test + public void testLockedProject() + { + ContainerManager.updateLockState(_otherProject, LockState.Inaccessible, () -> {}); + + try + { + assertMatchesPerContainerChecks(getUsers(), List.of(_project, ContainerManager.getForId(_otherProject.getId()))); + } + finally { - containerIds.put(c.getId(), c); + ContainerManager.updateLockState(_otherProject, LockState.Unlocked, () -> {}); } } - return containerIds; + private Map getUsers() + { + Map users = new LinkedHashMap<>(); + users.put("reader", _user); + users.put("group member", _groupMember); + users.put("site admin", _admin); + users.put("guest", User.guest); + users.put("project impersonator", getProjectImpersonator()); + return users; + } + + // Admin impersonating Reader, restricted to _project + private User getProjectImpersonator() + { + RoleImpersonationContextFactory factory = new RoleImpersonationContextFactory(_project, _admin, Set.of(RoleManager.getRole(ReaderRole.class)), Set.of(), null); + return new ImpersonatingUser(_admin, factory.getImpersonationContext()); + } + + private static class ImpersonatingUser extends ClonedUser + { + ImpersonatingUser(User user, PermissionsContext ctx) + { + super(user, ctx); + } + } + + private void assertMatchesPerContainerChecks(Map users, List currentContainers) + { + for (SearchScope scope : SearchScope.values()) + { + for (Map.Entry entry : users.entrySet()) + { + for (Container current : currentContainers) + { + // All is rooted at the root regardless of current container; one pass is enough + if (scope == All && !current.equals(currentContainers.getFirst())) + continue; + + User user = entry.getValue(); + String msg = scope + " as " + entry.getKey() + " from " + current.getPath(); + Set expected = getExpectedReadable(scope, user, current); + SearchableContainers actual = scope.getSearchableContainers(user, current, EXTRA_PERMISSIONS); + + assertEquals(msg, getIds(expected.stream()), actual.readable().keySet()); + + for (Class permission : EXTRA_PERMISSIONS) + assertEquals(msg + ", " + permission.getSimpleName(), getIds(expected.stream().filter(c -> c.hasPermission(user, permission))), actual.containerIdsByPermission().get(permission)); + } + } + } + } + + // The pre-optimization algorithm, which checks permissions on every container individually + private static Set getExpectedReadable(SearchScope scope, User user, Container current) + { + Container root = scope.getRoot(current); + Set expected = new HashSet<>(); + + if (scope.isRecursive()) + { + ContainerManager.getAllChildren(root, user).stream() + .filter(c -> (c.isSearchable() || c.equals(current)) && (c.isContainerFor(ContainerType.DataType.search) || c.shouldDisplay(user))) + .forEach(expected::add); + } + else + { + List roots = scope == FolderAndProject || scope == FolderAndProjectAndShared ? List.of(root, root.getProject()) : List.of(root); + roots.stream() + .filter(c -> c.hasPermission(user, ReadPermission.class)) + .forEach(expected::add); + } + + Container shared = ContainerManager.getSharedContainer(); + if (scope.includeShared() && shared.hasPermission(user, ReadPermission.class)) + expected.add(shared); + + return expected; + } + + private static Set getIds(Stream containers) + { + return containers.map(Container::getId).collect(Collectors.toSet()); + } } } diff --git a/search/src/org/labkey/search/SearchModule.java b/search/src/org/labkey/search/SearchModule.java index 1d313e9f65c..902bef6009a 100644 --- a/search/src/org/labkey/search/SearchModule.java +++ b/search/src/org/labkey/search/SearchModule.java @@ -270,7 +270,8 @@ private void reindexIfNeeded(@NotNull SearchService ss) ( LuceneSearchServiceImpl.TestCase.class, LuceneSearchServiceImpl.TikaTestCase.class, - LuceneSearchServiceImpl.IndexWriterTestCase.class + LuceneSearchServiceImpl.IndexWriterTestCase.class, + SecurityQuery.FilterTestCase.class ); } diff --git a/search/src/org/labkey/search/model/SecurityQuery.java b/search/src/org/labkey/search/model/SecurityQuery.java index 2b790fb07b9..0c1774aa2d8 100644 --- a/search/src/org/labkey/search/model/SecurityQuery.java +++ b/search/src/org/labkey/search/model/SecurityQuery.java @@ -19,49 +19,74 @@ import org.apache.commons.collections4.MultiValuedMap; import org.apache.commons.collections4.multimap.ArrayListValuedHashMap; import org.apache.commons.lang3.StringUtils; +import org.apache.lucene.document.BinaryDocValuesField; +import org.apache.lucene.document.Document; +import org.apache.lucene.document.Field; +import org.apache.lucene.document.StringField; import org.apache.lucene.index.BinaryDocValues; +import org.apache.lucene.index.DirectoryReader; +import org.apache.lucene.index.IndexWriter; +import org.apache.lucene.index.IndexWriterConfig; import org.apache.lucene.index.LeafReader; import org.apache.lucene.index.LeafReaderContext; +import org.apache.lucene.index.StoredFields; import org.apache.lucene.search.ConstantScoreScorer; import org.apache.lucene.search.ConstantScoreWeight; import org.apache.lucene.search.IndexSearcher; import org.apache.lucene.search.Query; import org.apache.lucene.search.QueryVisitor; +import org.apache.lucene.search.ScoreDoc; import org.apache.lucene.search.ScoreMode; import org.apache.lucene.search.Scorer; import org.apache.lucene.search.ScorerSupplier; +import org.apache.lucene.search.TopDocs; import org.apache.lucene.search.Weight; +import org.apache.lucene.store.ByteBuffersDirectory; +import org.apache.lucene.store.Directory; import org.apache.lucene.util.BitSetIterator; import org.apache.lucene.util.BytesRef; import org.apache.lucene.util.FixedBitSet; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; +import org.junit.AfterClass; import org.junit.Assert; +import org.junit.BeforeClass; 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.SearchScope.SearchableContainers; import org.labkey.api.search.SearchService; import org.labkey.api.search.SearchService.SearchCategory; +import org.labkey.api.security.MutableSecurityPolicy; import org.labkey.api.security.SecurableResource; import org.labkey.api.security.SecurityManager; +import org.labkey.api.security.SecurityPolicyManager; import org.labkey.api.security.User; +import org.labkey.api.security.UserManager; +import org.labkey.api.security.ValidEmail; 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.security.roles.EditorRole; +import org.labkey.api.security.roles.ReaderRole; +import org.labkey.api.util.GUID; +import org.labkey.api.util.MultiPhaseCPUTimer; import org.labkey.api.util.MultiPhaseCPUTimer.InvocationTimer; +import org.labkey.api.util.TestContext; import org.labkey.search.model.LuceneSearchServiceImpl.FIELD_NAME; import java.io.IOException; import java.util.Collection; +import java.util.EnumSet; import java.util.HashMap; import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Set; -import java.util.stream.Collectors; +import java.util.function.Function; import static org.apache.lucene.search.DocIdSetIterator.NO_MORE_DOCS; @@ -72,11 +97,16 @@ public class SecurityQuery extends Query private final boolean _recursive; private final HashMap> _categoryContainers = new HashMap<>(); - private final HashMap _containerIds; + private final Map _containerIds; private final HashMap _securableResourceIds = new HashMap<>(); private final InvocationTimer _iTimer; SecurityQuery(User user, SearchScope searchScope, Container currentContainer, InvocationTimer iTimer) + { + this(user, searchScope, currentContainer, iTimer, SearchService.get().getSearchCategories()); + } + + SecurityQuery(User user, SearchScope searchScope, Container currentContainer, InvocationTimer iTimer, Collection searchCategories) { // These three are used for hashCode() & equals(). We have disabled query caching for now (see #26416), but this gets us close to being able to use it. We // need to add some indication that permissions haven't changed since the query was cached, for example, include in the hash a counter that SecurityManager @@ -86,45 +116,20 @@ public class SecurityQuery extends Query _recursive = searchScope.isRecursive(); _iTimer = iTimer; - _containerIds = searchScope.getSearchableContainers(user, currentContainer); + // Categories that require only base container Read are resolved directly; the rest are grouped by required + // permission so categories that share one (e.g., the three assay categories all require AssayReadPermission) + // share a single container set. + CategoryPermissions categoryPermissions = groupCategoriesByRequiredPermission(searchCategories); + Map, Collection> categoriesByPermission = categoryPermissions.categoriesByPermission(); - // 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. - CategoryPermissions categoryPermissions = groupCategoriesByRequiredPermission(SearchService.get().getSearchCategories()); + SearchableContainers searchable = searchScope.getSearchableContainers(user, currentContainer, categoriesByPermission.keySet()); + _containerIds = searchable.readable(); for (String categoryName : categoryPermissions.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. - Map, Collection> categoriesByPermission = categoryPermissions.categoriesByPermission(); - Set> requiredPermissions = categoriesByPermission.keySet(); - HashMap>> permissionsByPolicy = new HashMap<>(); - - if (!requiredPermissions.isEmpty()) - { - for (Container c : _containerIds.values()) - { - permissionsByPolicy.computeIfAbsent(c.getPolicy().getResourceId(), _ -> SecurityManager.getPermissions(c, user, Set.of()) - .filter(requiredPermissions::contains) - .collect(Collectors.toSet())); - } - } - categoriesByPermission.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()); - } + Set permittedContainerIds = searchable.containerIdsByPermission().get(requiredPermission); for (SearchCategory category : categories) _categoryContainers.put(category.getName(), permittedContainerIds); @@ -453,4 +458,191 @@ public void testMixOfBaseReadAndPermissionRequiringCategories() assertEquals(List.of(data), result.categoriesByPermission().get(InsertPermission.class)); } } + + public static class FilterTestCase extends Assert + { + private static final String PROJECT_NAME = "SecurityQueryTestProject"; + private static final String EMAIL = "security_query_test@test.com"; + private static final SearchCategory BASE_CATEGORY = new SearchCategory("securityQueryTestBase", "Base"); + private static final SearchCategory INSERT_CATEGORY = TestCase.categoryRequiring("securityQueryTestInsert", InsertPermission.class); + private static final String ID_FIELD = "id"; + + /** One document of each kind is indexed in every test folder */ + private enum DocKind + { + BASE(c -> c.getId() + "|" + BASE_CATEGORY.getName()), + INSERT(c -> c.getId() + "|" + INSERT_CATEGORY.getName()), + UNKNOWN_CATEGORY(c -> c.getId() + "|notARegisteredCategory"), + NO_CATEGORY(Container::getId); + + private final Function _securityContext; + + DocKind(Function securityContext) + { + _securityContext = securityContext; + } + + String getId(Container c) + { + return c.getName() + ":" + name(); + } + } + + private static final Set ALL_KINDS = EnumSet.allOf(DocKind.class); + private static final Set READ_ONLY_KINDS = EnumSet.complementOf(EnumSet.of(DocKind.INSERT)); + + /** Documents in the project whose securable resource ID must also pass a read check */ + private enum ResourceDoc + { + READABLE_FOLDER_RESOURCE, + RESTRICTED_FOLDER_RESOURCE, + NON_CONTAINER_RESOURCE + } + + private static User _admin; + private static User _user; + private static Container _project; + private static Container _inherited; + private static Container _editable; + private static Container _restricted; + private static Directory _directory; + private static DirectoryReader _reader; + + @BeforeClass + public static void setUp() throws Exception + { + cleanup(); + _admin = TestContext.get().getUser(); + _user = SecurityManager.addUser(new ValidEmail(EMAIL), null).getUser(); + + _project = ContainerManager.createContainer(ContainerManager.getRoot(), PROJECT_NAME, _admin); + MutableSecurityPolicy projectPolicy = new MutableSecurityPolicy(_project.getPolicy()); + projectPolicy.addRoleAssignment(_user, ReaderRole.class); + SecurityPolicyManager.savePolicyForTests(projectPolicy, _admin); + + _inherited = ContainerManager.createContainer(_project, "Inherited", _admin); + SecurityManager.setInheritPermissions(_inherited); + + _editable = ContainerManager.createContainer(_project, "Editable", _admin); + MutableSecurityPolicy editablePolicy = new MutableSecurityPolicy(_editable); + editablePolicy.addRoleAssignment(_user, EditorRole.class); + SecurityPolicyManager.savePolicyForTests(editablePolicy, _admin); + + _restricted = ContainerManager.createContainer(_project, "Restricted", _admin); + SecurityPolicyManager.savePolicyForTests(new MutableSecurityPolicy(_restricted), _admin); + + _directory = new ByteBuffersDirectory(); + + try (IndexWriter writer = new IndexWriter(_directory, new IndexWriterConfig())) + { + for (Container c : getAllFolders()) + for (DocKind kind : DocKind.values()) + addDocument(writer, kind.getId(c), kind._securityContext.apply(c)); + + for (ResourceDoc doc : ResourceDoc.values()) + { + String resourceId = switch (doc) + { + case READABLE_FOLDER_RESOURCE -> _editable.getId(); + case RESTRICTED_FOLDER_RESOURCE -> _restricted.getId(); + case NON_CONTAINER_RESOURCE -> GUID.makeGUID(); + }; + addDocument(writer, doc.name(), DocKind.BASE._securityContext.apply(_project) + "|" + resourceId); + } + } + + _reader = DirectoryReader.open(_directory); + } + + private static List getAllFolders() + { + return List.of(_project, _inherited, _editable, _restricted); + } + + private static void addDocument(IndexWriter writer, String id, String securityContext) throws IOException + { + Document doc = new Document(); + doc.add(new StringField(ID_FIELD, id, Field.Store.YES)); + doc.add(new BinaryDocValuesField(FIELD_NAME.securityContext.name(), new BytesRef(securityContext))); + writer.addDocument(doc); + } + + @AfterClass + public static void cleanup() throws Exception + { + if (null != _reader) + _reader.close(); + if (null != _directory) + _directory.close(); + _reader = null; + _directory = null; + + Container project = ContainerManager.getForPath(PROJECT_NAME); + if (null != project) + ContainerManager.deleteAll(project, TestContext.get().getUser()); + + User user = UserManager.getUser(new ValidEmail(EMAIL)); + if (null != user) + UserManager.deleteUser(user.getUserId()); + } + + @Test + public void testReaderAcrossSubfolders() throws IOException + { + Set expected = getIds(List.of(_project, _inherited), READ_ONLY_KINDS); + expected.addAll(getIds(List.of(_editable), ALL_KINDS)); + expected.add(ResourceDoc.READABLE_FOLDER_RESOURCE.name()); + + assertEquals(expected, search(_user, SearchScope.FolderAndSubfolders, _project)); + } + + @Test + public void testFolderScope() throws IOException + { + assertEquals(getIds(List.of(_editable), ALL_KINDS), search(_user, SearchScope.Folder, _editable)); + assertEquals(Set.of(), search(_user, SearchScope.Folder, _restricted)); + } + + @Test + public void testGuest() throws IOException + { + assertEquals(Set.of(), search(User.guest, SearchScope.FolderAndSubfolders, _project)); + } + + @Test + public void testSiteAdmin() throws IOException + { + assertTrue(_admin.hasSiteAdminPermission()); + Set hits = search(_admin, SearchScope.FolderAndSubfolders, _project); + + assertTrue(hits.containsAll(getIds(getAllFolders(), ALL_KINDS))); + assertTrue(hits.contains(ResourceDoc.RESTRICTED_FOLDER_RESOURCE.name())); + } + + private static Set getIds(Collection folders, Set kinds) + { + Set ids = new HashSet<>(); + + for (Container c : folders) + for (DocKind kind : kinds) + ids.add(kind.getId(c)); + + return ids; + } + + private Set search(User user, SearchScope scope, Container current) throws IOException + { + IndexSearcher searcher = new IndexSearcher(_reader); + InvocationTimer timer = new MultiPhaseCPUTimer<>(SearchService.SEARCH_PHASE.class, SearchService.SEARCH_PHASE.values()).getInvocationTimer(); + Query query = new SecurityQuery(user, scope, current, timer, List.of(BASE_CATEGORY, INSERT_CATEGORY)); + TopDocs topDocs = searcher.search(query, _reader.maxDoc()); + StoredFields storedFields = searcher.storedFields(); + Set ids = new HashSet<>(); + + for (ScoreDoc scoreDoc : topDocs.scoreDocs) + ids.add(storedFields.document(scoreDoc.doc).get(ID_FIELD)); + + return ids; + } + } } From 393e501cd9341d49b543e36e317d801f0992a8ec Mon Sep 17 00:00:00 2001 From: Susan Hert Date: Tue, 6 Oct 2026 06:52:18 -0700 Subject: [PATCH 11/13] Issue 1603: Improve performance when creating a job with thousands of samples (#8116) --- api/src/org/labkey/api/workflow/Job.java | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/api/src/org/labkey/api/workflow/Job.java b/api/src/org/labkey/api/workflow/Job.java index b12b6e8d8b9..ba6936f4451 100644 --- a/api/src/org/labkey/api/workflow/Job.java +++ b/api/src/org/labkey/api/workflow/Job.java @@ -16,6 +16,7 @@ package org.labkey.api.workflow; import com.fasterxml.jackson.annotation.JsonIgnore; +import com.fasterxml.jackson.annotation.JsonInclude; import com.fasterxml.jackson.annotation.JsonProperty; import org.jetbrains.annotations.NotNull; import org.json.JSONObject; @@ -45,6 +46,7 @@ import java.util.Map; import java.util.Objects; import java.util.stream.Collectors; +import java.util.stream.Stream; public abstract class Job extends CreatedModified implements Identifiable { @@ -319,19 +321,29 @@ public void setAttachments(List attachments) _attachments = attachments; } + @JsonIgnore public abstract List getEntities(); + // Serialize only preloaded entities; getEntities() lazily queries them + @JsonProperty("entities") @JsonInclude(JsonInclude.Include.NON_NULL) + public List getLoadedEntities() + { + return _entities; + } + @JsonIgnore public abstract @NotNull List getSamples(); + // Backed by an open ResultSet; callers must close it @JsonIgnore - public abstract @NotNull List getSampleNames(); + public abstract @NotNull Stream getSampleNames(); @JsonIgnore public abstract @NotNull List getSources(); + // Backed by an open ResultSet; callers must close it @JsonIgnore - public abstract @NotNull List getSourceNames(); + public abstract @NotNull Stream getSourceNames(); public void setEntities(List entities) { From 3b04c360b8239d61b421c2371665d3f2d14879bc Mon Sep 17 00:00:00 2001 From: Susan Hert Date: Tue, 6 Oct 2026 11:10:50 -0700 Subject: [PATCH 12/13] Issue 980: Remove deprecated UI for deriving samples in LKS interface (#8135) --- .gitattributes | 3 - api/src/org/labkey/api/settings/AppProps.java | 1 - .../DerivedSamplePropertyHelper.java | 257 --------- .../labkey/experiment/ExperimentModule.java | 7 - .../controllers/exp/ExperimentController.java | 491 +----------------- .../exp/SampleTypeContentsView.java | 15 - .../experiment/deriveSamplesChooseTarget.jsp | 99 ---- .../experiment/summarizeMaterialInputs.jsp | 102 ---- 8 files changed, 1 insertion(+), 974 deletions(-) delete mode 100644 experiment/src/org/labkey/experiment/DerivedSamplePropertyHelper.java delete mode 100644 experiment/src/org/labkey/experiment/deriveSamplesChooseTarget.jsp delete mode 100644 experiment/src/org/labkey/experiment/summarizeMaterialInputs.jsp diff --git a/.gitattributes b/.gitattributes index 3d1bc142acb..540d4205b1e 100644 --- a/.gitattributes +++ b/.gitattributes @@ -1947,8 +1947,6 @@ experiment/src/org/labkey/experiment/DataClassWebPart.java -text experiment/src/org/labkey/experiment/DataURLRelativizer.java -text experiment/src/org/labkey/experiment/DefaultCustomPropertyRenderer.java -text experiment/src/org/labkey/experiment/defaults/DefaultValueServiceImpl.java -text -experiment/src/org/labkey/experiment/DerivedSamplePropertyHelper.java -text -experiment/src/org/labkey/experiment/deriveSamplesChooseTarget.jsp -text experiment/src/org/labkey/experiment/DotGraph.java -text experiment/src/org/labkey/experiment/ExpDataFileListener.java -text experiment/src/org/labkey/experiment/ExperimentAuditProvider.java -text @@ -1988,7 +1986,6 @@ experiment/src/org/labkey/experiment/SampleTypeAuditProvider.java -text experiment/src/org/labkey/experiment/SampleTypeDisplayColumn.java -text experiment/src/org/labkey/experiment/SampleTypeWebPart.java -text experiment/src/org/labkey/experiment/StandardAndCustomPropertiesView.java -text -experiment/src/org/labkey/experiment/summarizeMaterialInputs.jsp -text experiment/src/org/labkey/experiment/types/begin.jsp -text experiment/src/org/labkey/experiment/types/typeDetails.jsp -text experiment/src/org/labkey/experiment/types/types.jsp -text diff --git a/api/src/org/labkey/api/settings/AppProps.java b/api/src/org/labkey/api/settings/AppProps.java index 5a71b15b453..a9c61fff7c4 100644 --- a/api/src/org/labkey/api/settings/AppProps.java +++ b/api/src/org/labkey/api/settings/AppProps.java @@ -46,7 +46,6 @@ public interface AppProps String SCOPE_OPTIONAL_FEATURE = "ExperimentalFeature"; // Startup property prefix for all optional features; "Experimental" for historical reasons. String OPTIONAL_NO_GUESTS = "disableGuestAccount"; String EXPERIMENTAL_BLOCKER = "blockMaliciousClients"; - String DEPRECATED_DERIVE_SAMPLES_NOT_IN_APP = "deriveSamplesNotInApp"; String ADMIN_PROVIDED_ALLOWED_EXTERNAL_RESOURCES = "allowedExternalResources"; String QUANTITY_COLUMN_SUFFIX_TESTING = "quantityColumnSuffixTesting"; String REJECT_CONTROLLER_FIRST_URLS = "rejectControllerFirstUrls"; diff --git a/experiment/src/org/labkey/experiment/DerivedSamplePropertyHelper.java b/experiment/src/org/labkey/experiment/DerivedSamplePropertyHelper.java deleted file mode 100644 index 951bd1da2d5..00000000000 --- a/experiment/src/org/labkey/experiment/DerivedSamplePropertyHelper.java +++ /dev/null @@ -1,257 +0,0 @@ -/* - * Copyright (c) 2008-2026 LabKey Corporation - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -package org.labkey.experiment; - -import org.jetbrains.annotations.NotNull; -import org.labkey.api.assay.actions.UploadWizardAction; -import org.labkey.api.collections.CaseInsensitiveHashMap; -import org.labkey.api.collections.IntHashMap; -import org.labkey.api.data.Container; -import org.labkey.api.data.DbSequence; -import org.labkey.api.data.NameGenerator; -import org.labkey.api.data.NameGeneratorState; -import org.labkey.api.exp.DuplicateMaterialException; -import org.labkey.api.exp.ExperimentException; -import org.labkey.api.exp.Lsid; -import org.labkey.api.exp.PropertyDescriptor; -import org.labkey.api.exp.SamplePropertyHelper; -import org.labkey.api.exp.XarContext; -import org.labkey.api.exp.XarFormatException; -import org.labkey.api.exp.api.ExpMaterial; -import org.labkey.api.exp.api.ExpSampleType; -import org.labkey.api.exp.api.ExperimentService; -import org.labkey.api.exp.property.DomainProperty; -import org.labkey.api.exp.xar.LsidUtils; -import org.labkey.api.security.User; -import org.labkey.api.util.Pair; -import org.labkey.experiment.api.ExpSampleTypeImpl; -import org.labkey.experiment.api.ExperimentServiceImpl; -import org.labkey.experiment.api.property.DomainPropertyImpl; - -import java.io.IOException; -import java.util.ArrayList; -import java.util.Collections; -import java.util.HashSet; -import java.util.List; -import java.util.Map; -import java.util.Set; -import java.util.function.Supplier; - -import static org.labkey.api.exp.api.ExpRunItem.PARENT_IMPORT_ALIAS_MAP_PROP; - -/** - * Gets the sample-specific values from user-provided information when creating child samples from an existing set - * of parents. - */ -public class DerivedSamplePropertyHelper extends SamplePropertyHelper -{ - private final List _names; - private final Map> _lsids = new IntHashMap<>(); - private final ExpSampleTypeImpl _sampleType; - private final Container _container; - private final User _user; - - private final DomainProperty _nameProperty; - private final NameGenerator _nameGenerator; - private NameGeneratorState _state; - private Supplier> _genIdFn; - - public DerivedSamplePropertyHelper(ExpSampleTypeImpl sampleType, int sampleCount, Container c, User user) - { - super(Collections.emptyList()); - - _sampleType = sampleType; - if (_sampleType != null) - _nameGenerator = _sampleType.getNameGenerator(c, user); - else - _nameGenerator = null; - - _container = c; - _user = user; - _names = new ArrayList<>(); - for (int i = 1; i <= sampleCount; i++) - { - _names.add("Output Sample " + i); - } - - PropertyDescriptor namePropertyDescriptor = new PropertyDescriptor(ExperimentServiceImpl.get().getTinfoMaterial().getColumn("Name"), c); - namePropertyDescriptor.setRequired(_nameGenerator == null); - _nameProperty = new DomainPropertyImpl(null, namePropertyDescriptor); - - List dps = new ArrayList<>(); - if (sampleType != null) - { - if (sampleType.hasNameAsIdCol()) - { - dps.add(_nameProperty); - } - dps.addAll(sampleType.getDomain().getProperties()); - } - else - { - dps.add(_nameProperty); - } - setDomainProperties(Collections.unmodifiableList(dps)); - } - - public ExpSampleType getSampleType() - { - return _sampleType; - } - - @Override - public List getSampleNames() - { - return _names; - } - - @Override - protected Lsid getObject(int index, @NotNull Map sampleProperties, @NotNull Set parentMaterials) throws ExperimentException - { - return getObjectWithName(index, sampleProperties, parentMaterials).first; - } - - @Override - protected Pair getObjectWithName(int index, @NotNull Map sampleProperties, @NotNull Set parentMaterials) throws ExperimentException - { - Pair lsidName = _lsids.get(index); - Lsid lsid; - String name; - boolean isDuplicate; - if (lsidName == null) - { - name = determineMaterialName(sampleProperties, parentMaterials); - if (_sampleType == null) - { - XarContext context = new XarContext("DeriveSamples", _container, _user); - try - { - String lsidStr = LsidUtils.resolveLsidFromTemplate("${FolderLSIDBase}:" + name, context, ExpMaterial.DEFAULT_CPAS_TYPE); - lsid = Lsid.parse(lsidStr); - isDuplicate = ExperimentService.get().getExpMaterial(lsid.toString()) != null; - } - catch (XarFormatException e) - { - // Shouldn't happen - our template is safe - throw new RuntimeException(e); - } - } - else - { - lsid = _sampleType.generateNextDBSeqLSID().build(); - isDuplicate = _sampleType.getSample(_container, name) != null; - } - - lsidName = new Pair<>(lsid, name); - - if (isDuplicate || _lsids.containsValue(lsidName)) - { - // Default to not showing on a particular column - String colName = "main"; - if (!getNamePDs().isEmpty() && getSampleNames().size() > index) - { - colName = UploadWizardAction.getInputName(getNamePDs().getFirst(), getSampleNames().get(index)); - } - throw new DuplicateMaterialException("Duplicate material name: " + name, colName); - } - _lsids.put(index, lsidName); - } - return lsidName; - } - - public String determineMaterialName(Map sampleProperties, Set parentSamples) throws ExperimentException - { - if (_sampleType != null) - { - if (_state == null) - { - _state = _nameGenerator.createState(true); - DbSequence sequence = _sampleType.genIdSequence(); - _genIdFn = () -> Map.of("genId", sequence.next()); - } - - Map context = new CaseInsensitiveHashMap<>(); - for (Map.Entry entry : sampleProperties.entrySet()) - { - context.put(entry.getKey().getName(), entry.getValue()); - } - try - { - List>> extraPropsFns = new ArrayList<>(); - extraPropsFns.add(_genIdFn); - - try - { - Map importAlias = _sampleType.getImportAliases(); - extraPropsFns.add(() -> - Map.of(PARENT_IMPORT_ALIAS_MAP_PROP, importAlias) - ); - } - catch (IOException e) - { - // do nothing - } - - String generatedName = _nameGenerator.generateName(_state, context, null, parentSamples, extraPropsFns); // todo add alias - _state.cleanUp(); - return generatedName; - } - catch (NameGenerator.NameGenerationException e) - { - throw new ExperimentException(e); - } - } - else - { - assert _domainProperties.getFirst().getName().equals("Name"); - return sampleProperties.get(_nameProperty); - } - } - - @Override - protected boolean isCopyable(DomainProperty pd) - { - return !getNamePDs().contains(pd); - } - - public List getNamePDs() - { - if (_sampleType != null) - { - if (_sampleType.hasNameAsIdCol()) - { - return Collections.singletonList(_nameProperty); - } - - Set idColNames = new HashSet<>(); - for (DomainProperty pd : _sampleType.getIdCols()) - idColNames.add(pd.getName()); - List properties = new ArrayList<>(); - for (DomainProperty dp : _sampleType.getDomain().getProperties()) - { - if (idColNames.contains(dp.getName())) - properties.add(dp); - } - return properties; - } - else - { - assert _domainProperties.getFirst().getName().equals("Name"); - return Collections.singletonList(_domainProperties.getFirst()); - } - } -} diff --git a/experiment/src/org/labkey/experiment/ExperimentModule.java b/experiment/src/org/labkey/experiment/ExperimentModule.java index 79ceba7d51e..a09f3b29403 100644 --- a/experiment/src/org/labkey/experiment/ExperimentModule.java +++ b/experiment/src/org/labkey/experiment/ExperimentModule.java @@ -267,13 +267,6 @@ protected void init() ExperimentService.get().registerNameExpressionType("aliquots", "exp", "MaterialSource", "aliquotnameexpression"); ExperimentService.get().registerNameExpressionType("dataclass", "exp", "DataClass", "nameexpression"); - OptionalFeatureService.get().addFeatureFlag(new OptionalFeatureFlag( - AppProps.DEPRECATED_DERIVE_SAMPLES_NOT_IN_APP, - "Derive Samples in LabKey Server UI", - "Enables the UI for deriving samples in LabKey Server UI from either the samples grids or a sample lineage page. This option will be removed in LabKey Server 26.11", - false, - false, - OptionalFeatureService.FeatureType.Deprecated)); OptionalFeatureService.get().addExperimentalFeatureFlag(SAMPLE_FILES_TABLE, "Manage Unreferenced Sample Files", "Enable 'Unreferenced Sample Files' table to view and delete sample files that are no longer referenced by samples", false); diff --git a/experiment/src/org/labkey/experiment/controllers/exp/ExperimentController.java b/experiment/src/org/labkey/experiment/controllers/exp/ExperimentController.java index 156bfe387b8..2c9ce340300 100644 --- a/experiment/src/org/labkey/experiment/controllers/exp/ExperimentController.java +++ b/experiment/src/org/labkey/experiment/controllers/exp/ExperimentController.java @@ -40,7 +40,6 @@ import org.labkey.api.action.ExportAction; import org.labkey.api.action.FormHandlerAction; import org.labkey.api.action.FormViewAction; -import org.labkey.api.action.HasViewContext; import org.labkey.api.action.Marshal; import org.labkey.api.action.Marshaller; import org.labkey.api.action.MutatingApiAction; @@ -56,7 +55,6 @@ import org.labkey.api.assay.AssayProtocolSchema; import org.labkey.api.assay.AssayProvider; import org.labkey.api.assay.AssayService; -import org.labkey.api.assay.actions.UploadWizardAction; import org.labkey.api.assay.security.DesignAssayPermission; import org.labkey.api.attachments.AttachmentParent; import org.labkey.api.attachments.AttachmentService; @@ -100,7 +98,6 @@ import org.labkey.api.dataiterator.DataIteratorContext; import org.labkey.api.exp.AbstractParameter; import org.labkey.api.exp.DeleteForm; -import org.labkey.api.exp.DuplicateMaterialException; import org.labkey.api.exp.ExperimentDataHandler; import org.labkey.api.exp.ExperimentException; import org.labkey.api.exp.ExperimentRunForm; @@ -120,7 +117,6 @@ import org.labkey.api.exp.api.ExpExperiment; import org.labkey.api.exp.api.ExpLineageOptions; import org.labkey.api.exp.api.ExpMaterial; -import org.labkey.api.exp.api.ExpMaterialRunInput; import org.labkey.api.exp.api.ExpObject; import org.labkey.api.exp.api.ExpProtocol; import org.labkey.api.exp.api.ExpProtocolApplication; @@ -215,7 +211,6 @@ import org.labkey.api.security.roles.Role; import org.labkey.api.settings.AppProps; import org.labkey.api.settings.ConceptURIProperties; -import org.labkey.api.settings.OptionalFeatureService; import org.labkey.api.sql.LabKeySql; import org.labkey.api.study.Dataset; import org.labkey.api.study.StudyService; @@ -262,7 +257,6 @@ import org.labkey.api.view.VBox; import org.labkey.api.view.ViewBackgroundInfo; import org.labkey.api.view.ViewContext; -import org.labkey.api.view.ViewServlet; import org.labkey.api.view.WebPartView; import org.labkey.api.view.template.ClientDependency; import org.labkey.api.view.template.PageConfig; @@ -271,7 +265,6 @@ import org.labkey.experiment.ConfirmDeleteView; import org.labkey.experiment.CustomPropertiesView; import org.labkey.experiment.DataClassWebPart; -import org.labkey.experiment.DerivedSamplePropertyHelper; import org.labkey.experiment.DotGraph; import org.labkey.experiment.ExpDataFileListener; import org.labkey.experiment.ExperimentRunDisplayColumn; @@ -319,7 +312,6 @@ import org.springframework.mock.web.MockHttpServletResponse; import org.springframework.validation.BindException; import org.springframework.validation.Errors; -import org.springframework.validation.ObjectError; import org.springframework.web.multipart.MultipartFile; import org.springframework.web.multipart.MultipartHttpServletRequest; import org.springframework.web.servlet.ModelAndView; @@ -341,7 +333,6 @@ import java.util.Arrays; import java.util.Collection; import java.util.Collections; -import java.util.Comparator; import java.util.HashMap; import java.util.HashSet; import java.util.LinkedHashMap; @@ -1119,16 +1110,7 @@ public ModelAndView getView(Object o, BindException errors) { ExpSchema schema = new ExpSchema(getUser(), getContainer()); QuerySettings settings = schema.getSettings(getViewContext(), "Materials", ExpSchema.TableType.Materials.toString()); - QueryView view = new QueryView(schema, settings, errors) - { - @Override - protected void populateButtonBar(DataView view, ButtonBar bar) - { - super.populateButtonBar(view, bar); - if (OptionalFeatureService.get().isFeatureEnabled(AppProps.DEPRECATED_DERIVE_SAMPLES_NOT_IN_APP)) - bar.add(SampleTypeContentsView.getDeriveSamplesButton(getContainer(),null)); - } - }; + QueryView view = new QueryView(schema, settings, errors); view.setShowDetailsColumn(false); return view; } @@ -1266,16 +1248,6 @@ public VBox getView(ExpObjectForm form, BindException errors) throws Exception } } - if (getContainer().hasPermission(getUser(), InsertPermission.class) && OptionalFeatureService.get().isFeatureEnabled(AppProps.DEPRECATED_DERIVE_SAMPLES_NOT_IN_APP)) - { - ActionURL deriveURL = new ActionURL(DeriveSamplesChooseTargetAction.class, getContainer()); - deriveURL.addParameter("rowIds", _material.getRowId()); - if (st != null) - deriveURL.addParameter("targetSampleTypeId", st.getRowId()); - - updateLinks.append(LinkBuilder.labkeyLink("derive samples from this sample", deriveURL)).append(" "); - } - vbox.addView(new HtmlView(updateLinks)); ExperimentRunListView runListView = ExperimentRunListView.createView(getViewContext(), ExperimentRunType.ALL_RUNS_TYPE, true); @@ -5479,467 +5451,6 @@ public URLHelper getSuccessURL(SetFlagForm form) } } - @RequiresPermission(InsertPermission.class) - public class DeriveSamplesChooseTargetAction extends SimpleViewAction - { - private List _materials; - - @Override - public void addNavTrail(NavTree root) - { - setHelpTopic("sampleSets"); - addRootNavTrail(root); - root.addChild("Sample Types", ExperimentUrlsImpl.get().getShowSampleTypeListURL(getContainer())); - ExpSampleType sampleType = _materials != null && !_materials.isEmpty() ? _materials.getFirst().getSampleType() : null; - if (sampleType != null) - { - root.addChild(sampleType.getName(), ExperimentUrlsImpl.get().getShowSampleTypeURL(sampleType)); - } - root.addChild("Derive Samples"); - } - - @Override - public void validate(DeriveMaterialForm form, BindException errors) - { - _materials = form.lookupMaterials(); - if (_materials.isEmpty()) - { - throw new NotFoundException("Could not find any matching materials"); - } - } - - @Override - public ModelAndView getView(DeriveMaterialForm form, BindException errors) - { - Container c = getContainer(); - PipeRoot root = PipelineService.get().findPipelineRoot(c); - - if (root == null || !root.isValid()) - { - ActionURL pipelineURL = urlProvider(PipelineUrls.class).urlSetup(c); - return new HtmlView(DIV("You must ", - DOM.A(DOM.at(href, pipelineURL), "configure a valid pipeline root for this folder"), - " before deriving samples.")); - } - else - { - Set materialInputRoles = new TreeSet<>(ExperimentService.get().getMaterialInputRoles(getContainer(), getUser())); - Map materialsWithRoles = new LinkedHashMap<>(); - for (ExpMaterial material : _materials) - { - materialsWithRoles.put(material, null); - } - - List sampleTypes = getUploadableSampleTypes(); - - DeriveSamplesChooseTargetBean bean = new DeriveSamplesChooseTargetBean(form.getDataRegionSelectionKey(), form.getTargetSampleTypeId(), sampleTypes, materialsWithRoles, form.getOutputCount(), materialInputRoles, null); - return new JspView<>("/org/labkey/experiment/deriveSamplesChooseTarget.jsp", bean); - } - } - } - - public static class DeriveSamplesChooseTargetBean implements DataRegionSelection.DataSelectionKeyForm - { - private String _dataRegionSelectionKey; - - private final Integer _targetSampleTypeId; - private final List _sampleTypes; - private final Map _sourceMaterials; - private final int _sampleCount; - private final Collection _inputRoles; - private final DerivedSamplePropertyHelper _propertyHelper; - - public static final String CUSTOM_ROLE = "--CUSTOM--"; - - public DeriveSamplesChooseTargetBean(String dataRegionSelectionKey, Integer targetSampleTypeId, List sampleTypes, Map sourceMaterials, int sampleCount, Collection inputRoles, DerivedSamplePropertyHelper helper) - { - _dataRegionSelectionKey = dataRegionSelectionKey; - _targetSampleTypeId = targetSampleTypeId; - _sampleTypes = sampleTypes; - _sourceMaterials = sourceMaterials; - _sampleCount = sampleCount; - _inputRoles = inputRoles; - _propertyHelper = helper; - } - - public Integer getTargetSampleTypeId() - { - return _targetSampleTypeId; - } - - public DerivedSamplePropertyHelper getPropertyHelper() - { - return _propertyHelper; - } - - public int getSampleCount() - { - return _sampleCount; - } - - public Map getSourceMaterials() - { - return _sourceMaterials; - } - - public List getSampleTypes() - { - return _sampleTypes; - } - - public Collection getInputRoles() - { - return _inputRoles; - } - - @Override - public String getDataRegionSelectionKey() - { - return _dataRegionSelectionKey; - } - - @Override - public void setDataRegionSelectionKey(String key) - { - _dataRegionSelectionKey = key; - } - } - - private List getUploadableSampleTypes() - { - // Make a copy so we can modify it - List sampleTypes = new ArrayList<>(SampleTypeService.get().getSampleTypes(getContainer(), true)); - sampleTypes.removeIf(sampleType -> !sampleType.canImportMoreSamples()); - return sampleTypes; - } - - @RequiresPermission(InsertPermission.class) - public class DeriveSamplesAction extends FormViewAction - { - private List _materials; - private ActionURL _successUrl; - private final Map _inputMaterials = new LinkedHashMap<>(); - - @Override - public ModelAndView getView(DeriveMaterialForm form, boolean reshow, BindException errors) - { - _materials = form.lookupMaterials(); - if (_materials.isEmpty()) - { - throw new NotFoundException("Could not find any matching materials"); - } - - Container c = getContainer(); - - if (form.getOutputCount() <= 0) - { - form.setOutputCount(1); - } - - if (form.getTargetSampleTypeId() == 0) - throw new NotFoundException("Target sample type required for the derived samples"); - - ExpSampleTypeImpl sampleType = SampleTypeServiceImpl.get().getSampleType(getContainer(), form.getTargetSampleTypeId(), true); - if (sampleType == null) - throw new NotFoundException("Could not find sample type with rowId " + form.getTargetSampleTypeId()); - - InsertView insertView = new InsertView(new DataRegion(), errors); - - DerivedSamplePropertyHelper helper = new DerivedSamplePropertyHelper(sampleType, form.getOutputCount(), c, getUser()); - helper.addSampleColumns(insertView, getUser()); - - int[] rowIds = form.getRowIds(); - for (int i = 0; i < rowIds.length; i++) - { - insertView.getDataRegion().addHiddenFormField("rowIds", Integer.toString(rowIds[i])); - insertView.getDataRegion().addHiddenFormField("inputRole" + i, form.getInputRole(i) == null ? "" : form.getInputRole(i)); - insertView.getDataRegion().addHiddenFormField("customRole" + i, form.getCustomRole(i) == null ? "" : form.getCustomRole(i)); - } - - insertView.getDataRegion().addHiddenFormField("targetSampleTypeId", Integer.toString(form.getTargetSampleTypeId())); - insertView.getDataRegion().addHiddenFormField("outputCount", Integer.toString(form.getOutputCount())); - if (form.getDataRegionSelectionKey() != null) - insertView.getDataRegion().addHiddenFormField(DataRegionSelection.DATA_REGION_SELECTION_KEY, form.getDataRegionSelectionKey()); - insertView.setInitialValues(ViewServlet.adaptParameterMap(getViewContext().getRequest().getParameterMap())); - ButtonBar bar = new ButtonBar(); - bar.setStyle(ButtonBar.Style.separateButtons); - ActionButton submitButton = new ActionButton(DeriveSamplesAction.class, "Submit"); - submitButton.setActionType(ActionButton.Action.POST); - bar.add(submitButton); - insertView.getDataRegion().setButtonBar(bar); - insertView.setTitle("Output Samples"); - - Map materialsWithRoles = new LinkedHashMap<>(); - List materials = form.lookupMaterials(); - for (int i = 0; i < materials.size(); i++) - { - materialsWithRoles.put(materials.get(i), form.determineLabel(i)); - } - - DeriveSamplesChooseTargetBean bean = new DeriveSamplesChooseTargetBean(form.getDataRegionSelectionKey(), form.getTargetSampleTypeId(), getUploadableSampleTypes(), materialsWithRoles, form.getOutputCount(), Collections.emptyList(), helper); - JspView view = new JspView<>("/org/labkey/experiment/summarizeMaterialInputs.jsp", bean); - view.setTitle("Input Samples"); - - return new VBox(view, insertView); - } - - @Override - public void addNavTrail(NavTree root) - { - setHelpTopic("sampleSets"); - addRootNavTrail(root); - root.addChild("Sample Types", ExperimentUrlsImpl.get().getShowSampleTypeListURL(getContainer())); - ExpSampleType sampleType = _materials != null && !_materials.isEmpty() ? _materials.getFirst().getSampleType() : null; - if (sampleType != null) - { - root.addChild(sampleType.getName(), ExperimentUrlsImpl.get().getShowSampleTypeURL(sampleType)); - } - root.addChild("Derive Samples"); - } - - @Override - public void validateCommand(DeriveMaterialForm form, Errors errors) - { - List materials = form.lookupMaterials(); - - List lockedSamples = new ArrayList<>(); - for (int i = 0; i < materials.size(); i++) - { - ExpMaterial m = materials.get(i); - if (!m.isOperationPermitted(SampleTypeService.SampleOperations.EditLineage)) - { - lockedSamples.add(m); - } - String inputRole = form.determineLabel(i); - if (inputRole == null || inputRole.isEmpty()) - { - ExpSampleType st = m.getSampleType(); - inputRole = st != null ? st.getName() : ExpMaterialRunInput.DEFAULT_ROLE; - } - _inputMaterials.put(materials.get(i), inputRole); - } - - if (!lockedSamples.isEmpty()) - { - errors.reject(ERROR_MSG, SampleTypeService.get().getOperationNotPermittedMessage(lockedSamples, SampleTypeService.SampleOperations.EditLineage)); - } - } - - @Override - public boolean handlePost(DeriveMaterialForm form, BindException errors) - { - ExpSampleTypeImpl sampleType = SampleTypeServiceImpl.get().getSampleType(getContainer(), form.getTargetSampleTypeId(), true); - - DerivedSamplePropertyHelper helper = new DerivedSamplePropertyHelper(sampleType, form.getOutputCount(), getContainer(), getUser()); - - Map, Map> allProperties; - try - { - boolean valid = true; - for (Map.Entry> entry : helper.getPostedPropertyValues(getViewContext().getRequest()).entrySet()) - valid = UploadWizardAction.validatePostedProperties(getViewContext(), entry.getValue(), errors) && valid; - if (!valid) - return false; - - allProperties = helper.getSampleProperties(getViewContext().getRequest(), _inputMaterials.keySet()); - } - catch (DuplicateMaterialException e) - { - errors.addError(new ObjectError(e.getColName(), null, null, e.getMessage())); - return false; - } - catch (ExperimentException e) - { - errors.reject(SpringActionController.ERROR_MSG, e.getMessage()); - return false; - } - - try (DbScope.Transaction tx = ExperimentService.get().ensureTransaction()) - { - Map outputMaterials = new HashMap<>(); - int i = 0; - for (Map.Entry, Map> entry : allProperties.entrySet()) - { - Lsid lsid = entry.getKey().first; - String name = entry.getKey().second; - assert name != null; - - ExpMaterialImpl outputMaterial = ExperimentServiceImpl.get().createExpMaterial(getContainer(), lsid.toString(), name); - if (sampleType != null) - { - outputMaterial.setCpasType(sampleType.getLSID()); - } - outputMaterial.save(getUser()); - - if (sampleType != null) - { - Map pvs = new HashMap<>(); - for (Map.Entry propertyEntry : entry.getValue().entrySet()) - pvs.put(propertyEntry.getKey().getName(), propertyEntry.getValue()); - outputMaterial.setProperties(getUser(), pvs, false); - } - - outputMaterials.put(outputMaterial, helper.getSampleNames().get(i++)); - } - - ExperimentService.get().deriveSamples(_inputMaterials, outputMaterials, getViewBackgroundInfo(), _log); - - tx.commit(); - - // automatically link samples to study, if configured - StudyPublishService.get().autoLinkDerivedSamples(sampleType, outputMaterials.keySet().stream().map(ExpObject::getRowId).collect(toList()), getContainer(), getUser()); - - _successUrl = ExperimentUrlsImpl.get().getShowSampleURL(getContainer(), outputMaterials.keySet().iterator().next()); - - if (form.getDataRegionSelectionKey() != null) - DataRegionSelection.clearAll(getViewContext(), form.getDataRegionSelectionKey()); - } - catch (Exception e) - { - errors.reject(SpringActionController.ERROR_MSG, e.getMessage()); - return false; - } - - return true; - } - - @Override - public URLHelper getSuccessURL(DeriveMaterialForm deriveMaterialForm) - { - return _successUrl; - } - } - - public static class DeriveMaterialForm implements HasViewContext, DataRegionSelection.DataSelectionKeyForm - { - private String _dataRegionSelectionKey; - private int _outputCount = 1; - private int _targetSampleTypeId; - private int[] _rowIds; - private String _name; - - private ViewContext _context; - - @Override - public void setViewContext(ViewContext context) - { - _context = context; - } - - @Override - public ViewContext getViewContext() - { - return _context; - } - - public List lookupMaterials() - { - List result = new ArrayList<>(); - for (int rowId : getRowIds()) - { - ExpMaterial material = ExperimentService.get().getExpMaterial(rowId); - if (material != null) - { - if (material.getContainer().hasPermission(_context.getUser(), ReadPermission.class)) - { - result.add(material); - } - else - { - throw new UnauthorizedException(); - } - } - else - { - throw new NotFoundException("No material with RowId " + rowId); - } - } - result.sort(Comparator.comparing(Identifiable::getName)); - return result; - } - - public String getName() - { - return _name; - } - - public void setName(String name) - { - _name = name; - } - - @Override - public String getDataRegionSelectionKey() - { - return _dataRegionSelectionKey; - } - - @Override - public void setDataRegionSelectionKey(String dataRegionSelectionKey) - { - _dataRegionSelectionKey = dataRegionSelectionKey; - } - - public int[] getRowIds() - { - if (_rowIds == null) - { - _rowIds = PageFlowUtil.toInts(DataRegionSelection.getSelected(getViewContext(), getDataRegionSelectionKey(), false)); - } - return _rowIds; - } - - public void setRowIds(int[] rowIds) - { - _rowIds = rowIds; - } - - public int getOutputCount() - { - return _outputCount; - } - - public void setOutputCount(int outputCount) - { - _outputCount = outputCount; - } - - public int getTargetSampleTypeId() - { - return _targetSampleTypeId; - } - - public void setTargetSampleTypeId(int targetSampleTypeId) - { - _targetSampleTypeId = targetSampleTypeId; - } - - public String getInputRole(int i) - { - return _context.getRequest().getParameter("inputRole" + i); - } - - public String getCustomRole(int i) - { - return _context.getRequest().getParameter("customRole" + i); - } - - public String determineLabel(int index) - { - String result = getInputRole(index); - if (DeriveSamplesChooseTargetBean.CUSTOM_ROLE.equals(result)) - { - result = getCustomRole(index); - } - if (result != null) - { - result = result.trim(); - } - return result; - } - } - - public static class ExpInput { public String role; diff --git a/experiment/src/org/labkey/experiment/controllers/exp/SampleTypeContentsView.java b/experiment/src/org/labkey/experiment/controllers/exp/SampleTypeContentsView.java index c7c3ea5984d..fd4074ef4ec 100644 --- a/experiment/src/org/labkey/experiment/controllers/exp/SampleTypeContentsView.java +++ b/experiment/src/org/labkey/experiment/controllers/exp/SampleTypeContentsView.java @@ -68,18 +68,6 @@ public SampleTypeContentsView(ExpSampleTypeImpl source, SamplesSchema schema, Qu ); } - public static ActionButton getDeriveSamplesButton(@NotNull Container container, @Nullable Long targetSampleTypeId) - { - ActionURL urlDeriveSamples = new ActionURL(ExperimentController.DeriveSamplesChooseTargetAction.class, container); - if (targetSampleTypeId != null) - urlDeriveSamples.addParameter("targetSampleTypeId", targetSampleTypeId); - ActionButton deriveButton = new ActionButton(urlDeriveSamples, "Derive Samples"); - deriveButton.setActionType(ActionButton.Action.POST); - deriveButton.setDisplayPermission(InsertPermission.class); - deriveButton.setRequiresSelection(true); - return deriveButton; - } - @Override public DataView createDataView() { @@ -217,9 +205,6 @@ protected void populateButtonBar(DataView view, ButtonBar bar) { super.populateButtonBar(view, bar); - if (OptionalFeatureService.get().isFeatureEnabled(AppProps.DEPRECATED_DERIVE_SAMPLES_NOT_IN_APP)) - bar.add(getDeriveSamplesButton(getContainer(), _source.getRowId())); - ActionButton linkToStudyButton = getLinkToStudyButton(view); if (linkToStudyButton != null) bar.add(linkToStudyButton); diff --git a/experiment/src/org/labkey/experiment/deriveSamplesChooseTarget.jsp b/experiment/src/org/labkey/experiment/deriveSamplesChooseTarget.jsp deleted file mode 100644 index b606f526966..00000000000 --- a/experiment/src/org/labkey/experiment/deriveSamplesChooseTarget.jsp +++ /dev/null @@ -1,99 +0,0 @@ -<% -/* - * Copyright (c) 2008-2026 LabKey Corporation - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ -%> -<%@ page import="org.labkey.api.data.DataRegionSelection" %> -<%@ page import="org.labkey.api.exp.api.ExpMaterial" %> -<%@ page import="org.labkey.api.exp.api.ExpSampleType" %> -<%@ page import="org.labkey.api.view.HttpView" %> -<%@ page import="org.labkey.api.view.JspView" %> -<%@ page import="org.labkey.experiment.controllers.exp.ExperimentController.DeriveSamplesAction" %> -<%@ page import="org.labkey.experiment.controllers.exp.ExperimentController.DeriveSamplesChooseTargetBean" %> -<%@ page import="java.util.LinkedHashMap" %> -<%@ page import="java.util.Map" %> -<%@ page extends="org.labkey.api.jsp.JspBase" %> -<%@ taglib prefix="labkey" uri="http://www.labkey.org/taglib"%> -<% - JspView me = HttpView.currentView(); - DeriveSamplesChooseTargetBean bean = me.getModelBean(); - - Map sampleTypeOptions = new LinkedHashMap<>(); - for (ExpSampleType st : bean.getSampleTypes()) - { - sampleTypeOptions.put(st.getRowId(), st.getName() + " in " + st.getContainer().getPath()); - } -%> - - - <% if (bean.getDataRegionSelectionKey() != null) { %> - - <% } %> - - - - - - - - - - - - - - - - - - -
Source materials: - - - - - - <% - int roleIndex = 0; - for (ExpMaterial material : bean.getSourceMaterials().keySet()) - { %> - "> - - <% addHandler("inputRole" + roleIndex, "change", "document.getElementById('customRole" + roleIndex + "').disabled = this.value !== " + q(DeriveSamplesChooseTargetBean.CUSTOM_ROLE) + ";"); %> - - - <% - roleIndex++; - } - %> -
NameRole<%= helpPopup("Role", "Roles allow you to label an input as being used in a particular way. It serves to disambiguate the purpose of each of the input materials. Each input should have a unique role.")%>
<%= h(material.getName())%>
-
Number of derived samples: - -
Target sample type: - <%=select().name("targetSampleTypeId").addOptions(sampleTypeOptions).selected(bean.getTargetSampleTypeId())%> -
- diff --git a/experiment/src/org/labkey/experiment/summarizeMaterialInputs.jsp b/experiment/src/org/labkey/experiment/summarizeMaterialInputs.jsp deleted file mode 100644 index 0d553e63360..00000000000 --- a/experiment/src/org/labkey/experiment/summarizeMaterialInputs.jsp +++ /dev/null @@ -1,102 +0,0 @@ -<% -/* - * Copyright (c) 2008-2026 LabKey Corporation - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ -%> -<%@ page import="org.labkey.api.data.DisplayColumnGroup" %> -<%@ page import="org.labkey.api.exp.PropertyDescriptor" %> -<%@ page import="org.labkey.api.exp.api.ExpMaterial" %> -<%@ page import="org.labkey.api.exp.property.DomainProperty" %> -<%@ page import="org.labkey.api.view.HttpView" %> -<%@ page import="org.labkey.api.view.JspView" %> -<%@ page import="org.labkey.experiment.DerivedSamplePropertyHelper" %> -<%@ page import="org.labkey.experiment.controllers.exp.ExperimentController" %> -<%@ page import="java.util.ArrayList" %> -<%@ page import="java.util.List" %> -<%@ page import="java.util.Map" %> -<%@ page extends="org.labkey.api.jsp.JspBase" %> -<%@ taglib prefix="labkey" uri="http://www.labkey.org/taglib"%> -<% - JspView me = HttpView.currentView(); - ExperimentController.DeriveSamplesChooseTargetBean bean = me.getModelBean(); - List sameTypeInputs = new ArrayList<>(); - DerivedSamplePropertyHelper helper = bean.getPropertyHelper(); - if (helper.getSampleType() != null) - { - for (ExpMaterial material : bean.getSourceMaterials().keySet()) - { - if (helper.getSampleType().equals(material.getSampleType())) - { - sameTypeInputs.add(material); - } - } - } -%> - - - - - - <% if (!sameTypeInputs.isEmpty()) { %> - - <% } %> - -<% - int rowCount = 0; - for (Map.Entry entry : bean.getSourceMaterials().entrySet()) - { -%> - "> - - - <% if (sameTypeInputs.contains(entry.getKey())) { %> - - <% } %> - -<% - rowCount++; - } -%> -
Sample NameRole<%= helpPopup("Role", "Roles allow you to label an input as being used in a particular way. It serves to disambiguate the purpose of each of the input materials. Each input should have a unique role.")%>Copy properties to...
<%= h(entry.getKey().getName())%><%= h(entry.getValue()) %> - <% - String separator = ""; - Map groups = helper.getGroups(); - for (int i = 0; i < helper.getSampleNames().size(); i++) - { - StringBuilder handler = new StringBuilder(); - for (Map.Entry propEntry : entry.getKey().getPropertyValues().entrySet()) - { - DisplayColumnGroup group = groups.get(propEntry.getKey()); - if (group != null && group.isCopyable()) - { - String propName = group.getColumns().get(i).getColumnInfo().getName(); - String propValue = String.valueOf(propEntry.getValue()); - handler.append("summarize_setProperty(" + q(propName) + "," + q(propValue) + ");\n"); - } - } - handler.append("return false;"); - %><%=h(separator)%><%=link(helper.getSampleNames().get(i)).onClick(handler.toString())%><% - separator = ","; - } %> -
- \ No newline at end of file From 1b54a68377d35dbbf7d52e7458b7f02b30556a3f Mon Sep 17 00:00:00 2001 From: XingY Date: Tue, 6 Oct 2026 16:04:55 -0700 Subject: [PATCH 13/13] selenium test --- core/package-lock.json | 16 ++++++++-------- core/package.json | 2 +- experiment/package-lock.json | 16 ++++++++-------- experiment/package.json | 2 +- pipeline/package-lock.json | 16 ++++++++-------- pipeline/package.json | 2 +- 6 files changed, 27 insertions(+), 27 deletions(-) diff --git a/core/package-lock.json b/core/package-lock.json index 957fdbaec5b..ee5c340f338 100644 --- a/core/package-lock.json +++ b/core/package-lock.json @@ -8,7 +8,7 @@ "name": "labkey-core", "version": "0.0.0", "dependencies": { - "@labkey/components": "7.68.0", + "@labkey/components": "7.68.1-fb-issue1512.2", "@labkey/themes": "1.9.7" }, "devDependencies": { @@ -2485,9 +2485,9 @@ "license": "MIT" }, "node_modules/@labkey/api": { - "version": "1.53.0", - "resolved": "https://labkey.jfrog.io/artifactory/api/npm/libs-client/@labkey/api/-/@labkey/api-1.53.0.tgz", - "integrity": "sha512-3Cw8mVRAhQZkcbgcXPJVGmZuDecaiDEYlP2QxGm3NttFEbNTbRAhIw1BQufxnV0YzGd/3R9oZtpmc7eHzZSi4A==", + "version": "1.53.1-fb-issue1512.0", + "resolved": "https://labkey.jfrog.io/artifactory/api/npm/libs-client/@labkey/api/-/@labkey/api-1.53.1-fb-issue1512.0.tgz", + "integrity": "sha512-+TXs6JQoY7pmrQExcSvq2IeHOH1KDYvK2EBkDkOzdeiScieBOzvC17fQPRhpjCcv3c0eUbVBmRfxwDlMXnv3yw==", "license": "Apache-2.0" }, "node_modules/@labkey/build": { @@ -2519,13 +2519,13 @@ } }, "node_modules/@labkey/components": { - "version": "7.68.0", - "resolved": "https://labkey.jfrog.io/artifactory/api/npm/libs-client/@labkey/components/-/@labkey/components-7.68.0.tgz", - "integrity": "sha512-QdTwixs+wR7TXBon1UOOOEHCuLlWEakxC1wWyZOY27jmo7yOTp+8DmjaiKnTRa9jFVg2yb0QcRv4p9dbLfGbaA==", + "version": "7.68.1-fb-issue1512.2", + "resolved": "https://labkey.jfrog.io/artifactory/api/npm/libs-client/@labkey/components/-/@labkey/components-7.68.1-fb-issue1512.2.tgz", + "integrity": "sha512-uCeNXfwYZg5CblG4jTiGshEbsKbJPMOGABH9JqXFbNZnQgJ1ne52lby/JtfdpuekKGw4te1brRnL9tQkEE2U0A==", "license": "SEE LICENSE IN LICENSE.txt", "dependencies": { "@hello-pangea/dnd": "18.0.1", - "@labkey/api": "1.53.0", + "@labkey/api": "1.53.1-fb-issue1512.0", "@testing-library/dom": "~10.4.2", "@testing-library/jest-dom": "~7.0.1", "@testing-library/react": "~16.3.3", diff --git a/core/package.json b/core/package.json index b8c1506d039..c3678d4516c 100644 --- a/core/package.json +++ b/core/package.json @@ -20,7 +20,7 @@ "lint-branch-fix": "node lint.diff.mjs --currentBranch --fix" }, "dependencies": { - "@labkey/components": "7.68.0", + "@labkey/components": "7.68.1-fb-issue1512.2", "@labkey/themes": "1.9.7" }, "devDependencies": { diff --git a/experiment/package-lock.json b/experiment/package-lock.json index 7671824eaad..3b05d672320 100644 --- a/experiment/package-lock.json +++ b/experiment/package-lock.json @@ -8,7 +8,7 @@ "name": "experiment", "version": "0.0.0", "dependencies": { - "@labkey/components": "7.68.0" + "@labkey/components": "7.68.1-fb-issue1512.2" }, "devDependencies": { "@labkey/build": "10.1.3", @@ -2494,9 +2494,9 @@ } }, "node_modules/@labkey/api": { - "version": "1.53.0", - "resolved": "https://labkey.jfrog.io/artifactory/api/npm/libs-client/@labkey/api/-/@labkey/api-1.53.0.tgz", - "integrity": "sha512-3Cw8mVRAhQZkcbgcXPJVGmZuDecaiDEYlP2QxGm3NttFEbNTbRAhIw1BQufxnV0YzGd/3R9oZtpmc7eHzZSi4A==", + "version": "1.53.1-fb-issue1512.0", + "resolved": "https://labkey.jfrog.io/artifactory/api/npm/libs-client/@labkey/api/-/@labkey/api-1.53.1-fb-issue1512.0.tgz", + "integrity": "sha512-+TXs6JQoY7pmrQExcSvq2IeHOH1KDYvK2EBkDkOzdeiScieBOzvC17fQPRhpjCcv3c0eUbVBmRfxwDlMXnv3yw==", "license": "Apache-2.0" }, "node_modules/@labkey/build": { @@ -2528,13 +2528,13 @@ } }, "node_modules/@labkey/components": { - "version": "7.68.0", - "resolved": "https://labkey.jfrog.io/artifactory/api/npm/libs-client/@labkey/components/-/@labkey/components-7.68.0.tgz", - "integrity": "sha512-QdTwixs+wR7TXBon1UOOOEHCuLlWEakxC1wWyZOY27jmo7yOTp+8DmjaiKnTRa9jFVg2yb0QcRv4p9dbLfGbaA==", + "version": "7.68.1-fb-issue1512.2", + "resolved": "https://labkey.jfrog.io/artifactory/api/npm/libs-client/@labkey/components/-/@labkey/components-7.68.1-fb-issue1512.2.tgz", + "integrity": "sha512-uCeNXfwYZg5CblG4jTiGshEbsKbJPMOGABH9JqXFbNZnQgJ1ne52lby/JtfdpuekKGw4te1brRnL9tQkEE2U0A==", "license": "SEE LICENSE IN LICENSE.txt", "dependencies": { "@hello-pangea/dnd": "18.0.1", - "@labkey/api": "1.53.0", + "@labkey/api": "1.53.1-fb-issue1512.0", "@testing-library/dom": "~10.4.2", "@testing-library/jest-dom": "~7.0.1", "@testing-library/react": "~16.3.3", diff --git a/experiment/package.json b/experiment/package.json index e90e30b27eb..56b2d1368a9 100644 --- a/experiment/package.json +++ b/experiment/package.json @@ -13,7 +13,7 @@ "test-integration": "cross-env NODE_ENV=test jest --ci --runInBand -c test/js/jest.config.integration.js" }, "dependencies": { - "@labkey/components": "7.68.0" + "@labkey/components": "7.68.1-fb-issue1512.2" }, "devDependencies": { "@labkey/build": "10.1.3", diff --git a/pipeline/package-lock.json b/pipeline/package-lock.json index 130de0e9b13..ecf2480f096 100644 --- a/pipeline/package-lock.json +++ b/pipeline/package-lock.json @@ -8,7 +8,7 @@ "name": "pipeline", "version": "0.0.0", "dependencies": { - "@labkey/components": "7.68.0" + "@labkey/components": "7.68.1-fb-issue1512.2" }, "devDependencies": { "@labkey/build": "10.1.3", @@ -1483,9 +1483,9 @@ "license": "MIT" }, "node_modules/@labkey/api": { - "version": "1.53.0", - "resolved": "https://labkey.jfrog.io/artifactory/api/npm/libs-client/@labkey/api/-/@labkey/api-1.53.0.tgz", - "integrity": "sha512-3Cw8mVRAhQZkcbgcXPJVGmZuDecaiDEYlP2QxGm3NttFEbNTbRAhIw1BQufxnV0YzGd/3R9oZtpmc7eHzZSi4A==", + "version": "1.53.1-fb-issue1512.0", + "resolved": "https://labkey.jfrog.io/artifactory/api/npm/libs-client/@labkey/api/-/@labkey/api-1.53.1-fb-issue1512.0.tgz", + "integrity": "sha512-+TXs6JQoY7pmrQExcSvq2IeHOH1KDYvK2EBkDkOzdeiScieBOzvC17fQPRhpjCcv3c0eUbVBmRfxwDlMXnv3yw==", "license": "Apache-2.0" }, "node_modules/@labkey/build": { @@ -1517,13 +1517,13 @@ } }, "node_modules/@labkey/components": { - "version": "7.68.0", - "resolved": "https://labkey.jfrog.io/artifactory/api/npm/libs-client/@labkey/components/-/@labkey/components-7.68.0.tgz", - "integrity": "sha512-QdTwixs+wR7TXBon1UOOOEHCuLlWEakxC1wWyZOY27jmo7yOTp+8DmjaiKnTRa9jFVg2yb0QcRv4p9dbLfGbaA==", + "version": "7.68.1-fb-issue1512.2", + "resolved": "https://labkey.jfrog.io/artifactory/api/npm/libs-client/@labkey/components/-/@labkey/components-7.68.1-fb-issue1512.2.tgz", + "integrity": "sha512-uCeNXfwYZg5CblG4jTiGshEbsKbJPMOGABH9JqXFbNZnQgJ1ne52lby/JtfdpuekKGw4te1brRnL9tQkEE2U0A==", "license": "SEE LICENSE IN LICENSE.txt", "dependencies": { "@hello-pangea/dnd": "18.0.1", - "@labkey/api": "1.53.0", + "@labkey/api": "1.53.1-fb-issue1512.0", "@testing-library/dom": "~10.4.2", "@testing-library/jest-dom": "~7.0.1", "@testing-library/react": "~16.3.3", diff --git a/pipeline/package.json b/pipeline/package.json index 5fbcbd1168a..27348d73b17 100644 --- a/pipeline/package.json +++ b/pipeline/package.json @@ -14,7 +14,7 @@ "build-prod": "npm run clean && cross-env NODE_ENV=production rspack build --config node_modules/@labkey/build/configs/prod.config.js" }, "dependencies": { - "@labkey/components": "7.68.0" + "@labkey/components": "7.68.1-fb-issue1512.2" }, "devDependencies": { "@labkey/build": "10.1.3",