[sugar] review: gadget buddy search code

[Guillaume Desmottes]

Le mercredi 14 mai 2008 ? 22:52 +0100, Dafydd Harries a ?crit :
> > +# 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.
> 

Gabble supports the following types: str, bytes, int, uint and bool.

I started to write tests for parse_properties and properties_to_xml but
I encountered a problem.
Currently we store properties in a dict like {'key': (type, 'value')} as
{'color': (str, 'red')} for example.

But how should I store an uint as Python doesn't have an uint type?
Using dbus type? Or we could store the string representation of the type
instead of a python type ('str' instead of str).

And how should we store the value? Using the type-converted value (so
the numerical value for int/uint, True/False for bool, etc) or the
string version?


> > @@ -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.
> 
Good idea. Implemented.


> > +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.
> 
Right, fixed.


> >  @@ -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.
> 

fixed.


> >  +
> >  +        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.
> 

Don't know, I implemented the protocol described on
http://wiki.laptop.org/go/XMPP_Component_Protocol



> > +            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.
> 
fixed.


> 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:
> 

buddy_by_id can raise the exception too. I made this code clearer.

>   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"?
> 
done


> > +
> > +            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)
> 

Seems sane. done.


> 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.

Agree. done.


> > +    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.
> 

I added a comment warning about this.


> > @@ -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())
> 

Fixed.
I also add a cmp function so elements are sorted in a more predictable
way.

> >          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>
> 


I totally agree, having "XML oriented" test would be a big win. But I
think we could do that as as separated branch so this test refactoring
won't block the merge.



Thanks for your review,


	G.