launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Review: Approve



Diff comments:

> 
> === modified file 'lib/lp/answers/model/question.py'
> --- lib/lp/answers/model/question.py	2015-09-28 07:57:17 +0000
> +++ lib/lp/answers/model/question.py	2015-09-29 04:00:42 +0000
> @@ -660,6 +662,20 @@
>          self.status = new_status
>          return tktmsg
>  
> +    @property
> +    def bugs(self):
> +        from lp.bugs.model.bug import Bug
> +        if getFeatureFlag('bugs.xref_buglinks.query'):

Can the flag names used in this branch be in an interface module somewhere rather than repeating it, to guard against typos?

> +            bug_ids = [
> +                int(id) for _, id in getUtility(IXRefSet).findFrom(
> +                    (u'question', unicode(self.id)), types=[u'bug'])]

I feel that perhaps findFrom should handle the unicode() bit; this would also allow changing one place to use from_id_int instead where possible, rather than having to change all the callers.  Same applies to other IXRefSet methods.

> +        else:
> +            bug_ids = list(IStore(QuestionBug).find(
> +                QuestionBug,
> +                QuestionBug.question == self).values(QuestionBug.bugID))
> +        return list(sorted(
> +            bulk.load(Bug, bug_ids), key=operator.attrgetter('id')))
> +
>      # IBugLinkTarget implementation
>      def linkBug(self, bug, user=None):
>          """See `IBugLinkTarget`."""
> 
> === modified file 'lib/lp/scripts/garbo.py'
> --- lib/lp/scripts/garbo.py	2015-07-22 07:09:12 +0000
> +++ lib/lp/scripts/garbo.py	2015-09-29 04:00:42 +0000
> @@ -1678,6 +1679,66 @@
>                  transaction.abort()
>  
>  
> +class BugXRefMigrator(TunableLoop):
> +    """Creates an XRef record for each former IBugLink."""
> +
> +    maximum_chunk_size = 5000
> +
> +    def __init__(self, log, abort_time=None):
> +        super(BugXRefMigrator, self).__init__(log, abort_time)
> +        self.start_at = 1
> +        self.store = IMasterStore(Bug)
> +
> +    def findBugs(self):
> +        if not getFeatureFlag('bugs.xref_buglinks.garbo.enabled'):
> +            return EmptyResultSet()
> +        return self.store.find(
> +            Bug, Bug.id >= self.start_at).order_by(Bug.id)
> +
> +    def isDone(self):
> +        return self.findBugs().is_empty()
> +
> +    def __call__(self, chunk_size):
> +        # Grab a chunk of Bug IDs.
> +        # Find all QuestionBugs, SpecificationBugs and BugCves for each
> +        # of those bugs.
> +        # Compose a list of link IDs that should exist.
> +        # Perform a bulk XRef find for all of those.
> +        # Delete any extras, create any missing.

The actual code does not delete any extras.  Should it, or is this comment wrong?

> +        from lp.blueprints.model.specificationbug import SpecificationBug
> +        from lp.bugs.model.bugcve import BugCve
> +        from lp.bugs.model.cve import Cve
> +        from lp.coop.answersbugs.model import QuestionBug
> +        from lp.services.xref.interfaces import IXRefSet
> +        bug_ids = list(self.findBugs()[:chunk_size].values(Bug.id))
> +        qbs = list(self.store.find(
> +            QuestionBug, QuestionBug.bugID.is_in(bug_ids)))
> +        sbs = list(self.store.find(
> +            SpecificationBug, SpecificationBug.bugID.is_in(bug_ids)))
> +        bcs = list(self.store.find(BugCve, BugCve.bugID.is_in(bug_ids)))
> +        wanted = {(u'bug', unicode(bug_id)): {} for bug_id in bug_ids}

Could be a defaultdict to make it a little more obviously correct.

> +        for qb in qbs:
> +            wanted[(u'bug', unicode(qb.bugID))][
> +                (u'question', unicode(qb.questionID))] = {
> +                    'date_created': qb.date_created}
> +        for sb in sbs:
> +            wanted[(u'bug', unicode(sb.bugID))][
> +                (u'specification', unicode(sb.specificationID))] = {}
> +        load_related(Cve, bcs, ['cveID'])
> +        for bc in bcs:
> +            wanted[(u'bug', unicode(bc.bugID))][
> +                (u'cve', unicode(bc.cve.sequence))] = {}
> +        existing = getUtility(IXRefSet).findFromMany(wanted.keys())
> +        needed = {
> +            bug: {
> +                other: meta for other, meta in others.iteritems()
> +                if other not in existing.get(bug, {})}
> +            for bug, others in wanted.iteritems() if others}
> +        getUtility(IXRefSet).create(needed)
> +        self.start_at = bug_ids[-1] + 1
> +        transaction.commit()
> +
> +
>  class FrequentDatabaseGarbageCollector(BaseDatabaseGarbageCollector):
>      """Run every 5 minutes.
>  
> @@ -1714,6 +1775,7 @@
>          DuplicateSessionPruner,
>          RevisionCachePruner,
>          UnusedSessionPruner,
> +        BugXRefMigrator,
>          ]

This list is otherwise sorted.

>      experimental_tunable_loops = []
>  
> 
> === modified file 'lib/lp/scripts/tests/test_garbo.py'
> --- lib/lp/scripts/tests/test_garbo.py	2015-07-22 07:09:12 +0000
> +++ lib/lp/scripts/tests/test_garbo.py	2015-09-29 04:00:42 +0000
> @@ -1433,6 +1433,94 @@
>          for person in people_enf_true:
>              _assert_enf_by_person(person, True)
>  
> +    def test_BugXRefMigrator(self):
> +        from testtools.matchers import (
> +            Equals,
> +            Is,
> +            MatchesDict,
> +            Not,
> +            )
> +
> +        from lp.bugs.model.bug import Bug
> +        from lp.bugs.model.bugcve import BugCve
> +        from lp.blueprints.model.specificationbug import SpecificationBug
> +        from lp.coop.answersbugs.model import QuestionBug
> +        from lp.services.database.interfaces import IStore
> +        from lp.services.xref.interfaces import IXRefSet
> +
> +        switch_dbuser('testadmin')
> +        self.useFixture(FeatureFixture(
> +            {'bugs.xref_buglinks.garbo.enabled': 'on'}))
> +        store = IStore(Bug)
> +
> +        # The first bug has a spec and a question.
> +        bug1 = self.factory.makeBug()
> +        spec1 = self.factory.makeSpecification()
> +        sb1 = SpecificationBug(specification=spec1, bug=bug1)
> +        store.add(sb1)
> +        question1 = self.factory.makeQuestion()
> +        qb1 = QuestionBug(question=question1, bug=bug1)
> +        store.add(qb1)
> +
> +        # A second bug has a question and a CVE.
> +        bug2 = self.factory.makeBug()
> +        question2 = self.factory.makeQuestion()
> +        qb2 = QuestionBug(question=question2, bug=bug2)
> +        store.add(qb2)
> +        cve2 = self.factory.makeCVE(sequence='2099-1234')
> +        bc2 = BugCve(bug=bug2, cve=cve2)
> +        store.add(bc2)
> +
> +        # Bug the third is all alone.
> +        bug3 = self.factory.makeBug()
> +
> +        # Bug four has just a spec.
> +        bug4 = self.factory.makeBug()
> +        spec4 = self.factory.makeSpecification()
> +        sb4 = SpecificationBug(specification=spec4, bug=bug4)
> +        store.add(sb4)
> +
> +        # Initially the new XRef table has no links for the bugs.
> +        self.assertEqual(
> +            {},
> +            getUtility(IXRefSet).findFromMany(
> +                (u'bug', unicode(bug.id)) for bug in (bug1, bug2, bug3, bug4)))

If you change the code above to delete extras, then there should also be a test with pre-existing extraneous XRefs.

> +
> +        # Garbo fills in links for each QuestionBug, SpecificationBug
> +        # and BugCve.
> +        self.runHourly()
> +        matches_expected = MatchesDict({
> +            (u'bug', unicode(bug1.id)): MatchesDict({
> +                (u'specification', unicode(spec1.id)): MatchesDict({
> +                    'metadata': Is(None), 'creator': Is(None),
> +                    'date_created': Not(Is(None))}),
> +                (u'question', unicode(question1.id)): MatchesDict({
> +                    'metadata': Is(None), 'creator': Is(None),
> +                    'date_created': Equals(qb1.date_created)}),
> +                }),
> +            (u'bug', unicode(bug2.id)): MatchesDict({
> +                (u'question', unicode(question2.id)): MatchesDict({
> +                    'metadata': Is(None), 'creator': Is(None),
> +                    'date_created': Equals(qb2.date_created)}),
> +                (u'cve', cve2.sequence): MatchesDict({
> +                    'metadata': Is(None), 'creator': Is(None),
> +                    'date_created': Not(Is(None))}),
> +                }),
> +            (u'bug', unicode(bug4.id)): MatchesDict({
> +                (u'specification', unicode(spec4.id)): MatchesDict({
> +                    'metadata': Is(None), 'creator': Is(None),
> +                    'date_created': Not(Is(None))}),
> +                }),
> +            })
> +        self.assertThat(
> +            getUtility(IXRefSet).findFromMany(
> +                (u'bug', unicode(bug.id)) for bug in (bug1, bug2, bug3, bug4)),
> +            matches_expected)
> +
> +        # A second run is harmless.
> +        self.runHourly()
> +
> +
>  
>  class TestGarboTasks(TestCaseWithFactory):
>      layer = LaunchpadZopelessLayer


-- 
https://code.launchpad.net/~wgrant/launchpad/xref-buglinks/+merge/272591
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.