OpusVL / OpenERP-XMLRPC-Simple

OpenERP/Odoo XMLRPC wrapper library
2 stars 1 forks source link

[O2122] Fix problem with all-numeric database names #6

Closed Nick-OpusVL closed 4 years ago

Nick-OpusVL commented 4 years ago

All-numeric database names where causing the following traceback from Odoo:

odoo-for-website_1  | 2019-10-04 10:16:14,661 1 ERROR 27082019 odoo.http: 'int' object has no attribute 'startswith'
odoo-for-website_1  | Traceback (most recent call last):
odoo-for-website_1  |   File "/usr/lib/python2.7/dist-packages/odoo/http.py", line 118, in dispatch_rpc
odoo-for-website_1  |     result = dispatch(method, params)
odoo-for-website_1  |   File "/usr/lib/python2.7/dist-packages/odoo/service/common.py", line 57, in dispatch
odoo-for-website_1  |     return g[exp_method_name](*params)
odoo-for-website_1  |   File "/usr/lib/python2.7/dist-packages/odoo/service/common.py", line 23, in exp_login
odoo-for-website_1  |     res = security.login(db, login, password)
odoo-for-website_1  |   File "/usr/lib/python2.7/dist-packages/odoo/service/security.py", line 8, in login
odoo-for-website_1  |     res_users = odoo.registry(db)['res.users']
odoo-for-website_1  |   File "/usr/lib/python2.7/dist-packages/odoo/__init__.py", line 52, in registry
odoo-for-website_1  |     return modules.registry.Registry(database_name)
odoo-for-website_1  |   File "/usr/lib/python2.7/dist-packages/odoo/modules/registry.py", line 59, in __new__
odoo-for-website_1  |     return cls.new(db_name)
odoo-for-website_1  |   File "/usr/lib/python2.7/dist-packages/odoo/modules/registry.py", line 71, in new
odoo-for-website_1  |     registry.init(db_name)
odoo-for-website_1  |   File "/usr/lib/python2.7/dist-packages/odoo/modules/registry.py", line 122, in init
odoo-for-website_1  |     self._db = odoo.sql_db.db_connect(db_name)
odoo-for-website_1  |   File "/usr/lib/python2.7/dist-packages/odoo/sql_db.py", line 695, in db_connect
odoo-for-website_1  |     db, info = connection_info_for(to)
odoo-for-website_1  |   File "/usr/lib/python2.7/dist-packages/odoo/sql_db.py", line 669, in connection_info_for
odoo-for-website_1  |     if db_or_uri.startswith(('postgresql://', 'postgres://')):
odoo-for-website_1  | AttributeError: 'int' object has no attribute 'startswith'
odoo-for-website_1  | 2019-10-04 10:16:14,663 1 INFO 27082019 werkzeug: 172.21.0.12 - - [04/Oct/2019 10:16:14] "POST /xmlrpc/common HTTP/1.0" 200 -

This is due to weak typing and the DWIMmy nature of RPC::XML.

No tests because I don't believe we're in a position to control the test database names that are created for testing purposes. In fact Test::MockOpenERP is written in Perl so we probably wouldn't be able to die in the same way when it's the wrong type anyway.

Not all modified methods tested, so please review very closely.

Nick-OpusVL commented 4 years ago

Revoked for now

Nick-OpusVL commented 4 years ago

Re-opened.

I was going to move it into simple_request, but because there's nothing about that that suggests it always takes dbname as its second argument (even though within the class that is what happens), I decided against it. In other words all the usages might be the same only by coincidence, so we could break other usages of simple_request that might be used in other packages if I did that refactoring.

Nick-OpusVL commented 4 years ago

I think @JJ-OpusVL's review would be most useful here because he's been involved longer with the OOM so might foresee issues I can't. I'd also like to know if he agrees with my comment about things being "same by coincidence" because if we can confidently only change one common method that would be better from a DRY perspective. https://github.com/OpusVL/OpenERP-XMLRPC-Simple/pull/6#issuecomment-538360770

However I'd still like @PaulGWebster to read and accept this because he's the one (other than me)who's going to have to work with it into the future.

Nick-OpusVL commented 4 years ago

Approving this @PaulGWebster but let me know any retrospective thoughts if you have any, or raise a PR to fix anything you have problems with.