Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix auto registration of job methods and channels #69

Merged
merged 1 commit into from
May 25, 2018

Conversation

guewen
Copy link
Member

@guewen guewen commented May 22, 2018

The automatic registration of job methods and their defaut channel
has been a bit chaotic. The initial version for the new Odoo API could
crashing as soon as a method was decorated by @Property. There is
such a property in the code code, the method '_cache'.

The problem of the crash was that, the introspection basically uses a
'getattr' on every attribute of the instance. The '_cache' method
could then be called on an empty recordset, which is not supported by
'_cache'.

A first correction (49d8f37) was to naively skip the '_cache' method
from the introspection.

In any case, it is wrong to access the property of an instance only to
instrospect its members. That's why the correction 4ebb245 changed the
inspection from the instance to the class.

Properties are no longer accessed, however this correction was not
correct for Python 3. When members of the class are introspected, they
are neither bound neither unbound methods. they are mere functions.

The change here is to lookup for functions. _register_job must now takes
the model as input argument, because there is no way to get the name of
the model from the function.

Closes #64
Follows #50

@guewen
Copy link
Member Author

guewen commented May 22, 2018

Note: the behavior is different between python2 and python3

PY3:

Python 3.5.2 (default, Nov 23 2017, 16:37:01)
[GCC 5.4.0 20160609] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import inspect
>>> class A():
...     def test(self):
...         pass
...
>>> A.test
<function A.test at 0x7f6e4b6a29d8>
>>> A().test
<bound method A.test of <__main__.A object at 0x7f6e4bffd630>>
>>> inspect.getmembers(A, predicate=inspect.ismethod)
[]
>>> inspect.getmembers(A(), predicate=inspect.ismethod)
[('test', <bound method A.test of <__main__.A object at 0x7f988c7fc550>>)]
>>> inspect.getmembers(A, predicate=inspect.isfunction)
[('test', <function A.test at 0x7f988bea19d8>)]
>>> inspect.getmembers(A(), predicate=inspect.isfunction)
[]

PY2

Python 2.7.12 (default, Dec  4 2017, 14:50:18)
[GCC 5.4.0 20160609] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> import inspect
>>> class A(object):
...     def test(self):
...         pass
...
>>> A.test
<unbound method A.test>
>>> A().test
<bound method A.test of <__main__.A object at 0x7f2883429750>>
>>> inspect.getmembers(A, predicate=inspect.ismethod)
[('test', <unbound method A.test>)]
>>> inspect.getmembers(A(), predicate=inspect.ismethod)
[('test', <bound method A.test of <__main__.A object at 0x7f22066f9250>>)]
>>> inspect.getmembers(A, predicate=inspect.isfunction)
[]
>>> inspect.getmembers(A(), predicate=inspect.isfunction)
[]

So the fix for 10.0 will be different.

Copy link
Contributor

@simahawk simahawk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@simahawk
Copy link
Contributor

simahawk commented May 23, 2018

FTR runbot is only yellow: usual RST parsing warnings.
BTW I wonder if we could get rid of those: seems missing pygments pkg is the cause...

Copy link
Sponsor Contributor

@lmignon lmignon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@guewen Do you plan to change the version number later on the master branch?

@guewen
Copy link
Member Author

guewen commented May 25, 2018

@lmignon Yes, I'd like to merge this one and #72 then bump the version of 11.0. (#54 could be nice as well)

The automatic registration of job methods and their defaut channel
has been a bit chaotic. The initial version for the new Odoo API could
crashing as soon as a method was decorated by @Property. There is
such a property in the code code, the method '_cache'.

The problem of the crash was that, the introspection basically uses a
'getattr' on every attribute of the instance. The '_cache' method
could then be called on an empty recordset, which is not supported by
'_cache'.

A first correction (49d8f37) was to naively skip the '_cache' method
from the introspection.

In any case, it is wrong to access the property of an instance only to
instrospect its members.  That's why the correction 4ebb245 changed the
inspection from the instance to the class.

Properties are no longer accessed, however this correction was not
correct for Python 3. When members of the class are introspected, they
are neither bound neither unbound methods. they are mere functions.

The change here is to lookup for functions. _register_job must now takes
the model as input argument, because there is no way to get the name of
the model from the function.

Closes OCA#64
Follows OCA#50
@guewen guewen force-pushed the 11.0-fix-job-register-funcs branch from 5f6e3c6 to a5e0860 Compare May 25, 2018 12:57
Copy link

@p-tombez p-tombez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@lmignon lmignon merged commit b10dc1d into OCA:11.0 May 25, 2018
@lmignon
Copy link
Sponsor Contributor

lmignon commented May 25, 2018

@guewen everything is merged 😏

@guewen
Copy link
Member Author

guewen commented May 25, 2018

@lmignon thanks! Bumped the addon with a changelog :)

@lmignon
Copy link
Sponsor Contributor

lmignon commented May 25, 2018

Thank you @guewen !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants