Skip to content

[fix] Attribute error when trying to get bounding box from a method field value #246

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

[fix] Attribute error when trying to get bounding box from a method field value #246

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
58ec8d0
[fix] attribute error when trying to get bounding box from a method f…
hemache Nov 5, 2020
1d57899
[qa] Test bounding box calculation when using method field for geo field
hemache Nov 5, 2020
191209b
[qa] Format code
hemache Nov 5, 2020
0040739
[qa] Change PostgreSQL database port back to default value & Allow pe…
hemache Nov 11, 2020
84cd642
[qa] Rollback CI PostgreSQL default port fix
hemache Dec 2, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 12 additions & 2 deletions rest_framework_gis/serializers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
LIST_SERIALIZER_KWARGS,
ListSerializer,
ModelSerializer,
SerializerMethodField,
)

from .fields import GeometryField, GeometrySerializerMethodField # noqa
Expand Down Expand Up @@ -132,7 +133,12 @@ def to_representation(self, instance):
# MUST be present in output according to GeoJSON spec
field = self.fields[self.Meta.geo_field]
geo_value = field.get_attribute(instance)
feature["geometry"] = field.to_representation(geo_value)
if isinstance(field, SerializerMethodField):
method = getattr(field.parent, field.method_name)
geo_value = method(instance)
feature["geometry"] = field.to_representation(instance)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is using instance instead of geo_value intended?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is using instance instead of geo_value intended?

yes, because in this case, SerializerMethodField.to_representation receives object instance instead of the actual geometry value.

the problem was that geo_value initially stores the object instance (which doesn't provide .extent attribute), so here we're making sure to actually get the geometry value instead of the object instance.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, I don't understand then what's the purpose of calling geo_value = method(instance), it seems like that line can be removed, since geo_value is not used afterwards. Isn't it?
But I am also not entirely convinced of the approach as explained above.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since geo_value is not used afterwards. Isn't it?

yes, it's not used afterwards, but what if -later- someone else needed to use it?
I believe geo_value should have the correct/actual GEO value, not an instance of the object.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand this. I don't want to introduce changes in a library that is used by thousands of developers lightly. Each change must be justified.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nemesisdesign it's justified. did you try to reproduce this bug?

focus on the type of geo_value variable. it should hold a GEO value and not something else, right?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nemesisdesign if you're not convinced by my solution, then please try to reproduce the bug #247 and maybe suggest a proper fix?

else:
feature["geometry"] = field.to_representation(geo_value)
processed_fields.add(self.Meta.geo_field)

# Bounding Box
Expand All @@ -143,7 +149,11 @@ def to_representation(self, instance):
# otherwise it can be determined via another field
elif self.Meta.bbox_geo_field:
field = self.fields[self.Meta.bbox_geo_field]
value = field.get_attribute(instance)
if isinstance(field, SerializerMethodField):
method = getattr(field.parent, field.method_name)
value = method(instance)
else:
value = field.get_attribute(instance)
feature["bbox"] = value.extent if hasattr(value, 'extent') else None
processed_fields.add(self.Meta.bbox_geo_field)

Expand Down
30 changes: 30 additions & 0 deletions tests/django_restframework_gis_tests/serializers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,36 @@ class Meta:
exclude = []


class LocationGeoFeatureMethodAutoBboxSerializer(
gis_serializers.GeoFeatureModelSerializer
):
new_geometry = gis_serializers.GeometrySerializerMethodField()

class Meta:
model = Location
geo_field = 'new_geometry'
auto_bbox = True
exclude = []

def get_new_geometry(self, obj):
return obj.geometry


class LocationGeoFeatureMethodManualBboxSerializer(
gis_serializers.GeoFeatureModelSerializer
):
new_geometry = gis_serializers.GeometrySerializerMethodField()

class Meta:
model = Location
geo_field = 'new_geometry'
bbox_geo_field = 'new_geometry'
exclude = []

def get_new_geometry(self, obj):
return obj.geometry


class NoneGeoFeatureMethodSerializer(gis_serializers.GeoFeatureModelSerializer):
new_geometry = gis_serializers.GeometrySerializerMethodField()

Expand Down
24 changes: 24 additions & 0 deletions tests/django_restframework_gis_tests/test_bbox.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,12 @@ class TestRestFrameworkGisBBox(TestCase):
def setUp(self):
self.geojson_boxedlocation_list_url = reverse('api_geojson_boxedlocation_list')
self.geojson_location_bbox_list_url = reverse('api_geojson_location_bbox_list')
self.geojson_location_method_field_auto_bbox_list_url = reverse(
'api_geojson_location_method_field_auto_bbox_list'
)
self.geojson_location_method_field_manual_bbox_list_url = reverse(
'api_geojson_location_method_field_manual_bbox_list'
)

def _create_locations(self):
self.bl1 = BoxedLocation.objects.create(
Expand Down Expand Up @@ -87,6 +93,24 @@ def test_get_autogenerated_location_bbox_geojson(self):
self.assertEqual(len(response.data['features']), 1)
self.assertEqual(response.data['features'][0]['bbox'], self.l1.geometry.extent)

def test_get_autogenerated_location_auto_bbox_geojson_with_method_field(self):
self._create_locations()
response = self.client.get(
self.geojson_location_method_field_auto_bbox_list_url
)
self.assertEqual(response.status_code, 200)
self.assertEqual(len(response.data['features']), 1)
self.assertEqual(response.data['features'][0]['bbox'], self.l1.geometry.extent)

def test_get_autogenerated_location_manual_bbox_geojson_with_method_field(self):
self._create_locations()
response = self.client.get(
self.geojson_location_method_field_manual_bbox_list_url
)
self.assertEqual(response.status_code, 200)
self.assertEqual(len(response.data['features']), 1)
self.assertEqual(response.data['features'][0]['bbox'], self.l1.geometry.extent)

def test_bbox_improperly_configured(self):
self._create_locations()

Expand Down
10 changes: 10 additions & 0 deletions tests/django_restframework_gis_tests/urls.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,16 @@
views.geojson_location_bbox_list,
name='api_geojson_location_bbox_list',
),
url(
r'^geojson-location-method-field-auto-bbox/$',
views.geojson_location_method_field_auto_bbox_list,
name='api_geojson_location_method_field_auto_bbox_list',
),
url(
r'^geojson-location-method-field-manual-bbox/$',
views.geojson_location_method_field_manual_bbox_list,
name='api_geojson_location_method_field_manual_bbox_list',
),
# Filters
url(
r'^filters/contained_in_bbox$',
Expand Down
24 changes: 24 additions & 0 deletions tests/django_restframework_gis_tests/views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@
LocatedFileGeoFeatureSerializer,
LocationGeoFeatureBboxSerializer,
LocationGeoFeatureFalseIdSerializer,
LocationGeoFeatureMethodAutoBboxSerializer,
LocationGeoFeatureMethodManualBboxSerializer,
LocationGeoFeatureMethodSerializer,
LocationGeoFeatureNoIdSerializer,
LocationGeoFeatureSerializer,
Expand Down Expand Up @@ -240,6 +242,28 @@ class GeojsonLocationBboxList(generics.ListCreateAPIView):
geojson_location_bbox_list = GeojsonLocationBboxList.as_view()


class GeojsonLocationMethodFieldAutoBboxList(generics.ListCreateAPIView):
model = Location
serializer_class = LocationGeoFeatureMethodAutoBboxSerializer
queryset = Location.objects.all()


geojson_location_method_field_auto_bbox_list = (
GeojsonLocationMethodFieldAutoBboxList.as_view()
)


class GeojsonLocationMethodFieldManualBboxList(generics.ListCreateAPIView):
model = Location
serializer_class = LocationGeoFeatureMethodManualBboxSerializer
queryset = Location.objects.all()


geojson_location_method_field_manual_bbox_list = (
GeojsonLocationMethodFieldManualBboxList.as_view()
)


class GeojsonNullableDetails(generics.RetrieveUpdateDestroyAPIView):
model = Location
serializer_class = LocationGeoFeatureSerializer
Expand Down