delta_ts is updated even when no changes are made to bugs created via WebServices

RESOLVED FIXED in Bugzilla 4.2

Status

()

Bugzilla
Creating/Changing Bugs
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: Frédéric Buclin, Assigned: dkl)

Tracking

4.1.3
Bugzilla 4.2
Bug Flags:
approval +
approval4.2 +
blocking4.2 +
blocking4.0.3 -

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

7 years ago
This problem only appears with "old" bugs (I didn't try to define "old" yet). I realized that when clicking the "Save Changes" button in show_bug.cgi but doing no changes at all, delta_ts was updated anyway. Data::Dumper showed me that the reason is that %$changes is not empty, which triggers delta_ts to be updated:

$VAR1 = {
          'bug_file_loc' => [
                              undef,
                              ''
                            ],
          'cf_reporter' => [
                             undef,
                             ''
                           ]
        };

cf_reporter is a free text custom field I created before 3.4. For a reason I don't know yet, these two fields are converted from undef to an empty string. This problem doesn't appear in 4.0.2.
Flags: blocking4.2+
(Reporter)

Comment 1

7 years ago
I take that back. I can reproduce the problem on 4.0.2 too. Still investigating. Maybe Bugzilla 3.0 or 3.2 allowed these fields to be undefined?
(Reporter)

Comment 2

7 years ago
Ah, in fact the problem is unrelated to old or new bugs. The problem appears with bugs created using the Bug.create() WebService. In that case, _check_bug_file_loc() is not triggered (unless you specify this field, of course) and NULL is inserted into the DB. But once you edit this bug from the UI, an empty URL field is passed to process_bug.cgi and so it triggers _check_bug_file_loc() which returns an empty string. The same happens for free text custom fields. IMO, empty strings should be replaced by NULL in the DB.
Flags: blocking4.0.3?
Keywords: regression
Summary: delta_ts is updated even when no changes are made to an "old" bug → delta_ts is updated even when no changes are made to bugs created via WebServices

Comment 3

7 years ago
Ah, interesting. One way or another, I want to be consistent with what we do. I think probably it's simpler to make bug_file_loc DEFAULT '' NOT NULL instead of allowing nulls. 

For custom fields, it's more debatable, I suppose. The only time I'd actually (at some point) like to see NULL in the DB is for hidden fields or fields that actually aren't allowed to be set yet (for example, fields that couldn't have been set on enter_bug). But then again, for search reasons, maybe we want to be consistent even with those, and make them empty strings instead.
(Assignee)

Updated

7 years ago
Assignee: create-and-change → dkl
(Assignee)

Comment 4

7 years ago
Created attachment 577492 [details] [diff] [review]
Patch to update certain field definitions to not null with defaults (v1)

First crack at a patch for this issue. This fixes the error originally reported by LpSolit.

dkl
Attachment #577492 - Flags: review?(mkanat)

Comment 5

7 years ago
Comment on attachment 577492 [details] [diff] [review]
Patch to update certain field definitions to not null with defaults (v1)

Review of attachment 577492 [details] [diff] [review]:
-----------------------------------------------------------------

Hmm, okay. I suppose we're *not* using the NULL-ability of any of the custom fields for any good purpose *right now*, and this does make our code more consistent *right now*, so let's do it.

One fix, though, and then we're good:

::: Bugzilla/Install/DB.pm
@@ +3624,5 @@
> +                          {TYPE => 'MEDIUMTEXT', NOTNULL => 1,  
> +                           DEFAULT => "''"}, '');
> +
> +    my $custom_fields = $dbh->selectall_arrayref(
> +        'SELECT name, type FROM fielddefs WHERE custom = 1');

You can safely use Bugzilla::Field instead here, actually. It's pretty much the one object you can use during Install::DB. That way you could just do a match() against the two types you're looking for and not have to select all of fielddefs (which I'd like to avoid since this code is going to have to run every time checksetup runs).
Attachment #577492 - Flags: review?(mkanat) → review-
(Assignee)

Comment 6

7 years ago
Created attachment 579323 [details] [diff] [review]
Patch to update certain field definitions to not null with defaults (v2)

Patch with suggested change.

dkl
Attachment #577492 - Attachment is obsolete: true
Attachment #579323 - Flags: review?(mkanat)
(Reporter)

Comment 7

7 years ago
Comment on attachment 579323 [details] [diff] [review]
Patch to update certain field definitions to not null with defaults (v2)

Looks good to me, and works fine. r=LpSolit
Attachment #579323 - Flags: review?(mkanat) → review+
(Reporter)

Comment 8

7 years ago
dkl and glob think we shouldn't take it for 4.0.3, and I have no strong opinion on this, so let's take if for 4.2 only to avoid a DB schema change on a stable branch.
Status: NEW → ASSIGNED
Flags: blocking4.0.3?
Flags: blocking4.0.3-
Flags: approval4.2+
Flags: approval+
(Assignee)

Comment 9

7 years ago
trunk:
Committing to: bzr+ssh://dlawrence%40mozilla.com@bzr.mozilla.org/bugzilla/trunk
modified Bugzilla/Field.pm
modified Bugzilla/DB/Schema.pm
modified Bugzilla/Install/DB.pm
Committed revision 8045

4.2:
Committing to: bzr+ssh://dlawrence%40mozilla.com@bzr.mozilla.org/bugzilla/4.2
modified Bugzilla/Field.pm
modified Bugzilla/DB/Schema.pm
modified Bugzilla/Install/DB.pm
Committed revision 7983.

dkl
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.