From 4856c9cde80f4585a5f38ac125d137471051426d Mon Sep 17 00:00:00 2001 From: Caroline Denis Date: Tue, 20 Oct 2026 19:25:17 +0200 Subject: [PATCH 01/47] Fix(stats): serialize calculations and cache permission-checked results --- specifyweb/backend/stats/coordination.py | 100 ++++++++++++++++++ specifyweb/backend/stats/views.py | 5 + .../backend/stored_queries/execution.py | 10 +- specifyweb/backend/stored_queries/views.py | 22 +++- 4 files changed, 134 insertions(+), 3 deletions(-) create mode 100644 specifyweb/backend/stats/coordination.py diff --git a/specifyweb/backend/stats/coordination.py b/specifyweb/backend/stats/coordination.py new file mode 100644 index 00000000000..c66280ef9bb --- /dev/null +++ b/specifyweb/backend/stats/coordination.py @@ -0,0 +1,100 @@ +import hashlib +import json +import logging +from collections.abc import Callable +from functools import wraps + +from django.conf import settings +from django.http import HttpResponse, JsonResponse + +from specifyweb.backend.cache.redis.utils import format_key, redis_connection +from specifyweb.backend.permissions.permissions import check_table_permissions +from specifyweb.specify.models_utils.lock_tables import Lock + +logger = logging.getLogger(__name__) + +STATISTICS_CACHE_SECONDS = 300 +STATISTICS_RETRY_SECONDS = 2 + + +class StatisticsBusy(Exception): + pass + + +def busy_response() -> JsonResponse: + response = JsonResponse( + {"message": "Statistics are already being calculated. Please retry."}, + status=409, + ) + response["Retry-After"] = str(STATISTICS_RETRY_SECONDS) + return response + + +def statistics_result( + collection, + user, + query_key: str, + calculate: Callable[[], str], + validate_cached: Callable[[], None], +) -> str: + digest = hashlib.sha256(query_key.encode()).hexdigest() + cache_key = format_key( + f"specify:{{database}}:statistics:{collection.id}:{user.id}:{digest}" + ) + cache = redis_connection() + + def cached_result() -> str | None: + result = cache.get(cache_key) + if result is not None: + validate_cached() + return result + + result = cached_result() + if result is not None: + return result + + # A database advisory lock has no lease that could expire during a slow query. + lock_name = hashlib.sha256(settings.DATABASE_NAME.encode()).hexdigest()[:48] + lock = Lock(f"statistics:{lock_name}", timeout=0) + try: + lock.acquire() + except TimeoutError as error: + logger.debug("Deferring statistics calculation: %s", error) + raise StatisticsBusy from error + + try: + result = cached_result() + if result is None: + result = calculate() + cache.set(cache_key, result, ex=STATISTICS_CACHE_SECONDS) + return result + finally: + lock.release() + + +def statistics_response(*tables): + def decorate(view): + @wraps(view) + def wrapped(request, *args, **kwargs): + def validate() -> None: + for table in tables: + check_table_permissions( + request.specify_collection, request.specify_user, table, "read" + ) + + validate() + try: + content = statistics_result( + request.specify_collection, + request.specify_user, + json.dumps([request.path, sorted(request.GET.items())]), + lambda: view(request, *args, **kwargs).content.decode(), + validate, + ) + except StatisticsBusy: + return busy_response() + return HttpResponse(content, content_type="application/json") + + return wrapped + + return decorate diff --git a/specifyweb/backend/stats/views.py b/specifyweb/backend/stats/views.py index 6b447ce0848..bc4e5e69077 100644 --- a/specifyweb/backend/stats/views.py +++ b/specifyweb/backend/stats/views.py @@ -10,8 +10,10 @@ logger = logging.getLogger(__name__) from django.db import connection +from .coordination import statistics_response @login_maybe_required +@statistics_response(Preparation) @openapi(schema={ 'get': { 'responses': { @@ -53,6 +55,7 @@ def collection_preparations(request) -> HttpResponse: @login_maybe_required +@statistics_response(Discipline, Locality) @openapi(schema={ 'get': { 'responses': { @@ -82,6 +85,7 @@ def collection_locality_geography(request, stat) -> HttpResponse: return http.JsonResponse(geography_dict) @login_maybe_required +@statistics_response(Determination) @openapi(schema={ 'get': { 'responses': { @@ -145,6 +149,7 @@ def get_percent_georeferenced(request): return percent_georeferenced @login_maybe_required +@statistics_response(Collectionobject, Collectionobjectattachment, Attachment) @openapi(schema={ 'get': { 'responses': { diff --git a/specifyweb/backend/stored_queries/execution.py b/specifyweb/backend/stored_queries/execution.py index cc893fb1516..1048bdb6b70 100644 --- a/specifyweb/backend/stored_queries/execution.py +++ b/specifyweb/backend/stored_queries/execution.py @@ -677,9 +677,10 @@ def createPlacemark(kmlDoc, row, coord_cols, table, captions, host): return placemarkElement -def run_ephemeral_query(collection, user, spquery): +def run_ephemeral_query(collection, user, spquery, *, validate_only: bool = False): """Execute a Specify query from deserialized json and return the results - as an array for json serialization to the web app. + as an array for json serialization to the web app. With validate_only, + build the query and check permissions without evaluating its results. """ logger.info("ephemeral query: %s", spquery) limit = spquery.get("limit", 20) @@ -708,6 +709,7 @@ def run_ephemeral_query(collection, user, spquery): offset=offset, recordsetid=recordsetid, formatauditobjs=format_audits, + validate_only=validate_only, ) @@ -872,6 +874,7 @@ def execute( recordsetid=None, formatauditobjs=False, formatter_props=None, + validate_only: bool = False, ): "Build and execute a query, returning the results as a data structure for json serialization" @@ -895,6 +898,9 @@ def execute( ), ) + if validate_only: + return None + log_sqlalchemy_query(query) if count_only: diff --git a/specifyweb/backend/stored_queries/views.py b/specifyweb/backend/stored_queries/views.py index 70c5240a718..d70ba9abe1e 100644 --- a/specifyweb/backend/stored_queries/views.py +++ b/specifyweb/backend/stored_queries/views.py @@ -12,6 +12,9 @@ from django.utils import timezone from specifyweb.middleware.general import require_GET +from specifyweb.backend.stats.coordination import ( + StatisticsBusy, busy_response, statistics_result, +) from specifyweb.backend.stored_queries.batch_edit import run_batch_edit from specifyweb.specify.models_utils.models_by_table_id import model_names_by_table_id from specifyweb.specify.api.serializers import toJson, uri_for_model @@ -118,6 +121,24 @@ def ephemeral(request): """Executes and returns the results of the query provided as JSON in the POST body.""" spquery, collection = get_query(request) + if request.headers.get("X-Specify-Statistics") == "true": + def validate_cached() -> None: + run_ephemeral_query( + collection, request.specify_user, spquery, validate_only=True + ) + + try: + content = statistics_result( + collection, + request.specify_user, + json.dumps(spquery, sort_keys=True), + lambda: toJson(run_ephemeral_query(collection, request.specify_user, spquery)), + validate_cached, + ) + except StatisticsBusy: + return busy_response() + return HttpResponse(content, content_type='application/json') + data = run_ephemeral_query(collection, request.specify_user, spquery) return HttpResponse(toJson(data), content_type='application/json') @@ -356,4 +377,3 @@ def return_loan_preps(request): item['loanpreparations'].append({'loanpreparationid': lp_id, 'quantity': int(quantity)}) return JsonResponse(resp, safe=False) - From 2cd2c05991230072959614be0cb095235603753a Mon Sep 17 00:00:00 2001 From: Caroline Denis Date: Tue, 20 Oct 2026 19:26:36 +0200 Subject: [PATCH 02/47] Fix: disable refresh while loading and retry busy requests --- .../lib/components/Statistics/Buttons.tsx | 21 ++++ .../lib/components/Statistics/StatItems.tsx | 19 ++-- .../lib/components/Statistics/hooks.tsx | 97 +++++++++++-------- .../lib/components/Statistics/index.tsx | 13 ++- .../lib/components/Statistics/requests.ts | 69 +++++++++++++ .../js_src/lib/utils/ajax/throttledPromise.ts | 78 +++++++++------ 6 files changed, 214 insertions(+), 83 deletions(-) create mode 100644 specifyweb/frontend/js_src/lib/components/Statistics/requests.ts diff --git a/specifyweb/frontend/js_src/lib/components/Statistics/Buttons.tsx b/specifyweb/frontend/js_src/lib/components/Statistics/Buttons.tsx index 0e8d6441f81..8465c3b21cb 100644 --- a/specifyweb/frontend/js_src/lib/components/Statistics/Buttons.tsx +++ b/specifyweb/frontend/js_src/lib/components/Statistics/Buttons.tsx @@ -1,7 +1,28 @@ import React from 'react'; import { commonText } from '../../localization/common'; +import { statsText } from '../../localization/stats'; import { Button } from '../Atoms/Button'; +import { statisticsAreLoading, useStatisticsLoading } from './requests'; + +export function StatsRefreshButton({ + onClick: handleClick, +}: { + readonly onClick: () => void; +}): JSX.Element { + const isLoading = useStatisticsLoading(); + return ( + { + if (!statisticsAreLoading()) handleClick(); + }} + > + {statsText.refresh()} + + ); +} export function StatsAsideButton({ label, diff --git a/specifyweb/frontend/js_src/lib/components/Statistics/StatItems.tsx b/specifyweb/frontend/js_src/lib/components/Statistics/StatItems.tsx index 8d20af31452..70b2b6ff0c2 100644 --- a/specifyweb/frontend/js_src/lib/components/Statistics/StatItems.tsx +++ b/specifyweb/frontend/js_src/lib/components/Statistics/StatItems.tsx @@ -4,7 +4,6 @@ import type { LocalizedString } from 'typesafe-i18n'; import { statsText } from '../../localization/stats'; import { userText } from '../../localization/user'; import type { AjaxResponseObject } from '../../utils/ajax'; -import { ajax } from '../../utils/ajax'; import { Http } from '../../utils/ajax/definitions'; import { throttledPromise } from '../../utils/ajax/throttledPromise'; import { localized } from '../../utils/types'; @@ -13,11 +12,12 @@ import { serializeResource } from '../DataModel/serializers'; import { getNoAccessTables } from '../QueryBuilder/helpers'; import { makeSerializedFieldsFromPaths, - queryCountPromiseGenerator, querySpecToResource, + statisticsCountPromiseGenerator, useResolvedStatSpec, useStatValueLoad, } from './hooks'; +import { statisticsAjax } from './requests'; import { StatsResult } from './StatsResult'; import type { BackendStatsResult, @@ -149,18 +149,19 @@ function BackEndItem({ throttledPromise( 'backendStats', async () => - ajax(fetchUrl, { + statisticsAjax(fetchUrl, { method: 'GET', headers: { Accept: 'application/json', }, expectedErrors: [Http.FORBIDDEN], - }).then(({ data, status }) => { - if (status === Http.FORBIDDEN) { + }).then((response) => { + if (response === undefined) return undefined; + if (response.status === Http.FORBIDDEN) { setStatPermission(false); return undefined; } - return data; + return response.data; }), fetchUrl ).then((data) => { @@ -239,9 +240,11 @@ function QueryItem({ const handleLoadResolve = statState === 'valid' ? handleLoad : undefined; const promiseGenerator = React.useCallback( async () => - throttledPromise>( + throttledPromise< + AjaxResponseObject<{ readonly count: number }> | undefined + >( 'queryStats', - queryCountPromiseGenerator(serializedQuery), + statisticsCountPromiseGenerator(serializedQuery), JSON.stringify(querySpec) ).then((response) => { if (response === undefined) return undefined; diff --git a/specifyweb/frontend/js_src/lib/components/Statistics/hooks.tsx b/specifyweb/frontend/js_src/lib/components/Statistics/hooks.tsx index 5c25d5ab521..9a72f9d6417 100644 --- a/specifyweb/frontend/js_src/lib/components/Statistics/hooks.tsx +++ b/specifyweb/frontend/js_src/lib/components/Statistics/hooks.tsx @@ -2,7 +2,7 @@ import React from 'react'; import { useMultipleAsyncState } from '../../hooks/useAsyncState'; import { statsText } from '../../localization/stats'; -import type { AjaxResponseObject } from '../../utils/ajax'; +import type { AjaxProps, AjaxResponseObject } from '../../utils/ajax'; import { ajax } from '../../utils/ajax'; import { Http } from '../../utils/ajax/definitions'; import { throttledPromise } from '../../utils/ajax/throttledPromise'; @@ -19,7 +19,9 @@ import { } from '../DataModel/serializers'; import { genericTables } from '../DataModel/tables'; import type { SpQuery, SpQueryField, Tables } from '../DataModel/types'; +import { raise } from '../Errors/Crash'; import { makeQueryField } from '../QueryBuilder/fromTree'; +import { statisticsAjax } from './requests'; import { backEndStatsSpec, dynamicStatsSpec, statsSpec } from './StatsSpec'; import type { BackEndStatResolve, @@ -81,14 +83,14 @@ function backEndStatPromiseGenerator( throttledPromise( 'backendStats', async () => - ajax(key, { + statisticsAjax(key, { method: 'GET', headers: { Accept: 'application/json', }, expectedErrors: [Http.FORBIDDEN], - }).then(({ data, status }) => - status === Http.FORBIDDEN ? undefined : data + }).then((response) => + response?.status === Http.OK ? response.data : undefined ), key ), @@ -106,27 +108,30 @@ function dynamicEphermeralPromiseGenerator( throttledPromise | undefined>( 'queryStats', async () => - ajax<{ readonly results: RA> }>( - '/stored_query/ephemeral/', - { - method: 'POST', - headers: { - Accept: 'application/json', - }, - body: keysToLowerCase({ - ...serializeResource( - querySpecToResource(statsText.statistics(), spec) - ), - limit: 0, - }), - expectedErrors: Object.values(Http), - } - ).then(({ data }) => - filterArray( - data.results.map(([_id, distinctGroup]) => - distinctGroup === null ? undefined : distinctGroup.toString() - ) - ) + statisticsAjax<{ + readonly results: RA>; + }>('/stored_query/ephemeral/', { + method: 'POST', + headers: { + Accept: 'application/json', + }, + body: keysToLowerCase({ + ...serializeResource( + querySpecToResource(statsText.statistics(), spec) + ), + limit: 0, + }), + expectedErrors: [Http.FORBIDDEN], + }).then((response) => + response?.status === Http.OK + ? filterArray( + response.data.results.map(([_id, distinctGroup]) => + distinctGroup === null + ? undefined + : distinctGroup.toString() + ) + ) + : undefined ), key ), @@ -195,20 +200,28 @@ export function queryCountPromiseGenerator( return async () => ajax<{ readonly count: number; - }>('/stored_query/ephemeral/', { - method: 'POST', - headers: { - // eslint-disable-next-line @typescript-eslint/naming-convention - Accept: 'application/json', - }, - body: keysToLowerCase({ - ...query, - countOnly: true, - }), - expectedErrors: Object.values(Http), - }); + }>('/stored_query/ephemeral/', countRequestOptions(query)); } +export function statisticsCountPromiseGenerator( + query: SerializedResource +): () => Promise | undefined> { + return async () => + statisticsAjax<{ readonly count: number }>( + '/stored_query/ephemeral/', + countRequestOptions(query) + ); +} + +const countRequestOptions = ( + query: SerializedResource +): AjaxProps => ({ + method: 'POST', + headers: { Accept: 'application/json' }, + body: keysToLowerCase({ ...query, countOnly: true }), + expectedErrors: Object.values(Http), +}); + export const makeSerializedFieldsFromPaths = ( tableName: keyof Tables, fields: RA @@ -411,10 +424,12 @@ export function useStatValueLoad< React.useEffect(() => { if (!shouldFetch) return undefined; let destructorCalled = false; - promiseGenerator().then((value) => { - if (destructorCalled || value === undefined) return; - handleLoad?.(value); - }); + promiseGenerator() + .then((value) => { + if (destructorCalled || value === undefined) return; + handleLoad?.(value); + }) + .catch(raise); return (): void => { destructorCalled = true; }; diff --git a/specifyweb/frontend/js_src/lib/components/Statistics/index.tsx b/specifyweb/frontend/js_src/lib/components/Statistics/index.tsx index 1c420008e7d..af714c2bacb 100644 --- a/specifyweb/frontend/js_src/lib/components/Statistics/index.tsx +++ b/specifyweb/frontend/js_src/lib/components/Statistics/index.tsx @@ -24,7 +24,7 @@ import { ProtectedAction } from '../Permissions/PermissionDenied'; import { collectionPreferences } from '../Preferences/collectionPreferences'; import { userPreferences } from '../Preferences/userPreferences'; import { AddStatDialog } from './AddStatDialog'; -import { StatsAsideButton } from './Buttons'; +import { StatsAsideButton, StatsRefreshButton } from './Buttons'; import { Categories } from './Categories'; import { applyRefreshLayout, @@ -41,6 +41,7 @@ import { useDynamicCategorySetter, useDynamicGroups, } from './hooks'; +import { cancelStatisticsRequests, statisticsAreLoading } from './requests'; import { StatsPageEditing } from './StatsPageEditing'; import { backEndStatsSpec, @@ -296,7 +297,10 @@ function ProtectedStatsPage(): JSX.Element | null { /* Set Default Layout every time page is started from scratch*/ React.useEffect(() => { setDefaultLayout(defaultLayoutGenerated); - return cleanThrottledPromises; + return () => { + cancelStatisticsRequests(); + cleanThrottledPromises(); + }; }, [setDefaultLayout]); const pageLastUpdated = activePage.isShared @@ -477,6 +481,7 @@ function ProtectedStatsPage(): JSX.Element | null { ); const refreshPage = () => { + if (statisticsAreLoading()) return; cleanThrottledPromises(false); setCurrentLayout((layout) => layout === undefined @@ -517,9 +522,7 @@ function ProtectedStatsPage(): JSX.Element | null { )} - refreshPage()}> - {statsText.refresh()} - + {Object.values(layout).every((layouts) => layouts !== undefined) && ( { diff --git a/specifyweb/frontend/js_src/lib/components/Statistics/requests.ts b/specifyweb/frontend/js_src/lib/components/Statistics/requests.ts new file mode 100644 index 00000000000..b80eac5c6fa --- /dev/null +++ b/specifyweb/frontend/js_src/lib/components/Statistics/requests.ts @@ -0,0 +1,69 @@ +import React from 'react'; + +import type { AjaxProps, AjaxResponseObject } from '../../utils/ajax'; +import { ajax } from '../../utils/ajax'; +import { Http } from '../../utils/ajax/definitions'; +import { + subscribeThrottledPromises, + throttledPromisesAreLoading, +} from '../../utils/ajax/throttledPromise'; + +const pending = new Set(); +const listeners = new Set<() => void>(); +let generation = 0; + +const notify = (): void => listeners.forEach((listener) => listener()); + +export const statisticsAreLoading = (): boolean => + pending.size > 0 || throttledPromisesAreLoading(); + +export function useStatisticsLoading(): boolean { + return React.useSyncExternalStore( + React.useCallback((listener) => { + listeners.add(listener); + const unsubscribe = subscribeThrottledPromises(listener); + return () => { + listeners.delete(listener); + unsubscribe(); + }; + }, []), + statisticsAreLoading + ); +} + +export function cancelStatisticsRequests(): void { + generation += 1; +} + +export async function statisticsAjax( + url: string, + options: AjaxProps +): Promise | undefined> { + const requestGeneration = generation; + const token = Symbol(); + pending.add(token); + notify(); + try { + while (requestGeneration === generation) { + const result = await ajax(url, { + ...options, + headers: { ...options.headers, 'X-Specify-Statistics': 'true' }, + expectedErrors: [...(options.expectedErrors ?? []), Http.CONFLICT], + }); + if (requestGeneration !== generation) return undefined; + if (result.status !== Http.CONFLICT) return result; + const retryAfter = Number(result.response.headers.get('Retry-After')); + await new Promise((resolve) => + globalThis.setTimeout( + resolve, + (Number.isFinite(retryAfter) && retryAfter > 0 ? retryAfter : 2) * + 1000 + ) + ); + } + return undefined; + } finally { + pending.delete(token); + notify(); + } +} diff --git a/specifyweb/frontend/js_src/lib/utils/ajax/throttledPromise.ts b/specifyweb/frontend/js_src/lib/utils/ajax/throttledPromise.ts index 715e7739f58..626a8e69b20 100644 --- a/specifyweb/frontend/js_src/lib/utils/ajax/throttledPromise.ts +++ b/specifyweb/frontend/js_src/lib/utils/ajax/throttledPromise.ts @@ -6,23 +6,31 @@ export type PromiseWithSpec = Promise & { }; type KillablePromise = PromiseWithSpec & { + readonly group: string; // eslint-disable-next-line functional/prefer-readonly-type killPromise: () => void; }; const currentRequestsGenerator = (): WritableArray> => []; let maybeFulfilled: WritableArray> = []; +const pendingPromises = new Set>(); +const listeners = new Set<() => void>(); + +export const throttledPromisesAreLoading = (): boolean => + pendingPromises.size > 0; + +export function subscribeThrottledPromises(listener: () => void): () => void { + listeners.add(listener); + return () => { + listeners.delete(listener); + }; +} + +const notify = (): void => listeners.forEach((listener) => listener()); export const cleanThrottledPromises = (killPromises = true): void => { if (killPromises) maybeFulfilled.forEach((promise) => promise.killPromise()); maybeFulfilled = []; - /* - * Since the kill promise is already supplied, the promises will resolve within a work loop. - * So, it is sufficient to directly empty the currentRequests for future requests. - */ - Object.entries(networkRequestsSpec).forEach(([key, _]) => { - networkRequestsSpec[key].currentRequests = []; - }); }; export const networkRequestsSpec: RR< @@ -49,34 +57,46 @@ export async function throttledPromise( promiseSpec: number | string ): Promise { const { maxFetchCount, currentRequests } = networkRequestsSpec[key]; - while (currentRequests.length > maxFetchCount) - await Promise.any(currentRequests); const promiseInFulfilled = maybeFulfilled.find( - (fulfilledPromise) => fulfilledPromise.spec === promiseSpec + (fulfilledPromise) => + fulfilledPromise.group === key && fulfilledPromise.spec === promiseSpec ); if (promiseInFulfilled !== undefined) return promiseInFulfilled; let promiseKilled = false; - const returnPromise: KillablePromise = new Promise( - async (resolve) => { - while (currentRequests.length >= maxFetchCount && !promiseKilled) { - await Promise.any(currentRequests); - } - if (promiseKilled) { - resolve(undefined); - return; - } - const promiseToAdd = promiseGenerator().then((result) => { - currentRequests.splice(currentRequests.indexOf(promiseToAdd), 1); - resolve(result); - }); - currentRequests.push(promiseToAdd); + const run = async (): Promise => { + while (currentRequests.length >= maxFetchCount && !promiseKilled) { + await Promise.race( + currentRequests.map((promise) => + promise.then( + () => undefined, + () => undefined + ) + ) + ); + } + if (promiseKilled) return undefined; + const promiseToAdd = promiseGenerator(); + currentRequests.push(promiseToAdd); + try { + return await promiseToAdd; + } finally { + currentRequests.splice(currentRequests.indexOf(promiseToAdd), 1); } - ) as KillablePromise; - returnPromise.spec = promiseSpec; - - returnPromise.killPromise = () => { - promiseKilled = true; }; + const returnPromise: KillablePromise = Object.assign(run(), { + spec: promiseSpec, + group: key, + killPromise: () => { + promiseKilled = true; + }, + }); maybeFulfilled.push(returnPromise); + pendingPromises.add(returnPromise); + notify(); + const settled = (): void => { + pendingPromises.delete(returnPromise); + notify(); + }; + returnPromise.then(settled, settled); return returnPromise; } From 17487199cbd12aa63b99b8bd1ada76c43fe5f5f4 Mon Sep 17 00:00:00 2001 From: Caroline Denis Date: Tue, 20 Oct 2026 19:27:46 +0200 Subject: [PATCH 03/47] Tests: Add backend tests for stats coordination --- specifyweb/backend/stats/tests.py | 253 ++++++++++++++++++++++++++++++ 1 file changed, 253 insertions(+) create mode 100644 specifyweb/backend/stats/tests.py diff --git a/specifyweb/backend/stats/tests.py b/specifyweb/backend/stats/tests.py new file mode 100644 index 00000000000..551650501e0 --- /dev/null +++ b/specifyweb/backend/stats/tests.py @@ -0,0 +1,253 @@ +import json +from types import SimpleNamespace +from unittest.mock import Mock, patch + +from django.core.exceptions import PermissionDenied +from django.http import JsonResponse +from django.test import RequestFactory, SimpleTestCase, override_settings + +from .coordination import ( + STATISTICS_CACHE_SECONDS, + StatisticsBusy, + busy_response, + statistics_response, + statistics_result, +) + + +@override_settings(DATABASE_NAME="statistics_test") +class StatisticsCoordinationTests(SimpleTestCase): + def setUp(self): + self.collection = SimpleNamespace(id=1) + self.user = SimpleNamespace(id=2) + self.calculate = Mock(return_value='{"count": 12}') + self.validate = Mock() + self.cache = Mock() + self.cache.get.return_value = None + cache_patch = patch( + "specifyweb.backend.stats.coordination.redis_connection", + return_value=self.cache, + ) + lock_patch = patch("specifyweb.backend.stats.coordination.Lock") + cache_patch.start() + self.lock_class = lock_patch.start() + self.addCleanup(cache_patch.stop) + self.addCleanup(lock_patch.stop) + self.lock = self.lock_class.return_value + + def run_statistic(self, query_key="query"): + return statistics_result( + self.collection, self.user, query_key, self.calculate, self.validate + ) + + def test_result_is_cached_for_five_minutes(self): + self.assertEqual(self.run_statistic(), '{"count": 12}') + self.lock.acquire.assert_called_once_with() + self.lock.release.assert_called_once_with() + self.calculate.assert_called_once_with() + self.assertEqual(self.lock_class.call_args.kwargs, {"timeout": 0}) + self.assertLessEqual(len(self.lock_class.call_args.args[0]), 64) + self.cache.set.assert_called_once_with( + self.cache.get.call_args.args[0], + '{"count": 12}', + ex=STATISTICS_CACHE_SECONDS, + ) + self.assertEqual(STATISTICS_CACHE_SECONDS, 300) + + def test_cached_result_rechecks_permissions_without_running_query(self): + self.cache.get.return_value = '{"count": 12}' + self.assertEqual(self.run_statistic(), '{"count": 12}') + self.validate.assert_called_once_with() + self.calculate.assert_not_called() + self.lock.acquire.assert_not_called() + + def test_revoked_permissions_prevent_cached_result_access(self): + self.cache.get.return_value = '{"count": 12}' + self.validate.side_effect = PermissionDenied + with self.assertRaises(PermissionDenied): + self.run_statistic() + self.calculate.assert_not_called() + + def test_busy_database_does_not_start_or_wait_for_another_calculation(self): + self.lock.acquire.side_effect = TimeoutError + with self.assertRaises(StatisticsBusy): + self.run_statistic() + self.calculate.assert_not_called() + self.cache.set.assert_not_called() + self.lock.release.assert_not_called() + response = busy_response() + self.assertEqual(response.status_code, 409) + self.assertEqual(response["Retry-After"], "2") + self.assertIn("message", json.loads(response.content)) + + def test_lock_connection_errors_are_not_reported_as_busy(self): + self.lock.acquire.side_effect = ConnectionError + with self.assertRaises(ConnectionError): + self.run_statistic() + + def test_calculation_errors_release_lock_without_caching(self): + self.calculate.side_effect = ValueError("calculation failed") + with self.assertRaisesRegex(ValueError, "calculation failed"): + self.run_statistic() + self.lock.release.assert_called_once_with() + self.cache.set.assert_not_called() + + def test_query_timeouts_are_not_mistaken_for_lock_contention(self): + self.calculate.side_effect = TimeoutError("query timeout") + with self.assertRaisesRegex(TimeoutError, "query timeout"): + self.run_statistic() + self.lock.release.assert_called_once_with() + + def test_cache_is_checked_again_after_acquiring_lock(self): + self.cache.get.side_effect = [None, '{"count": 12}'] + self.assertEqual(self.run_statistic(), '{"count": 12}') + self.validate.assert_called_once_with() + self.calculate.assert_not_called() + self.lock.release.assert_called_once_with() + + def test_cache_keys_isolate_users_collections_queries_and_databases(self): + keys = [] + self.run_statistic() + keys.append(self.cache.get.call_args.args[0]) + self.user.id = 3 + self.run_statistic() + keys.append(self.cache.get.call_args.args[0]) + self.collection.id = 4 + self.run_statistic() + keys.append(self.cache.get.call_args.args[0]) + self.run_statistic("another query") + keys.append(self.cache.get.call_args.args[0]) + with override_settings(DATABASE_NAME="another_database"): + self.run_statistic("another query") + keys.append(self.cache.get.call_args.args[0]) + self.assertEqual(len(set(keys)), 5) + + def test_different_users_and_collections_share_database_gate(self): + self.run_statistic() + name = self.lock_class.call_args.args[0] + self.user.id = 5 + self.collection.id = 6 + self.run_statistic("another query") + self.assertEqual(self.lock_class.call_args.args[0], name) + + @patch("specifyweb.backend.stats.coordination.check_table_permissions") + def test_backend_view_rechecks_permissions_on_cache_hits(self, check): + table = Mock() + request = RequestFactory().get("/stats/collection/preparations/") + request.specify_collection = self.collection + request.specify_user = self.user + view = Mock(return_value=JsonResponse({"lots": 12})) + wrapped = statistics_response(table)(view) + self.cache.get.return_value = '{"lots": 12}' + response = wrapped(request) + self.assertEqual(json.loads(response.content), {"lots": 12}) + check.assert_called_with(self.collection, self.user, table, "read") + view.assert_not_called() + + @patch("specifyweb.backend.stats.coordination.check_table_permissions") + def test_backend_view_returns_busy_response(self, check): + request = RequestFactory().get("/stats/collection/preparations/") + request.specify_collection = self.collection + request.specify_user = self.user + self.lock.acquire.side_effect = TimeoutError + view = Mock() + response = statistics_response(Mock())(view)(request) + self.assertEqual(response.status_code, 409) + check.assert_called_once() + view.assert_not_called() + + def ephemeral_request(self, statistics=True): + request = RequestFactory().post( + "/stored_query/ephemeral/", + {}, + content_type="application/json", + **({"HTTP_X_SPECIFY_STATISTICS": "true"} if statistics else {}), + ) + request.user = SimpleNamespace(is_authenticated=True) + request.specify_user = self.user + request.specify_collection = self.collection + return request + + @patch("specifyweb.backend.stored_queries.views.run_ephemeral_query") + @patch("specifyweb.backend.stored_queries.views.get_query") + def test_ephemeral_statistics_are_gated_and_cached(self, get_query, execute): + from specifyweb.backend.stored_queries.views import ephemeral + + query = {"countonly": True} + get_query.return_value = query, self.collection + execute.return_value = {"count": 12} + response = ephemeral(self.ephemeral_request()) + self.assertEqual(json.loads(response.content), {"count": 12}) + execute.assert_called_once_with(self.collection, self.user, query) + self.lock.acquire.assert_called_once_with() + self.cache.set.assert_called_once() + + @patch("specifyweb.backend.stored_queries.views.run_ephemeral_query") + @patch("specifyweb.backend.stored_queries.views.get_query") + def test_ephemeral_cache_hit_validates_query_without_executing(self, get_query, execute): + from specifyweb.backend.stored_queries.views import ephemeral + + query = {"countonly": True} + get_query.return_value = query, self.collection + self.cache.get.return_value = '{"count": 12}' + response = ephemeral(self.ephemeral_request()) + self.assertEqual(json.loads(response.content), {"count": 12}) + execute.assert_called_once_with( + self.collection, self.user, query, validate_only=True + ) + self.lock.acquire.assert_not_called() + + @patch("specifyweb.backend.stored_queries.views.run_ephemeral_query") + @patch("specifyweb.backend.stored_queries.views.get_query") + def test_ephemeral_busy_response_does_not_execute_query(self, get_query, execute): + from specifyweb.backend.stored_queries.views import ephemeral + + get_query.return_value = {"countonly": True}, self.collection + self.lock.acquire.side_effect = TimeoutError + response = ephemeral(self.ephemeral_request()) + self.assertEqual(response.status_code, 409) + self.assertEqual(response["Retry-After"], "2") + execute.assert_not_called() + + @patch("specifyweb.backend.stored_queries.views.run_ephemeral_query") + @patch("specifyweb.backend.stored_queries.views.get_query") + def test_ordinary_ephemeral_queries_are_unchanged(self, get_query, execute): + from specifyweb.backend.stored_queries.views import ephemeral + + query = {"countonly": True} + get_query.return_value = query, self.collection + execute.return_value = {"count": 12} + response = ephemeral(self.ephemeral_request(statistics=False)) + self.assertEqual(json.loads(response.content), {"count": 12}) + execute.assert_called_once_with(self.collection, self.user, query) + self.cache.get.assert_not_called() + self.lock.acquire.assert_not_called() + + +class StatisticsPermissionValidationTests(SimpleTestCase): + @patch("specifyweb.backend.stored_queries.execution.set_group_concat_max_len") + @patch("specifyweb.backend.stored_queries.execution.build_query") + def test_validation_builds_and_checks_query_without_evaluating_it(self, build, setup): + from specifyweb.backend.stored_queries.execution import execute + + query = Mock() + build.return_value = query, [] + result = execute( + session=SimpleNamespace(info={"connection": Mock()}), + collection=Mock(), + user=Mock(), + tableid=1, + distinct=False, + series=False, + search_synonymy=False, + count_only=True, + query_fields=[], + limit=0, + offset=0, + validate_only=True, + ) + self.assertIsNone(result) + setup.assert_called_once() + build.assert_called_once() + query.count.assert_not_called() + query.all.assert_not_called() From 8b8145d6349d970ceccb98af6919b04ca393449f Mon Sep 17 00:00:00 2001 From: Caroline Denis Date: Tue, 20 Oct 2026 19:28:43 +0200 Subject: [PATCH 04/47] Tests: Add frontend tests to verify refresh button is disabled when stat loading --- .../Statistics/__tests__/Buttons.test.tsx | 79 +++++++++++++++++++ 1 file changed, 79 insertions(+) create mode 100644 specifyweb/frontend/js_src/lib/components/Statistics/__tests__/Buttons.test.tsx diff --git a/specifyweb/frontend/js_src/lib/components/Statistics/__tests__/Buttons.test.tsx b/specifyweb/frontend/js_src/lib/components/Statistics/__tests__/Buttons.test.tsx new file mode 100644 index 00000000000..d2b30b5282f --- /dev/null +++ b/specifyweb/frontend/js_src/lib/components/Statistics/__tests__/Buttons.test.tsx @@ -0,0 +1,79 @@ +import { act, fireEvent, render, screen } from '@testing-library/react'; +import React from 'react'; + +import { statsText } from '../../../localization/stats'; +import { ajax } from '../../../utils/ajax'; +import { Http } from '../../../utils/ajax/definitions'; +import { + cleanThrottledPromises, + throttledPromise, +} from '../../../utils/ajax/throttledPromise'; +import { StatsRefreshButton } from '../Buttons'; +import { statisticsAjax } from '../requests'; + +jest.mock('../../../utils/ajax', () => ({ ajax: jest.fn() })); + +afterEach(() => cleanThrottledPromises()); + +test('Refresh is disabled and ignores clicks until requests finish', async () => { + let finish!: (value: { + readonly status: number; + readonly data: { readonly count: number }; + readonly response: Response; + }) => void; + jest.mocked(ajax).mockImplementationOnce( + () => + new Promise((resolve) => { + finish = resolve; + }) + ); + const refreshPage = jest.fn(); + render(); + const refresh = screen.getByRole('button', { name: statsText.refresh() }); + expect(refresh).toBeEnabled(); + let request!: ReturnType; + act(() => { + request = statisticsAjax('/stored_query/ephemeral/', { + method: 'POST', + headers: { Accept: 'application/json' }, + }); + }); + expect(refresh).toBeDisabled(); + fireEvent.click(refresh); + expect(refreshPage).not.toHaveBeenCalled(); + await act(async () => { + finish({ + status: Http.OK, + data: { count: 12 }, + response: new Response(undefined, { status: Http.OK }), + }); + await request; + }); + expect(refresh).toBeEnabled(); + fireEvent.click(refresh); + expect(refreshPage).toHaveBeenCalledTimes(1); +}); + +test('Refresh is disabled for queued requests before their network request starts', async () => { + let finish!: (value: number) => void; + const running = throttledPromise( + 'backendStats', + () => + new Promise((resolve) => { + finish = resolve; + }), + 'running' + ); + const calculate = jest.fn(async () => 2); + const queued = throttledPromise('backendStats', calculate, 'queued'); + render(); + const refresh = screen.getByRole('button', { name: statsText.refresh() }); + expect(calculate).not.toHaveBeenCalled(); + expect(refresh).toBeDisabled(); + await act(async () => { + finish(1); + await Promise.all([running, queued]); + }); + expect(calculate).toHaveBeenCalledTimes(1); + expect(refresh).toBeEnabled(); +}); From b876e2d10cb6d976d4fca66d2905785740d4db6a Mon Sep 17 00:00:00 2001 From: Caroline Denis Date: Tue, 20 Oct 2026 19:29:35 +0200 Subject: [PATCH 05/47] Tests: Add frontend for stats queuing --- .../Statistics/__tests__/requests.test.ts | 121 ++++++++++++++++++ .../ajax/__tests__/throttledPromise.test.ts | 83 ++++++++++++ 2 files changed, 204 insertions(+) create mode 100644 specifyweb/frontend/js_src/lib/components/Statistics/__tests__/requests.test.ts create mode 100644 specifyweb/frontend/js_src/lib/utils/ajax/__tests__/throttledPromise.test.ts diff --git a/specifyweb/frontend/js_src/lib/components/Statistics/__tests__/requests.test.ts b/specifyweb/frontend/js_src/lib/components/Statistics/__tests__/requests.test.ts new file mode 100644 index 00000000000..634dded2a13 --- /dev/null +++ b/specifyweb/frontend/js_src/lib/components/Statistics/__tests__/requests.test.ts @@ -0,0 +1,121 @@ +import { act, renderHook } from '@testing-library/react'; + +import { ajax } from '../../../utils/ajax'; +import { Http } from '../../../utils/ajax/definitions'; +import { + cancelStatisticsRequests, + statisticsAjax, + statisticsAreLoading, + useStatisticsLoading, +} from '../requests'; + +jest.mock('../../../utils/ajax', () => ({ ajax: jest.fn() })); + +const ajaxMock = jest.mocked(ajax); +const options = { headers: { Accept: 'application/json' as const } }; +const result = (status: number, retryAfter?: string) => ({ + status, + data: { count: 12 }, + response: Object.assign(new Response(undefined, { status }), { + headers: { get: () => retryAfter ?? null }, + }), +}); + +beforeEach(() => { + ajaxMock.mockReset(); + jest.useFakeTimers(); +}); + +afterEach(() => { + cancelStatisticsRequests(); + jest.useRealTimers(); +}); + +test('marks statistics requests and tracks loading through completion', async () => { + const { result: loading } = renderHook(useStatisticsLoading); + let resolveRequest!: (value: ReturnType) => void; + ajaxMock.mockImplementationOnce( + () => + new Promise((resolve) => { + resolveRequest = resolve; + }) + ); + let request!: ReturnType; + act(() => { + request = statisticsAjax('/stats/collection/preparations/', options); + }); + expect(loading.current).toBe(true); + expect(ajaxMock).toHaveBeenCalledWith( + '/stats/collection/preparations/', + expect.objectContaining({ + headers: expect.objectContaining({ 'X-Specify-Statistics': 'true' }), + expectedErrors: [Http.CONFLICT], + }) + ); + await act(async () => { + resolveRequest(result(Http.OK)); + await request; + }); + expect(loading.current).toBe(false); +}); + +test('retries busy responses using Retry-After while remaining loading', async () => { + ajaxMock + .mockResolvedValueOnce(result(Http.CONFLICT, '3')) + .mockResolvedValueOnce(result(Http.OK)); + const request = statisticsAjax('/stored_query/ephemeral/', options); + await jest.advanceTimersByTimeAsync(2999); + expect(ajaxMock).toHaveBeenCalledTimes(1); + expect(statisticsAreLoading()).toBe(true); + await jest.advanceTimersByTimeAsync(1); + expect((await request)?.data).toEqual({ count: 12 }); + expect(ajaxMock).toHaveBeenCalledTimes(2); + expect(statisticsAreLoading()).toBe(false); +}); + +test('navigation cancels retries instead of starting more database work', async () => { + ajaxMock.mockResolvedValueOnce(result(Http.CONFLICT, '2')); + const request = statisticsAjax('/stored_query/ephemeral/', options); + await jest.advanceTimersByTimeAsync(0); + cancelStatisticsRequests(); + await jest.advanceTimersByTimeAsync(2000); + expect(await request).toBeUndefined(); + expect(ajaxMock).toHaveBeenCalledTimes(1); + expect(statisticsAreLoading()).toBe(false); +}); + +test('loading stays true until every concurrent request finishes', async () => { + let finish!: (value: ReturnType) => void; + ajaxMock + .mockImplementationOnce( + () => + new Promise((resolve) => { + finish = resolve; + }) + ) + .mockResolvedValueOnce(result(Http.OK)); + const first = statisticsAjax('/stats/one/', options); + await statisticsAjax('/stats/two/', options); + expect(statisticsAreLoading()).toBe(true); + finish(result(Http.OK)); + await first; + expect(statisticsAreLoading()).toBe(false); +}); + +test('request failures propagate and clear loading', async () => { + ajaxMock.mockRejectedValueOnce(new Error('request failed')); + await expect( + statisticsAjax('/stats/collection/preparations/', options) + ).rejects.toThrow('request failed'); + expect(statisticsAreLoading()).toBe(false); +}); + +test('permission errors are not retried', async () => { + ajaxMock.mockResolvedValueOnce(result(Http.FORBIDDEN)); + const response = await statisticsAjax('/stored_query/ephemeral/', { + ...options, + expectedErrors: [Http.FORBIDDEN], + }); + expect(response?.status).toBe(Http.FORBIDDEN); + expect(ajaxMock).toHaveBeenCalledTimes(1); +}); diff --git a/specifyweb/frontend/js_src/lib/utils/ajax/__tests__/throttledPromise.test.ts b/specifyweb/frontend/js_src/lib/utils/ajax/__tests__/throttledPromise.test.ts new file mode 100644 index 00000000000..1733884edff --- /dev/null +++ b/specifyweb/frontend/js_src/lib/utils/ajax/__tests__/throttledPromise.test.ts @@ -0,0 +1,83 @@ +import { + cleanThrottledPromises, + networkRequestsSpec, + throttledPromise, +} from '../throttledPromise'; + +afterEach(() => { + cleanThrottledPromises(); + expect(networkRequestsSpec.backendStats.currentRequests).toHaveLength(0); + expect(networkRequestsSpec.queryStats.currentRequests).toHaveLength(0); +}); + +test('refresh does not discard active requests or bypass concurrency limits', async () => { + let finish!: (value: number) => void; + const first = throttledPromise( + 'backendStats', + () => + new Promise((resolve) => { + finish = resolve; + }), + 'first' + ); + cleanThrottledPromises(false); + const next = jest.fn(async () => 2); + const second = throttledPromise('backendStats', next, 'second'); + expect(next).not.toHaveBeenCalled(); + finish(1); + expect(await first).toBe(1); + expect(await second).toBe(2); +}); + +test('cleanup cancels queued requests but retains active request accounting', async () => { + let finish!: (value: number) => void; + const first = throttledPromise( + 'backendStats', + () => + new Promise((resolve) => { + finish = resolve; + }), + 'first' + ); + const next = jest.fn(async () => 2); + const second = throttledPromise('backendStats', next, 'second'); + cleanThrottledPromises(); + expect(networkRequestsSpec.backendStats.currentRequests).toHaveLength(1); + finish(1); + await first; + expect(await second).toBeUndefined(); + expect(next).not.toHaveBeenCalled(); +}); + +test('rejected requests propagate errors and release their slot', async () => { + const first = throttledPromise( + 'backendStats', + async () => { + throw new Error('failed'); + }, + 'first' + ); + const second = throttledPromise('backendStats', async () => 2, 'second'); + await expect(first).rejects.toThrow('failed'); + expect(await second).toBe(2); +}); + +test('identical requests are deduplicated within a request group', async () => { + const calculate = jest.fn(async () => 12); + expect( + await Promise.all([ + throttledPromise('queryStats', calculate, 'same'), + throttledPromise('queryStats', calculate, 'same'), + ]) + ).toEqual([12, 12]); + expect(calculate).toHaveBeenCalledTimes(1); +}); + +test('matching specifications in different groups do not share results', async () => { + expect( + await Promise.all([ + throttledPromise('backendStats', async () => 'backend', 'same'), + throttledPromise('queryStats', async () => 'query', 'same'), + ]) + ).toEqual(['backend', 'query']); +}); From e608c7bb0da54e87813f07dab42b357907afd89b Mon Sep 17 00:00:00 2001 From: Caroline Denis Date: Tue, 20 Oct 2026 19:48:02 +0200 Subject: [PATCH 06/47] Fix: Make the Statistics cache optional. --- specifyweb/backend/stats/coordination.py | 24 +++++++++-- specifyweb/backend/stats/tests.py | 52 +++++++++++++++++++++++- 2 files changed, 72 insertions(+), 4 deletions(-) diff --git a/specifyweb/backend/stats/coordination.py b/specifyweb/backend/stats/coordination.py index c66280ef9bb..5acf4c415d5 100644 --- a/specifyweb/backend/stats/coordination.py +++ b/specifyweb/backend/stats/coordination.py @@ -6,6 +6,7 @@ from django.conf import settings from django.http import HttpResponse, JsonResponse +from redis.exceptions import RedisError from specifyweb.backend.cache.redis.utils import format_key, redis_connection from specifyweb.backend.permissions.permissions import check_table_permissions @@ -37,14 +38,27 @@ def statistics_result( calculate: Callable[[], str], validate_cached: Callable[[], None], ) -> str: + """Reuse cached statistics when Redis is available; always gate calculations.""" digest = hashlib.sha256(query_key.encode()).hexdigest() cache_key = format_key( f"specify:{{database}}:statistics:{collection.id}:{user.id}:{digest}" ) - cache = redis_connection() + try: + cache = redis_connection() + except (ValueError, RedisError): + logger.warning("Statistics cache unavailable; calculating without caching.", exc_info=True) + cache = None def cached_result() -> str | None: - result = cache.get(cache_key) + nonlocal cache + if cache is None: + return None + try: + result = cache.get(cache_key) + except RedisError: + logger.warning("Statistics cache read failed; calculating without caching.", exc_info=True) + cache = None + return None if result is not None: validate_cached() return result @@ -66,7 +80,11 @@ def cached_result() -> str | None: result = cached_result() if result is None: result = calculate() - cache.set(cache_key, result, ex=STATISTICS_CACHE_SECONDS) + if cache is not None: + try: + cache.set(cache_key, result, ex=STATISTICS_CACHE_SECONDS) + except RedisError: + logger.warning("Statistics cache write failed; returning uncached result.", exc_info=True) return result finally: lock.release() diff --git a/specifyweb/backend/stats/tests.py b/specifyweb/backend/stats/tests.py index 551650501e0..c2983a2afa0 100644 --- a/specifyweb/backend/stats/tests.py +++ b/specifyweb/backend/stats/tests.py @@ -5,6 +5,7 @@ from django.core.exceptions import PermissionDenied from django.http import JsonResponse from django.test import RequestFactory, SimpleTestCase, override_settings +from redis.exceptions import ConnectionError as RedisConnectionError, TimeoutError as RedisTimeoutError from .coordination import ( STATISTICS_CACHE_SECONDS, @@ -29,7 +30,7 @@ def setUp(self): return_value=self.cache, ) lock_patch = patch("specifyweb.backend.stats.coordination.Lock") - cache_patch.start() + self.redis_connection = cache_patch.start() self.lock_class = lock_patch.start() self.addCleanup(cache_patch.stop) self.addCleanup(lock_patch.stop) @@ -54,6 +55,55 @@ def test_result_is_cached_for_five_minutes(self): ) self.assertEqual(STATISTICS_CACHE_SECONDS, 300) + def assert_uncached_calculation(self): + self.assertEqual(self.run_statistic(), '{"count": 12}') + self.calculate.assert_called_once_with() + self.lock.acquire.assert_called_once_with() + self.lock.release.assert_called_once_with() + self.validate.assert_not_called() + + def test_unconfigured_redis_still_calculates_under_lock(self): + self.redis_connection.side_effect = ValueError("Redis is not correctly configured") + with self.assertLogs("specifyweb.backend.stats.coordination", level="WARNING"): + self.assert_uncached_calculation() + self.cache.get.assert_not_called() + self.cache.set.assert_not_called() + + def test_redis_connection_failure_still_calculates_under_lock(self): + self.redis_connection.side_effect = RedisConnectionError("unavailable") + with self.assertLogs("specifyweb.backend.stats.coordination", level="WARNING"): + self.assert_uncached_calculation() + self.cache.set.assert_not_called() + + def test_cache_read_failure_disables_cache_for_request(self): + self.cache.get.side_effect = RedisConnectionError("unavailable") + with self.assertLogs("specifyweb.backend.stats.coordination", level="WARNING"): + self.assert_uncached_calculation() + self.cache.get.assert_called_once() + self.cache.set.assert_not_called() + + def test_second_cache_read_failure_still_releases_lock_and_returns_result(self): + self.cache.get.side_effect = [None, RedisTimeoutError("timed out")] + with self.assertLogs("specifyweb.backend.stats.coordination", level="WARNING"): + self.assert_uncached_calculation() + self.assertEqual(self.cache.get.call_count, 2) + self.cache.set.assert_not_called() + + def test_cache_write_failure_returns_calculated_result_and_releases_lock(self): + self.cache.set.side_effect = RedisTimeoutError("timed out") + with self.assertLogs("specifyweb.backend.stats.coordination", level="WARNING"): + self.assert_uncached_calculation() + self.cache.set.assert_called_once() + + def test_unavailable_cache_does_not_bypass_busy_database_lock(self): + self.cache.get.side_effect = RedisConnectionError("unavailable") + self.lock.acquire.side_effect = TimeoutError + with self.assertLogs("specifyweb.backend.stats.coordination", level="WARNING"): + with self.assertRaises(StatisticsBusy): + self.run_statistic() + self.calculate.assert_not_called() + self.lock.release.assert_not_called() + def test_cached_result_rechecks_permissions_without_running_query(self): self.cache.get.return_value = '{"count": 12}' self.assertEqual(self.run_statistic(), '{"count": 12}') From 0cf07d5553137e5eeb748601f16f48defbd7e77b Mon Sep 17 00:00:00 2001 From: Caroline Denis Date: Tue, 20 Oct 2026 19:49:05 +0200 Subject: [PATCH 07/47] Fix: Restore tolerant error handling for dynamic-group queries. --- specifyweb/frontend/js_src/lib/components/Statistics/hooks.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/specifyweb/frontend/js_src/lib/components/Statistics/hooks.tsx b/specifyweb/frontend/js_src/lib/components/Statistics/hooks.tsx index 9a72f9d6417..3b8b6efe941 100644 --- a/specifyweb/frontend/js_src/lib/components/Statistics/hooks.tsx +++ b/specifyweb/frontend/js_src/lib/components/Statistics/hooks.tsx @@ -121,7 +121,7 @@ function dynamicEphermeralPromiseGenerator( ), limit: 0, }), - expectedErrors: [Http.FORBIDDEN], + expectedErrors: Object.values(Http), }).then((response) => response?.status === Http.OK ? filterArray( From 415333386726f450f59310a1dacbfbe31e5d6953 Mon Sep 17 00:00:00 2001 From: Caroline Denis Date: Tue, 27 Oct 2026 09:42:02 +0100 Subject: [PATCH 08/47] fix: use a private retry-delay header --- specifyweb/backend/stats/coordination.py | 2 +- specifyweb/backend/stats/tests.py | 4 ++-- .../lib/components/Statistics/__tests__/requests.test.ts | 7 +++++-- .../frontend/js_src/lib/components/Statistics/requests.ts | 4 +++- 4 files changed, 11 insertions(+), 6 deletions(-) diff --git a/specifyweb/backend/stats/coordination.py b/specifyweb/backend/stats/coordination.py index 5acf4c415d5..9c41e5ce78d 100644 --- a/specifyweb/backend/stats/coordination.py +++ b/specifyweb/backend/stats/coordination.py @@ -27,7 +27,7 @@ def busy_response() -> JsonResponse: {"message": "Statistics are already being calculated. Please retry."}, status=409, ) - response["Retry-After"] = str(STATISTICS_RETRY_SECONDS) + response["X-Specify-Retry-After"] = str(STATISTICS_RETRY_SECONDS) return response diff --git a/specifyweb/backend/stats/tests.py b/specifyweb/backend/stats/tests.py index c2983a2afa0..0ee1b1c6213 100644 --- a/specifyweb/backend/stats/tests.py +++ b/specifyweb/backend/stats/tests.py @@ -127,7 +127,7 @@ def test_busy_database_does_not_start_or_wait_for_another_calculation(self): self.lock.release.assert_not_called() response = busy_response() self.assertEqual(response.status_code, 409) - self.assertEqual(response["Retry-After"], "2") + self.assertEqual(response["X-Specify-Retry-After"], "2") self.assertIn("message", json.loads(response.content)) def test_lock_connection_errors_are_not_reported_as_busy(self): @@ -256,7 +256,7 @@ def test_ephemeral_busy_response_does_not_execute_query(self, get_query, execute self.lock.acquire.side_effect = TimeoutError response = ephemeral(self.ephemeral_request()) self.assertEqual(response.status_code, 409) - self.assertEqual(response["Retry-After"], "2") + self.assertEqual(response["X-Specify-Retry-After"], "2") execute.assert_not_called() @patch("specifyweb.backend.stored_queries.views.run_ephemeral_query") diff --git a/specifyweb/frontend/js_src/lib/components/Statistics/__tests__/requests.test.ts b/specifyweb/frontend/js_src/lib/components/Statistics/__tests__/requests.test.ts index 634dded2a13..3c0c5eb2a30 100644 --- a/specifyweb/frontend/js_src/lib/components/Statistics/__tests__/requests.test.ts +++ b/specifyweb/frontend/js_src/lib/components/Statistics/__tests__/requests.test.ts @@ -17,7 +17,10 @@ const result = (status: number, retryAfter?: string) => ({ status, data: { count: 12 }, response: Object.assign(new Response(undefined, { status }), { - headers: { get: () => retryAfter ?? null }, + headers: { + get: (name: string) => + name === 'X-Specify-Retry-After' ? (retryAfter ?? null) : null, + }, }), }); @@ -59,7 +62,7 @@ test('marks statistics requests and tracks loading through completion', async () expect(loading.current).toBe(false); }); -test('retries busy responses using Retry-After while remaining loading', async () => { +test('retries busy responses using the Specify retry header while remaining loading', async () => { ajaxMock .mockResolvedValueOnce(result(Http.CONFLICT, '3')) .mockResolvedValueOnce(result(Http.OK)); diff --git a/specifyweb/frontend/js_src/lib/components/Statistics/requests.ts b/specifyweb/frontend/js_src/lib/components/Statistics/requests.ts index b80eac5c6fa..475440ee144 100644 --- a/specifyweb/frontend/js_src/lib/components/Statistics/requests.ts +++ b/specifyweb/frontend/js_src/lib/components/Statistics/requests.ts @@ -52,7 +52,9 @@ export async function statisticsAjax( }); if (requestGeneration !== generation) return undefined; if (result.status !== Http.CONFLICT) return result; - const retryAfter = Number(result.response.headers.get('Retry-After')); + const retryAfter = Number( + result.response.headers.get('X-Specify-Retry-After') + ); await new Promise((resolve) => globalThis.setTimeout( resolve, From f8c9cdf2036afc79b139d6073b8224a5b2766a12 Mon Sep 17 00:00:00 2001 From: Caroline Denis Date: Tue, 27 Oct 2026 09:56:51 +0100 Subject: [PATCH 09/47] Fix: release calculation locks on every exit --- specifyweb/backend/stats/coordination.py | 20 +++++------ specifyweb/backend/stats/tests.py | 35 ++++++++++++++++--- .../specify/models_utils/lock_tables.py | 19 +++++++--- 3 files changed, 55 insertions(+), 19 deletions(-) diff --git a/specifyweb/backend/stats/coordination.py b/specifyweb/backend/stats/coordination.py index 9c41e5ce78d..18195eaeaf3 100644 --- a/specifyweb/backend/stats/coordination.py +++ b/specifyweb/backend/stats/coordination.py @@ -10,7 +10,7 @@ from specifyweb.backend.cache.redis.utils import format_key, redis_connection from specifyweb.backend.permissions.permissions import check_table_permissions -from specifyweb.specify.models_utils.lock_tables import Lock +from specifyweb.specify.models_utils.lock_tables import LockDispatcher logger = logging.getLogger(__name__) @@ -69,14 +69,16 @@ def cached_result() -> str | None: # A database advisory lock has no lease that could expire during a slow query. lock_name = hashlib.sha256(settings.DATABASE_NAME.encode()).hexdigest()[:48] - lock = Lock(f"statistics:{lock_name}", timeout=0) - try: - lock.acquire() - except TimeoutError as error: - logger.debug("Deferring statistics calculation: %s", error) - raise StatisticsBusy from error + # Keep the bounded lock name while letting the dispatcher own cleanup. + with LockDispatcher( + lock_namespace="statistics", lock_name_separator=":" + ) as locks: + try: + locks.acquire(lock_name, timeout=0) + except TimeoutError as error: + logger.debug("Deferring statistics calculation: %s", error) + raise StatisticsBusy from error - try: result = cached_result() if result is None: result = calculate() @@ -86,8 +88,6 @@ def cached_result() -> str | None: except RedisError: logger.warning("Statistics cache write failed; returning uncached result.", exc_info=True) return result - finally: - lock.release() def statistics_response(*tables): diff --git a/specifyweb/backend/stats/tests.py b/specifyweb/backend/stats/tests.py index 0ee1b1c6213..8aa181ac705 100644 --- a/specifyweb/backend/stats/tests.py +++ b/specifyweb/backend/stats/tests.py @@ -1,3 +1,4 @@ +import hashlib import json from types import SimpleNamespace from unittest.mock import Mock, patch @@ -29,7 +30,7 @@ def setUp(self): "specifyweb.backend.stats.coordination.redis_connection", return_value=self.cache, ) - lock_patch = patch("specifyweb.backend.stats.coordination.Lock") + lock_patch = patch("specifyweb.specify.models_utils.lock_tables.Lock") self.redis_connection = cache_patch.start() self.lock_class = lock_patch.start() self.addCleanup(cache_patch.stop) @@ -46,8 +47,13 @@ def test_result_is_cached_for_five_minutes(self): self.lock.acquire.assert_called_once_with() self.lock.release.assert_called_once_with() self.calculate.assert_called_once_with() - self.assertEqual(self.lock_class.call_args.kwargs, {"timeout": 0}) - self.assertLessEqual(len(self.lock_class.call_args.args[0]), 64) + expected_lock_name = ( + "statistics:" + + hashlib.sha256("statistics_test".encode()).hexdigest()[:48] + ) + self.assertEqual(self.lock_class.call_args.args[0], expected_lock_name) + self.assertLessEqual(len(expected_lock_name), 64) + self.assertEqual(self.lock_class.call_args.args[1], 0) self.cache.set.assert_called_once_with( self.cache.get.call_args.args[0], '{"count": 12}', @@ -55,6 +61,18 @@ def test_result_is_cached_for_five_minutes(self): ) self.assertEqual(STATISTICS_CACHE_SECONDS, 300) + def test_long_database_name_uses_bounded_lock_name(self): + database_name = "d" * 64 + with override_settings(DATABASE_NAME=database_name): + self.run_statistic() + + lock_name = self.lock_class.call_args.args[0] + self.assertEqual( + lock_name, + "statistics:" + hashlib.sha256(database_name.encode()).hexdigest()[:48], + ) + self.assertLessEqual(len(lock_name), 64) + def assert_uncached_calculation(self): self.assertEqual(self.run_statistic(), '{"count": 12}') self.calculate.assert_called_once_with() @@ -102,7 +120,7 @@ def test_unavailable_cache_does_not_bypass_busy_database_lock(self): with self.assertRaises(StatisticsBusy): self.run_statistic() self.calculate.assert_not_called() - self.lock.release.assert_not_called() + self.lock.release.assert_called_once_with() def test_cached_result_rechecks_permissions_without_running_query(self): self.cache.get.return_value = '{"count": 12}' @@ -124,7 +142,7 @@ def test_busy_database_does_not_start_or_wait_for_another_calculation(self): self.run_statistic() self.calculate.assert_not_called() self.cache.set.assert_not_called() - self.lock.release.assert_not_called() + self.lock.release.assert_called_once_with() response = busy_response() self.assertEqual(response.status_code, 409) self.assertEqual(response["X-Specify-Retry-After"], "2") @@ -134,6 +152,13 @@ def test_lock_connection_errors_are_not_reported_as_busy(self): self.lock.acquire.side_effect = ConnectionError with self.assertRaises(ConnectionError): self.run_statistic() + self.lock.release.assert_called_once_with() + + def test_interrupted_lock_acquisition_releases_lock(self): + self.lock.acquire.side_effect = KeyboardInterrupt + with self.assertRaises(KeyboardInterrupt): + self.run_statistic() + self.lock.release.assert_called_once_with() def test_calculation_errors_release_lock_without_caching(self): self.calculate.side_effect = ValueError("calculation failed") diff --git a/specifyweb/specify/models_utils/lock_tables.py b/specifyweb/specify/models_utils/lock_tables.py index ef6762b0c17..b5446ab546c 100644 --- a/specifyweb/specify/models_utils/lock_tables.py +++ b/specifyweb/specify/models_utils/lock_tables.py @@ -200,14 +200,24 @@ def __eq__(self, other): class LockDispatcher: - def __init__(self, lock_prefix: str | None = None, case_sensitive_names=False): + def __init__( + self, + lock_prefix: str | None = None, + case_sensitive_names=False, + *, + lock_namespace: str | None = None, + lock_name_separator: str = LOCK_NAME_SEPARATOR, + ): db_name = getattr(settings, "DATABASE_NAME") - self.lock_prefix_parts: list[str] = [db_name] + self.lock_prefix_parts: list[str] = [ + db_name if lock_namespace is None else lock_namespace + ] if lock_prefix is not None: self.lock_prefix_parts.append(lock_prefix) self.case_sensitive_names = case_sensitive_names + self.lock_name_separator = lock_name_separator self.locks: dict[str, Lock] = dict() self.in_context = False @@ -223,8 +233,9 @@ def __exit__(self, exc_type, exc_val, exc_tb): self.in_context = False def lock_name(self, *name_parts: str): - final_name = LOCK_NAME_SEPARATOR.join( - (*self.lock_prefix_parts, *name_parts)) + final_name = self.lock_name_separator.join( + (*self.lock_prefix_parts, *name_parts) + ) return final_name.lower() if not self.case_sensitive_names else final_name From 3499e61ee85a7947bb4316e3fd7c52666590387e Mon Sep 17 00:00:00 2001 From: Caroline Denis Date: Tue, 27 Oct 2026 10:03:58 +0100 Subject: [PATCH 10/47] Refactor(statistics): let views build cached responses --- specifyweb/backend/stats/coordination.py | 44 +++++++----------- specifyweb/backend/stats/tests.py | 26 ++++++----- specifyweb/backend/stats/views.py | 59 ++++++++++++++++++++---- 3 files changed, 81 insertions(+), 48 deletions(-) diff --git a/specifyweb/backend/stats/coordination.py b/specifyweb/backend/stats/coordination.py index 18195eaeaf3..5616bc18991 100644 --- a/specifyweb/backend/stats/coordination.py +++ b/specifyweb/backend/stats/coordination.py @@ -2,10 +2,9 @@ import json import logging from collections.abc import Callable -from functools import wraps from django.conf import settings -from django.http import HttpResponse, JsonResponse +from django.http import JsonResponse from redis.exceptions import RedisError from specifyweb.backend.cache.redis.utils import format_key, redis_connection @@ -90,29 +89,18 @@ def cached_result() -> str | None: return result -def statistics_response(*tables): - def decorate(view): - @wraps(view) - def wrapped(request, *args, **kwargs): - def validate() -> None: - for table in tables: - check_table_permissions( - request.specify_collection, request.specify_user, table, "read" - ) - - validate() - try: - content = statistics_result( - request.specify_collection, - request.specify_user, - json.dumps([request.path, sorted(request.GET.items())]), - lambda: view(request, *args, **kwargs).content.decode(), - validate, - ) - except StatisticsBusy: - return busy_response() - return HttpResponse(content, content_type="application/json") - - return wrapped - - return decorate +def statistics_content(request, tables, calculate: Callable[[], str]) -> str: + def validate() -> None: + for table in tables: + check_table_permissions( + request.specify_collection, request.specify_user, table, "read" + ) + + validate() + return statistics_result( + request.specify_collection, + request.specify_user, + json.dumps([request.path, sorted(request.GET.items())]), + calculate, + validate, + ) diff --git a/specifyweb/backend/stats/tests.py b/specifyweb/backend/stats/tests.py index 8aa181ac705..bf69bdbf052 100644 --- a/specifyweb/backend/stats/tests.py +++ b/specifyweb/backend/stats/tests.py @@ -12,7 +12,7 @@ STATISTICS_CACHE_SECONDS, StatisticsBusy, busy_response, - statistics_response, + statistics_content, statistics_result, ) @@ -212,24 +212,28 @@ def test_backend_view_rechecks_permissions_on_cache_hits(self, check): request.specify_collection = self.collection request.specify_user = self.user view = Mock(return_value=JsonResponse({"lots": 12})) - wrapped = statistics_response(table)(view) self.cache.get.return_value = '{"lots": 12}' - response = wrapped(request) - self.assertEqual(json.loads(response.content), {"lots": 12}) + content = statistics_content( + request, (table,), lambda: view(request).content.decode() + ) + self.assertEqual(json.loads(content), {"lots": 12}) check.assert_called_with(self.collection, self.user, table, "read") view.assert_not_called() - @patch("specifyweb.backend.stats.coordination.check_table_permissions") - def test_backend_view_returns_busy_response(self, check): + @patch( + "specifyweb.backend.stats.views.statistics_content", + side_effect=StatisticsBusy, + ) + def test_backend_view_returns_busy_response(self, statistics_content_mock): + from specifyweb.backend.stats.views import collection_preparations + request = RequestFactory().get("/stats/collection/preparations/") + request.user = SimpleNamespace(is_authenticated=True) request.specify_collection = self.collection request.specify_user = self.user - self.lock.acquire.side_effect = TimeoutError - view = Mock() - response = statistics_response(Mock())(view)(request) + response = collection_preparations(request) self.assertEqual(response.status_code, 409) - check.assert_called_once() - view.assert_not_called() + statistics_content_mock.assert_called_once() def ephemeral_request(self, statistics=True): request = RequestFactory().post( diff --git a/specifyweb/backend/stats/views.py b/specifyweb/backend/stats/views.py index bc4e5e69077..c034560b573 100644 --- a/specifyweb/backend/stats/views.py +++ b/specifyweb/backend/stats/views.py @@ -1,19 +1,15 @@ from django import http - from specifyweb.backend.permissions.permissions import check_table_permissions -from specifyweb.specify.views import openapi from django.http import HttpResponse, Http404 - from specifyweb.specify.models import Preparation, Determination, Discipline, Locality, Collectionobject, Collectionobjectattachment, Attachment -from specifyweb.specify.views import login_maybe_required +from specifyweb.specify.views import login_maybe_required, openapi import logging logger = logging.getLogger(__name__) from django.db import connection -from .coordination import statistics_response +from .coordination import StatisticsBusy, busy_response, statistics_content @login_maybe_required -@statistics_response(Preparation) @openapi(schema={ 'get': { 'responses': { @@ -31,6 +27,18 @@ } }}, ) def collection_preparations(request) -> HttpResponse: + try: + content = statistics_content( + request, + (Preparation,), + lambda: _collection_preparations(request).content.decode(), + ) + except StatisticsBusy: + return busy_response() + return HttpResponse(content, content_type="application/json") + + +def _collection_preparations(request) -> HttpResponse: check_table_permissions(request.specify_collection, request.specify_user, Preparation, "read") cursor = connection.cursor() # PREP_BY_TYPE_LOTS @@ -55,7 +63,6 @@ def collection_preparations(request) -> HttpResponse: @login_maybe_required -@statistics_response(Discipline, Locality) @openapi(schema={ 'get': { 'responses': { @@ -77,6 +84,18 @@ def collection_preparations(request) -> HttpResponse: } }}, ) def collection_locality_geography(request, stat) -> HttpResponse: + try: + content = statistics_content( + request, + (Discipline, Locality), + lambda: _collection_locality_geography(request, stat).content.decode(), + ) + except StatisticsBusy: + return busy_response() + return HttpResponse(content, content_type="application/json") + + +def _collection_locality_geography(request, stat) -> HttpResponse: geography_dict = {} if stat == 'percentGeoReferenced': geography_dict[stat] = get_percent_georeferenced(request) @@ -85,7 +104,6 @@ def collection_locality_geography(request, stat) -> HttpResponse: return http.JsonResponse(geography_dict) @login_maybe_required -@statistics_response(Determination) @openapi(schema={ 'get': { 'responses': { @@ -103,6 +121,18 @@ def collection_locality_geography(request, stat) -> HttpResponse: } }}, ) def collection_type_specimens(request) -> HttpResponse: + try: + content = statistics_content( + request, + (Determination,), + lambda: _collection_type_specimens(request).content.decode(), + ) + except StatisticsBusy: + return busy_response() + return HttpResponse(content, content_type="application/json") + + +def _collection_type_specimens(request) -> HttpResponse: check_table_permissions(request.specify_collection, request.specify_user, Determination, "read") cursor = connection.cursor() @@ -149,7 +179,6 @@ def get_percent_georeferenced(request): return percent_georeferenced @login_maybe_required -@statistics_response(Collectionobject, Collectionobjectattachment, Attachment) @openapi(schema={ 'get': { 'responses': { @@ -167,6 +196,18 @@ def get_percent_georeferenced(request): } }}, ) def collection_attachments(request, stat)->HttpResponse: + try: + content = statistics_content( + request, + (Collectionobject, Collectionobjectattachment, Attachment), + lambda: _collection_attachments(request, stat).content.decode(), + ) + except StatisticsBusy: + return busy_response() + return HttpResponse(content, content_type="application/json") + + +def _collection_attachments(request, stat)->HttpResponse: attachments_dict = {} if stat == 'percentCoImaged': attachments_dict[stat] = get_percent_imaged(request) From 9852b926d42c4e0ef466b925b9e8a9ec05d4c578 Mon Sep 17 00:00:00 2001 From: Caroline Denis Date: Tue, 27 Oct 2026 10:19:19 +0100 Subject: [PATCH 11/47] Refactor(query): separate cache permission checks from query execution --- specifyweb/backend/stats/coordination.py | 19 +- specifyweb/backend/stats/tests.py | 176 ++++++++++++++++-- .../backend/stored_queries/execution.py | 38 ++-- .../backend/stored_queries/query_construct.py | 29 +-- specifyweb/backend/stored_queries/views.py | 12 +- 5 files changed, 213 insertions(+), 61 deletions(-) diff --git a/specifyweb/backend/stats/coordination.py b/specifyweb/backend/stats/coordination.py index 5616bc18991..a054abf400b 100644 --- a/specifyweb/backend/stats/coordination.py +++ b/specifyweb/backend/stats/coordination.py @@ -35,9 +35,16 @@ def statistics_result( user, query_key: str, calculate: Callable[[], str], - validate_cached: Callable[[], None], + check_cached_permissions: Callable[[], None], ) -> str: - """Reuse cached statistics when Redis is available; always gate calculations.""" + """Reuse cached statistics when Redis is available; always gate calculations. + + Before returning a cache hit, check_cached_permissions must raise if the user + can no longer access the result. It checks authorization, not query validity. + On a cache miss, calculate is responsible for normal validation and permissions. + Exceptions propagate to the caller: failed calculations are not cached, and + failed permission checks leave the cached value unchanged. + """ digest = hashlib.sha256(query_key.encode()).hexdigest() cache_key = format_key( f"specify:{{database}}:statistics:{collection.id}:{user.id}:{digest}" @@ -59,7 +66,7 @@ def cached_result() -> str | None: cache = None return None if result is not None: - validate_cached() + check_cached_permissions() return result result = cached_result() @@ -90,17 +97,17 @@ def cached_result() -> str | None: def statistics_content(request, tables, calculate: Callable[[], str]) -> str: - def validate() -> None: + def check_permissions() -> None: for table in tables: check_table_permissions( request.specify_collection, request.specify_user, table, "read" ) - validate() + check_permissions() return statistics_result( request.specify_collection, request.specify_user, json.dumps([request.path, sorted(request.GET.items())]), calculate, - validate, + check_permissions, ) diff --git a/specifyweb/backend/stats/tests.py b/specifyweb/backend/stats/tests.py index bf69bdbf052..b438d44d076 100644 --- a/specifyweb/backend/stats/tests.py +++ b/specifyweb/backend/stats/tests.py @@ -1,13 +1,15 @@ import hashlib import json from types import SimpleNamespace -from unittest.mock import Mock, patch +from unittest.mock import Mock, call, patch from django.core.exceptions import PermissionDenied from django.http import JsonResponse from django.test import RequestFactory, SimpleTestCase, override_settings from redis.exceptions import ConnectionError as RedisConnectionError, TimeoutError as RedisTimeoutError +from specifyweb.specify.datamodel import datamodel + from .coordination import ( STATISTICS_CACHE_SECONDS, StatisticsBusy, @@ -23,7 +25,7 @@ def setUp(self): self.collection = SimpleNamespace(id=1) self.user = SimpleNamespace(id=2) self.calculate = Mock(return_value='{"count": 12}') - self.validate = Mock() + self.check_permissions = Mock() self.cache = Mock() self.cache.get.return_value = None cache_patch = patch( @@ -39,7 +41,7 @@ def setUp(self): def run_statistic(self, query_key="query"): return statistics_result( - self.collection, self.user, query_key, self.calculate, self.validate + self.collection, self.user, query_key, self.calculate, self.check_permissions ) def test_result_is_cached_for_five_minutes(self): @@ -78,7 +80,7 @@ def assert_uncached_calculation(self): self.calculate.assert_called_once_with() self.lock.acquire.assert_called_once_with() self.lock.release.assert_called_once_with() - self.validate.assert_not_called() + self.check_permissions.assert_not_called() def test_unconfigured_redis_still_calculates_under_lock(self): self.redis_connection.side_effect = ValueError("Redis is not correctly configured") @@ -125,16 +127,18 @@ def test_unavailable_cache_does_not_bypass_busy_database_lock(self): def test_cached_result_rechecks_permissions_without_running_query(self): self.cache.get.return_value = '{"count": 12}' self.assertEqual(self.run_statistic(), '{"count": 12}') - self.validate.assert_called_once_with() + self.check_permissions.assert_called_once_with() self.calculate.assert_not_called() self.lock.acquire.assert_not_called() def test_revoked_permissions_prevent_cached_result_access(self): self.cache.get.return_value = '{"count": 12}' - self.validate.side_effect = PermissionDenied + self.check_permissions.side_effect = PermissionDenied with self.assertRaises(PermissionDenied): self.run_statistic() self.calculate.assert_not_called() + self.cache.set.assert_not_called() + self.cache.delete.assert_not_called() def test_busy_database_does_not_start_or_wait_for_another_calculation(self): self.lock.acquire.side_effect = TimeoutError @@ -176,7 +180,7 @@ def test_query_timeouts_are_not_mistaken_for_lock_contention(self): def test_cache_is_checked_again_after_acquiring_lock(self): self.cache.get.side_effect = [None, '{"count": 12}'] self.assertEqual(self.run_statistic(), '{"count": 12}') - self.validate.assert_called_once_with() + self.check_permissions.assert_called_once_with() self.calculate.assert_not_called() self.lock.release.assert_called_once_with() @@ -261,20 +265,61 @@ def test_ephemeral_statistics_are_gated_and_cached(self, get_query, execute): self.lock.acquire.assert_called_once_with() self.cache.set.assert_called_once() + @patch("specifyweb.backend.stored_queries.execution.check_table_permissions") @patch("specifyweb.backend.stored_queries.views.run_ephemeral_query") @patch("specifyweb.backend.stored_queries.views.get_query") - def test_ephemeral_cache_hit_validates_query_without_executing(self, get_query, execute): + def test_ephemeral_cache_hit_checks_permissions_without_executing( + self, get_query, execute, check + ): from specifyweb.backend.stored_queries.views import ephemeral - query = {"countonly": True} + query = { + "countonly": True, + "fields": [{ + "position": 0, + "stringid": "1.collectionobject.catalogNumber", + "isrelfld": False, + }], + } get_query.return_value = query, self.collection self.cache.get.return_value = '{"count": 12}' response = ephemeral(self.ephemeral_request()) self.assertEqual(json.loads(response.content), {"count": 12}) - execute.assert_called_once_with( - self.collection, self.user, query, validate_only=True + check.assert_called_once_with( + self.collection, self.user, datamodel.get_table_strict("Collectionobject"), "read" ) + execute.assert_not_called() + self.lock.acquire.assert_not_called() + self.cache.set.assert_not_called() + + @patch( + "specifyweb.backend.stored_queries.execution.check_table_permissions", + side_effect=PermissionDenied, + ) + @patch("specifyweb.backend.stored_queries.views.run_ephemeral_query") + @patch("specifyweb.backend.stored_queries.views.get_query") + def test_ephemeral_cache_hit_rejects_revoked_permissions( + self, get_query, execute, check + ): + from specifyweb.backend.stored_queries.views import ephemeral + + query = { + "countonly": True, + "fields": [{ + "position": 0, + "stringid": "1.collectionobject.catalogNumber", + "isrelfld": False, + }], + } + get_query.return_value = query, self.collection + self.cache.get.return_value = '{"count": 12}' + with self.assertRaises(PermissionDenied): + ephemeral(self.ephemeral_request()) + check.assert_called_once() + execute.assert_not_called() self.lock.acquire.assert_not_called() + self.cache.set.assert_not_called() + self.cache.delete.assert_not_called() @patch("specifyweb.backend.stored_queries.views.run_ephemeral_query") @patch("specifyweb.backend.stored_queries.views.get_query") @@ -303,13 +348,113 @@ def test_ordinary_ephemeral_queries_are_unchanged(self, get_query, execute): self.lock.acquire.assert_not_called() -class StatisticsPermissionValidationTests(SimpleTestCase): +class StatisticsQueryPermissionTests(SimpleTestCase): + @patch("specifyweb.backend.stored_queries.execution.build_query") + @patch("specifyweb.backend.stored_queries.execution.check_table_permissions") + def test_checks_root_and_joined_tables_once_without_building_query(self, check, build): + from specifyweb.backend.stored_queries.execution import check_query_permissions + from specifyweb.backend.stored_queries.queryfield import fields_from_json + + collection, user = Mock(), Mock() + fields = fields_from_json([ + { + "position": 0, + "stringid": "1,10,2.locality.localityName", + "isrelfld": False, + }, + { + "position": 1, + "stringid": "1.collectionobject.catalogNumber", + "isrelfld": False, + }, + ]) + check_query_permissions(collection, user, fields) + self.assertCountEqual( + check.call_args_list, + [ + call(collection, user, datamodel.get_table_strict(name), "read") + for name in ("Collectionobject", "Collectingevent", "Locality") + ], + ) + build.assert_not_called() + + @patch("specifyweb.backend.stored_queries.execution.check_table_permissions") + def test_tree_rank_paths_check_referenced_tables(self, check): + from specifyweb.backend.stored_queries.execution import check_query_permissions + from specifyweb.backend.stored_queries.queryfield import fields_from_json + + collection, user = Mock(), Mock() + fields = fields_from_json([{ + "position": 0, + "stringid": "1,9-determinations,4.taxon.Genus", + "isrelfld": False, + }]) + check_query_permissions(collection, user, fields) + self.assertCountEqual( + check.call_args_list, + [ + call(collection, user, datamodel.get_table_strict(name), "read") + for name in ("Collectionobject", "Determination", "Taxon") + ], + ) + + def test_table_paths_accept_field_names(self): + from specifyweb.backend.stored_queries.query_construct import tables_in_path + + self.assertEqual( + tables_in_path( + datamodel.get_table_strict("Collectionobject"), + ("collectingEvent", "locality", "localityName"), + ), + [ + datamodel.get_table_strict(name) + for name in ("Collectionobject", "Collectingevent", "Locality") + ], + ) + + @patch("specifyweb.backend.stored_queries.execution.models.session_context") + @patch("specifyweb.backend.stored_queries.execution.execute") + def test_ephemeral_execution_returns_query_results(self, execute, context): + from specifyweb.backend.stored_queries.execution import run_ephemeral_query + from specifyweb.backend.stored_queries.queryfield import fields_from_json + + collection, user = Mock(), Mock() + query = { + "selectdistinct": False, + "contexttableid": 1, + "countonly": True, + "fields": [{ + "position": 0, + "stringid": "1.collectionobject.catalogNumber", + "isrelfld": False, + }], + } + execute.return_value = {"count": 12} + self.assertEqual(run_ephemeral_query(collection, user, query), {"count": 12}) + execute.assert_called_once_with( + session=context.return_value.__enter__.return_value, + collection=collection, + user=user, + tableid=1, + distinct=False, + series=None, + search_synonymy=False, + count_only=True, + query_fields=fields_from_json(query["fields"]), + limit=20, + offset=0, + recordsetid=None, + formatauditobjs=False, + ) + + @patch("specifyweb.backend.stored_queries.execution.log_sqlalchemy_query") @patch("specifyweb.backend.stored_queries.execution.set_group_concat_max_len") @patch("specifyweb.backend.stored_queries.execution.build_query") - def test_validation_builds_and_checks_query_without_evaluating_it(self, build, setup): + def test_execute_returns_query_results(self, build, setup, log): from specifyweb.backend.stored_queries.execution import execute query = Mock() + query.count.return_value = 12 build.return_value = query, [] result = execute( session=SimpleNamespace(info={"connection": Mock()}), @@ -323,10 +468,9 @@ def test_validation_builds_and_checks_query_without_evaluating_it(self, build, s query_fields=[], limit=0, offset=0, - validate_only=True, ) - self.assertIsNone(result) + self.assertEqual(result, {"count": 12}) setup.assert_called_once() build.assert_called_once() - query.count.assert_not_called() + query.count.assert_called_once_with() query.all.assert_not_called() diff --git a/specifyweb/backend/stored_queries/execution.py b/specifyweb/backend/stored_queries/execution.py index 1048bdb6b70..e6f16ba6622 100644 --- a/specifyweb/backend/stored_queries/execution.py +++ b/specifyweb/backend/stored_queries/execution.py @@ -27,7 +27,7 @@ from . import models from .format import ObjectFormatter, ObjectFormatterProps -from .query_construct import QueryConstruct +from .query_construct import QueryConstruct, tables_in_path from .field_spec_maps import transform_field_specs from .web_portal_export import query_to_web_portal_zip as _query_to_web_portal_zip, WebportalQueryResultProcessors from specifyweb.backend.notifications.models import Message @@ -677,10 +677,24 @@ def createPlacemark(kmlDoc, row, coord_cols, table, captions, host): return placemarkElement -def run_ephemeral_query(collection, user, spquery, *, validate_only: bool = False): +def check_query_permissions(collection, user, query_fields: Iterable[QueryField]) -> None: + """Check read permissions for query field paths without building the data query. + + Permission failures propagate from check_table_permissions. This checks current + authorization, not query validity. + """ + tables_to_read = { + table + for field in query_fields + for table in tables_in_path(field.fieldspec.root_table, field.fieldspec.join_path) + } + for table_to_read in tables_to_read: + check_table_permissions(collection, user, table_to_read, "read") + + +def run_ephemeral_query(collection, user, spquery): """Execute a Specify query from deserialized json and return the results - as an array for json serialization to the web app. With validate_only, - build the query and check permissions without evaluating its results. + as an array for json serialization to the web app. """ logger.info("ephemeral query: %s", spquery) limit = spquery.get("limit", 20) @@ -709,7 +723,6 @@ def run_ephemeral_query(collection, user, spquery, *, validate_only: bool = Fals offset=offset, recordsetid=recordsetid, formatauditobjs=format_audits, - validate_only=validate_only, ) @@ -874,7 +887,6 @@ def execute( recordsetid=None, formatauditobjs=False, formatter_props=None, - validate_only: bool = False, ): "Build and execute a query, returning the results as a data structure for json serialization" @@ -898,9 +910,6 @@ def execute( ), ) - if validate_only: - return None - log_sqlalchemy_query(query) if count_only: @@ -1182,16 +1191,7 @@ def build_query( query_fields = list(transform_field_specs(query_fields, user)) - tables_to_read = { - table - for field in query_fields - for table in query.tables_in_path( - field.fieldspec.root_table, field.fieldspec.join_path - ) - } - - for table_to_read in tables_to_read: - check_table_permissions(collection, user, table_to_read, "read") + check_query_permissions(collection, user, query_fields) query = filter_by_collection(model, query, collection) diff --git a/specifyweb/backend/stored_queries/query_construct.py b/specifyweb/backend/stored_queries/query_construct.py index 569e3fc0466..47dace5955f 100644 --- a/specifyweb/backend/stored_queries/query_construct.py +++ b/specifyweb/backend/stored_queries/query_construct.py @@ -17,6 +17,21 @@ def _safe_filter(query): return query.first() raise Exception(f"Got more than one matching: {list(query)}") + +def tables_in_path(table, join_path): + path = deque(join_path) + tables = [table] + while len(path) > 0: + field = path.popleft() + if isinstance(field, str): + field = tables[-1].get_field(field, strict=True) + if not field.is_relationship: # also handles tree ranks + break + + tables.append(spmodels.datamodel.get_table(field.relatedModelName, strict=True)) + return tables + + class QueryConstruct(namedtuple('QueryConstruct', 'collection objectformatter query join_cache tree_rank_count internal_filters')): def __new__(cls, *args, **kwargs): @@ -112,20 +127,6 @@ def make_tree_field_spec(tree_node): return query, column, field, table - def tables_in_path(self, table, join_path): - path = deque(join_path) - field = None - tables = [table] - while len(path) > 0: - field = path.popleft() - if isinstance(field, str): - field = tables[-1].get_field(field, strict=True) - if not field.is_relationship: # also handles tree ranks - break - - tables.append(spmodels.datamodel.get_table(field.relatedModelName, strict=True)) - return tables - def build_join(self, table, model, join_path): query = self path = deque(join_path) diff --git a/specifyweb/backend/stored_queries/views.py b/specifyweb/backend/stored_queries/views.py index d70ba9abe1e..e9e4ace299a 100644 --- a/specifyweb/backend/stored_queries/views.py +++ b/specifyweb/backend/stored_queries/views.py @@ -20,14 +20,14 @@ from specifyweb.specify.api.serializers import toJson, uri_for_model from . import models from .execution import execute, run_ephemeral_query, do_export, recordset -from .queryfield import QueryField +from .queryfield import QueryField, fields_from_json from specifyweb.backend.permissions.permissions import PermissionTarget, PermissionTargetAction, \ check_permission_targets, check_table_permissions from specifyweb.specify.models import Collection, Recordset, Recordsetitem, \ Loanreturnpreparation, Loanpreparation, Loan from specifyweb.specify.views import login_maybe_required from .execution import execute, run_ephemeral_query, do_export, recordset, \ - return_loan_preps as rlp + return_loan_preps as rlp, check_query_permissions logger = logging.getLogger(__name__) @@ -122,9 +122,9 @@ def ephemeral(request): spquery, collection = get_query(request) if request.headers.get("X-Specify-Statistics") == "true": - def validate_cached() -> None: - run_ephemeral_query( - collection, request.specify_user, spquery, validate_only=True + def check_cached_permissions() -> None: + check_query_permissions( + collection, request.specify_user, fields_from_json(spquery["fields"]) ) try: @@ -133,7 +133,7 @@ def validate_cached() -> None: request.specify_user, json.dumps(spquery, sort_keys=True), lambda: toJson(run_ephemeral_query(collection, request.specify_user, spquery)), - validate_cached, + check_cached_permissions, ) except StatisticsBusy: return busy_response() From a80af042ab5c83b7bb0f3618ca1e002f7778a9bb Mon Sep 17 00:00:00 2001 From: Caroline Denis Date: Tue, 27 Oct 2026 10:25:35 +0100 Subject: [PATCH 12/47] refactor(stats): validate permissions before cache lookup --- specifyweb/backend/stats/coordination.py | 26 +++---- specifyweb/backend/stats/tests.py | 88 +++++++++++++--------- specifyweb/backend/stored_queries/views.py | 7 +- 3 files changed, 64 insertions(+), 57 deletions(-) diff --git a/specifyweb/backend/stats/coordination.py b/specifyweb/backend/stats/coordination.py index a054abf400b..7f6049900ad 100644 --- a/specifyweb/backend/stats/coordination.py +++ b/specifyweb/backend/stats/coordination.py @@ -35,15 +35,13 @@ def statistics_result( user, query_key: str, calculate: Callable[[], str], - check_cached_permissions: Callable[[], None], ) -> str: - """Reuse cached statistics when Redis is available; always gate calculations. + """Reuse cached statistics without handling query validity. - Before returning a cache hit, check_cached_permissions must raise if the user - can no longer access the result. It checks authorization, not query validity. - On a cache miss, calculate is responsible for normal validation and permissions. - Exceptions propagate to the caller: failed calculations are not cached, and - failed permission checks leave the cached value unchanged. + Callers must check current permissions before invoking this function because + a cache hit skips calculate. On a miss, calculate is responsible for normal + query construction and execution; exceptions propagate and failed + calculations are not cached. """ digest = hashlib.sha256(query_key.encode()).hexdigest() cache_key = format_key( @@ -65,8 +63,6 @@ def cached_result() -> str | None: logger.warning("Statistics cache read failed; calculating without caching.", exc_info=True) cache = None return None - if result is not None: - check_cached_permissions() return result result = cached_result() @@ -97,17 +93,13 @@ def cached_result() -> str | None: def statistics_content(request, tables, calculate: Callable[[], str]) -> str: - def check_permissions() -> None: - for table in tables: - check_table_permissions( - request.specify_collection, request.specify_user, table, "read" - ) - - check_permissions() + for table in tables: + check_table_permissions( + request.specify_collection, request.specify_user, table, "read" + ) return statistics_result( request.specify_collection, request.specify_user, json.dumps([request.path, sorted(request.GET.items())]), calculate, - check_permissions, ) diff --git a/specifyweb/backend/stats/tests.py b/specifyweb/backend/stats/tests.py index b438d44d076..64f4148f124 100644 --- a/specifyweb/backend/stats/tests.py +++ b/specifyweb/backend/stats/tests.py @@ -25,7 +25,6 @@ def setUp(self): self.collection = SimpleNamespace(id=1) self.user = SimpleNamespace(id=2) self.calculate = Mock(return_value='{"count": 12}') - self.check_permissions = Mock() self.cache = Mock() self.cache.get.return_value = None cache_patch = patch( @@ -41,7 +40,7 @@ def setUp(self): def run_statistic(self, query_key="query"): return statistics_result( - self.collection, self.user, query_key, self.calculate, self.check_permissions + self.collection, self.user, query_key, self.calculate ) def test_result_is_cached_for_five_minutes(self): @@ -80,7 +79,6 @@ def assert_uncached_calculation(self): self.calculate.assert_called_once_with() self.lock.acquire.assert_called_once_with() self.lock.release.assert_called_once_with() - self.check_permissions.assert_not_called() def test_unconfigured_redis_still_calculates_under_lock(self): self.redis_connection.side_effect = ValueError("Redis is not correctly configured") @@ -124,22 +122,12 @@ def test_unavailable_cache_does_not_bypass_busy_database_lock(self): self.calculate.assert_not_called() self.lock.release.assert_called_once_with() - def test_cached_result_rechecks_permissions_without_running_query(self): + def test_cached_result_is_returned_without_running_query(self): self.cache.get.return_value = '{"count": 12}' self.assertEqual(self.run_statistic(), '{"count": 12}') - self.check_permissions.assert_called_once_with() self.calculate.assert_not_called() self.lock.acquire.assert_not_called() - def test_revoked_permissions_prevent_cached_result_access(self): - self.cache.get.return_value = '{"count": 12}' - self.check_permissions.side_effect = PermissionDenied - with self.assertRaises(PermissionDenied): - self.run_statistic() - self.calculate.assert_not_called() - self.cache.set.assert_not_called() - self.cache.delete.assert_not_called() - def test_busy_database_does_not_start_or_wait_for_another_calculation(self): self.lock.acquire.side_effect = TimeoutError with self.assertRaises(StatisticsBusy): @@ -180,7 +168,6 @@ def test_query_timeouts_are_not_mistaken_for_lock_contention(self): def test_cache_is_checked_again_after_acquiring_lock(self): self.cache.get.side_effect = [None, '{"count": 12}'] self.assertEqual(self.run_statistic(), '{"count": 12}') - self.check_permissions.assert_called_once_with() self.calculate.assert_not_called() self.lock.release.assert_called_once_with() @@ -224,6 +211,21 @@ def test_backend_view_rechecks_permissions_on_cache_hits(self, check): check.assert_called_with(self.collection, self.user, table, "read") view.assert_not_called() + @patch( + "specifyweb.backend.stats.coordination.check_table_permissions", + side_effect=PermissionDenied, + ) + def test_backend_view_rejects_revoked_permissions_before_cache_lookup(self, check): + table = Mock() + request = RequestFactory().get("/stats/collection/preparations/") + request.specify_collection = self.collection + request.specify_user = self.user + with self.assertRaises(PermissionDenied): + statistics_content(request, (table,), self.calculate) + check.assert_called_once_with(self.collection, self.user, table, "read") + self.cache.get.assert_not_called() + self.calculate.assert_not_called() + @patch( "specifyweb.backend.stats.views.statistics_content", side_effect=StatisticsBusy, @@ -251,17 +253,30 @@ def ephemeral_request(self, statistics=True): request.specify_collection = self.collection return request + @staticmethod + def ephemeral_query(): + return { + "countonly": True, + "fields": [{ + "position": 0, + "stringid": "1.collectionobject.catalogNumber", + "isrelfld": False, + }], + } + + @patch("specifyweb.backend.stored_queries.execution.check_table_permissions") @patch("specifyweb.backend.stored_queries.views.run_ephemeral_query") @patch("specifyweb.backend.stored_queries.views.get_query") - def test_ephemeral_statistics_are_gated_and_cached(self, get_query, execute): + def test_ephemeral_statistics_are_gated_and_cached(self, get_query, execute, check): from specifyweb.backend.stored_queries.views import ephemeral - query = {"countonly": True} + query = self.ephemeral_query() get_query.return_value = query, self.collection execute.return_value = {"count": 12} response = ephemeral(self.ephemeral_request()) self.assertEqual(json.loads(response.content), {"count": 12}) execute.assert_called_once_with(self.collection, self.user, query) + check.assert_called_once() self.lock.acquire.assert_called_once_with() self.cache.set.assert_called_once() @@ -273,14 +288,7 @@ def test_ephemeral_cache_hit_checks_permissions_without_executing( ): from specifyweb.backend.stored_queries.views import ephemeral - query = { - "countonly": True, - "fields": [{ - "position": 0, - "stringid": "1.collectionobject.catalogNumber", - "isrelfld": False, - }], - } + query = self.ephemeral_query() get_query.return_value = query, self.collection self.cache.get.return_value = '{"count": 12}' response = ephemeral(self.ephemeral_request()) @@ -303,30 +311,40 @@ def test_ephemeral_cache_hit_rejects_revoked_permissions( ): from specifyweb.backend.stored_queries.views import ephemeral - query = { - "countonly": True, - "fields": [{ - "position": 0, - "stringid": "1.collectionobject.catalogNumber", - "isrelfld": False, - }], - } + query = self.ephemeral_query() get_query.return_value = query, self.collection self.cache.get.return_value = '{"count": 12}' with self.assertRaises(PermissionDenied): ephemeral(self.ephemeral_request()) check.assert_called_once() execute.assert_not_called() + self.cache.get.assert_not_called() self.lock.acquire.assert_not_called() self.cache.set.assert_not_called() self.cache.delete.assert_not_called() - @patch("specifyweb.backend.stored_queries.views.run_ephemeral_query") + @patch("specifyweb.backend.stored_queries.views.statistics_result") @patch("specifyweb.backend.stored_queries.views.get_query") - def test_ephemeral_busy_response_does_not_execute_query(self, get_query, execute): + def test_malformed_ephemeral_query_is_rejected_before_cache_lookup( + self, get_query, statistics_result_mock + ): from specifyweb.backend.stored_queries.views import ephemeral get_query.return_value = {"countonly": True}, self.collection + with self.assertRaises(KeyError): + ephemeral(self.ephemeral_request()) + statistics_result_mock.assert_not_called() + self.cache.get.assert_not_called() + + @patch("specifyweb.backend.stored_queries.execution.check_table_permissions") + @patch("specifyweb.backend.stored_queries.views.run_ephemeral_query") + @patch("specifyweb.backend.stored_queries.views.get_query") + def test_ephemeral_busy_response_does_not_execute_query( + self, get_query, execute, check + ): + from specifyweb.backend.stored_queries.views import ephemeral + + get_query.return_value = self.ephemeral_query(), self.collection self.lock.acquire.side_effect = TimeoutError response = ephemeral(self.ephemeral_request()) self.assertEqual(response.status_code, 409) diff --git a/specifyweb/backend/stored_queries/views.py b/specifyweb/backend/stored_queries/views.py index e9e4ace299a..5a6b66c7da9 100644 --- a/specifyweb/backend/stored_queries/views.py +++ b/specifyweb/backend/stored_queries/views.py @@ -122,10 +122,8 @@ def ephemeral(request): spquery, collection = get_query(request) if request.headers.get("X-Specify-Statistics") == "true": - def check_cached_permissions() -> None: - check_query_permissions( - collection, request.specify_user, fields_from_json(spquery["fields"]) - ) + query_fields = fields_from_json(spquery["fields"]) + check_query_permissions(collection, request.specify_user, query_fields) try: content = statistics_result( @@ -133,7 +131,6 @@ def check_cached_permissions() -> None: request.specify_user, json.dumps(spquery, sort_keys=True), lambda: toJson(run_ephemeral_query(collection, request.specify_user, spquery)), - check_cached_permissions, ) except StatisticsBusy: return busy_response() From 9362ac72b7c8279c994813ae274e5f224ce29a56 Mon Sep 17 00:00:00 2001 From: Caroline Denis Date: Tue, 27 Oct 2026 13:08:57 +0100 Subject: [PATCH 13/47] Fix: avoid query validation on cache hits --- specifyweb/backend/stats/coordination.py | 6 +- specifyweb/backend/stats/tests.py | 74 +++++++++++++------ .../backend/stored_queries/execution.py | 16 ++-- specifyweb/backend/stored_queries/views.py | 30 ++++++-- 4 files changed, 88 insertions(+), 38 deletions(-) diff --git a/specifyweb/backend/stats/coordination.py b/specifyweb/backend/stats/coordination.py index 7f6049900ad..69c963cd05a 100644 --- a/specifyweb/backend/stats/coordination.py +++ b/specifyweb/backend/stats/coordination.py @@ -38,9 +38,9 @@ def statistics_result( ) -> str: """Reuse cached statistics without handling query validity. - Callers must check current permissions before invoking this function because - a cache hit skips calculate. On a miss, calculate is responsible for normal - query construction and execution; exceptions propagate and failed + Callers must enforce current permissions before returning the result to the + user because a cache hit skips calculate. On a miss, calculate is responsible + for normal query construction and execution; exceptions propagate and failed calculations are not cached. """ digest = hashlib.sha256(query_key.encode()).hexdigest() diff --git a/specifyweb/backend/stats/tests.py b/specifyweb/backend/stats/tests.py index 64f4148f124..1128124d18d 100644 --- a/specifyweb/backend/stats/tests.py +++ b/specifyweb/backend/stats/tests.py @@ -264,7 +264,7 @@ def ephemeral_query(): }], } - @patch("specifyweb.backend.stored_queries.execution.check_table_permissions") + @patch("specifyweb.backend.stored_queries.views.check_table_permissions") @patch("specifyweb.backend.stored_queries.views.run_ephemeral_query") @patch("specifyweb.backend.stored_queries.views.get_query") def test_ephemeral_statistics_are_gated_and_cached(self, get_query, execute, check): @@ -279,29 +279,49 @@ def test_ephemeral_statistics_are_gated_and_cached(self, get_query, execute, che check.assert_called_once() self.lock.acquire.assert_called_once_with() self.cache.set.assert_called_once() - - @patch("specifyweb.backend.stored_queries.execution.check_table_permissions") + cached_result = json.loads(self.cache.set.call_args.args[1]) + self.assertEqual(cached_result, { + "content": response.content.decode(), + "tables": [datamodel.get_table_strict("Collectionobject").name], + }) + expected_digest = hashlib.sha256( + json.dumps(["ephemeral-statistics-v1", query], sort_keys=True).encode() + ).hexdigest() + self.assertTrue(self.cache.set.call_args.args[0].endswith(f":{expected_digest}")) + + @patch("specifyweb.backend.stored_queries.views.check_table_permissions") @patch("specifyweb.backend.stored_queries.views.run_ephemeral_query") @patch("specifyweb.backend.stored_queries.views.get_query") - def test_ephemeral_cache_hit_checks_permissions_without_executing( + def test_ephemeral_cache_hit_checks_cached_dependencies_without_parsing_query( self, get_query, execute, check ): from specifyweb.backend.stored_queries.views import ephemeral query = self.ephemeral_query() + query["fields"][0]["stringid"] = "1,10,2.locality.localityName" get_query.return_value = query, self.collection - self.cache.get.return_value = '{"count": 12}' - response = ephemeral(self.ephemeral_request()) + table_names = ("Collectionobject", "Collectingevent", "Locality") + self.cache.get.return_value = json.dumps({ + "content": '{"count": 12}', + "tables": [datamodel.get_table_strict(name).name for name in table_names], + }) + with patch("specifyweb.backend.stored_queries.views.fields_from_json") as parse: + response = ephemeral(self.ephemeral_request()) + parse.assert_not_called() self.assertEqual(json.loads(response.content), {"count": 12}) - check.assert_called_once_with( - self.collection, self.user, datamodel.get_table_strict("Collectionobject"), "read" + self.assertCountEqual( + check.call_args_list, + [ + call(self.collection, self.user, datamodel.get_table_strict(name), "read") + for name in table_names + ], ) execute.assert_not_called() self.lock.acquire.assert_not_called() self.cache.set.assert_not_called() @patch( - "specifyweb.backend.stored_queries.execution.check_table_permissions", + "specifyweb.backend.stored_queries.views.check_table_permissions", side_effect=PermissionDenied, ) @patch("specifyweb.backend.stored_queries.views.run_ephemeral_query") @@ -313,30 +333,39 @@ def test_ephemeral_cache_hit_rejects_revoked_permissions( query = self.ephemeral_query() get_query.return_value = query, self.collection - self.cache.get.return_value = '{"count": 12}' - with self.assertRaises(PermissionDenied): - ephemeral(self.ephemeral_request()) + self.cache.get.return_value = json.dumps({ + "content": '{"count": 12}', + "tables": [datamodel.get_table_strict("Collectionobject").name], + }) + with patch("specifyweb.backend.stored_queries.views.fields_from_json") as parse: + with self.assertRaises(PermissionDenied): + ephemeral(self.ephemeral_request()) + parse.assert_not_called() check.assert_called_once() execute.assert_not_called() - self.cache.get.assert_not_called() + self.cache.get.assert_called_once() self.lock.acquire.assert_not_called() self.cache.set.assert_not_called() self.cache.delete.assert_not_called() - @patch("specifyweb.backend.stored_queries.views.statistics_result") + @patch( + "specifyweb.backend.stored_queries.views.run_ephemeral_query", + side_effect=ValueError("Invalid query"), + ) @patch("specifyweb.backend.stored_queries.views.get_query") - def test_malformed_ephemeral_query_is_rejected_before_cache_lookup( - self, get_query, statistics_result_mock - ): + def test_ephemeral_query_errors_propagate_without_caching(self, get_query, execute): from specifyweb.backend.stored_queries.views import ephemeral - get_query.return_value = {"countonly": True}, self.collection - with self.assertRaises(KeyError): + query = self.ephemeral_query() + get_query.return_value = query, self.collection + with self.assertRaisesRegex(ValueError, "Invalid query"): ephemeral(self.ephemeral_request()) - statistics_result_mock.assert_not_called() - self.cache.get.assert_not_called() + execute.assert_called_once_with(self.collection, self.user, query) + self.cache.get.assert_called() + self.cache.set.assert_not_called() + self.lock.release.assert_called_once_with() - @patch("specifyweb.backend.stored_queries.execution.check_table_permissions") + @patch("specifyweb.backend.stored_queries.views.check_table_permissions") @patch("specifyweb.backend.stored_queries.views.run_ephemeral_query") @patch("specifyweb.backend.stored_queries.views.get_query") def test_ephemeral_busy_response_does_not_execute_query( @@ -350,6 +379,7 @@ def test_ephemeral_busy_response_does_not_execute_query( self.assertEqual(response.status_code, 409) self.assertEqual(response["X-Specify-Retry-After"], "2") execute.assert_not_called() + check.assert_not_called() @patch("specifyweb.backend.stored_queries.views.run_ephemeral_query") @patch("specifyweb.backend.stored_queries.views.get_query") diff --git a/specifyweb/backend/stored_queries/execution.py b/specifyweb/backend/stored_queries/execution.py index e6f16ba6622..d3e888030d1 100644 --- a/specifyweb/backend/stored_queries/execution.py +++ b/specifyweb/backend/stored_queries/execution.py @@ -677,18 +677,22 @@ def createPlacemark(kmlDoc, row, coord_cols, table, captions, host): return placemarkElement +def get_query_tables(query_fields: Iterable[QueryField]) -> set[Table]: + """Return the tables referenced by query field paths.""" + return { + table + for field in query_fields + for table in tables_in_path(field.fieldspec.root_table, field.fieldspec.join_path) + } + + def check_query_permissions(collection, user, query_fields: Iterable[QueryField]) -> None: """Check read permissions for query field paths without building the data query. Permission failures propagate from check_table_permissions. This checks current authorization, not query validity. """ - tables_to_read = { - table - for field in query_fields - for table in tables_in_path(field.fieldspec.root_table, field.fieldspec.join_path) - } - for table_to_read in tables_to_read: + for table_to_read in get_query_tables(query_fields): check_table_permissions(collection, user, table_to_read, "read") diff --git a/specifyweb/backend/stored_queries/views.py b/specifyweb/backend/stored_queries/views.py index 5a6b66c7da9..ceda5ccaf80 100644 --- a/specifyweb/backend/stored_queries/views.py +++ b/specifyweb/backend/stored_queries/views.py @@ -18,6 +18,7 @@ from specifyweb.backend.stored_queries.batch_edit import run_batch_edit from specifyweb.specify.models_utils.models_by_table_id import model_names_by_table_id from specifyweb.specify.api.serializers import toJson, uri_for_model +from specifyweb.specify.datamodel import datamodel from . import models from .execution import execute, run_ephemeral_query, do_export, recordset from .queryfield import QueryField, fields_from_json @@ -27,7 +28,7 @@ Loanreturnpreparation, Loanpreparation, Loan from specifyweb.specify.views import login_maybe_required from .execution import execute, run_ephemeral_query, do_export, recordset, \ - return_loan_preps as rlp, check_query_permissions + return_loan_preps as rlp, get_query_tables logger = logging.getLogger(__name__) @@ -118,23 +119,38 @@ def query(request, id): @login_maybe_required @never_cache def ephemeral(request): - """Executes and returns the results of the query provided as JSON in the POST body.""" + """Execute the query provided as JSON in the POST body. + + Statistics cache hits authorize the recorded table dependencies without + reparsing or rebuilding the query. + """ spquery, collection = get_query(request) if request.headers.get("X-Specify-Statistics") == "true": - query_fields = fields_from_json(spquery["fields"]) - check_query_permissions(collection, request.specify_user, query_fields) + def calculate_statistics() -> str: + data = run_ephemeral_query(collection, request.specify_user, spquery) + tables = get_query_tables(fields_from_json(spquery["fields"])) + return json.dumps({ + "content": toJson(data), + "tables": sorted(table.name for table in tables), + }) try: + # Keep metadata-bearing entries separate from older body-only cache entries. content = statistics_result( collection, request.specify_user, - json.dumps(spquery, sort_keys=True), - lambda: toJson(run_ephemeral_query(collection, request.specify_user, spquery)), + json.dumps(["ephemeral-statistics-v1", spquery], sort_keys=True), + calculate_statistics, ) except StatisticsBusy: return busy_response() - return HttpResponse(content, content_type='application/json') + result = json.loads(content) + for table_name in result["tables"]: + check_table_permissions( + collection, request.specify_user, datamodel.get_table_strict(table_name), "read" + ) + return HttpResponse(result["content"], content_type='application/json') data = run_ephemeral_query(collection, request.specify_user, spquery) From 41a099c2ed4f32e850c79aae0404987ee6d3677e Mon Sep 17 00:00:00 2001 From: Caroline Denis Date: Tue, 27 Oct 2026 13:14:22 +0100 Subject: [PATCH 14/47] Fix(stats): exclude query metadata from cache key --- specifyweb/backend/stats/tests.py | 54 +++++++++++++++++++++- specifyweb/backend/stored_queries/views.py | 34 +++++++++++++- 2 files changed, 86 insertions(+), 2 deletions(-) diff --git a/specifyweb/backend/stats/tests.py b/specifyweb/backend/stats/tests.py index 1128124d18d..1929a113673 100644 --- a/specifyweb/backend/stats/tests.py +++ b/specifyweb/backend/stats/tests.py @@ -256,6 +256,8 @@ def ephemeral_request(self, statistics=True): @staticmethod def ephemeral_query(): return { + "contexttableid": 1, + "selectdistinct": False, "countonly": True, "fields": [{ "position": 0, @@ -284,11 +286,61 @@ def test_ephemeral_statistics_are_gated_and_cached(self, get_query, execute, che "content": response.content.decode(), "tables": [datamodel.get_table_strict("Collectionobject").name], }) + from specifyweb.backend.stored_queries.views import ephemeral_statistics_cache_key + expected_digest = hashlib.sha256( - json.dumps(["ephemeral-statistics-v1", query], sort_keys=True).encode() + ephemeral_statistics_cache_key(query).encode() ).hexdigest() self.assertTrue(self.cache.set.call_args.args[0].endswith(f":{expected_digest}")) + def test_ephemeral_statistics_cache_key_ignores_query_metadata(self): + from specifyweb.backend.stored_queries.views import ephemeral_statistics_cache_key + + query = self.ephemeral_query() + query.update({ + "id": 57, + "name": "New Query", + "ordinal": 1, + "remarks": None, + "timestampmodified": "2025-04-01T16:22:51", + "version": 1, + "modifiedbyagent": "/api/specify/agent/3/", + }) + query["fields"][0].update({ + "id": 1344, + "remarks": "Original", + "timestampmodified": "2025-04-01T16:22:51", + "version": 0, + "modifiedbyagent": None, + }) + key = ephemeral_statistics_cache_key(query) + + query["name"] = "Renamed Query" + query["ordinal"] = 2 + query["remarks"] = "Updated" + query["timestampmodified"] = "2025-04-02T16:22:51" + query["version"] = 2 + query["modifiedbyagent"] = "/api/specify/agent/4/" + query["fields"][0].update({ + "position": 3, + "remarks": "Updated", + "timestampmodified": "2025-04-02T16:22:51", + "version": 1, + "modifiedbyagent": "/api/specify/agent/4/", + }) + + self.assertEqual(ephemeral_statistics_cache_key(query), key) + + def test_ephemeral_statistics_cache_key_changes_with_query_inputs(self): + from specifyweb.backend.stored_queries.views import ephemeral_statistics_cache_key + + query = self.ephemeral_query() + key = ephemeral_statistics_cache_key(query) + + query["fields"][0]["startvalue"] = "new filter value" + + self.assertNotEqual(ephemeral_statistics_cache_key(query), key) + @patch("specifyweb.backend.stored_queries.views.check_table_permissions") @patch("specifyweb.backend.stored_queries.views.run_ephemeral_query") @patch("specifyweb.backend.stored_queries.views.get_query") diff --git a/specifyweb/backend/stored_queries/views.py b/specifyweb/backend/stored_queries/views.py index ceda5ccaf80..dfffd2d8ff9 100644 --- a/specifyweb/backend/stored_queries/views.py +++ b/specifyweb/backend/stored_queries/views.py @@ -76,6 +76,38 @@ def format_export_file_name(spquery, file_extension = ''): return non_extension if not file_extension else non_extension + ".%s" % file_extension +def ephemeral_statistics_cache_key(spquery): + """Return a cache key based only on inputs that affect ephemeral query results.""" + fields = sorted(spquery["fields"], key=lambda field: field["position"]) + execution_fields = ( + "stringid", + "isrelfld", + "operstart", + "startvalue", + "isnot", + "isdisplay", + "sorttype", + "formatname", + "isstrict", + ) + query = { + "contexttableid": spquery["contexttableid"], + "selectdistinct": spquery["selectdistinct"], + "countonly": spquery["countonly"], + "limit": spquery.get("limit", 20), + "offset": spquery.get("offset", 0), + "recordsetid": spquery.get("recordsetid"), + "smushed": spquery.get("smushed"), + "searchsynonymy": bool(spquery.get("searchsynonymy", False)), + "formatauditrecids": spquery.get("formatauditrecids", False), + "fields": [ + {name: field.get(name) for name in execution_fields} + for field in fields + ], + } + return json.dumps(["ephemeral-statistics-v2", query], sort_keys=True) + + @require_GET @login_maybe_required @never_cache @@ -140,7 +172,7 @@ def calculate_statistics() -> str: content = statistics_result( collection, request.specify_user, - json.dumps(["ephemeral-statistics-v1", spquery], sort_keys=True), + ephemeral_statistics_cache_key(spquery), calculate_statistics, ) except StatisticsBusy: From 37a3b97e5b6b807d6db55436aa0fcd1070cc6ff5 Mon Sep 17 00:00:00 2001 From: Caroline Denis Date: Tue, 27 Oct 2026 13:21:41 +0100 Subject: [PATCH 15/47] Fix: bound advisory lock names in dispatcher --- specifyweb/backend/stats/coordination.py | 10 +++------- specifyweb/backend/stats/tests.py | 17 +++++++++++----- .../specify/models_utils/lock_tables.py | 20 ++++++++++++++----- 3 files changed, 30 insertions(+), 17 deletions(-) diff --git a/specifyweb/backend/stats/coordination.py b/specifyweb/backend/stats/coordination.py index 69c963cd05a..a0c476a0745 100644 --- a/specifyweb/backend/stats/coordination.py +++ b/specifyweb/backend/stats/coordination.py @@ -3,7 +3,6 @@ import logging from collections.abc import Callable -from django.conf import settings from django.http import JsonResponse from redis.exceptions import RedisError @@ -70,13 +69,10 @@ def cached_result() -> str | None: return result # A database advisory lock has no lease that could expire during a slow query. - lock_name = hashlib.sha256(settings.DATABASE_NAME.encode()).hexdigest()[:48] - # Keep the bounded lock name while letting the dispatcher own cleanup. - with LockDispatcher( - lock_namespace="statistics", lock_name_separator=":" - ) as locks: + # Keep the lock database-scoped while letting the dispatcher bound its name. + with LockDispatcher(lock_name_separator=":") as locks: try: - locks.acquire(lock_name, timeout=0) + locks.acquire("statistics", timeout=0) except TimeoutError as error: logger.debug("Deferring statistics calculation: %s", error) raise StatisticsBusy from error diff --git a/specifyweb/backend/stats/tests.py b/specifyweb/backend/stats/tests.py index 1929a113673..eb60b484444 100644 --- a/specifyweb/backend/stats/tests.py +++ b/specifyweb/backend/stats/tests.py @@ -48,10 +48,7 @@ def test_result_is_cached_for_five_minutes(self): self.lock.acquire.assert_called_once_with() self.lock.release.assert_called_once_with() self.calculate.assert_called_once_with() - expected_lock_name = ( - "statistics:" - + hashlib.sha256("statistics_test".encode()).hexdigest()[:48] - ) + expected_lock_name = "statistics_test:statistics" self.assertEqual(self.lock_class.call_args.args[0], expected_lock_name) self.assertLessEqual(len(expected_lock_name), 64) self.assertEqual(self.lock_class.call_args.args[1], 0) @@ -68,12 +65,22 @@ def test_long_database_name_uses_bounded_lock_name(self): self.run_statistic() lock_name = self.lock_class.call_args.args[0] + full_name = f"{database_name}:statistics" self.assertEqual( lock_name, - "statistics:" + hashlib.sha256(database_name.encode()).hexdigest()[:48], + f"{full_name[:15]}:{hashlib.sha256(full_name.encode()).hexdigest()[:48]}", ) self.assertLessEqual(len(lock_name), 64) + def test_long_database_names_with_shared_prefix_have_distinct_lock_names(self): + names = [] + for database_name in ("d" * 63 + "1", "d" * 63 + "2"): + with override_settings(DATABASE_NAME=database_name): + self.run_statistic() + names.append(self.lock_class.call_args.args[0]) + + self.assertNotEqual(names[0], names[1]) + def assert_uncached_calculation(self): self.assertEqual(self.run_statistic(), '{"count": 12}') self.calculate.assert_called_once_with() diff --git a/specifyweb/specify/models_utils/lock_tables.py b/specifyweb/specify/models_utils/lock_tables.py index b5446ab546c..74bb93897ef 100644 --- a/specifyweb/specify/models_utils/lock_tables.py +++ b/specifyweb/specify/models_utils/lock_tables.py @@ -2,11 +2,14 @@ from django.conf import settings from contextlib import contextmanager from typing import Iterable +import hashlib import logging import json logger = logging.getLogger(__name__) LOCK_NAME_SEPARATOR = "_" +LOCK_NAME_MAX_LENGTH = 64 +LOCK_NAME_HASH_LENGTH = 48 @contextmanager @@ -205,13 +208,10 @@ def __init__( lock_prefix: str | None = None, case_sensitive_names=False, *, - lock_namespace: str | None = None, lock_name_separator: str = LOCK_NAME_SEPARATOR, ): db_name = getattr(settings, "DATABASE_NAME") - self.lock_prefix_parts: list[str] = [ - db_name if lock_namespace is None else lock_namespace - ] + self.lock_prefix_parts: list[str] = [db_name] if lock_prefix is not None: self.lock_prefix_parts.append(lock_prefix) @@ -237,7 +237,17 @@ def lock_name(self, *name_parts: str): (*self.lock_prefix_parts, *name_parts) ) - return final_name.lower() if not self.case_sensitive_names else final_name + if not self.case_sensitive_names: + final_name = final_name.lower() + + if len(final_name) > LOCK_NAME_MAX_LENGTH: + prefix_length = LOCK_NAME_MAX_LENGTH - LOCK_NAME_HASH_LENGTH - 1 + digest = hashlib.sha256(final_name.encode()).hexdigest()[ + :LOCK_NAME_HASH_LENGTH + ] + final_name = f"{final_name[:prefix_length]}:{digest}" + + return final_name @contextmanager def lock_and_release(self, name: str, timeout: int = 5): From 0b735e0cb60a5ecfa557568117d43d146cd3c4ba Mon Sep 17 00:00:00 2001 From: Caroline Denis Date: Tue, 27 Oct 2026 13:49:13 +0100 Subject: [PATCH 16/47] Dependencies: Upgrade Hypothesis in requirement testing --- requirements-testing.txt | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/requirements-testing.txt b/requirements-testing.txt index 3953ddb3b70..649b9de0778 100644 --- a/requirements-testing.txt +++ b/requirements-testing.txt @@ -1,7 +1,8 @@ # django-stubs==1.12.0 django-stubs==5.2.9 mypy==1.13.0 -hypothesis>=6.131.9,<7 +# hypothesis-jsonschema 0.19.0 depends on Hypothesis's private many.more() API. +hypothesis==6.155.7 hypothesis-jsonschema==0.19.0 types-requests==2.28.5 lxml From da3e17754988a4a00f76d2d8378aa579813d5b8d Mon Sep 17 00:00:00 2001 From: "Caroline D." <108160931+CarolineDenis@users.noreply.github.com> Date: Tue, 27 Oct 2026 14:16:47 +0100 Subject: [PATCH 17/47] Potential fix for pull request finding 'CodeQL / Cyclic import' Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com> --- specifyweb/backend/stored_queries/execution.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/specifyweb/backend/stored_queries/execution.py b/specifyweb/backend/stored_queries/execution.py index d3e888030d1..f59675b312c 100644 --- a/specifyweb/backend/stored_queries/execution.py +++ b/specifyweb/backend/stored_queries/execution.py @@ -27,7 +27,6 @@ from . import models from .format import ObjectFormatter, ObjectFormatterProps -from .query_construct import QueryConstruct, tables_in_path from .field_spec_maps import transform_field_specs from .web_portal_export import query_to_web_portal_zip as _query_to_web_portal_zip, WebportalQueryResultProcessors from specifyweb.backend.notifications.models import Message @@ -46,6 +45,7 @@ # import it would already be handled. if TYPE_CHECKING: from specifyweb.backend.stored_queries.queryfield import QueryField + from .query_construct import QueryConstruct logger = logging.getLogger(__name__) From 58599027551d2c64c82a983aad9132a669d7bb03 Mon Sep 17 00:00:00 2001 From: Caroline Denis Date: Tue, 27 Oct 2026 14:21:56 +0100 Subject: [PATCH 18/47] test(stats): pass retry header via Response constructor --- .../Statistics/__tests__/requests.test.ts | 9 +++-- .../js_src/lib/tests/__mocks__/Response.ts | 33 +++++++++++++++---- 2 files changed, 31 insertions(+), 11 deletions(-) diff --git a/specifyweb/frontend/js_src/lib/components/Statistics/__tests__/requests.test.ts b/specifyweb/frontend/js_src/lib/components/Statistics/__tests__/requests.test.ts index 3c0c5eb2a30..7228330751d 100644 --- a/specifyweb/frontend/js_src/lib/components/Statistics/__tests__/requests.test.ts +++ b/specifyweb/frontend/js_src/lib/components/Statistics/__tests__/requests.test.ts @@ -16,11 +16,10 @@ const options = { headers: { Accept: 'application/json' as const } }; const result = (status: number, retryAfter?: string) => ({ status, data: { count: 12 }, - response: Object.assign(new Response(undefined, { status }), { - headers: { - get: (name: string) => - name === 'X-Specify-Retry-After' ? (retryAfter ?? null) : null, - }, + response: new Response(undefined, { + status, + headers: + retryAfter === undefined ? {} : { 'X-Specify-Retry-After': retryAfter }, }), }); diff --git a/specifyweb/frontend/js_src/lib/tests/__mocks__/Response.ts b/specifyweb/frontend/js_src/lib/tests/__mocks__/Response.ts index df7fa342e4e..552bf2990af 100644 --- a/specifyweb/frontend/js_src/lib/tests/__mocks__/Response.ts +++ b/specifyweb/frontend/js_src/lib/tests/__mocks__/Response.ts @@ -8,13 +8,34 @@ Object.defineProperty(globalThis, 'Response', { _body: string | undefined, { status, - }: { readonly status: number; readonly statusText: string | undefined } + headers = {}, + }: { + readonly status: number; + readonly statusText: string | undefined; + readonly headers?: Readonly>; + } ) { - return { - status, - statusText: '', - ok: status >= 200 && status <= 299, - }; + const normalizedHeaders = new Map( + Object.entries(headers).map(([name, value]) => [ + name.toLowerCase(), + value, + ]) + ); + return Object.defineProperty( + { + status, + statusText: '', + ok: status >= 200 && status <= 299, + }, + // Like a real Response, headers is not an own enumerable property + 'headers', + { + value: { + get: (name: string) => + normalizedHeaders.get(name.toLowerCase()) ?? null, + }, + } + ); }, }); From f91eca16b7dd28e97ed8a75b3796e7d5a62e04dd Mon Sep 17 00:00:00 2001 From: "Caroline D." <108160931+CarolineDenis@users.noreply.github.com> Date: Tue, 27 Oct 2026 14:23:27 +0100 Subject: [PATCH 19/47] Potential fix for pull request finding 'CodeQL / Cyclic import' Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com> --- specifyweb/backend/stored_queries/execution.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/specifyweb/backend/stored_queries/execution.py b/specifyweb/backend/stored_queries/execution.py index f59675b312c..6622861662c 100644 --- a/specifyweb/backend/stored_queries/execution.py +++ b/specifyweb/backend/stored_queries/execution.py @@ -45,7 +45,9 @@ # import it would already be handled. if TYPE_CHECKING: from specifyweb.backend.stored_queries.queryfield import QueryField - from .query_construct import QueryConstruct + + class QueryConstruct: # typing-only forward declaration to avoid cyclic import + pass logger = logging.getLogger(__name__) From 52eb613efb45d6e3d36d55cc4ffbad797f59617d Mon Sep 17 00:00:00 2001 From: Caroline Denis Date: Tue, 27 Oct 2026 15:11:49 +0100 Subject: [PATCH 20/47] Merge conflicts --- specifyweb/backend/stored_queries/execution.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/specifyweb/backend/stored_queries/execution.py b/specifyweb/backend/stored_queries/execution.py index 6622861662c..ec94db95756 100644 --- a/specifyweb/backend/stored_queries/execution.py +++ b/specifyweb/backend/stored_queries/execution.py @@ -47,6 +47,7 @@ from specifyweb.backend.stored_queries.queryfield import QueryField class QueryConstruct: # typing-only forward declaration to avoid cyclic import + pass logger = logging.getLogger(__name__) @@ -681,6 +682,8 @@ def createPlacemark(kmlDoc, row, coord_cols, table, captions, host): def get_query_tables(query_fields: Iterable[QueryField]) -> set[Table]: """Return the tables referenced by query field paths.""" + from .query_construct import tables_in_path + return { table for field in query_fields @@ -984,6 +987,8 @@ def build_query_construct_base( model, props: BuildQueryProps, ): + from .query_construct import QueryConstruct + id_field = model._id catalognumber_field = model.catalogNumber if hasattr(model, 'catalogNumber') else None query_construct_query = session.query(id_field) From 6b76e1eb6143940258e7de709b2295b87ef702a8 Mon Sep 17 00:00:00 2001 From: Caroline Denis Date: Tue, 27 Oct 2026 15:55:47 +0100 Subject: [PATCH 21/47] Refactor: Fix cycling import --- specifyweb/backend/express_search/views.py | 2 +- specifyweb/backend/stats/tests.py | 2 +- .../backend/stored_queries/batch_edit.py | 2 +- .../backend/stored_queries/execution.py | 10 +- specifyweb/backend/stored_queries/format.py | 2 +- .../backend/stored_queries/group_concat.py | 2 +- ..._construct.py => query_construct_model.py} | 92 +++++++++++-------- .../backend/stored_queries/queryfieldspec.py | 15 +++ .../tests/test_age_query_columns.py | 2 +- .../tests/test_format/test_format_query.py | 2 +- 10 files changed, 79 insertions(+), 52 deletions(-) rename specifyweb/backend/stored_queries/{query_construct.py => query_construct_model.py} (74%) diff --git a/specifyweb/backend/express_search/views.py b/specifyweb/backend/express_search/views.py index 633763e8dd0..01fd5663c87 100644 --- a/specifyweb/backend/express_search/views.py +++ b/specifyweb/backend/express_search/views.py @@ -21,7 +21,7 @@ from specifyweb.backend.stored_queries.execution import DefaultQueryFormatterProps, filter_by_collection from specifyweb.backend.stored_queries.format import ObjectFormatter from specifyweb.backend.stored_queries.queryfield import QueryField -from specifyweb.backend.stored_queries.query_construct import QueryConstruct +from specifyweb.backend.stored_queries.query_construct_model import QueryConstruct from specifyweb.backend.stored_queries.queryfieldspec import QueryFieldSpec logger = logging.getLogger(__name__) diff --git a/specifyweb/backend/stats/tests.py b/specifyweb/backend/stats/tests.py index eb60b484444..a08a26d3b66 100644 --- a/specifyweb/backend/stats/tests.py +++ b/specifyweb/backend/stats/tests.py @@ -506,7 +506,7 @@ def test_tree_rank_paths_check_referenced_tables(self, check): ) def test_table_paths_accept_field_names(self): - from specifyweb.backend.stored_queries.query_construct import tables_in_path + from specifyweb.backend.stored_queries.queryfieldspec import tables_in_path self.assertEqual( tables_in_path( diff --git a/specifyweb/backend/stored_queries/batch_edit.py b/specifyweb/backend/stored_queries/batch_edit.py index ba136c59b70..7eb9ac19b42 100644 --- a/specifyweb/backend/stored_queries/batch_edit.py +++ b/specifyweb/backend/stored_queries/batch_edit.py @@ -706,7 +706,7 @@ def to_upload_plan( # Yuk, finally. # Whether we are something like [det-> (T -- what we are) -> tree]. - # Set break points in handle_tree_field in query_construct.py to figure out what this means. + # Set break points in handle_tree_field in query_construct_model.py to figure out what this means. intermediary_to_tree = any( canonical.batch_edit_pack is not None and canonical.batch_edit_pack.is_part_of_tree(query_fields) diff --git a/specifyweb/backend/stored_queries/execution.py b/specifyweb/backend/stored_queries/execution.py index ec94db95756..077717a4331 100644 --- a/specifyweb/backend/stored_queries/execution.py +++ b/specifyweb/backend/stored_queries/execution.py @@ -28,6 +28,8 @@ from . import models from .format import ObjectFormatter, ObjectFormatterProps from .field_spec_maps import transform_field_specs +from .query_construct_model import QueryConstruct +from .queryfieldspec import tables_in_path from .web_portal_export import query_to_web_portal_zip as _query_to_web_portal_zip, WebportalQueryResultProcessors from specifyweb.backend.notifications.models import Message from specifyweb.backend.permissions.permissions import check_table_permissions @@ -46,10 +48,6 @@ if TYPE_CHECKING: from specifyweb.backend.stored_queries.queryfield import QueryField - class QueryConstruct: # typing-only forward declaration to avoid cyclic import - - pass - logger = logging.getLogger(__name__) SERIES_MAX_ROWS = 10000 @@ -682,8 +680,6 @@ def createPlacemark(kmlDoc, row, coord_cols, table, captions, host): def get_query_tables(query_fields: Iterable[QueryField]) -> set[Table]: """Return the tables referenced by query field paths.""" - from .query_construct import tables_in_path - return { table for field in query_fields @@ -987,8 +983,6 @@ def build_query_construct_base( model, props: BuildQueryProps, ): - from .query_construct import QueryConstruct - id_field = model._id catalognumber_field = model.catalogNumber if hasattr(model, 'catalogNumber') else None query_construct_query = session.query(id_field) diff --git a/specifyweb/backend/stored_queries/format.py b/specifyweb/backend/stored_queries/format.py index 6837a5af4b7..a686f6ea3cb 100644 --- a/specifyweb/backend/stored_queries/format.py +++ b/specifyweb/backend/stored_queries/format.py @@ -30,7 +30,7 @@ from .build_models import get_many_to_many_join_info from .group_concat import group_concat from .blank_nulls import blank_nulls -from .query_construct import QueryConstruct +from .query_construct_model import QueryConstruct from .queryfieldspec import QueryFieldSpec from typing import NamedTuple diff --git a/specifyweb/backend/stored_queries/group_concat.py b/specifyweb/backend/stored_queries/group_concat.py index 6b21055cb09..d3caa9988e1 100644 --- a/specifyweb/backend/stored_queries/group_concat.py +++ b/specifyweb/backend/stored_queries/group_concat.py @@ -7,7 +7,7 @@ from sqlalchemy.sql.elements import ClauseElement from sqlalchemy.types import String -from specifyweb.backend.stored_queries.query_construct import QueryConstruct +from specifyweb.backend.stored_queries.query_construct_model import QueryConstruct class group_concat(expression.ColumnElement): # MySQL GROUP_CONCAT(expr [ORDER BY ...] [SEPARATOR ...]) diff --git a/specifyweb/backend/stored_queries/query_construct.py b/specifyweb/backend/stored_queries/query_construct_model.py similarity index 74% rename from specifyweb/backend/stored_queries/query_construct.py rename to specifyweb/backend/stored_queries/query_construct_model.py index 47dace5955f..8a75027490e 100644 --- a/specifyweb/backend/stored_queries/query_construct.py +++ b/specifyweb/backend/stored_queries/query_construct_model.py @@ -11,6 +11,7 @@ logger = logging.getLogger(__name__) + def _safe_filter(query): count = query.count() if count <= 1: @@ -18,31 +19,28 @@ def _safe_filter(query): raise Exception(f"Got more than one matching: {list(query)}") -def tables_in_path(table, join_path): - path = deque(join_path) - tables = [table] - while len(path) > 0: - field = path.popleft() - if isinstance(field, str): - field = tables[-1].get_field(field, strict=True) - if not field.is_relationship: # also handles tree ranks - break - - tables.append(spmodels.datamodel.get_table(field.relatedModelName, strict=True)) - return tables - - -class QueryConstruct(namedtuple('QueryConstruct', 'collection objectformatter query join_cache tree_rank_count internal_filters')): - +class QueryConstruct( + namedtuple( + "QueryConstruct", + "collection objectformatter query join_cache tree_rank_count internal_filters", + ) +): def __new__(cls, *args, **kwargs): - kwargs['join_cache'] = dict() + kwargs["join_cache"] = dict() # TODO: Use tree_rank_count to implement cases where formatter of taxon is defined with fields from the parent. # In that case, the cycle will end (unlike other cyclical cases). - kwargs['tree_rank_count'] = 0 - kwargs['internal_filters'] = [] + kwargs["tree_rank_count"] = 0 + kwargs["internal_filters"] = [] return super().__new__(cls, *args, **kwargs) - def handle_tree_field(self, node, table, tree_rank: TreeRankQuery, next_join_path, current_field_spec: QueryFieldSpec): + def handle_tree_field( + self, + node, + table, + tree_rank: TreeRankQuery, + next_join_path, + current_field_spec: QueryFieldSpec, + ): query = self query_before_tree_joins = query if query.collection is None: # Not sure it makes sense to query across collections @@ -50,12 +48,17 @@ def handle_tree_field(self, node, table, tree_rank: TreeRankQuery, next_join_pat f"No Collection found in Query for {table}", {"table": table, "localizationKey": "noCollectionInQuery"}, ) - logger.info('handling treefield %s rank: %s field: %s', table, tree_rank.name, next_join_path) + logger.info( + "handling treefield %s rank: %s field: %s", + table, + tree_rank.name, + next_join_path, + ) - treedefitem_column = table.name + 'TreeDefItemID' - treedef_column = table.name + 'TreeDefID' + treedefitem_column = table.name + "TreeDefItemID" + treedef_column = table.name + "TreeDefID" - cache_key = (table, 'TreeRanks') + cache_key = (table, "TreeRanks") if cache_key in query.join_cache: logger.debug("using join cache for %r tree ranks.", node) ancestors, treedefs = query.join_cache[cache_key] @@ -79,12 +82,23 @@ def handle_tree_field(self, node, table, tree_rank: TreeRankQuery, next_join_pat item_model = getattr(spmodels, table.django_name + "treedefitem") # TODO: optimize out the ranks that appear? cache them - treedefs_with_ranks: list[tuple[int, int]] = [tup for tup in [ - (treedef_id, _safe_filter(item_model.objects.filter(treedef_id=treedef_id, name=tree_rank.name).values_list('id', flat=True))) - for treedef_id, _ in treedefs - # For constructing tree queries for batch edit - if (tree_rank.treedef_id is None or tree_rank.treedef_id == treedef_id) - ] if tup[1] is not None] + treedefs_with_ranks: list[tuple[int, int]] = [ + tup + for tup in [ + ( + treedef_id, + _safe_filter( + item_model.objects.filter( + treedef_id=treedef_id, name=tree_rank.name + ).values_list("id", flat=True) + ), + ) + for treedef_id, _ in treedefs + # For constructing tree queries for batch edit + if tree_rank.treedef_id is None or tree_rank.treedef_id == treedef_id + ] + if tup[1] is not None + ] if not treedefs_with_ranks: logger.warning( @@ -98,19 +112,21 @@ def handle_tree_field(self, node, table, tree_rank: TreeRankQuery, next_join_pat def make_tree_field_spec(tree_node): return current_field_spec._replace( - root_table=table, # rebasing the query - root_sql_table=tree_node, # this is needed to preserve SQL aliased going to next part - join_path=next_join_path, # slicing join path to begin from after the tree + root_table=table, # rebasing the query + root_sql_table=tree_node, # this is needed to preserve SQL aliased going to next part + join_path=next_join_path, # slicing join path to begin from after the tree ) cases = [] - field = None # just to stop mypy from complaining. + field = None # just to stop mypy from complaining. for ancestor in ancestors: field_spec = make_tree_field_spec(ancestor) query, orm_field, field, table = field_spec.add_spec_to_query(query) # Field and table won't matter. Rank acts as fork, and these two will be same across siblings for treedefitem_param in treedefitem_params: - cases.append((getattr(ancestor, treedefitem_column) == treedefitem_param, orm_field)) + cases.append( + (getattr(ancestor, treedefitem_column) == treedefitem_param, orm_field) + ) column = sql.case(cases) @@ -154,7 +170,6 @@ def build_join(self, table, model, join_path): table, model = next_table, aliased return query, model, table, field - # To make things "simpler", it doesn't apply any filters, but returns a single predicate # @model is an input parameter, because cannot guess if it is aliased or not (callers are supposed to know that) def get_internal_filters(self): @@ -166,11 +181,14 @@ def get_internal_filters(self): return self.internal_filters[0] return sql.or_(*self.internal_filters) + def add_proxy_method(name): def proxy(self, *args, **kwargs): method = getattr(self.query, name) return self._replace(query=method(*args, **kwargs)) + setattr(QueryConstruct, name, proxy) -for name in 'filter join outerjoin add_columns reset_joinpoint group_by'.split(): + +for name in "filter join outerjoin add_columns reset_joinpoint group_by".split(): add_proxy_method(name) diff --git a/specifyweb/backend/stored_queries/queryfieldspec.py b/specifyweb/backend/stored_queries/queryfieldspec.py index 292f4777016..eda7ebee439 100644 --- a/specifyweb/backend/stored_queries/queryfieldspec.py +++ b/specifyweb/backend/stored_queries/queryfieldspec.py @@ -187,6 +187,21 @@ def null_safe_not(field_expr, predicate): QueryNode = Field | Relationship | TreeRankQuery FieldSpecJoinPath = tuple[QueryNode] + +def tables_in_path(table, join_path): + path = deque(join_path) + tables = [table] + while len(path) > 0: + field = path.popleft() + if isinstance(field, str): + field = tables[-1].get_field(field, strict=True) + if not field.is_relationship: # also handles tree ranks + break + + tables.append(datamodel.get_table(field.relatedModelName, strict=True)) + return tables + + class QueryFieldSpec( namedtuple( "QueryFieldSpec", diff --git a/specifyweb/backend/stored_queries/tests/test_age_query_columns.py b/specifyweb/backend/stored_queries/tests/test_age_query_columns.py index a709569b184..090880982c2 100644 --- a/specifyweb/backend/stored_queries/tests/test_age_query_columns.py +++ b/specifyweb/backend/stored_queries/tests/test_age_query_columns.py @@ -6,7 +6,7 @@ from specifyweb.backend.stored_queries import models from specifyweb.backend.stored_queries.execution import add_fields_to_query -from specifyweb.backend.stored_queries.query_construct import QueryConstruct +from specifyweb.backend.stored_queries.query_construct_model import QueryConstruct from specifyweb.backend.stored_queries.queryfield import fields_from_json diff --git a/specifyweb/backend/stored_queries/tests/test_format/test_format_query.py b/specifyweb/backend/stored_queries/tests/test_format/test_format_query.py index 2dd0883c277..cd41624872b 100644 --- a/specifyweb/backend/stored_queries/tests/test_format/test_format_query.py +++ b/specifyweb/backend/stored_queries/tests/test_format/test_format_query.py @@ -1,5 +1,5 @@ from specifyweb.backend.stored_queries.format import ObjectFormatter -from specifyweb.backend.stored_queries.query_construct import QueryConstruct +from specifyweb.backend.stored_queries.query_construct_model import QueryConstruct from specifyweb.backend.stored_queries.tests.base_format import SIMPLE_DEF from specifyweb.backend.stored_queries.tests.tests import SQLAlchemySetup from xml.etree import ElementTree From 96b7f7580c57e1e6fdb461902e84220a35708ee2 Mon Sep 17 00:00:00 2001 From: "Caroline D." <108160931+CarolineDenis@users.noreply.github.com> Date: Tue, 27 Oct 2026 16:02:53 +0100 Subject: [PATCH 22/47] Potential fix for pull request finding 'CodeQL / Cyclic import' Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com> --- specifyweb/backend/express_search/views.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/specifyweb/backend/express_search/views.py b/specifyweb/backend/express_search/views.py index 01fd5663c87..3d0276ff1c4 100644 --- a/specifyweb/backend/express_search/views.py +++ b/specifyweb/backend/express_search/views.py @@ -21,7 +21,6 @@ from specifyweb.backend.stored_queries.execution import DefaultQueryFormatterProps, filter_by_collection from specifyweb.backend.stored_queries.format import ObjectFormatter from specifyweb.backend.stored_queries.queryfield import QueryField -from specifyweb.backend.stored_queries.query_construct_model import QueryConstruct from specifyweb.backend.stored_queries.queryfieldspec import QueryFieldSpec logger = logging.getLogger(__name__) @@ -32,6 +31,8 @@ def get_express_search_config(collection, user): def build_primary_query(session, searchtable, terms, collection, user, as_scalar=False): + from specifyweb.backend.stored_queries.query_construct_model import QueryConstruct + table = datamodel.get_table(searchtable.find('tableName').text) check_table_permissions(collection, user, table, "read") From a2357081ced71eaa12c51e56a7f5b71ed2d9bed6 Mon Sep 17 00:00:00 2001 From: "Caroline D." <108160931+CarolineDenis@users.noreply.github.com> Date: Tue, 27 Oct 2026 16:03:07 +0100 Subject: [PATCH 23/47] Potential fix for pull request finding 'CodeQL / Cyclic import' Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com> --- specifyweb/backend/stored_queries/execution.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/specifyweb/backend/stored_queries/execution.py b/specifyweb/backend/stored_queries/execution.py index 077717a4331..1fbbe79976f 100644 --- a/specifyweb/backend/stored_queries/execution.py +++ b/specifyweb/backend/stored_queries/execution.py @@ -28,7 +28,6 @@ from . import models from .format import ObjectFormatter, ObjectFormatterProps from .field_spec_maps import transform_field_specs -from .query_construct_model import QueryConstruct from .queryfieldspec import tables_in_path from .web_portal_export import query_to_web_portal_zip as _query_to_web_portal_zip, WebportalQueryResultProcessors from specifyweb.backend.notifications.models import Message @@ -47,6 +46,7 @@ # import it would already be handled. if TYPE_CHECKING: from specifyweb.backend.stored_queries.queryfield import QueryField + from specifyweb.backend.stored_queries.query_construct_model import QueryConstruct logger = logging.getLogger(__name__) From 56af4f860b9874e5f8314c68b0568ce960797bf9 Mon Sep 17 00:00:00 2001 From: "Caroline D." <108160931+CarolineDenis@users.noreply.github.com> Date: Tue, 27 Oct 2026 16:03:27 +0100 Subject: [PATCH 24/47] Potential fix for pull request finding 'CodeQL / Cyclic import' Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com> --- specifyweb/backend/stored_queries/format.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/specifyweb/backend/stored_queries/format.py b/specifyweb/backend/stored_queries/format.py index a686f6ea3cb..c726455e660 100644 --- a/specifyweb/backend/stored_queries/format.py +++ b/specifyweb/backend/stored_queries/format.py @@ -1,3 +1,4 @@ +from __future__ import annotations import logging import re @@ -30,9 +31,11 @@ from .build_models import get_many_to_many_join_info from .group_concat import group_concat from .blank_nulls import blank_nulls -from .query_construct_model import QueryConstruct from .queryfieldspec import QueryFieldSpec -from typing import NamedTuple +from typing import NamedTuple, TYPE_CHECKING + +if TYPE_CHECKING: + from .query_construct_model import QueryConstruct logger = logging.getLogger(__name__) From 454d472c47b273e721aec657f040d4ff3ea39ee7 Mon Sep 17 00:00:00 2001 From: "Caroline D." <108160931+CarolineDenis@users.noreply.github.com> Date: Tue, 27 Oct 2026 16:03:40 +0100 Subject: [PATCH 25/47] Potential fix for pull request finding 'CodeQL / Cyclic import' Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com> --- specifyweb/backend/stored_queries/group_concat.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/specifyweb/backend/stored_queries/group_concat.py b/specifyweb/backend/stored_queries/group_concat.py index d3caa9988e1..66a7c4bf5ac 100644 --- a/specifyweb/backend/stored_queries/group_concat.py +++ b/specifyweb/backend/stored_queries/group_concat.py @@ -1,13 +1,18 @@ +from __future__ import annotations + # Based on stackoverflow answer from Wolph: # https://stackoverflow.com/questions/19205850/how-do-i-write-a-group-concat-function-in-sqlalchemy +from typing import TYPE_CHECKING + import sqlalchemy from sqlalchemy.sql import expression, bindparam from sqlalchemy.ext import compiler from sqlalchemy.sql.elements import ClauseElement from sqlalchemy.types import String -from specifyweb.backend.stored_queries.query_construct_model import QueryConstruct +if TYPE_CHECKING: + from specifyweb.backend.stored_queries.query_construct_model import QueryConstruct class group_concat(expression.ColumnElement): # MySQL GROUP_CONCAT(expr [ORDER BY ...] [SEPARATOR ...]) From 627c90f7ee7446bb36c52baf32e4d31fa7b4b356 Mon Sep 17 00:00:00 2001 From: "Caroline D." <108160931+CarolineDenis@users.noreply.github.com> Date: Tue, 27 Oct 2026 16:09:26 +0100 Subject: [PATCH 26/47] Potential fix for pull request finding 'CodeQL / Cyclic import' Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com> --- specifyweb/backend/stored_queries/execution.py | 1 - 1 file changed, 1 deletion(-) diff --git a/specifyweb/backend/stored_queries/execution.py b/specifyweb/backend/stored_queries/execution.py index 1fbbe79976f..25cb576ab2a 100644 --- a/specifyweb/backend/stored_queries/execution.py +++ b/specifyweb/backend/stored_queries/execution.py @@ -28,7 +28,6 @@ from . import models from .format import ObjectFormatter, ObjectFormatterProps from .field_spec_maps import transform_field_specs -from .queryfieldspec import tables_in_path from .web_portal_export import query_to_web_portal_zip as _query_to_web_portal_zip, WebportalQueryResultProcessors from specifyweb.backend.notifications.models import Message from specifyweb.backend.permissions.permissions import check_table_permissions From f1106c0ba55530654f86eb22a960e1fff3a18d4d Mon Sep 17 00:00:00 2001 From: "Caroline D." <108160931+CarolineDenis@users.noreply.github.com> Date: Tue, 27 Oct 2026 16:09:53 +0100 Subject: [PATCH 27/47] Potential fix for pull request finding 'CodeQL / Cyclic import' Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com> --- specifyweb/backend/express_search/views.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/specifyweb/backend/express_search/views.py b/specifyweb/backend/express_search/views.py index 3d0276ff1c4..478e3c3d2db 100644 --- a/specifyweb/backend/express_search/views.py +++ b/specifyweb/backend/express_search/views.py @@ -31,8 +31,6 @@ def get_express_search_config(collection, user): def build_primary_query(session, searchtable, terms, collection, user, as_scalar=False): - from specifyweb.backend.stored_queries.query_construct_model import QueryConstruct - table = datamodel.get_table(searchtable.find('tableName').text) check_table_permissions(collection, user, table, "read") @@ -42,7 +40,7 @@ def build_primary_query(session, searchtable, terms, collection, user, as_scalar fields = [table.get_field(fn.text) for fn in searchtable.findall('.//searchfield/fieldName')] - query = QueryConstruct( + query = build_query_construct( collection=collection, objectformatter=ObjectFormatter(collection, user, False, props=DefaultQueryFormatterProps()), query=session.query(id_field), From b29173e18778a6ffa192643610b1a1834d2f4989 Mon Sep 17 00:00:00 2001 From: "Caroline D." <108160931+CarolineDenis@users.noreply.github.com> Date: Tue, 27 Oct 2026 16:10:14 +0100 Subject: [PATCH 28/47] Potential fix for pull request finding 'CodeQL / Cyclic import' Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com> --- specifyweb/backend/stored_queries/group_concat.py | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/specifyweb/backend/stored_queries/group_concat.py b/specifyweb/backend/stored_queries/group_concat.py index 66a7c4bf5ac..ad124533380 100644 --- a/specifyweb/backend/stored_queries/group_concat.py +++ b/specifyweb/backend/stored_queries/group_concat.py @@ -3,7 +3,7 @@ # Based on stackoverflow answer from Wolph: # https://stackoverflow.com/questions/19205850/how-do-i-write-a-group-concat-function-in-sqlalchemy -from typing import TYPE_CHECKING +from typing import Any import sqlalchemy from sqlalchemy.sql import expression, bindparam @@ -11,9 +11,6 @@ from sqlalchemy.sql.elements import ClauseElement from sqlalchemy.types import String -if TYPE_CHECKING: - from specifyweb.backend.stored_queries.query_construct_model import QueryConstruct - class group_concat(expression.ColumnElement): # MySQL GROUP_CONCAT(expr [ORDER BY ...] [SEPARATOR ...]) name = "group_concat" @@ -46,7 +43,7 @@ def _group_concat_mysql(element, compiler, **kwargs): return 'GROUP_CONCAT(%s)' % inner_expr -def group_by_displayed_fields(query: QueryConstruct, fields, ignore_cat_num=False): +def group_by_displayed_fields(query: Any, fields, ignore_cat_num=False): for field in fields: if ( ignore_cat_num From ddecda63017312c5ed1b5e529d724a100fcd744b Mon Sep 17 00:00:00 2001 From: "Caroline D." <108160931+CarolineDenis@users.noreply.github.com> Date: Tue, 27 Oct 2026 16:10:51 +0100 Subject: [PATCH 29/47] Potential fix for pull request finding 'CodeQL / Cyclic import' Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com> --- specifyweb/backend/stored_queries/execution.py | 1 - 1 file changed, 1 deletion(-) diff --git a/specifyweb/backend/stored_queries/execution.py b/specifyweb/backend/stored_queries/execution.py index 25cb576ab2a..e6d95ad8775 100644 --- a/specifyweb/backend/stored_queries/execution.py +++ b/specifyweb/backend/stored_queries/execution.py @@ -45,7 +45,6 @@ # import it would already be handled. if TYPE_CHECKING: from specifyweb.backend.stored_queries.queryfield import QueryField - from specifyweb.backend.stored_queries.query_construct_model import QueryConstruct logger = logging.getLogger(__name__) From a25151c1c09f32393116cbc382d60a8cd1c981fc Mon Sep 17 00:00:00 2001 From: Caroline Denis Date: Tue, 27 Oct 2026 16:21:00 +0100 Subject: [PATCH 30/47] Fix cycling imports --- specifyweb/backend/stats/tests.py | 2 +- .../backend/stored_queries/execution.py | 2 + specifyweb/backend/stored_queries/format.py | 2 +- .../stored_queries/query_construct_model.py | 6 ++- .../backend/stored_queries/query_path.py | 17 +++++++ .../backend/stored_queries/queryfieldspec.py | 50 +------------------ .../backend/stored_queries/tree_rank.py | 39 +++++++++++++++ 7 files changed, 65 insertions(+), 53 deletions(-) create mode 100644 specifyweb/backend/stored_queries/query_path.py create mode 100644 specifyweb/backend/stored_queries/tree_rank.py diff --git a/specifyweb/backend/stats/tests.py b/specifyweb/backend/stats/tests.py index a08a26d3b66..2a5844641cf 100644 --- a/specifyweb/backend/stats/tests.py +++ b/specifyweb/backend/stats/tests.py @@ -506,7 +506,7 @@ def test_tree_rank_paths_check_referenced_tables(self, check): ) def test_table_paths_accept_field_names(self): - from specifyweb.backend.stored_queries.queryfieldspec import tables_in_path + from specifyweb.backend.stored_queries.query_path import tables_in_path self.assertEqual( tables_in_path( diff --git a/specifyweb/backend/stored_queries/execution.py b/specifyweb/backend/stored_queries/execution.py index e6d95ad8775..0c3ed778834 100644 --- a/specifyweb/backend/stored_queries/execution.py +++ b/specifyweb/backend/stored_queries/execution.py @@ -28,6 +28,8 @@ from . import models from .format import ObjectFormatter, ObjectFormatterProps from .field_spec_maps import transform_field_specs +from .query_construct_model import QueryConstruct +from .query_path import tables_in_path from .web_portal_export import query_to_web_portal_zip as _query_to_web_portal_zip, WebportalQueryResultProcessors from specifyweb.backend.notifications.models import Message from specifyweb.backend.permissions.permissions import check_table_permissions diff --git a/specifyweb/backend/stored_queries/format.py b/specifyweb/backend/stored_queries/format.py index c726455e660..555cd52b1eb 100644 --- a/specifyweb/backend/stored_queries/format.py +++ b/specifyweb/backend/stored_queries/format.py @@ -25,7 +25,6 @@ from specifyweb.specify.models import datamodel, Splocalecontainer from specifyweb.specify.datamodel import Field, Relationship, Table -from specifyweb.backend.stored_queries.queryfield import QueryField from . import models from .build_models import get_many_to_many_join_info @@ -35,6 +34,7 @@ from typing import NamedTuple, TYPE_CHECKING if TYPE_CHECKING: + from specifyweb.backend.stored_queries.queryfield import QueryField from .query_construct_model import QueryConstruct logger = logging.getLogger(__name__) diff --git a/specifyweb/backend/stored_queries/query_construct_model.py b/specifyweb/backend/stored_queries/query_construct_model.py index 8a75027490e..83256bddcf4 100644 --- a/specifyweb/backend/stored_queries/query_construct_model.py +++ b/specifyweb/backend/stored_queries/query_construct_model.py @@ -1,3 +1,5 @@ +from __future__ import annotations + import logging from collections import namedtuple, deque @@ -6,7 +8,7 @@ import specifyweb.specify.models as spmodels from specifyweb.backend.trees.utils import get_treedefs -from .queryfieldspec import TreeRankQuery, QueryFieldSpec +from .tree_rank import TreeRankQuery from specifyweb.backend.stored_queries import models logger = logging.getLogger(__name__) @@ -39,7 +41,7 @@ def handle_tree_field( table, tree_rank: TreeRankQuery, next_join_path, - current_field_spec: QueryFieldSpec, + current_field_spec, ): query = self query_before_tree_joins = query diff --git a/specifyweb/backend/stored_queries/query_path.py b/specifyweb/backend/stored_queries/query_path.py new file mode 100644 index 00000000000..5211b8dc1d1 --- /dev/null +++ b/specifyweb/backend/stored_queries/query_path.py @@ -0,0 +1,17 @@ +from collections import deque + +from specifyweb.specify.datamodel import datamodel + + +def tables_in_path(table, join_path): + path = deque(join_path) + tables = [table] + while len(path) > 0: + field = path.popleft() + if isinstance(field, str): + field = tables[-1].get_field(field, strict=True) + if not field.is_relationship: # also handles tree ranks + break + + tables.append(datamodel.get_table(field.relatedModelName, strict=True)) + return tables diff --git a/specifyweb/backend/stored_queries/queryfieldspec.py b/specifyweb/backend/stored_queries/queryfieldspec.py index eda7ebee439..286e9097758 100644 --- a/specifyweb/backend/stored_queries/queryfieldspec.py +++ b/specifyweb/backend/stored_queries/queryfieldspec.py @@ -18,6 +18,7 @@ from . import models from .query_ops import QueryOps +from .tree_rank import TreeRankQuery logger = logging.getLogger(__name__) @@ -130,41 +131,6 @@ def make_stringid(fs: "QueryFieldSpec", table_list: list[str]) -> tuple[list[str return table_list, fs.table.name.lower(), field_name.strip() -class TreeRankQuery(Relationship): - # FUTURE: used to remember what the previous value was. Useless after 6 retires - original_field: str - # This is used to query a particular treedef. If this is none, all treedefs are searched, otherwise a specific treedef is searched. - treedef_id: int | None - # Yeah this can be inferred from treedef_id but doing it this way avoids a database lookup because we already fetch it once. - treedef_name: str | None - - def __hash__(self): - return hash((TreeRankQuery, self.relatedModelName, self.name)) - - def __eq__(self, value): - return ( - isinstance(value, TreeRankQuery) - and value.name == self.name - and value.relatedModelName == self.relatedModelName - ) - - @staticmethod - def create(name, table_name, treedef_id=None, treedef_name=None): - obj = TreeRankQuery( - name=name, - relatedModelName=table_name, - type="many-to-one", - column=datamodel.get_table_strict(table_name).idFieldName - ) - obj.treedef_id = treedef_id - obj.treedef_name = treedef_name - return obj - - def get_workbench_name(self): - from specifyweb.backend.workbench.upload.treerecord import RANK_KEY_DELIMITER - # Treedef id included to make it easier to pass it to batch edit - return f"{self.treedef_name}{RANK_KEY_DELIMITER}{self.name}{RANK_KEY_DELIMITER}{self.treedef_id}" - def null_safe_not(field_expr, predicate): """Return a NOT clause that still matches NULL values on the target field. @@ -188,20 +154,6 @@ def null_safe_not(field_expr, predicate): FieldSpecJoinPath = tuple[QueryNode] -def tables_in_path(table, join_path): - path = deque(join_path) - tables = [table] - while len(path) > 0: - field = path.popleft() - if isinstance(field, str): - field = tables[-1].get_field(field, strict=True) - if not field.is_relationship: # also handles tree ranks - break - - tables.append(datamodel.get_table(field.relatedModelName, strict=True)) - return tables - - class QueryFieldSpec( namedtuple( "QueryFieldSpec", diff --git a/specifyweb/backend/stored_queries/tree_rank.py b/specifyweb/backend/stored_queries/tree_rank.py new file mode 100644 index 00000000000..cc3e2ceaa66 --- /dev/null +++ b/specifyweb/backend/stored_queries/tree_rank.py @@ -0,0 +1,39 @@ +from specifyweb.specify.datamodel import datamodel +from specifyweb.specify.models_utils.load_datamodel import Relationship + + +class TreeRankQuery(Relationship): + # FUTURE: used to remember what the previous value was. Useless after 6 retires + original_field: str + # This is used to query a particular treedef. If this is none, all treedefs are searched, otherwise a specific treedef is searched. + treedef_id: int | None + # Yeah this can be inferred from treedef_id but doing it this way avoids a database lookup because we already fetch it once. + treedef_name: str | None + + def __hash__(self): + return hash((TreeRankQuery, self.relatedModelName, self.name)) + + def __eq__(self, value): + return ( + isinstance(value, TreeRankQuery) + and value.name == self.name + and value.relatedModelName == self.relatedModelName + ) + + @staticmethod + def create(name, table_name, treedef_id=None, treedef_name=None): + obj = TreeRankQuery( + name=name, + relatedModelName=table_name, + type="many-to-one", + column=datamodel.get_table_strict(table_name).idFieldName, + ) + obj.treedef_id = treedef_id + obj.treedef_name = treedef_name + return obj + + def get_workbench_name(self): + from specifyweb.backend.workbench.upload.treerecord import RANK_KEY_DELIMITER + + # Treedef id included to make it easier to pass it to batch edit + return f"{self.treedef_name}{RANK_KEY_DELIMITER}{self.name}{RANK_KEY_DELIMITER}{self.treedef_id}" From 663823416d89683ca2d9b30a25139fbd99c1e7dd Mon Sep 17 00:00:00 2001 From: "Caroline D." <108160931+CarolineDenis@users.noreply.github.com> Date: Tue, 27 Oct 2026 16:25:53 +0100 Subject: [PATCH 31/47] Potential fix for pull request finding 'CodeQL / Cyclic import' Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com> --- specifyweb/backend/stored_queries/execution.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/specifyweb/backend/stored_queries/execution.py b/specifyweb/backend/stored_queries/execution.py index 0c3ed778834..1e63790386b 100644 --- a/specifyweb/backend/stored_queries/execution.py +++ b/specifyweb/backend/stored_queries/execution.py @@ -28,7 +28,6 @@ from . import models from .format import ObjectFormatter, ObjectFormatterProps from .field_spec_maps import transform_field_specs -from .query_construct_model import QueryConstruct from .query_path import tables_in_path from .web_portal_export import query_to_web_portal_zip as _query_to_web_portal_zip, WebportalQueryResultProcessors from specifyweb.backend.notifications.models import Message @@ -47,6 +46,7 @@ # import it would already be handled. if TYPE_CHECKING: from specifyweb.backend.stored_queries.queryfield import QueryField + from .query_construct_model import QueryConstruct logger = logging.getLogger(__name__) From 14ec429bce715424b43e5e6a3841431962e31101 Mon Sep 17 00:00:00 2001 From: "Caroline D." <108160931+CarolineDenis@users.noreply.github.com> Date: Tue, 27 Oct 2026 16:26:33 +0100 Subject: [PATCH 32/47] Potential fix for pull request finding 'CodeQL / Cyclic import' Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com> --- specifyweb/backend/stored_queries/queryfieldspec.py | 1 - 1 file changed, 1 deletion(-) diff --git a/specifyweb/backend/stored_queries/queryfieldspec.py b/specifyweb/backend/stored_queries/queryfieldspec.py index 286e9097758..1b25744b0a0 100644 --- a/specifyweb/backend/stored_queries/queryfieldspec.py +++ b/specifyweb/backend/stored_queries/queryfieldspec.py @@ -18,7 +18,6 @@ from . import models from .query_ops import QueryOps -from .tree_rank import TreeRankQuery logger = logging.getLogger(__name__) From b6baa4bdebf0105f942c56668e0c4d307140f596 Mon Sep 17 00:00:00 2001 From: "Caroline D." <108160931+CarolineDenis@users.noreply.github.com> Date: Tue, 27 Oct 2026 16:26:55 +0100 Subject: [PATCH 33/47] Potential fix for pull request finding 'CodeQL / Cyclic import' Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com> --- .../backend/stored_queries/query_construct_model.py | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/specifyweb/backend/stored_queries/query_construct_model.py b/specifyweb/backend/stored_queries/query_construct_model.py index 83256bddcf4..65657b86517 100644 --- a/specifyweb/backend/stored_queries/query_construct_model.py +++ b/specifyweb/backend/stored_queries/query_construct_model.py @@ -2,15 +2,18 @@ import logging from collections import namedtuple, deque +from typing import TYPE_CHECKING from sqlalchemy import orm, sql, or_ import specifyweb.specify.models as spmodels from specifyweb.backend.trees.utils import get_treedefs -from .tree_rank import TreeRankQuery from specifyweb.backend.stored_queries import models +if TYPE_CHECKING: + from .tree_rank import TreeRankQuery + logger = logging.getLogger(__name__) @@ -39,7 +42,7 @@ def handle_tree_field( self, node, table, - tree_rank: TreeRankQuery, + tree_rank: "TreeRankQuery", next_join_path, current_field_spec, ): @@ -153,8 +156,9 @@ def build_join(self, table, model, join_path): field = path.popleft() if isinstance(field, str): field = table.get_field(field, strict=True) + from . import tree_rank # basically, tree ranks act as forks. - if not field.is_relationship or isinstance(field, TreeRankQuery): + if not field.is_relationship or isinstance(field, tree_rank.TreeRankQuery): break next_table = spmodels.datamodel.get_table(field.relatedModelName, strict=True) logger.debug("joining: %r to %r via %r", table, next_table, field) From 7410664f19a6442b5f232bfaa64fb9be1bfba99d Mon Sep 17 00:00:00 2001 From: "Caroline D." <108160931+CarolineDenis@users.noreply.github.com> Date: Tue, 27 Oct 2026 16:27:11 +0100 Subject: [PATCH 34/47] Potential fix for pull request finding 'CodeQL / Cyclic import' Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com> --- specifyweb/backend/stored_queries/tree_rank.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/specifyweb/backend/stored_queries/tree_rank.py b/specifyweb/backend/stored_queries/tree_rank.py index cc3e2ceaa66..5c951beefd8 100644 --- a/specifyweb/backend/stored_queries/tree_rank.py +++ b/specifyweb/backend/stored_queries/tree_rank.py @@ -2,6 +2,9 @@ from specifyweb.specify.models_utils.load_datamodel import Relationship +RANK_KEY_DELIMITER = "::" + + class TreeRankQuery(Relationship): # FUTURE: used to remember what the previous value was. Useless after 6 retires original_field: str @@ -33,7 +36,5 @@ def create(name, table_name, treedef_id=None, treedef_name=None): return obj def get_workbench_name(self): - from specifyweb.backend.workbench.upload.treerecord import RANK_KEY_DELIMITER - # Treedef id included to make it easier to pass it to batch edit return f"{self.treedef_name}{RANK_KEY_DELIMITER}{self.name}{RANK_KEY_DELIMITER}{self.treedef_id}" From 4cc0670a5bb6aaafb603cf9a7237457d1b3d98d4 Mon Sep 17 00:00:00 2001 From: "Caroline D." <108160931+CarolineDenis@users.noreply.github.com> Date: Tue, 27 Oct 2026 16:31:25 +0100 Subject: [PATCH 35/47] Potential fix for pull request finding 'CodeQL / Cyclic import' Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com> --- specifyweb/backend/stored_queries/format.py | 1 - 1 file changed, 1 deletion(-) diff --git a/specifyweb/backend/stored_queries/format.py b/specifyweb/backend/stored_queries/format.py index 555cd52b1eb..7a96041aacc 100644 --- a/specifyweb/backend/stored_queries/format.py +++ b/specifyweb/backend/stored_queries/format.py @@ -34,7 +34,6 @@ from typing import NamedTuple, TYPE_CHECKING if TYPE_CHECKING: - from specifyweb.backend.stored_queries.queryfield import QueryField from .query_construct_model import QueryConstruct logger = logging.getLogger(__name__) From 198cb2ca8df2a1eec49ec691983a98eabae8e109 Mon Sep 17 00:00:00 2001 From: "Caroline D." <108160931+CarolineDenis@users.noreply.github.com> Date: Tue, 27 Oct 2026 18:50:23 +0100 Subject: [PATCH 36/47] Potential fix for pull request finding 'CodeQL / Cyclic import' Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com> --- specifyweb/backend/stored_queries/execution.py | 1 - 1 file changed, 1 deletion(-) diff --git a/specifyweb/backend/stored_queries/execution.py b/specifyweb/backend/stored_queries/execution.py index 1e63790386b..0a599164055 100644 --- a/specifyweb/backend/stored_queries/execution.py +++ b/specifyweb/backend/stored_queries/execution.py @@ -46,7 +46,6 @@ # import it would already be handled. if TYPE_CHECKING: from specifyweb.backend.stored_queries.queryfield import QueryField - from .query_construct_model import QueryConstruct logger = logging.getLogger(__name__) From 48c17283963675ca6021d6e7330bf4a017bc576c Mon Sep 17 00:00:00 2001 From: "Caroline D." <108160931+CarolineDenis@users.noreply.github.com> Date: Tue, 27 Oct 2026 18:53:17 +0100 Subject: [PATCH 37/47] Potential fix for pull request finding 'CodeQL / Cyclic import' Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com> --- specifyweb/backend/stored_queries/format.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/specifyweb/backend/stored_queries/format.py b/specifyweb/backend/stored_queries/format.py index 7a96041aacc..7f463de7146 100644 --- a/specifyweb/backend/stored_queries/format.py +++ b/specifyweb/backend/stored_queries/format.py @@ -31,10 +31,7 @@ from .group_concat import group_concat from .blank_nulls import blank_nulls from .queryfieldspec import QueryFieldSpec -from typing import NamedTuple, TYPE_CHECKING - -if TYPE_CHECKING: - from .query_construct_model import QueryConstruct +from typing import NamedTuple logger = logging.getLogger(__name__) From d576a09e4f87c4b4b65635b11e01941f65dba0b6 Mon Sep 17 00:00:00 2001 From: Caroline Denis Date: Tue, 27 Oct 2026 19:32:45 +0100 Subject: [PATCH 38/47] Fix: Add missing import --- specifyweb/backend/stored_queries/queryfieldspec.py | 1 + 1 file changed, 1 insertion(+) diff --git a/specifyweb/backend/stored_queries/queryfieldspec.py b/specifyweb/backend/stored_queries/queryfieldspec.py index 1b25744b0a0..286e9097758 100644 --- a/specifyweb/backend/stored_queries/queryfieldspec.py +++ b/specifyweb/backend/stored_queries/queryfieldspec.py @@ -18,6 +18,7 @@ from . import models from .query_ops import QueryOps +from .tree_rank import TreeRankQuery logger = logging.getLogger(__name__) From 4d316e21be1869858bca404ca05fcbf4cbfe8ce5 Mon Sep 17 00:00:00 2001 From: Caroline Denis Date: Tue, 27 Oct 2026 20:04:39 +0100 Subject: [PATCH 39/47] Add missing import --- specifyweb/backend/express_search/views.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/specifyweb/backend/express_search/views.py b/specifyweb/backend/express_search/views.py index 478e3c3d2db..2a7fd59f2e7 100644 --- a/specifyweb/backend/express_search/views.py +++ b/specifyweb/backend/express_search/views.py @@ -10,6 +10,7 @@ from django.http import HttpResponse, HttpResponseBadRequest from sqlalchemy.sql.expression import or_, and_ +from specifyweb.backend.stored_queries.query_construct_model import QueryConstruct from specifyweb.middleware.general import require_GET from .search_terms import parse_search_str from specifyweb.backend.context.app_resource import get_app_resource @@ -40,7 +41,7 @@ def build_primary_query(session, searchtable, terms, collection, user, as_scalar fields = [table.get_field(fn.text) for fn in searchtable.findall('.//searchfield/fieldName')] - query = build_query_construct( + query = QueryConstruct( collection=collection, objectformatter=ObjectFormatter(collection, user, False, props=DefaultQueryFormatterProps()), query=session.query(id_field), From f0c50329185de953f9d84fc30927cee240e368f0 Mon Sep 17 00:00:00 2001 From: "Caroline D." <108160931+CarolineDenis@users.noreply.github.com> Date: Tue, 27 Oct 2026 20:11:23 +0100 Subject: [PATCH 40/47] Potential fix for pull request finding 'CodeQL / Cyclic import' Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com> --- specifyweb/backend/express_search/views.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/specifyweb/backend/express_search/views.py b/specifyweb/backend/express_search/views.py index 2a7fd59f2e7..3d0276ff1c4 100644 --- a/specifyweb/backend/express_search/views.py +++ b/specifyweb/backend/express_search/views.py @@ -10,7 +10,6 @@ from django.http import HttpResponse, HttpResponseBadRequest from sqlalchemy.sql.expression import or_, and_ -from specifyweb.backend.stored_queries.query_construct_model import QueryConstruct from specifyweb.middleware.general import require_GET from .search_terms import parse_search_str from specifyweb.backend.context.app_resource import get_app_resource @@ -32,6 +31,8 @@ def get_express_search_config(collection, user): def build_primary_query(session, searchtable, terms, collection, user, as_scalar=False): + from specifyweb.backend.stored_queries.query_construct_model import QueryConstruct + table = datamodel.get_table(searchtable.find('tableName').text) check_table_permissions(collection, user, table, "read") From 5437b0941a06f7ac42155aafd3124540d5928c5e Mon Sep 17 00:00:00 2001 From: Caroline Denis Date: Tue, 27 Oct 2026 20:25:10 +0100 Subject: [PATCH 41/47] Add missing import --- specifyweb/backend/stored_queries/format.py | 1 + 1 file changed, 1 insertion(+) diff --git a/specifyweb/backend/stored_queries/format.py b/specifyweb/backend/stored_queries/format.py index 7f463de7146..6af3e9ecb1f 100644 --- a/specifyweb/backend/stored_queries/format.py +++ b/specifyweb/backend/stored_queries/format.py @@ -30,6 +30,7 @@ from .build_models import get_many_to_many_join_info from .group_concat import group_concat from .blank_nulls import blank_nulls +from .query_construct_model import QueryConstruct from .queryfieldspec import QueryFieldSpec from typing import NamedTuple From 8a6422599605d66a1dd7673bcf775322a987f12b Mon Sep 17 00:00:00 2001 From: Caroline Denis Date: Wed, 28 Oct 2026 10:01:02 +0100 Subject: [PATCH 42/47] Fix: Add query construct import --- specifyweb/backend/stored_queries/execution.py | 1 + 1 file changed, 1 insertion(+) diff --git a/specifyweb/backend/stored_queries/execution.py b/specifyweb/backend/stored_queries/execution.py index 0a599164055..87aa5ede229 100644 --- a/specifyweb/backend/stored_queries/execution.py +++ b/specifyweb/backend/stored_queries/execution.py @@ -16,6 +16,7 @@ from django.db import transaction from django.utils import timezone from specifyweb.backend.inheritance.api import DefaultQueryProcessors +from specifyweb.backend.stored_queries.query_construct_model import QueryConstruct from specifyweb.backend.stored_queries.utils import log_sqlalchemy_query from specifyweb.specify.utils.field_change_info import FieldChangeInfo from sqlalchemy import sql, orm, func, text From 1f46aa57a745ef5999925ec1640d056ac9c8f542 Mon Sep 17 00:00:00 2001 From: Caroline Denis Date: Wed, 28 Oct 2026 10:02:29 +0100 Subject: [PATCH 43/47] Fix: Use the existing workbench delimiter. --- specifyweb/backend/stored_queries/tree_rank.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/specifyweb/backend/stored_queries/tree_rank.py b/specifyweb/backend/stored_queries/tree_rank.py index 5c951beefd8..df8780a04e7 100644 --- a/specifyweb/backend/stored_queries/tree_rank.py +++ b/specifyweb/backend/stored_queries/tree_rank.py @@ -2,9 +2,6 @@ from specifyweb.specify.models_utils.load_datamodel import Relationship -RANK_KEY_DELIMITER = "::" - - class TreeRankQuery(Relationship): # FUTURE: used to remember what the previous value was. Useless after 6 retires original_field: str @@ -36,5 +33,6 @@ def create(name, table_name, treedef_id=None, treedef_name=None): return obj def get_workbench_name(self): + from specifyweb.backend.workbench.upload.treerecord import RANK_KEY_DELIMITER # Treedef id included to make it easier to pass it to batch edit return f"{self.treedef_name}{RANK_KEY_DELIMITER}{self.name}{RANK_KEY_DELIMITER}{self.treedef_id}" From 7c347517c6a2588ad6f4af784decd2abe10f5323 Mon Sep 17 00:00:00 2001 From: Caroline Denis Date: Wed, 28 Oct 2026 10:04:36 +0100 Subject: [PATCH 44/47] Fix: Clear the retry delay when cancelStatisticsRequests runs. --- .../lib/components/Statistics/requests.ts | 20 ++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/specifyweb/frontend/js_src/lib/components/Statistics/requests.ts b/specifyweb/frontend/js_src/lib/components/Statistics/requests.ts index 475440ee144..34ed4c37b40 100644 --- a/specifyweb/frontend/js_src/lib/components/Statistics/requests.ts +++ b/specifyweb/frontend/js_src/lib/components/Statistics/requests.ts @@ -11,6 +11,7 @@ import { const pending = new Set(); const listeners = new Set<() => void>(); let generation = 0; +const delayCancellers = new Set<() => void>(); const notify = (): void => listeners.forEach((listener) => listener()); @@ -33,6 +34,7 @@ export function useStatisticsLoading(): boolean { export function cancelStatisticsRequests(): void { generation += 1; + Array.from(delayCancellers).forEach((cancel) => cancel()); } export async function statisticsAjax( @@ -55,13 +57,17 @@ export async function statisticsAjax( const retryAfter = Number( result.response.headers.get('X-Specify-Retry-After') ); - await new Promise((resolve) => - globalThis.setTimeout( - resolve, - (Number.isFinite(retryAfter) && retryAfter > 0 ? retryAfter : 2) * - 1000 - ) - ); + const delay = + (Number.isFinite(retryAfter) && retryAfter > 0 ? retryAfter : 2) * 1000; + await new Promise((resolve) => { + const finish = (): void => { + globalThis.clearTimeout(timer); + delayCancellers.delete(finish); + resolve(); + }; + const timer = globalThis.setTimeout(finish, delay); + delayCancellers.add(finish); + }); } return undefined; } finally { From 7f46ebeda05e31277f663f4a7721e28b54f81bd9 Mon Sep 17 00:00:00 2001 From: Caroline Denis Date: Wed, 28 Oct 2026 10:16:28 +0100 Subject: [PATCH 45/47] Fix cycles --- specifyweb/backend/stored_queries/execution.py | 2 +- .../backend/stored_queries/query_construct_model.py | 10 +++------- specifyweb/backend/stored_queries/tree_rank.py | 2 +- 3 files changed, 5 insertions(+), 9 deletions(-) diff --git a/specifyweb/backend/stored_queries/execution.py b/specifyweb/backend/stored_queries/execution.py index 87aa5ede229..62862cabfe6 100644 --- a/specifyweb/backend/stored_queries/execution.py +++ b/specifyweb/backend/stored_queries/execution.py @@ -16,7 +16,6 @@ from django.db import transaction from django.utils import timezone from specifyweb.backend.inheritance.api import DefaultQueryProcessors -from specifyweb.backend.stored_queries.query_construct_model import QueryConstruct from specifyweb.backend.stored_queries.utils import log_sqlalchemy_query from specifyweb.specify.utils.field_change_info import FieldChangeInfo from sqlalchemy import sql, orm, func, text @@ -47,6 +46,7 @@ # import it would already be handled. if TYPE_CHECKING: from specifyweb.backend.stored_queries.queryfield import QueryField + from specifyweb.backend.stored_queries.query_construct_model import QueryConstruct logger = logging.getLogger(__name__) diff --git a/specifyweb/backend/stored_queries/query_construct_model.py b/specifyweb/backend/stored_queries/query_construct_model.py index 65657b86517..26e0cdc2428 100644 --- a/specifyweb/backend/stored_queries/query_construct_model.py +++ b/specifyweb/backend/stored_queries/query_construct_model.py @@ -2,7 +2,6 @@ import logging from collections import namedtuple, deque -from typing import TYPE_CHECKING from sqlalchemy import orm, sql, or_ @@ -11,9 +10,6 @@ from specifyweb.backend.stored_queries import models -if TYPE_CHECKING: - from .tree_rank import TreeRankQuery - logger = logging.getLogger(__name__) @@ -42,7 +38,7 @@ def handle_tree_field( self, node, table, - tree_rank: "TreeRankQuery", + tree_rank: "object", next_join_path, current_field_spec, ): @@ -156,9 +152,9 @@ def build_join(self, table, model, join_path): field = path.popleft() if isinstance(field, str): field = table.get_field(field, strict=True) - from . import tree_rank # basically, tree ranks act as forks. - if not field.is_relationship or isinstance(field, tree_rank.TreeRankQuery): + is_tree_rank_query = field.__class__.__name__ == "TreeRankQuery" + if not field.is_relationship or is_tree_rank_query: break next_table = spmodels.datamodel.get_table(field.relatedModelName, strict=True) logger.debug("joining: %r to %r via %r", table, next_table, field) diff --git a/specifyweb/backend/stored_queries/tree_rank.py b/specifyweb/backend/stored_queries/tree_rank.py index df8780a04e7..1ba8dfeda9d 100644 --- a/specifyweb/backend/stored_queries/tree_rank.py +++ b/specifyweb/backend/stored_queries/tree_rank.py @@ -1,6 +1,7 @@ from specifyweb.specify.datamodel import datamodel from specifyweb.specify.models_utils.load_datamodel import Relationship +RANK_KEY_DELIMITER = ":::" class TreeRankQuery(Relationship): # FUTURE: used to remember what the previous value was. Useless after 6 retires @@ -33,6 +34,5 @@ def create(name, table_name, treedef_id=None, treedef_name=None): return obj def get_workbench_name(self): - from specifyweb.backend.workbench.upload.treerecord import RANK_KEY_DELIMITER # Treedef id included to make it easier to pass it to batch edit return f"{self.treedef_name}{RANK_KEY_DELIMITER}{self.name}{RANK_KEY_DELIMITER}{self.treedef_id}" From 6478516eb0349d5428dafabb4027216b571e6f17 Mon Sep 17 00:00:00 2001 From: Caroline Denis Date: Wed, 28 Oct 2026 13:41:42 +0100 Subject: [PATCH 46/47] Fix: delimiter mismatch --- specifyweb/backend/stored_queries/execution.py | 2 ++ specifyweb/backend/stored_queries/tree_rank.py | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/specifyweb/backend/stored_queries/execution.py b/specifyweb/backend/stored_queries/execution.py index 62862cabfe6..d1df3b97b3b 100644 --- a/specifyweb/backend/stored_queries/execution.py +++ b/specifyweb/backend/stored_queries/execution.py @@ -983,6 +983,8 @@ def build_query_construct_base( model, props: BuildQueryProps, ): + from .query_construct_model import QueryConstruct + id_field = model._id catalognumber_field = model.catalogNumber if hasattr(model, 'catalogNumber') else None query_construct_query = session.query(id_field) diff --git a/specifyweb/backend/stored_queries/tree_rank.py b/specifyweb/backend/stored_queries/tree_rank.py index 1ba8dfeda9d..ee2d9b577ad 100644 --- a/specifyweb/backend/stored_queries/tree_rank.py +++ b/specifyweb/backend/stored_queries/tree_rank.py @@ -1,7 +1,7 @@ from specifyweb.specify.datamodel import datamodel from specifyweb.specify.models_utils.load_datamodel import Relationship -RANK_KEY_DELIMITER = ":::" +RANK_KEY_DELIMITER = "~>" class TreeRankQuery(Relationship): # FUTURE: used to remember what the previous value was. Useless after 6 retires From 4ccbb041931e4f79b7f8ee1f69a1badf323d06f9 Mon Sep 17 00:00:00 2001 From: Caroline Denis Date: Wed, 28 Oct 2026 14:05:47 +0100 Subject: [PATCH 47/47] Refactor: Extract users_collections_for_sp6 --- specifyweb/backend/businessrules/exceptions.py | 2 -- specifyweb/backend/context/collection_access.py | 10 ++++++++++ specifyweb/backend/context/views.py | 12 +----------- specifyweb/backend/permissions/initialize.py | 2 +- 4 files changed, 12 insertions(+), 14 deletions(-) create mode 100644 specifyweb/backend/context/collection_access.py diff --git a/specifyweb/backend/businessrules/exceptions.py b/specifyweb/backend/businessrules/exceptions.py index da05e0bb215..52423786519 100644 --- a/specifyweb/backend/businessrules/exceptions.py +++ b/specifyweb/backend/businessrules/exceptions.py @@ -1,5 +1,3 @@ -from specifyweb.middleware.general import SpecifyExceptionWrapper - class BusinessRuleException(Exception): pass diff --git a/specifyweb/backend/context/collection_access.py b/specifyweb/backend/context/collection_access.py new file mode 100644 index 00000000000..c4755e10997 --- /dev/null +++ b/specifyweb/backend/context/collection_access.py @@ -0,0 +1,10 @@ +def users_collections_for_sp6(cursor, user_id): + cursor.execute(""" + select distinct c.usergroupscopeid, c.collectionname from collection c + inner join spprincipal p on p.usergroupscopeid = c.usergroupscopeid + inner join specifyuser_spprincipal up on up.spprincipalid = p.spprincipalid + inner join specifyuser u on u.specifyuserid = up.specifyuserid and p.grouptype is null + where up.specifyuserid = %s + """, [user_id]) + + return list(cursor.fetchall()) diff --git a/specifyweb/backend/context/views.py b/specifyweb/backend/context/views.py index 53e3a98c03f..bf62138d318 100644 --- a/specifyweb/backend/context/views.py +++ b/specifyweb/backend/context/views.py @@ -41,6 +41,7 @@ from .app_resource import get_app_resource, FORM_RESOURCE_EXCLUDED_LST from .schema_localization import get_schema_languages, get_schema_localization from .viewsets import get_views +from .collection_access import users_collections_for_sp6 from specifyweb.backend.setup_tool.api import ( get_config_progress, filter_ready_disciplines_for_config_tasks, @@ -53,17 +54,6 @@ def set_collection_cookie(response, collection_id): # pragma: no cover response.set_cookie('collection', str(collection_id), max_age=365*24*60*60) -def users_collections_for_sp6(cursor, user_id): # pragma: no cover - cursor.execute(""" - select distinct c.usergroupscopeid, c.collectionname from collection c - inner join spprincipal p on p.usergroupscopeid = c.usergroupscopeid - inner join specifyuser_spprincipal up on up.spprincipalid = p.spprincipalid - inner join specifyuser u on u.specifyuserid = up.specifyuserid and p.grouptype is null - where up.specifyuserid = %s - """, [user_id]) - - return list(cursor.fetchall()) - def users_collections_for_sp7(userid: int) -> list: # pragma: no cover return [ c diff --git a/specifyweb/backend/permissions/initialize.py b/specifyweb/backend/permissions/initialize.py index 70182afa58b..d3bbc6e1f04 100644 --- a/specifyweb/backend/permissions/initialize.py +++ b/specifyweb/backend/permissions/initialize.py @@ -173,7 +173,7 @@ def assign_users_to_roles(apps=apps) -> None: logger.info(f"Assigned user {user_name} to role {role_name} for collection {collection_name}.") def assign_users_to_roles_during_testing(apps=apps) -> None: - from specifyweb.backend.context.views import users_collections_for_sp6 + from specifyweb.backend.context.collection_access import users_collections_for_sp6 Role = apps.get_model('permissions', 'Role') UserPolicy = apps.get_model('permissions', 'UserPolicy')