INTPYTHON-835 Add support for GIS lookups #448
Conversation
| "GIS Union not supported.": { | ||
| "gis_tests.geoapp.tests.GeoLookupTest.test_gis_lookups_with_complex_expressions", | ||
| }, |
There was a problem hiding this comment.
Note to self that we should add @skipUnlessDBFeature("supports_Union_function") and send the patch upstream. I'll hold off in case any more similar changes are identified by this PR.
There was a problem hiding this comment.
There are a couple other places where tests not marked as requiring a feature use it. Maybe the authors didn't expect a backend to implement only the slice of operations that we do?
There was a problem hiding this comment.
Correct. The skipping is generally just best effort... what's needed for the built-in backends.
|
All tests (including some simple ones I added due to many of the Django ones requiring features we don't support) passing, marking as ready for an initial review pass. |
timgraham
left a comment
timgraham
left a comment
There was a problem hiding this comment.
The documentation to update is docs/ref/contrib/gis.rst.
tests/gis_tests_/tests.py
Outdated
| with self.assertRaisesMessage(NotSupportedError, msg): | ||
| City.objects.get(point__same_as=Point(95, 30)) | ||
|
|
||
| def test_within_lookup(self): |
There was a problem hiding this comment.
Please alphabetize the new tests. The _lookup suffix can be omitted since the class is LookupTests.
| if isinstance(value, Distance): | ||
| if f.geodetic(self.connection): | ||
| raise ValueError( | ||
| "Only numeric values of degree units are allowed on geodetic distance queries." |
There was a problem hiding this comment.
Use self.assertRaisesMessage() to test for this.
| def gis_lookup(self, compiler, connection, as_expr=False): # noqa: ARG001 | ||
| raise NotSupportedError(f"MongoDB does not support the {self.lookup_name} lookup.") | ||
|
|
||
| def _gis_lookup(self, compiler, connection, as_expr=False): |
There was a problem hiding this comment.
Adding an underscore prefix is not needed.
tests/gis_tests_/tests.py
Outdated
| def test_within_lookup(self): | ||
| city = City.objects.create(point=Point(95, 30)) | ||
| qs = City.objects.filter(point__within=Point(95, 30).buffer(10)) | ||
| self.assertIn(city, qs) |
There was a problem hiding this comment.
It would be nice to have more than one test geometry to show that the operator does some filtering. assertIn() isn't the best assertion since the queryset could inadvertently return all objects without any filtering.
timgraham
changed the title
timgraham
left a comment
timgraham
left a comment
There was a problem hiding this comment.
Did you forget the documentation updates? :-)
| name = models.CharField(max_length=30) | ||
| point = models.PointField() |
There was a problem hiding this comment.
add def __str__(): return self.name to make debugging failures easier
| self.assertEqual(qs.count(), 1) | ||
| self.assertEqual(houston, qs.first()) |
There was a problem hiding this comment.
prefer self.assertCountEqual(qs, [houston])
QuerySet.count() adds an extra query and makes a failure more difficult to debug since the first thing you need to do is run the test to find which extra objects appeared.
Also, in Django tests, the expected values usually appear as the second argument.
| with self.assertRaisesMessage( | ||
| ValueError, | ||
| "Only numeric values of radian units are allowed on geodetic distance queries.", | ||
| ): | ||
| qs.first() |
There was a problem hiding this comment.
I recommend this style to avoid the awkward multi-line version.
msg = "..."
with self.assertRaisesMessage(ValueError, msg):
| def test_within(self): | ||
| zipcode = Zipcode.objects.get(code="77002") | ||
| qs = City.objects.filter(point__within=zipcode.poly).values_list("name", flat=True) | ||
| self.assertEqual(["Houston"], list(qs)) |
There was a problem hiding this comment.
values_list() doesn't need a list(qs)
|
It seems to me that diff --git a/django_mongodb_backend/gis/operations.py b/django_mongodb_backend/gis/operations.py
index e271c8f..8022824 100644
--- a/django_mongodb_backend/gis/operations.py
+++ b/django_mongodb_backend/gis/operations.py
@@ -9,17 +9,27 @@ from .adapter import Adapter
from .utils import SpatialOperator
-def _gis_within_operator(field, value, op=None, params=None): # noqa: ARG001
- return {
- field: {
- "$geoWithin": {
- "$geometry": {
- "type": value["type"],
- "coordinates": value["coordinates"],
+class Operator:
+ def as_sql(self, connection, lookup, template_params, sql_params):
+ # Return some dummy value to prevent str(queryset.query) from crashing.
+ # The output of as_sql() is meaningless for this no-SQL backend.
+ return self.name
+
+
+class Within(Operator):
+ name = "within"
+
+ def as_mql(field, value, params):
+ return {
+ field: {
+ "$geoWithin": {
+ "$geometry": {
+ "type": value["type"],
+ "coordinates": value["coordinates"],
+ }
}
}
}
- }
def _gis_intersects_operator(field, value, op=None, params=None): # noqa: ARG001
@@ -116,7 +126,7 @@ class GISOperations(BaseSpatialOperations, BaseDatabaseOperations):
"contains": SpatialOperator("contains", _gis_contains_operator),
"intersects": SpatialOperator("intersects", _gis_intersects_operator),
"disjoint": SpatialOperator("disjoint", _gis_disjoint_operator),
- "within": SpatialOperator("within", _gis_within_operator),
+ "within": Within,
"distance_gt": SpatialOperator("distance_gt", _gis_distance_operator),
"distance_gte": SpatialOperator("distance_gte", _gis_distance_operator),
"distance_lt": SpatialOperator("distance_lt", _gis_distance_operator), |
2 participants
No description provided.