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. ------------------------------------------------------------------------------ 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
