enhanced MapFish Server code

[elemoine]

This is a meta ticket for:

• #88 Python class Feature and FeatureCollection unneeded
• #177 mf-layer generates too much code
• #183 create a filter hierarchy in MapFish Server
• #185 MapFish Server doesn't work out-of-the-box if the PostGIS table includes numeric and/or decimal columns

[elemoine]

The dump of the editing database must also be installed on MapFish demo server.

[sanjiv]

Hi Eric,

I just tested your patch and it seems to work perfectly. The server code is definitely much improved now. Thanks for cranking it up.

regards Sanjiv

[elemoine]

I updated the MapFish patch with changes to accommodate #185.

[sypasche]

Fredj showed me some features on the branch he worked on which I can't find in your patch. Is it intentional or did you miss part of his work?

1. Use of multiple inheritance in the controllers. This makes implementing post/put/method unnecessary because they are inherited. He also implemented a readonly attribute to deny the mutating methods (put, delete).
2. Use of the Mapper.resource method in routing.py which will deal with rest methods automatically and reduces routing code quite a bit (see source:sandbox/fredj/MapFishSample/mapfishsample/config/routing.py)

more to come...

[elemoine]

Sylvain, thanks for the review. Below are my answers to your first two comments.

1. I favor composition over inheritance here because I think it leads to a better API. My goal is that the protocol module (protocol.py) can be easily reused outside the MapFish framework (Sanjiv would use it in his TG2 project). If we make Protocol a mixin class (as fredj did) I think it makes it a bit hard to understand how to use that class - one must define the properties readonly, model and mapped_class in the inherited class, which I think is kind of hard to document. In my implementation, these properties are passed to the Protocol constructor (see controller template code. Moreover, having the get/post/put/delete methods in the generated controller code makes it clear to the user what can be overriden. If you disagree and you feel that my arguments aren't strong enough, please tell and I'll reconsider all this.
2. I've been a bit chicken with using Mapper.resource because it actually generates more routes that we actually need, see this section in the Routes user manual. Again, I can change my mind if you think using it is a better choice.

[sypasche]

        raise NotImplementedError, 'Filter cannot be instantiated' 
...
        raise NotImplementedError, 'to_sql_expr must be implemented'

use raise Not Implemented Error?('...') which is what PEP 8 recommends.

+            exported = self.__table__.c.keys()

Isn't the c property deprecated in SA 0.5?

Index: python/mapfish/lib/request.py
===================================================================
--- python/mapfish/lib/request.py

I'm not sure I understood the reasoning for having a separate request.py file instead of having it inline in protocol.py. Do you intend to use it outside of protocol?

+        attributes = {}
+        for k in exported:
+            k = k.encode('ascii')

Is this to convert from unicode to str? (I find k = str(k) more readable, but I'm fine with this too).

+        return Feature(id=self.fid, 
+                       geometry=self.geometry, 

nit: trailing space

+def do_select_request(Session, Class, expr=None, limit=None):
+    """

Use lower case parameter name for session.

+
+if __name__ == "__main__":
+    pass

Should this be removed?

+class Spatial(Filter):
+    BOX = 'BOX'
+    WITHIN = 'WITHIN'

The 'BOX' filter uses the '&&' postgis operator which is documented as: "The "&&" operator is the "overlaps" operator. If A's bounding box overlaps B's bounding box the operator returns true."

Maybe we should call this 'OVERLAP' instead?

+        if tolerance > 0:
+            e = func.distance(geom, pg_point) < tolerance
+        else:

That would be nice to use the && operator here so that indexes can be used. That means creating a polygon around the point with a size of 2*tolerance, and having a query as described on http://postgis.refractions.net/documentation/manual-1.3/ch04.html#id2685786. This could be done later by opening a ticket and adding a comment there.

from geojson.codec import PyGFPEncoder

I tried this in a "paster shell":

from geojson.codec import PyGFPEncoder <type 'exceptions.Import Error?'>: No module named geojson.codec

Is there a missing dependency in setup.py?

+class Protocol(object):
+
+    def __init__(self, Session, mapped_class, readonly=False):
+        self.Session = Session

same comment as above for session.

+    def show(self, request, response, id, format='json'):
+        """ Build a query based on the id argument, send the query
+        to the database, and return a GeoJSON representation of the
+        result. """
+        return self._encode(self.Session.query(self.mapped_class).get(id))

so only json is supported for now? It should return an error if another format is asked.

+    def create(self, request, response):
+        """ Read the GeoJSON feature collection from the request body and
+            create new objects in the database. """
+        if self.readonly:
+            response.status_code = 404

I'd rather use 403 Forbidden. This applies to several places in the file.

+        factory = lambda ob: GeoJSON.to_instance(ob)
+        collection = loads(content, object_hook=factory)

Is this safe? I mean could an attacker create json that could do nasty things with the GeoJSON.to_instance constructor?

Thanks for the work, please commit with comments addressed.

[elemoine]

Replying to sypasche:

{{{ raise Not Implemented Error?, 'Filter cannot be instantiated' ... raise Not Implemented Error?, 'to_sql_expr must be implemented' }}} use raise Not Implemented Error?('...') which is what PEP 8 recommends.

Ok.

{{{ + exported = self.table.c.keys() }}} Isn't the c property deprecated in SA 0.5?

Not on Table objects. From SA 0.5 doc: "the c attribute continues to remain on Table objects where they indicate the namespace of Column objects present on the table."

{{{ Index: python/mapfish/lib/request.py =================================================================== --- python/mapfish/lib/request.py }}} I'm not sure I understood the reasoning for having a separate request.py file instead of having it inline in protocol.py. Do you intend to use it outside of protocol?

I removed it.

{{{ + attributes = {} + for k in exported: + k = k.encode('ascii') }}} Is this to convert from unicode to str? (I find k = str(k) more readable, but I'm fine with this too).

I changed that to using str.

{{{ + return Feature(id=self.fid, + geometry=self.geometry, }}} nit: trailing space

Good catch. Fixed.

{{{ +def do_select_request(Session, Class, expr=None, limit=None): + """ }}} Use lower case parameter name for session.

I'd rather not since it is a class.

{{{ + +if name == "main": + pass }}} Should this be removed?

Yes. Done.

{{{ +class Spatial(Filter): + BOX = 'BOX' + WITHIN = 'WITHIN' }}} The 'BOX' filter uses the '&&' postgis operator which is documented as: "The "&&" operator is the "overlaps" operator. If A's bounding box overlaps B's bounding box the operator returns true." Maybe we should call this 'OVERLAP' instead?

Hmmm the spatial box filter is about getting the features that intersect a given box. So it looks like the use of the '&&' operator isn't appropriate here. I'll open a separate ticket for that thing.

{{{ + if tolerance > 0: + e = func.distance(geom, pg_point) < tolerance + else: }}} That would be nice to use the && operator here so that indexes can be used. That means creating a polygon around the point with a size of 2*tolerance, and having a query as described on http://postgis.refractions.net/documentation/manual-1.3/ch04.html#id2685786. This could be done later by opening a ticket and adding a comment there.

I'll open a ticket for this.

{{{ from geojson.codec import PyGFPEncoder }}} I tried this in a "paster shell": from geojson.codec import PyGFPEncoder <type 'exceptions.Import Error?'>: No module named geojson.codec

Maybe your geojson package is too old. Try:

virtualenv env
cd env/bin
./easy_install geojson
./python
>>> from geojson.codec import PyGFPEncoder
>>>

Is there a missing dependency in setup.py? {{{ +class Protocol(object): + + def init(self, Session, mapped_class, readonly=False): + self.Session = Session }}} same comment as above for session.

Session is a class object.

{{{ + def show(self, request, response, id, format='json'): + """ Build a query based on the id argument, send the query + to the database, and return a GeoJSON representation of the + result. """ + return self._encode(self.Session.query(self.mapped_class).get(id)) }}} so only json is supported for now? It should return an error if another format is asked.

Ok.

{{{ + def create(self, request, response): + """ Read the GeoJSON feature collection from the request body and + create new objects in the database. """ + if self.readonly: + response.status_code = 404 }}} I'd rather use 403 Forbidden. This applies to several places in the file.

Ok.

{{{ + factory = lambda ob: GeoJSON.to_instance(ob) + collection = loads(content, object_hook=factory) }}} Is this safe? I mean could an attacker create json that could do nasty things with the GeoJSON.to_instance constructor?

I trust the GeoJSON.to_instance code. See the code.

[elemoine]

(In [867]) enhanced Map Fish? Server code, r=sypasche, see Releases/0.3/HowToMigrateToMapFishServer0.3 to migrate your application code. (closes #184)