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

Improve error message when OverpassQL query mistakenly specifies [out:xml] #1142

Merged
merged 1 commit into from
Aug 13, 2024

Conversation

jake-low
Copy link
Contributor

Re-opening #1140 under a new branch in the main repo (rather than from my fork).


Fixes maproulette/maproulette3#1908

I've made two logical changes here, which can be separated if necessary.

  1. Checking the Content-Type returned by the Overpass API; if it's set to something other than application/json, mark the challenge as having failed to build, with a specific user-facing error string advising them to use [out:json]. overpass-api.de returns Content-Type: application/json for [out:json] queries, and application/osm3s+xml for [out:xml] queries. I believe this change is low risk.

  2. Made the query rewriting code more robust, eliminating some of the currently known pitfalls. Previously rewriting would produce syntactically invalid queries (and thus confusing error messages) if [out:foo] was present but was not the first directive, or if [timeout:###] was present but was not the first directive (or was but was followed by [out:foo]). The previous code was also sensitive to otherwise-insignificant whitespace. The new code isn't perfect, but it is somewhat more robust. It will handle any combination of query settings, provided that they all appear together on the first line of the query.

    I believe this code will have equivalent behavior to the old code on all queries that are currently working. However, this part of the change is higher risk, because if any queries that work today are mishandled by my new implementation, existing challenges may break when their tasks are rebuilt (and since queries cannot be edited once a challenge is successfully created, this will force users to recreate the challenge, which would be frustrating). So the benefits of this change should be weighed against those risks. If you'd prefer I am happy to remove this change from the PR.

I'm not super familiar with Scala so please also let me know if there are better idioms that I should use or other style problems.

Screenshot of what the new error looks like in the UI:

image

@jake-low jake-low requested a review from ljdelight August 12, 2024 20:07
Copy link

sonarcloud bot commented Aug 12, 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. I tested it on staging with a few different queries and it worked as expected. It's a good step forward for folks that attempt xml format.

@ljdelight ljdelight merged commit 208f358 into main Aug 13, 2024
11 checks passed
@ljdelight ljdelight deleted the jlow/overpassql-xml-error-message branch August 13, 2024 21:16
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.

Warn user if [out:xml] is in overpass query
2 participants