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

[ADD] base_json_request: Module to allow standard JSON requests #3

Open
wants to merge 1 commit into
base: 10.0
Choose a base branch
from

Conversation

lasley
Copy link

@lasley lasley commented Oct 10, 2017

  • Create a module that circumvents the JSON-RPC requirement for all requests with application/json

This logic is taken from my REST API work in OCA/server-tools#889. It has been working well on multiple productions for a few months now. Note that none of these productions have multiple databases, which is an important detail in this instance (because this is a monkey patch).

That said, I do not intend to provide compatibility for multi-tenant here. I assert that the concept of a webhook and the concept of a multi-tenant Odoo instance are at odds with each other in most cases.

While multi-tenant is still a possibility while just using this module, the next PR I submit (base_web_hook - #4) will have an explicit note that this is not possible. IMO it is worth having the ability for a web hook, as long as the possible sacrifice is known. This may be possible to circumvent with database filters, but I haven't tested.

cc @moylop260 (I think you guys were looking for something like this for Gitlab webhooks)

Note: I still need to add tests, but the functionality is here to review.

* Create a module that circumvents the JSON-RPC requirement for all requests with application/json
@lasley lasley added this to the 10.0 milestone Oct 10, 2017
'name': 'Base JSON Request',
'summary': 'Allows you to receive JSON requests that are not RPC.',
'version': '10.0.1.0.0',
'category': 'Authentication',
Copy link
Author

Choose a reason for hiding this comment

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

Not


This module allows you to receive JSON requests in Odoo that are not
RPC.

Copy link
Author

@lasley lasley Oct 16, 2017

Choose a reason for hiding this comment

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

Add note that route needs to be defined with type=json

@lmignon
Copy link
Sponsor

lmignon commented Oct 16, 2017

@lasley after oca-decorators, oca-jsonrequests 😏 Since this module is not designed to support multi-tenant and extend/fix the odoo api, it could be more appropriate to provide this functionality as a python lib. You can find a example on how you can monkey patch the odoo package from an other python package here https://github.com/acsone/odoo-autodiscover/blob/master/https://github.com/acsone/odoo-autodiscover/blob/master/.
In short odoo_autodiscover uses a Site-specific configuration hook (zzz_odoo_autodiscover.pth) to execute code during the initialisation of the python interpreter (https://docs.python.org/2/library/site.html)
As odoo_autodiscover you can use the wrapt python package to monkey patch Odoo. In your case you should maybe call the method wrap.wrap_function_wrapper from the hook file to patch Odoo. Here it's a link to an article describing how this lib can be use to monkey patch python code http://blog.dscpl.com.au/2015/03/safely-applying-monkey-patches-in-python.html.
IMO it's safer to provide this kind of monkey patch as a python package since this change is very intrusive and changes the Odoo api transversally.
It's my opinion and I'll not block this PR since you fix an annoying limitation in Odoo. Do you have tried to propose this change on Odoo master?

@lasley
Copy link
Author

lasley commented Oct 16, 2017

Thank you for the insight @lmignon

it could be more appropriate to provide this functionality as a python lib.

I think I agree with you, and I have no problem making this a lib instead. Thanks for the examples, they will keep me from redesigning wheels.

We do have a dissenting opinion on the using of external libs versus Odoo modules in OCA/server-tools#944 (comment) - the arguments I believe could be applied here as well. The overall strategy of using libraries is being questioned, so I'm going to bring this one over here & apply whatever we decide there to here too.

@lasley
Copy link
Author

lasley commented Oct 16, 2017

Do you have tried to propose this change on Odoo master?

Oops forgot this point. I could look into this, yes. odoo/odoo#7766 (comment) seemed to allude to Odoo SA being willing to accept a patch, but I need this in v10 unfortunately. I'll likely submit something though, yeah.

@lmignon
Copy link
Sponsor

lmignon commented Oct 17, 2017

@lasley

Oops forgot this point. I could look into this, yes. odoo/odoo#7766 (comment) seemed to allude to Odoo SA being willing to accept a patch, but I need this in v10 unfortunately. I'll likely submit something though, yea

We've observed that it's easier to propose enhancement on the core Odoo with PR on master. By doing this we've a chance to get this functionality supported into the next version of Odoo. In the meantime, the only solution to support this functionality is via an external lib.

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

Successfully merging this pull request may close these issues.

None yet

2 participants