Row.__getattr__ raises AttributeError when trying to access '_extra'

[gi0baro]

Just spotted. I have this Row returned from pyDAL:

<Row {'users': {'id': 26470719833643260735021266671L}, '_extra': {'registration_id': '', 'last_name': 'Barillari', 'first_name': 'Giovanni', 'reset_password_key': '', 'password': '***', 'registration_key': 'b40bde36-258a-48fb-b565-ec22cabc0586', 'email': '***'}}>

In v15.03 was working as expected. In current master the problem seems caused by the difference behavior between Row.__getitem__ and Row.__getattr__.
@ilvalle maybe a consequence of your refactoring with BasicStorage?
I propose to add a:

__getattr__ = __getitem__

in Row class.

[ilvalle]

Can you double check with https://github.com/web2py/pydal/tree/v15.05.29?

[gi0baro]

@ilvalle 15.05.* seems to be affected too.
Looks correlated to the fix you made for get(): 6480b27

[ilvalle]

I see, however it is strange that tests are still passing, probably they are not enough.
You can add the fix proposed, I suggest adding a test for this issue too

[gi0baro]

@ilvalle maybe depends on how attributes are added as normal keys or under _extra attribute. I had never inspected the flow. Will push a PR later today.

[ilvalle]

Is it still valid? Can you provide a failing example?

[gi0baro]

I think __getattr__ should behave as __getitem__.

[gi0baro]

Update: actually I've re-tested this with latest master. The issue will show up only if you try to access something inside _extra as attributes:

>>> row['_extra']['asd']
'lol'
>>> row['asd']
'lol'
>>> row._extra.asd
'lol'
>>> row.asd
Traceback (most recent call last):
  File "<console>", line 1, in <module>
  File "/home/sellf/pydal/pydal/helpers/classes.py", line 348, in __getattr__
    raise AttributeError
AttributeError

Now, since in _extra you usually have keys like 'COUNT(contacts.id)', I don't know if there are conditions where _extra items are accessible as attributes.
@ilvalle @mdipierro should we consider this not reproducible and close it? Do you know any condition where _extra items have proper naming to be accessible as attributes?

[ilvalle]

I don't think is documented anywhere

Paolo

2015-07-02 10:42 GMT+02:00 Giovanni Barillari notifications@github.com:

Update: actually I've re-tested this with latest master. The issue will
show up only if you try to access something inside _extra as attributes:

row['_extra']['asd']'lol'>>> row['asd']'lol'>>> row._extra.asd'lol'>>> row.asd
Traceback (most recent call last):
File "", line 1, in
File "/home/sellf/pydal/pydal/helpers/classes.py", line 348, in getattr
raise AttributeErrorAttributeError

Now, since in _extra you usually have keys like 'COUNT(contacts.id)', I
don't know if there are conditions where _extra items are accessible as
attributes.
@ilvalle https://github.com/ilvalle @mdipierro
https://github.com/mdipierro should we consider this not reproducible
and close it? Do you know any condition where _extra items have proper
naming to be accessible as attributes?

—
Reply to this email directly or view it on GitHub
#229 (comment).

[gi0baro]

@mdipierro I'm gonna close this as invalid. Feel free to re-open if you think is a valid bug.

[gi0baro]

@ilvalle hints on how to solve this?

[mdipierro]

Although theory I agree that__getattr__ should behave as getitem, in practice they did not. This is because in
row.xxx
row['yyy']
xxx is constraint to be alphanumeric and therefore has to be a field (and we can optimize for this) or a table (in result of joins) while yyy can be an expression (from _extra) or a 'table.field'. I do not object to having a single underlying implementation unless we make it slower by ignoring the fact that they they serve different purposes. Has anybody test the speed of the code after the change? Some time ago we spent huge amount of time refactoring this code since it is a web2py bottle neck.

[ilvalle]

According to my test, we have better performance now.
Surely, placing __getattr__ = __getitem__ in Row will have its impact. Before proposing the new implementation of Row I didn't know you could access an element of _extra directly.
As written in #246 (comment), my proposal is to change the current approach: writing db(db.tt).select('vv') should output <Row {'vv': 1L}> which is the output of db(db.tt).select(db.tt.vv) . Now we have <Row {'_extra': {'vv': 1}}>. By doing that, we can avoid to add __getattr__ = __getitem__ in Row.
I don't see why we should have two different results for the same select.

[mdipierro]

OK. As long as performance was tested I think the code is fine as it is now. Can we close this issue?

[gi0baro]

@mdipierro don't close this until we successfully decided how to manage this. #246 is a duplicated and a consequence of this.

[gi0baro]

@ilvalle should we wait for the change you proposed in #246 to release 15.07?

[ilvalle]

For me is ok, if it doesn't decrease the overall performance.

Paolo

2015-07-08 22:00 GMT+02:00 Giovanni Barillari notifications@github.com:

@ilvalle https://github.com/ilvalle should we wait for the change you
proposed in #246 #246 to release
15.07?

—
Reply to this email directly or view it on GitHub
#229 (comment).