Skip to content

Commit

Permalink
Use where instead of find to prevent 404
Browse files Browse the repository at this point in the history
  • Loading branch information
@ENT8R
ENT8R committed Aug 25, 2023
1 parent 6759130 commit b9c85c2
Show file tree
Hide file tree
Showing 2 changed files with 2 additions and 2 deletions.
2 changes: 1 addition & 1 deletion app/controllers/api/users_controller.rb
View file
Expand Up @@ -19,7 +19,7 @@ def index

raise OSM::APIBadUserInput, "No users were given to search for" if ids.empty?

@users = User.visible.find(ids)
@users = User.visible.where(:id => ids)

# Render the result
respond_to do |format|
Expand Down
2 changes: 1 addition & 1 deletion app/views/api/users/index.xml.builder
View file
@@ -1,4 +1,4 @@
xml.instruct! :xml, :version => "1.0"
xml.osm(OSM::API.new.xml_root_attributes) do |osm|
osm << render(@users)
osm << (render(@users) || "")
end

45 comments on commit b9c85c2

@Zaczero
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a breaking change for some of my apps. While I completely agree with the concept behind it, I don't agree with addressing it within API v0.6. I believe this should be introduced in a new API version instead. Breaking changes such as this should not be released at random.

@tomhughes
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You have apps with do selects for multiple users and rely on getting 404 if any one of the users is missing?!? Was that ever documented anywhere as expected behaviour because I'm pretty sure it was only ever an accident and it really doesn't seem like sensible or useful behaviour.

@Zaczero
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like a very common use case.

  1. Fetch changesets
  2. For each changeset uid, fetch latest information about that user
  3. Previously not found users caused 404 behavior but now it's 200
  4. There is https://planet.osm.org/users_deleted/users_deleted.txt but it updates just once a day and is very unreliable

@Zaczero
Copy link

@Zaczero Zaczero commented on b9c85c2 Aug 26, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When dealing with widely-used APIs, it's crucial to exercise caution and avoid altering any existing behaviors. Releasing the changes under a new API version seems like a simpler approach. These changes as I see it are non-critical. Implementing random changes of this nature can undermine the projects reliability for developers. Isn't that a common sense?

Regardless of whether the change was accidental or intentional, established API methods should remain consistent. This principle is fundamental to creating a dependable and esteemed API. Even the slightest modifications pose a risk of disrupting applications for users. It would be reasonable to designate this change with a new version, as it's a non-critical modification.

@AntonKhorev
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For each changeset uid, fetch latest information about that user

Do you use /api/0.6/users?users to fetch single users instead of /api/0.6/user?

@Zaczero
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, batched:
https://www.openstreetmap.org/api/0.6/users.json?users=20113056,20113059,20113060,20113061,20113065,20113067,20113069,20113072,20113073,20113075,20113079,20113092,20113097

This returns 200 now and breaks my logic completely. I previously expected 404 here.

@AntonKhorev
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you expect 404 when one user was missing or when all of them were?

@wzbartczak
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tomhughes Shouldn't such changes be publicized on the forum before they are implemented?

@Zaczero
Copy link

@Zaczero Zaczero commented on b9c85c2 Aug 26, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you expect 404 when one user was missing or when all of them were?

Previous logic worked this way:

  • Return 404 if at least one user is missing

While a very weird behavior indeed, I wrote my code around this issue. I have seen worse things in my life.

The current logic works this way:

  • Always return 200 and skip not found users

My previously coded applications are not ready for such change, this is not backwards compatible, thus: breaking change.

The green lines are additions I had to do today when I noticed my app stopped working:

image

@AntonKhorev
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd put new behavior on a different endpoint, for example /api/0.6/users/search (similar to /api/0.6/notes/search).

@Zaczero
Copy link

@Zaczero Zaczero commented on b9c85c2 Aug 26, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd put new behavior on a different endpoint, for example /api/0.6/users/search (similar to /api/0.6/notes/search).

Or 0.7. Why is community scared of releasing a new API version with improvements - it's just a number. If for some reason you want to stay on 0.6 - sure - but please don't change its behavior.

@AntonKhorev
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

404s if even a single thing is missing are especially annoying during element multifetches, where you ask for hundreds of items.

@Zaczero
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

404s if even a single thing is missing are especially annoying during element multifetches, where you ask for hundreds of items.

I completely agree, but this is unfortunately a breaking change. Code has already been written and changes like these undermine the reliability of the OSM API. Please don't waste peoples time just because.

@AntonKhorev
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or 0.7, why is community scared of releasing a new API with improvements. It's just a number.

Then two versions will have to be supported in parallel, or almost every api client will be broken.

@Zaczero
Copy link

@Zaczero Zaczero commented on b9c85c2 Aug 26, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or 0.7, why is community scared of releasing a new API with improvements. It's just a number.

Then two versions will have to be supported in parallel, or almost every api client will be broken.

That's very common to see nowadays. What's exactly the issue here because I don't fully understand it. If you want to make any significant improvements, you do it under a new version. That's how API development works - no? This is not an ordinary website/client/mobile app development.

@AntonKhorev
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With changes like here you'll quickly get to 0.10 and five api versions active at the same time.

@Zaczero
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With changes like here you'll quickly get to 0.10 and five api versions active at the same time.

How exactly does this help the situation in any way? I am trying to help. Today's changes simply broke the backwards compatibility and I wanted to point out that behavior like this is very odd, surprising, and must be avoided. This is a non-critical update and there is no reason for it to break peoples applications.

@AntonKhorev
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm saying that doing api 0.7 just for user multifetches without 404s is not a good idea and a new api version is not just a number. Keeping one api/users#index method in addition to /api/0.6/users/search is going to be easier than keeping api 0.6 together with api 0.7.

search endpoint later can be extended for date ranges, like other search endpoints. Then the old endpoint can be declared deprecated and api 0.7 will have it dropped.

@Zaczero
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I understand. However, I didn't suggest that we make the switch to 0.7 solely due to the multifetch change. My idea is that we accumulate enhancements gradually and then introduce version 0.7, instead of repeatedly patching up 0.6. It's important to strike a balance here. Having a significant version update every 2-4 years seems quite reasonable to me.

@tomhughes
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, but as I understand it this was an urgent change to assist DWG with their efforts to fight the ongoing vandalism which is why I was keen to merge and deploy it as quickly as possible - obviously we had not anticipated that people would rely on either the ordering or the 404 behaviour.

@Zaczero
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, but as I understand it this was an urgent change to assist DWG with their efforts to fight the ongoing vandalism which is why I was keen to merge and deploy it as quickly as possible - obviously we had not anticipated that people would rely on either the ordering or the 404 behaviour.

I understand your point. Identifying non-obvious breaking changes can be challenging. However, the manner in which you address such issues is crucial. Making assertive statements like the ones, b9c85c2#commitcomment-125593592, be96aa7#commitcomment-125593392, is, in my opinion, not the most effective approach. A more constructive method of handling such situation would be welcome.

@tomhughes
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've pushed c7a31eb to restore the ordering.

@Zaczero
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you provide an update on this issue, @tomhughes? I commend your efforts in restoring the ordering, but it seems slightly off-topic considering the context of this commit and this conversation.

@tomhughes
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What sort of update would you like? The ordering change has been reverted and there are no current plans to make any other changes.

@Zaczero
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you actually reviewed the discussion? Your response is very odd.

@AntonKhorev
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So new 404 behavior is going to stay?

You have apps with do selects for multiple users and rely on getting 404 if any one of the users is missing?!? Was that ever documented anywhere as expected behaviour [...]

Was any missing user response documented anywhere? Because if not, the only way to use this endpoint was to read the source code and expect it to keep working as it did before.

because I'm pretty sure it was only ever an accident

Elements multifetches behave the same way, so you can't be so sure.

@Zaczero
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It appears that this will be the case. It is quite perplexing that @tomhughes opted to disrupt individuals' applications purely for the sake of optimization. This alteration doesn't bring forth any new functionalities, and as I highlighted, the prior version, despite its inefficiency, could operate in precisely the same manner as it does "currently". This situation raises significant concerns about the trajectory of OSM API development.

@tomhughes
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did no such thing. I merged a change which seemed reasonable and which I did not anticipate would disrupt anybody. There was absolutely no intention to disrupt anybody.

The reference to optimisation related to ordering and that wasn't a deliberate attempt to optimise, it was entirely accidental because none of us realised that find preserved order. The mention of optimisation once that came of was because I thought maybe it was doing lots of single selects which would be horrible but it doesn't and in any case I have reverted the entirely accidental ordering change now.

@woodpeck
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Zaczero, I'd suggest to tone down your outrage a little. In my eyes, it is not a "breaking" change if undocumented behaviour changes. And it clearly is a change for the better, bringing the user multi-fetch in line with the other multi-fetches already existing. I can understand that it is annoying if you have to change your application - perhaps even after your application's users writing to you and complaining about it not working. Of course it would be nice if everyone could comfortably rely on OSM providing several APIs in parallel so that everyone can upgrade their app to the latest API at leisure. But resources - personnel and otherwise - are limited, and we can't afford that. We regularly change the existing API in minuscle ways that are unlikely to trip up anyone but that do change the behaviour at little, and things might break. So in this case it was you who had to bear a tiny little bit of the work that we all do, as volunteers, on a regular basis. I, for one, am happy that small and sensible changes like this do not have to go through a two-year staging process. Yet ;)

@Zaczero
Copy link

@Zaczero Zaczero commented on b9c85c2 Aug 28, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the majority consensus leans toward accepting the sacrifice of backward compatibility in the name of optimization, then let it be so. Thank you for the clarification.

My concern regarding optimization pertains specifically to this commit, not the other sorting-related change. This change specifically didn't introduce fresh functionality; it merely enhanced efficiency, as evidenced by my previous code example (performing binary search on 404). While I wholeheartedly support this modification, I firmly believe it should not have been executed in a manner that breaks compatibility. This, I perceive, is a lack of consideration for developers who utilize this API.

@grischard
Copy link
Contributor

@grischard grischard commented on b9c85c2 Aug 28, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Zaczero you might be right, but this might not be the best way of saying it. Remember to assume good faith, and that OSM is a community project - we have to work with each other instead of against each other. It sucks when undocumented behaviour breaks and you have to change your code, but that's the faustian bargain you make when you rely on it. Tom and ENT8R do this in their spare time, can't be expected to know every way in which the API is being (ab)used.

"Every change breaks someone's workflow"

@Zaczero
Copy link

@Zaczero Zaczero commented on b9c85c2 Aug 28, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I completely agree @grischard. This is precisely why I opted to shed light on this matter. The predicament lies in the fact that despite my explicit description of the breaking change, no action is being taken to correct it; instead, it's being acknowledged without resolution.

This modification has impacted the only efficient method for querying the most recent user information. This is not some obscure workflow.

PS. This breaking change was actually anticipated. By reviewing the pull request, you'll observe the corresponding test modifications to ensure their success. What even is the point of having tests then?
#4203

@AntonKhorev
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It sucks when undocumented behaviour breaks and you have to change your code, but that's the faustian bargain you make when you rely on it.

There was no way not to rely on it. And in case of elements multifetch, it was documented: https://wiki.openstreetmap.org/wiki/API_v0.6#Error_codes_21

@AntonKhorev
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This didn't break iD because there seems to be no error handling:

https://github.com/openstreetmap/iD/blob/68ff64b01d5a125950474dbf567bc0a9fa3b9edd/modules/services/osm.js#L896

Users are requested only together with notes and deleted users' note comments are hidden by the api.

@AntonKhorev
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By the way, this didn't happen:

bringing the user multi-fetch in line with the other multi-fetches already existing.

Now we're up to three different styles of multifetches:

  • ordered by id, fail on any item (elements)
  • ordered by id, no-fail (users)
  • ordered by other search parameters, no-fail (changesets)

Previously it was two, without the middle one.

@Zaczero
Copy link

@Zaczero Zaczero commented on b9c85c2 Aug 29, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I often presume the accuracy of information shared by individuals. Unfortunately, I had to verify the claim that "this was undocumented behavior", and it turns out to be false.

In the API v0.6 documentation, search for "or an empty file if no user found for given identifier" (2nd result). Despite what the documentation states, the following requests do not yield an empty file:

(they did yield an empty file before this breaking change)

At this point, I'm struggling to comprehend the rationale behind persistently presenting such erroneous arguments. It's quite peculiar that a head of development cannot acknowledge their errors and, instead, shifts blame onto others' code. The concepts we're discussing here are fundamental to code development, and it appears that some individuals lack a basic understanding of them. This entire conversation has proven quite draining for me. Despite my best attempts to highlight the issues, it seems unlikely that corrective measures will be taken. Such changes demonstrate a lack of respect for individuals utilizing this API. Hopefully, this will change for the better in the future.

@tomhughes
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please understand that the "documentation" you refer to is not written by us and has no official status - that is essentially documented by users reverse engineering and assuming the current behaviour is correct and documenting it.

@Zaczero
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is getting quite silly... Where can I find the "official" documentation? Is every behavior undefined?

@tomhughes
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no official documentation.

@Zaczero
Copy link

@Zaczero Zaczero commented on b9c85c2 Aug 29, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Understood. In that case, it's crucial to inform users of https://wiki.openstreetmap.org/wiki/API_v0.6 that this documentation is unofficial and that any behaviors outlined there should be regarded as subject to change in the future. Essentially, there is no consistent 0.6 behavior. It's a bit ironic for me to observe that instead of acknowledging the mistake, the conversation continues with such perplexing arguments.

I find it quite surprising that there isn't an official OSM API behavior. Why hasn't this been addressed before? It appears to be a significant oversight. How can people be expected to develop apps on OSM without a such a fundamental standard?

@woodpeck
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Zaczero, please take a step back and ask yourself why this whole discussion is "draining" and if perhaps your own style of discussion contributes to that situation. Personally, I find most of your posts rather condescending; there's hardly any sentence in which you do not accuse others of doing things "at random", of making things "unreliable", of being "not reasonable", of being "odd", of having "opted to disrupt", of "sacrificing" something. Even if you were 100% right, this righteous attitude will drain everyone's resources, and everyone's willingness to deal with you.

Just change those few lines of code in your application and accept that OSM is a moving project driven by volunteers, and not a commercial endeavour where people pay millions to have a guaranteed API with release cycles and all.

And please, refrain from sullenly putting a huge red box on top of the API documentation on the wiki pointing out how unreliable and unofficial everything is. Because you could do that on every wiki page ;)

And maybe you can even swallow that defiant one-liner you'd like to reply to this message with, and let us all, including yourself, get back to more productive things for OSM.

@mmd-osm
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CGImap maintainer here. The OSM APi 0.6 Wiki page now displays an ambox including the text "Every behavior described here results from reverse engineering". I don't think this is accurate the way it has been stated there, since I won't be reverse engineering my own code at least.

Originally I have added the comment ""or an empty file if no user found for given identifier". That "empty file" should read as "JSON document with empty users array". Probably just a sloppy way of documenting the JSON result. You're invited to correct it, it's a Wiki after all.

Don't get too much hooked up that there's no official documentation. That's ok, everyone knows it. It's been this way since the beginning, and unlikely to change anytime soon. We have some ideas to include the documentation in this repo, and automatically generate the documentation. Unfortunately, there's not enough bandwidth for such topics, since, as you probably know, we all do this here in our spare time as volunteers. Again, you're invited to help improving things. Until then, the OSM API 0.6 Wiki page is the best documentation available. I try to keep it up-to-date with changes, as do many others, but I don't always have time to work on it.

@Zaczero
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mmd-osm Thank you for your input. I've revised the text on the wiki as follows:

Please note that this is NOT official documentation[1]. Every behavior described here is subject to change in the future.

This revision should more accurately reflect the current status. As this is the only API documentation available, it's crucial to inform developers about its unofficial nature and potential for change. This type of information might come as a surprise given the project's size, so it's better to be transparent about it.

Don't get too much hooked up that there's no official documentation. That's ok, everyone knows it. It's been this way since the beginning, and unlikely to change anytime soon.

The landscape of the OSM project today differs from its state a decade ago. With more developers, including commercial ventures, relying on this project, having dependable documentation becomes critical. Once again, this appears to be a significant oversight.

Personally, the distinction between official and unofficial doesn't concern me greatly. What I care about the most is writing reliable software. If we're at the point where we decide to intentionally change tests and go against the only available documentation (the consensus), this is a major red flag for me.

@gravitystorm
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a lot here in this conversation, so I'll be brief:

  • Ideally when the backwards-incompatible changes were made, and were found to break someone's application, they should have been reverted immediately and then the options discussed. However, I'm not involved in the time-sensitive tasks that OWG and DWG have been involved in over the last few days, so I can't judge whether this breakage was worthwhile for other reasons.
  • Ideally the backwards-incompatible changes should have been highlighted in the PR and at review time, so that they could be investigated (e.g. using server logs) and discussed before merging.
  • The topics of how to handle backward-incompatible changes, running multiple versions of the API, etc are all dear to my heart, but far to substantial to be discussed in a commit comment!
  • If you are surprised by the lack of formal documentation covering all edge cases of every API, then you'll be shocked when you find out how few volunteers contribute to the codebase. It's not "a significant oversight", it's a balance of priorities under extremely tight resource constraints. I'd love to have 1000 other things improved too!

@AntonKhorev
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally when the backwards-incompatible changes were made, and were found to break someone's application, they should have been reverted immediately and then the options discussed.

#4207

Please sign in to comment.