Actually, yeah, I have one objection. Beside the fact that I am not so convinced that separating the two calls will make the api cleaner, I am quite worried that this would make the whole process much weaker. In the real world, when you want to add data in a dynamic way with a heavily loaded server with the current geoserver internal catalog it is not uncommon that everything breaks due to the fact that it takes quite some time to persiste the changes. You understand that starting to explode the number of calls to configure a single layer will not work in real use cases.
Simone. ------------------------------------------------------- Ing. Simone Giannecchini GeoSolutions S.A.S. Owner - Software Engineer Via Carignoni 51 55041 Camaiore (LU) Italy phone: +39 0584983027 fax: +39 0584983027 mob: +39 333 8128928 http://www.geo-solutions.it http://simboss.blogspot.com/ http://www.linkedin.com/in/simonegiannecchini ------------------------------------------------------- On Fri, May 8, 2009 at 5:25 PM, Justin Deoliveira <[email protected]> wrote: > For the first patch: > > + final LayerInfo layerInfo; > + final Map<String,String> layerProperties = new > HashMap<String,String>(2); > + if (form.getFirst("style") != null) > + layerProperties.put("style", > form.getFirstValue("style")); > + if (form.getFirst("wmspath") != null) > + layerProperties.put("path", > form.getFirstValue("wmspath")); > + if (layerProperties.isEmpty()) > + layerInfo = builder.buildLayer(cinfo); > + else > + layerInfo = builder.buildLayer(cinfo,layerProperties); > + catalog.add(layerInfo); > > I think this should be handled with a second call, rather than stack up > available options. It makes the api cleaner imo. And is less arbitrary. > For instance why do we support style and wms path but not the other > properties? > > So I would prefer the interaction to be: > > 1) PUT to upload the coverage > 2) PUT to modify the coverage > > Objections? > > Daniele Romagnoli wrote: >> Hi Justin, >> I'm really sorry about this. >> Next time I'll follows the steps you are talking about. >> I was working on the coverages ingestion and I needed to re-enable some >> capabilities available in 1.7.2 for an urgent application. >> I was too hasty. Sorry again. >> >> >> On Fri, May 8, 2009 at 4:22 PM, Justin Deoliveira <[email protected] >> <mailto:[email protected]>> wrote: >> >> Reviewing the commits more closely I definitely have a few concerns >> which affirms my preference to have these changes reviewed before hand. >> >> * The method added to catalog builder seems unnecessary to me. I think >> just the plain buildLayer can be called, and then properties overrideen >> after the fact, instead put putting them into a map and passing it into >> the method. It also does not sync up with any of the other methods in >> that class. I would like to keep that class as consistent as possible. >> >> * You have made changes to api without any discussion. Supporting >> wmspath and and style as query parameters is not documented as api >> options: >> >> >> http://gridlock.openplans.org/geoserver/1.7.x/doc/user/extensions/rest/rest-config-api.html#coverage-stores >> >> >> The previous RESTConfig allowed to handle them. I had tought during >> migration to extensions they have been forgotten. >> >> >> >> <http://gridlock.openplans.org/geoserver/1.7.x/doc/user/extensions/rest/rest-config-api.html#coverage-stores> >> >> Not saying we should not have them, but this is an extension now, not a >> community module anymore, such things require discussion before hand. >> >> >> You are Right. >> >> >> >> * Many of the changes seem a bit arbitrary, changing variable >> declarations to final for instance. Again, for consistency sake I prefer >> that you delegate to how the rest of the class / method does things when >> making chagnes. >> >> * Setting the layer name as the entity is again an api change, and one I >> don't agree with. It is more RESTful to set the location of the resource >> using the appropriate HTTP header, 'Location' like we do for the POST >> case. Also, the indication of where the resource should be down with a >> full uri, just returning the name assumes that URI's are non-opaque, ie >> the client has pre-knowledge of how URI's are built, which is another >> REST nono. >> >> >> Do you think it would be better if I revert my changes? >> Daniele >> >> >> >> >> >> -Justin >> >> Justin Deoliveira wrote: >> > Hi Daniele, >> > >> > I see you are making commits on 1.7.x to restconfig and main. I would >> > appreciate posting patches for pre review first before committing. >> > Especially in this case where we have not yet wrapped up discussion >> > about how to handle the PUT issue from previous email. >> > >> > I am not trying to hamper work here but I have been deemed the >> > maintainer of that extension, and while there are no hard rules >> in place >> > about who is allowed to commit, i would appreciate being to review >> > commits before they go through. >> > >> > -Justin >> > >> >> >> -- >> Justin Deoliveira >> OpenGeo - http://opengeo.org >> Enterprise support for open source geospatial. >> >> >> ------------------------------------------------------------------------------ >> The NEW KODAK i700 Series Scanners deliver under ANY circumstances! Your >> production scanning environment may not be a perfect world - but >> thanks to >> Kodak, there's a perfect scanner to get the job done! With the NEW >> KODAK i700 >> Series Scanner you'll get full speed at 300 dpi even with all image >> processing features enabled. http://p.sf.net/sfu/kodak-com >> _______________________________________________ >> Geoserver-devel mailing list >> [email protected] >> <mailto:[email protected]> >> https://lists.sourceforge.net/lists/listinfo/geoserver-devel >> >> >> >> >> -- >> ------------------------------------------------------- >> Eng. Daniele Romagnoli >> Software Engineer >> >> GeoSolutions S.A.S. >> Via Carignoni 51 >> 55041 Camaiore (LU) >> Italy >> >> phone: +39 0584983027 >> fax: +39 0584983027 >> mob: +39 328 0559267 >> >> >> http://www.geo-solutions.it >> >> ------------------------------------------------------- >> >> >> ------------------------------------------------------------------------ >> >> ------------------------------------------------------------------------------ >> The NEW KODAK i700 Series Scanners deliver under ANY circumstances! Your >> production scanning environment may not be a perfect world - but thanks to >> Kodak, there's a perfect scanner to get the job done! With the NEW KODAK i700 >> Series Scanner you'll get full speed at 300 dpi even with all image >> processing features enabled. http://p.sf.net/sfu/kodak-com >> >> >> ------------------------------------------------------------------------ >> >> _______________________________________________ >> Geoserver-devel mailing list >> [email protected] >> https://lists.sourceforge.net/lists/listinfo/geoserver-devel > > > -- > Justin Deoliveira > OpenGeo - http://opengeo.org > Enterprise support for open source geospatial. > > ------------------------------------------------------------------------------ > The NEW KODAK i700 Series Scanners deliver under ANY circumstances! Your > production scanning environment may not be a perfect world - but thanks to > Kodak, there's a perfect scanner to get the job done! With the NEW KODAK i700 > Series Scanner you'll get full speed at 300 dpi even with all image > processing features enabled. http://p.sf.net/sfu/kodak-com > _______________________________________________ > Geoserver-devel mailing list > [email protected] > https://lists.sourceforge.net/lists/listinfo/geoserver-devel > ------------------------------------------------------------------------------ The NEW KODAK i700 Series Scanners deliver under ANY circumstances! Your production scanning environment may not be a perfect world - but thanks to Kodak, there's a perfect scanner to get the job done! With the NEW KODAK i700 Series Scanner you'll get full speed at 300 dpi even with all image processing features enabled. http://p.sf.net/sfu/kodak-com _______________________________________________ Geoserver-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/geoserver-devel
