Closed Bug 281550 Opened 17 years ago Closed 17 years ago

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

Categories

(Bugzilla :: Bugzilla-General, defect)

2.19.2
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Bugzilla 2.20

People

(Reporter: mkanat, Assigned: mkanat)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
Actually, I'll do it. We need this change, locally.
Assignee: general → mkanat
OK, here's the code, without RelationSet being used there.

I also added a nice function to Bugzilla::Util, diff_arrays, to facilitate
this.
Attachment #173802 - Flags: review?
Status: NEW → ASSIGNED
Blocks: 281590
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.
Summary: Bugzilla::RelationSet::generateSqlDeltas uses a non-ANSI INSERT syntax → Remove RelationSet from userprefs.cgi (and thus fix non-ANSI INSERT)
OK, here we go. Once again, it was pitifully simple and much more efficient to
not use RelationSet here.
Attachment #173802 - Attachment is obsolete: true
Attachment #173827 - Flags: review?
Attachment #173802 - Flags: review?
Attachment #173827 - Flags: review? → review?(LpSolit)
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.
Attachment #173827 - Flags: review?(LpSolit) → review-
Attached patch v2Splinter Review
(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. :-)
Attachment #173827 - Attachment is obsolete: true
Attachment #175358 - Flags: review?(LpSolit)
Comment on attachment 175358 [details] [diff] [review]
v2

Bye bye RelationSet.pm! :)

r=LpSolit
Attachment #175358 - Flags: review?(LpSolit) → review+
Flags: approval?
Target Milestone: --- → Bugzilla 2.20
Flags: approval? → approval+
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
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Blocks: 292117
You need to log in before you can comment on or make changes to this bug.