JSON output nodes, ways, relations, map

[mmd-osm]

This is a follow up for #2221, this time using jbuilder. To avoid large scale changes to routes.rb and test cases, I added some method in the respective controllers to set a default output format "XML".

---

Mandatory motivation slide from API 0.7; or, The Whale, by Matt Amos:

[gravitystorm]

This looks great! Just a few comments from me based on an initial look through the PR. I plan to look at this in more detail in the next few weeks.

[mmd-osm]

Just one more comment regarding the "visible" flag - this is not present in the original Overpass JSON format. To keep the /map result in line with this format, I didn't include a visible attribute for visible objects (which all objects in the /map call are).

Now, because we also need to support history, some objects may be invisible. Only for those objects an additional "visible": false attribute is added. This is in line with what CGImap does atm.

If this turns out to be too confusing, we could still review/change this part.

[mmd-osm]

Meanwhile, a first version of a JSON parsing logic has been merged into iD. Once enabled, we're seeing significant speed ups by using JSON, like anticipated back in 2013. As an example > 10s loading + rendering time on XML vs. 3s using JSON.

I should have mentioned that those runtimes were measured on a cgimap backend. The goal of this PR is to have feature parity only. The Rails code will not be used in production later on.

[gravitystorm]

Again, all looks good to me, just a couple of minor points inline.

The bigger question is around which methods are covered by this PR, and which methods aren't. For example, I think the set_default_request_format would be better applied to every api method, but you only have it for a few (e.g. before_action :set_default_request_format, :except => [:create, :update, :delete]). Can you talk more about that?

[mmd-osm]

I think the set_default_request_format would be better applied to every api method, but you only have it for a few (e.g. before_action :set_default_request_format, :except => [:create, :update, :delete]). Can you talk more about that?

Yes, that's also intentional. On the one hand, I only wanted to include those methods for which we have some test coverage. On the other hand, some of those methods return plain text, rather than XML. I felt like overriding request.format could potentially introduce some havoc in the future, so I deliberately excluded those methods which are not in scope right now.

[gravitystorm]

On the other hand, some of those methods return plain text, rather than XML.

So that's a big mistake in the API design that I don't want us to make again! I would like to see all json requests getting a json response (and of course fix the plain-text responses for XML requests in 0.7).

If I understand this PR correctly, it adds json endpoints for only a subset of the API, and only for GET requests, is that correct? Do you plan to cover the rest of the API methods in followup pull requests, like changeset upload, and other controllers like traces etc?

[mmd-osm]

If I understand this PR correctly, it adds json endpoints for only a subset of the API, and only for GET requests, is that correct

Yes, that's correct. The only goal here was to have feature parity with cgimap. We don't have JSON support for changeset upload on cgimap that's why it's not included here. Traces aren't even implemented in cgimap at all so that's also out of scope.

There's nothing keeping us from adding more JSON support for other endpoints as well over time, if there's some popular demand for it.

[gravitystorm]

Sorry that I didn't get to finish the review this week, this is still at the top of my todo list.

[mmd-osm]

Thanks for the update. By the way, I'm planning to release I just released a new CGImap version 0.8.0 soon, along with a new PPA file for production that includes json support. Do think you could finish the review in the next couple of weeks maybe?

[gravitystorm]

Thanks for your patience - today was the first day for a few weeks that I've been able to spin up the code and poke around with it locally (rather than just reviewing via the website). I wanted to check a few things in the output before approval.

Everything looks fine to me, so I'm going to merge this. In the (unlikely) event that there's a showstopping bug in the format that none of us have spotted, then I think we should reserve the right to make changes over the next month, even if that breaks compatibility. After that, we'll reserve breaking changes for api version increments, as normal.

[tomhughes]

Ah... I would have liked to have given that the once over before we merged it given that some of it is pretty tricky stuff I think.

[tomhughes]

I assume @gravitystorm has looked at the actual JSON being generated and is happy with that so I'm not going to do into that in any detail, and the basics of the changes to the controllers mostly look sensible enough.

My real concerns are around set_default_request_format both from the point of view of having to call it in all the controllers and also the actual code that is in it - there really ought to be a better solution and I was planning to do some investigation of the problem but it will require some serious deep diving in the rails code I suspect.

At the very least I want to go through what is there with a fine toothed comb and make sure I understand what it is doing before we think about deploying this.

[gravitystorm]

Ah... I would have liked to have given that the once over before we merged it given that some of it is pretty tricky stuff I think.

Ah, sorry about that! I'm happy for you to revert the merge and reopen the PR until you've had the chance to review it.

[mmd-osm]

there really ought to be a better solution and I was planning to do some investigation of the problem but it will require some serious deep diving in the rails code I suspect.

Here are some of the challanges I was facing:

• Ensure backwards compatibility for clients sending broken Accept headers (i.e. JOSM).
• Ensure backwards compatibility for clients that are not requesting any particular format (/)
• In general, XML is always more important than JSON (also backwards compatibility)
• Avoid large scale changes to existing unit tests (setting some default format on API level would not work)
• URL Path suffix is more important than Accept header

We have test cases covering all of those special cases.

I haven't found any approach to achieve those goals without additional logic outside of the Rails framework. People reported a few issues re. Accept header processing on the Rails repo. Hopefully those somewhat more fragile bits of the framework won't bite us anymore now.

[tomhughes]

So have you completely ignored the rails processing and done your own from scratch? I'm not sure that isn't even more scary... At the very least it means I now have to read and understand the RFC and check it's right :-(

Can you explain more about the JOSM issue? Does it only affect old versions or does it still affect the current version? Has it been reported?

Large scale changes to the tests may be annoying, but avoiding them is not a good reason to use a suboptimal solution to something - sometimes refactoring is the right thing to do.

All the other points seem reasonable though I don't know yet what that actually means in terms of our options. I hope to investigate over the weekend.

[mmd-osm]

So have you completely ignored the rails processing and done your own from scratch?

I wouldn't call it doing it all from scratch. I'm reusing the same MIME parsing logic the framework also uses (this includes handling of the "q=" parameter) and set a default format in case of XML, JSON or */* for selected methods only.

The downside to this approach is that we need to parse the Accept header twice. I haven't found any option to do this only once, and adjust the format derivation logic within the framework itself according to our needs.

Can you explain more about the JOSM issue? Does it only affect old versions or does it still affect the current version? Has it been reported?

Sure. What they are currently sending is the following:

text/html, image/gif, image/jpeg, *; q=.2, */*; q=.2

There are a number of issues here:

• * is not a valid mime type, the spec requires <MIME_type>/<MIME_subtype>
• The extra space after the semicolon is not permitted
• The number format in q=.2 is invalid, that should be q=0.2

As the rails parser is very strict, we need to have some fallback logic in place.

I haven't reported those yet. JOSM ticket: https://josm.openstreetmap.de/ticket/18812
Given that old versions will linger around for a longish time, we'd need to deal with this in any case.

but avoiding them is not a good reason to use a suboptimal solution to something

I think this would only affect the initial part of the set_default_request_format method, where neither an Accept header, not an url suffix were set (as in the case of unit tests only).

      accept_header = request.headers["HTTP_ACCEPT"]
      if accept_header.nil?
        # e.g. unit tests don't set an Accept: header by default, force XML in this case
        request.format = "xml"
        return
      end

[tomhughes]

I've done a bit of a rework in aaf9d15 which is partly just making it more idiomatic ruby and partly slightly changing the way it interacts with rails.

The big issues requiring us to take so much control are (a) the JOSM issue and (b) the fact that rails doesn't really have a way to set a default format as such - it has a hardwired default of :js for XHR requests and :html for everything else. In fact that second one is the real problem because it we could set a default of :xml then it would be used when the JOSM accept failed to parse...

[simon04]

The issue has been addressed in JOSM:

https://josm.openstreetmap.de/changeset/15968/josm

fix #18812 - HttpClient: specify Accept=/ to prevent Java from adding Accept=text/html, image/gif, image/jpeg, *; q=.2, /; q=.2

https://josm.openstreetmap.de/changeset/15969/josm

see #18812 - OsmApi, OsmServerReader, NameFinder: specify Accept=application/xml, /;q=0.8

[mmd-osm]

@tomhughes : thanks a lot for the review. I'm happy that my initial approach wasn't completely off and I haven't missed anything really obvious in the Rails framework.

@simon04 : thanks a lot for the quick fix! While going through your changes, I noticed that CGimap wasn't handling application/xml as expected. This triggered a follow on patch in zerebubuth/openstreetmap-cgimap#221.

[PeWu]

FYI, this change broke osm-history: PeWu/osm-history#13

[mmd-osm]

@PeWu : as you wrote in the issue, you are simply sending the wrong Accept header:

"Unfortunately, osm-history sends an accept: application/json header"

By the way, JOSM did not have this issue. We have implemented a special case handling to bypass the issue.

[gravitystorm]

@PeWu : as you wrote in the issue, you are simply sending the wrong Accept header:

I suspect that there could be other applications, mainly nodejs ones, that were sending application/json headers without really meaning it (e.g. the http request library they are using does that by default, but the developer was expecting only an XML response since that's the only thing we offered).

I think that's just something each developer will have to adjust to though, I can't imagine a clean way of handling these situations.

[mmd-osm]

If people are unable to override their Accept header, there's also the option to append an .xml suffix to the URL (unless application/json is the only permitted format according to their Accept header).

Some examples:

• https://api.openstreetmap.org/api/0.6/way/4972913.xml
• https://api.openstreetmap.org/api/0.6/way/4972913/history.xml
• curl -H "Accept: application/json, */*" https://api.openstreetmap.org/api/0.6/way/4972913.xml --> returns XML

[tomhughes]

In this case they probably want to convert to JSON anyway - currently they are getting the XML and turning it into JSON immediately!

[anatoliegolovco]

Are there plans to implement json response for following endpoint: /api/0.6/changesets ?
We use it for activity monitoring.

[tomhughes]

In the general sense that we plan to make the entire API available? Sure. There are no concrete plans that I know of however.

[gravitystorm]

In the general sense that we plan to make the entire API available? Sure. There are no concrete plans that I know of however.

Yep, it just needs volunteers.

[prusswan]

Just to confirm, this only adds the possibility of JSON responses from those endpoints? Looks like iD is still requesting XML due to openstreetmap/iD#3765

[mmd-osm]

Newest iD v3 uses json.

[mmd-osm]

iD 2.18.3 that was deployed to production yesterday is the first release to use JSON.