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

RESOLVED FIXED in Bugzilla 2.20

Status

()

defect
RESOLVED FIXED
15 years ago
14 years ago

People

(Reporter: mkanat, Assigned: mkanat)

Tracking

2.19.2
Bugzilla 2.20
Dependency tree / graph
Bug Flags:
approval +

Details

Attachments

(1 attachment, 2 obsolete attachments)

Assignee

Description

15 years ago
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.
Assignee

Comment 1

15 years ago
Actually, I'll do it. We need this change, locally.
Assignee: general → mkanat
Assignee

Comment 2

15 years ago
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?
Assignee

Updated

15 years ago
Status: NEW → ASSIGNED
Assignee

Updated

15 years ago
Blocks: 281590
Assignee

Comment 3

15 years ago
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)
Assignee

Comment 4

15 years ago
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?
Assignee

Updated

15 years ago
Attachment #173802 - Flags: review?
Assignee

Updated

15 years ago
Attachment #173827 - Flags: review? → review?(LpSolit)

Comment 5

15 years ago
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-
Assignee

Comment 6

15 years ago
Posted 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 7

15 years ago
Comment on attachment 175358 [details] [diff] [review]
v2

Bye bye RelationSet.pm! :)

r=LpSolit
Attachment #175358 - Flags: review?(LpSolit) → review+

Updated

15 years ago
Flags: approval?
Target Milestone: --- → Bugzilla 2.20
Flags: approval? → approval+
Assignee

Comment 8

15 years ago
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: 15 years ago
Resolution: --- → FIXED
Assignee

Updated

14 years ago
Blocks: 292117
You need to log in before you can comment on or make changes to this bug.