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?
On Sun, Apr 15, 2012 at 4:44 PM, Gabriel Roldan <[email protected]> wrote:
> The one thing that still bothers me with this API is that
> CatalogImpl.getStyleByName(WorkspaceInfo, String) can return one of
> four different things:
> - the requested style, given a non null workspace argument
> - a global style, given a null workspace argument
> - a style named the same but belonging to the default workspace, if
> none of the above was found.
> - if none of the above matches, returns a style from ANY workspace
> with that name
>
> My interpretation is this makes style lookup by workspace almost
> futil, but I may be missing the use cases that rely on this behavior.
> Could you elaborate and tell whether we can make this method contract
> stricter? (like in null workspace argument == global style, otherwise
> lookup enforced on the argument workspace. Period).
>
> TIA,
> Gabriel
>
> On Fri, Apr 13, 2012 at 6:19 PM, Justin Deoliveira <[email protected]>
> wrote:
> >
> >
> > On Fri, Apr 13, 2012 at 4:27 PM, Gabriel Roldan <[email protected]>
> wrote:
> >>
> >> On Fri, Apr 13, 2012 at 4:36 PM, Justin Deoliveira <
> [email protected]>
> >> wrote:
> >> > Hey Gabriel,
> >> >
> >> > Yeah, I think it would make sense to check that the workspace is null.
> >> > We
> >> > could perhaps be lax and actually look first for a global style and if
> >> > not
> >> > found use one found within a workspace if there is only one. Does that
> >> > make
> >> > sense.
> >> hmmm.. that last option would be confusing imho. Besides, I don't see
> >> it's usage. In my opinion, for clarity and backwards compatible, it
> >> should be make clear that getStyleByName(String) is a convenient
> >> shortcut for getStyleByName(Workspace, String) with a null workspace
> >> argument?
> >
> >
> > Works for me. I agree the most sensible approach.
> >>
> >> >
> >> > Also layer groups will have this same lookup issue.
> >> Indeed.
> >>
> >> Just found this fixing a side effect of
> >> CatalogImplTest.testAddStyleWithNameConflict(). That test does a
> >> number of checks on a style with a workspace, but the assigned
> >> workspace is not added to the catalog (hence has no id), so the test
> >> works with the default in memory catalog by accident. Fix is to just
> >> call addWorkspace() besides addStyle(). Ok with that too?
> >
> > Yup, i imagine there will be a couple of cases like these... feel free to
> > fix as you come across them.
> >>
> >>
> >> >
> >> > So yeah, go ahead and amend the lookup, we'll see what unit tests
> fail.
> >> Will do.
> >>
> >> Thanks for the prompt reply.
> >> Gabriel
> >> >
> >> > -Justin
> >> >
> >> > On Fri, Apr 13, 2012 at 2:00 PM, Gabriel Roldan <[email protected]>
> >> > wrote:
> >> >>
> >> >> Hey Justin,
> >> >>
> >> >> just a question. Working on getting all the jdbc catalog tests to
> pass
> >> >> after merging with the latest per workspace settings work, I'm not
> >> >> sure whether the old implementation of
> >> >> DefaultCatalogFacade.getStyleByName(String name) is still accurate.
> >> >>
> >> >> It is like this:
> >> >> public StyleInfo getStyleByName(String name) {
> >> >> for (Iterator s = styles.iterator(); s.hasNext();) {
> >> >> StyleInfo style = (StyleInfo) s.next();
> >> >> if (name.equals(style.getName())) {
> >> >> return ModificationProxy.create(style,
> StyleInfo.class);
> >> >> }
> >> >> }
> >> >> return null;
> >> >> }
> >> >>
> >> >> Meaning that it'll return the first style with the given name,
> >> >> regardless of it's workspace.
> >> >> Is that the intended behaviour, or should it return only a style
> >> >> that's "global".
> >> >>
> >> >> TIA,
> >> >> Gabriel.
> >> >>
> >> >> --
> >> >> Gabriel Roldan
> >> >> OpenGeo - http://opengeo.org
> >> >> Expert service straight from the developers.
> >> >
> >> >
> >> >
> >> >
> >> > --
> >> > Justin Deoliveira
> >> > OpenGeo - http://opengeo.org
> >> > Enterprise support for open source geospatial.
> >> >
> >>
> >>
> >>
> >> --
> >> Gabriel Roldan
> >> OpenGeo - http://opengeo.org
> >> Expert service straight from the developers.
> >
> >
> >
> >
> > --
> > Justin Deoliveira
> > OpenGeo - http://opengeo.org
> > Enterprise support for open source geospatial.
> >
>
>
>
> --
> Gabriel Roldan
> OpenGeo - http://opengeo.org
> Expert service straight from the developers.
>
--
Justin Deoliveira
OpenGeo - http://opengeo.org
Enterprise support for open source geospatial.
------------------------------------------------------------------------------
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
