[sugar] review: Guillaume's 'roster' branch of Gadget

[Guillaume Desmottes]

Le mardi 27 mai 2008 ? 14:08 +0100, Dafydd Harries a ?crit :
> > +        for jid in self.roster.jids:
> > +            # FIXME: How can I find the right 'from' ?
> > +            self.send_presence("gadget.localhost", jid)
> > +
> 
> Isn't the right from address the same address as we authenticated to the
> jabber server with? This is the same as config['domain'], which can be
> accessed here as self.parent.thisEntity.full().
> 

I got a problem here.
When Gadget is run as a daemon (using twisted), self.parent is a
ServiceManager manager instance and we have its jid in
self.parent.jabberId.
But when it's run in the test environnement, self.parent is a XmlStream
and we have the jid in self.parent.thisHost.

I guess the test environnement is wrong?
Didn't find how to solve it (tbh, I'm a bit lost in all the twisted
component involved).

> Should we send presence probes to all our contacts here too?
> 

Not sure. Gadget never sends presence probe. Just "normal" presence.

According to http://www.xmpp.org/rfcs/rfc3921.html#presence 5.1.3.,
presence probes are only used by peer's server, so I'm not sure if
Gadget should send one or not.

> > +class GadgetRoster:
> > +    def __init__(self, path):
> > +        self.path = path
> > +
> > +        try:
> > +            lines = file(path, 'r').readlines()
> > +        except IOError:
> > +            self.jids = set()
> 
> I'm a bit worried that we might lose errors here. Not all IO errors are "file
> not found".

good point. I changed that to use os.path.isfile instead.


> > +        else:
> > +            # JID's have to contain one and only one '@'
> > +            jids = filter(lambda x: x.count('@') == 1, lines)
> 
> Are we expecting that the roster might contain invalid JIDs somehow? This
> seems a bit strange for me.
> 
It shouldn't but it doesn't hurt to be prudent and check IMHO.

> At any rate, if we are, we should use twisted.jabber.words.jid.parse() to
> validate JIDs.
> 

done.

> > +            # remove ending '\n'
> > +            self.jids = set(map(lambda x: x.strip(), jids))
> > +
> > +    def save(self):
> 
> Perhaps this method should be prefixed with an underscore.
> 
done.

> > +        file(self.path, 'w').write(reduce(lambda x,y: "%s\n%s" % (x,y), self.jids))
> 
> There's a problem here: opening the file for writing might succeed, in which
> case it will be truncated, but writing to the file might fail, in which case
> we would lose the roster data. The correct approach is to write the new roster
> to a temporary file and rename it over the old one.
> twisted.python.filepath.FilePath implements this behaviour; we could use that.
> 

done.

> reduce() can be inefficient for large lists. Perhaps this would be better:
> 
>   ''.join('%s\n' % x for x in jids)
> 
> (By using a generator expression, the temporary strings can be garbage
> collected as they are used.)
> 

Didn't know that, thanks for explanation.
fixed.

> In terms of IO, writing the entire roster out on every change might be
> expensive. Another option would be to append JIDs to the file. A third option
> would be to use SQLite, which would also take care of the data loss problem.
> 

SQLite is not a bad idea. I added a comment about it and will
investigate it later.


> > +# FIXME: can we use this path?
> > +ROSTER_PATH = '/tmp/roster'
> > +
> 
> No we can't! This is vulnerable to a symlink attack bug where an attacker
> makes /tmp/roster point to one of your files. Unlinking the file first won't
> help you because there is always a race. Writing to fixed locations in shared
> directories is never a good idea. Always use the tempfile module for such
> things.
> 
> This is moot, however, because I'm going to recommend that we use an in-memory
> roster object for the tests. In fact, the roster object can just be a set,
> because the API is just the add() and remove() functions.
> 

Good idea. Done.


> > +        # FIXME: presence2 is called twice if we send this stanza
> > +        self.done.callback(None)
> > +
> 
> This is surprising and should be investigated. This is not how
> addOnetimeObserver() is supposed to behave.
> 

nod.

> > +    def later(self):
> > +        assert self.roster.jids == set(['bob at foo.org'])
> >          self.done.callback(None)
> >  
> >  class ProbePresenceTest(TestCase):
> > diff --git a/gadget/test_roster.py b/gadget/test_roster.py
> > new file mode 100644
> > index 0000000..e4af0ff
> > --- /dev/null
> > +++ b/gadget/test_roster.py
> > @@ -0,0 +1,44 @@
> > +import os
> > +import unittest
> > +
> > +from gadget.roster import GadgetRoster
> > +
> > +# FIXME: can we use this path?
> > +ROSTER_PATH = '/tmp/roster'
> > +
> 
> Again, we should change this.
> 

fixed.


thanks for the review,


	G.