I've made two logical changes here, which can be separated if necessary.
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.
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:
Re-opening #1140 under a new branch in the main repo (rather than from my fork).
Fixes https://github.com/maproulette/maproulette3/issues/1908
I've made two logical changes here, which can be separated if necessary.
Content-Type
returned by the Overpass API; if it's set to something other thanapplication/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 returnsContent-Type: application/json
for[out:json]
queries, andapplication/osm3s+xml
for[out:xml]
queries. I believe this change is low risk.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: