348477 - Move simple validations from post_bug.cgi to Bugzilla::Bug

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, enhancement)

Description

[Max Kanat-Alexander]

There's a lot of very simple validations that happen in post_bug.cgi, particularly toward the beginning. For example, bug_file_loc becomes the empty string if it's 'http://'.

I could move these all in different bugs, but they're so simple, I might as well just move them all at the same time.

Comment 1

[Max Kanat-Alexander]

Attachment: v1

Okay, here we go. These are only simple validations. I've tested it somewhat thoroughly, and make sure that post_bug still works properly.

You may ask yourself, "Why are we calling private functions from a CGI?" It's because eventually all of this stuff will happen inside Bugzilla::Bug->create, but for now it's happening manually inside of post_bug.cgi.

Comment 2

[Olav Vitters]

Whoops.. the review:

Haven't fully tested the changes yet.

>Index: post_bug.cgi
>===================================================================
>RCS file: /cvsroot/mozilla/webtools/bugzilla/post_bug.cgi,v
>retrieving revision 1.160
>diff -u -r1.160 post_bug.cgi
>--- post_bug.cgi	11 Aug 2006 23:45:07 -0000	1.160
>+++ post_bug.cgi	13 Aug 2006 00:21:05 -0000

>@@ -184,26 +168,18 @@

>+    my $alias = Bugzilla::Bug::_check_alias($cgi->param('alias'));
>+    if ($alias) {
>+        $cgi->param('alias', $alias);
>+        push (@bug_fields,"alias");

Nit: Doesn't follow Bugzilla code style; make it: push(@bug_fields, "alias");

>+    }
> }
> 

>@@ -364,12 +335,6 @@
>                                  estimated_time, remaining_time, deadline)
>                VALUES ($sql_placeholders ?, ?, ?, ?, ?)};
> 
>-$comment =~ s/\r\n?/\n/g;     # Get rid of \r.
>-$comment = trim($comment);

This trim was forgotten in _check_comment.

>Index: Bugzilla/Bug.pm
>===================================================================
>RCS file: /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Bug.pm,v
>retrieving revision 1.132
>diff -u -r1.132 Bug.pm
>--- Bugzilla/Bug.pm	11 Aug 2006 23:45:08 -0000	1.132
>+++ Bugzilla/Bug.pm	13 Aug 2006 00:21:06 -0000
>@@ -238,6 +238,114 @@
>     return $self;
> }
> 
>+#####################################################################
>+# Validators
>+#####################################################################
>+
>+sub _check_alias {
>+   my ($alias) = @_;
>+   $alias = trim($alias);
>+   ValidateBugAlias($alias) if ($alias);

This would allow an alias of '0'. Fortunately post_bug.cgi changes ensure this alias will not actually be used, but it is still not good. I think an '0' would be ignored before your patch too, but it is wrong to let it pass.

>+   return $alias;
>+}

>+sub _check_comment {
>+    my ($comment) = @_;
>+
>+    if (!defined $comment) {
>+        ThrowCodeError('undefined_field', { field => 'comment' });
>+    }
>+
>+    # Remove any trailing whitespace. Leading whitespace could be
>+    # a valid part of the comment.
>+    $comment =~ s/\s*$//s;
>+    $comment =~ s/\r\n?/\n/g; # Get rid of \r.

Missing: $comment = trim($comment);

>+    ValidateComment($comment);
>+
>+    if (Bugzilla->params->{"commentoncreate"} && !$comment) {
>+        ThrowUserError("description_required");
>+    }
>+
>+    return $comment;
>+}
>+
>+sub _check_component {
>+    my ($name, $product) = @_;
>+    $name = trim($name);
>+    $name || ThrowUserError("require_component");
>+    my $obj = Bugzilla::Component::check_component($product, $name);
>+    # XXX Right now, post_bug needs this to return an object. However,
>+    # when we move to Bugzilla::Bug->create, this should just return
>+    # what it was passed.

Yuck (same as _check_product). Also wonder if the _check_component should be here at all. Maybe instead just push all logic into Bugzilla::Component::check_component and reference that from the create function?

>+    return $obj;
>+}
>+
>+sub _check_product {
>+    my ($product) = @_;
>+    # Check that the product exists and that the user
>+    # is allowed to enter bugs into this product.
>+    Bugzilla->user->can_enter_product($product, 1);

What is this 1? Shouldn't that be THROW_ERROR? Oh, was already there.. should still be fixed.

>+    my $obj = Bugzilla::Product::check_product($product);
>+    # XXX Right now, post_bug needs this to return an object. However,
>+    # when we move to Bugzilla::Bug->create, this should just return
>+    # what it was passed.
>+    return $obj;

Yuck! Thought about making you return $obj->name, except that is ugly as well (constructing the same object right after each other). Seems to me with THROW_ERROR, can_enter_product does everything that check_product does and more?

>+}
>+
>+sub _check_short_desc {
>+    my ($short_desc) = @_;
>+    # Set the parameter to itself, but cleaned up
>+    $short_desc = clean_text($short_desc);

That could theoretically generate a 'Use of uninitialized value in substitution' if the short_desc cgi param did not exist (received undef).

>+    if (!defined $short_desc|| $short_desc eq '') {
>+        ThrowUserError("require_summary");
>+    }
>+    return $short_desc;
>+}

Comment 3

[Max Kanat-Alexander]

(In reply to comment #2)
> Nit: Doesn't follow Bugzilla code style; make it: push(@bug_fields, "alias");

  Oh yeah, it was like that. :-) I didn't even notice! I'll fix it.

> >-$comment = trim($comment);
> 
> This trim was forgotten in _check_comment.

  No, it was purposely left out. :-) We shouldn't be trimming leading whitespace from comments--that was a bug. See the comment in _check_comment. 

> This would allow an alias of '0'. [snip]

  Ah yeah, you're right. The trick is that if the validator is passed undef, it should return undef, so that's why I didn't check "eq ''". I'll fix this, though.

> Also wonder if the _check_component should be here at all.

  No, it should be here. Don't worry about it.

> Maybe instead just push all logic into
> Bugzilla::Component::check_component and reference that from the create
> function?

  The validators need to live here, it just makes life easier.

> What is this 1? Shouldn't that be THROW_ERROR? Oh, was already there.. should
> still be fixed.

  Good point. :-)

> Yuck! Thought about making you return $obj->name, except that is ugly as well
> (constructing the same object right after each other). Seems to me with
> THROW_ERROR, can_enter_product does everything that check_product does and
> more?

  I don't know. It doesn't matter all that much. Bug creation doesn't have to be efficient, we're not creating hundreds of bugs in a row.

  I think it may actually turn out fine to return the object like this, based on how I'm going to implement Bugzilla::Bug->create.

> That could theoretically generate a 'Use of uninitialized value in
> substitution' if the short_desc cgi param did not exist (received undef).

  Ah, true. :-) I'll fix it.

Comment 4

[Max Kanat-Alexander]

Attachment: v2

Okay, here's the version with all your comments fixed. And I checked, and made sure that it doesn't allow an alias of "0" now. Because of how ValidateBugAlias works, it throws a funny error message, but ValidateBugAlias has always worked that way, so that's nothing new.

Comment 5

[Olav Vitters]

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

>Index: post_bug.cgi

>+    my $alias = Bugzilla::Bug::_check_alias($cgi->param('alias'));
>+    if ($alias) {
>+        $cgi->param('alias', $alias);
>+        push (@bug_fields, "alias");

Nit: Still an extra space after push.

Passes my tests. r=bkor

Comment 6

[Max Kanat-Alexander]

Thanks, bkor! I fixed the nit on checkin.

Checking in post_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/post_bug.cgi,v  <--  post_bug.cgi
new revision: 1.162; previous revision: 1.161
done
Checking in Bugzilla/Bug.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Bug.pm,v  <--  Bug.pm
new revision: 1.136; previous revision: 1.135
done