[OGR] Problem with numbers in STYLEITEM AUTO labels

[dmorissette]

Daniel Morissette wrote:
> Frank Warmerdam wrote:
> 
>>When handling labels in msOGRLayerGetAutoStyle() the following logic appears:
>>
>>           if (poStylePart->GetType() == OGRSTCLabel)
>>           {
>>               OGRStyleLabel *poLabelStyle = (OGRStyleLabel*)poStylePart;
>>
>>               loadExpressionString(&(c->text),
>>                                    (char*)poLabelStyle->TextString(bIsNull));
>>
>>The TextString() method is returning the right value ... in this case "6.4",
>>but loadExpressionString() is failing because the value string "6.4" is 
returned
>>as type MS_NUMBER, not one of MS_STRING, MS_EXPRESSION or MS_REGEX.
>>
>>int loadExpressionString(expressionObj *exp, char *value)
>>{
>>   msyystate = 2; msyystring = value;
>>
>>   freeExpression(exp); // we're totally replacing the old expression so free 
then init to
>>start over
>>   // initExpression(exp);
>>
>>   if((exp->type = getSymbol(3, MS_STRING,MS_EXPRESSION,MS_REGEX)) == -1) 
return(-1);
>>
>>I was able to fix the problem locally by adding MS_NUMBER to the getSymbol()
>>call, but loadExpressionString() is "core" mapserver, not just for OGR's use
>>so I am not sure this is wise to change in general.
>>
>>Would it be OK to just have msOGRLayerGetAutoStyle() just set c->text->exp
>>to strdup(oLabelStyle->TextString(bIsNull)), avoiding any risk of the
>>loadExpressionString() doing parsing weirdness to the input value?  The values
>>should always be taken as a literal at this point anyways, right?
>>
> 
> 
> Hi Frank,
> 
> I'm scared that forcing the value of c->text->exp could break if
> something changes in the expression code.  We would also have to set the
> 'type' value as well at least, and who knows what else could break in
> the future if a change is made.
> 
> Accepting type MS_NUMBER would probably be safe for the rest of
> MapServer since number are constant expressions anyway just like
> strings, but that would still leave one potential issue: if someone has
> a label which says "LAYER" for instance (or something else that is a
> mapfile keyword) then you'll get MS_LAYER instead of MS_STRING and it
> would still fail.
> 
> I think a clean solution would be to force the label value to be seen as
> a string by enclosing it inside quotes, using something like:
> 
> loadExpressionString(&(c->text),
>          CPLSPrintf("\"%s\", (char*)poLabelStyle->TextString(bIsNull)));
> 
> I can implement and test this fix if you want.

[dmorissette]

I implemented the proposed fix (enclosing the string value inside quotes) and it 
works great in 3.6.3-dev

I'll apply the same fix in 3.7-dev as well.