From 1c0cd11ff97305be70895e3ea02091b39c6314c7 Mon Sep 17 00:00:00 2001 From: Gianluca Ficarelli Date: Tue, 14 Oct 2025 17:05:37 +0200 Subject: [PATCH 1/2] Remove legacy support for comma separated list params --- app/filters/base.py | 24 +------------------ app/filters/common.py | 6 +---- tests/test_calibration.py | 14 ----------- tests/test_cell_morphology.py | 29 ----------------------- tests/test_electrical_cell_recording.py | 8 ------- tests/test_etype.py | 8 ------- tests/test_experimental_bouton_density.py | 8 ------- tests/test_experimental_neuron_density.py | 8 ------- tests/test_ion_channel_recording.py | 8 ------- tests/test_mtype.py | 7 ------ tests/test_simulation_execution.py | 14 ----------- tests/test_simulation_generation.py | 14 ----------- tests/test_validation.py | 14 ----------- 13 files changed, 2 insertions(+), 160 deletions(-) diff --git a/app/filters/base.py b/app/filters/base.py index 6448a6547..2e49879b1 100644 --- a/app/filters/base.py +++ b/app/filters/base.py @@ -9,7 +9,6 @@ from sqlalchemy.orm import DeclarativeBase from app.db.model import Identifiable -from app.logger import L Aliases = dict[type[Identifiable], type[Identifiable] | dict[str, type[Identifiable]]] @@ -25,29 +24,8 @@ class Constants(Filter.Constants): @field_validator("*", mode="before") @classmethod - def split_str(cls, value, field): # pyright: ignore reportIncompatibleMethodOverride + def split_str(cls, value, field): # pyright: ignore reportIncompatibleMethodOverride # noqa: ARG003 """Prevent splitting field logic from parent class.""" - # backwards compatibility by splitting only comma separated single list elements that do not - # have space directly after the comma. e.g "a,b,c" will be split but not 'a, b, c'. - if ( - field.field_name is not None # noqa: PLR0916 - and ( - field.field_name == cls.Constants.ordering_field_name - or field.field_name.endswith("__in") - or field.field_name.endswith("__not_in") - ) - and value - and len(value) == 1 - and isinstance(value[0], str) - and "," in value[0] - and ", " not in value[0] - ): - msg = ( - "Deprecated comma separated single-string IN query used instead of native list. " - f"Filter: field.config['title'] Field name: {field.field_name} Value: {value}" - ) - L.warning(msg) - return value[0].split(",") return value @field_validator("order_by", check_fields=False) diff --git a/app/filters/common.py b/app/filters/common.py index 749027953..3cf001bc4 100644 --- a/app/filters/common.py +++ b/app/filters/common.py @@ -20,11 +20,7 @@ class IdFilterMixin: id: uuid.UUID | None = None - - # id__in needs to be a str for backwards compatibility when instead of a native list a comma - # separated string is provided, e.g. 'id1,id2' . With list[UUID] backwards compatibility would - # fail because of validation of the field which would be expected to be a UUID. - id__in: list[str] | None = None + id__in: list[uuid.UUID] | None = None class AuthorizedFilterMixin: diff --git a/tests/test_calibration.py b/tests/test_calibration.py index 4a7e01730..5c6d3e5a8 100644 --- a/tests/test_calibration.py +++ b/tests/test_calibration.py @@ -232,12 +232,6 @@ def test_filtering(client, models, root_circuit, simulation_result): assert len(data) == 2 """ - # backwards compat - data = assert_request( - client.get, url=ROUTE, params={"used__id__in": f"{root_circuit.id},{simulation_result.id}"} - ).json()["data"] - assert len(data) == 5 - data = assert_request( client.get, url=ROUTE, @@ -245,14 +239,6 @@ def test_filtering(client, models, root_circuit, simulation_result): ).json()["data"] assert len(data) == 5 - # backwards compat - data = assert_request( - client.get, - url=ROUTE, - params={"generated__id__in": f"{root_circuit.id},{simulation_result.id}"}, - ).json()["data"] - assert len(data) == 4 - data = assert_request( client.get, url=ROUTE, diff --git a/tests/test_cell_morphology.py b/tests/test_cell_morphology.py index 7e065c6e2..6093a0c09 100644 --- a/tests/test_cell_morphology.py +++ b/tests/test_cell_morphology.py @@ -649,15 +649,6 @@ def test_filter_by_id__in(db, client, brain_region_id, person_id, subject_id): # filtering by multiple IDs selected_ids = [morphology_ids[1], morphology_ids[3]] - - # backwards compat - response = client.get(ROUTE, params={"id__in": ",".join(selected_ids)}) - assert response.status_code == 200 - data = response.json()["data"] - assert len(data) == 2 - returned_ids = [item["id"] for item in data] - assert set(returned_ids) == set(selected_ids) - response = client.get(ROUTE, params={"id__in": selected_ids}) assert response.status_code == 200 data = response.json()["data"] @@ -666,15 +657,6 @@ def test_filter_by_id__in(db, client, brain_region_id, person_id, subject_id): assert set(returned_ids) == set(selected_ids) # filtering by all IDs - - # backwards compat - response = client.get(ROUTE, params={"id__in": ",".join(morphology_ids)}) - assert response.status_code == 200 - data = response.json()["data"] - assert len(data) == 5 - returned_ids = [item["id"] for item in data] - assert set(returned_ids) == set(morphology_ids) - response = client.get(ROUTE, params={"id__in": morphology_ids}) assert response.status_code == 200 data = response.json()["data"] @@ -689,17 +671,6 @@ def test_filter_by_id__in(db, client, brain_region_id, person_id, subject_id): assert len(data) == 0 # combining id__in with other filters - - # backwards compat - response = client.get( - ROUTE, - params={"id__in": ",".join(morphology_ids), "name__ilike": "%Filter Test Morphology 2%"}, - ) - assert response.status_code == 200 - data = response.json()["data"] - assert len(data) == 1 - assert data[0]["id"] == morphology_ids[2] - response = client.get( ROUTE, params={"id__in": morphology_ids, "name__ilike": "%Filter Test Morphology 2%"}, diff --git a/tests/test_electrical_cell_recording.py b/tests/test_electrical_cell_recording.py index 74e50bbb3..9400e453e 100644 --- a/tests/test_electrical_cell_recording.py +++ b/tests/test_electrical_cell_recording.py @@ -428,14 +428,6 @@ def test_filtering(client, models): ).json()["data"] assert {d["name"] for d in data} == {"e-1", "e-2"} - # backwards compat - data = assert_request( - client.get, - url=ROUTE, - params={"name__in": "e-1,e-2"}, - ).json()["data"] - assert {d["name"] for d in data} == {"e-1", "e-2"} - data = assert_request( client.get, url=ROUTE, diff --git a/tests/test_etype.py b/tests/test_etype.py index 1ed2bd70f..970a6cc78 100644 --- a/tests/test_etype.py +++ b/tests/test_etype.py @@ -93,14 +93,6 @@ def test_retrieve(db, client, person_id): assert data[0] == with_creation_fields(items[5]) # test filter (in) - - # backwards compat - response = client.get(ROUTE, params={"pref_label__in": "pref_label_5,pref_label_6"}) - assert response.status_code == 200 - data = response.json()["data"] - assert len(data) == 2 - assert data == [with_creation_fields(items[5]), with_creation_fields(items[6])] - response = client.get(ROUTE, params={"pref_label__in": ["pref_label_5", "pref_label_6"]}) assert response.status_code == 200 data = response.json()["data"] diff --git a/tests/test_experimental_bouton_density.py b/tests/test_experimental_bouton_density.py index fe884992a..d8afe2c0e 100644 --- a/tests/test_experimental_bouton_density.py +++ b/tests/test_experimental_bouton_density.py @@ -334,14 +334,6 @@ def test_filtering(client, models): ).json()["data"] assert {d["name"] for d in data} == {"d-1", "d-2"} - # backwards compat - data = assert_request( - client.get, - url=ROUTE, - params={"name__in": "d-1,d-2"}, - ).json()["data"] - assert {d["name"] for d in data} == {"d-1", "d-2"} - data = assert_request( client.get, url=ROUTE, params="contribution__pref_label=test_person_1" ).json()["data"] diff --git a/tests/test_experimental_neuron_density.py b/tests/test_experimental_neuron_density.py index 7d590135b..f34768982 100644 --- a/tests/test_experimental_neuron_density.py +++ b/tests/test_experimental_neuron_density.py @@ -353,14 +353,6 @@ def test_filtering(client, models): ).json()["data"] assert {d["name"] for d in data} == {"d-1", "d-2"} - # backwards compat - data = assert_request( - client.get, - url=ROUTE, - params={"name__in": "d-1,d-2"}, - ).json()["data"] - assert {d["name"] for d in data} == {"d-1", "d-2"} - data = assert_request( client.get, url=ROUTE, params="contribution__pref_label=test_person_1" ).json()["data"] diff --git a/tests/test_ion_channel_recording.py b/tests/test_ion_channel_recording.py index e080afdba..8eab9aad4 100644 --- a/tests/test_ion_channel_recording.py +++ b/tests/test_ion_channel_recording.py @@ -443,14 +443,6 @@ def test_filtering(client, models): ).json()["data"] assert {d["name"] for d in data} == {"e-1", "e-2"} - # backwards compat - data = assert_request( - client.get, - url=ROUTE, - params={"name__in": "e-1,e-2"}, - ).json()["data"] - assert {d["name"] for d in data} == {"e-1", "e-2"} - data = assert_request( client.get, url=ROUTE, diff --git a/tests/test_mtype.py b/tests/test_mtype.py index a84033f1c..771812547 100644 --- a/tests/test_mtype.py +++ b/tests/test_mtype.py @@ -94,13 +94,6 @@ def test_retrieve(db, client, person_id): assert data[0] == with_creation_fields(items[5]) # test filter (in) - # backwards compat - response = client.get(ROUTE, params={"pref_label__in": "pref_label_5,pref_label_6"}) - assert response.status_code == 200 - data = response.json()["data"] - assert len(data) == 2 - assert data == [with_creation_fields(items[5]), with_creation_fields(items[6])] - response = client.get(ROUTE, params={"pref_label__in": ["pref_label_5", "pref_label_6"]}) assert response.status_code == 200 data = response.json()["data"] diff --git a/tests/test_simulation_execution.py b/tests/test_simulation_execution.py index 94998b150..4e8fc9683 100644 --- a/tests/test_simulation_execution.py +++ b/tests/test_simulation_execution.py @@ -223,12 +223,6 @@ def test_filtering(client, models, root_circuit, simulation_result): ).json()["data"] assert len(data) == 2 - # backwards compat - data = assert_request( - client.get, url=ROUTE, params={"used__id__in": f"{root_circuit.id},{simulation_result.id}"} - ).json()["data"] - assert len(data) == 5 - data = assert_request( client.get, url=ROUTE, @@ -236,14 +230,6 @@ def test_filtering(client, models, root_circuit, simulation_result): ).json()["data"] assert len(data) == 5 - # backwards compat - data = assert_request( - client.get, - url=ROUTE, - params={"generated__id__in": f"{root_circuit.id},{simulation_result.id}"}, - ).json()["data"] - assert len(data) == 4 - data = assert_request( client.get, url=ROUTE, diff --git a/tests/test_simulation_generation.py b/tests/test_simulation_generation.py index d5849d164..b424fd4ff 100644 --- a/tests/test_simulation_generation.py +++ b/tests/test_simulation_generation.py @@ -202,12 +202,6 @@ def test_filtering(client, models, root_circuit, simulation_result): ).json()["data"] assert len(data) == 2 - # backwards compat - data = assert_request( - client.get, url=ROUTE, params={"used__id__in": f"{root_circuit.id},{simulation_result.id}"} - ).json()["data"] - assert len(data) == 5 - data = assert_request( client.get, url=ROUTE, @@ -215,14 +209,6 @@ def test_filtering(client, models, root_circuit, simulation_result): ).json()["data"] assert len(data) == 5 - # backwards compat - data = assert_request( - client.get, - url=ROUTE, - params={"generated__id__in": f"{root_circuit.id},{simulation_result.id}"}, - ).json()["data"] - assert len(data) == 4 - data = assert_request( client.get, url=ROUTE, diff --git a/tests/test_validation.py b/tests/test_validation.py index 58661ce0f..57d9bed35 100644 --- a/tests/test_validation.py +++ b/tests/test_validation.py @@ -224,12 +224,6 @@ def test_filtering(client, models, root_circuit, simulation_result): assert len(data) == 2 """ - # backwards compat - data = assert_request( - client.get, url=ROUTE, params={"used__id__in": f"{root_circuit.id},{simulation_result.id}"} - ).json()["data"] - assert len(data) == 5 - data = assert_request( client.get, url=ROUTE, @@ -237,14 +231,6 @@ def test_filtering(client, models, root_circuit, simulation_result): ).json()["data"] assert len(data) == 5 - # backwards compat - data = assert_request( - client.get, - url=ROUTE, - params={"generated__id__in": f"{root_circuit.id},{simulation_result.id}"}, - ).json()["data"] - assert len(data) == 4 - data = assert_request( client.get, url=ROUTE, From 8d74c74feffecff631aebee0947bc86e58b075a2 Mon Sep 17 00:00:00 2001 From: Gianluca Ficarelli Date: Tue, 14 Oct 2025 17:41:25 +0200 Subject: [PATCH 2/2] Fix test_cell_morphology_ordering --- tests/test_ordering.py | 10 ++++++---- tests/utils.py | 17 +++++++++++++---- 2 files changed, 19 insertions(+), 8 deletions(-) diff --git a/tests/test_ordering.py b/tests/test_ordering.py index 3c8518c5f..0ff519a19 100644 --- a/tests/test_ordering.py +++ b/tests/test_ordering.py @@ -1,5 +1,3 @@ -import pytest - from app.db.model import CellMorphology, License from tests.utils import PROJECT_ID, add_all_db, check_sort_by_field @@ -72,6 +70,10 @@ def test_cell_morphology_ordering(db, client, subject_id, license_id, brain_regi assert response.status_code == 200 data = response.json()["data"] assert len(data) == count / 2 - with pytest.raises(AssertionError, match="Items unsorted by id"): - check_sort_by_field(data, "id") check_sort_by_field(data, "name") + + response = client.get(ROUTE_MORPHOLOGY, params={"name__ilike": "to_find", "order_by": "-name"}) + assert response.status_code == 200 + data = response.json()["data"] + assert len(data) == count / 2 + check_sort_by_field(data, "name", how="descending") diff --git a/tests/utils.py b/tests/utils.py index b724fa470..6e94a1b5e 100644 --- a/tests/utils.py +++ b/tests/utils.py @@ -1,4 +1,5 @@ import functools +import operator import uuid from collections.abc import Callable from datetime import UTC, datetime, timedelta @@ -791,10 +792,18 @@ def delete_entity_classifications(client, client_admin, entity_id): ) -def check_sort_by_field(items, field_name): - assert all(items[i][field_name] < items[i + 1][field_name] for i in range(len(items) - 1)), ( - f"Items unsorted by {field_name}" - ) +def check_sort_by_field(items, field_name, how="ascending"): + op = { + "ascending": operator.le, + "descending": operator.ge, + }[how] + assert all( + op( + items[i][field_name], + items[i + 1][field_name], + ) + for i in range(len(items) - 1) + ), f"Items not sorted {how} by {field_name}" def create_subject_ids(db, *, created_by_id, n):