99215 - Attachments have no midair collision protection

Status: RESOLVED FIXED

(Bugzilla :: Attachments & Requests, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(I'm reporting this in whatever version of bugzilla http://bugzilla.mozilla.org/
runs as of 10 minutes ago.)

Changes to attachment status have no midair collision detection.  This causes
problems like the one seen in
http://bugzilla.mozilla.org/show_activity.cgi?id=93164 at 2001-09-10 17:02.

Steps to reproduce:
 * Click the "Edit" link for one attachment in one browser
 * do the same in another browser
 * check "has review" in the first, and submit
 * check "has super-review" in the second, and submit

Result:
 * the attachment no longer has review

Expected result alternative #1:
 * midair collision report

Expected result alternative #2:
 * the attachment has both review and super-review

I prefer #1 because of the possibility of withdrawing review for a bug (or the
possibility of one person marking an attachment obsolete as another is
reviewing) -- if it were not possible to unset the states once they were set
then #2 would be preferable.

Comment 1

[Matthew Tuck [:CodeMachine]]

I'll agree here, but do we want midair protection over the whole bug?  Two
create attachments currently clash ... I think a change to one attachment should
clash with a change to another attachment, because it's all the same bug.

Comment 2

[Myk Melez [:myk] [@mykmelez]]

The purpose of midair protection is to prevent one user's changes from
overwriting another's, not to prevent two people from changing the same bug at
once.  Simultaneous changes to two different attachments at the same time do not
have the potential to collide in midair and thus do not need midair protection.

Comment 3

[Matthew Tuck [:CodeMachine]]

AIUI the purpose of midair is to prevent a second person making a change
unnecessary after the first change.  There could be situations where this could
occur on separate attachments or between an attachment and the bug.

If what you say was true, we would have attempted to make midair collisions not
occur when different fields on the same bug were touched.

Comment 4

[Bradley Baetz (:bbaetz)]

(side note)It is sometimes useful not to clash, eg someone adding themselves to
the cc list shouldn't clash with a summary change.

If there are no fields in common (or the fields are lits, like
cc/dependancy/etc), I'd like a "keep going" option on the midair screen. Thats a
separate bug, though

Comment 5

[Gervase Markham [:gerv]]

Nominating for 2.16 blocker - this really isn't ideal.

Gerv

Comment 6

[Dave Miller [:justdave]]

Let me get this straight...  we want midair collisions to occur when two people
edit the same attachment, but not when people edit different attachments or when
one person edits the bug while someone else is editing an attachment?

So basically, we need a timestamp field for last modified on each attachment
record as well?

Comment 7

[Matthew Tuck [:CodeMachine]]

We should do the quick fix and just update the bug's timestamp.  Luckily, this
is also the right fix.

Comment 8

[Gervase Markham [:gerv]]

MattyT: are you able to produce a patch to that effect?

Gerv

Comment 9

[Bradley Baetz (:bbaetz)]

Well, commenting should check the bug while attachment statues should check the
attachment. However, in practice can't we just check the bug? An extra midair
when editing a different attachment won't hurt, and is probably sufficiently
rare that its not worth adding an extra tiemstamp field to the attachment table.

The obsolete stuff from creating an attachment is sort of protected, because it
errors out if one of the attachments is already obsolete. (Although of course it
doesn't do any locking, so its not really safe. Setting isobsolete on an
attachment which is already obsolete won't hurt, though)

Should creating two different attachments midair?

Theres attachments.creation_ts - is that really the last-modified time? It looks
that way, and thats what the header in Attachment.pm::List says...

Comment 10

[Matthew Tuck [:CodeMachine]]

Creating two attachments should definitely midair - all duplicate actions should
midair because they can be mistakes.

Comment 11

[Bradley Baetz (:bbaetz)]

myk: see my laste paragraph in comment 9. If what we're doing is correct, then I
can just use that.

Do note that going back after having done "edit attachment as comment" in
mozilla will lose your comments. I'm not sure if there is a mozilla bug filed on
that.

Comment 12

[Myk Melez [:myk] [@mykmelez]]

Yes, attachments.creation_ts is indeed to the last modified time.

Comment 13

[Bradley Baetz (:bbaetz)]

Would be very nice, but no patch, not a stopper, and probably a fair bit of work
to check all the cases.

->2.18

Comment 14

[Gervase Markham [:gerv]]

Definitely not a blocker.

Gerv

Comment 15

[Dave Miller [:justdave]]

All 2.18 bugs that haven't been touched in over 60 days and aren't flagged as
blockers are getting pushed out to 2.20

Comment 16

[Max Kanat-Alexander]

This bug has not been touched by its owner in over six months, even though it is
targeted to 2.20, for which the freeze is 10 days away. Unsetting the target
milestone, on the assumption that nobody is actually working on it or has any
plans to soon.

If you are the owner, and you plan to work on the bug, please give it a real
target milestone. If you are the owner, and you do *not* plan to work on it,
please reassign it to [e-mail] or a .bugs component owner. If you are
*anybody*, and you get this comment, and *you* plan to work on the bug, please
reassign it to yourself if you have the ability.

Comment 17

[Frédéric Buclin]

*** Bug 212180 has been marked as a duplicate of this bug. ***

Comment 18

[Frédéric Buclin]

*** Bug 233393 has been marked as a duplicate of this bug. ***

Comment 19

[Frédéric Buclin]

(In reply to comment #12)
> Yes, attachments.creation_ts is indeed to the last modified time.

Oh really? Actually, creation_ts is the creation date of the attachment and an
additional field similar to delta_ts is required in the attachments table in
order to fix this bug correctly.

It does not make sense to midair when the bug or another attachment is being
modified at the same time. And marking an attachment as obsolete when submitting
a new attachment should update the delta_ts-like field of the obsoleted
attachment in order to correctly reflect the change.

We are frozen and 2.20rc1 should be released this week. So pushing it to 2.22,
unfortunately.

Comment 20

[Frédéric Buclin]

I won't have time to play with it before the 2.22 freeze

Comment 22

[Dave Miller [:justdave]]

Attachments actually have working independent creation and modification dates now (they didn't the last time there was major discussion on this bug) so this should actually be easily doable now.  Too much change to shoehorn into 3.0 though, we're frozen.

Comment 24

[Nelson Bolyard (seldom reads bugmail)]

Here's another recent example.
I received a review request on a patch.  So I went to the bug, clicked
on the "details" link for the patch, spent about 20 minutes writing a 
review and setting the review flag on the attachment details page, and 
then I clicked submit.

While I had been writing that review, the author of the patch found a bug
in the patch I was reviewing, and submitted another patch, and marked the
patch I was reviewing obsolete, which cancelled the review request to me
as well as to another reviewer.  But I did not know any of that had happened.

So, when I submitted my review, submitting the contents of the details
page, I effectively undid his recent changes.  My submission removed the
obsolete flag from the patch I reviewed, and it restored the review request
to the other reviewer for the patch I had reviewed.  I received no warning
from the b.m.o web server about any of this.  

I learned about the collision when I received the email from b.m.o, telling
me of the changes I had made.  There I saw that it appeared at I had 
unobsoleted the patch, and that I had requested that another reviewer 
also review that patch. 

Comment 25

[Frédéric Buclin]

Attachment: patch, v1

Comment 26

[Frédéric Buclin]

Comment on attachment 280247 [details] [diff] [review]
patch, v1

Max, could you look at the DB and upgrade parts of the patch?
Myk, could you look at the attachment part?

Comment 27

[Max Kanat-Alexander]

Comment on attachment 280247 [details] [diff] [review]
patch, v1

>         INDEXES => [
>             attachments_bug_id_idx => ['bug_id'],
>             attachments_creation_ts_idx => ['creation_ts'],
>+            attachments_modif_time_idx => ['modification_time'],

  Index names *must* contain the full name of the first field.

>Index: Bugzilla/Install/DB.pm
> [snip]
>+sub _fix_attachment_modification_date {
>+    my $dbh = Bugzilla->dbh;
>+    $dbh->bz_add_column('attachments', 'modification_time', {TYPE => 'DATETIME'});
>+
>+    my $attachments_with_no_modif_time =
>+      $dbh->selectcol_arrayref('SELECT attach_id FROM attachments
>+                                WHERE modification_time IS NULL');

  Just check if the column exists, and then add it and do these things if it doesn't exist.

>+    print "Setting the modification time for attachments having none...\n";
>+    foreach my $attach_id (@$attachments_with_no_modif_time) {
>+        my $time = $dbh->selectrow_array($sth_select, undef, $attach_id);
>+        if ($time) {
>+            $sth_update->execute($time, $attach_id);
>+        }
>+        else {
>+            # The attachment has never been edited. Use the creation time.
>+            $sth_sync->execute($attach_id);
>+        }

  Wait, if this is set to creation_ts when the attachment hasn't been modified, then why is modified_time allowed to be NULL? Either it should be NOT NULL or NULL should represent "never modified". (I prefer the second, but if the first makes sense, that's fine too. You could always put that logic into Attachment.pm instead of into the DB, though.)

Comment 28

[Frédéric Buclin]

Attachment: patch, v2

mkanat: DB + upgrade parts; myk: attachment part.

Comment 29

[Max Kanat-Alexander]

Comment on attachment 282039 [details] [diff] [review]
patch, v2

Yeah, the DB part looks fine.

At the bottom of the upgrade code, you might want to make that foreach loop a bit clearer, for people who aren't perl gurus.

Comment 30

[Dave Miller [:justdave]]

Comment on attachment 282039 [details] [diff] [review]
patch, v2

ok, this works as designed on the trunk in my testing. My only concern is the text "This will cause all of the above changes to be overwritten" next to the Submit anyway button, but we have the same text on the bug midair page, too, and I'm sure there's another bug on that somewhere (because it's not always accurate)

Comment 31

[Dave Miller [:justdave]]

They want this on b.m.o, but we're not upgrading to tip yet, which means we get to backport this patch.  It *almost* applies cleanly on the 3.0 branch, and the one conflict was painless to apply manually, however there must be API changes somewhere between 3.0 and trunk that affect this because it doesn't work on the branch (no errors anywhere, but the midair is never triggered).

For the record, I don't want a 3.0 backport committed to CVS, it does schema changes, and I don't want schema changes on the branch unless necessary for a security issue.

Comment 32

[Frédéric Buclin]

Checking in attachment.cgi;
/cvsroot/mozilla/webtools/bugzilla/attachment.cgi,v  <--  attachment.cgi
new revision: 1.137; previous revision: 1.136
done
Checking in process_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v  <--  process_bug.cgi
new revision: 1.394; previous revision: 1.393
done
Checking in Bugzilla/Attachment.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Attachment.pm,v  <--  Attachment.pm
new revision: 1.51; previous revision: 1.50
done
Checking in Bugzilla/Bug.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Bug.pm,v  <--  Bug.pm
new revision: 1.218; previous revision: 1.217
done
Checking in Bugzilla/DB/Schema.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB/Schema.pm,v  <--  Schema.pm
new revision: 1.90; previous revision: 1.89
done
Checking in Bugzilla/Install/DB.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Install/DB.pm,v  <--  DB.pm
new revision: 1.43; previous revision: 1.42
done
Checking in template/en/default/filterexceptions.pl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/filterexceptions.pl,v  <--  filterexceptions.pl
new revision: 1.109; previous revision: 1.108
done
Checking in template/en/default/attachment/edit.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/attachment/edit.html.tmpl,v  <--  edit.html.tmpl
new revision: 1.49; previous revision: 1.48
done
RCS file: /cvsroot/mozilla/webtools/bugzilla/template/en/default/attachment/midair.html.tmpl,v
done
Checking in template/en/default/attachment/midair.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/attachment/midair.html.tmpl,v  <--  midair.html.tmpl
initial revision: 1.1
done

Comment 33

[Frédéric Buclin]

Attachment: add missing index when upgrading, v1

The DB index is omitted when upgrading, making Tinderbox to fail. Oops!

Comment 34

[Frédéric Buclin]

mkanat, could you review the 2nd fix about the missing index?

Comment 35

[Max Kanat-Alexander]

Comment on attachment 290760 [details] [diff] [review]
add missing index when upgrading, v1

Looks good to me.

Comment 36

[Frédéric Buclin]

Checking in Bugzilla/Install/DB.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Install/DB.pm,v  <--  DB.pm
new revision: 1.44; previous revision: 1.43
done

Comment 37

[Max Kanat-Alexander]

Added to the Bugzilla 3.2 release notes in bug 432331.