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

Fix edge case in OverpassQL query rewriting #1144

Merged
merged 1 commit into from
Aug 17, 2024

Conversation

jake-low
Copy link
Contributor

Fixes maproulette/maproulette3#2414

My recent changes to the query rewriting logic had broken the (perfectly valid) query in the linked issue. The problem is that query has settings formatted over multiple lines. I noted in a comment that rewriteQuery wouldn't understand these settings, but what I didn't think about was that this would mean it would prepend a second set of settings, and having more than one settings statement is a syntax error in OverpassQL.

The fix I'm proposing here is to check if there appear to be any settings given anywhere in the query, using a crude String.contains check that should have no false negatives. If there are, rewriteQuery will return the query unmodified, and only if there aren't will it prepend the default [out:json] and timeout settings.

I'd appreciate a careful review of this. The list of OverpassQL settings is here and is what I based the settings array in the PR on.

An assumption I'm making with this code that should be reviewed is that an OverpassQL setting starts with [ + a setting name + :, e.g. [out:json]. OverpassQL query expressions often also contain [, e.g. way[highway = motorway];, and they may also contain :, e.g. way["piste:type" = "advanced"]; or node[amenity = parking](around:100);. However, I believe they never contain [ + a bare string + :, unless that entire sequence appeared inside a quoted string. I'm only searching for a list of known settings names, not \[.*:, so the odds of this triggering a false positive seem very low. Therefore, I believe that the check on line 801 should always be true if the query contains any settings, and additionally should almost never be true if the query does not contain any settings (something like node[name ~ "[out:xml]"] would trigger a false positive but that's a pretty contrived edge case IMO).

Feedback or suggested testcases are welcome.

@jake-low jake-low force-pushed the jlow/fix-overpasslq-xml-parsing branch from 64f28c9 to d9fc804 Compare August 16, 2024 17:37
Copy link

sonarcloud bot commented Aug 16, 2024

Copy link
Contributor

@ljdelight ljdelight left a comment

Choose a reason for hiding this comment

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

LGTM. The items I tested last time still pass, and the user-provided failed queries now pass. In the near future we can simplify this a bit to avoid trying to implement an overpassql parser, but it's good enough to avoid basic issues and improves the errors provided to users.

@ljdelight ljdelight merged commit ba1f32e into main Aug 17, 2024
11 checks passed
@ljdelight ljdelight deleted the jlow/fix-overpasslq-xml-parsing branch August 17, 2024 04:00
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.

Task Rebuild ends with "bad request" error
2 participants