launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Review: Approve



Diff comments:

> 
> === added file 'lib/lp/services/xref/model.py'
> --- lib/lp/services/xref/model.py	1970-01-01 00:00:00 +0000
> +++ lib/lp/services/xref/model.py	2015-09-28 12:44:55 +0000
> @@ -0,0 +1,131 @@
> +# Copyright 2015 Canonical Ltd.  This software is licensed under the
> +# GNU Affero General Public License version 3 (see the file LICENSE).
> +
> +from __future__ import absolute_import, print_function, unicode_literals
> +
> +__metaclass__ = type
> +__all__ = [
> +    "XRefSet",
> +    ]
> +
> +import pytz
> +from storm.expr import (
> +    And,
> +    Or,
> +    )
> +from storm.properties import (
> +    DateTime,
> +    Int,
> +    JSON,
> +    Unicode,
> +    )
> +from storm.references import Reference
> +from zope.interface import implementer
> +
> +from lp.services.database import bulk
> +from lp.services.database.constants import UTC_NOW
> +from lp.services.database.interfaces import IStore
> +from lp.services.database.stormbase import StormBase
> +from lp.services.xref.interfaces import IXRefSet
> +
> +
> +class XRef(StormBase):
> +    """Cross-reference between two objects.
> +
> +    For references to local objects (there is currently no other kind),
> +    another reference in the opposite direction exists.
> +
> +    The to_id_int and from_id_int columns exist for efficient SQL joins.
> +    They are set automatically when the ID looks like an integer.
> +    """
> +
> +    __storm_table__ = 'XRef'
> +    __storm_primary__ = "to_type", "to_id", "from_type", "from_id"

Is it significant that this is in a different order from the PRIMARY KEY declaration in the DB patch?  I'm guessing not, but I'd probably make them match.

> +
> +    to_type = Unicode(allow_none=False)
> +    to_id = Unicode(allow_none=False)
> +    to_id_int = Int()  # For efficient joins.
> +    from_type = Unicode(allow_none=False)
> +    from_id = Unicode(allow_none=False)
> +    from_id_int = Int()  # For efficient joins.
> +    creator_id = Int(name="creator")
> +    creator = Reference(creator_id, "Person.id")
> +    date_created = DateTime(name='date_created', tzinfo=pytz.UTC)
> +    metadata = JSON()
> +
> +
> +def _int_or_none(s):
> +    if s.isdigit():
> +        return int(s)
> +    else:
> +        return None
> +
> +
> +@implementer(IXRefSet)
> +class XRefSet:
> +
> +    def create(self, xrefs):
> +        # All references are currently to local objects, so add
> +        # backlinks as well to keep queries in both directions quick.
> +        # The *_id_int columns are also set if the ID looks like an int.
> +        rows = []
> +        for from_, tos in xrefs.items():
> +            for to, props in tos.items():
> +                rows.append((
> +                    from_[0], from_[1], _int_or_none(from_[1]),
> +                    to[0], to[1], _int_or_none(to[1]),
> +                    props.get('creator'), props.get('date_created', UTC_NOW),
> +                    props.get('metadata')))
> +                rows.append((
> +                    to[0], to[1], _int_or_none(to[1]),
> +                    from_[0], from_[1], _int_or_none(from_[1]),
> +                    props.get('creator'), props.get('date_created', UTC_NOW),
> +                    props.get('metadata')))
> +        bulk.create(
> +            (XRef.from_type, XRef.from_id, XRef.from_id_int,
> +             XRef.to_type, XRef.to_id, XRef.to_id_int,
> +             XRef.creator, XRef.date_created, XRef.metadata), rows)
> +
> +    def delete(self, xrefs):
> +        # Delete both directions.
> +        pairs = []
> +        for from_, tos in xrefs.items():
> +            for to in tos:
> +                pairs.extend([(from_, to), (to, from_)])
> +
> +        IStore(XRef).find(
> +            XRef,
> +            Or(*[
> +                And(XRef.from_type == pair[0][0],
> +                    XRef.from_id == pair[0][1],
> +                    XRef.to_type == pair[1][0],
> +                    XRef.to_id == pair[1][1])
> +                for pair in pairs])
> +            ).remove()
> +
> +    def findFromMany(self, object_ids, types=None):
> +        from lp.registry.model.person import Person
> +
> +        object_ids = list(object_ids)
> +        if not object_ids:
> +            return {}
> +
> +        store = IStore(XRef)
> +        rows = list(store.using(XRef).find(
> +            (XRef.from_type, XRef.from_id, XRef.to_type, XRef.to_id,
> +             XRef.creator_id, XRef.date_created, XRef.metadata),
> +            Or(*[
> +                And(XRef.from_type == id[0], XRef.from_id == id[1])
> +                for id in object_ids]),

Shouldn't this check XRef.from_id_int instead if id.isdigit() is true?  Otherwise you seem to have nothing that actually uses the *_int columns.

> +            XRef.to_type.is_in(types) if types is not None else True))
> +        bulk.load(Person, [row[4] for row in rows])
> +        result = {}
> +        for row in rows:
> +            result.setdefault((row[0], row[1]), {})[(row[2], row[3])] = {

collections.defaultdict would make this a bit more readable.

> +                "creator": store.get(Person, row[4]) if row[4] else None,
> +                "date_created": row[5],
> +                "metadata": row[6]}
> +        return result
> +
> +    def findFrom(self, object_id, types=None):
> +        return self.findFromMany([object_id], types=types).get(object_id, {})
> 
> === added file 'lib/lp/services/xref/tests/test_model.py'
> --- lib/lp/services/xref/tests/test_model.py	1970-01-01 00:00:00 +0000
> +++ lib/lp/services/xref/tests/test_model.py	2015-09-28 12:44:55 +0000
> @@ -0,0 +1,159 @@
> +# Copyright 2015 Canonical Ltd.  This software is licensed under the
> +# GNU Affero General Public License version 3 (see the file LICENSE).
> +
> +from __future__ import absolute_import, print_function, unicode_literals
> +
> +__metaclass__ = type
> +
> +import datetime
> +
> +import pytz
> +from testtools.matchers import Equals
> +from zope.component import getUtility
> +
> +from lp.services.database.interfaces import IStore
> +from lp.services.xref.interfaces import IXRefSet
> +from lp.services.xref.model import XRef
> +from lp.testing import (
> +    StormStatementRecorder,
> +    TestCaseWithFactory,
> +    )
> +from lp.testing.layers import DatabaseFunctionalLayer
> +from lp.testing.matchers import HasQueryCount
> +
> +
> +class TestXRefSet(TestCaseWithFactory):
> +
> +    layer = DatabaseFunctionalLayer
> +
> +    def test_create_sets_date_created(self):
> +        # date_created defaults to now, but can be overridden.
> +        old = datetime.datetime.strptime('2005-01-01', '%Y-%m-%d').replace(
> +            tzinfo=pytz.UTC)
> +        now = IStore(XRef).execute(
> +            "SELECT CURRENT_TIMESTAMP AT TIME ZONE 'UTC'"
> +            ).get_one()[0].replace(tzinfo=pytz.UTC)
> +        getUtility(IXRefSet).create({
> +            ('a', '1'): {('b', 'foo'): {}},
> +            ('a', '2'): {('b', 'bar'): {'date_created': old}}})
> +        rows = IStore(XRef).find(
> +            (XRef.from_id, XRef.to_id, XRef.date_created),
> +            XRef.from_type == 'a')
> +        self.assertContentEqual(
> +            [('1', 'foo', now), ('2', 'bar', old)], rows)
> +
> +    def test_create_sets_int_columns(self):
> +        # The string ID columns have integers equivalents for quick and
> +        # easy joins to integer PKs. They're set automatically when the
> +        # string ID looks like an integer.
> +        getUtility(IXRefSet).create({
> +            ('a', '1234'): {('b', 'foo'): {}, ('b', '2468'): {}},
> +            ('a', '12ab'): {('b', '1234'): {}, ('b', 'foo'): {}}})
> +        rows = IStore(XRef).find(
> +            (XRef.from_type, XRef.from_id, XRef.from_id_int, XRef.to_type,
> +             XRef.to_id, XRef.to_id_int),
> +            XRef.from_type == 'a')
> +        self.assertContentEqual(
> +            [('a', '1234', 1234, 'b', 'foo', None),
> +             ('a', '1234', 1234, 'b', '2468', 2468),
> +             ('a', '12ab', None, 'b', '1234', 1234),
> +             ('a', '12ab', None, 'b', 'foo', None)
> +             ],
> +            rows)
> +
> +    def test_findFrom(self):
> +        creator = self.factory.makePerson()
> +        now = IStore(XRef).execute(
> +            "SELECT CURRENT_TIMESTAMP AT TIME ZONE 'UTC'"
> +            ).get_one()[0].replace(tzinfo=pytz.UTC)
> +        getUtility(IXRefSet).create({
> +            ('a', 'bar'): {
> +                ('b', 'foo'): {'creator': creator, 'metadata': {'test': 1}}},
> +            ('b', 'foo'): {
> +                ('a', 'baz'): {'creator': creator, 'metadata': {'test': 2}}},
> +            })

I'd explicitly test with different creators somewhere around here to ensure that the bulk-loading of Person rows works.

> +
> +        with StormStatementRecorder() as recorder:
> +            bar_refs = getUtility(IXRefSet).findFrom(('a', 'bar'))
> +        self.assertThat(recorder, HasQueryCount(Equals(2)))
> +        self.assertEqual(
> +            {('b', 'foo'): {
> +                'creator': creator, 'date_created': now,
> +                'metadata': {'test': 1}}},
> +            bar_refs)
> +
> +        with StormStatementRecorder() as recorder:
> +            foo_refs = getUtility(IXRefSet).findFrom(('b', 'foo'))
> +        self.assertThat(recorder, HasQueryCount(Equals(2)))
> +        self.assertEqual(
> +            {('a', 'bar'): {
> +                'creator': creator, 'date_created': now,
> +                'metadata': {'test': 1}},
> +             ('a', 'baz'): {
> +                 'creator': creator, 'date_created': now,
> +                 'metadata': {'test': 2}}},
> +            foo_refs)
> +
> +        with StormStatementRecorder() as recorder:
> +            bar_refs = getUtility(IXRefSet).findFrom(('a', 'baz'))
> +        self.assertThat(recorder, HasQueryCount(Equals(2)))
> +        self.assertEqual(
> +            {('b', 'foo'): {
> +                'creator': creator, 'date_created': now,
> +                'metadata': {'test': 2}}},
> +            bar_refs)
> +
> +        with StormStatementRecorder() as recorder:
> +            bar_baz_refs = getUtility(IXRefSet).findFromMany(
> +                [('a', 'bar'), ('a', 'baz')])
> +        self.assertThat(recorder, HasQueryCount(Equals(2)))
> +        self.assertEqual(
> +            {('a', 'bar'): {
> +                ('b', 'foo'): {
> +                    'creator': creator, 'date_created': now,
> +                    'metadata': {'test': 1}}},
> +             ('a', 'baz'): {
> +                ('b', 'foo'): {
> +                    'creator': creator, 'date_created': now,
> +                    'metadata': {'test': 2}}}},
> +             bar_baz_refs)
> +
> +    def test_findFrom_types(self):
> +        # findFrom can look for only particular types of related
> +        # objects.
> +        getUtility(IXRefSet).create({
> +            ('a', '1'): {('a', '2'): {}, ('b', '3'): {}},
> +            ('b', '4'): {('a', '5'): {}, ('c', '6'): {}},
> +            })
> +        self.assertContentEqual(
> +            [('a', '2')],
> +            getUtility(IXRefSet).findFrom(('a', '1'), types=['a', 'c']).keys())
> +        self.assertContentEqual(
> +            [('a', '5'), ('c', '6')],
> +            getUtility(IXRefSet).findFrom(('b', '4'), types=['a', 'c']).keys())
> +
> +        # Asking for no types or types that don't exist finds nothing.
> +        self.assertContentEqual(
> +            [],
> +            getUtility(IXRefSet).findFrom(('b', '4'), types=[]).keys())
> +        self.assertContentEqual(
> +            [],
> +            getUtility(IXRefSet).findFrom(('b', '4'), types=['d']).keys())
> +
> +    def test_findFromMany_none(self):
> +        self.assertEqual({}, getUtility(IXRefSet).findFromMany([]))
> +
> +    def test_delete(self):
> +        getUtility(IXRefSet).create({
> +            ('a', 'bar'): {('b', 'foo'): {}},
> +            ('b', 'foo'): {('a', 'baz'): {}},
> +            })
> +        self.assertContentEqual(
> +            [('a', 'bar'), ('a', 'baz')],
> +            getUtility(IXRefSet).findFrom(('b', 'foo')).keys())
> +        with StormStatementRecorder() as recorder:
> +            getUtility(IXRefSet).delete({('b', 'foo'): [('a', 'bar')]})
> +        self.assertThat(recorder, HasQueryCount(Equals(1)))
> +        self.assertEqual(
> +            [('a', 'baz')],
> +            getUtility(IXRefSet).findFrom(('b', 'foo')).keys())


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