Feature: Add geometry filters #710
Conversation
Reviewer's Guide by SourceryThis pull request introduces the Class diagram for GeometryFilterLookupclassDiagram
class GeometryFilterLookup {
bbcontains: Geometry
bboverlaps: Geometry
contained: Geometry
contains: Geometry
contains_properly: Geometry
coveredby: Geometry
covers: Geometry
crosses: Geometry
disjoint: Geometry
equals: Geometry
exacts: Geometry
intersects: Geometry
isempty: bool
isvalid: bool
overlaps: Geometry
touches: Geometry
within: Geometry
left: Geometry
right: Geometry
overlaps_left: Geometry
overlaps_right: Geometry
overlaps_above: Geometry
overlaps_below: Geometry
strictly_above: Geometry
strictly_below: Geometry
}
note for GeometryFilterLookup "This class is only available when GeoDjango is installed"
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
for more information, see https://pre-commit.ci
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @shmoon-kr - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding a test case to ensure that the GeometryFilterLookup works as expected.
- It might be worth extracting the GeometryFilterLookup try/except block into a separate function or module to improve readability.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
strawberry_django/fields/types.py
Outdated
| @@ -557,9 +558,11 @@ | |||
| ): | |||
| if using_old_filters: | |||
| field_type = filters.FilterLookup[field_type] | |||
| elif type(field_type) is ScalarWrapper and field_type._scalar_definition.name in ("Point", "LineString", "LinearRing", "Polygon", "MultiPoint", "MultilineString", "MultiPolygon", "Geometry"): | |||
There was a problem hiding this comment.
suggestion: Consider using isinstance for type checking of ScalarWrapper.
Using 'type(field_type) is ScalarWrapper' might be too strict if subclasses of ScalarWrapper should also match. Switching to isinstance(field_type, ScalarWrapper) could provide a more flexible and robust check.
| elif type(field_type) is ScalarWrapper and field_type._scalar_definition.name in ("Point", "LineString", "LinearRing", "Polygon", "MultiPoint", "MultilineString", "MultiPolygon", "Geometry"): | |
| elif isinstance(field_type, ScalarWrapper) and field_type._scalar_definition.name in ("Point", "LineString", "LinearRing", "Polygon", "MultiPoint", "MultilineString", "MultiPolygon", "Geometry"): |
| pass | ||
| else: | ||
| @strawberry.input | ||
| class GeometryFilterLookup(Generic[T]): |
There was a problem hiding this comment.
issue (complexity): Consider dynamically generating the geometry fields to reduce code duplication.
Consider reducing the repetitive field definitions by generating the geometry fields dynamically. For example, you can define a list of method names and then use a helper to build your fields. This will reduce the nesting and repetitive code without changing functionality. For instance:
try:
from django.contrib.gis.geos import GEOSGeometry
except ImproperlyConfigured:
# If gdal is not available, skip.
pass
else:
GEO_FIELD_TYPE = Optional[Annotated["Geometry", strawberry.lazy(".types")]]
geometry_methods = [
"bbcontains", "bboverlaps", "contained", "contains", "contains_properly",
"coveredby", "covers", "crosses", "disjoint", "equals", "exacts",
"intersects", "overlaps", "touches", "within", "left", "right",
"overlaps_left", "overlaps_right", "overlaps_above", "overlaps_below",
"strictly_above", "strictly_below",
]
def _build_geometry_fields():
fields = {name: UNSET for name in geometry_methods}
fields.update({
"isempty": filter_field(description=f"Test whether it's empty. {_SKIP_MSG}"),
"isvalid": filter_field(description=f"Test whether it's valid. {_SKIP_MSG}"),
})
return fields
GeometryFields = _build_geometry_fields()
GeometryFilterLookup = strawberry.input(
type("GeometryFilterLookup", (Generic[T],), GeometryFields)
)This refactoring maintains all functionality while reducing the duplication and nesting in your class definition.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #710 +/- ##
==========================================
+ Coverage 88.29% 88.32% +0.03%
==========================================
Files 42 42
Lines 3920 3956 +36
==========================================
+ Hits 3461 3494 +33
- Misses 459 462 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
I didn't even think about this possibility - great work @shmoon-kr |
strawberry_django/__init__.py
Outdated
| from django.core.exceptions import ImproperlyConfigured | ||
|
|
||
| try: | ||
| from .fields.filter_types import GeometryFilterLookup | ||
|
|
||
| __all__.append("GeometryFilterLookup") | ||
| except ImproperlyConfigured: | ||
| # If gdal is not available, skip. | ||
| pass |
There was a problem hiding this comment.
suggestion: Maybe we should always expose this in __all__, but make it None inside filter_typers for import errors
|
|
||
|
|
||
| try: | ||
| pass |
There was a problem hiding this comment.
todo: hrm, I think this is missing something =P
There was a problem hiding this comment.
It seems that it was removed by auto fixes from pre-commit.com hooks. Originally there was a code to test whether geodjango is available. from django.contrib.gis.geos import GEOSGeometry And I think that it's a better idea to define a configuration variable that indicates geodjango is available or not. But I don't know where it should be located.
There was a problem hiding this comment.
try:
from django.contrib.gis.db import models as geos_fields
GEOS_IMPORTED = True
class GeosFieldsModel(models.Model):
point = geos_fields.PointField(null=True, blank=True)
line_string = geos_fields.LineStringField(null=True, blank=True)
polygon = geos_fields.PolygonField(null=True, blank=True)
multi_point = geos_fields.MultiPointField(null=True, blank=True)
multi_line_string = geos_fields.MultiLineStringField(null=True, blank=True)
multi_polygon = geos_fields.MultiPolygonField(null=True, blank=True)
geometry = geos_fields.GeometryField(null=True, blank=True)
except ImproperlyConfigured:
GEOS_IMPORTED = False
I found this in tests/models.py file. But I think it should be located in somewhere else.
| @@ -557,6 +558,19 @@ def resolve_model_field_type( | |||
| ): | |||
| if using_old_filters: | |||
| field_type = filters.FilterLookup[field_type] | |||
| elif type( | |||
| field_type | |||
| ) is ScalarWrapper and field_type._scalar_definition.name in ( | |||
There was a problem hiding this comment.
suggestion: might need a # type: ignore here
|
Hi. I found the following error when I was trying to run a test code. File "/Users/evan/PycharmProjects/strawberry-django/tests/polymorphism_inheritancemanager/models.py", line 2, in And one more question: I tried to write a test code which is run only if GeoDjango is available. But in a testing environment, GeoDjango seems not available so I cannot test geometry operations. Any suggestions? |
That's a new dev dependency that got merged from a PR yesterday. Running
You just need to make sure that the geospatial libraries are available in your system. This should help: https://docs.djangoproject.com/en/5.1/ref/contrib/gis/install/geolibs/ From your reply, I can see that you are on MacOS, which is slightly more complicated than linux (e.g. on debian/ubuntu you just need to |
|
@bellini666 I revised the PR as you instructed. I have one more question. Can I test whether GeoDjango is available or not with the following code from now? Then the code may get simpler. |
yep, I think it is completely fine :) |
Description
Added GeometryLookupFilter type so that spatial filters available in GeoDjango can be used in strawberry_django.
Types of Changes
Issues Fixed or Closed by This PR
Checklist
Summary by Sourcery
Adds support for spatial filters from GeoDjango within strawberry-django. It introduces a new GeometryFilterLookup type and integrates it into the existing filter system, allowing users to filter data based on geographical relationships.
New Features:
Enhancements: