644256 - noresolveonopenblockers doesn't allow resolving blocker and dependent simultaneously

Status: NEW

(Bugzilla :: Creating/Changing Bugs, defect, P5)

Description

[Kent Rogers [:mrbball]]

User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.2.15) Gecko/20110308 Fedora/3.6.15-1.fc14 Firefox/3.6.15
Build Identifier: 4.1.1+

When noresolveonopenblockers is enabled and you have a blocker/dependent
relationship, you cannot simultaneously resolve both a blocker bug and
its dependent bug via 'Change Several Bugs at Once'.

You are forced to either remove the relationship or resolve the bugs one
at a time.  Frequently neither of these options is desirable, especially
when bugs are being automatically resolved from a source code control
system.


Reproducible: Always

Comment 1

[Max Kanat-Alexander]

This is true, but unlikely to be fixed due to the complexity of doing so.

Comment 2

[Kent Rogers [:mrbball]]

Perhaps that points to a deeper issue.  I wouldn't expect it to be so
complex.

Could Object::set() simply take a params argument and pass it along to
the validator?  That would appear to allow the flexibility to perform
additional checks in the validator, as necessary.

In this particular case, it would allow process_bug to add a hash of all
bugs being updated to the params arg on the calls to Bug::set_resolution()
and Bug::set_status() (which also calls set_resolution()), and then
Bug::set_resolution() would pass the hash on to Object::set().  Ultimately,
Bug::_check_resolution() would get the hash and NOT throw a "still_unresolved_bugs" error if all the unresolved bugs were in the hash.

Comment 3

[Max Kanat-Alexander]

  Hey Kent. Tracing down a dependency tree for every single dependency change on every single bug in a list probably isn't a good idea. You're welcome to look at the code of Bugzilla::Bug::set_all and see what you can do.

Comment 4

[Kent Rogers [:mrbball]]

Why would you need to trace down the whole tree?  Isn't _check_resolution()
called for every bug in a batch update?  If so, each bug only needs to
worry about its blockers, and the check simply needs to be added to the
current noresolveonopenblockers check there.

The only bit of information that _check_resolution() doesn't have is the
list of bugs being updated.  But, if Object::set() were updated to take a
params argument that allowed it to take additional information, it could
easily be passed along.

I was able to implement this fairly quickly on my local installation.
However, we are only running 3.6.2 and I see now that the code around
batch update has changed quite a bit since that release. :-|

After looking at this briefly in tip, I'm guessing that my
implementation is still pretty close.  Bug::set_all() is already
receiving a params hash with an 'other_bugs' key, and hopefully it
wouldn't be too difficult to pass that down to the validator.

Unfortunately, I wouldn't be able to look at this any closer until
we actually migrate to 4.0, which won't be until late 2011.

Comment 5

[Max Kanat-Alexander]

(In reply to comment #4)
> Why would you need to trace down the whole tree? 

  You can't guarantee the order that bugs are being changed in. Think about situations where a later change will lead to an invalid state, with complex dependency trees (at least A -> B -> C where the order of changes happens as C, B, A).

> But, if Object::set() were updated to take a
> params argument that allowed it to take additional information, it could
> easily be passed along.

  Every validator already gets such an argument, actually.

> Unfortunately, I wouldn't be able to look at this any closer until
> we actually migrate to 4.0, which won't be until late 2011.

  Okay. If you do look at it, we might take it if it's very simple.

Comment 6

[Kent Rogers [:mrbball]]

>   You can't guarantee the order that bugs are being changed in. Think about
> situations where a later change will lead to an invalid state, with complex
> dependency trees (at least A -> B -> C where the order of changes happens as C,
> B, A).
> 
I don't see why order matters.  If your blocker(s) are in the list,
you're good to go because they're going to get updated.  In your example,
regardless of whether A is updated first or last, if B is in the list, it's
allowed.

> > But, if Object::set() were updated to take a
> > params argument that allowed it to take additional information, it could
> > easily be passed along.
> 
>   Every validator already gets such an argument, actually.
> 
I don't see that.  For example, Bug::_check_resolution() only appears
to get only a resolution value.  Actually, it looks like the field name
is also passed in (from Object::set()) but is ignored.

Comment 7

[Max Kanat-Alexander]

  Hmm, okay, you may be right. Let's work this out in detail:

  Here's a dependency chain: A -> B -> C -> D. Let's pretend they're all open, but only A and B are in our list.

  Right now, here's what would happen:

  Resolve A first: Fails, B is open.

  In your proposal, we would check B and see "okay, we're also resolving B, it is OK to resolve A." But then we would fail to resolve B. So that works properly.

  And yeah, in any other situation that I can currently come up with, you're also right, provided there are no side effects of running the check (and Bugzilla is architected such that there should be no side effects) and provided there are no situations in which Bugzilla can silently fail to change a bug to a closed state (I don't know of any such situations).

Comment 8

[Max Kanat-Alexander]

By the way, you could accomplish this now by changing the sort order. (That'd be a workaround.)

Comment 9

[Kent Rogers [:mrbball]]

Attachment: V1 - patch to get list of bugs being updated to _check_resolution()

I finally got around to migrating to 4.0 and re-working my 3.6 mod to
work on 4.0.  It was fairly simple, requiring only that Bug::set_all()
pass ALL the params (including 'other_bugs') up to Object::set_all() so
that they can be passed on to the Bug set_* functions and thus eventually
to the validators.

Once the _check_resolution() validator has access to the "other_bugs"
parameter, it can easily determine if all blocker bugs are also in the
list to be updated.

Comment 10

[Frédéric Buclin]

Comment on attachment 579344 [details] [diff] [review]
V1 - patch to get list of bugs being updated to _check_resolution()

>=== modified file 'Bugzilla/Object.pm'
>-        next if !exists $field_values{$key};
>         my $method = "set_$key";
>+        next if !$self->can($method) || !exists $field_values{$key};

It's not ok to ignore a set_foo() command() if this command doesn't exist. Such attempts must be caught and an error must be thrown. set_all() is used in many places within the Bugzilla code, not only from Bug.pm.

Comment 11

[Kent Rogers [:mrbball]]

I didn't understand the comments in your review.  Can you elaborate on why it is a
problem to ignore set_foo() if it doesn't exist?

If "foo" was intended to be set and there wasn't a set_foo(), that would be an
internal (i.e. code) error.  It might be nice to catch that and throw an error, but
without this logic, I don't see a way to pass additional, non-settable parameters to
Object::set_all() such that they can be passed on to the individual set methods and
then to the validators.

Do you have other ideas/recommendations?  Another argument to set_all()?

I have been running the code in attachment 579344 [details] [diff] [review] in my Bugzilla installation for 1.5
years without issue.

FWIW, as of 4.2.3, I only see the Object::set_all() called from these places:

Bug::set_all()
editproducts.cgi (via Product class)
editkeywords.cgi (via Keyword class)
email_in.pl (via Comment class)
post_bug.cgi (via Comment class)