From: Andrea A. <aa...@op...> - 2008-03-31 15:44:19
|
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 |