HI Ben, Thanks for your review!
On 02/02/2016 08:37 PM, Ben Caradoc-Davies wrote: > +1. Looking pretty good. Observations: Cool thanks, that makes it approved then. > > - Instead of metadata, it would be (arguably) more RESTful to support > a HEAD operation. You have a header for Last-Modified, could add an > X-Parent and X-Type, and name could be deduced from the URL. Or is the > need for custom HTTP headers a red flag? (Not to mention the problems > passing these through the stack.) Andrea raised the issue of HEAD and > I do not recall seeing a response. Please state why you are not using > HEAD. The core need I see is for directory traversal. I dislike > parsing URLs to find a parent resource, but would that be more > RESTful? Is the use of a metadata parameter a REST best practice? I did send an email about the HEAD operation. Last-modified will be supported. I did not actually think of using custom http headers for the other fields. That is interesting. We can support both. > > - I am not sure that "operation" is the right word to GET "metadata", > but I do not have an alternative. Metadata is not a format. "aspect", > perhaps? Do we have anything similar in the existing API? I guess I can change it back to metadata=1 as it was in an earlier version if that is preferred, but I found it more consistent with the other requests this way. The way I see it, getting the metadata is a different operation than getting the data. > - Are resources strictly a tree? Can you have loops? Unix filesystems > support this with hard links, in which there can be multiple paths for > a single resource, all canonical. What does this mean for recursive > deletion? It might be worth documenting your assumptions. I don't know if I would call it "supported". It is technically possible, but strongly discouraged, and most modern Linuxes make it impossible to hard link directories to prevent it from happening. I believe last time I encountered this about 10 years ago, my delete operation got into an infinitive loop and started to delete my whole hard drive. The ResourceStore API definitely doesn't support it, it would regard paths of different sizes as different resources. The same for JDBCstore, you can technically create such a loop directly into the database but I would regard that as a corruption of the database's integrity (and there are many other ways in which one could corrupt the resource database if an evil or stupid person manipulated the DBMS directly). > > - Should this API support parameters from the main REST API like > "recurse" and "quietOnNotFound"? There may be more. I am not saying > that I like these parameters; I think this GSIP is a bit more RESTful. > I just thought that users might expect them. Please skim the existing > REST API as a final review with consistency in mind. recurse -> delete, move and copy on a directory is always recursive. The alternative is failing, but I see little need to force people to specify this explicitly. If you do an operation on a directory that is generally what you intend to do. That is different to someone wanting to delete a store who might not realise he is deleting layers. I dunno, if people still want it I can easily implement this. quietOnNotFound -> I'd never log exceptions when someone requests a resource that is not found. That is not an error, it is still a valid request. Simply return 404 > > - Should these be lowercase? Uppercase looks out of place: > "<type> UNDEFINED | RESOURCE | DIRECTORY </type>" Yeah that is the internal representation, I can easily convert it to lowercase of course. > > - Document that POST returns a 405. ok. Regards Niels ------------------------------------------------------------------------------ Site24x7 APM Insight: Get Deep Visibility into Application Performance APM + Mobile APM + RUM: Monitor 3 App instances at just $35/Month Monitor end-to-end web transactions and take corrective actions now Troubleshoot faster and improve end-user experience. Signup Now! http://pubads.g.doubleclick.net/gampad/clk?id=267308311&iu=/4140 _______________________________________________ Geoserver-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/geoserver-devel
