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 fb972295cf5..ae88c240252 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/express_search/views.py b/specifyweb/backend/express_search/views.py index 633763e8dd0..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 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") 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') diff --git a/specifyweb/backend/stats/coordination.py b/specifyweb/backend/stats/coordination.py new file mode 100644 index 00000000000..86a5bb12471 --- /dev/null +++ b/specifyweb/backend/stats/coordination.py @@ -0,0 +1,120 @@ +import hashlib +import json +import logging +from collections.abc import Callable +from typing import cast +from contextlib import contextmanager + +from django.http import JsonResponse +from redis.exceptions import RedisError + +from specifyweb.backend.cache.redis.connect import RedisString, RedisConnection +from specifyweb.backend.permissions.permissions import check_table_permissions +from specifyweb.specify.models_utils.lock_tables import LockDispatcher + +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["X-Specify-Retry-After"] = str(STATISTICS_RETRY_SECONDS) + return response + + +def statistics_result( + collection, + user, + query_key: str, + calculate: Callable[[], str], +) -> str: + """Reuse cached statistics without handling query validity. + + 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. + """ + try: + redis_cache = RedisString(RedisConnection()) + except (ValueError, RedisError): + logger.warning("Statistics cache unavailable; calculating without caching.", exc_info=True) + with run_statistics_query(calculate) as result: + return result + + digest = hashlib.sha256(query_key.encode()).hexdigest() + redis_cache_key = f"statistics:{collection.id}:{user.id}:{digest}" + + skip_caching = False + try: + cached_value = redis_cache.get(redis_cache_key) + except RedisError: + logger.warning("Statistics cache read failed; calculating without caching.", exc_info=True) + cached_value = None + skip_caching = True + + if cached_value is not None: + # We know this should be a string because the RedisConnection has + # decode_responses=True and we should only be setting strings to this + # key + return cast(str, cached_value) + + with run_statistics_query(calculate) as result: + if skip_caching: + return result + try: + redis_cache.set(redis_cache_key, result, time_to_live=STATISTICS_CACHE_SECONDS) + except RedisError: + logger.warning("Statistics cache write failed; returning uncached result.", exc_info=True) + return result + +@contextmanager +def run_statistics_query(calculate: Callable[[], str]): + """ + This context manager will hold statistics locks until your code completely + exits. + If statistics locks are already being held, this will instead raise a + StatisticsBusy exception. + + Example: + ```py + with run_statistics_query(calculate) as result: + # if the locks are already being held, StatisticsBusy is raised + # otherwise, statistics locks are acquired and calculate is ran + # do something with the final result here + print("out of block, statistics locks are released") + # once the with block is exited, the statistics locks will be released + ``` + """ + # A database advisory lock has no lease that could expire during a slow query. + # Keep the lock database-scoped while letting the dispatcher bound its name. + with LockDispatcher(lock_name_separator=":") as locks: + try: + locks.acquire("statistics", timeout=0) + except TimeoutError as error: + logger.debug("Deferring statistics calculation: %s", error) + raise StatisticsBusy from error + + result = calculate() + yield result + +def statistics_content(request, tables, calculate: Callable[[], str]) -> str: + 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, + ) diff --git a/specifyweb/backend/stats/tests.py b/specifyweb/backend/stats/tests.py new file mode 100644 index 00000000000..2a5844641cf --- /dev/null +++ b/specifyweb/backend/stats/tests.py @@ -0,0 +1,583 @@ +import hashlib +import json +from types import SimpleNamespace +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, + busy_response, + statistics_content, + 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.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.specify.models_utils.lock_tables.Lock") + self.redis_connection = 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 + ) + + 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() + 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) + 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_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] + full_name = f"{database_name}:statistics" + self.assertEqual( + lock_name, + 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() + self.lock.acquire.assert_called_once_with() + self.lock.release.assert_called_once_with() + + 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_called_once_with() + + 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.calculate.assert_not_called() + self.lock.acquire.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_called_once_with() + response = busy_response() + self.assertEqual(response.status_code, 409) + 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): + 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") + 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.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})) + self.cache.get.return_value = '{"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", + 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, + ) + 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 + response = collection_preparations(request) + self.assertEqual(response.status_code, 409) + statistics_content_mock.assert_called_once() + + 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 + + @staticmethod + def ephemeral_query(): + return { + "contexttableid": 1, + "selectdistinct": False, + "countonly": True, + "fields": [{ + "position": 0, + "stringid": "1.collectionobject.catalogNumber", + "isrelfld": False, + }], + } + + @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): + from specifyweb.backend.stored_queries.views import ephemeral + + 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() + 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], + }) + from specifyweb.backend.stored_queries.views import ephemeral_statistics_cache_key + + expected_digest = hashlib.sha256( + 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") + 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 + 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}) + 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.views.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 = self.ephemeral_query() + get_query.return_value = query, self.collection + 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_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.run_ephemeral_query", + side_effect=ValueError("Invalid query"), + ) + @patch("specifyweb.backend.stored_queries.views.get_query") + def test_ephemeral_query_errors_propagate_without_caching(self, get_query, execute): + from specifyweb.backend.stored_queries.views import ephemeral + + query = self.ephemeral_query() + get_query.return_value = query, self.collection + with self.assertRaisesRegex(ValueError, "Invalid query"): + ephemeral(self.ephemeral_request()) + 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.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( + 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) + 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") + 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 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_path 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_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()}), + collection=Mock(), + user=Mock(), + tableid=1, + distinct=False, + series=False, + search_synonymy=False, + count_only=True, + query_fields=[], + limit=0, + offset=0, + ) + self.assertEqual(result, {"count": 12}) + setup.assert_called_once() + build.assert_called_once() + query.count.assert_called_once_with() + query.all.assert_not_called() diff --git a/specifyweb/backend/stats/views.py b/specifyweb/backend/stats/views.py index 6b447ce0848..c034560b573 100644 --- a/specifyweb/backend/stats/views.py +++ b/specifyweb/backend/stats/views.py @@ -1,15 +1,13 @@ 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 StatisticsBusy, busy_response, statistics_content @login_maybe_required @openapi(schema={ @@ -29,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 @@ -74,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) @@ -99,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() @@ -162,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) diff --git a/specifyweb/backend/stored_queries/batch_edit.py b/specifyweb/backend/stored_queries/batch_edit.py index 7aad9cadbde..6631a1120e8 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 cc893fb1516..d1df3b97b3b 100644 --- a/specifyweb/backend/stored_queries/execution.py +++ b/specifyweb/backend/stored_queries/execution.py @@ -27,8 +27,8 @@ from . import models from .format import ObjectFormatter, ObjectFormatterProps -from .query_construct import QueryConstruct from .field_spec_maps import transform_field_specs +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 @@ -46,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__) @@ -677,6 +678,25 @@ 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. + """ + for table_to_read in get_query_tables(query_fields): + 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. @@ -963,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) @@ -1176,16 +1198,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/format.py b/specifyweb/backend/stored_queries/format.py index 6837a5af4b7..6af3e9ecb1f 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 @@ -24,13 +25,12 @@ 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 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..ad124533380 100644 --- a/specifyweb/backend/stored_queries/group_concat.py +++ b/specifyweb/backend/stored_queries/group_concat.py @@ -1,14 +1,16 @@ +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 Any + 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 import QueryConstruct - class group_concat(expression.ColumnElement): # MySQL GROUP_CONCAT(expr [ORDER BY ...] [SEPARATOR ...]) name = "group_concat" @@ -41,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 diff --git a/specifyweb/backend/stored_queries/query_construct.py b/specifyweb/backend/stored_queries/query_construct_model.py similarity index 71% rename from specifyweb/backend/stored_queries/query_construct.py rename to specifyweb/backend/stored_queries/query_construct_model.py index 569e3fc0466..26e0cdc2428 100644 --- a/specifyweb/backend/stored_queries/query_construct.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,28 +8,40 @@ import specifyweb.specify.models as spmodels from specifyweb.backend.trees.utils import get_treedefs -from .queryfieldspec import TreeRankQuery, QueryFieldSpec from specifyweb.backend.stored_queries import models logger = logging.getLogger(__name__) + def _safe_filter(query): count = query.count() if count <= 1: return query.first() raise Exception(f"Got more than one matching: {list(query)}") -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: "object", + next_join_path, + current_field_spec, + ): query = self query_before_tree_joins = query if query.collection is None: # Not sure it makes sense to query across collections @@ -35,12 +49,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] @@ -64,12 +83,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( @@ -83,19 +113,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) @@ -112,20 +144,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) @@ -135,7 +153,8 @@ def build_join(self, table, model, join_path): if isinstance(field, str): field = table.get_field(field, strict=True) # basically, tree ranks act as forks. - if not field.is_relationship or isinstance(field, 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) @@ -153,7 +172,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): @@ -165,11 +183,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/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 2e66a451d71..599a966b602 100644 --- a/specifyweb/backend/stored_queries/queryfieldspec.py +++ b/specifyweb/backend/stored_queries/queryfieldspec.py @@ -20,6 +20,7 @@ from . import models from .query_ops import QueryOps +from .tree_rank import TreeRankQuery logger = logging.getLogger(__name__) @@ -132,41 +133,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. @@ -189,6 +155,7 @@ def null_safe_not(field_expr, predicate): QueryNode = Field | Relationship | TreeRankQuery FieldSpecJoinPath = tuple[QueryNode] + 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 0637a293e4c..4c86f3e4fb0 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 diff --git a/specifyweb/backend/stored_queries/tree_rank.py b/specifyweb/backend/stored_queries/tree_rank.py new file mode 100644 index 00000000000..ee2d9b577ad --- /dev/null +++ b/specifyweb/backend/stored_queries/tree_rank.py @@ -0,0 +1,38 @@ +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 + 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): + # 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}" diff --git a/specifyweb/backend/stored_queries/views.py b/specifyweb/backend/stored_queries/views.py index 70c5240a718..dfffd2d8ff9 100644 --- a/specifyweb/backend/stored_queries/views.py +++ b/specifyweb/backend/stored_queries/views.py @@ -12,19 +12,23 @@ 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 +from specifyweb.specify.datamodel import datamodel 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, get_query_tables logger = logging.getLogger(__name__) @@ -72,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 @@ -115,9 +151,39 @@ 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": + 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, + ephemeral_statistics_cache_key(spquery), + calculate_statistics, + ) + except StatisticsBusy: + return busy_response() + 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) return HttpResponse(toJson(data), content_type='application/json') @@ -356,4 +422,3 @@ def return_loan_preps(request): item['loanpreparations'].append({'loanpreparationid': lp_id, 'quantity': int(quantity)}) return JsonResponse(resp, safe=False) - 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/__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(); +}); 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..7228330751d --- /dev/null +++ b/specifyweb/frontend/js_src/lib/components/Statistics/__tests__/requests.test.ts @@ -0,0 +1,123 @@ +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: new Response(undefined, { + status, + headers: + retryAfter === undefined ? {} : { 'X-Specify-Retry-After': retryAfter }, + }), +}); + +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 the Specify retry header 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/components/Statistics/hooks.tsx b/specifyweb/frontend/js_src/lib/components/Statistics/hooks.tsx index 5c25d5ab521..3b8b6efe941 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: Object.values(Http), + }).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..34ed4c37b40 --- /dev/null +++ b/specifyweb/frontend/js_src/lib/components/Statistics/requests.ts @@ -0,0 +1,77 @@ +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 delayCancellers = new Set<() => void>(); + +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; + Array.from(delayCancellers).forEach((cancel) => cancel()); +} + +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('X-Specify-Retry-After') + ); + 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 { + pending.delete(token); + notify(); + } +} 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, + }, + } + ); }, }); 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']); +}); 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; } diff --git a/specifyweb/specify/models_utils/lock_tables.py b/specifyweb/specify/models_utils/lock_tables.py index ef6762b0c17..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 @@ -200,7 +203,13 @@ 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_name_separator: str = LOCK_NAME_SEPARATOR, + ): db_name = getattr(settings, "DATABASE_NAME") self.lock_prefix_parts: list[str] = [db_name] @@ -208,6 +217,7 @@ def __init__(self, lock_prefix: str | None = None, case_sensitive_names=False): 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,10 +233,21 @@ 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) + ) + + 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.lower() if not self.case_sensitive_names else final_name + return final_name @contextmanager def lock_and_release(self, name: str, timeout: int = 5):