GMGTableBase uses default kwarg of {} in methods

[nyergler]

A few places in GMGTableBase have method definitions that look something like:

    def find_one(cls, query_dict={}):
       ...

This is almost certainly not what the authors intended. The default argument list is only evaluated once during execution, at import time. The code above creates a single dictionary as the default, and if this is mutated down the line, the mutated value becomes the default for subsequent calls. In these particular cases it's unlikely to cause problems -- that argument will almost certainly be passed in -- but in the event it's not, this can cause very strange, difficult to diagnose behavior. The correct approach is to use an immutable value as an argument default.

Note that this same problem exists for class level attributes: defining a class such as:

    class Foo(object):

        bar = {}

means that for *every* instance of Foo, self.bar will point to the *same* dict.

[nyergler]

I've pushed a fix for this to my personal clone and opened a merge request.

​https://gitorious.org/~nyergler/mediagoblin/nyerglers-mediagoblin/commits/552-kwarg-default-cleanup

[Elrond]

Problem at hand

First of all: Thanks for catching this!

The answer is manyfold:

1. These functions should not modify an empty dict, really. If they do, something is completely wrong with them (despite the problem in this bug).
2. These functions were written to emulate monogo(kit).
   1. They are allowed to go away. In fact, spaetz has started on this task.
   2. In a first step, one can kill the default argument and fix every caller that expects a default argument.

Your suggested patch

I think your suggested patch is wrong.
_fix_query_dict modifies in place, right. So passing in query_dict or {} will either modifiy query_dict (good!) or just modify the {} and leave query_dict as None which will nicely fail the (*query_dict) in the next line.
Let me know, if I miss something.

My suggestion:

... query_dict=Nnone):
       if query_dict is None:
           query_dict = {}

But the real fix is to find all callers using the default arg and either rewrite them in sqlalchemy or (easier!) rewrite them to pass {} in and be done.

[nyergler]

Oh, you're totally right, I wasn't paying attention. I'll update my branch with a corrected patch and update here.

Thanks for reviewing.

[spaetz]

Hi all, thanks for highlighting this issue. a) I have a branch ready to remove the _fix_query_dict function completely as it does away with the ._id -> .id aliasing.

But until that is merged, the code should obviously be fixed, I have an even better suggestion than Elrond. _fix_query_dict is only ever useful with a non-empty query dict in any case, so I suggest:

    def find(cls, query_dict=None):
        if query_dict:
            _fix_query_dict(query_dict)
        return cls.query.filter_by(**query_dict)

How about that?
P.S. Ahh, just noticed that this might pass down "None" as kwarg, which is obviously wrong. So I concur with Elrond's solution.

[nyergler]

Thanks for the feedback, Spaetz and Elrond; I've updated the merge request per Elrond's suggestion.

[Elrond]

I really think this was merged also. Today...

Can someone confirm this?

[spaetz]

this was merged in today indeed