From e901d88b2a5b4c0a2525bf4d2201c74224da3030 Mon Sep 17 00:00:00 2001 From: melton-jason Date: Thu, 17 Sep 2026 10:57:56 -0500 Subject: [PATCH 1/7] refactor: use consistent naming for query_fields and reduce debug log output --- specifyweb/backend/inheritance/api.py | 43 ++++---- .../backend/stored_queries/batch_edit.py | 2 +- .../backend/stored_queries/execution.py | 104 ++++++++---------- .../backend/stored_queries/field_spec_maps.py | 27 +++++ .../backend/stored_queries/geology_time.py | 4 +- .../stored_queries/relative_date_utils.py | 7 -- .../tests/test_execution/test_execute.py | 20 ++-- .../tests/test_relative_date_utils.py | 3 +- specifyweb/backend/stored_queries/views.py | 2 +- 9 files changed, 113 insertions(+), 99 deletions(-) diff --git a/specifyweb/backend/inheritance/api.py b/specifyweb/backend/inheritance/api.py index ce739d8856a..72f1e80b5db 100644 --- a/specifyweb/backend/inheritance/api.py +++ b/specifyweb/backend/inheritance/api.py @@ -1,15 +1,16 @@ -from typing import Callable +from typing import Callable, Iterable from specifyweb.backend.inheritance.utils import get_cat_num_inheritance_setting, get_parent_cat_num_inheritance_setting from specifyweb.specify.models import Collectionobjectgroupjoin, Component +from specifyweb.backend.stored_queries.queryfield import QueryField def do_nothing[T](items: T) -> T: return items -def parent_inheritance_query_processor(tableid, field_specs, collection, user) -> Callable[[list], list]: - first_field_names = [fs.fieldspec.join_path[0].name for fs in field_specs if fs.fieldspec.join_path] +def parent_inheritance_query_processor(tableid: int, query_fields: list[QueryField], collection, user) -> Callable[[list], list]: + first_field_names = [qf.fieldspec.join_path[0].name for qf in query_fields if qf.fieldspec.join_path] if tableid != 1029 or 'catalogNumber' not in first_field_names: return do_nothing @@ -20,7 +21,7 @@ def parent_inheritance_query_processor(tableid, field_specs, collection, user) - catalog_number_field_index = first_field_names.index('catalogNumber') + 1 # op_num 1 is refering to the filter equal, the inheritance will only work if we have cat num equal, other operators will not function - if field_specs[catalog_number_field_index - 1].op_num != 1: + if query_fields[catalog_number_field_index - 1].op_num != 1: return do_nothing def _processor(row: list): @@ -39,11 +40,11 @@ def _processor(row: list): return _processor -def cog_inheritance_query_processor(tableid, field_specs, collection, user) -> Callable[[list], list]: +def cog_inheritance_query_processor(tableid: int, query_fields: list[QueryField], collection, user) -> Callable[[list], list]: first_field_names = [ - fs.fieldspec.join_path[0].name.lower() - for fs in field_specs - if fs.fieldspec.join_path + qf.fieldspec.join_path[0].name.lower() + for qf in query_fields + if qf.fieldspec.join_path ] if tableid != 1 or 'catalognumber' not in first_field_names: return do_nothing @@ -55,7 +56,7 @@ def cog_inheritance_query_processor(tableid, field_specs, collection, user) -> C catalog_number_field_index = first_field_names.index('catalognumber') + 1 # op_num 1 is refering to the filter equal, the inheritance will only work if we have cat num equal, other operators will not function - if field_specs[catalog_number_field_index - 1].op_num != 1: + if query_fields[catalog_number_field_index - 1].op_num != 1: return do_nothing # For a given result, replace null catalog numbers with the collection @@ -81,15 +82,19 @@ def _processor(row: list): return _processor -def DefaultQueryProcessors(tableid, field_specs, collection, user) -> list[Callable[[list], list]]: - visible_field_specs = list(filter(lambda qfield: qfield.display, field_specs)) - kwargs = { - "tableid": tableid, - "field_specs": visible_field_specs, - "collection": collection, - "user": user - } +def DefaultQueryProcessors(tableid: int, query_fields: Iterable[QueryField], collection, user) -> list[Callable[[list], list]]: + visible_query_fields = list(filter(lambda qfield: qfield.display, query_fields)) return [ - parent_inheritance_query_processor(**kwargs), - cog_inheritance_query_processor(**kwargs) + parent_inheritance_query_processor( + tableid=tableid, + query_fields=visible_query_fields, + collection=collection, + user=user + ), + cog_inheritance_query_processor( + tableid=tableid, + query_fields=visible_query_fields, + collection=collection, + user=user + ) ] diff --git a/specifyweb/backend/stored_queries/batch_edit.py b/specifyweb/backend/stored_queries/batch_edit.py index a6fcc271c82..9f6a2906dca 100644 --- a/specifyweb/backend/stored_queries/batch_edit.py +++ b/specifyweb/backend/stored_queries/batch_edit.py @@ -1017,7 +1017,7 @@ def run_batch_edit_query(props: BatchEditProps): series=False, search_synonymy=False, count_only=False, - field_specs=query_with_hidden, + query_fields=query_with_hidden, limit=limit, offset=offset, recordsetid=recordsetid, diff --git a/specifyweb/backend/stored_queries/execution.py b/specifyweb/backend/stored_queries/execution.py index cde46621e8e..f33504c2961 100644 --- a/specifyweb/backend/stored_queries/execution.py +++ b/specifyweb/backend/stored_queries/execution.py @@ -5,7 +5,7 @@ import re import traceback -from typing import Callable, Literal, NamedTuple +from typing import Callable, Literal, NamedTuple, Iterable import xml.dom.minidom from collections import namedtuple, defaultdict from functools import reduce @@ -26,9 +26,9 @@ from . import models from .format import ObjectFormatter, ObjectFormatterProps from .query_construct import QueryConstruct -from .relative_date_utils import apply_absolute_date -from .field_spec_maps import apply_specify_user_name +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.stored_queries.queryfield import QueryField from specifyweb.backend.notifications.models import Message from specifyweb.backend.permissions.permissions import check_table_permissions from specifyweb.specify.models import Loan, Loanpreparation, Loanreturnpreparation, Taxontreedef @@ -82,17 +82,17 @@ def set_group_concat_max_len(connection): """ connection.execute("SET group_concat_max_len = 1024 * 1024 * 1024") -def _pick_synonymy_table(field_specs, base_table): +def _pick_synonymy_table(query_fields: list[QueryField], base_table): if base_table is not None and is_tree_table(base_table): return base_table - for field_spec in field_specs: - if field_spec.fieldspec.contains_tree_rank(): - return field_spec.fieldspec.table + for query_field in query_fields: + if query_field.fieldspec.contains_tree_rank(): + return query_field.fieldspec.table - for field_spec in field_specs: - if is_tree_table(field_spec.fieldspec.table): - return field_spec.fieldspec.table + for query_field in query_fields: + if is_tree_table(query_field.fieldspec.table): + return query_field.fieldspec.table return None @@ -327,14 +327,14 @@ def query_to_web_portal_zip( collection=collection, user=user, tableid=tableid, - field_specs=field_specs, + query_fields=field_specs, props=BuildQueryProps(recordsetid=recordsetid, replace_nulls=True, distinct=distinct), ) processors = [ *DefaultQueryProcessors( tableid=tableid, - field_specs=field_specs, + query_fields=field_specs, collection=collection, user=user ), @@ -414,7 +414,7 @@ def query_to_csv( query=query, processors=DefaultQueryProcessors( tableid=tableid, - field_specs=field_specs, + query_fields=field_specs, collection=collection, user=user ) @@ -492,7 +492,7 @@ def query_to_kml( query=query, processors=DefaultQueryProcessors( tableid=tableid, - field_specs=field_specs, + query_fields=field_specs, collection=collection, user=user ) @@ -695,7 +695,7 @@ def run_ephemeral_query(collection, user, spquery): series=series, search_synonymy=search_synonymy, count_only=count_only, - field_specs=field_specs, + query_fields=field_specs, limit=limit, offset=offset, recordsetid=recordsetid, @@ -853,12 +853,12 @@ def execute( session, collection, user, - tableid, - distinct, - series, - search_synonymy, - count_only, - field_specs, + tableid: int, + distinct: bool, + series: bool, + search_synonymy: bool, + count_only: bool, + query_fields: Iterable[QueryField], limit, offset, recordsetid=None, @@ -876,7 +876,7 @@ def execute( collection, user, tableid, - field_specs, + query_fields, BuildQueryProps( recordsetid=recordsetid, formatauditobjs=formatauditobjs, @@ -887,12 +887,14 @@ def execute( ), ) + log_sqlalchemy_query(query) + if count_only: if series: cat_num_sort_type = 0 - for field_spec in field_specs: - if field_spec.fieldspec.get_field() and field_spec.fieldspec.get_field().name.lower() == 'catalognumber': - cat_num_sort_type = field_spec.sort_type + for query_field in query_fields: + if query_field.fieldspec.get_field() and query_field.fieldspec.get_field().name.lower() == 'catalognumber': + cat_num_sort_type = query_field.sort_type break return {'count': len(series_post_query(query, limit=SERIES_MAX_ROWS, offset=0, sort_type=cat_num_sort_type, is_count=True))} else: @@ -901,10 +903,10 @@ def execute( cat_num_col_id = None cat_num_sort_type = None idx = 0 - for field_spec in field_specs: - if field_spec.fieldspec.get_field() and field_spec.fieldspec.get_field().name.lower() == 'catalognumber': + for query_field in query_fields: + if query_field.fieldspec.get_field() and query_field.fieldspec.get_field().name.lower() == 'catalognumber': cat_num_col_id = idx - cat_num_sort_type = field_spec.sort_type + cat_num_sort_type = query_field.sort_type break idx += 1 is_valid_series_query = series and \ @@ -928,17 +930,12 @@ def execute( if limit: query = query.limit(limit) - log_sqlalchemy_query(query) - - - log_sqlalchemy_query(query) # Debugging - results = list( apply_special_post_query_processing( query=query, processors=DefaultQueryProcessors( tableid=tableid, - field_specs=field_specs, + query_fields=query_fields, collection=collection, user=user ) @@ -950,8 +947,8 @@ def build_query( session, collection, user, - tableid, - field_specs, + tableid: int, + query_fields: Iterable[QueryField], props: BuildQueryProps = BuildQueryProps(), ): """Build a sqlalchemy query using the QueryField objects given by @@ -990,8 +987,7 @@ def build_query( id_field = model._id catalog_number_field = model.catalogNumber if hasattr(model, 'catalogNumber') else None - field_specs = [apply_absolute_date(field_spec) for field_spec in field_specs] - field_specs = [apply_specify_user_name(field_spec, user) for field_spec in field_specs] + query_fields = list(transform_field_specs(query_fields, user)) query_construct_query = session.query(id_field) if props.series and catalog_number_field: @@ -1023,9 +1019,9 @@ def build_query( tables_to_read = { table - for fs in field_specs + for field in query_fields for table in query.tables_in_path( - fs.fieldspec.root_table, fs.fieldspec.join_path + field.fieldspec.root_table, field.fieldspec.join_path ) } @@ -1071,17 +1067,15 @@ def build_query( order_by_exprs = [] selected_fields = [] predicates_by_field = defaultdict(list) - # augment_field_specs(field_specs, formatauditobjs) - for fs in field_specs: - # sort_type = SORT_TYPES[fs.sort_type] - sort_type = QuerySort.by_id(fs.sort_type) - - if props.series and fs.fieldspec.get_field() and fs.fieldspec.get_field().name.lower() == 'catalognumber': - _, _, predicate = fs.add_to_query(query, formatauditobjs=props.formatauditobjs) - predicates_by_field[fs.fieldspec].append(predicate) if predicate is not None else None + for query_field in query_fields: + sort_type = QuerySort.by_id(query_field.sort_type) + + if props.series and query_field.fieldspec.get_field() and query_field.fieldspec.get_field().name.lower() == 'catalognumber': + _, _, predicate = query_field.add_to_query(query, formatauditobjs=props.formatauditobjs) + predicates_by_field[query_field.fieldspec].append(predicate) if predicate is not None else None continue - query, field, predicate = fs.add_to_query( + query, field, predicate = query_field.add_to_query( query, formatauditobjs=props.formatauditobjs, collection=collection, user=user ) @@ -1089,8 +1083,8 @@ def build_query( continue formatted_field = None - if fs.display: - formatted_field = query.objectformatter.fieldformat(fs, field) + if query_field.display: + formatted_field = query.objectformatter.fieldformat(query_field, field) query = query.add_columns(formatted_field) selected_fields.append(formatted_field) @@ -1102,7 +1096,7 @@ def build_query( order_by_exprs.append(sort_type(field)) if predicate is not None: - predicates_by_field[fs.fieldspec].append(predicate) + predicates_by_field[query_field.fieldspec].append(predicate) if props.implicit_or: implicit_ors = [ @@ -1122,18 +1116,16 @@ def build_query( query = group_by_displayed_fields(query, selected_fields) if props.search_synonymy: - synonymy_table = _pick_synonymy_table(field_specs, base_table) + synonymy_table = _pick_synonymy_table(query_fields, base_table) if synonymy_table is None: logger.info("search_synonymy requested but no tree table found... skipping") else: - log_sqlalchemy_query(query.query) synonymized_query = synonymize_tree_query(query.query, synonymy_table) query = query._replace(query=synonymized_query) internal_predicate = query.get_internal_filters() query = query.filter(internal_predicate) - logger.debug("query: %s", query.query) return query.query, order_by_exprs def series_post_query(query, limit=40, offset=0, sort_type=0, co_id_cat_num_pair_col_index=0, is_count=False): @@ -1141,8 +1133,6 @@ def series_post_query(query, limit=40, offset=0, sort_type=0, co_id_cat_num_pair and adding a co_id colum and formatted catnum range column. Sort the results by the first catnum in the range.""" - log_sqlalchemy_query(query) - def parse_catalog_for_comparing(s): def check_for_decimal(s): decimal_match = re.search(r'\d+\.\d+', s) diff --git a/specifyweb/backend/stored_queries/field_spec_maps.py b/specifyweb/backend/stored_queries/field_spec_maps.py index d02a2b7b6eb..9e2b706cb6e 100644 --- a/specifyweb/backend/stored_queries/field_spec_maps.py +++ b/specifyweb/backend/stored_queries/field_spec_maps.py @@ -1,5 +1,24 @@ +from typing import Callable, Iterable + from specifyweb.backend.stored_queries.queryfield import QueryField +from specifyweb.backend.stored_queries.relative_date_utils import relative_to_absolute_date + +def transform_field_specs(query_fields: Iterable[QueryField], user): + transform = query_field_transformer(user) + for query_field in query_fields: + yield transform(query_field) +def query_field_transformer(user) -> Callable[[QueryField], QueryField]: + transforms = [ + apply_absolute_date, + lambda qf: apply_specify_user_name(qf, user) + ] + def transform_query_field(query_field): + qf = query_field + for transform in transforms: + qf = transform(qf) + return qf + return transform_query_field def apply_specify_user_name(query_field: QueryField, user): if query_field.fieldspec.is_specify_username_end(): @@ -7,3 +26,11 @@ def apply_specify_user_name(query_field: QueryField, user): return query_field._replace(value=user.name) return query_field + +def apply_absolute_date(query_field: QueryField): + if query_field.fieldspec.date_part is None or query_field.fieldspec.date_part != 'Full Date': + return query_field + + field_value = query_field.value + new_field_value = ','.join([relative_to_absolute_date(value_split) for value_split in field_value.split(',')]) + return query_field._replace(value=new_field_value) diff --git a/specifyweb/backend/stored_queries/geology_time.py b/specifyweb/backend/stored_queries/geology_time.py index 0a1a4ffe9d8..0a0ae4d79ab 100644 --- a/specifyweb/backend/stored_queries/geology_time.py +++ b/specifyweb/backend/stored_queries/geology_time.py @@ -2,7 +2,6 @@ import os from django.db.models import Case, FloatField, F, Q, Value, When from django.db.models.functions import Coalesce, Greatest, Least, Cast -from specifyweb.backend.stored_queries.utils import log_sqlalchemy_query from sqlalchemy import select, union_all, func, cast, DECIMAL, case, or_, and_, String, join from sqlalchemy.orm import aliased @@ -976,6 +975,5 @@ def modify_query_add_meta_age_range(query, start_time, end_time, require_full_ov ) ).label("age") new_query = new_query.add_columns(age_expr) - - log_sqlalchemy_query(new_query) + return new_query diff --git a/specifyweb/backend/stored_queries/relative_date_utils.py b/specifyweb/backend/stored_queries/relative_date_utils.py index ab284564c8e..526a459236c 100644 --- a/specifyweb/backend/stored_queries/relative_date_utils.py +++ b/specifyweb/backend/stored_queries/relative_date_utils.py @@ -3,13 +3,6 @@ relative_date_re = r"today\s*([+-])\s*(\d+)\s*(second|minute|hour|day|week|month|year)" -def apply_absolute_date(query_field): - if query_field.fieldspec.date_part is None or query_field.fieldspec.date_part != 'Full Date': - return query_field - - field_value = query_field.value - new_field_value = ','.join([relative_to_absolute_date(value_split) for value_split in field_value.split(',')]) - return query_field._replace(value=new_field_value) def relative_to_absolute_date(raw_date_value): date_parse = re.findall(relative_date_re, raw_date_value) diff --git a/specifyweb/backend/stored_queries/tests/test_execution/test_execute.py b/specifyweb/backend/stored_queries/tests/test_execution/test_execute.py index db37156d3b6..7b2c8d10987 100644 --- a/specifyweb/backend/stored_queries/tests/test_execution/test_execute.py +++ b/specifyweb/backend/stored_queries/tests/test_execution/test_execute.py @@ -30,7 +30,7 @@ def _execute_catalog_number_in_query(self, value, format_name=None): distinct=False, series=False, count_only=False, - field_specs=query_fields, + query_fields=query_fields, limit=0, offset=0, ) @@ -77,7 +77,7 @@ def test_simple_query(self): series=False, search_synonymy=False, count_only=False, - field_specs=query_fields, + query_fields=query_fields, limit=0, offset=0, ) @@ -110,7 +110,7 @@ def test_simple_query_count(self): series=False, search_synonymy=False, count_only=True, - field_specs=query_fields, + query_fields=query_fields, limit=0, offset=0, ) @@ -132,7 +132,7 @@ def test_simple_query_distinct(self): series=False, search_synonymy=False, count_only=False, - field_specs=query_fields, + query_fields=query_fields, limit=0, offset=0, ) @@ -168,7 +168,7 @@ def test_simple_query_distinct_count(self): series=False, search_synonymy=False, count_only=True, - field_specs=query_fields, + query_fields=query_fields, limit=0, offset=0, ) @@ -203,7 +203,7 @@ def test_simple_query_recordset_limit(self): series=False, search_synonymy=False, count_only=False, - field_specs=query_fields, + query_fields=query_fields, limit=3, offset=0, recordsetid=test_rs.id, @@ -219,7 +219,7 @@ def test_simple_query_recordset_limit(self): series=False, search_synonymy=False, count_only=True, - field_specs=query_fields, + query_fields=query_fields, limit=3, offset=0, recordsetid=test_rs.id, @@ -277,7 +277,7 @@ def test_related_date_part_query_fields(self): series=False, search_synonymy=False, count_only=False, - field_specs=query_fields, + query_fields=query_fields, limit=0, offset=0, ) @@ -308,7 +308,7 @@ def test_simple_query_series(self): series=True, search_synonymy=False, count_only=False, - field_specs=query_fields, + query_fields=query_fields, limit=0, offset=0, ) @@ -322,7 +322,7 @@ def test_simple_query_series(self): series=True, search_synonymy=False, count_only=True, - field_specs=query_fields, + query_fields=query_fields, limit=0, offset=0, ) diff --git a/specifyweb/backend/stored_queries/tests/test_relative_date_utils.py b/specifyweb/backend/stored_queries/tests/test_relative_date_utils.py index 0c616752478..0dd3ea576c2 100644 --- a/specifyweb/backend/stored_queries/tests/test_relative_date_utils.py +++ b/specifyweb/backend/stored_queries/tests/test_relative_date_utils.py @@ -1,5 +1,6 @@ from specifyweb.specify.tests.test_api import ApiTests, MockDateTime -from specifyweb.backend.stored_queries.relative_date_utils import apply_absolute_date, relative_to_absolute_date +from specifyweb.backend.stored_queries.relative_date_utils import relative_to_absolute_date +from specifyweb.backend.stored_queries.field_spec_maps import apply_absolute_date from unittest.mock import patch, Mock import datetime diff --git a/specifyweb/backend/stored_queries/views.py b/specifyweb/backend/stored_queries/views.py index 0f99dc5d46f..4229924ba59 100644 --- a/specifyweb/backend/stored_queries/views.py +++ b/specifyweb/backend/stored_queries/views.py @@ -103,7 +103,7 @@ def query(request, id): series=series, search_synonymy=search_synonymy, count_only=count_only, - field_specs=field_specs, + query_fields=field_specs, limit=limit, offset=offset ) From 452deb9e915733019a1b84092fed87d830c2e1c1 Mon Sep 17 00:00:00 2001 From: melton-jason Date: Thu, 17 Sep 2026 14:32:20 -0500 Subject: [PATCH 2/7] fix: use table over node in query join cache Fixes #8529 --- specifyweb/backend/stored_queries/query_construct.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/specifyweb/backend/stored_queries/query_construct.py b/specifyweb/backend/stored_queries/query_construct.py index 614ee449d43..212a6788c3c 100644 --- a/specifyweb/backend/stored_queries/query_construct.py +++ b/specifyweb/backend/stored_queries/query_construct.py @@ -39,7 +39,7 @@ def handle_tree_field(self, node, table, tree_rank: TreeRankQuery, next_join_pat treedefitem_column = table.name + 'TreeDefItemID' treedef_column = table.name + 'TreeDefID' - cache_key = (node, '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] From 4975da0a7692dc3b55243005afd62ae7d4c1290b Mon Sep 17 00:00:00 2001 From: melton-jason Date: Fri, 18 Sep 2026 00:16:30 -0500 Subject: [PATCH 3/7] refactor: decompose build_query --- .../backend/stored_queries/execution.py | 298 +++++++++++------- 1 file changed, 186 insertions(+), 112 deletions(-) diff --git a/specifyweb/backend/stored_queries/execution.py b/specifyweb/backend/stored_queries/execution.py index f33504c2961..7ac052f0dc7 100644 --- a/specifyweb/backend/stored_queries/execution.py +++ b/specifyweb/backend/stored_queries/execution.py @@ -38,6 +38,7 @@ from specifyweb.backend.stored_queries.synonomy import synonymize_tree_query from specifyweb.specify.datamodel import datamodel, is_tree_table +from specifyweb.specify.models_utils.load_datamodel import Table logger = logging.getLogger(__name__) @@ -943,6 +944,163 @@ def execute( ) return {"results": results} + +# REFACTOR: Clean up this and the other QueryConstruct functions +def build_query_construct_base( + session, + collection, + user, + id_field, + props: BuildQueryProps, + catalognumber_field = None +): + query_construct_query = session.query(id_field) + if props.series and catalognumber_field: + query_construct_query = session.query( + func.group_concat( + func.concat( + id_field, + ':', + catalognumber_field + ), + separator='|' + ).label('co_id_catnum_paired_values') + ) + elif props.distinct: + query_construct_query = session.query( + func.group_concat(id_field.distinct(), separator=',')) + else: + query_construct_query = session.query(id_field) + + query = QueryConstruct( + collection=collection, + objectformatter=ObjectFormatter( + collection, + user, + props.replace_nulls, + props=props.formatter_props, + ), + query=query_construct_query + ) + return query + +# REFACTOR: Clean up this and the other QueryConstruct functions +def filter_query_by_recordset( + session, + collection, + query: QueryConstruct, + id_field: int, + tableid: int, + recordsetid: int +): + recordset = session.query(models.RecordSet).get(recordsetid) + if recordset is None: + raise AssertionError( + f"Unexpected recordset id '{recordsetid}' in request. Recordset not found.", + { + "recordsetId": recordsetid, + "localizationKey": "unexpectedRecordsetId", + }, + ) + if recordset.collectionMemberId != collection.id: + raise AssertionError( + f"Unexpected recordset id '{recordsetid}' in request. Recordset is not in collection '{collection.id}'.", + { + "recordsetId": recordsetid, + "collectionId": collection.id, + "expectedCollectionId": recordset.collectionMemberId, + "localizationKey": "unexpectedRecordsetCollection", + }, + ) + if recordset.dbTableId != tableid: + raise AssertionError( + f"Unexpected tableId '{tableid}' in request. Expected '{recordset.dbTableId}'", + { + "tableId": tableid, + "expectedTableId": recordset.dbTableId, + "localizationKey": "unexpectedTableId", + }, + ) + return query.join( + models.RecordSetItem, models.RecordSetItem.recordId == id_field + ).filter(models.RecordSetItem.recordSet == recordset) + +# REFACTOR: Clean up this and the other QueryConstruct functions +def apply_where_condition_to_query( + query: QueryConstruct, + predicates_by_fieldspec, + use_implicit_ors: bool = True +): + if use_implicit_ors: + implicit_ors = [ + reduce(sql.or_, ps) for ps in predicates_by_fieldspec.values() if ps + ] + + if implicit_ors: + where = reduce(sql.and_, implicit_ors) + query = query.filter(where) + else: + where = reduce(sql.and_, (p for ps in predicates_by_fieldspec.values() for p in ps)) + query = query.filter(where) + return query + +# REFACTOR: Clean up this and the other QueryConstruct functions +def add_fields_to_query( + collection, + user, + query: QueryConstruct, + query_fields: list[QueryField], + series: bool = False, + formatauditobjs: bool = False, + use_implicit_ors: bool = True +): + order_by_exprs = [] + selected_fields = [] + predicates_by_fieldspec = defaultdict(list) + for query_field in query_fields: + sort_type = QuerySort.by_id(query_field.sort_type) + + if series and query_field.fieldspec.get_field() and query_field.fieldspec.get_field().name.lower() == 'catalognumber': + _, _, predicate = query_field.add_to_query(query, formatauditobjs=formatauditobjs) + predicates_by_fieldspec[query_field.fieldspec].append(predicate) if predicate is not None else None + continue + + query, field, predicate = query_field.add_to_query( + query, formatauditobjs=formatauditobjs, collection=collection, user=user + ) + + if field is None: + continue + + formatted_field = None + if query_field.display: + formatted_field = query.objectformatter.fieldformat(query_field, field) + query = query.add_columns(formatted_field) + selected_fields.append(formatted_field) + + + if sort_type is not None: + order_by_exprs.append(sort_type(field)) + + if predicate is not None: + predicates_by_fieldspec[query_field.fieldspec].append(predicate) + query = apply_where_condition_to_query(query, predicates_by_fieldspec, use_implicit_ors) + return query, selected_fields, order_by_exprs + +# REFACTOR: Clean up this and the other QueryConstruct functions +def search_on_synonyms( + query: QueryConstruct, + query_fields: list[QueryField], + base_table: Table, +): + synonymy_table = _pick_synonymy_table(query_fields, base_table) + if synonymy_table is None: + logger.info("search_synonymy requested but no tree table found... skipping") + else: + synonymized_query = synonymize_tree_query(query.query, synonymy_table) + query = query._replace(query=synonymized_query) + return query + def build_query( session, collection, @@ -983,40 +1141,20 @@ def build_query( search_synonymy = if True, search synonym nodes as well, and return all record IDs associated with parent node """ model = models.models_by_tableid[tableid] - base_table = datamodel.get_table_by_id(tableid, strict=True) id_field = model._id - catalog_number_field = model.catalogNumber if hasattr(model, 'catalogNumber') else None - - query_fields = list(transform_field_specs(query_fields, user)) - - query_construct_query = session.query(id_field) - if props.series and catalog_number_field: - query_construct_query = session.query( - func.group_concat( - func.concat( - id_field, - ':', - catalog_number_field - ), - separator='|' - ).label('co_id_catnum_paired_values') - ) - elif props.distinct: - query_construct_query = session.query(func.group_concat(id_field.distinct(), separator=',')) - else: - query_construct_query = session.query(id_field) - - query = QueryConstruct( + query = build_query_construct_base( + session=session, collection=collection, - objectformatter=ObjectFormatter( - collection, - user, - props.replace_nulls, - props=props.formatter_props, - ), - query=query_construct_query + user=user, + id_field=id_field, + props=props, + catalognumber_field=( + model.catalogNumber if hasattr(model, 'catalogNumber') else None + ) ) + query_fields = list(transform_field_specs(query_fields, user)) + tables_to_read = { table for field in query_fields @@ -1032,96 +1170,32 @@ def build_query( if props.recordsetid is not None: logger.debug("joining query to recordset: %s", props.recordsetid) - recordset = session.query(models.RecordSet).get(props.recordsetid) - if recordset is None: - raise AssertionError( - f"Unexpected recordset id '{props.recordsetid}' in request. Recordset not found.", - { - "recordsetId": props.recordsetid, - "localizationKey": "unexpectedRecordsetId", - }, - ) - if recordset.collectionMemberId != collection.id: - raise AssertionError( - f"Unexpected recordset id '{props.recordsetid}' in request. Recordset is not in collection '{collection.id}'.", - { - "recordsetId": props.recordsetid, - "collectionId": collection.id, - "expectedCollectionId": recordset.collectionMemberId, - "localizationKey": "unexpectedRecordsetCollection", - }, - ) - if recordset.dbTableId != tableid: - raise AssertionError( - f"Unexpected tableId '{tableid}' in request. Expected '{recordset.dbTableId}'", - { - "tableId": tableid, - "expectedTableId": recordset.dbTableId, - "localizationKey": "unexpectedTableId", - }, - ) - query = query.join( - models.RecordSetItem, models.RecordSetItem.recordId == id_field - ).filter(models.RecordSetItem.recordSet == recordset) - - order_by_exprs = [] - selected_fields = [] - predicates_by_field = defaultdict(list) - for query_field in query_fields: - sort_type = QuerySort.by_id(query_field.sort_type) - - if props.series and query_field.fieldspec.get_field() and query_field.fieldspec.get_field().name.lower() == 'catalognumber': - _, _, predicate = query_field.add_to_query(query, formatauditobjs=props.formatauditobjs) - predicates_by_field[query_field.fieldspec].append(predicate) if predicate is not None else None - continue - - query, field, predicate = query_field.add_to_query( - query, formatauditobjs=props.formatauditobjs, collection=collection, user=user + query = filter_query_by_recordset( + session=session, + collection=collection, + query=query, + id_field=id_field, + tableid=tableid, + recordsetid=props.recordsetid ) - if field is None: - continue - - formatted_field = None - if query_field.display: - formatted_field = query.objectformatter.fieldformat(query_field, field) - query = query.add_columns(formatted_field) - selected_fields.append(formatted_field) - - if hasattr(field, 'key') and field.key and field.key.lower() == 'catalognumber': - catalog_number_field = formatted_field - - - if sort_type is not None: - order_by_exprs.append(sort_type(field)) - - if predicate is not None: - predicates_by_field[query_field.fieldspec].append(predicate) - - if props.implicit_or: - implicit_ors = [ - reduce(sql.or_, ps) for ps in predicates_by_field.values() if ps - ] - - if implicit_ors: - where = reduce(sql.and_, implicit_ors) - query = query.filter(where) - else: - where = reduce(sql.and_, (p for ps in predicates_by_field.values() for p in ps)) - query = query.filter(where) + query, selected_fields, order_by_exprs = add_fields_to_query( + collection=collection, + user=user, + query=query, + query_fields=query_fields, + series=props.series, + formatauditobjs=props.formatauditobjs + ) if props.series: query = group_by_displayed_fields(query, selected_fields, ignore_cat_num=True) elif props.distinct: query = group_by_displayed_fields(query, selected_fields) - + if props.search_synonymy: - synonymy_table = _pick_synonymy_table(query_fields, base_table) - if synonymy_table is None: - logger.info("search_synonymy requested but no tree table found... skipping") - else: - synonymized_query = synonymize_tree_query(query.query, synonymy_table) - query = query._replace(query=synonymized_query) + base_table = datamodel.get_table_by_id_strict(tableid) + query = search_on_synonyms(query, query_fields, base_table) internal_predicate = query.get_internal_filters() query = query.filter(internal_predicate) From 3e33c9bf97ef8173c9f217d3f7fcc754963fe2f5 Mon Sep 17 00:00:00 2001 From: melton-jason Date: Fri, 18 Sep 2026 08:44:43 -0500 Subject: [PATCH 4/7] chore: add test for Tree Rank JOIN caching for build_query --- .../backend/stored_queries/execution.py | 24 +++- .../stored_queries/tests/test_build_query.py | 129 ++++++++++++++++++ 2 files changed, 147 insertions(+), 6 deletions(-) create mode 100644 specifyweb/backend/stored_queries/tests/test_build_query.py diff --git a/specifyweb/backend/stored_queries/execution.py b/specifyweb/backend/stored_queries/execution.py index 7ac052f0dc7..2e4e197d454 100644 --- a/specifyweb/backend/stored_queries/execution.py +++ b/specifyweb/backend/stored_queries/execution.py @@ -946,14 +946,18 @@ def execute( # REFACTOR: Clean up this and the other QueryConstruct functions +# Make it easier to use for external callers (e.g., tests) and make sure it's +# pure +# Maybe add or merge these to QueryConstruct? def build_query_construct_base( session, collection, user, - id_field, + model, props: BuildQueryProps, - catalognumber_field = None ): + id_field = model._id + catalognumber_field = model.catalogNumber if hasattr(model, 'catalogNumber') else None query_construct_query = session.query(id_field) if props.series and catalognumber_field: query_construct_query = session.query( @@ -985,6 +989,9 @@ def build_query_construct_base( return query # REFACTOR: Clean up this and the other QueryConstruct functions +# Make it easier to use for external callers (e.g., tests) and make sure it's +# pure +# Maybe add or merge these to QueryConstruct? def filter_query_by_recordset( session, collection, @@ -1026,6 +1033,8 @@ def filter_query_by_recordset( ).filter(models.RecordSetItem.recordSet == recordset) # REFACTOR: Clean up this and the other QueryConstruct functions +# This could probably be folded into add_fields_to_query? +# Though if possible/feasible, we can keep this separate to make testing easier def apply_where_condition_to_query( query: QueryConstruct, predicates_by_fieldspec, @@ -1045,6 +1054,9 @@ def apply_where_condition_to_query( return query # REFACTOR: Clean up this and the other QueryConstruct functions +# Make it easier to use for external callers (e.g., tests) and make sure it's +# pure +# Maybe add or merge these to QueryConstruct? def add_fields_to_query( collection, user, @@ -1088,6 +1100,9 @@ def add_fields_to_query( return query, selected_fields, order_by_exprs # REFACTOR: Clean up this and the other QueryConstruct functions +# Make it easier to use for external callers (e.g., tests) and make sure it's +# pure +# Maybe add or merge these to QueryConstruct? def search_on_synonyms( query: QueryConstruct, query_fields: list[QueryField], @@ -1146,11 +1161,8 @@ def build_query( session=session, collection=collection, user=user, - id_field=id_field, + model=model, props=props, - catalognumber_field=( - model.catalogNumber if hasattr(model, 'catalogNumber') else None - ) ) query_fields = list(transform_field_specs(query_fields, user)) diff --git a/specifyweb/backend/stored_queries/tests/test_build_query.py b/specifyweb/backend/stored_queries/tests/test_build_query.py new file mode 100644 index 00000000000..7330b738282 --- /dev/null +++ b/specifyweb/backend/stored_queries/tests/test_build_query.py @@ -0,0 +1,129 @@ +from specifyweb.specify.models import Taxontreedefitem, datamodel +from specifyweb.backend.stored_queries import models as models +from specifyweb.backend.stored_queries.queryfield import fields_from_json +from specifyweb.backend.stored_queries.execution import QuerySort, BuildQueryProps, build_query, DefaultQueryFormatterProps, build_query_construct_base, add_fields_to_query +from specifyweb.backend.stored_queries.tests.tests import SQLAlchemySetup + +class TestBuildQuery(SQLAlchemySetup): + def setUp(self): + super().setUp() + root_rank = Taxontreedefitem.objects.create( + rankid=0, + parent=None, + treedef=self.taxontreedef, + name="Root", + title="Root" + ) + kingdom_rank = Taxontreedefitem.objects.create( + rankid=10, + parent=root_rank, + treedef=self.taxontreedef, + name="Kingdom", + title="Kingdom" + ) + phylum_rank = Taxontreedefitem.objects.create( + rankid=30, + parent=kingdom_rank, + treedef=self.taxontreedef, + name="Phylum", + title="Phylum" + ) + family_rank = Taxontreedefitem.objects.create( + rankid=140, + parent=phylum_rank, + treedef=self.taxontreedef, + name="Family", + title="Family" + ) + genus_rank = Taxontreedefitem.objects.create( + rankid=180, + parent=family_rank, + treedef=self.taxontreedef, + name="Genus", + title="Genus" + ) + Taxontreedefitem.objects.create( + rankid=220, + parent=genus_rank, + treedef=self.taxontreedef, + name="Species", + title="Species" + ) + + # This test helps guard against Issues like #8529 and #3369 + def test_no_extra_tree_joins(self): + base_field_attrs = { + "formatname": None, + "isdisplay": True, + "isnot": False, + "isrelfld": False, + # operstart 8 = Don't Care / Any + # REFACTOR: Make an OpNum Enum with possible values + # There's QUERYFIELD_OPERATION_NUMBER type, but no easy-to-read + # value we can use + "operstart": 8, + "sorttype": QuerySort.NONE, + "startvalue": "", + "isstrict": False + } + base_query_fields = [ + { + **base_field_attrs, + "position": 0, + "stringid": "1,9-determinations,4.taxon.Genus", + }, + { + **base_field_attrs, + "position": 1, + "stringid": "1,9-determinations,4-preferredTaxon.taxon.Species", + } + ] + query_fields = fields_from_json(base_query_fields) + collection = self.collection + user = self.specifyuser + tableid = datamodel.get_table_strict("collectionobject").tableId + with TestBuildQuery.test_session_context() as session: + props = BuildQueryProps() + model = models.models_by_tableid[tableid] + query_base = build_query_construct_base( + session=session, + collection=collection, + user=user, + model=model, + props=props + ) + query, selected_fields, order_by_exprs = add_fields_to_query( + collection=collection, + user=user, + query=query_base, + query_fields=query_fields, + series=props.series, + formatauditobjs=props.formatauditobjs + ) + # There should be 4 objects in the join cache + # CollectionObject -> Determinations + # Determination -> taxon + # Taxon Ranks for Determination -> taxon + # Determination -> preferredTaxon + self.assertEqual( + len(query.join_cache), + 4 + ) + # BUG: This is technically undesirable, as it causes #8650 + # In the underlying query, the Preferred Taxon currently uses the + # JOIN for Determination -> taxon. Specifically, it uses the cached + # JOINs for the tree ranks. + taxon_table = datamodel.get_table_strict("taxon") + cache_key = (taxon_table, 'TreeRanks') + tree_ranks_in_cache = list(filter(lambda cache_key: 'TreeRanks' in cache_key, query.join_cache.keys())) + self.assertEqual( + tree_ranks_in_cache, + [cache_key] + ) + tree_join_information = query.join_cache[cache_key] + tree_ranks = tree_join_information[0] + + self.assertEqual( + len(tree_ranks), + len(self.taxontreedef.treedefitems.all()) + ) From 92fae443e32af57e7fd58f72821478ab4acd33d6 Mon Sep 17 00:00:00 2001 From: melton-jason Date: Fri, 18 Sep 2026 09:52:39 -0500 Subject: [PATCH 5/7] refactor: narrow type of query_fields to list --- specifyweb/backend/stored_queries/execution.py | 6 +++--- specifyweb/backend/stored_queries/views.py | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/specifyweb/backend/stored_queries/execution.py b/specifyweb/backend/stored_queries/execution.py index 2e4e197d454..04514ca9b0c 100644 --- a/specifyweb/backend/stored_queries/execution.py +++ b/specifyweb/backend/stored_queries/execution.py @@ -686,7 +686,7 @@ def run_ephemeral_query(collection, user, spquery): format_audits = spquery.get("formatauditrecids", False) with models.session_context() as session: - field_specs = fields_from_json(spquery["fields"]) + query_fields = fields_from_json(spquery["fields"]) return execute( session=session, collection=collection, @@ -696,7 +696,7 @@ def run_ephemeral_query(collection, user, spquery): series=series, search_synonymy=search_synonymy, count_only=count_only, - query_fields=field_specs, + query_fields=query_fields, limit=limit, offset=offset, recordsetid=recordsetid, @@ -859,7 +859,7 @@ def execute( series: bool, search_synonymy: bool, count_only: bool, - query_fields: Iterable[QueryField], + query_fields: list[QueryField], limit, offset, recordsetid=None, diff --git a/specifyweb/backend/stored_queries/views.py b/specifyweb/backend/stored_queries/views.py index 4229924ba59..70c5240a718 100644 --- a/specifyweb/backend/stored_queries/views.py +++ b/specifyweb/backend/stored_queries/views.py @@ -91,7 +91,7 @@ def query(request, id): tableid = sp_query.contextTableId count_only = sp_query.countOnly - field_specs = [QueryField.from_spqueryfield(field, value_from_request(field, request.GET)) + query_fields = [QueryField.from_spqueryfield(field, value_from_request(field, request.GET)) for field in sorted(sp_query.fields, key=lambda field: field.position)] data = execute( @@ -103,7 +103,7 @@ def query(request, id): series=series, search_synonymy=search_synonymy, count_only=count_only, - query_fields=field_specs, + query_fields=query_fields, limit=limit, offset=offset ) From ee6bd34b5ae973a425280806471ffc02d72ec843 Mon Sep 17 00:00:00 2001 From: melton-jason Date: Fri, 18 Sep 2026 12:25:22 -0500 Subject: [PATCH 6/7] fix: pass implicit ors preference to add_fields_to_query --- specifyweb/backend/stored_queries/execution.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/specifyweb/backend/stored_queries/execution.py b/specifyweb/backend/stored_queries/execution.py index 04514ca9b0c..71a399fbab7 100644 --- a/specifyweb/backend/stored_queries/execution.py +++ b/specifyweb/backend/stored_queries/execution.py @@ -1197,7 +1197,8 @@ def build_query( query=query, query_fields=query_fields, series=props.series, - formatauditobjs=props.formatauditobjs + formatauditobjs=props.formatauditobjs, + use_implicit_ors=props.implicit_or ) if props.series: From 1f1f74fc2579ff58c24cf446ffcc39ad18e53bfc Mon Sep 17 00:00:00 2001 From: melton-jason Date: Wed, 23 Sep 2026 10:24:39 -0500 Subject: [PATCH 7/7] chore: clean up unused imports and unused code --- specifyweb/backend/stored_queries/execution.py | 2 -- specifyweb/backend/stored_queries/tests/test_build_query.py | 4 ++-- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/specifyweb/backend/stored_queries/execution.py b/specifyweb/backend/stored_queries/execution.py index 5f7d11611c8..fe683668915 100644 --- a/specifyweb/backend/stored_queries/execution.py +++ b/specifyweb/backend/stored_queries/execution.py @@ -973,8 +973,6 @@ def build_query_construct_base( elif props.distinct: query_construct_query = session.query( func.group_concat(id_field.distinct(), separator=',')) - else: - query_construct_query = session.query(id_field) query = QueryConstruct( collection=collection, diff --git a/specifyweb/backend/stored_queries/tests/test_build_query.py b/specifyweb/backend/stored_queries/tests/test_build_query.py index 7330b738282..38d5449ffe0 100644 --- a/specifyweb/backend/stored_queries/tests/test_build_query.py +++ b/specifyweb/backend/stored_queries/tests/test_build_query.py @@ -1,7 +1,7 @@ from specifyweb.specify.models import Taxontreedefitem, datamodel from specifyweb.backend.stored_queries import models as models from specifyweb.backend.stored_queries.queryfield import fields_from_json -from specifyweb.backend.stored_queries.execution import QuerySort, BuildQueryProps, build_query, DefaultQueryFormatterProps, build_query_construct_base, add_fields_to_query +from specifyweb.backend.stored_queries.execution import QuerySort, BuildQueryProps, build_query_construct_base, add_fields_to_query from specifyweb.backend.stored_queries.tests.tests import SQLAlchemySetup class TestBuildQuery(SQLAlchemySetup): @@ -92,7 +92,7 @@ def test_no_extra_tree_joins(self): model=model, props=props ) - query, selected_fields, order_by_exprs = add_fields_to_query( + query, _selected_fields, _order_by_expressions = add_fields_to_query( collection=collection, user=user, query=query_base,