André Batosti Reporter
	Description • 19 years ago

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.10) Gecko/20050725 Firefox/1.0.6 (Ubuntu package 1.0.6)
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.10) Gecko/20050725 Firefox/1.0.6 (Ubuntu package 1.0.6)

We need methods and/or subroutines for validations and database persistence.
All related routines about inserts, updates, deletes and validations for those
routines have to be implemented at this bug.

Reproducible: Always

	Hermann Schwab
	Comment 1 • 19 years ago

*** This bug has been marked as a duplicate of 313126 ***

	Olav Vitters
	Comment 2 • 19 years ago

Not a dupe.

	Hermann Schwab
	Comment 3 • 19 years ago

(In reply to comment #1)
> 
> *** This bug has been marked as a duplicate of 313126 ***

Sorry, I compared both bugs by switching tabs and didn't find a difference. The difference is in the second, hidden part of the summary, seen only in fine print in the title, which I obviously hadn't included in this comparison.
Wouldn't have happened in the previous version of bugzilla, I'm allergic to fine print, but I'll look at bug titles next time, not only at the visible part of the summary.

	Frédéric Buclin Assignee
	Comment 5 • 19 years ago

Comment on attachment 200925 [details] [diff] [review]
batosti_v1

This patch no longer applies due to the checkin of bug 311258.

	Frédéric Buclin Assignee
	Comment 7 • 19 years ago

Comment on attachment 213888 [details] [diff] [review]
batosti_v2

>Index: Bugzilla/Milestone.pm

>+use Bugzilla::Config qw($datadir);

Do not call Bugzilla::Config from here. $datadir/versioncache should not be deleted from here.


>+sub remove_from_db {

>+    if ($self->bug_count) {
>+        my $bug_ids =
>+            $dbh->selectcol_arrayref(q{SELECT bug_id FROM bugs
>+                                       WHERE product_id = ? AND 
>+                                       target_milestone = ?},
>+                                     undef, 
>+                                     ($self->product->id, $self->name));

Nit: maybe should we implement a Bugzilla::Version::bug_ids() method.


>+    unlink "$datadir/versioncache";

As said above, do not delete versioncache from here. Let the caller do it.


>+sub update {
>+    my $self = shift;
>+    my ($name, $sortkey) = (@_);

@_ is an array. Write ($foo, $bar) = @_;


>+    if ($name && $name ne $self->name) {
>+        if (length($name) > 20) {
>+            ThrowUserError('milestone_name_too_long',
>+                           {'name' => $name});
>+        }

This check is done when creating and updating a milestone. So you should write a separate routine for this check (e.g. check_name_length).


>+            ThrowUserError('milestone_already_exists',
>+                           {'name' => $milestone->name,
>+                            'product' => $milestone->product->name});

Write $product->name instead. Maybe $product should be passed as an argument to update()?


>+        $dbh->do(q{UPDATE products SET defaultmilestone = ?
>+                   WHERE id = ? AND defaultmilestone = ?},
>+                 undef, $name, $product->id, $self->name);
>+
>+    }
>+
>+    if ($sortkey && $sortkey ne $self->sortkey) {
>+        $sortkey = Bugzilla::Milestone::check_sort_key($self->name,
>+                                                       $sortkey);

*All* checks must be done *before* altering the DB. If check_sort_key() throws an error, then you have a partial update as the milestone name has already been updated. That's bad.


>+        $dbh->do(q{UPDATE milestones SET sortkey = ?
>+                  WHERE product_id = ? AND value = ?},
>+                 undef, $sortkey, $self->product->id, $self->name);

$self->product is not a valid method. Use $product instead.


>+sub create_milestone {

>+    if (length($name) > 20) {
>+        ThrowUserError('milestone_name_too_long',
>+                       {'name' => $name});
>+    }

As said above, this check should be in a separate routine.


> 1;
> 
> __END__

Do not forget to add POD too.

	Frédéric Buclin Assignee
	Comment 8 • 19 years ago

Oh, and keep the table lock when updating the milestone. This lock must be set in editmilestones.cgi, not in Milestone.pm.

	Frédéric Buclin Assignee
	Comment 10 • 19 years ago

Comment on attachment 217924 [details] [diff] [review]
batosti_v2_fix

>Index: Bugzilla/Milestone.pm

>+use Bugzilla::Bug;

Argh! We are going to introduce a new dependency loops, involving Milestone -> Bug -> User -> Product -> Milestone. We definitely need a Bug::Util module (there is a bug for that).


>+sub bug_ids {

>+    my $product = new Bugzilla::Product($self->product_id);
>+
>+    $self->{'bug_ids'} =
>+        $dbh->selectcol_arrayref(q{SELECT bug_id FROM bugs
>+                                    WHERE product_id = ?
>+                                      AND target_milestone = ?},
>+                                 undef, ($product->id, $self->name));

Huh? Creating a product object to use its ID you just used to create this object is pretty useless!! Use $self->product_id directly.


>+sub remove_from_db {

>+    my $product = new Bugzilla::Product($self->product_id);

Better is to pass the product object as argument IMO.


>+sub update {

>+    my $product = new Bugzilla::Product($self->product_id);

Here again, passing the product object would be useful.


>+    my $sortkey_obj = Bugzilla::Milestone::check_sort_key($self->name,
>+                                                          $sortkey)

Huh?? check_sort_key() doesn't return an object, but the detainted sortkey itself. So no need for a new variable here. Moreover, we should fall back to 0 if no sortkey was given.

Another point: no need to append Bugzilla::Milestone:: as check_sort_key() is defined for the whole package.


>+    if ($name && $name ne $self->name) {
>+        check_name($name);

check_name should return the detained name.


>+        trick_taint($name);

As said above, this should be done by check_name() as part of the validation process.


>+    if ($sortkey_obj) {
>+
>+        detaint_natural($sortkey);

$sortkey_obj is detainted already (but use only one variable!).


>+sub check_name {
>+    my $name = shift;
>+
>+    $name || ThrowUserError('milestone_blank_name');
>+
>+    if (length($name) > 20) {
>+        ThrowUserError('milestone_name_too_long',
>+                       {'name' => $name});
>+    }
>+}

Return the detainted name.


>+    $milestones->remove_from_db;

$milestone without the trailing 's'. You cannot remove several milestones at once.


>+=item C<bug_ids()>
>+
>+ Description: Create a array of bug ids of this milestone.

"Creates an array of bug IDs having this milestone" or something like that.


>+ Returns:     A reference of a integer array.

"A reference to an array of bug IDs."


>+=item C<remove_from_db()>
>+
>+ Description: Removes the milestone of the database and set milestones
>+              of this milestones bugs as default milestone.

"Removes the milestone from the database and reassigns bugs having this milestone to the default milestone of the product."


>+=item C<check_name($name)>
>+
>+ Description: Check if the milestone name is not null and is minor than 20.

"[...] and is shorter than 20 characters."


This patch is pretty close to what we want. Maybe the next patch will be the final one. ;)

	André Batosti Reporter
	Comment 11 • 18 years ago

(In reply to comment #10)
> (From update of attachment 217924 [details] [diff] [review] [edit])
> >Index: Bugzilla/Milestone.pm
> 
> >+use Bugzilla::Bug;
> 
> Argh! We are going to introduce a new dependency loops, involving Milestone ->
> Bug -> User -> Product -> Milestone. We definitely need a Bug::Util module
> (there is a bug for that).
> 
> 

What i do here? wait for the Bug::Util?

	Frédéric Buclin Assignee
	Comment 12 • 18 years ago

We are in "soft freeze" mode to prepare 3.0 RC1.

	Max Kanat-Alexander
	Comment 14 • 17 years ago

Comment on attachment 284037 [details] [diff] [review]
patch, v3

Milestone.pm
------------
>+sub _check_value {
>+    my ($invocant, $name, $product) = @_;
>+
>+    $name || ThrowUserError('milestone_blank_name');

  You should trim($name) first.

>+    if (length($name) > 20) {
>+        ThrowUserError('milestone_name_too_long', {'name' => $name});
>+    }

  Make that 20 a constant either here or in Bugzilla::Constants.

>+    my $milestone = new Bugzilla::Milestone({product => $product, name => $name});
>+    if ($milestone && (!ref $invocant || $milestone->id != $invocant->id)) {
>+        ThrowUserError('milestone_already_exists', { 'name'    => $milestone->name,
>+                                                     'product' => $product->name });

  Nit: You don't have to quote 'name' and 'product'.

>+    trick_taint($name);

  That is unnecessary. Bugzilla::Object itself should run trick_taint against any validated value. If there's a taint error somewhere, even with a validator, that's an error.

>+    if (!detaint_signed($sortkey) || $sortkey < -32768 || $sortkey > 32767) {

  Those numbers should be constants. You can call them MAX_SMALLINT and MIN_SMALLINT, perhaps.

>+sub set_value { $_[0]->set('value', $_[1]); }

  This should be set_name instead. (Since we're calling the accessor as "name".)

>@@ -101,11 +242,8 @@ sub check_sort_key {

  There's no reason to have this twice. Is this subroutine even used anymore?

>Index: template/en/default/global/user-error.html.tmpl
>+    Sorry, but [% milestone.name FILTER html %] is the default milestone
>+    for product '[% milestone.product.name FILTER html %]', and so
>+    it cannot be deleted.

  While we're here, "for the %product% product" is better English.

  Everything else looks great! This is a great patch. :-)

	Frédéric Buclin Assignee
	Comment 15 • 17 years ago

(In reply to comment #14)
> >+    if (length($name) > 20) {
> >+        ThrowUserError('milestone_name_too_long', {'name' => $name});
> >+    }
> 
>   Make that 20 a constant either here or in Bugzilla::Constants.

This doesn't make sense to set it as a constant. Bugzilla::DB::Schema has this value hardcoded varchar(20), so that's not something you can freely change.


> >+    trick_taint($name);
> 
>   That is unnecessary. Bugzilla::Object itself should run trick_taint against
> any validated value. If there's a taint error somewhere, even with a validator,
> that's an error.

Well, without this trick_taint() call here, I get a taint error when it tries to insert the milestone in the DB.


> >@@ -101,11 +242,8 @@ sub check_sort_key {
> 
>   There's no reason to have this twice. Is this subroutine even used anymore?

Right, that's old code which I copied into set_sortkey(), and then I forgot to remove it. Heh.

	Frédéric Buclin Assignee
	Comment 16 • 17 years ago

I found why $milestone->name is still tainted: Object::update() is the one detaining values before inserting them into the DB. But Milestone::update() calls $milestone->name to also update the bugs table, but Object::set() doesn't detaint the value before updating $milestone->{name}, generating a taint issue. Maybe Object::set() should detaint the value itself.

	Max Kanat-Alexander
	Comment 17 • 17 years ago

(In reply to comment #15)
> This doesn't make sense to set it as a constant. Bugzilla::DB::Schema has this
> value hardcoded varchar(20), so that's not something you can freely change.

  Yeah, but it's bad programming practice to have "magic numbers" in code. Constants are the usual way of making the code make more sense.

(In reply to comment #16)
  Hrm. I'll have to think about this a bit. The problem is that when you trick_taint something, it's detainted forever. Usually I detaint something right before inserting it into the DB, because that's when I know it's safe *for the DB*.

  The same string could be dangerous in other circumstances, like as an argument to Email::Send or in a system() call (or otherwise being passed to the system). 

  I don't want Object::set to detaint something, because we have no idea where that variable is going afterward, and I want it to throw a taint error if it goes somewhere it shouldn't.

	Frédéric Buclin Assignee
	Comment 18 • 17 years ago

(In reply to comment #17)
>   I don't want Object::set to detaint something, because we have no idea where
> that variable is going afterward, and I want it to throw a taint error if it
> goes somewhere it shouldn't.

If the caller calls new(), you get a detainted object anyway, so this is a bad argument from a security point of view. For me, a detainted value means the value passed validation checks. Then that's the job of the caller to do the right thing with it when calling, e.g., system(), or to use placeholders in SQL queries, or to use $dbh->quote(), etc...

	Frédéric Buclin Assignee
	Comment 19 • 17 years ago

I fixed all your comments. I also let Object->set() detaint values if the validator checked the field value successfully. This way, we are consistent with how Object->run_create_validators() works.

	Max Kanat-Alexander
	Comment 20 • 17 years ago

Comment on attachment 284167 [details] [diff] [review]
patch, v3.1

Yes, this looks good now.

A few comments:

1) The grouping of the constants in @Bugzilla::Constants::EXPORT doesn't make any sense. Also, they constants should be grouped in @EXPORT in the same order that they show up in the code, ideally.

2) In every other create() statement in Bugzilla, we use the DB name of the column--it's our one exception. So I think you should use "value" in create(), and if we want to make that "name" in the future, we can do it *everywhere* in Bugzilla, instead of being inconsistent in one place.

	Frédéric Buclin Assignee
	Comment 21 • 17 years ago

(In reply to comment #20)
> 2) In every other create() statement in Bugzilla, we use the DB name of the
> column--it's our one exception.

It doesn't make sense to build a milestone object passing "name" and "product" as arguments, update the milestone name using set_name(), but creating a new milestone with "value" and "product" as arguments. That's simply confusing. The reason other Bugzilla modules use the name of the column is that the DB already contains has a "name" column instead of a "value" column. Rather than wanting to stick on the DB column name, I think we should rather be consistent in the API and use "name" instead.

About Constants.pm, it's a mistake from me. I will group them the same way in EXPORT and in the file itself on checkin.

Please approve this bug if you are fine to commit this patch with "name" instead of "value" in create().

	Max Kanat-Alexander
	Comment 22 • 17 years ago

I'm fine with it, but there are other modules that use the DB name of the column instead of the accessor name--for example, Bug::create uses the DB names of columns. I suppose it should use the accessor names instead, and we should have some system in Bugzilla::Object to map them.

	Frédéric Buclin Assignee
	Comment 23 • 17 years ago

Checking in editmilestones.cgi;
/cvsroot/mozilla/webtools/bugzilla/editmilestones.cgi,v  <--  editmilestones.cgi
new revision: 1.59; previous revision: 1.58
done
Checking in Bugzilla/Constants.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Constants.pm,v  <--  Constants.pm
new revision: 1.81; previous revision: 1.80
done
Checking in Bugzilla/Milestone.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Milestone.pm,v  <--  Milestone.pm
new revision: 1.10; previous revision: 1.9
done
Checking in Bugzilla/Object.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Object.pm,v  <--  Object.pm
new revision: 1.20; previous revision: 1.19
done
Checking in template/en/default/admin/milestones/updated.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/milestones/updated.html.tmpl,v  <--  updated.html.tmpl
new revision: 1.5; previous revision: 1.4
done
Checking in template/en/default/global/user-error.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/user-error.html.tmpl,v  <--  user-error.html.tmpl
new revision: 1.233; previous revision: 1.232
done

	Frédéric Buclin Assignee
	Comment 24 • 17 years ago

I checked in this patch which now correctly trims the milestone name.