On Sun, Apr 15, 2012 at 6:24 PM, Justin Deoliveira <jdeolive@anonymised.com> wrote:
On Sun, Apr 15, 2012 at 5:12 PM, Gabriel Roldan <groldan@anonymised.com> wrote:
On Sun, Apr 15, 2012 at 5:55 PM, Justin Deoliveira <jdeolive@anonymised.com>
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
{