Roundup Tracker - Issues

Message4109

Author ezio.melotti
Recipients ezio.melotti
Date 2010-08-10.08:14:30
Message-id <1281428072.19.0.39171953094.issue2550663@psf.upfronthosting.co.za>
In-reply-to
While I was trying to debug
http://psf.upfronthosting.co.za/roundup/meta/issue336 I found out a few
weird things about the lookupIds function in the templating module. This
is the code of the function (without comments and docstring):
def lookupIds(db, prop, ids, fail_ok=0, num_re=num_re, do_lookup=True):
    cl = db.getclass(prop.classname)
    l = []
    for entry in ids:
        if do_lookup:
            try:
                item = cl.lookup(entry)
            except (TypeError, KeyError):
                pass
            else:
                l.append(item)
                continue
        if fail_ok or num_re.match(entry):
            l.append(entry)
    return l

First of all, if do_lookup is False the cl var is useless, so it should
be created inside the if.
Second, if do_lookup is False and fail_ok is True the function just
return a copy of ids.
Third, if both do_lookup and fail_ok are True, the function returns half
of the arguments converted and the other half as they are. This was what
caused the bug I was trying to debug (i.e., when invalid names are in
the nosy, this function returns a list of invalid names and valid ids,
and the invalid names are passed to the db when it expects numeric ids).

An example of this is in the MultilinkHTMLProperty class
(templating.py). This is apparently used for the nosy list and when it's
instantiated the following things happen:
1) MultilinkHTMLProperty.__init__ calls the __init__ of its superclass
HTMLProperty;
2) HTMLProperty.__init__ calls lookupIds passing the names in the nosy
list with do_lookup and fail_ok set to True (so only the valid names in
the nosy are converted to ids);
3) HTMLProperty.__init__ stores the resulting list (invalid names mixed
with valid ids) in self._value;
4) MultilinkHTMLProperty.__init__ calls lookupIds passing self._value
with do_lookup=False and fail_ok=True;
5) lookupIds returns a copy of the list;
6) that copy of the list is then sorted to be stored to self._value.
7) the sorting tries to get some info from the db and sends an invalid
query ("select ... from _users where id='invalidname'");
8) the db gets blocked because the query is invalid;
9) at the next (possibly valid) query the db raises a
psycopg2.InternalError exception because the db is blocked and it's not
possible to do other queries and an error page is displayed.

I'm not sure what is the best way to fix this.
One solution is to check that nodeid is numeric in
rdbms_common.py:getnode before the sql call and raise an error if it's
not numeric. This just abort the sorting and the page is rendered
properly with a message saying "xxx name is invalid" on the top in a red
box. This is probably a good idea assuming that nodeid must be numeric.
Another solution would be calling the lookupIds function with other
flags in order to avoid mixing invalid names and valid ids.
Otherwise the lookupIds can be changed to never return mixed lists, but
I don't know if other functions "need" this behavior.
The lookupIds function and some of its called (i.e. the ones with
do_lookup=False and fail_ok=True) should be refactored anyway.
History
Date User Action Args
2010-08-10 08:14:34ezio.melottisetrecipients: + ezio.melotti
2010-08-10 08:14:32ezio.melottisetmessageid: <1281428072.19.0.39171953094.issue2550663@psf.upfronthosting.co.za>
2010-08-10 08:14:32ezio.melottilinkissue2550663 messages
2010-08-10 08:14:30ezio.melotticreate