JSON #153
JSON #153
Conversation
|
LGTM! The type ambiguity in |
|
While the compilation error is fixed, there seems to be some other issue. I tried The following snipped in routes.cpp looks a bit strange. Somehow, a path 'map.json' doesn't get resolved correctly here (at least not according to the description in the comment). Maybe this could be replaced by: /**
* figures out the mime type from the path specification, e.g: a resource ending
* in .xml should be text/xml, .json should be text/json, etc...
*/
pair<string, mime::type> resource_mime_type(const string &path) {
std::size_t json_found = path.rfind(".json");
if (json_found != string::npos && json_found == path.length() - 5) {
return make_pair(path.substr(0, json_found), mime::text_json);
}
return make_pair(path, mime::unspecified_type);
}The other issue is that the code doesn't really work when the client requests gzip compression. Also, the http header must contain |
|
The other question would be, if we should make the format compatible with the Overpass JSON format http://overpass-api.de/output_formats.html#json - this is already in widespread use, but would require some changes to the current cgimap implementation (which has never been released yet to my knowledge). Example: http://overpass-turbo.eu/s/ADD |
|
Yes the need to decide on what format exactly to return (along with the rails code not being able to do JSON output) is one reason why this has never been enabled. |
|
In the meantime I have found out why the gzip compressed response didn't work. There was a missing out->close(), which is getting called in case of xml but not for json. Fix could be something along those lines: diff --git a/include/cgimap/json_writer.hpp b/include/cgimap/json_writer.hpp
index f1b7d1c..07e7f2a 100644
--- a/include/cgimap/json_writer.hpp
+++ b/include/cgimap/json_writer.hpp
@@ -42,6 +42,7 @@ private:
// PIMPL idiom
struct pimpl_;
pimpl_ *pimpl;
+ boost::shared_ptr<output_buffer> out;
};
#endif /* JSON_WRITER_HPP */
diff --git a/src/json_writer.cpp b/src/json_writer.cpp
index 9b4b30f..0a2bfe7 100644
--- a/src/json_writer.cpp
+++ b/src/json_writer.cpp
@@ -33,7 +33,7 @@ static void wrap_write(void *context, const char *str, unsigned int len) {
}
json_writer::json_writer(boost::shared_ptr<output_buffer> &out, bool indent)
- : pimpl(new pimpl_()) {
+ : pimpl(new pimpl_()), out(out) {
#ifdef HAVE_YAJL2
pimpl->gen = yajl_gen_alloc(NULL);
@@ -71,6 +71,17 @@ json_writer::~json_writer() throw() {
yajl_gen_clear(pimpl->gen);
yajl_gen_free(pimpl->gen);
delete pimpl;
+
+ if (out != nullptr) {
+ try {
+ out->close();
+ } catch (...) {
+ // don't do anything here or we risk FUBARing the entire program.
+ // it might not be possible to end the document because the output
+ // stream went away. if so, then there is nothing to do but try
+ // and reclaim the extra memory.
+ }
+ }
}
void json_writer::start_object() { yajl_gen_map_open(pimpl->gen); } |
We could cc: a number of folks here, or post something on osm dev for some community feedback. And yes, there's sotm18. |
|
By the way, the reason I started playing with this is https://youtu.be/Jn5VWd7C6_g?t=37m49s which is a lightning talk given at SOTM yesterday. |
|
I thought so already, Parsing XML line by line seems like a clever hack, but to me this is really a big no-go. It relies on undocumented line breaks and would force the API to always send those line breaks in the future. If someone ever decides to get rid of those lines breaks (which is totally ok from an XML pov), that new OSM XML parser would immediately break. So yes, consuming JSON right from the start seems like the preferred approach. I'm a bit sceptic though, if optimizing a 25% portion of the total runtime will have a noticeable impact on perceived performance... but that's another story. We'll find that out soon I'd guess. Also see my comments here: openstreetmap/iD#5180 (comment), kepta/idly#4 |
|
I implemented the Overpass API JSON format now, as well as adding the fixes described above to a new pull request: tomhughes#2 - it's probably one of the most widespread JSON representations for raw OSM data, and has been around for at least 5 years now. Once you merge this pull request into your branch, it should automatically show up in this pull request here. I didn't want to create yet another pull request for the JSON topic in this repo. Let's see if this works out somehow. I also tested the result via http://tyrasd.github.io/osmtogeojson/ by @tyrasd |
|
Let's loop in a few people to get some initial feedback on this topic. Question: Should we consider the Overpass JSON format also for the OSM API JSON output? Reasoning: Overpass JSON format is around for several years now and supported by a number of tools already. Every overpass turbo user should be familiar with this format. It's also the default that gets shown on the data tab along with the Format specification is available here: http://overpass-api.de/output_formats.html#json In a first step, we're reaching out to developers that are in one way or another involved in OSM editing apps. Please feel free to forward this on to other people that might be interested in this discussion as well. //cc: @zerebubuth , @pnorman , @bhousel, @Zverik, @don-vip, @simonpoole, @woodpeck , @drolbr Please use the respective Github voting button, if you think it would be a good idea to also implement this format for the OSM API JSON output If you think this format is not suitable, please consider leaving a comment below. Thanks for your feedback! |
|
Cool! Tagging @kepta who gave the talk about the XML parser and @thomas-hervey who just wrote the OSM Notes parser assuming we'd get XML. On the iD side, we'd love to consume JSON instead of XML, and it would result in a real performance benefit (which @kepta mentioned in his talk). @mmd-osm if it would help, I could provide a list of API calls that iD uses. That would be our priority list. Overpass-style JSON for the OSM features looks ok to me. iD would ideally consume JSON for things like Notes and Users too, and even the capabilities call. 😄 |
|
No strong feelings about the choice as the benefits as the subject matter tends to be a) specific to the in browser editors that have traditionally downloaded on panning (yes I know that you can so that in JOSM too) and b) other benefits from using JSON are likely miniscule for non-JS apps. So yes I think we should do this because the benefit of silencing some of the commentary from the sidelines and make things somewhat easier for JS devs, but outside of that .... |
|
We wouldn't want to have two JSON formats for OSM data, so I'm strongly in favour of Overpass JSON format. I have used it in my tools, and it's pretty good. |
|
We don't support JSON format in JOSM yet (see 16546) but I agree with Zverik :) |
|
If JSON output format is currently borken and going to change anyway, should it be minified by setting |
|
Thanks to everyone for your feedback so far, much appreciated! @bhousel :
The focus at the moment is to support all calls returning OSM XML format, as currently implemented by cgimap. This includes:
I'm not sure if the Apache forwarding rules will include them all right from the start, but I'm pretty sure we will support at least /map once this available. There's already a first idea how "changset metadata downloads, including discussions" could look like: /api/0.6/changeset/{{changeset id}}?include_discussion=true {
"version": "0.6",
"generator": "CGImap 0.6.1 (17051 ubuntu)",
"copyright": "OpenStreetMap and contributors",
"attribution": "http://www.openstreetmap.org/copyright",
"license": "http://opendatacommons.org/licenses/odbl/1-0/",
"elements": [
{
"type": "changeset",
"id": 1140,
"created_at": "2018-07-15T19:53:26Z",
"closed_at": "2018-07-15T20:58:03Z",
"open": false,
"user": "mmd2",
"uid": 1,
"minlat": -33.9133123,
"minlon": 6.9028606,
"maxlat": 49.2343776,
"maxlon": 151.1173123,
"comments_count": 3,
"tags": {
"comment": "test",
"created_by": "JOSM/1.5 (14031 SVN de)",
"source": "test"
},
"discussion": [
{
"date": "2018-08-02T19:48:14Z",
"uid": 1,
"user": "mmd2",
"text": "sdfdsaf"
},
{
"date": "2018-08-02T19:48:17Z",
"uid": 1,
"user": "mmd2",
"text": "asdcadcadsfadsf"
},
{
"date": "2018-08-02T19:48:47Z",
"uid": 1,
"user": "mmd2",
"text": "<html></html> {{{ \"\"\""
}
]
}
]
}
--
Other API calls, like notes and capabilities aren't implemented in cgimap but in the Rails port, so those are clearly out of scope atm. |
|
JOSM 14086+ supports Overpass API JSON format. |
|
@tomhughes : I have released a new cgimap version 0.6.2 just now with the following changes:
Code comparison: v0.6.1...v0.6.2 Like last time, I'd suggest to copy the following cgimap package over to the osmadmins cgimap PPA: https://launchpad.net/~mmd-osm/+archive/ubuntu/cgimap-062/+packages |
|
@mmd-osm great work! Work in progress in JOSM to support |
|
We won't be releasing 0.6.2, as there's too much concern out there, that Rails port lags behind cgimap features due to the lack of JSON support. This means that JSON support will be basically dead for the foreseeable future. |
|
@mmd-osm where has that concern been voiced? |
|
@simonpoole : it's in the operations issue, openstreetmap/operations#244 |
|
This decision is not cool for editor developers like me who spent effort to add JSON support... I'm really disappointed. |
|
I really tried my best to convince the Rails camp to run cgimap alongside the Rails port, and be just fine. In the end, it didn't matter to them. cgimap-ruby is seen by some as the go-to option to integrate cgimap code directly into Rails via some custom Gem. The downside is that the only guru familiar with cgimap-ruby, doesn't have the time to get it ready for production. As sad as it is, we're pretty much stuck here, and I have no idea on how to proceed otherwise. |
|
Look we all want to do it, we just want to do it properly. I for one don't have strong opinions on how it is done but I do think we shouldn't have multiple versions of the API that behave in different ways. |
|
Another attempt of bringing JSON to the Rails port has started in openstreetmap/openstreetmap-website#2221 As I have literally no relevant experience with Rails, we need everyone to help out! You can review code, add comments and suggestions, test it, report errors... |
|
Another follow up in openstreetmap/openstreetmap-website#2485 |
This gets the JSON output formatter building again, and enables the JSON output support in the travis builds.