On Sun, Apr 15, 2012 at 6:24 PM, Justin Deoliveira <[email protected]> wrote:
>
>
> On Sun, Apr 15, 2012 at 5:12 PM, Gabriel Roldan <[email protected]> wrote:
>>
>> On Sun, Apr 15, 2012 at 5:55 PM, Justin Deoliveira <[email protected]>
>> wrote:
>> > It may be a bit too lax... but that is the general idea. We did the same
>> > thing for layers so I kind of just followed that template. As you say
>> > the
>> > issue is null is kind of ambiguous... does it mean global or does it
>> > mean
>> > any workspace? I sort of opted to make it mean both since we don;t
>> > really
>> > expose the concept of an ANY workspace. And it only really takes affect
>> > in
>> > cases where only a single style exists which matches the name.
>> >
>> > That said I am fine with making it stricter, as I can't think of any use
>> > case that doesn't involve going through a virtual service endpoint, and
>> > that
>> > which the right workspace will be forcibly set anyways. So to be clear
>> > you
>> > are advocating for removing the last part of the lookup heuristic? Have
>> > you
>> > tried doing so and see what happens in unit tests?
>>
>> Find attached the complete patch with the tests expectations that
>> needed to be changed.
>> I think the check for ANY_WORKSPACE in
>> DefaultCatalogFacade.getStyleByName(Workspace, String) should also be
>> removed. End goal being having less code that's never gonna be
>> executed, as you mentioned, because it is there without a use case
>> that hits it.
>>
> Well the fact that test assertions are being changed sort of means there is
> a use case :)
>
> I am always weary of changing this sort of stuff just for the purposes of
> code cleanup. Can you provide more information about how it makes your life
> easier to have this removed? Is it strictly just api clarity? Does it make
> implementing a new catalog easier?
>
> Anyways, not trying to discourage just trying to be careful. As this is
> generally new code shouldn't be a big deal though. Just want to do my due
> diligence :)
>
It does make my life easier implementing a new catalog. I'd need to
switch back to the other branch and revert the patch to recall why
exactly, as I'm on master and cherry-picked this one, which I can do.
But in any case, take the following as an example of why I think
enforcing a clear contract on new code is important: running the full
build with the stricter contract revealed the following bug: modifying
a style on a given workspace leads to modifying any style on any
workspace that's named the same. So far that REST config code seems
like the only user of such an expectation, and proved to be a bug (the
other methods in StyleResource do take care of getting the requested
workspace).
So I'm not trying to be an asshole here, and I do realize CatalogImpl
in fact prevents adding two styles with the same name, even if on
different workspaces. But that might change too (how long it's gonna
be until someone complains he can't have styles road and foo:road). In
any case my motivation to pursue this is not anticipating to that
request. It's having a clear contract to attach to when implementing
an alternate catalog facade. Having to put so many if's by looking at
what the default one does instead of having a contract to comply with
is painful, and so many control sentences also smell bad (for example,
as we go down from CatalogImpl to CatalogFacade, it'd be better if
CatalogFacade's method doesn't need to care about whether one input
argument is null, as CatalogImpl already did, which is the case for
ANY_ and NO_WORKSPACE, and that kind of stuff).
diff --git
a/src/restconfig/src/main/java/org/geoserver/catalog/rest/StyleResource.java
b/src/restconfig/src/main/java/org/geoserver/catalog/rest/StyleResource.java
index ef6cdf7..ad3e80a 100644
--- a/src/restconfig/src/main/java/org/geoserver/catalog/rest/StyleResource.java
+++ b/src/restconfig/src/main/java/org/geoserver/catalog/rest/StyleResource.java
@@ -197,10 +197,11 @@ public class StyleResource extends
AbstractCatalogResource {
@Override
protected void handleObjectPut(Object object) throws Exception {
String style = getAttribute("style");
-
+ String workspace = getAttribute("workspace");
+
if ( object instanceof StyleInfo ) {
StyleInfo s = (StyleInfo) object;
- StyleInfo original = catalog.getStyleByName( style );
+ StyleInfo original = catalog.getStyleByName( workspace, style );
//ensure no workspace change
if (s.getWorkspace() != null) {
@@ -218,7 +219,7 @@ public class StyleResource extends AbstractCatalogResource {
* Force the .sld file to be overriden and it's Style
object cleared from the
* ResourcePool cache
*/
- StyleInfo s = catalog.getStyleByName( style );
+ StyleInfo s = catalog.getStyleByName( workspace, style );
catalog.getResourcePool().writeStyle( s, (Style) object, true );
/*
* make sure to save the StyleInfo so that the Catalog
issues the notification events
------------------------------------------------------------------------------
For Developers, A Lot Can Happen In A Second.
Boundary is the first to Know...and Tell You.
Monitor Your Applications in Ultra-Fine Resolution. Try it FREE!
http://p.sf.net/sfu/Boundary-d2dvs2
_______________________________________________
Geoserver-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/geoserver-devel
