Update version to 1.5.4.dev1 and enhance query parameter handling#185
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR refactors query parameter handling in REST API handlers across Chalice and FastAPI frameworks. It introduces a new utility module ( Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #185 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 19 20 +1
Lines 716 772 +56
Branches 75 88 +13
=========================================
+ Hits 716 772 +56
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
tests/core/test_query_params.py (2)
75-79: ⚡ Quick winAdd a regression test for the empty-query mapping path.
Please add a test that calls
validate_query_params(EmptyQueryMapping(), SampleQuery)and asserts it does not raise and keeps all optional fields asNone.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/core/test_query_params.py` around lines 75 - 79, Add a new regression test that calls validate_query_params with an EmptyQueryMapping and SampleQuery and asserts it does not raise and returns a mapping where all optional SampleQuery fields are None; specifically, create a test function (e.g., test_validate_query_params_accepts_empty_mapping) that does: result = validate_query_params(EmptyQueryMapping(), SampleQuery) and then asserts no exception and that for each optional field defined on SampleQuery the result mapping has a key with value None (use the same field names as in SampleQuery to locate them).
48-53: ⚡ Quick winRename this test to match its behavior.
The current name says “rejects invalid list”, but it verifies successful normalization to
['a', 'b']. This is misleading during failure triage.Suggested rename
-def test_validate_query_params_rejects_invalid_list() -> None: +def test_validate_query_params_normalizes_list_fields() -> None:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/core/test_query_params.py` around lines 48 - 53, Rename the test function test_validate_query_params_rejects_invalid_list to a name that reflects its behavior (it verifies normalization rather than rejection), e.g. test_validate_query_params_normalizes_list_field or test_validate_query_params_returns_list_for_multiple_values; update the function definition and any references accordingly so the test still calls QueryParams('ids=a&ids=b'), invokes validate_query_params(query, SampleQuery), and asserts validated.ids == ['a', 'b'].
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@agave/core/query_params.py`:
- Around line 64-72: EmptyQueryMapping is missing iteration support required by
build_query_dict and the QueryParamMapping protocol; implement __iter__ to yield
no keys and __len__ to return 0 (and keep existing __contains__, get, getlist)
so EmptyQueryMapping behaves like an empty mapping when iterated by
build_query_dict, e.g., add an __iter__(self) -> Iterator[str] that yields
nothing and a __len__(self) -> int that returns 0 to satisfy consumers expecting
an iterable mapping.
In `@tests/blueprint/test_blueprint.py`:
- Around line 289-297: The test test_query_with_repeated_ids_param currently
only checks status and presence of 'items' but doesn't assert filtering; modify
it to parse resp.json(), extract the list under 'items', and assert that every
returned item's id is one of the requested ids ('US1' or 'US2') and that no
other ids are present (optionally assert the set of returned ids equals
{'US1','US2'} or that length matches expected). Use the existing
client.get('/accounts?ids=US1&ids=US2') response object (resp) and its
resp.json() to perform these assertions so the test fails if ids filtering is
ignored.
---
Nitpick comments:
In `@tests/core/test_query_params.py`:
- Around line 75-79: Add a new regression test that calls validate_query_params
with an EmptyQueryMapping and SampleQuery and asserts it does not raise and
returns a mapping where all optional SampleQuery fields are None; specifically,
create a test function (e.g., test_validate_query_params_accepts_empty_mapping)
that does: result = validate_query_params(EmptyQueryMapping(), SampleQuery) and
then asserts no exception and that for each optional field defined on
SampleQuery the result mapping has a key with value None (use the same field
names as in SampleQuery to locate them).
- Around line 48-53: Rename the test function
test_validate_query_params_rejects_invalid_list to a name that reflects its
behavior (it verifies normalization rather than rejection), e.g.
test_validate_query_params_normalizes_list_field or
test_validate_query_params_returns_list_for_multiple_values; update the function
definition and any references accordingly so the test still calls
QueryParams('ids=a&ids=b'), invokes validate_query_params(query, SampleQuery),
and asserts validated.ids == ['a', 'b'].
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4f0f809e-0417-47be-b8f1-e535d2006294
📒 Files selected for processing (8)
agave/chalice/rest_api.pyagave/core/filters.pyagave/core/query_params.pyagave/fastapi/rest_api.pyagave/version.pyexamples/validators.pytests/blueprint/test_blueprint.pytests/core/test_query_params.py
| 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() | ||
|
|
There was a problem hiding this comment.
Test does not verify that ids filtering actually works.
At Line 294, this test sends repeated ids, but current assertions only check status and key existence, so it still passes if ids is ignored.
✅ Suggested test hardening
`@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,
+ accounts: list[Account],
) -> None:
client = request.getfixturevalue(client_fixture)
- resp = client.get('/accounts?ids=US1&ids=US2')
+ 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}')
assert resp.status_code == 200
- assert 'items' in resp.json()
+ json_body = resp.json()
+ assert 'items' in json_body
+ returned_ids = {item['id'] for item in json_body['items']}
+ assert returned_ids == set(account_ids)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/blueprint/test_blueprint.py` around lines 289 - 297, The test
test_query_with_repeated_ids_param currently only checks status and presence of
'items' but doesn't assert filtering; modify it to parse resp.json(), extract
the list under 'items', and assert that every returned item's id is one of the
requested ids ('US1' or 'US2') and that no other ids are present (optionally
assert the set of returned ids equals {'US1','US2'} or that length matches
expected). Use the existing client.get('/accounts?ids=US1&ids=US2') response
object (resp) and its resp.json() to perform these assertions so the test fails
if ids filtering is ignored.
| 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,) | ||
| ) |
There was a problem hiding this comment.
Quitar soporte para <3.10 y dejar más simple el import
| params[name] = query_mapping.getlist(name) | ||
| else: | ||
| params[name] = query_mapping.get(name) | ||
| return model_cls(**params) |
There was a problem hiding this comment.
Estuve revisando y al parecer no hay una forma estandar de pasar listas de parametros.
Veo que estas adoptando esta forma
endpoint?param=value_1¶m=value2
Creo que es más simple si lo haces separado por ,
endpoint?param=value_1,value2
El parametro lo puedes poner como str y hacer la conversión aqui en agave
class (QueryParams):
ids:str Asi te evitas estas funciones auxiliares para aceptar la lista
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@agave/core/filters.py`:
- Line 11: The function generic_query currently uses a mutable default argument
excluded: list[str] = []; change the signature to use excluded:
Optional[list[str]] = None and inside generic_query initialize excluded = [] if
excluded is None to avoid sharing the same list across calls; update any type
hints/imports as needed and ensure all places that reference excluded still work
with the possibly None input before initialization.
In `@agave/core/query_params.py`:
- Around line 58-64: The function query_params_for_url currently uses
query.model_dump() without excluding None, which lets Optional fields become the
string "None" when later urlencoded; change it to omit None-valued keys by
calling query.model_dump(exclude_none=True) (or filter out keys with value is
None from the params dict) before the list-annotation handling so that Optional
fields are not serialized into the query string; keep the existing
_is_list_annotation and join logic intact after the None-filtering.
In `@tests/core/test_query_params.py`:
- Around line 65-67: The test only checks list serialization but not
None-handling; update
test_query_params_for_url_serializes_list_as_comma_separated to also assert that
None-valued fields from SampleQuery (e.g., a field like cursor or other optional
attribute) are not emitted as the string "None" by calling
query_params_for_url(query) and asserting the result either does not contain
that key or that result.get('cursor') is None, ensuring query_params_for_url
preserves omission of None values rather than serializing them to "None".
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7f537742-beea-4b56-b59d-4b5a7f1ec75a
📒 Files selected for processing (10)
.github/workflows/test.ymlagave/chalice/rest_api.pyagave/core/filters.pyagave/core/query_params.pyagave/fastapi/rest_api.pyagave/version.pyrequirements.txtsetup.pytests/blueprint/test_blueprint.pytests/core/test_query_params.py
✅ Files skipped from review due to trivial changes (2)
- requirements.txt
- agave/version.py
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/blueprint/test_blueprint.py
- agave/chalice/rest_api.py
| return list(ids) | ||
|
|
||
|
|
||
| def generic_query(query: QueryParams, excluded: list[str] = []) -> Q: |
There was a problem hiding this comment.
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 with None and initialize inside the function.
🔧 Proposed fix
-def generic_query(query: QueryParams, excluded: list[str] = []) -> Q:
+def generic_query(query: QueryParams, excluded: list[str] | None = None) -> Q:
+ if excluded is None:
+ excluded = []
filters = Q()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
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def generic_query(query: QueryParams, excluded: list[str] = []) -> Q: | |
| def generic_query(query: QueryParams, excluded: list[str] | None = None) -> Q: | |
| if excluded is None: | |
| excluded = [] | |
| filters = Q() |
🧰 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
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@agave/core/filters.py` at line 11, The function generic_query currently uses
a mutable default argument excluded: list[str] = []; change the signature to use
excluded: Optional[list[str]] = None and inside generic_query initialize
excluded = [] if excluded is None to avoid sharing the same list across calls;
update any type hints/imports as needed and ensure all places that reference
excluded still work with the possibly None input before initialization.
2 participants
Summary by CodeRabbit
New Features
idsquery parameter with comma-separated values across query endpoints.idsfilters in next page URLs.Tests
Chores
1.5.4.dev3.cuenca-validationsdependency.