Bug changes with intermediate pages munges fields with multiple values (e.g., CC)

RESOLVED FIXED in Bugzilla 2.18

Status

()

P1
critical
RESOLVED FIXED
16 years ago
6 years ago

People

(Reporter: randall_gee_51227124, Assigned: randall_gee_51227124)

Tracking

({dataloss})

2.14
Bugzilla 2.18
dataloss

Details

(Whiteboard: [fixed in 2.16.1] [FIXED ON TRUNK])

Attachments

(6 attachments, 2 obsolete attachments)

(Assignee)

Description

16 years ago
Sometimes, when changing a bug, Bugzilla has to display an intermidiate
page.  Examples include specifying the component/version/milestone when a
product is changed, the user logging in, resolving "mid-air collisions",
and confirming duplicates.

In that intermediate page, Bugzilla needs to save the changes the user
wanted to make.  However, for fields with multiple values, e.g., the CC
lists, these values are munged.  All the values are concatenated together
into one value.

To see this in action, take a bug with CCs.  Change the product
and remove a couple of CCs at the same time, so you get the "verify
component/version/milestone" page.  Once you specify these values, you
get an error, because "user1@domain.comuser2@domain.netuser3@example.org"
is not a registered Bugzilla account.

Note that this was noticed in bug 101056.  But it was only noticed
and fixed for the case where this user has to log in before committing
his change.  It still exists in the other cases.

I discovered this bug in 2.14, but it still exists in 2.16.

I have a fix for this, and I will be submitting them.
(Assignee)

Comment 1

16 years ago
Created attachment 94103 [details] [diff] [review]
Patch to write %::MFORM values, if the field has multiple values

The problem is that the intermediate pages write out the values in %::FORM
as hidden fields.  Unfortunately, %::FORM just concatenates multiple values.
To get all the values, you need to look at %::MFORM.  This patch will write
the values of %::MFORM{"field"} if "field" contains multiple values.
(Assignee)

Comment 2

16 years ago
Created attachment 94104 [details] [diff] [review]
Patch to write %::MFORM values, instead of %::FORM values

Now, you may notice that the preceding patch checks $::MFORM{"field"}, to
see if it has multiple values, but writes $::FORM{"field"} if it doesn't.

That's a bit weird, because if it doesn't, $::MFORM{"field"} should still
contain the same thing as $::FORM{"field"}, just in list form.	So this patch
always uses %::MFORM, and completely ignores %::FORM.

I'm submitting both patches, because the fix for the case where the user
has to log-in uses both %::FORM and %::MFORM, so I felt that one patch
should use that method.  Also, it's possible that some scripts re-write
%::FORM values, so checking both values may just being cautious.  (But,
if you're re-writing %::FORM, wouldn't the new values be what you want to
save anyway?  It seems excessively cautious.)
(Assignee)

Comment 3

16 years ago
Created attachment 95135 [details] [diff] [review]
Patch to write %::MFORM values, instead of %::FORM values (corrected)

Bleah.	I reversed the second patch, and left some debugging in.  I've
corrected it now.
Attachment #94104 - Attachment is obsolete: true
(Assignee)

Comment 4

16 years ago
Created attachment 95136 [details] [diff] [review]
Patch to write %::MFORM values, instead of %::FORM values (really corrected)

What the...?!  (splutter)  OK, this time I'm really correcting the second
patch.
Attachment #95135 - Attachment is obsolete: true
Severity: normal → critical
Keywords: dataloss
Priority: -- → P1
Whiteboard: [want for 2.16.1]
Target Milestone: --- → Bugzilla 2.18
OK, to what extent are things being rewritten in %::FORM?  I'm sure the correct
approach is to use %::MFORM if there's more than one value and %::FORM if not,
since it's quite possible the scripts might be doing some internal rewriting of
things.

Apparently things have changed enough since 2.16 that this won't apply to the
trunk.  We'll need it for both anyway, this is worth backporting to the 2.16
branch I think. (well, the existing patch applies cleanly there anyway, we'll
have to port it to the trunk)

Comment 6

16 years ago
Comment on attachment 95136 [details] [diff] [review]
Patch to write %::MFORM values, instead of %::FORM values (really corrected)


I'm needs-working this for two reasons, but I'll be happy to reconsider when I
get an answer to my questions.

>diff -ur bz216box/bugzilla-2.16/CGI.pl CGI.pl
>--- bz216box/bugzilla-2.16/CGI.pl	Tue Jul  9 23:27:15 2002
>+++ CGI.pl	Mon Aug  5 11:08:19 2002
>@@ -824,15 +824,10 @@
>         # Add all the form fields into the form as hidden fields
>         # (except for Bugzilla_login and Bugzilla_password which we
>         # already added as text fields above).
>-        foreach my $i ( grep( $_ !~ /^Bugzilla_/ , keys %::FORM ) ) {
>-          if (defined $::MFORM{$i} && scalar(@{$::MFORM{$i}}) > 1) {
>-            # This field has multiple values; add each one separately.
>-            foreach my $val (@{$::MFORM{$i}}) {
>-              print qq|<input type="hidden" name="$i" value="@{[value_quote($val)]}">\n|;
>-            }
>-          } else {
>-            # This field has a single value; add it.
>-            print qq|<input type="hidden" name="$i" value="@{[value_quote($::FORM{$i})]}">\n|;
>+        foreach my $i ( grep( $_ !~ /^Bugzilla_/ , keys %::MFORM ) ) {
>+          # This field has multiple values; add each one separately.
>+          foreach my $val (@{$::MFORM{$i}}) {
>+            print qq|<input type="hidden" name="$i" value="@{[value_quote($val)]}">\n|;
>           }
>         }

Is there a reason you're changing this code in CGI.pl?

The way I'm reading it, your code may be slightly faster, but it doesn't fix
any bug or change the logic of the code at all.

Is that correct?

Also, this patch doesn't apply cleanly to the trunk; this will probably get
applied to a 2.16.1 when we get ready to ship that, but it will also need to
apply cleanly to the trunk.

You'll probably have to have 2 versions of this patch: a 2.16 branch version
and a trunk version.
Attachment #95136 - Flags: review-
Bleh. It looks like bug 101056 is regressed on the trunk, too. (Which is why
this doesn't apply - bug 158660 templatised the login stuff)

Anyway, this looks fine, I guess. The first comment says that this happens in
2.14 too, though - do we want this for 2.16?

Comment 8

16 years ago
... or 2.14.4, for that matter...
(Assignee)

Comment 9

16 years ago
Repsonding to comment 6...

The change to the code in CGI.pl does change the logic.  (Whether or not
it fixes any bug is another story.)

In attachment 95136 [details] [diff] [review], I'm using the principle that the %::MFORM values
are the ones that should be preserved in the intermediate page, not the
%::FORM values.  So I changed CGI.pl to save only the %::MFORM values
when putting up the login page.

In attachment 94103 [details] [diff] [review], I use the "save single-valued %::FORM values, and
multiply-valued %::MFORM values", which is what was already in CGI.pl,
so there isn't any change.

I'll take a look at the trunk to see what needs to be done to get this
patch working for it.  I'll make sure bug 101056 is re-fixed in the
trunk, too.

I do have a patch for this against 2.14.1-3 as well, if you want it for
2.14.4.
Yeah, the patch looks right, because %FORM values are in %MFORM too. The form
code sucks...
(Assignee)

Comment 11

16 years ago
Created attachment 95640 [details] [diff] [review]
Patch against trunk, write %::MFORM only if multi-valued

This patch goes against the trunk, or whatever it is I got with a
cvs update -A -dP.  It uses the logic of "write %::FORM, unless
%::MFORM has multiple values, in which case, write %::MFORM."
(Assignee)

Comment 12

16 years ago
Created attachment 95641 [details] [diff] [review]
Patch against trunk, write %::MFORM always

This patch also goes against the trunk, but it always writes
the %::MFORM values and ignores the %::FORM values.
(Assignee)

Comment 13

16 years ago
Created attachment 95642 [details] [diff] [review]
Patch against 2.14.3

This patch goes against 2.14.3 (and should fit with the other 2.14 versions.)
Comment on attachment 95641 [details] [diff] [review]
Patch against trunk, write %::MFORM always

This won't work, because 'empty' values are only put into $FORM, not $MFORM in
CGI.pl

All together now...'YUCK!'
Attachment #95641 - Flags: review-
Comment on attachment 95640 [details] [diff] [review]
Patch against trunk, write %::MFORM only if multi-valued

This one seems to work

r=bbaetz, but I want a second reviewer to sanity check this.
Attachment #95640 - Flags: review+
Comment on attachment 95640 [details] [diff] [review]
Patch against trunk, write %::MFORM only if multi-valued

r=myk

This is the right solution, and it looks good and works.
Attachment #95640 - Flags: review+
This has been checked in on the TRUNK.

Dave - you marked this as [wanted for 2.16.1], but its not a regression there;
the only regression part of this was trunk only. Do you still want this for the
branch?

-> patch author, in any event
Assignee: myk → randall_gee_51227124
Whiteboard: [want for 2.16.1] → [want for 2.16.1] [FIXED ON TRUNK]
Created attachment 97580 [details] [diff] [review]
2.16 patch

Can I get a rubberstamping for this? I jsut removed the changes for the login
template, which doesn't exist in 2.16
Comment on attachment 97580 [details] [diff] [review]
2.16 patch

Don't know how valid my rubber stamp is, but it looks good.

What about 2.14.3? Was that ever checked in? Did it need to be?
Attachment #97580 - Flags: review+
No, it wasn't, and we're not going to be fixing this sort of bug for 2.14, esp
since it took this long for someone to notice.

Checked in to 2.16
Status: NEW → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
Whiteboard: [want for 2.16.1] [FIXED ON TRUNK] → [fixed in 2.16.1] [FIXED ON TRUNK]
*** Bug 183992 has been marked as a duplicate of this bug. ***
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.