102622 - Dependency graphs should be able to be limited by relationship

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, defect, P3)

Description

[Myk Melez [:myk] [@mykmelez]]

showdependencygraph.cgi creates graphs containing any bug with any relationship
to the bug whose graph is being requested.  In some cases this means that
Bugzilla generates a webdot file with thousands of dependencies, which is too
many for webdot to parse, so showdependencygraph.cgi returns a page that hangs
for five minutes and then times out waiting for the graph image to be displayed.

showdependencygraph.cgi should provide options for limiting the level of
recursion or other mechanisms to make it usable with bugs that have a large
number of dependencies.  It could use the same logic used by showdependencytree.cgi
 to limit the number of bugs it displays.

Comment 1

[Hixie (not reading bugmail)]

I think a very useful way of limiting this which is not available in the tree
view and which makes little sense in the tree view but a lot of sense for graphs
is a way to stop recursing when you hit a bug with more than <n> dependencies
(with <n> being some large user-customisable number default to, e.g., 20).

I think this would massively decrease the size of the graphs being generated,
because I expect most of the time the graph snowballs out of control when the
chain hits one of the uber meta bugs. Nobody really cares about going up then
down through those uber meta bugs. They either want to see down from that bug,
or up to that bug.

When bugs are trimmed, the graph should show this (6 is the uber meta bug):

                             +---------+
                             |   123   |
                             +---------+
                              /   |   \
         +------+            /    |    \        +-----+
         |  45  |-----------'     |     '-------|  6  |
         +------+              +-----+          +-----+
                               | 789 |             |
                               +-----+             o
                                                   o
                                                   o

Comment 2

[Bradley Baetz (:bbaetz)]

We no longer rely on webdot, but we still could use some depth-limiting, even
for the local case.

Comment 3

[Stewart Gordon]

It would be necessary to have the depth limit as a default setting, so that
people can access it in the first place where the infinite depth has caused it
to fail.

Comment 4

[Dave Miller [:justdave]]

All 2.18 bugs that haven't been touched in over 60 days and aren't flagged as
blockers are getting pushed out to 2.20

Comment 5

[Dave Miller [:justdave]]

Myk: is this a dupe of bug 193947 (or vice versa?)

Comment 6

[Max Kanat-Alexander]

This bug has not been touched by its owner in over six months, even though it is
targeted to 2.20, for which the freeze is 10 days away. Unsetting the target
milestone, on the assumption that nobody is actually working on it or has any
plans to soon.

If you are the owner, and you plan to work on the bug, please give it a real
target milestone. If you are the owner, and you do *not* plan to work on it,
please reassign it to [e-mail] or a .bugs component owner. If you are
*anybody*, and you get this comment, and *you* plan to work on the bug, please
reassign it to yourself if you have the ability.

Comment 7

[Dave Miller [:justdave]]

Reassigning bugs that I'm not actively working on to the default component owner
in order to try to make some sanity out of my personal buglist.  This doesn't
mean the bug isn't being dealt with, just that I'm not the one doing it.  If you
are dealing with this bug, please assign it to yourself.

Comment 8

[Dave Miller [:justdave]]

*** Bug 164085 has been marked as a duplicate of this bug. ***

Comment 9

[Stewart Gordon]

*** Bug 313563 has been marked as a duplicate of this bug. ***

Comment 10

[Frédéric Buclin]

*** Bug 193947 has been marked as a duplicate of this bug. ***

Comment 11

[Frédéric Buclin]

*** Bug 331587 has been marked as a duplicate of this bug. ***

Comment 12

[Dave Miller [:justdave]]

*** Bug 339642 has been marked as a duplicate of this bug. ***

Comment 13

[Dave Miller [:justdave]]

The lack of ability to do this causes denial of service attacks on bmo...

Comment 14

[Stewart Gordon]

Slight understatement - the ability _not_ to do this can lead to DoS attacks.  I now think there should be a maximum depth set in the configuration of the Bugzilla installation.

Comment 15

[Bishop Clark (LC957)]

Attachment: hint patch

I just finished a patch to my own 2.20 and 2.22, one which seems to do most of what you're thinking -- mine omits bugs which aren't in the dep or block path of the IDs given.  You'll see that you will need to pass 'constrain=1' at the URL bar if you want to see it.  I haven't even thought about template modification to support it, although bug 248166 has a very similar hack and DOES have some template mods we can use.

Either way, I made this mod and thought maybe it'd scratch someone else's itch.  It'll need to be cleaned up probably, in addition to the template fixes, but I'm hoping I've done the heavier lifting already.

ignore the arrow reversal at the head of the patch -- the reverse dependency graph layout always annoyed me, like we can either choose a) backwards, or b) upside-down for our graph layout  ;-)

HTH

Comment 16

[Frédéric Buclin]

(In reply to comment #15)
> ignore the arrow reversal at the head of the patch -- the reverse dependency
> graph layout always annoyed me

This is bug 370885. :)

Comment 18

[Frédéric Buclin]

Attachment: patch, v1

In this patch, I consider "depth" as restricting bugs to those directly blocking or depending on the current bug(s). This will severely limit the number of bugs displayed, even if a meta bug is selected as root of the tree.

Comment 19

[Frédéric Buclin]

Comment on attachment 257847 [details] [diff] [review]
patch, v1

/me hopes myk will be faster than dave.

Comment 20

[Frédéric Buclin]

Comment on attachment 257847 [details] [diff] [review]
patch, v1

Dave told me he was too busy to review it.

Comment 21

[Michiel van Everdingen]

I've implemented patch,v1 (attachment 257847 [details] [diff] [review]) instead of my solution in Bug 348166. Patch,v1 works fine for me.

Comment 22

[Teemu Mannermaa (:wicked)]

Comment on attachment 257847 [details] [diff] [review]
patch, v1

This is a nice patch and seems to do what it's supposed to do. Following problems can be fixed on checkin or you can carry on my r+ to a patch with only those changes.

However, I don't think this really fixes this bug as described. This patch restricts by relationship and not by depth. It does cut down on nodes rendered so maybe it's a correct fix for the "too much to handle" problem.

BTW, this CGI has a nasty habit of spitting out an ugly Internal Error when going to it directly. Instead it should show no graph but ask for parameters. :)

>Index: template/en/default/bug/dependency-graph.html.tmpl
>===================================================================
>+      <td><input id="dep_id" name="id" value="[% bug_id %]"></td>

Use same value for both name and id to avoid confusion and problems.

>+            Restrict to [% terms.bugs %] having a direct relationship with this bug</option>
>+            Show all [% terms.bugs %] having any relationship with this bug</option>

Term "this bug" should be plural as you can input more than one bug ID. Maybe something like "these bugs" or "selected bugs" or "entered bugs". Also, shouldn't those words use terms.bugs too?

>+      <th align="left"><label for="rankdir">Orientation:</label></th>

Drop "Orient" words from the options now that this has a label.

I wonder why we don't show all valid values for this one. All four orientations are supported but only two of them are selectable here.

Comment 23

[Frédéric Buclin]

(In reply to comment #22)
> However, I don't think this really fixes this bug as described. This patch
> restricts by relationship and not by depth. It does cut down on nodes rendered
> so maybe it's a correct fix for the "too much to handle" problem.

Yes, I think that's the right approach for now. Speaking of depth when the relationship is random doesn't make sense to me. justdave suggested this alternative anyway, but he didn't convince me. ;)

Comment 24

[Frédéric Buclin]

Attachment: patch, v1.1

I fixed all comments made by wicked. Carrying forward his r+.

Comment 25

[Frédéric Buclin]

Checking in showdependencygraph.cgi;
/cvsroot/mozilla/webtools/bugzilla/showdependencygraph.cgi,v  <--  showdependencygraph.cgi
new revision: 1.57; previous revision: 1.56
done
Checking in template/en/default/bug/dependency-graph.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/dependency-graph.html.tmpl,v  <--  dependency-graph.html.tmpl
new revision: 1.13; previous revision: 1.12
done

Comment 26

[Max Kanat-Alexander]

Added to the release notes for Bugzilla 3.2 in a patch on bug 432331.