281550 - Remove RelationSet from userprefs.cgi (and thus fix non-ANSI INSERT)

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, defect)

Description

[Max Kanat-Alexander]

Bugzilla::RelationSet::generateSqlDeltas generates INSERT syntax like this:

INSERT INTO watch (watcher, watched) VALUES (83, 1),(83,
77),(83, 3)

That, unfortunately, is MySQL-specific.

I think the solution is that generateSqlDeltas just shouldn't be used. :-) It's
only used for watchers in userprefs.cgi, so we can just get rid of its use there.

Comment 1

[Max Kanat-Alexander]

Actually, I'll do it. We need this change, locally.

Comment 2

[Max Kanat-Alexander]

Attachment: Stop using RelationSet for that purpose

OK, here's the code, without RelationSet being used there.

I also added a nice function to Bugzilla::Util, diff_arrays, to facilitate
this.

Comment 3

[Max Kanat-Alexander]

Actually, let's just change the focus of this bug, since it's the last blocker
to 281590 and I've already done the hard part.

Comment 4

[Max Kanat-Alexander]

Attachment: Remove RelationSet entirely from userprefs

OK, here we go. Once again, it was pitifully simple and much more efficient to
not use RelationSet here.

Comment 5

[Frédéric Buclin]

Comment on attachment 173827 [details] [diff] [review]
Remove RelationSet entirely from userprefs

>+        my $watched_now = 
>+            $dbh->selectcol_arrayref("SELECT watched FROM watch"
>+                                   . " WHERE watcher = ?", undef, $userid);

What about $old_watch_ids for consistency? "_now" is ambiguous, because I read
it as "now that I have updated my list", which is clearly not the case here.


>+    # If they're equal, set them to empty. When done, @old contains entries
>+    # that were removed; @new contains ones that got added.
>+    foreach my $oldv (@old) {
>+        foreach my $newv (@new) {
>+            next if ($newv eq '');
>+            if ($oldv eq $newv) {
>+                $newv = $oldv = '';
>+            }
>+        }
>+    }

Funny way to do this. But I'm not sure it is optimal for long arrays. What
about sorting them first so that you don't need to restart from the beginning
of the @new array everytime?


The rest of the patch is bitrotten and I have absolutely no idea what you do
here (this part is completely missing from Util.pm). So please update your
patch so that I can review this last part.

Comment 6

[Max Kanat-Alexander]

Attachment: v2

(In reply to comment #5)
> (From update of attachment 173827 [details] [diff] [review] [edit])
> >+	    my $watched_now = 
> >+		$dbh->selectcol_arrayref("SELECT watched FROM watch"
> >+				       . " WHERE watcher = ?", undef, $userid);

> 
> What about $old_watch_ids for consistency? "_now" is ambiguous, because I
read
> it as "now that I have updated my list", which is clearly not the case here.

  Oh, good idea! I've updated it. :-)

> Funny way to do this. But I'm not sure it is optimal for long arrays. What
> about sorting them first so that you don't need to restart from the beginning

> of the @new array everytime?

  Because this is exactly the way the old code did it, and I don't want to
modify it. You'll notice that I just copied this code out of diff_strings,
which is currently working, and rarely has to work on long arrays.

  Premature optimization is the root of all evil.


  I fixed the bitrot. :-)

Comment 7

[Frédéric Buclin]

Comment on attachment 175358 [details] [diff] [review]
v2

Bye bye RelationSet.pm! :)

r=LpSolit

Comment 8

[Max Kanat-Alexander]

Checking in userprefs.cgi;
/cvsroot/mozilla/webtools/bugzilla/userprefs.cgi,v  <--  userprefs.cgi
new revision: 1.68; previous revision: 1.67
done
Checking in Bugzilla/Util.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Util.pm,v  <--  Util.pm
new revision: 1.21; previous revision: 1.20
done