Skip to content
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

Use Case Insensitive Filters for SLD Output #5980

Merged
merged 14 commits into from Jan 24, 2020

Conversation

geographika
Copy link
Member

@geographika geographika commented Jan 19, 2020

See issue at #5526.
This pull request makes checks for operators case-insensitive so "AND", "and", and "Annd" in a Mapfile expression will all produce an SLD filter.

The functions strcasecmp, strcasestr, and toupper are used to ensure this.

A new test Mapfile has been added to ensure these filters are created in each case.
TODO - add test for brackets e.g. AND(

@geographika
Copy link
Member Author

geographika commented Jan 19, 2020

There are some memory leaks showing up in Travis. See https://travis-ci.org/mapserver/mapserver/jobs/639062323?utm_source=github_status
I'm not sure if this is due to my changes, or due to previously untested codepaths now being tested.

For example:

Direct leak of 48 byte(s) in 1 object(s) allocated from:

    #0 0x4bb533 in __interceptor_malloc /local/mnt/workspace/tmp/ubuntu_rel/llvm/utils/release/final/llvm.src/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:67:3
    #1 0x2b28ecb6e84f in FLTCreateFilterEncodingNode /home/travis/build/mapserver/mapserver/mapogcfilter.c:970:25
    #2 0x2b28ecd088b0 in BuildExpressionTree /home/travis/build/mapserver/mapserver/mapogcsld.c:4835:18
    #3 0x2b28ecd0afb4 in msSLDParseLogicalExpression /home/travis/build/mapserver/mapserver/mapogcsld.c:4968:12
    #4 0x2b28ecd0df4c in msSLDGetFilter /home/travis/build/mapserver/mapserver/mapogcsld.c:5166:19
    #5 0x2b28eccfd066 in msSLDGenerateUserStyle /home/travis/build/mapserver/mapserver/mapogcsld.c:4150:21
    #6 0x2b28eccefbb1 in msSLDGenerateSLDLayer /home/travis/build/mapserver/mapserver/mapogcsld.c:4314:9
    #7 0x2b28eccee7d3 in msSLDGenerateSLD /home/travis/build/mapserver/mapserver/mapogcsld.c:3196:18

@rouault
Copy link
Contributor

rouault commented Jan 21, 2020

I'm not sure if this is due to my changes, or due to previously untested codepaths now being tested.

Likely the later. I believe that adding CPLDestroyXMLNode(psNode) at https://github.com/mapserver/mapserver/blob/e04350e23843cf3c7749aeac25cc7c63fe7e1180/mapogcsld.c#L5000 should fix it

@geographika
Copy link
Member Author

geographika commented Jan 21, 2020

@rouault - thanks for the hint. I added in 547c5b2 but leaks still reported.

Also failing to build in Appveyor, is there some casting that needs to be done?

C:\projects\mapserver\mapogcsld.c(4999): error C2220: warning treated as error - no 'object' file generated [C:\projects\mapserver\build\mapserver.vcxproj]
C:\projects\mapserver\mapogcsld.c(4999): warning C4133: 'function': incompatible types - from 'FilterEncodingNode *' to 'CPLXMLNode *' [C:\projects\mapserver\build\mapserver.vcxproj]

I removed the erroneous free(pszTmp); I added as per your comment.

@rouault
Copy link
Contributor

rouault commented Jan 21, 2020

is there some casting that needs to be done?

ah my mistake... FLTFreeFilterEncodingNode(psNode); should do it hopefully

@geographika
Copy link
Member Author

geographika commented Jan 22, 2020

@rouault - thanks for that - updated. I also needed to free the left and right expression variables as at 79f66b6 and now all reported leaks are gone.

The existing parsing used in the code looks like it could have some edge cases that would cause it to fail e.g. if an expression contained NOR( it would be picked up as an OR (although this situation may not be possible). There is also an issue (present in the existing code) where attribute values aren't trimmed - I can open a separate issue about this.

This pull request fixes issues around case-sensitivity (previously no filters were created for these), adds some tests for the SLD creation functions, fixes some comment typos, and fixes a couple of memory leaks.

@jbo-ads - you have pull request #5832 which affects the same file so may want to be aware of this update. I had a quick check and there don't seem to be any merge conflicts.

@geographika geographika changed the title [WIP] Use Case Insensitive Filters for SLD Output Use Case Insensitive Filters for SLD Output Jan 22, 2020
@geographika geographika requested a review from rouault Jan 22, 2020
mapogcsld.c Outdated
@@ -4993,8 +4996,8 @@ char *msSLDParseLogicalExpression(char *pszExpression, const char *pszWfsFilter)

free(pszFLTExpression);
pszFLTExpression = pszTmp;
FLTFreeFilterEncodingNode(psNode);
Copy link
Contributor

@rouault rouault Jan 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be moved just after the } of line 5000, so that psNode is also freed when != NULL but when pszFLTExpression is NULL

Copy link
Contributor

@rouault rouault left a comment

LGTM with the comment I made. But I'm not super familiar with this code

@rouault rouault merged commit 8427420 into MapServer:master Jan 24, 2020
@geographika
Copy link
Member Author

geographika commented Jan 24, 2020

@rouault - many thanks for the review and explanations

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants