[storm] Dangerous initialisation of storm.properties.Property

[Jamu Kakar]

Hi Michael,

I'm CC'ing the Storm list, since these kinds of conversations are nice
to have in the open.

On Tue, Aug 23, 2011 at 12:35 PM, Michael Löffler <ml at redcowmedia.de> wrote:
> as you seem to be involved in active developement of storm, and as it's just
> a minor thing, I'd like to suggest one change directly to you. I saw the
> initialisation of Property:
>
> class Property(object):
>
>    def __init__(self, name=None, primary=False,
>                 variable_class=Variable, variable_kwargs={}):
>        self._name = name
>
> In my eyes, using {} in the function header here is quite dangerous, as it
> can lead to side effects, which I think in the case of the Property object
> are not really expected and wished:
>
> In [1]: class Test(object):
>   ...:     def __init__(self, data={}):
>   ...:         self.data = data
>
> In [2]: a = Test()
>
> In [3]: a.data
> Out[3]: {}
>
> In [4]: a.data[3] = 5
>
> In [5]: c = Test()
>
> In [6]: c.data
> Out[6]: {3: 5}
>
> In this special case, it looks like most of the time, the empty dictionary
> should not be touched afterwards. But in case it happens, it would also
> change properties in places, where it would not be expected.

In this particular case variable_kwargs is eventually expanded when
it's used to instantiate a PropertyColumn, so there is no real safety
issue here.  Also, Property._variable_kwargs is a private attribute,
so no one should be messing with it without knowing what they're
doing.

Anyway, I tend to agree that defining an empty dict in a keyword
argument isn't so great, but I guess it was a micro-optimization to
avoid creating a new empty dict for every Property that gets
instantiated.

Thanks,
J.