[filters] Handle invalid geometry input safely in GeometryFilter #187 #344
Conversation
…p#187 Implemented safer handling of invalid geometry input by: - validating geometry in a custom SafeGeometryField - catching GEOSException, GDALException, ValueError and TypeError in GeometryFilter.filter This prevents server error like: "Invalid geometry pointer returned from OGR_G_CreateGeometryFromJson" and ensures a clean 400 ValidationError response instead. Fixes openwisp#187
There was a problem hiding this comment.
Pull request overview
This PR addresses issue #187 by adding safe error handling for invalid geometry input in the GeometryFilter. The changes prevent unhandled GEOS/GDAL exceptions from causing 500 errors and instead return proper 400 ValidationError responses to API clients.
Key changes:
- Introduces a
SafeGeometryFieldthat wraps Django's GeometryField to catch and convert GEOS/GDAL parsing exceptions into validation errors - Adds exception handling in
GeometryFilter.filter()to catch errors during queryset spatial lookups - Imports GDALException and GEOSException to handle geometry parsing failures gracefully
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # backward compatibility | ||
| InBBOXFilter = InBBoxFilter | ||
|
|
||
| class SafeGeometryField(forms.GeometryField): |
There was a problem hiding this comment.
Missing blank lines before class definition. According to PEP 8, there should be two blank lines before top-level class definitions. There should be two blank lines before this class definition.
rest_framework_gis/filters.py
Outdated
| raise forms.ValidationError("Invalid geometry value.") | ||
|
|
||
| class GeometryFilter(django_filters.Filter): | ||
| field_class = forms.GeometryField | ||
| field_class = SafeGeometryField | ||
|
|
||
| def __init__(self, *args, **kwargs): | ||
| kwargs.setdefault("widget", forms.TextInput) | ||
| super().__init__(*args, **kwargs) | ||
|
|
||
|
|
||
|
|
||
| def filter(self, qs, value): | ||
| try: | ||
| return super().filter(qs, value) | ||
| except (GEOSException, GDALException, ValueError, TypeError) as e: | ||
| raise ValidationError(f"Invalid geometry value: {str(e)}") |
There was a problem hiding this comment.
Inconsistent error messages between SafeGeometryField and GeometryFilter. SafeGeometryField raises "Invalid geometry value." (with period) while GeometryFilter raises "Invalid geometry value: {str(e)}" (with colon and exception details). Consider using consistent error messages. Additionally, the SafeGeometryField error message has a period at the end while GeometryFilter does not.
Suggestion: Use the same base message format in both places, e.g., "Invalid geometry value" without the period, or include the exception details in SafeGeometryField as well for consistency.
rest_framework_gis/filters.py
Outdated
| super().__init__(*args, **kwargs) | ||
|
|
||
|
|
||
|
|
There was a problem hiding this comment.
Trailing whitespace on line 113. There is an extra space after the code on this line. This should be removed to maintain clean code formatting.
rest_framework_gis/filters.py
Outdated
| return super().filter(qs, value) | ||
| except (GEOSException, GDALException, ValueError, TypeError) as e: | ||
| raise ValidationError(f"Invalid geometry value: {str(e)}") | ||
|
|
There was a problem hiding this comment.
Trailing whitespace on line 119. There are two extra spaces after the code on this line. This should be removed to maintain clean code formatting.
rest_framework_gis/filters.py
Outdated
| except (GEOSException, GDALException, ValueError, TypeError) as e: | ||
| raise ValidationError(f"Invalid geometry value: {str(e)}") |
There was a problem hiding this comment.
[nitpick] The exception variable e is captured but may expose internal implementation details. While this provides helpful debugging information, consider whether exposing raw GEOS/GDAL exception messages in the API response could leak sensitive information about the backend implementation. If this is acceptable, the current implementation is fine. Otherwise, consider logging the full exception details and returning only a generic message to the user.
rest_framework_gis/filters.py
Outdated
| def to_python(self, value): | ||
| try: | ||
| return super().to_python(value) | ||
| except (GEOSException, GDALException, ValueError, TypeError): | ||
| raise forms.ValidationError("Invalid geometry value.") | ||
|
|
||
| class GeometryFilter(django_filters.Filter): | ||
| field_class = forms.GeometryField | ||
| field_class = SafeGeometryField | ||
|
|
||
| def __init__(self, *args, **kwargs): | ||
| kwargs.setdefault("widget", forms.TextInput) | ||
| super().__init__(*args, **kwargs) | ||
|
|
||
|
|
||
|
|
||
| def filter(self, qs, value): | ||
| try: | ||
| return super().filter(qs, value) | ||
| except (GEOSException, GDALException, ValueError, TypeError) as e: | ||
| raise ValidationError(f"Invalid geometry value: {str(e)}") |
There was a problem hiding this comment.
The new error handling in SafeGeometryField and GeometryFilter.filter() lacks test coverage. Given that tests/django_restframework_gis_tests/test_filters.py has test cases for invalid input handling (e.g., test_inBBOXFilter_ValueError, test_TileFilter_ValueError), similar tests should be added for GeometryFilter to verify that:
- Invalid GeoJSON strings return a 400 ValidationError (not 500)
- Invalid WKT strings return a 400 ValidationError
- The error message is appropriate and user-friendly
This is especially important since this PR specifically addresses issue #187 about handling invalid geometry input safely.
rest_framework_gis/filters.py
Outdated
| def to_python(self, value): | ||
| try: | ||
| return super().to_python(value) | ||
| except (GEOSException, GDALException, ValueError, TypeError): | ||
| raise forms.ValidationError("Invalid geometry value.") | ||
|
|
||
| class GeometryFilter(django_filters.Filter): | ||
| field_class = forms.GeometryField | ||
| field_class = SafeGeometryField | ||
|
|
||
| def __init__(self, *args, **kwargs): | ||
| kwargs.setdefault("widget", forms.TextInput) | ||
| super().__init__(*args, **kwargs) | ||
|
|
||
|
|
||
|
|
||
| def filter(self, qs, value): | ||
| try: | ||
| return super().filter(qs, value) | ||
| except (GEOSException, GDALException, ValueError, TypeError) as e: | ||
| raise ValidationError(f"Invalid geometry value: {str(e)}") |
There was a problem hiding this comment.
Potential redundancy in exception handling. Both SafeGeometryField.to_python() and GeometryFilter.filter() catch the same exceptions (GEOSException, GDALException, ValueError, TypeError).
If SafeGeometryField successfully converts these exceptions to forms.ValidationError in to_python(), then the exceptions in filter() may never be raised. Consider:
- Whether both layers of exception handling are necessary
- If both are needed, document why (e.g., different failure points in the processing pipeline)
- If the filter() exception handling is primarily catching exceptions from the actual queryset filtering operations (not from field validation), this should be clarified in a comment
This would help future maintainers understand the error handling flow.
-Added required blank lines before class definition -Unified error messages between SafeGeometryField and GeometryFilter -Removed trailing whitespaces Fixes openwisp#187
1 participant
Checklist
This pull request addresses issue #187 by ensuring that invalid geometry
input no longer raises unhandled GEOS/GDAL exceptions and instead returns
a clean 400 ValidationError response.
What was happening
When an invalid GeoJSON/WKT string was passed to a geometry filter,
Django’s internal spatial routines (
OGR_G_CreateGeometryFromJson) couldraise
GDALExceptionorGEOSException, resulting in a 500 error insteadof a proper validation error response.
What this PR changes
SafeGeometryFieldthat converts GDAL/GEOS parsing errorsinto
ValidationError.GeometryFilter.filter()in a try/except block to catch anyexceptions raised during queryset spatial lookups.
"Invalid geometry value".Fixes #187