Code review comment for lp:~bac/launchpad/bug-5927

Hi Brad,

The search methods for bugs/bugtasks are horrible to work on, and
eager loading doesn't make it any prettier... you got the short straw
on this one :-/

I found a few issues, one fairly major, but none too hard to fix.

Gavin.

[6]

In lib/lp/bugs/model/bug.py:

- FROM Bug, BugSubscription, TeamParticipation
- WHERE Bug.id = BugSubscription.bug AND
- TeamParticipation.person = %(personid)s AND
- BugSubscription.person = TeamParticipation.team))
+ FROM Bug, BugSubscription, BugTask, TeamParticipation
+ WHERE (Bug.id = BugSubscription.bug AND
+ TeamParticipation.person = %(personid)s AND
+ BugSubscription.person = TeamParticipation.team)
+ OR (
+ BugTask.bug = Bug.id AND
+ TeamParticipation.person = %(personid)s AND
+ TeamParticipation.team = BugTask.assignee)
+ ))

The new join to BugTask combined with the first of the ORed clauses:

    Bug.id = BugSubscription.bug AND
    TeamParticipation.person = %(personid)s AND
    BugSubscription.person = TeamParticipation.team

results in a cartestian product against BugTask.

I think this could be expressed as a UNION:

    SELECT Bug.id
      FROM Bug, BugSubscription, TeamParticipation
     WHERE Bug.id = BugSubscription.bug AND
           TeamParticipation.person = %(personid)s AND
           BugSubscription.person = TeamParticipation.team)
    UNION
    SELECT Bug.id
      FROM Bug, BugTask, TeamParticipation
     WHERE BugTask.bug = Bug.id AND
           TeamParticipation.person = %(personid)s AND
           TeamParticipation.team = BugTask.assignee

In addition, the join to Bug could be removed from both queries:

    SELECT BugSubscription.bug
      FROM BugSubscription, TeamParticipation
     WHERE TeamParticipation.person = %(personid)s AND
           BugSubscription.person = TeamParticipation.team
    UNION
    SELECT BugTask.bug
      FROM BugTask, TeamParticipation
     WHERE TeamParticipation.person = %(personid)s AND
           TeamParticipation.team = BugTask.assignee

[7]

In lib/lp/bugs/model/bugtask.py:

- FROM BugSubscription, TeamParticipation
- WHERE TeamParticipation.person = %(personid)s AND
+ FROM BugSubscription, TeamParticipation, BugTask
+ WHERE (TeamParticipation.person = %(personid)s AND
                    BugSubscription.person = TeamParticipation.team AND
- BugSubscription.bug = Bug.id))
+ BugSubscription.bug = Bug.id) OR
+ (BugTask.bug = Bug.id AND
+ TeamParticipation.person = %(personid)s AND
+ TeamParticipation.team = BugTask.assignee)
+ ))

Similar problem to [6].

[8]

+ def cache_assignees(rows):
+ # The decorator must be called to turn the rows of the result into
+ # a list of BugTasks, which can be used for caching the assignees.
+ BugTaskSet._cache_assignees([decorator(row) for row in rows])

Passing a generator alone will work and eliminate a short-lived list:

            BugTaskSet._cache_assignees(decorator(row) for row in rows)

[10]

BugTaskSet._search() is a fairly ugly beast, and cache_assignees is
making it more convoluted. I think we should try not to make that
method harder to unravel later.

A kind of post-iter hook might work and be a bit cleaner. How about
ditching cache_assignees and returning an instance of the following
class? It does the same as cache_assignees, but later, so that the
decorators are only run once.

    class BugTaskResultSet(DecoratedResultSet):

        def __iter__(self, *args, **kwargs):
            bugtasks = list(
                super(BugTaskResultSet, self).__iter__(*args, **kwargs))
            BugTaskSet._cache_assignees(bugtasks)
            return iter(bugtasks)

Once caveat: if any of the decorators need something that is loaded by
_cache_assignees then this will be too late, making it unsuitable.

Not tested either :-/

[11]

+ assignees = getUtility(IPersonSet).getPrecachedPersonsFromIDs(
+ assignee_ids, need_validity=True)

Indentation.