Weirdness with styles
Closed this issue · 9 comments
There's something kind of odd with styles in OGCServer. In loadXML in BaseWMSFactory you have an if / else branch on style_count, the number of styles for a layer in the xml file. Zero raises an error; after that you distinct between style_count == 1 and style_count > 1.
Ignoring meta styles for now, if a layer has 1 style only, you add this style to both props layer.wmsdefaultstyle and layer.wmsextrastyles, by calling register_layer as follows:
self.register_layer(lyr_, style_name, extrastyles=(style_name,))
When requesting a map, a style must be contained in layer.wmsextrastyles to be a valid candidate for the STYLE WMS parameter (if not, you raise an exception). Since the layer's style has been added to both layer.wmsdefaultstyle and layer.wmsextrastyles, you can add this style to your request's STYLE parameter or not - you always get the same result and no exception.
The problem with these single-style layers comes with the GetCapabilities request. You advertise styles for a layer only, if there are extra styles present. However, you write out the default style and all extra styles. As a result, for single-style layers, the layer's default style occurs twice in the capabilities document. According to OGC, default styles may be omitted from the capabilities response.
With multi-style layers, things go different. You create an xxx_aggregates style, that is used as the layer's default style. The actual styles go into extra styles. As with all layers, both default and extra styles are published in the capabilities document. However, since the advertised xxx_aggregates default style is not part of extra styles, you'll get an exception, if you actually include it in you STYLES parameter.
IMO, only extra styles should be advertised in the capabilities document. Single-style layers should not add their style name to layer.wmsextrastyles. Then, these problems could be fixed with only two modifications: (actually three, since there is wms111 and wms130)
First, change WMS.py, line 128 from
self.register_layer(lyr_, style_name, extrastyles=(style_name,))
to
self.register_layer(lyr_, style_name, extrastyles=())
Then, change in both wms111.py, line 211 and wms130.py, line 234 from
for extrastyle in [layer.wmsdefaultstyle] + list(layer.wmsextrastyles):
to
for extrastyle in list(layer.wmsextrastyles):
However, after that, it is no longer possible to explicitly request the default style through the STYLE parameter for any layer (currently it's possible for single-style layers).
Carsten
Hi Carsten,
The styles code has worked for me as I've been always using default styles. We should definitely remove that duplicated style in GetCapabilities. Still advertise the default style using Mapnik's style name.
About extra styles, in a Mapnik's stylesheet you either use a style or not, there is no way to say "this style can also be applied to this Layer but it is not by default" because this is a WMS concept. I'd say this is something we need to put in a "wms config file" if it is really needed.
When I apply more than one style to a Mapnik Layer it is because it is easier to get the desired symbology and in that case all the layer styles should be handled as just one style, probably named "default".
If you want to write the tests and the code to fix the issues please go ahead.
Hi Manel,
ok, we could start off with this. Let's make things clear:
For layers with only one style, you want this style be advertised in the capabilities response. This style should be a valid option for the STYLES parameter.
For layers with several styles, of course, each individual style will be advertised in the capabilities response and also is a valid option for the STYLES parameter. What about the default style for these layers? Currently, you advertise this style named '<layer_name>_aggregates' (its internal name) in the caps document. However, this one is not a valid option for the STYLES parameter (raises an exception).
As you've proposed, we could name it 'default' in the caps document. When processing a request, we could just treat the hard-coded identifier 'default' like the style for this layer being not specified.
Hi Carsten,
For layers with multiple styles, we should advertise "default" as the "style" that will apply all the styles, I'm thinking about:
- style buildings_lines
- style buildings_labels
"default" will apply both, and we will advertise buildings_lines/labels as extra styles. What do you think?
Do you have any other use case?
Hi Manel,
sounds good. However, we should check OGC WMS specs, whether styles shall be unique throughout the server or if it's sufficient to have them unique for a layer. Not including the layer name (like buildings_default) creates a number of equally named styles, each being a completely different style. Not sure, whether the specs do recommend this. I can try finding an answer to that question.
Finally, to make it waterproof, we should define what do do, if the default style name ('default' or 'buildings_default') is actually the name of a defined Mapnik style. I recommend we should rename / mask this extra style name in the caps document (append '_1' or '_intl' for example). However, this name must be unique, too, so we'll end up in storing this mapping (reverse direction, get real name from masked name) in a hashmap in memory.
Hi Carsten,
the WMS specs allow for zero or more styles to be advertised for a layer. For me it is clear that the styles list is per layer, so no problem on repeating names, no need for masking. I've seen capabilities of other wms servers and you'll get a "default" style advertised in each layer.
When we start supporting wms groups we will need to think about styles inheritance, if the feature is really needed, because I can hardly think of an use case. I'm used to the plain list of layers in the Mapnik stylesheet.
Right now I see that removing the duplicated style name and properly supporting "default" in the multiple styles case will make it more robust and usable.
Hi Manel,
new style code is ready and tested. However, I don't think, we need special tests for that.
How to go on now. Do you expect me to push these changes directly to GIThub? Currently, I didn't get this working using eclipse and its GIT support. Also, I have no experiences with GIT. Likely I should push/commit the changes using my GIT user account so you can pull these into the master branch. However, I have no idea how to accomplish that. Maybe you could give me some hints...
I could provide you a diff file...
Carsten
Hi Carsten,
About the tests, it is the only way to ensure the code is working, they are a must.
To generate a pull request:
- Fork the ogcserver project into your account
- You probably want to create a different branch for each different feature you want to implement or fix, so you can work on different things at the same time
- Github will offer you to create a pull request from your changes
Hi Manel,
just as an early preview, I've forked the project and pushed the style improvements code changes to GitHub into branch style-improvements. Still working on the tests, which I will push when ready. Since this are my first experiences with GitHub (and git), could you please have a look at this new branch? I wonder whether this will likely work with a future pull request as expected.
Carsten