openwisp / django-rest-framework-gis

Geographic add-ons for Django REST Framework. Maintained by the OpenWISP Project.
http://openwisp.org
MIT License
1.07k stars 200 forks source link

Output properly formatted GeoJson #2

Closed dmeehan closed 10 years ago

dmeehan commented 11 years ago

We need to properly format the outputted GeoJson so that it can be consumed by client-side mapping software (Leaflet, OpenLayers, etc)

So, for example, lets look at the following Location model:

class Location(models.Model):
    """
    A model which holds information about a particular location
   """
   address = models.Charfield(max_length=255)
   city = models.CharField(max_length=100)
   state = models.CharField(max_length=100)
   point = models.PointField()

By default, DRF will format a json response as (for example):

{
     "id": 1, 
     "address": "742 Evergreen Terrace", 
      "city":  "Springfield", 
      "state": "Oregon",
      "point": "POINT(-123.0208 44.0464)" 
}

With a new GeoDjango enabled GeometryField as an add on, it will output "point" as a GeoJson:

{
     "id": 1, 
     "address": "742 Evergreen Terrace", 
      "city":  "Springfield", 
      "state": "Oregon",
      "point": {
            "type": "Point",
            "coordinates": [-123.0208, 44.0464],
       },
}

But, we now need to format the entire json response as GeoJson, for example:

{ 
    "type": "Feature",
     "geometry": {
           "type": "Point",
           "coordinates": [-123.0208, 44.0464],
     },
     "properties": {
         "id": 1, 
         "address": "742 Evergreen Terrace", 
         "city":  "Springfield", 
         "state": "Oregon",
     }
}

Need to investigate the best place in DRF to override for this behavior.

For reference: http://geojson.org/geojson-spec.html

tomchristie commented 11 years ago

You've got two options for anything like this:

  1. Deal with it at the serialization layer.
  2. Deal with it at the parser/renderer layer.

If you go with (1) you'd want to create a custom serializer that does some wrapping around to_native/from_native to get the output into the right structure. Also you'd wan to doc that it expects models with a single geo type. If you go with (2) you'll want to document the constraints that your renderer has (eg. Must be dict input, and one element must be a geo style type, other keys will be treated as properties.)

I'm not totally sure what'd work best, I guess (2) seems sensible enough?

dmeehan commented 11 years ago

Thanks for the insight. I will start looking into an implementation for (2). I'd like to add the option for the user to specify which field is the geo field. I'm thinking of adding logic that first looks for an explicitly specified geo field, and if it does not find that, uses the first geo field it comes across as the geometry and treats additional geofields as a property.

dmeehan commented 11 years ago

Would it make sense to implement both a serializer and a renderer/parser? Are there cases where each would be more useful?

tomchristie commented 11 years ago

On second thoughts (1) seems fairly sensible. At least at that level you can check for GeoFields, rather than having to rely on a particular input structure that "looks" like the right thing. I could go either way though I guess. I guess you could take the approach of pulling together a bit of skeleton docs for each and see what you think looks nicer to the end-user.

Would it make sense to implement both a serializer and a renderer/parser? Are there cases where each would be more useful?

Not especially. If you implement it in the renderer, and include both a json renderer and a geojson renderer on a particular view, then a client could potentially switch to whichever style it wants, but I'm not sure that'd be very helpful (if anything probably just confusing.)

dmeehan commented 11 years ago

In thinking about it last night, I also came to the same conclusion that (1) is probably a better bet. I think the only advantage to (2) would be that the user could trigger the output by specifying a media type ("media_type = 'application/geo-json')

tomchristie commented 11 years ago

That'd let you have quite a nice API for your end users, something like.

from rest_framework_gis import serializers

class LocationSerializer(serializers.GeoFeatureModelSerializer):
    class Meta:
        geo_field = 'point'
        fields = ('id', 'address', 'city', 'state')

Also it could nicely handle outputting FeatureCollection in the lists/queryset case.

Perhaps, optional crs_field and bbox_field? (Not sure I get geojson enough to comment on that)

I think it'd make sense for you to force the geo_field to be explicit.

dmeehan commented 11 years ago

This is actually very similar to how Vectorformats does it, which is what I am currently using in my project.

http://packages.python.org/vectorformats/

dmeehan commented 11 years ago

In development:

A GeoFeatureModelSerializer which with these conventions:

class LocationSerializer(serializers.GeoFeatureModelSerializer):
     class Meta:
          geo_field = 'point'
          fields = ('id', 'address', 'city', 'state')
dmeehan commented 11 years ago

Implemented GeoFeatureModelSerializer.

At the moment, if you explicitly declare fields, you must also include the geo_field in your list or tuple, or it won't be included in the serialization. For example:

class LocationSerializer(serializers.GeoFeatureModelSerializer):
    class Meta:
        geo_field = 'point'
        fields = ('id', 'address', 'city', 'state', 'point')

I've tried a couple of solutions to get around this, but none that are ideal. The easiest solution would be to append the geo_field to the "fields" variable, but this won't work if fields is a tuple, and I'd like to keep this behavior equivalent to stock DRF.

Another option is to override the BaseSerializer.get_fields(), but this seems like overkill.

Any other suggestions would be very welcome.

tomchristie commented 11 years ago

@dmeehan - Can you not just do self.opts.fields = self.opts.fields + (self.opts.geo_field, ) at the end of __init__. Sure it's a tuple, but you can still reassign to it.

dmeehan commented 11 years ago

Makes sense. I will try that out. Thanks!

dmeehan commented 11 years ago

Hmm, I tried this out and I'm getting some strange behavior.

I inserted this code in the initmethod of the GeoFeatureModelSerializer:

if self.opts.geo_field not in self.opts.fields:
   self.opts.fields = self.opts.fields + (self.opts.geo_field, )

And created this Serializer class:

class LocationSerializer(serializers.GeoFeatureModelSerializer):
    class Meta:
        model = Location
        geo_field = 'point'
        fields = ('city', 'state')

Then created this instance:

>>> obj = Location.objects.get(pk=1)
>>> s = LocationSerializer(obj)

This gives me the following results:

>>> s.opts.geo_field
'point'
>>> s.opts.fields
('city', 'state', 'point')
>>> s.get_fields()
{'city': <rest_framework.fields.CharField object at 0x33ff410>, 'state': <rest_framework.fields.CharField object at   0x33ff550>, 'point': <drfgis.rest_framework_gis.fields.GeometryField object at 0x33ff610>}
>>> s.fields
{'city': <rest_framework.fields.CharField object at 0x33f4750>, 'state': <rest_framework.fields.CharField object at     0x33f4710>}
>>> s.data
{'type': 'Feature', 'properties': {'city': u'Springfield', 'state': u'Oregon'}, 'geometry': {}}

If i don't declare a 'fields' option, or include 'point' in the 'fields' option, then it works as expected.

This is the full init method:

def __init__(self, *args, **kwargs):
    super(GeoFeatureModelSerializer, self).__init__(*args, **kwargs)
    if self.opts.geo_field is None:
        raise ImproperlyConfigured("You must define a 'geo_field'.")
    else:
        # TODO: make sure the geo_field is a GeoDjango geometry field
        # make sure geo_field is included in fields
        if self.opts.geo_field in self.opts.exclude:
            raise ImproperlyConfigured("You cannot exclude your 'geo_field'.")
        if self.opts.geo_field not in self.opts.fields:
            self.opts.fields = self.opts.fields + (self.opts.geo_field, )
dmeehan commented 11 years ago

Figured out the issue.

self.fields = self.get_fields() was being assigned in the Parent init before opts.fields was updated. I just assigned this again at the end of init and now it works:

def __init__(self, *args, **kwargs):
    super(GeoFeatureModelSerializer, self).__init__(*args, **kwargs)
    if self.opts.geo_field is None:
        raise ImproperlyConfigured("You must define a 'geo_field'.")
    else:
        # TODO: make sure the geo_field is a GeoDjango geometry field
        # make sure geo_field is included in fields
        if self.opts.geo_field in self.opts.exclude:
            raise ImproperlyConfigured("You cannot exclude your 'geo_field'.")
        if self.opts.geo_field not in self.opts.fields:
            self.opts.fields = self.opts.fields + (self.opts.geo_field, )
            self.fields = self.get_fields()
dmeehan commented 10 years ago

to_native() method now working properly. need to implement a working from_native() method that strips the GeoJSON formatting before passing the data to the parent method.

nemesifier commented 10 years ago

what do you mean for "working from_native() method"?

I would expect it:

But this is not yet detailed enough. Pheraphs we could lie down some more details and write some tests!

What would be the best way to write tests for this module? Is a test app needed?

dmeehan commented 10 years ago

what do you mean for "working from_native() method"?

GeoFeatureModelSerializer inherits from DRF ModelSerializer, as such it inherits the "from_native()" method.

ModelSerializer.from_native() looks like this:

def from_native(self, data, files):
    """
    Override the default method to also include model field validation.
    """
    instance = super(ModelSerializer, self).from_native(data, files)
    if not self._errors:
        return self.full_clean(instance)

It inherits from BaseSerializer.from_native():

def from_native(self, data, files):
    """
    Deserialize primitives -> objects.
    """
    self._errors = {}
    if data is not None or files is not None:
        attrs = self.restore_fields(data, files)
        if attrs is not None:
            attrs = self.perform_validation(attrs)
    else:
        self._errors['non_field_errors'] = ['No input provided']

    if not self._errors:
        return self.restore_object(attrs, instance=getattr(self, 'object', None))

I was assuming we would need to override this method in our GeoFeatureModelSerializer to handle our GeoJSON formatting. But looking at it now, perhaps this is wrong? Try removing GeoFeatureModelSerializer.from_native() altogther and see if this works.

What would be the best way to write tests for this module? Is a test app needed?

It might be useful to include an example app for testing. I'm rather inexperienced when it comes to writing tests, so I'm open to suggestions. We should probably follow the DRF conventions as much as possible.

nemesifier commented 10 years ago

i'm trying to write a test app in my free time, started yesterday evening, resuming now.. will keep you updated

nemesifier commented 10 years ago

I tried removing the from_native and the normal behaviour works nicely.

I'll investigate further and hopefully we'll soon get a method that works both with the normal DRF behaviour and the geojson format.

dmeehan commented 10 years ago

Great. Do you want to make a pull request with that method removed?

nemesifier commented 10 years ago

Got a working from_native method (with tests) here: https://github.com/nemesisdesign/django-rest-framework-gis/commit/1a4e83990f9d8cf5b2fca286ef12afa1fb427a66

It works both with GeoJSON format and with normal requests (eg: from the web form).

Try it when you have time and let me know what you think..! Fed

dmeehan commented 10 years ago

Awesome! I'll try it out as soon as I have moment.

dmeehan commented 10 years ago

closed by #11