From 6a81f132bee880e0faf585413091a43363f0e053 Mon Sep 17 00:00:00 2001 From: Juliette Date: Fri, 29 May 2026 13:27:17 -0600 Subject: [PATCH 1/6] Update version to 1.5.4.dev1 and enhance query parameter handling --- agave/chalice/rest_api.py | 11 +++- agave/core/filters.py | 1 + agave/core/query_params.py | 105 ++++++++++++++++++++++++++++++ agave/fastapi/rest_api.py | 12 +++- agave/version.py | 2 +- examples/validators.py | 1 + tests/blueprint/test_blueprint.py | 38 +++++++++++ tests/core/test_query_params.py | 78 ++++++++++++++++++++++ 8 files changed, 241 insertions(+), 7 deletions(-) create mode 100644 agave/core/query_params.py create mode 100644 tests/core/test_query_params.py diff --git a/agave/chalice/rest_api.py b/agave/chalice/rest_api.py index 0465a80b..25112b0c 100644 --- a/agave/chalice/rest_api.py +++ b/agave/chalice/rest_api.py @@ -15,6 +15,7 @@ from pydantic import BaseModel, ValidationError from ..core.blueprints.decorators import copy_attributes +from ..core.query_params import EmptyQueryMapping, validate_query_params class RestApiBlueprint(Blueprint): @@ -238,9 +239,13 @@ def query(): next_page = } """ - params = self.current_request.query_params or dict() + query_mapping = ( + self.current_request.query_params or EmptyQueryMapping() + ) try: - query_params = cls.query_validator(**params) + query_params = validate_query_params( + query_mapping, cls.query_validator + ) except ValidationError as e: return Response(e.json(), status_code=400) @@ -301,7 +306,7 @@ def _all(query: QueryParams, filters: Q): params.pop('user_id') if self.platform_id_filter_required(): params.pop('platform_id') - next_page_uri = f'{path}?{urlencode(params)}' + next_page_uri = f'{path}?{urlencode(params, doseq=True)}' return dict(items=item_dicts, next_page_uri=next_page_uri) return cls diff --git a/agave/core/filters.py b/agave/core/filters.py index f2d10669..e7b291ce 100644 --- a/agave/core/filters.py +++ b/agave/core/filters.py @@ -15,6 +15,7 @@ def generic_query(query: QueryParams, excluded: list[str] = []) -> Q: 'limit', 'page_size', 'key', + 'ids', *excluded, } fields = query.model_dump(exclude=exclude_fields) diff --git a/agave/core/query_params.py b/agave/core/query_params.py new file mode 100644 index 00000000..78731d27 --- /dev/null +++ b/agave/core/query_params.py @@ -0,0 +1,105 @@ +from __future__ import annotations + +from types import UnionType +from typing import ( + Any, + Iterator, + Protocol, + TypeVar, + Union, + get_args, + get_origin, +) + +from pydantic import BaseModel + +ModelT = TypeVar('ModelT', bound=BaseModel) + + +class QueryParamMapping(Protocol): + def __contains__(self, key: str) -> bool: ... + + def __iter__(self) -> Iterator[str]: ... + + def get(self, key: str, default: Any = None) -> Any: ... + + def getlist(self, key: str) -> list[str]: ... + + +def _is_list_annotation(annotation: Any) -> bool: + origin = get_origin(annotation) + if origin is list: + return True + if origin in (Union, UnionType): + return any( + _is_list_annotation(arg) + for arg in get_args(annotation) + if arg is not type(None) + ) + return False + + +def build_query_dict( + query_mapping: QueryParamMapping, model_cls: type[BaseModel] +) -> dict[str, Any]: + params: dict[str, Any] = {} + for name in query_mapping: + if name in model_cls.model_fields: + field = model_cls.model_fields[name] + if _is_list_annotation(field.annotation): + params[name] = query_mapping.getlist(name) + else: + params[name] = query_mapping.get(name) + else: + params[name] = query_mapping.get(name) + return params + + +def validate_query_params( + query_mapping: QueryParamMapping, model_cls: type[ModelT] +) -> ModelT: + return model_cls(**build_query_dict(query_mapping, model_cls)) + + +class EmptyQueryMapping: + def __contains__(self, key: str) -> bool: + return False + + def get(self, key: str, default: Any = None) -> Any: + return default + + def getlist(self, key: str) -> list[str]: + return [] + + +class ParseQsQueryMapping: + """Query mapping built from a raw query string (e.g. Chalice).""" + + def __init__(self, query_string: str | bytes | None) -> None: + from urllib.parse import parse_qsl + + if not query_string: + self._items: list[tuple[str, str]] = [] + return + if isinstance(query_string, bytes): + query_string = query_string.decode('latin-1') + self._items = parse_qsl(query_string, keep_blank_values=True) + + def __iter__(self) -> Iterator[str]: + seen: set[str] = set() + for key, _ in self._items: + if key not in seen: + seen.add(key) + yield key + + def __contains__(self, key: str) -> bool: + return any(k == key for k, _ in self._items) + + def get(self, key: str, default: Any = None) -> Any: + values = self.getlist(key) + if not values: + return default + return values[-1] + + def getlist(self, key: str) -> list[str]: + return [v for k, v in self._items if k == key] diff --git a/agave/fastapi/rest_api.py b/agave/fastapi/rest_api.py index 89b573a7..062efe1f 100644 --- a/agave/fastapi/rest_api.py +++ b/agave/fastapi/rest_api.py @@ -1,6 +1,6 @@ import asyncio import mimetypes -from typing import Any, Optional +from typing import Any, Optional, cast from urllib.parse import urlencode from cuenca_validations.types import QueryParams @@ -23,6 +23,7 @@ from ..core.blueprints.decorators import copy_attributes from ..core.exc import NotFoundError, UnprocessableEntity +from ..core.query_params import QueryParamMapping, validate_query_params SAMPLE_404 = { "summary": "Not found item", @@ -358,7 +359,10 @@ class QueryResponse(BaseModel): def validate_params(request: Request): try: - return cls.query_validator(**request.query_params) + return validate_query_params( + cast(QueryParamMapping, request.query_params), + cls.query_validator, + ) except ValidationError as e: raise UnprocessableEntity(e.json()) @@ -435,7 +439,9 @@ async def _all(query: QueryParams, filters: Q, resource_path: str): params.pop('user_id') if self.platform_id_filter_required(): params.pop('platform_id') - next_page_uri = f'{resource_path}?{urlencode(params)}' + next_page_uri = ( + f'{resource_path}?{urlencode(params, doseq=True)}' + ) return dict(items=item_dicts, next_page_uri=next_page_uri) return cls diff --git a/agave/version.py b/agave/version.py index c24ed73b..154f11c4 100644 --- a/agave/version.py +++ b/agave/version.py @@ -1 +1 @@ -__version__ = '1.5.4' +__version__ = '1.5.4.dev1' diff --git a/examples/validators.py b/examples/validators.py index 7fe22071..758d92e3 100644 --- a/examples/validators.py +++ b/examples/validators.py @@ -10,6 +10,7 @@ class AccountQuery(QueryParams): user_id: Optional[str] = None platform_id: Optional[str] = None active: Optional[bool] = None + ids: Optional[list[str]] = None class TransactionQuery(QueryParams): diff --git a/tests/blueprint/test_blueprint.py b/tests/blueprint/test_blueprint.py index 71a06b5d..21bed58f 100644 --- a/tests/blueprint/test_blueprint.py +++ b/tests/blueprint/test_blueprint.py @@ -282,6 +282,44 @@ def test_query_count_resource( assert json_body['count'] == 1 +@pytest.mark.parametrize( + "client_fixture", ["fastapi_client", "chalice_client"] +) +@pytest.mark.usefixtures('accounts') +def test_query_with_repeated_ids_param( + client_fixture: str, + request: pytest.FixtureRequest, +) -> None: + client = request.getfixturevalue(client_fixture) + resp = client.get('/accounts?ids=US1&ids=US2') + assert resp.status_code == 200 + assert 'items' in resp.json() + + +@pytest.mark.parametrize( + "client_fixture", ["fastapi_client", "chalice_client"] +) +@pytest.mark.usefixtures('accounts') +def test_query_pagination_preserves_repeated_ids( + client_fixture: str, + request: pytest.FixtureRequest, + accounts: list[Account], +) -> None: + client = request.getfixturevalue(client_fixture) + account_ids = [accounts[0].id, accounts[1].id] + query = '&'.join(f'ids={account_id}' for account_id in account_ids) + resp = client.get(f'/accounts?{query}&page_size=1&limit=10') + assert resp.status_code == 200 + json_body = resp.json() + next_page_uri = json_body['next_page_uri'] + assert next_page_uri is not None + for account_id in account_ids: + assert f'ids={account_id}' in next_page_uri + + resp = client.get(next_page_uri) + assert resp.status_code == 200 + + @pytest.mark.parametrize( "client_fixture", ["fastapi_client", "chalice_client"] ) diff --git a/tests/core/test_query_params.py b/tests/core/test_query_params.py new file mode 100644 index 00000000..a90dbc4b --- /dev/null +++ b/tests/core/test_query_params.py @@ -0,0 +1,78 @@ +from typing import Optional, cast + +import pytest +from pydantic import BaseModel, ConfigDict, ValidationError +from starlette.datastructures import QueryParams + +from agave.core.query_params import ( + ParseQsQueryMapping, + QueryParamMapping, + build_query_dict, + validate_query_params, +) + + +class SampleQuery(BaseModel): + model_config = ConfigDict(extra='forbid') + + ids: Optional[list[str]] = None + name: Optional[str] = None + active: Optional[bool] = None + + +def test_build_query_dict_repeated_ids() -> None: + query = cast(QueryParamMapping, QueryParams('ids=US1&ids=US2&active=true')) + params = build_query_dict(query, SampleQuery) + assert params == {'ids': ['US1', 'US2'], 'active': 'true'} + + +def test_build_query_dict_single_id() -> None: + query = cast(QueryParamMapping, QueryParams('ids=US1')) + params = build_query_dict(query, SampleQuery) + assert params == {'ids': ['US1']} + + +def test_build_query_dict_unknown_param_for_extra_forbid() -> None: + query = cast(QueryParamMapping, QueryParams('wrong_param=value')) + params = build_query_dict(query, SampleQuery) + assert params == {'wrong_param': 'value'} + + +def test_validate_query_params_list_fields() -> None: + query = cast(QueryParamMapping, QueryParams('ids=a&ids=b&name=Frida')) + validated = validate_query_params(query, SampleQuery) + assert validated.ids == ['a', 'b'] + assert validated.name == 'Frida' + + +def test_validate_query_params_rejects_invalid_list() -> None: + # Starlette last-value semantics without getlist would pass a string; + # validate_query_params must still produce a list for list fields. + query = cast(QueryParamMapping, QueryParams('ids=a&ids=b')) + validated = validate_query_params(query, SampleQuery) + assert validated.ids == ['a', 'b'] + + +def test_parse_qs_query_mapping() -> None: + mapping = ParseQsQueryMapping('ids=US1&ids=US2&name=test') + assert mapping.getlist('ids') == ['US1', 'US2'] + assert mapping.get('name') == 'test' + assert 'missing' not in mapping + + +def test_validate_query_params_parse_qs_mapping() -> None: + mapping = ParseQsQueryMapping('ids=x&ids=y') + validated = validate_query_params(mapping, SampleQuery) + assert validated.ids == ['x', 'y'] + + +def test_old_unpack_pattern_would_fail_validation() -> None: + query = QueryParams('ids=US1&ids=US2') + with pytest.raises(ValidationError): + SampleQuery(**dict(query)) # type: ignore[arg-type] + + +def test_validate_query_params_rejects_unknown_fields() -> None: + query = cast(QueryParamMapping, QueryParams('wrong_param=value')) + with pytest.raises(ValidationError): + validate_query_params(query, SampleQuery) From 0e53811ed051132fa40527a4723a3ec50e95f25f Mon Sep 17 00:00:00 2001 From: Juliette Date: Fri, 29 May 2026 13:47:26 -0600 Subject: [PATCH 2/6] Refactor query parameter handling by removing unused classes and simplifying validation logic --- agave/core/query_params.py | 79 +++++---------------------------- agave/fastapi/rest_api.py | 7 ++- tests/core/test_query_params.py | 75 +++++++++++++++---------------- 3 files changed, 49 insertions(+), 112 deletions(-) diff --git a/agave/core/query_params.py b/agave/core/query_params.py index 78731d27..16ff90fd 100644 --- a/agave/core/query_params.py +++ b/agave/core/query_params.py @@ -1,31 +1,13 @@ from __future__ import annotations from types import UnionType -from typing import ( - Any, - Iterator, - Protocol, - TypeVar, - Union, - get_args, - get_origin, -) +from typing import Any, Iterator, TypeVar, Union, get_args, get_origin from pydantic import BaseModel ModelT = TypeVar('ModelT', bound=BaseModel) -class QueryParamMapping(Protocol): - def __contains__(self, key: str) -> bool: ... - - def __iter__(self) -> Iterator[str]: ... - - def get(self, key: str, default: Any = None) -> Any: ... - - def getlist(self, key: str) -> list[str]: ... - - def _is_list_annotation(annotation: Any) -> bool: origin = get_origin(annotation) if origin is list: @@ -39,67 +21,28 @@ def _is_list_annotation(annotation: Any) -> bool: return False -def build_query_dict( - query_mapping: QueryParamMapping, model_cls: type[BaseModel] -) -> dict[str, Any]: +def validate_query_params( + query_mapping: Any, model_cls: type[ModelT] +) -> ModelT: params: dict[str, Any] = {} for name in query_mapping: - if name in model_cls.model_fields: - field = model_cls.model_fields[name] - if _is_list_annotation(field.annotation): - params[name] = query_mapping.getlist(name) - else: - params[name] = query_mapping.get(name) + field = model_cls.model_fields.get(name) + if field and _is_list_annotation(field.annotation): + params[name] = query_mapping.getlist(name) else: params[name] = query_mapping.get(name) - return params - - -def validate_query_params( - query_mapping: QueryParamMapping, model_cls: type[ModelT] -) -> ModelT: - return model_cls(**build_query_dict(query_mapping, model_cls)) + return model_cls(**params) class EmptyQueryMapping: def __contains__(self, key: str) -> bool: return False - def get(self, key: str, default: Any = None) -> Any: - return default - - def getlist(self, key: str) -> list[str]: - return [] - - -class ParseQsQueryMapping: - """Query mapping built from a raw query string (e.g. Chalice).""" - - def __init__(self, query_string: str | bytes | None) -> None: - from urllib.parse import parse_qsl - - if not query_string: - self._items: list[tuple[str, str]] = [] - return - if isinstance(query_string, bytes): - query_string = query_string.decode('latin-1') - self._items = parse_qsl(query_string, keep_blank_values=True) - def __iter__(self) -> Iterator[str]: - seen: set[str] = set() - for key, _ in self._items: - if key not in seen: - seen.add(key) - yield key - - def __contains__(self, key: str) -> bool: - return any(k == key for k, _ in self._items) + return iter(()) def get(self, key: str, default: Any = None) -> Any: - values = self.getlist(key) - if not values: - return default - return values[-1] + return default def getlist(self, key: str) -> list[str]: - return [v for k, v in self._items if k == key] + return [] diff --git a/agave/fastapi/rest_api.py b/agave/fastapi/rest_api.py index 062efe1f..316d547e 100644 --- a/agave/fastapi/rest_api.py +++ b/agave/fastapi/rest_api.py @@ -1,6 +1,6 @@ import asyncio import mimetypes -from typing import Any, Optional, cast +from typing import Any, Optional from urllib.parse import urlencode from cuenca_validations.types import QueryParams @@ -23,7 +23,7 @@ from ..core.blueprints.decorators import copy_attributes from ..core.exc import NotFoundError, UnprocessableEntity -from ..core.query_params import QueryParamMapping, validate_query_params +from ..core.query_params import validate_query_params SAMPLE_404 = { "summary": "Not found item", @@ -360,8 +360,7 @@ class QueryResponse(BaseModel): def validate_params(request: Request): try: return validate_query_params( - cast(QueryParamMapping, request.query_params), - cls.query_validator, + request.query_params, cls.query_validator ) except ValidationError as e: raise UnprocessableEntity(e.json()) diff --git a/tests/core/test_query_params.py b/tests/core/test_query_params.py index a90dbc4b..c7f9e0d5 100644 --- a/tests/core/test_query_params.py +++ b/tests/core/test_query_params.py @@ -1,13 +1,12 @@ -from typing import Optional, cast +from typing import Optional import pytest from pydantic import BaseModel, ConfigDict, ValidationError from starlette.datastructures import QueryParams from agave.core.query_params import ( - ParseQsQueryMapping, - QueryParamMapping, - build_query_dict, + EmptyQueryMapping, + _is_list_annotation, validate_query_params, ) @@ -20,50 +19,24 @@ class SampleQuery(BaseModel): active: Optional[bool] = None -def test_build_query_dict_repeated_ids() -> None: - query = cast(QueryParamMapping, QueryParams('ids=US1&ids=US2&active=true')) - params = build_query_dict(query, SampleQuery) - assert params == {'ids': ['US1', 'US2'], 'active': 'true'} - - -def test_build_query_dict_single_id() -> None: - query = cast(QueryParamMapping, QueryParams('ids=US1')) - params = build_query_dict(query, SampleQuery) - assert params == {'ids': ['US1']} - - -def test_build_query_dict_unknown_param_for_extra_forbid() -> None: - query = cast(QueryParamMapping, QueryParams('wrong_param=value')) - params = build_query_dict(query, SampleQuery) - assert params == {'wrong_param': 'value'} - - def test_validate_query_params_list_fields() -> None: - query = cast(QueryParamMapping, QueryParams('ids=a&ids=b&name=Frida')) + query = QueryParams('ids=a&ids=b&name=Frida') validated = validate_query_params(query, SampleQuery) assert validated.ids == ['a', 'b'] assert validated.name == 'Frida' -def test_validate_query_params_rejects_invalid_list() -> None: - # Starlette last-value semantics without getlist would pass a string; - # validate_query_params must still produce a list for list fields. - query = cast(QueryParamMapping, QueryParams('ids=a&ids=b')) +def test_validate_query_params_single_list_value() -> None: + query = QueryParams('ids=US1') validated = validate_query_params(query, SampleQuery) - assert validated.ids == ['a', 'b'] - - -def test_parse_qs_query_mapping() -> None: - mapping = ParseQsQueryMapping('ids=US1&ids=US2&name=test') - assert mapping.getlist('ids') == ['US1', 'US2'] - assert mapping.get('name') == 'test' - assert 'missing' not in mapping + assert validated.ids == ['US1'] -def test_validate_query_params_parse_qs_mapping() -> None: - mapping = ParseQsQueryMapping('ids=x&ids=y') - validated = validate_query_params(mapping, SampleQuery) - assert validated.ids == ['x', 'y'] +def test_validate_query_params_scalar_field() -> None: + query = QueryParams('name=Frida') + validated = validate_query_params(query, SampleQuery) + assert validated.name == 'Frida' + assert validated.ids is None def test_old_unpack_pattern_would_fail_validation() -> None: @@ -73,6 +46,28 @@ def test_old_unpack_pattern_would_fail_validation() -> None: def test_validate_query_params_rejects_unknown_fields() -> None: - query = cast(QueryParamMapping, QueryParams('wrong_param=value')) + query = QueryParams('wrong_param=value') with pytest.raises(ValidationError): validate_query_params(query, SampleQuery) + + +def test_empty_query_mapping() -> None: + mapping = EmptyQueryMapping() + assert 'x' not in mapping + assert mapping.get('x') is None + assert mapping.get('x', 'default') == 'default' + assert mapping.getlist('x') == [] + assert list(mapping) == [] + + +def test_validate_query_params_empty_mapping() -> None: + validated = validate_query_params(EmptyQueryMapping(), SampleQuery) + assert validated.ids is None + assert validated.name is None + + +def test_is_list_annotation() -> None: + assert _is_list_annotation(list[str]) is True + assert _is_list_annotation(Optional[list[str]]) is True + assert _is_list_annotation(str) is False + assert _is_list_annotation(Optional[str]) is False From ce4e91c8beebb07e0d2bbd935278372bb6ca8f72 Mon Sep 17 00:00:00 2001 From: Juliette Date: Fri, 29 May 2026 13:58:19 -0600 Subject: [PATCH 3/6] Enhance query parameter handling to support UnionType fallback --- agave/core/query_params.py | 14 +++++++++++--- tests/core/test_query_params.py | 33 ++++++++++++++++++++++++++++++++- 2 files changed, 43 insertions(+), 4 deletions(-) diff --git a/agave/core/query_params.py b/agave/core/query_params.py index 16ff90fd..5bddf1ef 100644 --- a/agave/core/query_params.py +++ b/agave/core/query_params.py @@ -1,10 +1,18 @@ from __future__ import annotations -from types import UnionType -from typing import Any, Iterator, TypeVar, Union, get_args, get_origin +from typing import Any, Iterator, Tuple, TypeVar, Union, get_args, get_origin from pydantic import BaseModel +try: + from types import UnionType +except ImportError: # Python < 3.10 + UnionType = None # type: ignore[misc, assignment] + +_UNION_ORIGINS: Tuple[Any, ...] = ( + (Union, UnionType) if UnionType is not None else (Union,) +) + ModelT = TypeVar('ModelT', bound=BaseModel) @@ -12,7 +20,7 @@ def _is_list_annotation(annotation: Any) -> bool: origin = get_origin(annotation) if origin is list: return True - if origin in (Union, UnionType): + if origin in _UNION_ORIGINS: return any( _is_list_annotation(arg) for arg in get_args(annotation) diff --git a/tests/core/test_query_params.py b/tests/core/test_query_params.py index c7f9e0d5..d72c0446 100644 --- a/tests/core/test_query_params.py +++ b/tests/core/test_query_params.py @@ -1,4 +1,7 @@ -from typing import Optional +import builtins +import importlib +import sys +from typing import Optional, Union import pytest from pydantic import BaseModel, ConfigDict, ValidationError @@ -71,3 +74,31 @@ def test_is_list_annotation() -> None: assert _is_list_annotation(Optional[list[str]]) is True assert _is_list_annotation(str) is False assert _is_list_annotation(Optional[str]) is False + + +def test_reload_query_params_without_union_type() -> None: + mod_key = 'agave.core.query_params' + original_import = builtins.__import__ + + def custom_import( + name, + globals=None, + locals=None, + fromlist=(), + level=0, + ): + if name == 'types' and fromlist == ('UnionType',): + raise ImportError + return original_import(name, globals, locals, fromlist, level) + + sys.modules.pop(mod_key, None) + builtins.__import__ = custom_import + try: + mod = importlib.import_module(mod_key) + assert mod.UnionType is None + assert mod._UNION_ORIGINS == (Union,) + assert mod._is_list_annotation(Optional[list[str]]) is True + finally: + builtins.__import__ = original_import + sys.modules.pop(mod_key, None) + importlib.import_module(mod_key) From b2178547a4a5cc51dd3bb1ee9f18932132b1c581 Mon Sep 17 00:00:00 2001 From: Juliette Date: Fri, 29 May 2026 14:05:17 -0600 Subject: [PATCH 4/6] Update version to 1.5.4.dev2 --- agave/version.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/agave/version.py b/agave/version.py index 154f11c4..8639ed32 100644 --- a/agave/version.py +++ b/agave/version.py @@ -1 +1 @@ -__version__ = '1.5.4.dev1' +__version__ = '1.5.4.dev2' From 0246bc6665c912b1ea754fab0371881bf3bb33ec Mon Sep 17 00:00:00 2001 From: Juliette Date: Tue, 2 Jun 2026 01:05:54 -0600 Subject: [PATCH 5/6] Update dependencies and enhance query parameter handling --- .github/workflows/test.yml | 2 +- agave/chalice/rest_api.py | 14 ++++--- agave/core/filters.py | 10 +++++ agave/core/query_params.py | 61 +++++++++++++++++---------- agave/fastapi/rest_api.py | 12 +++--- agave/version.py | 2 +- examples/validators.py | 1 - requirements.txt | 2 +- setup.py | 3 +- tests/blueprint/test_blueprint.py | 13 +++--- tests/core/test_query_params.py | 69 +++++++------------------------ 11 files changed, 90 insertions(+), 99 deletions(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index f229b320..efc5f048 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -20,7 +20,7 @@ jobs: runs-on: ubuntu-latest strategy: matrix: - python-version: ['3.9', '3.10', '3.11', '3.12', '3.13'] + python-version: ['3.10', '3.11', '3.12', '3.13'] steps: - uses: actions/checkout@v4 - name: Set up Python ${{ matrix.python-version }} diff --git a/agave/chalice/rest_api.py b/agave/chalice/rest_api.py index 25112b0c..8c9edbcb 100644 --- a/agave/chalice/rest_api.py +++ b/agave/chalice/rest_api.py @@ -15,7 +15,11 @@ from pydantic import BaseModel, ValidationError from ..core.blueprints.decorators import copy_attributes -from ..core.query_params import EmptyQueryMapping, validate_query_params +from ..core.query_params import ( + EmptyQueryMapping, + query_params_for_url, + validate_query_params, +) class RestApiBlueprint(Blueprint): @@ -301,12 +305,12 @@ def _all(query: QueryParams, filters: Q): if wants_more and has_more: query.created_before = item_dicts[-1]['created_at'] path = self.current_request.context['resourcePath'] - params = query.model_dump() + params = query_params_for_url(query) if self.user_id_filter_required(): - params.pop('user_id') + params.pop('user_id', None) if self.platform_id_filter_required(): - params.pop('platform_id') - next_page_uri = f'{path}?{urlencode(params, doseq=True)}' + params.pop('platform_id', None) + next_page_uri = f'{path}?{urlencode(params)}' return dict(items=item_dicts, next_page_uri=next_page_uri) return cls diff --git a/agave/core/filters.py b/agave/core/filters.py index e7b291ce..ea419fe9 100644 --- a/agave/core/filters.py +++ b/agave/core/filters.py @@ -2,12 +2,22 @@ from mongoengine import Q +def _ids_filter_value(ids: str | list[str]) -> list[str]: + if isinstance(ids, str): + return [part.strip() for part in ids.split(',') if part.strip()] + return list(ids) + + def generic_query(query: QueryParams, excluded: list[str] = []) -> Q: filters = Q() if query.created_before: filters &= Q(created_at__lt=query.created_before) if query.created_after: filters &= Q(created_at__gt=query.created_after) + ids = getattr(query, 'ids', None) + if ids is not None: + id_list = _ids_filter_value(ids) + filters &= Q(id__in=id_list) if id_list else Q(id__in=[]) exclude_fields = { 'created_before', 'created_after', diff --git a/agave/core/query_params.py b/agave/core/query_params.py index 5bddf1ef..2b581498 100644 --- a/agave/core/query_params.py +++ b/agave/core/query_params.py @@ -1,26 +1,24 @@ from __future__ import annotations -from typing import Any, Iterator, Tuple, TypeVar, Union, get_args, get_origin +from types import UnionType +from typing import Any, Iterator, TypeVar, Union, get_args, get_origin from pydantic import BaseModel -try: - from types import UnionType -except ImportError: # Python < 3.10 - UnionType = None # type: ignore[misc, assignment] +ModelT = TypeVar('ModelT', bound=BaseModel) -_UNION_ORIGINS: Tuple[Any, ...] = ( - (Union, UnionType) if UnionType is not None else (Union,) -) -ModelT = TypeVar('ModelT', bound=BaseModel) +def comma_separated_list(value: str | None) -> list[str]: + if not value: + return [] + return [part.strip() for part in value.split(',') if part.strip()] def _is_list_annotation(annotation: Any) -> bool: origin = get_origin(annotation) if origin is list: return True - if origin in _UNION_ORIGINS: + if origin in (Union, UnionType): return any( _is_list_annotation(arg) for arg in get_args(annotation) @@ -29,17 +27,41 @@ def _is_list_annotation(annotation: Any) -> bool: return False -def validate_query_params( - query_mapping: Any, model_cls: type[ModelT] -) -> ModelT: +def build_query_dict( + query_mapping: Any, model_cls: type[BaseModel] +) -> dict[str, Any]: params: dict[str, Any] = {} for name in query_mapping: - field = model_cls.model_fields.get(name) - if field and _is_list_annotation(field.annotation): - params[name] = query_mapping.getlist(name) + raw = query_mapping.get(name) + if name in model_cls.model_fields: + field = model_cls.model_fields[name] + if _is_list_annotation(field.annotation): + if raw is None: + continue + if isinstance(raw, str): + params[name] = comma_separated_list(raw) + else: + params[name] = list(raw) + else: + params[name] = raw else: - params[name] = query_mapping.get(name) - return model_cls(**params) + params[name] = raw + return params + + +def validate_query_params( + query_mapping: Any, model_cls: type[ModelT] +) -> ModelT: + return model_cls(**build_query_dict(query_mapping, model_cls)) + + +def query_params_for_url(query: BaseModel) -> dict[str, Any]: + params = query.model_dump() + for name, field in type(query).model_fields.items(): + value = params.get(name) + if _is_list_annotation(field.annotation) and isinstance(value, list): + params[name] = ','.join(value) + return params class EmptyQueryMapping: @@ -51,6 +73,3 @@ def __iter__(self) -> Iterator[str]: def get(self, key: str, default: Any = None) -> Any: return default - - def getlist(self, key: str) -> list[str]: - return [] diff --git a/agave/fastapi/rest_api.py b/agave/fastapi/rest_api.py index 316d547e..213fc833 100644 --- a/agave/fastapi/rest_api.py +++ b/agave/fastapi/rest_api.py @@ -23,7 +23,7 @@ from ..core.blueprints.decorators import copy_attributes from ..core.exc import NotFoundError, UnprocessableEntity -from ..core.query_params import validate_query_params +from ..core.query_params import query_params_for_url, validate_query_params SAMPLE_404 = { "summary": "Not found item", @@ -433,14 +433,12 @@ async def _all(query: QueryParams, filters: Q, resource_path: str): next_page_uri: Optional[str] = None if wants_more and has_more: query.created_before = item_dicts[-1]['created_at'] - params = query.model_dump() + params = query_params_for_url(query) if self.user_id_filter_required(): - params.pop('user_id') + params.pop('user_id', None) if self.platform_id_filter_required(): - params.pop('platform_id') - next_page_uri = ( - f'{resource_path}?{urlencode(params, doseq=True)}' - ) + params.pop('platform_id', None) + next_page_uri = f'{resource_path}?{urlencode(params)}' return dict(items=item_dicts, next_page_uri=next_page_uri) return cls diff --git a/agave/version.py b/agave/version.py index 8639ed32..a54e32fc 100644 --- a/agave/version.py +++ b/agave/version.py @@ -1 +1 @@ -__version__ = '1.5.4.dev2' +__version__ = '1.5.4.dev3' diff --git a/examples/validators.py b/examples/validators.py index 758d92e3..7fe22071 100644 --- a/examples/validators.py +++ b/examples/validators.py @@ -10,7 +10,6 @@ class AccountQuery(QueryParams): user_id: Optional[str] = None platform_id: Optional[str] = None active: Optional[bool] = None - ids: Optional[list[str]] = None class TransactionQuery(QueryParams): diff --git a/requirements.txt b/requirements.txt index a32f0386..aa2ed858 100644 --- a/requirements.txt +++ b/requirements.txt @@ -1,6 +1,6 @@ boto3==1.35.74 types-boto3[sqs]==1.35.74 -cuenca-validations==2.1.3 +cuenca-validations==2.1.35.dev2 chalice==1.31.3 mongoengine==0.29.1 fastapi==0.115.11 diff --git a/setup.py b/setup.py index bd8803d0..2bc77786 100644 --- a/setup.py +++ b/setup.py @@ -20,7 +20,7 @@ packages=find_packages(), include_package_data=True, package_data=dict(agave=['py.typed']), - python_requires='>=3.9', + python_requires='>=3.10', install_requires=[ 'cuenca-validations>=2.1.0,<3.0.0', 'mongoengine>=0.29.0,<0.30.0', @@ -54,7 +54,6 @@ ], }, classifiers=[ - 'Programming Language :: Python :: 3.9', 'Programming Language :: Python :: 3.10', 'Programming Language :: Python :: 3.11', 'Programming Language :: Python :: 3.12', diff --git a/tests/blueprint/test_blueprint.py b/tests/blueprint/test_blueprint.py index 21bed58f..6ecbf6ab 100644 --- a/tests/blueprint/test_blueprint.py +++ b/tests/blueprint/test_blueprint.py @@ -286,12 +286,12 @@ def test_query_count_resource( "client_fixture", ["fastapi_client", "chalice_client"] ) @pytest.mark.usefixtures('accounts') -def test_query_with_repeated_ids_param( +def test_query_with_comma_separated_ids_param( client_fixture: str, request: pytest.FixtureRequest, ) -> None: client = request.getfixturevalue(client_fixture) - resp = client.get('/accounts?ids=US1&ids=US2') + resp = client.get('/accounts?ids=US1,US2') assert resp.status_code == 200 assert 'items' in resp.json() @@ -300,21 +300,20 @@ def test_query_with_repeated_ids_param( "client_fixture", ["fastapi_client", "chalice_client"] ) @pytest.mark.usefixtures('accounts') -def test_query_pagination_preserves_repeated_ids( +def test_query_pagination_preserves_comma_separated_ids( client_fixture: str, request: pytest.FixtureRequest, accounts: list[Account], ) -> None: client = request.getfixturevalue(client_fixture) account_ids = [accounts[0].id, accounts[1].id] - query = '&'.join(f'ids={account_id}' for account_id in account_ids) - resp = client.get(f'/accounts?{query}&page_size=1&limit=10') + ids_param = ','.join(account_ids) + resp = client.get(f'/accounts?ids={ids_param}&page_size=1&limit=10') assert resp.status_code == 200 json_body = resp.json() next_page_uri = json_body['next_page_uri'] assert next_page_uri is not None - for account_id in account_ids: - assert f'ids={account_id}' in next_page_uri + assert f'ids={ids_param}' in next_page_uri.replace('%2C', ',') resp = client.get(next_page_uri) assert resp.status_code == 200 diff --git a/tests/core/test_query_params.py b/tests/core/test_query_params.py index d72c0446..982b8fea 100644 --- a/tests/core/test_query_params.py +++ b/tests/core/test_query_params.py @@ -1,7 +1,4 @@ -import builtins -import importlib -import sys -from typing import Optional, Union +from typing import Optional import pytest from pydantic import BaseModel, ConfigDict, ValidationError @@ -9,7 +6,8 @@ from agave.core.query_params import ( EmptyQueryMapping, - _is_list_annotation, + comma_separated_list, + query_params_for_url, validate_query_params, ) @@ -22,17 +20,19 @@ class SampleQuery(BaseModel): active: Optional[bool] = None -def test_validate_query_params_list_fields() -> None: - query = QueryParams('ids=a&ids=b&name=Frida') - validated = validate_query_params(query, SampleQuery) - assert validated.ids == ['a', 'b'] - assert validated.name == 'Frida' +def test_comma_separated_list() -> None: + assert comma_separated_list('a,b') == ['a', 'b'] + assert comma_separated_list('a, b ,c') == ['a', 'b', 'c'] + assert comma_separated_list('US1') == ['US1'] + assert comma_separated_list('') == [] + assert comma_separated_list(None) == [] -def test_validate_query_params_single_list_value() -> None: - query = QueryParams('ids=US1') +def test_validate_query_params_comma_separated_ids() -> None: + query = QueryParams('ids=a,b&name=Frida') validated = validate_query_params(query, SampleQuery) - assert validated.ids == ['US1'] + assert validated.ids == ['a', 'b'] + assert validated.name == 'Frida' def test_validate_query_params_scalar_field() -> None: @@ -42,12 +42,6 @@ def test_validate_query_params_scalar_field() -> None: assert validated.ids is None -def test_old_unpack_pattern_would_fail_validation() -> None: - query = QueryParams('ids=US1&ids=US2') - with pytest.raises(ValidationError): - SampleQuery(**dict(query)) # type: ignore[arg-type] - - def test_validate_query_params_rejects_unknown_fields() -> None: query = QueryParams('wrong_param=value') with pytest.raises(ValidationError): @@ -59,7 +53,6 @@ def test_empty_query_mapping() -> None: assert 'x' not in mapping assert mapping.get('x') is None assert mapping.get('x', 'default') == 'default' - assert mapping.getlist('x') == [] assert list(mapping) == [] @@ -69,36 +62,6 @@ def test_validate_query_params_empty_mapping() -> None: assert validated.name is None -def test_is_list_annotation() -> None: - assert _is_list_annotation(list[str]) is True - assert _is_list_annotation(Optional[list[str]]) is True - assert _is_list_annotation(str) is False - assert _is_list_annotation(Optional[str]) is False - - -def test_reload_query_params_without_union_type() -> None: - mod_key = 'agave.core.query_params' - original_import = builtins.__import__ - - def custom_import( - name, - globals=None, - locals=None, - fromlist=(), - level=0, - ): - if name == 'types' and fromlist == ('UnionType',): - raise ImportError - return original_import(name, globals, locals, fromlist, level) - - sys.modules.pop(mod_key, None) - builtins.__import__ = custom_import - try: - mod = importlib.import_module(mod_key) - assert mod.UnionType is None - assert mod._UNION_ORIGINS == (Union,) - assert mod._is_list_annotation(Optional[list[str]]) is True - finally: - builtins.__import__ = original_import - sys.modules.pop(mod_key, None) - importlib.import_module(mod_key) +def test_query_params_for_url_serializes_list_as_comma_separated() -> None: + query = SampleQuery(ids=['a', 'b'], name='Frida') + assert query_params_for_url(query)['ids'] == 'a,b' From f3cdc4582aa610dbe88abd7e7ece5012425610fe Mon Sep 17 00:00:00 2001 From: Juliette Date: Tue, 2 Jun 2026 12:33:33 -0600 Subject: [PATCH 6/6] Add tests for ID filtering in generic query and enhance query parameter handling --- tests/core/test_filters.py | 36 ++++++++++++++++++++++++++- tests/core/test_query_params.py | 44 +++++++++++++++++++++++++++++++++ 2 files changed, 79 insertions(+), 1 deletion(-) diff --git a/tests/core/test_filters.py b/tests/core/test_filters.py index 2b87d6c9..76bb6c85 100644 --- a/tests/core/test_filters.py +++ b/tests/core/test_filters.py @@ -2,7 +2,7 @@ from cuenca_validations.types import QueryParams -from agave.core.filters import generic_query +from agave.core.filters import _ids_filter_value, generic_query def test_generic_query_before(): @@ -17,3 +17,37 @@ def test_generic_query_after(): query = generic_query(params) assert "created_at__gt" in repr(query) assert "user" not in repr(query) + + +def test_ids_filter_value_from_string() -> None: + assert _ids_filter_value('US1, US2') == ['US1', 'US2'] + + +def test_ids_filter_value_from_list() -> None: + assert _ids_filter_value(['US1', 'US2']) == ['US1', 'US2'] + + +def test_generic_query_filters_by_ids_list() -> None: + params = QueryParams.model_construct(ids=['US1', 'US2']) + query = generic_query(params) + assert 'id__in' in repr(query) + assert 'US1' in repr(query) + + +def test_generic_query_filters_by_ids_comma_separated_string() -> None: + params = QueryParams.model_construct(ids='US1,US2') + query = generic_query(params) + assert 'id__in' in repr(query) + assert 'US1' in repr(query) + + +def test_generic_query_empty_ids_string() -> None: + params = QueryParams.model_construct(ids='') + query = generic_query(params) + assert 'id__in' in repr(query) + + +def test_generic_query_excludes_count_field() -> None: + params = QueryParams.model_construct(count=True) + query = generic_query(params) + assert 'count' not in repr(query) diff --git a/tests/core/test_query_params.py b/tests/core/test_query_params.py index 982b8fea..f7b3737d 100644 --- a/tests/core/test_query_params.py +++ b/tests/core/test_query_params.py @@ -6,6 +6,7 @@ from agave.core.query_params import ( EmptyQueryMapping, + build_query_dict, comma_separated_list, query_params_for_url, validate_query_params, @@ -65,3 +66,46 @@ def test_validate_query_params_empty_mapping() -> None: def test_query_params_for_url_serializes_list_as_comma_separated() -> None: query = SampleQuery(ids=['a', 'b'], name='Frida') assert query_params_for_url(query)['ids'] == 'a,b' + + +def test_query_params_for_url_skips_none_list_fields() -> None: + query = SampleQuery(name='Frida') + params = query_params_for_url(query) + assert params['name'] == 'Frida' + assert params.get('ids') is None + + +class _ListQueryMapping: + def __init__(self, values: dict[str, object]) -> None: + self._values = values + + def __iter__(self): + return iter(self._values) + + def get(self, key: str, default: object = None) -> object: + return self._values.get(key, default) + + +def test_build_query_dict_list_from_non_string_value() -> None: + mapping = _ListQueryMapping({'ids': ['a', 'b']}) + params = build_query_dict(mapping, SampleQuery) + assert params['ids'] == ['a', 'b'] + + +def test_build_query_dict_skips_none_list_value() -> None: + mapping = _ListQueryMapping({'ids': None}) + params = build_query_dict(mapping, SampleQuery) + assert 'ids' not in params + + +class _ExtraFieldQuery(BaseModel): + model_config = ConfigDict(extra='allow') + + name: Optional[str] = None + + +def test_build_query_dict_unknown_field() -> None: + mapping = _ListQueryMapping({'unknown': 'value', 'name': 'Frida'}) + params = build_query_dict(mapping, _ExtraFieldQuery) + assert params['unknown'] == 'value' + assert params['name'] == 'Frida'