Opened 17 years ago

Closed 14 years ago

Last modified 14 years ago

#2123 closed defect (fixed)

Layer filter does not work when values contain quotes

Reported by: unicoletti Owned by: sdlime
Priority: normal Milestone: 5.6 release
Component: MapServer C Library Version: svn-trunk (development)
Severity: normal Keywords:
Cc: dmorissette, aboudreault

Description

Setting a filter on a shp layer like the following :

'[NAME]'=='some name'

where the NAME attribute can contain values with quotes (single quote in my case) causes the following error (in Java mapscript):

msEvalExpression: Expression parser error. Failed to parse expression;msyyparse():

This seems to be a know problem as it is even documented here (towards the end):

http://mapserver.gis.umn.edu/docs/howto/msexpressions

Attachments (4)

issue2123.patch (2.5 KB ) - added by unicoletti 17 years ago.
Proposed patch: works for me with shape files
issue2123-maputil.patch (789 bytes ) - added by unicoletti 17 years ago.
In case the quote is in the value stored in the shapefile
issue2123.2.patch (2.5 KB ) - added by unicoletti 17 years ago.
Fix memory leaks because of strdup
issue2123-mapparser.patch (2.6 KB ) - added by unicoletti 17 years ago.
Fix memory leaks because of strdup (forget previous, is an error)

Download all attachments as: .zip

Change History (25)

comment:1 by unicoletti, 17 years ago

I have investigated on this issue and found that the cgi works (without raising an error) because it silently discards a parsing error that occurs at line 363 of maputil.c. Both (mapscript and cgi) will fail when the right hand expression contains a single quote.

I'm looking into mapparser.y if we can introduce an escape character.

by unicoletti, 17 years ago

Attachment: issue2123.patch added

Proposed patch: works for me with shape files

comment:2 by unicoletti, 17 years ago

Steve could you look at it? This is my first attempt at flex so it could be that I have made mistakes.

Inspired by:

http://www.gnu.org/software/flex/manual/html_chapter/flex_11.html

by unicoletti, 17 years ago

Attachment: issue2123-maputil.patch added

In case the quote is in the value stored in the shapefile

by unicoletti, 17 years ago

Attachment: issue2123.2.patch added

Fix memory leaks because of strdup

by unicoletti, 17 years ago

Attachment: issue2123-mapparser.patch added

Fix memory leaks because of strdup (forget previous, is an error)

comment:3 by unicoletti, 17 years ago

double free error becaus of free at maputil.c around line 360. Please remove it:

if (status != 0) {

free(tmpstr); <----- REMOVE THIS FREE msSetError(MS_PARSEERR, "Failed to parse expression", "msEvalExpression"); return MS_FALSE;

}

comment:4 by sdlime, 17 years ago

Why not just write:

FILTER '"[NAME]" = "some name"'

instead? The outer single quotes are stripped off at parse time. Ultimately I think escaped chars is a good idea though. Will check this bug out over the weekend...

Steve

in reply to:  4 ; comment:5 by unicoletti, 17 years ago

Replying to sdlime:

Why not just write:

FILTER '"[NAME]" = "some name"'

instead? The outer single quotes are stripped off at parse time.

Because valid matches will be discarded as well. For example:

' "[CITY]" == "Arqua' Petrarca" '

will never match even if in fact there is a record with such an attribute and value.

You reasoning is probably justified because there is a big difference in the way that the cgi and mapscript deal with parse errors raised in msEvalExpression: while the cgi silently discards the parse error caused by the filter failure, mapscript reads the error from the stack, raises an exception and the map generation aborts.

comment:6 by unicoletti, 17 years ago

Resolution: fixed
Status: newclosed

Fixed in revision 6277.

comment:7 by dmorissette, 17 years ago

Cc: dmorissette added

I came across this by accident, this is a very nice enhancement and I think it should be documented somewhere for the users (MIGRATION_GUIDE.TXT, mapfile ref, etc) instead of just being silently marked fixed.

Note for those reading this ticket and wondering (like me) how the escape works, from a browse of the code it seems to use C-like escaping for "
", "\"" and "\'" e.g.

"[CITY]" == "Arqua\' Petrarca"

comment:8 by dmorissette, 17 years ago

Milestone: 5.0 release

comment:9 by unicoletti, 17 years ago

I'm going to modify the relevant page on the website.

in reply to:  5 comment:10 by sdlime, 17 years ago

Replying to unicoletti:

Replying to sdlime:

Why not just write:

FILTER '"[NAME]" = "some name"'

instead? The outer single quotes are stripped off at parse time.

Because valid matches will be discarded as well. For example:

' "[CITY]" == "Arqua' Petrarca" '

will never match even if in fact there is a record with such an attribute and value.

You reasoning is probably justified because there is a big difference in the way that the cgi and mapscript deal with parse errors raised in msEvalExpression: while the cgi silently discards the parse error caused by the filter failure, mapscript reads the error from the stack, raises an exception and the map generation aborts.

I'll have to work up a test dataset and try it. I have a lake names here like O'Brien and I was pretty sure they worked fine with an expression like:

("[LAKENAME]" = "O'Brien")

Steve

comment:11 by sdlime, 17 years ago

Also, does expression writing re-escape strings so msSaveMap will work ok?

Steve

in reply to:  11 comment:12 by unicoletti, 17 years ago

Resolution: fixed
Status: closedreopened

Replying to sdlime:

Also, does expression writing re-escape strings so msSaveMap will work ok?

I haven't tested, in fact I didn't even think about it. Looking into it asap.

comment:13 by unicoletti, 17 years ago

Resolution: fixed
Status: reopenedclosed

Looked at the code and it seems that msSaveMap should work because the escaping code is done each time the espression is evaluated and not when the map file is read. The unescaped original expression string is always held in:

expression->string

and strdupped in msEvalExpression each time. expression->string is then used by writeExpression

comment:14 by sdlime, 17 years ago

I'm wondering if we might consider unescape/escape functions in the future so you have to do that only once. Unescape when tokenizing and escaping when writing.

Steve

comment:15 by sdlime, 14 years ago

Resolution: fixed
Status: closedreopened

comment:16 by sdlime, 14 years ago

Milestone: 5.0 release5.6 release
Version: svn-trunk (development)

The solution here limits expression tokens to 1024 and that's showing up on the mailing list as a problem for a number of users with long expressions using the IN operator. No error occur, it's just that not all features that should be drawn are drawn. Depending on the expression the effects could be very subtle, just a few features not being drawn.

Would make several users happy if this were fixed somehow.

Steve

comment:17 by sdlime, 14 years ago

Cc: aboudreault added

Alan: Is this one you'd have time to look at?

Steve

comment:18 by aboudreault, 14 years ago

Resolution: fixed
Status: reopenedclosed

Fixed and committed in r9560.

comment:19 by sdlime, 14 years ago

Alan: Sweet. I can verify the fix works based on a test case I have here. I'm kinda surprised the right hand token in an expression like this:

'foo' in 'foo,bar'

even gets into that processing given that 'foo,bar' contains no internal quotes. I would have thought the rule:

<EXPRESSION_STRING>\"[\"]*\"i|\'[\']*\'i

would have caught those, although I have no idea what the i's are for in that expression.

Steve

comment:20 by dmorissette, 14 years ago

Here is the regex again without Trac formatting:

 <EXPRESSION_STRING>\"[^\"]*\"i|\'[^\']*\'i

comment:21 by sdlime, 14 years ago

You can ignore my last comment. The i is obviously for case insensitive strings. I take it we don't use those in the context of logical expressions...

Steve

Note: See TracTickets for help on using tickets.