Validation for note transitions about openstreetmap-website HOT 10 OPEN

You are not guaranteed to see all comments.

1. user A reopens the note
2. user B closes the note
3. user A reopens the note again
4. user B deletes their account - now you won't see (2), you'll see two reopens

from openstreetmap-website.

Hi Anton, Thanks for your example, but this is not the case. The date/time is the same for the 2 "reopen" actions, with the same user (vramirez) who still has an active account.

from openstreetmap-website.

Ok, the two reopens at 2013-10-05 15:13:48 UTC probably didn't happen as I described, but that's still a possible scenario.

Maybe it's possible to reopen a note twice if the requests are done quick enough. The api code didn't change much since it was added in d74d4f8.

from openstreetmap-website.

Therefore, you confirm that this is a bug that the API has, which does not check the current status when a new status arrives, and it is the same.

I have found the other case, close a closed note: 19523

from openstreetmap-website.

I have found other interesting cases:

• 4 consecutive closes https://api.openstreetmap.org/api/0.6/notes/1258997 in 4-minute window.
• Different users closing the note https://api.openstreetmap.org/api/0.6/notes/365932

I put the complete list here: https://wiki.openstreetmap.org/wiki/User:Angoca/invalidTransitions

from openstreetmap-website.

Maybe it's possible to reopen a note twice if the requests are done quick enough

At least checking the current status of a note (Note.find_by_id(id) ... raise OSM::APINoteAlreadyOpenError.new(@note) unless @note.closed?) is done without exclusive or optimistic locking (based on version numbers).

If multiple API requests for the same note are processed at the same time, such duplicates can occur.

from openstreetmap-website.

There is a check for duplicate closes (https://github.com/openstreetmap/openstreetmap-website/blob/master/app/controllers/api/notes_controller.rb#L181) and has been since ae27f7a in February 2013 but as it is at the application layer two closes very close together might still make it to the database because both might pass the check before either one has made the change as the check is not inside a transaction and there is no constraint in the database that would block it.

from openstreetmap-website.

I guess something like this should do then?

      # Close the note and add a comment
      Note.transaction do
        # Find the note and check it is valid
        @note = Note.lock.find_by(:id => id)
        raise OSM::APINotFoundError unless @note
        raise OSM::APIAlreadyDeletedError.new("note", @note.id) unless @note.visible?
        raise OSM::APINoteAlreadyClosedError, @note if @note.closed?

        @note.close

        add_comment(@note, comment, "closed")
      end

from openstreetmap-website.

I doubt an explicit lock is needed or a good idea - a transaction on it's own should be enough surely?

from openstreetmap-website.

Try to add a "sleep 10" right before @note.close and then test in two different browser windows. Without explicit locking you can resolve the note twice, with locking in place the second resolve attempt fails with an error message "The note xxx was closed at yyy UTC".

lock will add "FOR UPDATE" when fetching details for one note only. It's not some sort of table lock.

SELECT "notes".* FROM "notes" WHERE "notes"."id" = $1 LIMIT $2 FOR UPDATE  [["id", 4], ["LIMIT", 1]]

It would probably be better to use optimistic locking in this case, since conflicts should be extremely rare. On the other hand, our Note model doesn't have a lock_version field, which is needed according to https://api.rubyonrails.org/classes/ActiveRecord/Locking/Optimistic.html

from openstreetmap-website.