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.

That's quite the effort you've made to trace through some of the ickiest code in Roundup :-)

The data quality issue in the various *Property classes - in particular the Link and Multilink 
ones - should probably be handled at the templating level, rather than down in the database 
(potentially masking other bugs).

To that end I think it's reasonable to have make_sort_function() do the right thing. It should 
*expect* to be passed lists with mixed content (ids and labels). I think a reasonable solution 
could be:


def make_sort_function(db, classname, sort_on=None):
    """Make a sort function for a given class.
    
    The list being sorted may contain mixed ids and labels.
    """
    linkcl = db.getclass(classname)
    if sort_on is None:
        sort_on = linkcl.orderprop()
    def sortfunc(a, b):
        if num_re.match(a):
            a = linkcl.get(a, sort_on)
        if num_re.match(b):
            b = linkcl.get(b, sort_on)
        return cmp(a, b)
    return sortfunc

So you suggest to check it in templating.py:make_sort_function rather
that in rdbms_common.py:getnode?
Fixing it there makes sense, but adding a check in getnode too might be
safer (assuming that all the nodeids are numeric (also are they always
supposed to be passed as string? I managed to pass them as ints while
working at the roundup weekly summary script and it worked fine;
checking with a regex in getnode without checking the type will break it)).

I can provide a patch to add the check (in make_sort_function or in
both) and a separate patch to refactor lookupIds and some of its calls a
little without changing its behavior (i.e. put an "if do_lookup:" before
"cl = db.getclass(prop.classname)" and replace calls that use
do_lookup=False and fail_ok=True to just use list() (I'm not sure a copy
of the list is really needed, but that's what happen now)).

I've made make_sort_function more robust as discussed. If you wish to supply a patch for 
additional changes that'd be fine.

Your fix seems to work fine (thanks!), so I think this can be closed.

Ezio, thanks for the feedback and the confirmation on the issue!