New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add metadata to optionally not emit root layer name in WMS Capabilities XML #5404

Open
tomkralidis opened this Issue Mar 7, 2017 · 18 comments

Comments

Projects
None yet
4 participants
@tomkralidis
Member

tomkralidis commented Mar 7, 2017

A sample GetCapabilities request shows the following:

...
<Capability>
...
<Layer>
 <Name>foo</Name>
 <Title>bar</Title>
...

The root layer element's foo value is derived from the mapfile's MAP.NAME value. WMS says any Layer with a Name element should be GetMap capable. In our case this requests all layers in the WMS, which may prove problematic for a number of reasons:

  1. potential performance issue
  2. if one layer is broken/not functioning, will result in error
  3. even on a successful request, the result is not meaningful/valuable

The workaround is to set MAP.NAME to "". The proper fix is to tell MapServer to not emit the root layer name via something like "wms_rootlayer_name_enabled" "false"

@tomkralidis tomkralidis self-assigned this Mar 7, 2017

@jratike80

This comment has been minimized.

jratike80 commented Mar 7, 2017

@tomkralidis

This comment has been minimized.

Member

tomkralidis commented Mar 7, 2017

cc @dmorissette @rouault for thoughts.

@jratike80 I would tend to agree however I'm not sure how many installations utilize the functionality (imagine a 2 layer mapfile where is it easy to just call the root layer) already, so the concern is breaking existing applications.

@tomkralidis

This comment has been minimized.

Member

tomkralidis commented Mar 8, 2017

FYI PR in #5405 pending consensus.

@jratike80

This comment has been minimized.

jratike80 commented Mar 8, 2017

Copy of the the comment that I wrote to PR follows because I think this is where it belongs.

I think that also the WMS documentation http://mapserver.org/ogc/wms_server.html should be updated from this place:

Map Name and wms_title:
WMS Capabilities requires a Name and a Title tag for every layer. The Map’s NAME and wms_title metadata will be used to set the root layer’s name and title in the GetCapabilities XML output. The root layer in the WMS context corresponds to the whole mapfile.

Did you consider an alternative to add an explicit new metadata item "wms_name" that would override the name of the mapfile? The same effect than by setting
"wms_rootlayer_name_enabled" "yes"
would be obtained then by setting an empty wms_name
"wms_name" ""
That might be more user friendly because at the moment and with this change also in the future the WMS title is set in metadata but WMS name in a different and sort of non-logical place.

@dmorissette

This comment has been minimized.

Contributor

dmorissette commented Mar 8, 2017

(Ooppss... copying my comment from the PR here)

I agree with @jratike80's comment and suggestion of setting "wms_name" "" instead of creating a new "wms_rootlayer_name_enabled" metadata.

@tomkralidis

This comment has been minimized.

Member

tomkralidis commented Mar 8, 2017

@jratike80 so implement wms_name instead (suggest calling this wms_rootlayer_name)? Isn't implementing "wms_name" the same effect as setting MAP.NAME to "", then?

@tomkralidis

This comment has been minimized.

Member

tomkralidis commented Mar 8, 2017

@jratike80

This comment has been minimized.

jratike80 commented Mar 9, 2017

Yes, wms_rootlayer_name would be consistent with existing wms_rootlayer_abstract, wms_rootlayer_keywordlist, and wms_rootlayer_title.

I was thinking that "wms_name" would have the same effect than MAP NAME but just interpreted later and having precedence . But by looking at WMS standard that name is not good because it makes user to think that it would be a pair to "wms_title" which sets title for the whole service. However, the standard mandates that the service name is always "WMS". So forget "wms_name".

New "wms_rootlayer_name" would make is possible to have different names in MAP-NAME and MAP-METADATA-"wms_rootlayer_name". You may consider if this is good or not. From the documentation I can't find especially much use for MAP-NAME

NAME [name]
Prefix attached to map, scalebar and legend GIF filenames created using this mapfile. It should be kept short.

Some edits to documentation would be needed: a new entry for wms_rootlayer_name and some edits to "Map Name and wms_title" and perhaps a new bullet item for the name of the root layer.

tomkralidis added a commit to tomkralidis/mapserver that referenced this issue Mar 17, 2017

tomkralidis added a commit to tomkralidis/mapserver that referenced this issue Mar 17, 2017

@tomkralidis

This comment has been minimized.

Member

tomkralidis commented Mar 17, 2017

@jratike80 PR #5405 updated. Logic sequence in pseudo code:

if MAP.WEB.METADATA.wms_rootlayer_name is set and is not ""
    emit MAP.WEB.METADATA.wms_rootlayer_name
else
    proceed as normal/existing behaviour

Makes sense? We can merge and then update docs if this works for you.

@jratike80

This comment has been minimized.

jratike80 commented Mar 17, 2017

Thinking about and is not "". It means that if MAP>NAME is set there is no way to prevent it from appearing as the name of the root_layer.

if MAP.WEB.METADATA.wms_rootlayer_name is set and 
     if MAP.WEB.METADATA.wms_rootlayer_name is ""
           remove root layer name from GetCapabilities
     else
           emit MAP.WEB.METADATA.wms_rootlayer_name
else
    proceed as normal/existing behaviour

Would that make sense?

@tomkralidis

This comment has been minimized.

Member

tomkralidis commented Mar 17, 2017

@jratike80 even if MAP.NAME is set wms_rootlayer_name (if set) overrides it. See the tests as part of the PR. Note that existing tests pass as well so the PR does not break existing functionality. Or are you thinking to not emit the root layer name as a default?

@jratike80

This comment has been minimized.

jratike80 commented Mar 17, 2017

Emit root layer name is for sure useful. I was just thinking about a small corner case when user, for some reason, would like to override root layer name that heritages from MAP-NAME into nothing and remove the name element from GetCapabilities XML.

I was reading just your pseudocode
if MAP.WEB.METADATA.wms_rootlayer_name is set and is not ""
which made me believe that if wms_rootlayer_name is set and IS an empty string "" then the rootlayer name from MAP-NAME is kept.

I am only a user who does not really write nor read code but test msautotest/wxs/ows_wms_rootlayer_name_empty.map actually seems to test overriding MAP-NAME into empty.

In test msautotest/wxs/ows_wms_rootlayer_name.map you have perhaps done copy-paste without edit:
+# Results expected
+# getcapabilities: root element does not emit element
Shouldn't it be the opposite and give Name "foo" as a result?

tomkralidis added a commit to tomkralidis/mapserver that referenced this issue Mar 17, 2017

@tomkralidis

This comment has been minimized.

Member

tomkralidis commented Mar 17, 2017

Emit root layer name is for sure useful. I was just thinking about a small corner case when user, for some >reason, would like to override root layer name that heritages from MAP-NAME into nothing and >remove the name element from GetCapabilities XML.

I was reading just your pseudocode
if MAP.WEB.METADATA.wms_rootlayer_name is set and is not ""
which made me believe that if wms_rootlayer_name is set and IS an empty string "" then the rootlayer >name from MAP-NAME is kept.

If wms_rootlayer_name is "" then rootlayer name from MAP-NAME is not kept. The test case in /msautotest/wxs/ows_wms_rootlayer_name.map should take care of this corner case.

I am only a user who does not really write nor read code but test msautotest/wxs/ows_wms_rootlayer_name_empty.map actually seems to test overriding MAP-NAME into empty.

In test msautotest/wxs/ows_wms_rootlayer_name.map you have perhaps done copy-paste without edit:
+# Results expected
+# getcapabilities: root element does not emit element
Shouldn't it be the opposite and give Name "foo" as a result?

Typo in msautotest/wxs/ows_wms_rootlayer_name.map now fixed.

Thanks for the valuable comments and discussion! Ok to merge PR?

@mkofahl

This comment has been minimized.

Contributor

mkofahl commented Mar 18, 2017

@jratike80

This comment has been minimized.

jratike80 commented Mar 18, 2017

Ok to merge PR.

@tomkralidis

This comment has been minimized.

Member

tomkralidis commented Mar 18, 2017

@mkofahl correct, setting MAP.NAME does indeed suppress the root Layer name, however there are cases where we want MAP.NAME set (temp files creation, etc.)

@tomkralidis

This comment has been minimized.

Member

tomkralidis commented Mar 18, 2017

@jratike80 thanks, merging.

@tomkralidis

This comment has been minimized.

Member

tomkralidis commented Mar 18, 2017

FYI related docs PR in mapserver/docs#168

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment