I'm going to to try and address most of your comments and questions inline,
then send a summary in another email, so please bear with me.
On Fri, Jul 14, 2017 at 9:17 AM, Andrea Aime <[email protected]>
wrote:
> Ok,
> more substantive review this time.
>
> So, StyleGroup is really a link to a multi-layer SLD.
>
> The statement "StyleGroupInfo will not be stored as catalog objects" threw
> me off a few times, I believe that what you mean is that
> StyleGroupInfo is not a top level catalog object, so it has no identity,
> and cannot be searched directly from a catalog object,
> but can only be found by scanning the containing group (I had no
> familiarity with LegendInfo so could not really see what you meant there
> at a first read).
>
> Correct. Additionally, we'd probably only ever use a single static
instance, similar to CatalogFacade.NO_WORKSPACE
> Making it a PublishedInfo will cause some confusion in Catalog, as one can
> search for PublishedInfo in the list method
> of the Catalog, e.g. in the preview page we have:
>
> CloseableIterator<PublishedInfo> pi = catalog.list(PublishedInfo.class,
> filter, (int) first, ...
>
> Since this thing exist only in a group, does it make sense to list it?
> Basically a style is just a style, but becomes a group
>
Probably not, since it is not something in the catalog with an id.
> the moment it's being referred as one in another layer group... and you
> say it can be chosen and previewed in the style editor
> (but really, what you're doing there is previewing the current style no?)
>
Yes, previewing the current style. For the style editor, this would
probably be implemented through the WMS SLD parameter, rather than a
StyleGroup (I think the proposal mentions this, but if not I can update it).
> but when you refer to it from the layer preview, then it can only be
> previewed through the containing group?
> My head is starting to spin.
>
> Then I see the rest representation, and it really looks like what you're
> after there is a empty layer slot with
> a style attached to it, and when that happens, then the style is to be
> used as as a sub-group definition.
>
> Before I make a counter proposal, one more thing, I have a patch sitting
> on my disk allowing someone to
> preview a group while editing a style, because it's common to have to look
> at a whole map while tweaking
> a single style of it.
>
> With this in mind, wouldn't it be simpler to just allow the layer group to
> have an empty layer entry, and when
> that happens, then make the style spec compulsory and use it as the source
> of layers, without the need
> to be talking about a style group to start with?
>
That does accomplish the same thing, without making (as many) changes to
the Catalog API. Null entries in the layer list of a layer group feels a
bit hacky, but we already do this for styles (when using a default style),
so I guess it is fine.
If you modify the LayerGroupHelper to perform the style expansion when no
> layer is found, most of the
> current code should be working already.
>
> In the style editor, then also allow a "preview style as a group"
> link/option, that would activate
> after checking that the style is indeed referring to layers (existing
> ones).
>
>
Now, some other concerns the proposal is not covering.
> Using GetMap with SLD or SLD_BODY the code will perform a few consistency
> checks (assuming you're not entering library mode, mind), in particular,
> that all layers referred to by name are there.
> However, nowadays most styles are not referring to layers, or are
> referring to layers that do not exist, so some checking
> should be done, either before or after the fact, to ensure a style used as
> a group definition is indeed suitable for the job.
>
> Also, taken into consideration deletion and cascading deletion via UI and
> REST. Nowadays if a layer gets removed
> groups containing it are found and listed in a warning dialog, and on
> confirmation they are updated removing the
> layer.
> If a layer is referred by a style group, what do we do? Update the style?
> Disallow the deletion?
> Or allow the catalog to get into an inconsistent state? The latter does
> not seem acceptable to me, as the current behavior does better than that.
>
> Updating the style seems infeasible, especially given the number of
supported style formats we have (I'm not sure if CSS has an equivalent to
NamedLayers, but I am fairly sure YSLD does).
If you are using this feature in the first place, I expect it is likely you
intend to have a style that can easily be transferred between different
systems (e.g., different geoservers, or geoserver and some other program
that supports SLD), assuming they have the same layer names defined.
My first thought would have been to treat the style like any other external
style, and allow it to become invalid, but I can see how this would be
problematic.
That said, even in current geoserver, it is not uncommon to make a style
invalid after some sort of catalog change (moreso if there are changes to
the underlying data, especially changes to attribute names, but even
so...), or have a style that only works against certain layers.
Given this, I am still leaning towards letting the change (be it deleting a
layer, or renaming one) go through without any changes to the style. In the
case of a deletion through the UI, we could present a custom warning
dialog. As for the REST api, we could send along an informational message
with the 200 response.
> Also, nowadays references to layers are done via ID, which is an
> immutable, but style groups would be referring layer by
> name, which is mutable instead. So a new set of checks should be enacted,
> making sure that when a store changes
> workspace, or a layer gets renamed, the style groups referring to it are
> modified.
> I see no other option here, as disallowing the change would require the
> user to first remove the reference from the style,
> then modify the store/layer, and then go back and add the new name in the
> style.
>
> As above, this sort of style is well-suited for transfer between different
systems, in part because it refers to layers by name. I agree that
disallowing the name change would be problematic.
Since the style body is not actually part of the Catalog, checks for style
name would be relatively expensive from a performance perspective, since
one would have to parse any styles in a group and check for name matches.
There is also the complication that the if the layer name has no workspace
prefix, it will apply to the layer in the default workspace, so changing
the default workspace will also affect the validity of such styles.
> One final note on the editing of such styles. Nowadays when validating a
> style we just check it's valid in generic
> terms, that it can be parsed. But binding it to a layer seems to suggest
> two more validations could be performed:
>
> - Check that the layer exists
> - Check the attributes referred from the styles are usable with that
> layer
>
> I say could because at least the second one could also be performed when
> associating a layer to a style, and
> nowadays it's not done. So don't take this a requirement.
>
This touches on one of the reasons it is so easy to have an invalid style
these days.
My biggest concern with enforcing style validity in this case is if you are
editing a fairly large style (perhaps already associated with a style), and
want to save it part way through, even if it might not be valid (this is
especially relevant for GeoServer, given the standard server timeout). This
is one of the reasons that the Style Editor UI still allows you so save a
syntactically invalid style.
>
> Cheers
> Andrea
>
> Torben
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Geoserver-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/geoserver-devel
