-
Notifications
You must be signed in to change notification settings - Fork 1
Update version to 1.5.4.dev1 and enhance query parameter handling #185
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
6a81f13
0e53811
ce4e91c
b217854
0246bc6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,75 @@ | ||
| from __future__ import annotations | ||
|
|
||
| from types import UnionType | ||
| from typing import Any, Iterator, TypeVar, Union, get_args, get_origin | ||
|
|
||
| from pydantic import BaseModel | ||
|
|
||
| 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, 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: Any, model_cls: type[BaseModel] | ||
| ) -> dict[str, Any]: | ||
| params: dict[str, Any] = {} | ||
| for name in query_mapping: | ||
| 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] = 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 | ||
|
Comment on lines
+58
to
+64
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ❓ Verification inconclusive🏁 Script executed: #!/bin/bash
python - <<'PY'
from cuenca_validations.types import QueryParams
for n, f in QueryParams.model_fields.items():
print(n, f.default, f.annotation)
PY
git log -n 5 --oneline -- agave/fastapi/rest_api.py
git show HEAD~1:agave/fastapi/rest_api.py 2>/dev/null | sed -n '430,445p'Repository: cuenca-mx/agave Length of output: 273 🏁 Script executed: #!/bin/bash
set -euo pipefail
# Locate QueryParams in-repo
fd -a "query_params.py" -t f . || true
rg -n "class QueryParams|QueryParams\s*=" . || true
rg -n "query_params_for_url\(" agave || true
# Inspect the exact urlencode/next_page_uri code path
rg -n "next_page_uri|urlencode\(" agave/fastapi/rest_api.py agave/fastapi -S || true
# Show the current function implementation and surrounding context
rg -n "def query_params_for_url" agave/core/query_params.py -n
sed -n '1,140p' agave/core/query_params.py
# Show the relevant portion of rest_api.py (around the urlencode/next_page_uri)
sed -n '380,520p' agave/fastapi/rest_api.py || trueRepository: cuenca-mx/agave Length of output: 6974 🏁 Script executed: #!/bin/bash
set -euo pipefail
# Find the QueryParams model definition
rg -n "class QueryParams|QueryParams\s*=" . -S
# Inspect the tests for query_params_for_url / next_page_uri
sed -n '1,220p' tests/core/test_query_params.py 2>/dev/null || true
# If QueryParams is defined elsewhere, print a smaller window around it
# (Uses file discovery first)
query_file="$(rg -l "class QueryParams" . -S | head -n 1 || true)"
if [ -n "${query_file}" ]; then
echo "QueryParams file: $query_file"
# Print around the class definition
rg -n "class QueryParams" "$query_file" -n
line="$(rg -n "class QueryParams" "$query_file" | head -n 1 | cut -d: -f1)"
start=$((line-30))
end=$((line+220))
sed -n "${start},${end}p" "$query_file"
fi
# Also inspect query_params_for_url-focused tests, if any
rg -n "query_params_for_url|next_page_uri" tests/core/test_query_params.py tests -S || trueRepository: cuenca-mx/agave Length of output: 41 🏁 Script executed: #!/bin/bash
set -u
set +e
rg -n "class QueryParams|QueryParams\s*=" . -S
echo "---- tests/core/test_query_params.py ----"
ls -la tests/core/test_query_params.py 2>/dev/null || ls -la ./tests/core/test_query_params.py 2>/dev/null
sed -n '1,240p' tests/core/test_query_params.py 2>/dev/null || sed -n '1,240p' ./tests/core/test_query_params.py 2>/dev/null
echo "---- search for next_page_uri / query_params_for_url in tests ----"
rg -n "query_params_for_url|next_page_uri" tests -S || trueRepository: cuenca-mx/agave Length of output: 3331 🏁 Script executed: #!/bin/bash
set -euo pipefail
# Locate the QueryParams model definition
rg -n "class QueryParams\b" agave -S || true
rg -n "QueryParams\(" agave -S || true
# Search for likely field names to find the model
rg -n "created_after|created_before|page_size|platform_id|user_id|limit:" agave -S || true
rg -n "created_after|created_before|page_size|platform_id|user_id|limit" agave -S --glob='*.py'
echo "---- inspect pagination tests around next_page_uri ----"
sed -n '250,620p' tests/blueprint/test_blueprint.pyRepository: cuenca-mx/agave Length of output: 23536 🏁 Script executed: #!/bin/bash
set -euo pipefail
# Find QueryParams definition across whole repo
rg -n "class QueryParams\b" . -S || true
rg -n "QueryParams\s*=\s*|QueryParams\s*\(" . -S || true
# Search for core filter logic to see which fields are used and how
sed -n '1,160p' agave/core/filters.py
# Inspect next-page tests that may expose inclusion of None params
rg -n "next_page_uri" tests/blueprint/test_blueprint.py -n
sed -n '280,430p' tests/blueprint/test_blueprint.py
sed -n '430,590p' tests/blueprint/test_blueprint.py
# Inspect blueprint models / query param models in tests folder
rg -n "created_before|created_after|page_size|limit:" tests -S --glob='*.py'
rg -n "platform_id|user_id" tests -S --glob='*.py'Repository: cuenca-mx/agave Length of output: 17364 🏁 Script executed: #!/bin/bash
set -euo pipefail
sed -n '1,220p' tests/core/test_query_params.py
sed -n '220,500p' tests/core/test_query_params.py
# Search for expectations about "None" in query params / next_page_uri
rg -n "\"None\"|limit=None|created_before=None|created_after=None|exclude_none" tests -S || true
# Inspect agave/core/query_params.py to see if model_dump is customized by config elsewhere
sed -n '1,120p' agave/core/query_params.pyRepository: cuenca-mx/agave Length of output: 4231 🏁 Script executed: #!/bin/bash
set -euo pipefail
# Search for any Pydantic config/default that might exclude None during model_dump
rg -n "exclude_none" agave tests -S --glob='*.py' || true
# Search for field definitions used by pagination (created_before/after, page_size, limit, etc.)
rg -n "created_before|created_after|page_size|limit:" . -S --glob='*.py' || true
# Inspect the QueryParams reference type in tests (import path indicates external package)
rg -n "from cuenca_validations\.types import QueryParams" -S . || true
# Also check if agave provides its own QueryParams somewhere else (besides external)
rg -n "QueryParams\s*:" agave -S --glob='*.py' || true
rg -n "class .*QueryParams" . -S --glob='*.py' || trueRepository: cuenca-mx/agave Length of output: 2722 🏁 Script executed: #!/bin/bash
set -euo pipefail
# Look for any model_dump override in this repo
rg -n "def model_dump\(" . -S --glob='*.py' || true
# Inspect query param field declarations in examples (subclasses of cuenca_validations.types.QueryParams)
sed -n '1,220p' examples/validators.py
# Inspect imports/top of rest_api.py to confirm urlencode source
sed -n '1,120p' agave/fastapi/rest_api.py
sed -n '400,470p' agave/fastapi/rest_api.py
# Inspect chalice version too (also uses query_params_for_url)
sed -n '240,360p' agave/chalice/rest_api.pyRepository: cuenca-mx/agave Length of output: 11511 🌐 Web query:
💡 Result: In Pydantic (and libraries utilizing it, such as cuenca-validations), model_dump is a method used to convert a model instance into a dictionary [1][2]. When using the exclude_none=True argument, all fields with a value of None are excluded from the output [1][3]. This functionality is a standard feature of Pydantic v2 [4][3]. In the context of QueryParams or similar objects, calling.model_dump(exclude_none=True) is a common pattern to clean up request or filtering data, ensuring that parameters explicitly set to None are not included in the resulting dictionary, which helps prevent issues when constructing subsequent queries or API requests [1][4]. While cuenca-validations is a specialized library used by the Cuenca organization for common shared validations [5][6], it relies on Pydantic v2 [6][7]. Therefore, its models behave like standard Pydantic models, and you should use the official Pydantic documentation for behaviors related to model_dump and field exclusion [1][2][8]. Important Considerations: - Precedence: When using both include and exclude_none=True in model_dump, the exclusion logic often takes precedence, meaning even if you explicitly include a field, it may be dropped if its value is None [4]. - Alternatives: If you need to keep specific fields even if they are None, you may need to use a model_serializer to explicitly handle that logic [4]. - Dependency: Ensure your environment uses a compatible version of Pydantic (v2.10.0 or higher is typically used in recent versions of cuenca-validations) [6][7]. Citations:
🏁 Script executed: #!/bin/bash
set -euo pipefail
# Find where cuenca-validations is pinned
rg -n "cuenca-validations|cuenca_validations" pyproject.toml requirements*.txt setup.cfg setup.py poetry.lock Pipfile.lock -S || true
# Print pyproject/build metadata sections that might include dependencies
ls -ლა
ls -la pyproject.toml 2>/dev/null || true
sed -n '1,220p' pyproject.toml 2>/dev/null || trueRepository: cuenca-mx/agave Length of output: 408 🌐 Web query:
💡 Result: The cuenca-validations library is a shared Python package used across various Cuenca projects [1][2]. The QueryParams class within this library is a Pydantic model (inheriting from BaseModel) used to standardize query parameter handling [1][3][4]. When you invoke model_dump(exclude_none=True) on a QueryParams instance, Pydantic's native behavior is applied [5][3]. This method generates a dictionary representation of the model while omitting any fields that have a value of None [3]. If you are encountering issues where this behavior does not meet your specific requirements (such as needing to exclude None values while still including certain fields, or vice versa), Pydantic provides several mechanisms to customize serialization [5][6]: 1. model_serializer: You can use the Citations:
🌐 Web query:
💡 Result: I can’t find any source that shows the exact Cuenca type definition you referenced (the file path and the snippet: Citations:
Exclude
Exclude 🐛 Proposed fix def query_params_for_url(query: BaseModel) -> dict[str, Any]:
- params = query.model_dump()
+ params = query.model_dump(exclude_none=True)
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🤖 Prompt for AI Agents |
||
|
|
||
|
|
||
| class EmptyQueryMapping: | ||
| def __contains__(self, key: str) -> bool: | ||
| return False | ||
|
|
||
| def __iter__(self) -> Iterator[str]: | ||
| return iter(()) | ||
|
|
||
| def get(self, key: str, default: Any = None) -> Any: | ||
| return default | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1 +1 @@ | ||
| __version__ = '1.5.4' | ||
| __version__ = '1.5.4.dev3' |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,67 @@ | ||
| from typing import Optional | ||
|
|
||
| import pytest | ||
| from pydantic import BaseModel, ConfigDict, ValidationError | ||
| from starlette.datastructures import QueryParams | ||
|
|
||
| from agave.core.query_params import ( | ||
| EmptyQueryMapping, | ||
| comma_separated_list, | ||
| query_params_for_url, | ||
| 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_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_comma_separated_ids() -> None: | ||
| query = QueryParams('ids=a,b&name=Frida') | ||
| validated = validate_query_params(query, SampleQuery) | ||
| assert validated.ids == ['a', 'b'] | ||
| assert validated.name == 'Frida' | ||
|
|
||
|
|
||
| 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_validate_query_params_rejects_unknown_fields() -> None: | ||
| 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 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_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' | ||
|
Comment on lines
+65
to
+67
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider asserting This test only checks the list field. Adding an assertion that 💚 Suggested addition 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'
+ params = query_params_for_url(query)
+ assert params['ids'] == 'a,b'
+ assert 'active' not in params # None fields must not leak as "None"🤖 Prompt for AI Agents |
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix mutable default argument.
The default argument
excluded: list[str] = []is a mutable object that is shared across all function calls. If any caller mutates this list, it will affect subsequent calls. Replace withNoneand initialize inside the function.🔧 Proposed fix
As per coding guidelines, the static analysis tool Ruff flagged this with rule B006: "Do not use mutable data structures for argument defaults."
📝 Committable suggestion
🧰 Tools
🪛 Ruff (0.15.15)
[warning] 11-11: Do not use mutable data structures for argument defaults
Replace with
None; initialize within function(B006)
🤖 Prompt for AI Agents