Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add notes multi fetch API call #3707

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

AntonKhorev
Copy link
Collaborator

@AntonKhorev AntonKhorev commented Sep 21, 2022

An API call to fetch several notes at once by their ids, similar to changesets query changesets parameter
(prevously to the elements multi fetch call):

/api/0.6/notes/search?notes=1,2,3

I named it fetch here. If I was to follow a naming convention similar to the elements' call, it should have been named /api/0.6/notes?notes. However notes path is already in use, even twice: for bbox queries with GET and for new notes with POST.

What is it useful for?

If you receive notes from some source other than OSM API they may contain incomplete information or be out of date. That source may be some search engine or some feed for an area. For example, resultmaps.neis-one.org/osm-notes has feeds for countries. Unfortunately they don't have coordinates, so if you want to see the notes on the map, you have to get them from the API. The only way to do that is download the notes one by one by their ids. That's a lot of requests because the feeds may contain hundreds of notes. But there are API calls already that can send hundreds of notes in one response: bbox and search queries. And bbox calls are routinely made when notes layer is turned on. So there's nothing in principle that prevents fetching multiple notes in one request. If you are already committed to downloading a bunch of notes by their ids, it will be faster and less taxing for the API to get all of the notes in one call. Of course there's a limit to the url length, in this case you'd have to make several calls to get really many notes, but that's still better than making really many calls.

Another possible use is if you have a set of notes selected in some app to work on. Maybe other people also work on these notes. In this case you may want to know if any of these notes was updated. Again, the only way to do this is to download them one by one.

@Robbendebiene
Copy link
Contributor

Just an idea:
Instead of adding another API entry point this could also be included as another parameter called ids for the Search for notes: GET /api/0.6/notes/search API entry point similar how it is done for the Query: GET /api/0.6/changesets API.

The benefit of this would be that you could also directly filter notes. For example because you are only interested in notes that are still open.

@AntonKhorev
Copy link
Collaborator Author

If you look at /api/0.6/changesets you can also see that it has bbox parameter. And /api/0.6/notes/search doesn't have it, it's in /api/0.6/notes instead. Why? Who knows. Maybe there are reasons for not extending the existing entry points. As you can see, I also proposed extensions (#3578, #3605), with no reaction so far.

I'm fine with any of these:

  • separare entry point
  • extra parameter for /api/0.6/notes/search
  • extra parameter for /api/0.6/notes

@AntonKhorev
Copy link
Collaborator Author

Although there is a bit of difference between extending the existing note search entry points and adding a new one. The existing ones always sort the notes by date. The new one may can be added without sorting or with sorting in whatever order the ids are passed. The latter seems to be the case with the most straightforward implementation using .find()

@rezaarefi
Copy link

سلام، چگونه باید این مشکل را برطرف کنم؟

@gravitystorm gravitystorm added api Related to the XML or JSON APIs notes Related to the notes feature labels Dec 29, 2022
@AntonKhorev
Copy link
Collaborator Author

I changed it to a search parameter like suggested in #3707 (comment) but named notes to be similar to changesets search.

@AntonKhorev
Copy link
Collaborator Author

Updated reasoning about entry points:

  • /api/0.6/notes
    There are proposals to show notes on the website by default. One objection to that is performance. Notes for the note layer are fetched with this call, and it's not implemented in cgimap. Adding more parameters to it makes a potential fast implementation more complicated.
  • My original pull request was intended to be similar to element multifetch calls. That would also be a reason to put it at /api/0.6/notes, whoch has its drawbacks. But it may also be undesirable for another reason. Element multifetches fail if there is even a single element that can't be shown. You may request hundreds of elements and an error response. Theres's no partial result in this response and no indication which element caused the error. Meanwhile changeset query by ids just skips changesets it can't show.
  • /api/0.6/notes/search
    This seems like a better option in case we don't want to introduce another entry point and if we want feature parity with changeset searching. But there's a drawback. If you supply the note ids as the only parameter, you won't necessarily receive all of the requested notes even if they exist and not hidden. By default the close date filter is enabled, which will skip notes closed more than a week ago. Actual multifetch-like call should include closed=-1 parameter to counteract that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api Related to the XML or JSON APIs notes Related to the notes feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants