restore version as optional parameter for GetCapabilities

[tomkralidis]

Email from Pål Kristensen:

I did a build of mapserver nightly this evening and noticed some changed WMS behavior compared to my last build (approx 2 months ago). The old build does reply to at GetCapabilities request that misses the version parameter with a version 1.1.1 capabilities xml-file. The new build fails withe an error message that says that the version parameter is missing. According to the WMS 1.1.1 standard version is a mandatory parameter so the new behavior is correct according to the standard. Unfortunately many of the clients around do not comply with the standard, and of course the server gets the blame.

[tomkralidis]

The only related change I could spot is ​http://trac.osgeo.org/mapserver/changeset/7352. I have tested this in branch-5.0 and it does work. Will fix/revert for trunk.

[tomkralidis]

Fixed in r7407. Pål: can you confirm? I will then close this ticket.

[dmorissette]

Tom, in r7407 you only commented out the test. Please don't leave unused code commented out as it accumulates over time (unless there is a good reason to leave it there). I have removed it in r7410.

[tomkralidis]

Forgot. Thanks for removing.

[dmorissette]

BTW, do we not need to distinguish between version being not specified (optional) and version being specified in an invalid format? There is a legit case to produce an exception if the version format is invalid, but not if it's simply omitted.

I'm working on a fix to handle this.

[tomkralidis]

Good point. Might want to add some tests to msautotest as well for this.

[dmorissette]

I have modified msOWSParseVersionString() to return either OWS_VERSION_NOTSET (-1) or OWS_VERSION_BADFORMAT (-2) if the version cannot be parsed, updated all calling code to use those two new #defines instead of the -1 constant, and finally updated the test that was at the root of this ticket in mapwms.c to still produce an exception if the version format is invalid, but not if it's omitted since it's optional in the GetCapabilities case.

I have not added tests to msautotests, I will leave that to you Tom.

[tomkralidis]

Thanks for this. I've tested with:

$ ./mapserv QUERY_STRING="map=/tmp/test.map&service=WMS&version=1."

and I do not get back an exception, but I should.

Does msOWSParseVersionString need fixing?

[tomkralidis]

Dan: here's a patch that works against stuff like "1", "1.", ".1"; not sure if you're working on this already, I will commit if you are not and this change is satisfactory.

Index: mapows.c
===================================================================
--- mapows.c    (revision 7411)
+++ mapows.c    (working copy)
@@ -391,8 +391,19 @@
     if (pszVersion)
     {
         int nVersion = 0;
+        int nValid = 0;
+        int i;
+
         digits = msStringSplit(pszVersion, '.', &numDigits);
-        if (digits == NULL || numDigits < 2 || numDigits > 3)
+
+        for (i=0;i<numDigits;i++) {
+          if (strcasecmp(digits[i], "\0") == 0) {
+            nValid = 1;
+            break;
+          }
+        }
+
+        if (digits == NULL || numDigits < 2 || numDigits > 3 || nValid == 1)
         {
             msSetError(MS_OWSERR, 
                        "Invalid version (%s). Version must be in the "

[dmorissette]

Do we need to be that picky?

[tomkralidis]

Well, if we are going to be checking for an invalid format. Here's what is from OWS Common 1.1, subclause 7.3.1:

" Each OWS Implementation Specification revision shall specify a version number, which enables interacting clients and servers to agree on which version of the specification they are conforming to. A version number shall contain three non-negative integers separated by decimal points, in the form "x.y.z". The integers y and z shall not exceed 99. "

Since MapServer does not support any versions of any specifications which break these conditions.

[dmorissette]

WMS 1.0.0 used to be more open on the version format, that's probably why the current implementation is more relaxed:

6.1 Version Numbering and Negotiation

6.1.1 Version Number Form

The specification version number contains up to three positive integers, 
separated by decimal points. The following forms are possible: "x", "x.y", 
or "x.y.z".

The current implementation treats ".1" as 0.1, and "1." as "1.0" which is an acceptable behavior to me even if it's not not strictly *enforcing* the text of the OWS common.

My vote goes to leaving things as is unless there is a CITE test that we fail because of that.

[tomkralidis]

Yet another new (OWS Common) vs old "common approaches" issue. We can leave this for now, but at least we have this on record.

Having said this, I'll probably make a tighter msOWSCommonParseVersionString in mapowscommon.c if/when required, given the SOS support and in general on how OGC services are moving w.r.t. OWS Common.

[dmorissette]

Please no, we don't need a separate one.

My point is simply that being more strict than we are today in this specific case would bring no value. The current msOWSParseVersionString() makes enough checks to make sure that any reasonable input values results in a valid request response, and there is no known possibility that feeding junk through the VERSION param can crash MapServer or result in unpredictable behavior. Adding more validation would be purely useless overhead as fas as MapServer is concerned.

Let's leave it up to CITE to make tests to sTrIcTlY eNfOrCe the text of the specs. ;)