New Mapfile Validation Rules

[geographika]

Following on from #6458 and the docs pull request here, there are a few validation rules to check/confirm:

1. Double check that the ANGLE keywords can have negative values. For example the STYLE ANGLE docs currently state clockwise angles should be calculated as positive values:

Angle, given in degrees, to rotate the symbol (counter clockwise).
Default is 0 (no rotation).  If you have an attribute that
specifies angles in a clockwise direction (compass direction), you
have to adjust the angle attribute values before they reach
MapServer  (360-ANGLE), as it is not possible to use a mathematical
expression for `ANGLE`.

2. There are several cases where the default values for objects would not pass validation e.g. STYLE MINSIZE, MINWIDTH, OUTLINEWIDTH. Docs would read as Default is 0. Must be greater than 0.
   Also CLUSTER BUFFER (defaults to 0 but must be > 0). Same for LAYER TOLERANCE.

3. Perhaps more problematic are when default values are written out to Mapfiles by MapServer, which would then produce invalid maps. E.g in writeLayer all the following default to -1, but then validation is set to > 0.

  writeNumber(stream, indent, "MINGEOWIDTH", -1, layer->mingeowidth);
  writeNumber(stream, indent, "MINSCALEDENOM", -1, layer->minscaledenom);
  writeNumber(stream, indent, "MINFEATURESIZE", -1, layer->minfeaturesize);

4. STYLE SYMBOL docs state the index starts at 1, but validation allows 0.

5. (nitpick) Minor inconsistency in validation for same property names - MAP MAXSIZE is >= 1 and LABEL MAXSIZE is > 0 (results in same validation as these are both integer values.

[sdlime]

For 3...

writeNumber(stream, indent, "MINGEOWIDTH", -1, layer->mingeowidth);

means that if layer->mingeowidth == -1 then don't write that property so that default value should never be output. We do that to avoid bloated output filled with useless settings. Are you seeing something different?

[sdlime]

For 4... There is always a default symbol 0 so the first symbol in a symbol set is 1, then 2 and so on. I think we're ok but the docs could probably be clarified. I'm wondering if all the "new in version x.x" is really necessary unless it pertains to the newest version - otherwise it just clutters the text.

[jratike80]

This reminds me that I have never understood why we do not have default symbol for points. Or if there is it does not render anything.

[sdlime]

This reminds me that I have never understood why we do not have default symbol for points. Or if there is it does not render anything.

There is a default but it but it's a single pixel so it would be easy to miss.

[jmckenna]

(bringing my points here from the closed pull request)

6. It would be good to include a line number in the error message; for example, a mapfile with 30 entries of WIDTH 0 returns only the following with map2img:

loadStyle(): General error message. Invalid WIDTH, must be greater than 0
loadStyle(): General error message. Invalid WIDTH, must be greater than 0 <br>

7. I think we should test for WIDTH >=0, as WIDTH 0 is a very common trick when using multiple STYLEs inside a CLASS.

[sdlime]

@geographika, is 2 kinda the same issue as 3?

[geographika]

@geographika, is 2 kinda the same issue as 3?

Yes, the only difference is that issue 2 would only be caused by user Mapfiles, whereas issue 3 could be caused by MapServer writing out a Mapfile.

[jratike80]

There is a default but it but it's a single pixel so it would be easy to miss.

But we document it in https://mapserver.org/mapfile/style.html#mapfile-style-symbol "If SYMBOL is not specified, the behaviour depends on the type of feature.
For points, nothing will be rendered."
From user's perspective it does not make big difference if point is not rendered or if it is impossible to see.

[sdlime]

@geographika, is 2 kinda the same issue as 3?

Yes, the only difference is that issue 2 would only be caused by user Mapfiles, whereas issue 3 could be caused by MapServer writing out a Mapfile.

Except I don't the issue 3 can happen via writing - unless I'm missing something... We avoid writing defaults.

@geographika, I would argue we shouldn't advertise defaults that just turn something off. For example, for something like layer MIBSCALEDENOM, the default is something nebulous like "not set" and the valid values are doubles greater than 1. There's no reason to let folks know what we're using internally for initialization. We should probably switch to a standard MS_VALUE_NOT_SET constant at some point to make this more clear.

[geographika]

Except I don't the issue 3 can happen via writing - unless I'm missing something... We avoid writing defaults.

@sdlime - sorry my mistake - I'd missed that in the writeNumber function it excluded defaults (which are often initialised to -1):

if(number == defaultNumber) return; /* don't output default */

[jmckenna]

8. MapScript's save() method for mapObj saves the mapfile but by default adds a new QUERYMAP object (and LEGEND, SCALEBAR) with SIZE -1 -1 for QUERYMAP :

     QUERYMAP
       SIZE -1 -1
       STATUS OFF
       STYLE HILITE
     END # QUERYMAP
   

that then fails on the QUERYMAP's SIZE with map2img:

getInteger(): Symbol definition error. Parsing error near (-1):(line 47)
getInteger(): Symbol definition error. Parsing error near (-1):(line 47) <br>

[sdlime]

8. MapScript's save() method for mapObj saves the mapfile but adds a new QUERYMAP object (and LEGEND, SCALEBAR) with SIZE -1 -1 for QUERYMAP

Well that's a PITA... I can/will suppress writing -1 -1 going forward but if we want to have as much backwards compatibility as possible then we'll have to allow it for now. @jmckenna, have you run into this with any other parameters w/two values?

[sdlime]

@geographika, for 1:

• map-level angle seems to work as expected with positive or negative values
• label-level angle seems to work as expected with positive or negative values
• since style-level angle documentation refers specifically to 0-360 I think we should limit from 0-360 if there are no objections

[geographika]

@sdlime - in order to allow consistency for the same keywords across objects is there the option of setting the following at parse time in mapfile.c? Any further code using style->angle would then be unaffected.

if(msyynumber < -360 || msyynumber > 360) return(MS_FAILURE);
style->angle = (double) msyynumber;
if (style->angle < 0) {
    style->angle = 360 - fabs(style->angle);
}

I guess a drawback would be when writing back out to a Mapfile ANGLE -30 would become ANGLE 330.

[sdlime]

We could, probably should try some tests first - maybe the docs are wrong? Can't avoid the drawback though.

[sdlime]

I think hit all of these. Please have a look. Note that I left style->angle at 0,360 for the moment. I added line numbers to the various misc errors...

[jmckenna]

new report

9. a valid MINSCALEDENOM 0 in a LAYER fails, such as:

   LAYER
     ...
     MINSCALEDENOM 0
     MAXSCALEDENOM 2539
     ...
   END # layer

with error message:

getDouble(): Symbol definition error. Parsing error near (0):(line 9281)
getDouble(): Symbol definition error. Parsing error near (0):(line 9281) <br>

[jmckenna]

8. MapScript's save() method for mapObj saves the mapfile but adds a new QUERYMAP object (and LEGEND, SCALEBAR) with SIZE -1 -1 for QUERYMAP

Well that's a PITA... I can/will suppress writing -1 -1 going forward but if we want to have as much backwards compatibility as possible then we'll have to allow it for now. @jmckenna, have you run into this with any other parameters w/two values?

Oops for some reason I didn't see this whole comment, at the time I only saw "that a PITA". Answer: this is the only time I faced it.

[geographika]

Thanks @sdlime - the issues raised here have all been addressed. Anything else arising can be opened in a new ticket.