[sugar] review: gadget buddy search code

[Dafydd Harries]

This is a code review of Guillaume's buddy_search Gadget branch, as taken
from:

https://dev.laptop.org/git?p=users/guillaume/gadget;a=shortlog;h=buddy_search

> diff --git a/gadget/component.py b/gadget/component.py
> index 6fcb682..ae1ba16 100644
> --- a/gadget/component.py
> +++ b/gadget/component.py
> @@ -27,6 +27,10 @@ def make_iq_result(stanza):
>
>      return reply
>
> +# FIXME: add more types
> +type_to_str = {str: 'str'}
> +str_to_type = {'str': str}
> +

We should check which types the buddy/activity interfaces support for
properties, and test each of those types.

>  def parse_properties(elem):
>      properties = {}
>
> @@ -46,10 +50,25 @@ def parse_properties(elem):
>          if type is None or name is None or value is None:
>              continue
>
> -        properties[name] = (type, value)
> +        try:
> +            properties[name] = (str_to_type[type], value)
> +        except KeyError:
> +            continue

We shouldn't just ignore failed conversions. We should throw an error, and
make the caller return an error to the client. Perhaps the exception class
would have some convenience code for generating the error stanza.

This function should have some dedicated unit tests.

>      return properties
>
> +def properties_to_xml(properties, elem):
> +    for key, (type, value) in properties.iteritems():
> +        try:
> +            str_type = type_to_str[type]
> +        except KeyError:
> +            continue
> +
> +        property = elem.addElement((None, 'property'))
> +        property['type'] = str_type
> +        property['name'] = key
> +        property.addContent(value)
> +
>  class Room(object):
>      def __init__(self, own_nick):
>          self.own_nick = own_nick

Again, we shouldn't silently ignore errors. This lookup should never fail,
because it would mean that we've stored invalid property data in the model.

This function should have some dedicated unit tests.

>  @@ -118,19 +137,100 @@ class GadgetService(component.Service):
>
>       def iq_buddy_query(self, stanza):
>           result = make_iq_result(stanza)
>  +        _query = stanza.firstChildElement()
>           query = result.firstChildElement()
>
>  -        for buddy in self.model.buddy_search({}):
>  +        results = set()
>  +        if len(_query.children) == 0:
>  +            # no buddy childs. We want all the buddies
>  +            results.update(self.model.buddy_search({}))

s/childs/children/.

Please put blank lines before blocks, for consistency.

Can we say "if _query.children:" instead? It's more idiomatic.

>  +
>  +        for elem in _query.children:

I'm not sure we should support answering multiple queries in one IQ. I'm not
sure if it does any harm, but I haven't seen other XMPP protocols allow it.

> +            if elem.name != 'buddy':
> +                continue
> +            found = None
> +
> +            # jid search
> +            try:
> +                tmp = set([self.model.buddy_by_id(elem['jid'])])
> +                if found is None:
> +                    found = tmp
> +                else:
> +                    found.intersection_update(tmp)
> +            except KeyError:
> +                pass

The "if found is None:" seems strange, becuase "found" should always be None
at this point in the code.

There's too much code inside the try: clause. What lookup are we expecting
might fail? If it's the elem['jid'] lookup, I'd prefer this approach:

  jid = elem.getAttribute('jid')

  if jid is None:
      continue

> +
> +            # properties search
> +            for e in elem.children:
> +                if e.name == 'properties' and e.uri == ns.BUDDY_PROP:
> +                    props = parse_properties(e)
> +                    tmp = set(self.model.buddy_search(props))
> +                    if found is None:
> +                        found = tmp
> +                    else:
> +                        found.intersection_update(tmp)

Can we rename "tmp" to "buddies" or "results"?

> +
> +            if found is not None:
> +                results.update(found)
> +
> +        for buddy in results:
>              buddy_elem = query.addElement((ns.BUDDY, 'buddy'))
>              buddy_elem['jid'] = buddy.jid
> +            properties = buddy_elem.addElement((ns.BUDDY_PROP, 'properties'))
> +            properties_to_xml(buddy.properties, properties)
>
>          return result

I would prefer that properties_to_xml be a pure function. I.e. used like:

  for node in properties_to_xml(buddy.properties):
      properties.addChild(node)

I think much of this logic could be moved into the model, and that the code
in iq_buddy_query() be limited to converting to/from XML.

>      def iq_activity_query(self, stanza):
>          result = make_iq_result(stanza)
> +        _query = stanza.firstChildElement()
>          query = result.firstChildElement()
>
> -        for activity in self.model.activity_search({}):
> +        results = set()
> +        if len(_query.children) == 0:
> +            # no activity childs. We want all the activities
> +            results.update(self.model.activity_search({}))
> +
> +        for elem in _query.children:
> +            if elem.name != 'activity':
> +                continue
> +            found = None
> +
> +            # room search
> +            try:
> +                tmp = set(self.model.activity_by_room(elem['room']))
> +                if found is None:
> +                    found = tmp
> +                else:
> +                    found.intersection_update(tmp)
> +            except KeyError:
> +                pass
> +
> +            buddies = []
> +            for e in elem.children:
> +                # properties search
> +                if e.name == 'properties' and e.uri == ns.ACTIVITY_PROP:
> +                    props = parse_properties(e)
> +                    tmp = set(self.model.activity_search(props))
> +                    if found is None:
> +                        found = tmp
> +                    else:
> +                        found.intersection_update(tmp)
> +                if e.name == 'buddy':
> +                    buddies.append(e['jid'])
> +
> +            if len(buddies) > 0:
> +                # participants search
> +                tmp = set(self.model.activity_by_participants(buddies))
> +                if found is None:
> +                    found = tmp
> +                else:
> +                    found.intersection_update(tmp)
> +
> +            if found is not None:
> +                results.update(found)

Much of my commentary on iq_buddy_query() applies here.

> +
> +        for activity in results:
>              activity_elem = query.addElement((ns.ACTIVITY, 'activity'))
>              activity_elem['id'] = str(activity.id)
>              activity_elem['room'] = activity.room
>
> diff --git a/gadget/model.py b/gadget/model.py
> index 1dd0ff4..7f30992 100644
> --- a/gadget/model.py
> +++ b/gadget/model.py
> @@ -44,6 +44,22 @@ class GadgetModel:
>          activity = self.activities[id]
>          return activity
>
> +    def activity_by_room(self, room):
> +        results = []
> +        for activity in self.activities.itervalues():
> +            if activity.room == room:
> +                results.append(activity)
> +
> +        return results
> +
> +    def activity_by_participants(self, buddies):
> +        results = []
> +        for activity in self.activities.itervalues():
> +            if set(buddies).issubset(set(activity.members)):
> +                    results.append(activity)
> +
> +        return results
> +

These lookups seem like they will scale poorly. We shouldn't optimise them
prematurely, but we should develop some stress tests for this code so that we
can find bottlenecks. I have a little (uncommitted) code that stresses the
model directly, but we should also test via the XMPP interface.

>      def activity_search(self, properties):
>          results = []
>
> diff --git a/gadget/ns.py b/gadget/ns.py
> index b85bc20..293d4dd 100644
> --- a/gadget/ns.py
> +++ b/gadget/ns.py
> @@ -2,6 +2,7 @@
>  ACTIVITY = 'http://laptop.org/xmpp/activity'
>  ACTIVITY_PROP = 'http://laptop.org/xmpp/activity-properties'
>  BUDDY = 'http://laptop.org/xmpp/buddy'
> +BUDDY_PROP = 'http://laptop.org/xmpp/buddy-properties'
>  CLIENT = 'jabber:client'
>  DISCO_INFO = 'http://jabber.org/protocol/disco#info'
>  MUC = 'http://jabber.org/protocol/muc'
> diff --git a/gadget/test_component.py b/gadget/test_component.py
> index 17c6625..8530878 100644
> --- a/gadget/test_component.py
> +++ b/gadget/test_component.py
> @@ -118,11 +118,139 @@ class BuddyIqTest2(TestCase):
>          query = stanza.firstChildElement()
>          assert query.name == 'query'
>          elements = list(query.elements())
> +        elements.sort()

I would prefer:

  elements = sorted(query.elements())

>          assert len(elements) == 2
>          assert elements[0].name == 'buddy'
> -        assert elements[0]['jid'] == 'foo'
> +        assert elements[0]['jid'] == 'bar'
>          assert elements[1].name == 'buddy'
> -        assert elements[1]['jid'] == 'bar'
> +        assert elements[1]['jid'] == 'foo'
> +        self.done.callback(None)
> +
> +class BuddyIqTest3(TestCase):
...
> +
> +class BuddyIqTest4(TestCase):
...
> +
> +class BuddyIqTest5(TestCase):
...
>
>  class ActivityIqTest(TestCase):
...
>
> +class ActivityIqTest4(TestCase):
...
> +
> +class ActivityIqTest5(TestCase):
...
> +
> +class ActivityIqTest6(TestCase):
...
> +
> +class ActivityIqTest7(TestCase):
...
> +
> +class ActivityIqTest8(TestCase):
...

There's a lot of repeated/boilerplate code in these tests that makes them hard
to read. We should try and factor out the common elements, either by adding
some convenience functions (easier), or developing a framework that allows us
to write tests directly in terms of XMPP stanzas sent and received (harder,
but I think a bigger win). I imagine the second option involving having a
directory containing text files, where each file is a test consisting of a
trace of stanzas sent and received, similar to what you can do with Python
doctests. The format might be something like:

  -> <iq type="get" from="bob at foo.org" to="gadget at bar.org">
  ->   ...
  -> </iq>

  <- <iq type="result" from="gadget at bar.org" to="bob at foo.org">
  <-  ...
  <- </iq>

The main difficulty I see with this approach is that it's harder to set up
specific scenarios, but perhaps we can allow inserting Python code as well.

  >>> model.buddy_add(...)
  >>> model.activity_add(...)

  -> <iq type="get" from="bob at foo.org" to="gadget at bar.org">
  ...

Another option would be to just use doctests, with some convenience code for
sending/receiving stanzas.

  >>> send('''
  ... <iq type="get" from="bob at foo.org" to="gadget at bar.org">
  ... ...
  ... </iq>''')

  >>> print recv()
  <iq type="result" from="gadget at bar.org" to="bob at foo.org">
  ...
  </iq>

> +
>  class DiscoInfoTest(TestCase):
>      def runTest(self):
>          self.done = defer.Deferred()
> @@ -345,5 +676,4 @@ class PropertiesTest(TestCase):
>          self.server.send(m)
>
>          room = self.service.mucs['chat at conf.foo.org']
> -        assert room.properties['foo'] == ('str', 'hello')
> -
> +        assert room.properties['foo'] == (str, 'hello')
> diff --git a/gadget/test_model.py b/gadget/test_model.py
> index b124f34..02005bc 100644
> --- a/gadget/test_model.py
> +++ b/gadget/test_model.py
> @@ -6,7 +6,8 @@ from gadget.model import GadgetModel
>  class ModelTest(unittest.TestCase):
>      def test_activity_search(self):
>          model = GadgetModel()
> -        model.activity_add(123, 'room at conf.foo.org', {'type': (str, 'Paint')})
> +        activity = model.activity_add(123, 'room at conf.foo.org',
> +                {'type': (str, 'Paint')})
>
>          assert model.activity_by_id(123).id == 123
>          self.assertRaises(KeyError, model.activity_by_id, 124)
> @@ -18,3 +19,36 @@ class ModelTest(unittest.TestCase):
>          results = model.activity_search({'type': (str, 'Draw')})
>          assert len(results) == 0
>
> +        results = model.activity_by_room('room at conf.foo.org')
> +        assert len(results) == 1
> +
> +        results = model.activity_by_room('room at conf.bar.org')
> +        assert len(results) == 0
> +
> +        activity.members.append('alice at foo.org')
> +        results = model.activity_by_participants(['alice at foo.org'])
> +        assert len(results) == 1
> +        results = model.activity_by_participants(['boo at foo.org'])
> +        assert len(results) == 0
> +
> +        activity = model.activity_add(456, 'room2 at conf.foo.org', {})
> +        activity.members.append('alice at foo.org')
> +        activity.members.append('bob at foo.org')
> +        results = model.activity_by_participants(['alice at foo.org'])
> +        assert len(results) == 2
> +        results = model.activity_by_participants(['alice at foo.org', 'bob at foo.org'])
> +        assert len(results) == 1
> +
> +    def test_buddy_search(self):
> +        model = GadgetModel()
> +        model.buddy_add('alice at foo.org', {})
> +
> +        result = model.buddy_by_id('alice at foo.org')
> +        assert result.jid == 'alice at foo.org'
> +        assert result.properties == {}
> +        self.assertRaises(KeyError, model.buddy_by_id, 'bob at foo.org')
> +
> +        model.buddy_update('alice at foo.org', {'color': (str, 'red')})
> +        result = model.buddy_search({'color': (str, 'red')})
> +        assert len(result) == 1
> +        assert result[0].jid == 'alice at foo.org'

Regards,

-- 
Dafydd