Modify

Opened 6 years ago

Closed 4 years ago

#5402 closed enhancement (fixed)

add support for detecting mid-air collisions

Reported by: stp Owned by: osimons
Priority: normal Component: XmlRpcPlugin
Severity: normal Keywords:
Cc: thijs, stp Trac Release: 0.11

Description (last modified by osimons)

If the same field of a ticket is updated concurrently by two users the later update overrides the previous change without notice. It would be helpful if Trac supported versioning of tickets and would reject an update if the provided data did not match the version currently in the repository. An example of that is Bugzilla's has a notion of mid-air collision which require updating first and re-submitting in case of a conflict.

The original request filed against Mylyn is tracked here:

253932: submitting changes also sends unchanged fields possibly resetting them to old values https://bugs.eclipse.org/bugs/show_bug.cgi?id=253932

Attachments (3)

t5402-ticket_collision-r6070.diff (6.3 KB) - added by osimons 6 years ago.
Ticket update collision detection.
trac-xmlrpc-mid-aircollision.diff (6.5 KB) - added by stp 4 years ago.
updated patch
trac-xmlrpc-mid-aircollision-v2.diff (8.0 KB) - added by stp 4 years ago.
version that uses _ts field to transmit token

Download all attachments as: .zip

Change History (12)

comment:1 Changed 6 years ago by osimons

  • Description modified (diff)

This implies adding a version field to the returned ticket attributes, and then expecting the same marker to be included in the attributes returned for updating. Should be an easy fix.

2 things come to mind:

  • 'version' is already taken by the ticket 'version' field, so I need some other naming. It would need to be something like _version or _ver. I kind of like _ver because it is sufficiently different.
  • We'll likely need to support updates without the marker for a while. I'll update the docs to strongly recommend against not including it, and then remove old-style update along with some other deprecated changes when releasing a 1.1 version or something.

See also #2794 for similar request for wiki pages.

comment:2 Changed 6 years ago by stp

  • Description modified (diff)

Does Trac atually have a notion of a version for tickets? If not, we could also consider using the modification timestamp, i.e. the client would send it back and only if it matches the timestamp in the repository would the update succeed.

comment:3 Changed 6 years ago by osimons

  • Description modified (diff)

I had timestamp in mind for version - alternatively 'version' as a distinct count of timestamps in ticket_change table (more or less what is used for comment-numbers). Anyway, Trac itself uses the timestamp to detect collision so just using that makes the most sense. It is also returned with any ticket.get() and update() call so it easily accessible.

I've made a patch, and again like for 'action' it tries to do so without changing the API. To support update collision, add a time_changed timestamp to attributes when updating. Not using time_changed is deprecated in the patch, so in some future version any update that includes attributes will require those two keys. When working on update(), I also took the time to make a full set of tests for all various incarnations of updating (all passing).

Attaching patch. Review and feedback welcome.

Changed 6 years ago by osimons

Ticket update collision detection.

comment:4 Changed 6 years ago by osimons

Need to consider #3988 in this - if the API should actually allow (time_created and) time_changed to be specified and used in updates.

comment:5 Changed 6 years ago by osimons

Thinking about #3988 in this again (flexibility vs rigor in the api): What if something like time_changed supported both use cases like this:

  • If time_changed is older than currently latest, the change is rejected.
  • If it is the same as latest, it is a collision-detection, and current time is stored with new update.
  • If time_changed is newer than currently latest, it is taken as a request to update a ticket using this particular timestamp - typically for usage whereby tickets are pushed in from some other system with the need to preserve timestamps. That way such push/import could be done as long as all updates where passed in order - ie, just append changes at the end (which makes sense anyway).

comment:6 Changed 6 years ago by anonymous

  • Cc thijs added; anonymous removed

Changed 4 years ago by stp

updated patch

comment:7 Changed 4 years ago by stp

  • Cc stp added

I have updated the patch to work with trunk and added support for sending the modification timestamp in Mylyn (https://bugs.eclipse.org/bugs/show_bug.cgi?id=253932).

These are the only modifications I made to the original patch:

  • Renamed time_changed to changetime which is the field name Trac uses to store the time stamp in the database.
  • Fixed import for date conversion function in test case.

I'll run the Mylyn test suite to ensure that the change does not introduce any regressions. If everything checks out would you consider merging it?

comment:8 Changed 4 years ago by anonymous

Unfortunately testing revealed problems on Trac 0.12 which uses more precise time stamps. The time stamp that is transfered through XML-RPC is only accurate to the second whereas the database timestamp that is used to validate ticket updates is accurate to the milli-second, e.g.:

XML-RPC TS: 2010-07-08 01:58:02+00:00
Ticket TS:  2010-07-08 01:58:02.320199+00:00

Since there is no simple way to work around that we might need to consider an alternative approach.

How about adding a new "hidden" attribute called _ts for instance that is a string representing the token required for ticket updates and only used for that purpose (hence the under score)? This way data conversions won't interfere with the validation.

Changed 4 years ago by stp

version that uses _ts field to transmit token

comment:9 Changed 4 years ago by osimons

  • Resolution set to fixed
  • Status changed from new to closed

(In [9912]) XmlRpcPlugin: Added a ticket '_ts' token to detect mid-air collisions for ticket.update() calls.

Big thanks to stp for raising this issue and providing the patch + lots of new ticket tests to verify main ticket functionality.

Closes #5402

Add Comment

Modify Ticket

Action
as closed The owner will remain osimons.
The resolution will be deleted. Next status will be 'reopened'.
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.