Skip to content

Commit

Permalink
Fix 500 when trying to filter on an unknown subfield
Browse files Browse the repository at this point in the history
Also included all other cases in tests, this didn't have test coverage so far.
  • Loading branch information
vdboor committed Aug 1, 2024
1 parent d227569 commit 85ef6fa
Show file tree
Hide file tree
Showing 2 changed files with 73 additions and 13 deletions.
30 changes: 17 additions & 13 deletions src/dso_api/dynamic_api/filters/parser.py
Original file line number Diff line number Diff line change
Expand Up @@ -527,16 +527,20 @@ def _get_field_by_id( # noqa: C901
# Found regular field, or forward relation
return FilterPathPart(field=field, is_many=field.is_array)

try:
additional_relation = parent.get_additional_relation_by_id(name)
except SchemaObjectNotFound:
return None
else:
# The additional relation name is used as ORM path to navigate over the relation.
# Yet when directly filtering, the value/lookup should work directly
# against the primary key of the reverse table (hence field is also resolved here)
return FilterPathPart(
field=additional_relation.related_table.identifier_fields[0],
reverse_field=additional_relation,
is_many=True,
)
if isinstance(parent, DatasetTableSchema):
# For tables, see if a reverse relation can be traversed. This is not possible for fields.
try:
additional_relation = parent.get_additional_relation_by_id(name)
except SchemaObjectNotFound:
return None
else:
# The additional relation name is used as ORM path to navigate over the relation.
# Yet when directly filtering, the value/lookup should work directly
# against the primary key of the reverse table (hence field is also resolved here)
return FilterPathPart(
field=additional_relation.related_table.identifier_fields[0],
reverse_field=additional_relation,
is_many=True,
)

return None
56 changes: 56 additions & 0 deletions src/tests/test_dynamic_api/test_filters.py
Original file line number Diff line number Diff line change
Expand Up @@ -250,6 +250,24 @@ def test_filter_loose_relation(
result = engine.filter_queryset(statistieken_model.objects.all())
assert result.count() == expect, result

@staticmethod
@pytest.mark.parametrize(
"query",
[
"nonexistant",
"buurt.nonexistant=test",
"buurt.identificatie.nonexistant=test",
"buurt.naam.nonexistant=test",
],
)
def test_filter_loose_relation_invalid(
api_client, statistieken_model, statistieken_data, query
):
"""Prove that field-resolving on invalid fields is properly handled."""
engine = create_filter_engine(query)
with pytest.raises(ValidationError):
engine.filter_queryset(statistieken_model.objects.all())

@staticmethod
@pytest.mark.parametrize(
"query,expect",
Expand Down Expand Up @@ -366,6 +384,24 @@ def test_array_field(parkeervak, query, expect):
assert response.status_code == 200, data
assert len(data["_embedded"]["parkeervakken"]) == expect

@staticmethod
@pytest.mark.parametrize(
["query", "expect_code"],
[
("regimes.dagen=ma,di,wo,do,vr", 200),
("regimes.dagen.foo=test", 400),
("regimes.foo=test", 400), # subfield has different codepath if not found.
("foobar=test", 400),
],
)
def test_parsing_invalid(
parkeervakken_parkeervak_model, parkeervakken_regime_model, query, expect_code
):
"""Prove that looking for a relation on a nested field won't crash."""
response = APIClient().get(f"/v1/parkeervakken/parkeervakken/?{query}")
data = read_response_json(response)
assert response.status_code == expect_code, data

@staticmethod
def test_numerical_filters(parkeervakken_parkeervak_model, filled_router):
"""Test comparisons on numerical fields."""
Expand Down Expand Up @@ -575,6 +611,26 @@ def test_filter_reverse_m2m(movies_data_with_actors, filled_router):
names = [a["name"] for a in data["_embedded"]["actor"]]
assert names == ["John Doe", "Jane Doe"]

@staticmethod
@pytest.mark.parametrize(
"url",
[
"/v1/movies/movie/?nonexistent=foo123", # invalid normal field
"/v1/movies/movie/?name.nonexistent=foo123", # not a relation
"/v1/movies/category/?movies.nonexistent=foo123", # using reverse FK
"/v1/movies/category/?movies.name.nonexistent=foo123", # using reverse FK
"/v1/movies/actor/?movies.nonexistent=foo123", # using reverse M2M
"/v1/movies/actor/?movies.name.nonexistent=foo123", # using reverse M2M
],
)
def test_filter_invalid_field(movies_model, filled_router, url):
"""Prove that walking over M2M models works and doesn't crash the parser.
Note this uses the "movies" dataset, not the hardcoded movie models/serializers.
"""
response = APIClient().get(url)
data = read_response_json(response)
assert response.status_code == 400, data

@staticmethod
@pytest.mark.parametrize("params", [{"e_type": "whatever"}, {"_sort": "e_type"}])
def test_snake_case_400(params, parkeervakken_parkeervak_model):
Expand Down

0 comments on commit 85ef6fa

Please sign in to comment.