Hi,
since I'm using the rest module for the first time, I thought I
could use the occasion to provide a little feedback.
Here we go (long one, grab seat, coffee, muffins and whatnot):
* I've noticed the BeanResourceFinder is declared both in
rest and RESTConfig (seems a copy and paste)
* spring configuration seems a little verbose. Instead of
declaring the path in the map, can't we have the resources
themselves provide the path, and just declare the resources
in the spring context? Something like:
<bean name="layerGroupResource"
class="org.geoserver.restconfig.LayerGroupListResource">
<property name="WMSConfig">
<ref bean="wmsConfig" />
</property>
<property name="location" value="/layergroups/{group}.{type}"/>
</bean>
</property>
</bean>
This of course would be an extra, that is, it should not
disallow people from writing their own restlets of finders
(maybe the default router could scan the context for custom restlet,
finders and resources that do expose a location property,
which means, custom geoserver subclasses of the ones provided
by Restlet).
* MapResource should probably throw an Exception in the putMap
method, if the subclass does not override it a better error is, imho,
better than silence
* MapResources does not implement handlePost, yet it would be probably
handy to have one for the cases where using POST is justified
(resource creation whose url cannot be computed by the client)
* I do not see any unit or functional testing in either rest
and RestConfig... /me cries...
* MapResource and DataFormat. I see lots of places where the same
types are repeated, like "json"... can't DataFormat have a
getType() method, and we use it directly (so we turn that
getSupportedFormats return type from Map to List)?
* Json wise, it seems quite common to return not only json
maps, but also arrays. One could roll a ListResource, but
also DataFormats assumes a map.
What about having a CollectionResource instead? It's a bit
more generic...
* I have the strong impression the whole rest thing, as implemented,
is not thread safe. You instantiate the bean finder as a singleton,
that references a singleton resorce. When the call is made,
the context, request and response is provided using
myBeanToFind.init(getContext(), request, response);
Now, what happens when two threads do hit the same url with
different params at the same time? Boom 
* MapResource.getMap() cannot throw any exception? How do I
tell the world that something went wrong? Not sure if it's
better to let it throw a generic exception like putMap(...)
or have something that's rest specific.
For example, how would you distinguish between a security
issue and an internal error? Could be done with generic
exceptions, provided an exception translator could be found
to turn a generic exception to an HTTP error code
Ok, I'll write further observations as I find them down the road,
enough for the first mail
Cheers
Andrea
On Monday 31 March 2008 11:44:07 Andrea Aime wrote:
Hi,
since I'm using the rest module for the first time, I thought I
could use the occasion to provide a little feedback.
Here we go (long one, grab seat, coffee, muffins and whatnot):
* I've noticed the BeanResourceFinder is declared both in
rest and RESTConfig (seems a copy and paste)
Right, the rest module was basically pulled from RESTConfig. The copy of
RESTConfig that lives in the 1.6.x branch shouldn't be taken too seriously,
the trunk version is the one I'm actually working on. The 1.6.x version is a
somewhat older version that I just commented out parts of to have something
that compiled against the rest module.
* spring configuration seems a little verbose. Instead of
declaring the path in the map, can't we have the resources
themselves provide the path, and just declare the resources
in the spring context? Something like:
<bean name="layerGroupResource"
class="org.geoserver.restconfig.LayerGroupListResource">
<property name="WMSConfig">
<ref bean="wmsConfig" />
</property>
<property name="location" value="/layergroups/{group}.{type}"/>
</bean>
</property>
</bean>
This of course would be an extra, that is, it should not
disallow people from writing their own restlets of finders
(maybe the default router could scan the context for custom restlet,
finders and resources that do expose a location property,
which means, custom geoserver subclasses of the ones provided
by Restlet).
The thought behind doing it this way is that you may want to have different
paths to the same resource (for a trivial example, you might want to allow
/foo and /foo?{query} and have them handled by the same restlet).
* MapResource should probably throw an Exception in the putMap
method, if the subclass does not override it a better error is, imho,
better than silence
* MapResources does not implement handlePost, yet it would be probably
handy to have one for the cases where using POST is justified
(resource creation whose url cannot be computed by the client)
* I do not see any unit or functional testing in either rest
and RestConfig... /me cries...
* MapResource and DataFormat. I see lots of places where the same
types are repeated, like "json"... can't DataFormat have a
getType() method, and we use it directly (so we turn that
getSupportedFormats return type from Map to List)?
* Json wise, it seems quite common to return not only json
maps, but also arrays. One could roll a ListResource, but
also DataFormats assumes a map.
What about having a CollectionResource instead? It's a bit
more generic...
* I have the strong impression the whole rest thing, as implemented,
is not thread safe. You instantiate the bean finder as a singleton,
that references a singleton resorce. When the call is made,
the context, request and response is provided using
myBeanToFind.init(getContext(), request, response);
Now, what happens when two threads do hit the same url with
different params at the same time? Boom
* MapResource.getMap() cannot throw any exception? How do I
tell the world that something went wrong? Not sure if it's
better to let it throw a generic exception like putMap(...)
or have something that's rest specific.
For example, how would you distinguish between a security
issue and an internal error? Could be done with generic
exceptions, provided an exception translator could be found
to turn a generic exception to an HTTP error code
Ok, I'll write further observations as I find them down the road,
enough for the first mail
Cheers
Andrea
Thanks for the comments, Andrea.
!DSPAM:4040,47f106d2201412085621377!
David Winslow ha scritto:
...
* spring configuration seems a little verbose. Instead of
declaring the path in the map, can't we have the resources
themselves provide the path, and just declare the resources
in the spring context? Something like:
<bean name="layerGroupResource"
class="org.geoserver.restconfig.LayerGroupListResource">
<property name="WMSConfig">
<ref bean="wmsConfig" />
</property>
<property name="location" value="/layergroups/{group}.{type}"/>
</bean>
</property>
</bean>
This of course would be an extra, that is, it should not
disallow people from writing their own restlets of finders
(maybe the default router could scan the context for custom restlet,
finders and resources that do expose a location property,
which means, custom geoserver subclasses of the ones provided
by Restlet).
The thought behind doing it this way is that you may want to have different paths to the same resource (for a trivial example, you might want to allow /foo and /foo?{query} and have them handled by the same restlet).
I can have the same by registering the same resource twice and providing
the url in the spring context... and it still seems a lot more compact to me 
...
Thanks for the comments, Andrea.
Hum, ok, so I gather that if I need a list based resource, I'll have
to roll my own restlet? In the work I'm doing I've been asked to return
arrays of json maps.
Cheers
Andrea