GH Issue 1512: user defined queries causes performance issues with lookup field query listing

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;

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<Pair<String, Boolean>, TableInfo> _cache = new HashMap<>();
     private final Map<Pair<String, Boolean>, TableInfo> _cache = new HashMap<>();
 
+    // 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<String, Boolean> HAS_PK_COLUMN_CACHE = CacheManager.getCache(CacheManager.UNLIMITED, CacheManager.DAY, "Query has-PK-column flags");
+
     private Map<String, TableType> _metadataTableMap = null;
 
     public QueryDefinitionImpl(User user, Container container, QueryDef queryDef)
@@ -814,6 +822,46 @@ 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.
+    @Nullable
+    private String getHasPkColumnCacheKey()
+    {
+        String version = getHasPkCacheVersion();
+        if (null == version || null == getContainer() || null == getName())
+            return null;
+        return getContainer().getId() + "/" + getSchemaPath() + "/" + getName() + "/" + version;
+    }
+
+    public static void clearHasPkColumnCache()
+    {
+        HAS_PK_COLUMN_CACHE.clear();
+    }
+
+    /** @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)
     {

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<Map<String, Object>> 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<String, Object> 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<String, Object> getQueryProps(QueryDefinition qdef, ActionURL viewDataUrl, boolean isUserDefined, UserSchema schema, boolean includeColumns, boolean useQueryDetailColumns, boolean includeTitle)
+        private Map<String, Object> 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<String, Object> qinfo = new HashMap<>();
             qinfo.put("hidden", qdef.isHidden());
             qinfo.put("snapshot", qdef.isSnapshot());
@@ -7008,15 +7033,22 @@ protected Map<String, Object> getQueryProps(QueryDefinition qdef, ActionURL view
 
             String title = qdef.getName();
             String name = qdef.getName();
+            boolean hasPk = false;
             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)
                     {
+                        hasPk = table.getPkColumns().stream().anyMatch(col -> !col.isAdditionalQueryColumn());
+                        if (isUserDefined && impl != null)
+                            impl.cacheHasPkColumn(hasPk);
+                        if (requirePk && !hasPk)
+                            return null;
+
                         if (includeColumns)
                         {
                             Collection<Map<String, Object>> columns;
@@ -7062,6 +7094,10 @@ protected Map<String, Object> getQueryProps(QueryDefinition qdef, ActionURL view
                 //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;

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();
     }
 }

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)