Validation for note transitions

[angoca]

URL

https://api.openstreetmap.org/api/0.6/notes/52751

How to reproduce the issue?

I am analyzing the notes dump from Planet and found this invalid transition: reopen twice.

This is impacting the validations I am doing on my code.

I would like to know if this is normal (no validation). Or what is the issue behind this?

Screenshot(s) or anything else?

No response

[AntonKhorev]

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

[angoca]

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.

[AntonKhorev]

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 https://github.com/openstreetmap/openstreetmap-website/commit/d74d4f8d195673250c3f2e841d28f185d74d8849.

[angoca]

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

[angoca]

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

[mmd-osm]

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.

[tomhughes]

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 ae27f7adbe561b20203fa639ac1aa50d0408edc0 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.

[mmd-osm]

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

[tomhughes]

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

[mmd-osm]

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