openstreetmap / openstreetmap-website

The Rails application that powers OpenStreetMap
https://www.openstreetmap.org/
GNU General Public License v2.0
2.2k stars 911 forks source link

The response is /changeset/:id.json endpoint is different for Ruby API and CGImap #5047

Closed deevroman closed 2 months ago

deevroman commented 3 months ago

URL

https://api.openstreetmap.org/api/0.6/changeset/70.json

How to reproduce the issue?

CGImap returns in the response to /changeset/:id.json array of elements with a single element-meta information about changeset

I deployed the OSM API locally without CGImap, and the Ruby implementation immediately returns a changeset object in the response (probably based on the XML response)


CGImap also calls the bbox borders differently for the JSON response: minlat vs min_lat. In the case of an XML response, the results are the same.

Is this behavior worth documenting, or is it a bug that can still be fixed?

Screenshot(s) or anything else?

mmd-osm commented 3 months ago

Thank you for reporting the issue. I think it's a bug. I'm undecided if we should change the Rails or CGImap implementation. Usually, we say that Rails is the leading imlementation. However, since we're using the CGImap version in production already, we might have to adjust the Rails version. If noone is using the JSON version of this endpoint today, we might as well change the CGImap implementation.

Any opinions?

deevroman commented 3 months ago

Are there any projects forks osm.org what does the Ruby implementation use in production? I see that OpenHistoricalMap, OpenGeofiction use CGImap

https://api.openhistoricalmap.org/api/0.6/changeset/7.json https://www.opengeofiction.net/api/0.6/changeset/6.json

mmd-osm commented 3 months ago

I think if the main question is if there are any tools / editors out there which are using the CGImap json version today. If so, we probably need to change the Rails version to avoid breaking changes.

The other issue is that the https://api.openstreetmap.org/api/0.6/changesets.json endpoint + changeset.json use the same app/views/api/changesets/_changeset.json.jbuilder. This is getting a bit messy to fix.

We probably need some stats from the Apache log files to decide which option is better suited.

gravitystorm commented 3 months ago

I think we need to first decide what we want the output to be, and then figure out what is practical. For example, either one of them could be changed to include the output from the other, while retaining backwards compatibility, since the changeset and elements keys don't conflict. Similarly, the maxlat and max_lat keys don't conflict, so one of the outputs could contain both, depending on which one we want to standardise on.

mmd-osm commented 3 months ago

It seems that both Rapid and iD aren't using the json version of the endpoint (and neither the XML version). TBC by maintainers.

As far as I know, JOSM does not use OSM API JSON endpoints.

I'd prefer not to create a superset version that includes a mix of both variants. I also find the Rails version cleaner.

My proposal would be:

deevroman commented 3 months ago

Note that at least osmapiR supports receiving data via the json api. As I can see, this does not affect the default behavior, but it does affect the library documentation (the library is young, so there may not be any users of this method at all)

https://github.com/ropensci/osmapiR/blob/790e35139d8ae1b5c47169d1a0fe4243fa3bc8ab/R/osmapi_changesets.R#L276

bhousel commented 3 months ago

It seems that both Rapid and iD aren't using the json version of the endpoint (and neither the XML version). TBC by maintainers.

I agree, it doesn't look like we are using this endpoint.

AntonKhorev commented 3 months ago

It would be surprising if these differences are not documented anywhere. API users have to write code expecting either response, like this:

https://github.com/AntonKhorev/osm-note-viewer/blob/67185792b458d5a1c6a7f886c281b107f02be0be/src/osm/changeset.ts#L61-L69

If you rush to fix the issue by changing one of the implementations, API users would still have to write code expecting either response because they can't force all of the deployed osm-website/cgimap servers to be updated.

AntonKhorev commented 3 months ago

@mmd-osm Why do you label this issue as a bug here? If you think there's a bug, the bug is in CGImap. Returning changeset data inside the elements array is wring because a changeset is not an osm element. Compare it to https://api.openstreetmap.org/api/0.6/changesets.json that returns multiple changesets but not in the elements array. But of course you can't fix this bug without breaking compatibility with everything using that endpoint.

mmd-osm commented 3 months ago

Well, it's labeled as a bug because the title says that the two responses are different, which they shouldn't.

It's clear that we haven't decided yet, how exactly the output should look like (like @gravitystorm mentioned), and where and how we want to fix the bug. I will for sure open a separate issue labeled as "Bug" on the CGImap repo, if that's the conclusion of our discussion here.

Switching to the Rails implementation should be fairly easy:

Either remove "changeset" from both lines and send both XML and JSON requests to Rails, or split them up into two rows and only send the XML request to CGImap.

Returning changeset data inside the elements array is wring because a changeset is not an osm element.

Yes, I already mentioned that I find the Rails version cleaner, and that's exactly the reason.

mmd-osm commented 3 months ago

For some reason, the changeset discussions (/api/0.6/changeset/nnn.json?include_discussion=true) also differs across implementations. Are we ok with the current Rails JSON format as a baseline?

mmd-osm commented 3 months ago

I just merged the updated json format in CGImap, which means it will be ready for testing on the dev instance in the next 10 mins or so. Please do some testing and let me know if there are any issues left.

https://master.apis.dev.openstreetmap.org/api/0.6/changeset/375687.json

mmd-osm commented 2 months ago

Apparently, there are either no further issues. The fix will be shipped in next CGImap release. Closing here.

pietervdvn commented 2 months ago

I was using this endpoint with MapComplete, and this breaking change caused me quite some troubles!

Luckily, I have error reports and could (manually) salvage all the changes that people wanted to make, so no information got lost.

I've quickly updated my code so it works again.

mmd-osm commented 2 months ago

Yes, this was really an exceptional case. As a general recommendation, I would encourage more developers to run unit tests on the dev instance from time to time (maybe once a week or so), to discover issues early on. As you can see in the discussion above (and also the OAuth 2.0 discussion), it's sometimes rather difficult to find out which apps are impacted by a given change.