Hi all,
I just committed the improvements and cleanup to the community rest module as per GEOS-2579.
I also updated the depending modules, namely RESTConfig, geosearch, QueryUsers, and sldService. I tested them best i could but it might be good if the owner of those modules could do some quick testing as well. David, I believe these fall under your domain?
Thanks,
-Justin
--
Justin Deoliveira
OpenGeo - http://opengeo.org
Enterprise support for open source geospatial.
On Tue, 2009-02-03 at 23:23 -0700, Justin Deoliveira wrote:
Hi all,
I just committed the improvements and cleanup to the community rest
module as per GEOS-2579.I also updated the depending modules, namely RESTConfig, geosearch,
QueryUsers, and sldService. I tested them best i could but it might be
good if the owner of those modules could do some quick testing as well.
David, I believe these fall under your domain?Thanks,
-Justin
I wrote most of the old RESTConfig, geosearch, and QueryUsers;
sldService is squarely outside my hands. I've fixed a failing test in
geosearch, but haven't really used the fine-toothed comb yet.
I also haven't tried out the new RESTConfig, though I'm pretty excited
about it. Looking over the docs though, I have a couple of points:
1) Status codes: There are a few places in the API as documented where
HTTP status codes are not used semantically; for example, a PUT that
changes the name of a coverage is not allowed, so you used "403
forbidden". A 403 implies that this is a security restriction, when in
fact this is a design choice, and no user will ever be able to change
names this way, so a 400 Bad request (or perhaps a 409 Conflict) would
be more apt. This doesn't matter so much for some admin hacking
together a script with curl, but if someone were writing a configuration
tool based on RESTConfig then this distinction would be quite useful.
2) Data encoded in resources that is already in the path. Going back to
the name changing issue: it would not be a concern if the name was not
supplied in two places in the request; the request path and the body.
Since the name is already implicitly in the request path, I would
suggest removing it from the XML and JSON representations so that a
conflicting request can't be encoded.
Hopefully I will be able to dig into the code a bit tomorrow; I'll be on
IRC for sure.
--
David Winslow
OpenGeo - http://opengeo.org/
Thanks for the initial review David. Some comments inline.
David Winslow wrote:
On Tue, 2009-02-03 at 23:23 -0700, Justin Deoliveira wrote:
Hi all,
I just committed the improvements and cleanup to the community rest module as per GEOS-2579.
I also updated the depending modules, namely RESTConfig, geosearch, QueryUsers, and sldService. I tested them best i could but it might be good if the owner of those modules could do some quick testing as well. David, I believe these fall under your domain?
Thanks,
-Justin
I wrote most of the old RESTConfig, geosearch, and QueryUsers;
sldService is squarely outside my hands. I've fixed a failing test in
geosearch, but haven't really used the fine-toothed comb yet.I also haven't tried out the new RESTConfig, though I'm pretty excited
about it. Looking over the docs though, I have a couple of points:1) Status codes: There are a few places in the API as documented where
HTTP status codes are not used semantically; for example, a PUT that
changes the name of a coverage is not allowed, so you used "403
forbidden". A 403 implies that this is a security restriction, when in
fact this is a design choice, and no user will ever be able to change
names this way, so a 400 Bad request (or perhaps a 409 Conflict) would
be more apt. This doesn't matter so much for some admin hacking
together a script with curl, but if someone were writing a configuration
tool based on RESTConfig then this distinction would be quite useful.
I was under the impression that 403 (Forbidden) did not imply an authentication issue, but just that the server is refusing to fulfill the request for some reason. Where as a 401 (Unauthorized) implies a security restriction. I am basing this understanding on:
http://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html
However i am fine with using a more appropriate code.
2) Data encoded in resources that is already in the path. Going back to
the name changing issue: it would not be a concern if the name was not
supplied in two places in the request; the request path and the body.
Since the name is already implicitly in the request path, I would
suggest removing it from the XML and JSON representations so that a
conflicting request can't be encoded.
That is a good idea. It would make things a bit simpler.
Hopefully I will be able to dig into the code a bit tomorrow; I'll be on
IRC for sure.
Well i have not committed any of the code yet... i was waiting for approval about the api first. So what is in svn is still the first cut of the api from way back. But if it is ok with you i would be happy to commit what i have, and refine as feedback comes in.
--
Justin Deoliveira
OpenGeo - http://opengeo.org
Enterprise support for open source geospatial.
On Wed, 2009-02-04 at 16:29 -0700, Justin Deoliveira wrote:
Thanks for the initial review David. Some comments inline.
David Winslow wrote:
> On Tue, 2009-02-03 at 23:23 -0700, Justin Deoliveira wrote:
>> Hi all,
>>
>> I just committed the improvements and cleanup to the community rest
>> module as per GEOS-2579.
>>
>> I also updated the depending modules, namely RESTConfig, geosearch,
>> QueryUsers, and sldService. I tested them best i could but it might be
>> good if the owner of those modules could do some quick testing as well.
>> David, I believe these fall under your domain?
>>
>> Thanks,
>>
>> -Justin
>>
>
> I wrote most of the old RESTConfig, geosearch, and QueryUsers;
> sldService is squarely outside my hands. I've fixed a failing test in
> geosearch, but haven't really used the fine-toothed comb yet.
>
> I also haven't tried out the new RESTConfig, though I'm pretty excited
> about it. Looking over the docs though, I have a couple of points:
>
> 1) Status codes: There are a few places in the API as documented where
> HTTP status codes are not used semantically; for example, a PUT that
> changes the name of a coverage is not allowed, so you used "403
> forbidden". A 403 implies that this is a security restriction, when in
> fact this is a design choice, and no user will ever be able to change
> names this way, so a 400 Bad request (or perhaps a 409 Conflict) would
> be more apt. This doesn't matter so much for some admin hacking
> together a script with curl, but if someone were writing a configuration
> tool based on RESTConfig then this distinction would be quite useful.
I was under the impression that 403 (Forbidden) did not imply an
authentication issue, but just that the server is refusing to fulfill
the request for some reason. Where as a 401 (Unauthorized) implies a
security restriction. I am basing this understanding on:http://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html
However i am fine with using a more appropriate code.
Shows what I know. The Forbidden = permissions thing is actually a
convention suggested to me by Tim Schaub when I was working on
RESTConfig way back when; I suppose I never bothered to fact-check that
one. Still, I think it is a useful convention.
> 2) Data encoded in resources that is already in the path. Going back to
> the name changing issue: it would not be a concern if the name was not
> supplied in two places in the request; the request path and the body.
> Since the name is already implicitly in the request path, I would
> suggest removing it from the XML and JSON representations so that a
> conflicting request can't be encoded.
That is a good idea. It would make things a bit simpler.
>
> Hopefully I will be able to dig into the code a bit tomorrow; I'll be on
> IRC for sure.Well i have not committed any of the code yet... i was waiting for
approval about the api first. So what is in svn is still the first cut
of the api from way back. But if it is ok with you i would be happy to
commit what i have, and refine as feedback comes in.
>
I think I would like to work this way.
--
David Winslow
OpenGeo - http://opengeo.org/
Well i have not committed any of the code yet... i was waiting for approval about the api first. So what is in svn is still the first cut of the api from way back. But if it is ok with you i would be happy to commit what i have, and refine as feedback comes in.
I think I would like to work this way.
Ok, I have committed my new version of the rest config classes to the RESTConfig module that implement the new proposed api as it statnds. All the new stuff is in the org.geoserver.catalog.rest package. Since the new code is much different than the old code i did not touch the old classes. I did however have to comment out the mapping for styles in the applicationContext.xml, and set it to the new version of the StyleFinder.
Enjoy 
-Justin
--
David Winslow
OpenGeo - http://opengeo.org/
--
Justin Deoliveira
OpenGeo - http://opengeo.org
Enterprise support for open source geospatial.