From 4b6fcbefa9aa1b5beb44456799f61af1f409fbb2 Mon Sep 17 00:00:00 2001 From: "Moises Lopez - https://www.vauxoo.com/" Date: Tue, 24 May 2022 18:41:25 -0500 Subject: [PATCH] [ADD] external-request-timeout: It could wait for a long time (#370) Calling external request needs to use a timeout in order to avoid waiting for a long time It could be even worse if you have one or many records locked then calling a request it could accumulate a lot of locks and the workers could be used for more time and the system could be down waiting for release You can define your custom methods using the parameter: `--external_request_timeout_methods` The default methods are: - http.client.HTTPConnection - http.client.HTTPSConnection - odoo.addons.iap.models.iap.jsonrpc - requests.delete - requests.get - requests.head - requests.options - requests.patch - requests.post - requests.put - requests.request - serial.Serial - smtplib.SMTP - suds.client.Client - urllib.request.urlopen --- pylint_odoo/checkers/no_modules.py | 79 ++++++++- pylint_odoo/test/main.py | 1 + .../broken_module/models/broken_model.py | 163 ++++++++++++++++++ 3 files changed, 242 insertions(+), 1 deletion(-) diff --git a/pylint_odoo/checkers/no_modules.py b/pylint_odoo/checkers/no_modules.py index 138fe263..5d010028 100644 --- a/pylint_odoo/checkers/no_modules.py +++ b/pylint_odoo/checkers/no_modules.py @@ -23,6 +23,7 @@ visit_expr visit_extslice visit_for + visit_import visit_importfrom visit_functiondef visit_genexpr @@ -144,6 +145,12 @@ 'str-format-used', settings.DESC_DFLT ), + 'E%d06' % settings.BASE_NOMODULE_ID: ( + 'Use of external request method `%s` without timeout. ' + 'It could wait for a long time', + 'external-request-timeout', + settings.DESC_DFLT + ), 'C%d01' % settings.BASE_NOMODULE_ID: ( 'One of the following authors must be present in manifest: %s', 'manifest-required-author', @@ -302,6 +309,23 @@ # From odoo/odoo 10.0: odoo/odoo/fields.py:29 'digits_compute:digits', 'select:index' ] +DFTL_EXTERNAL_REQUEST_TIMEOUT_METHODS = [ + "http.client.HTTPConnection", + "http.client.HTTPSConnection", + "odoo.addons.iap.models.iap.jsonrpc", + "requests.delete", + "requests.get", + "requests.head", + "requests.options", + "requests.patch", + "requests.post", + "requests.put", + "requests.request", + "serial.Serial", + "smtplib.SMTP", + "suds.client.Client", + "urllib.request.urlopen", +] class NoModuleChecker(misc.PylintOdooChecker): @@ -413,8 +437,29 @@ class NoModuleChecker(misc.PylintOdooChecker): 'help': 'List of valid missing return method names, ' 'separated by a comma.' }), + ('external_request_timeout_methods', { + 'type': 'csv', + 'metavar': '', + 'default': DFTL_EXTERNAL_REQUEST_TIMEOUT_METHODS, + 'help': 'List of library.method that must have a timeout ' + 'parameter defined, separated by a comma. ' + 'e.g. "requests.get,requests.post"' + }), ) + def visit_module(self, node): + """Initizalize the cache to save the original library name + of all imported node + It is filled from "visit_importfrom" and "visit_import" + and it is used in "visit_call" + All these methods are these "visit_*" methods are called from pylint API + """ + self._from_imports = {} + + def leave_module(self, node): + """Clear variables""" + self._from_imports = {} + def open(self): super(NoModuleChecker, self).open() self.config.deprecated_field_parameters = \ @@ -589,6 +634,7 @@ def visit_print(self, node): 'translation-contains-variable', 'print-used', 'translation-positional-used', 'str-format-used', 'context-overridden', + 'external-request-timeout', ) def visit_call(self, node): infer_node = utils.safe_infer(node.func) @@ -750,6 +796,27 @@ def visit_call(self, node): if self._check_sql_injection_risky(node): self.add_message('sql-injection', node=node) + # external-request-timeout + lib_alias = self.get_func_lib(node.func) + # Use dict "self._from_imports" to know the source library of the method + lib_original = self._from_imports.get(lib_alias) or lib_alias + func_name = self.get_func_name(node.func) + lib_original_func_name = ( + # If it using "requests.request()" + "%s.%s" % (lib_original, func_name) if lib_original + # If it using "from requests import request;request()" + else self._from_imports.get(func_name)) + if lib_original_func_name in self.config.external_request_timeout_methods: + for argument in misc.join_node_args_kwargs(node): + if not isinstance(argument, astroid.Keyword): + continue + if argument.arg == 'timeout': + break + else: + self.add_message( + 'external-request-timeout', node=node, + args=(lib_original_func_name,)) + @utils.check_messages( 'license-allowed', 'manifest-author-string', 'manifest-deprecated-key', 'manifest-required-author', 'manifest-required-key', @@ -911,12 +978,22 @@ def visit_functiondef(self, node): node.name not in self.config.no_missing_return: self.add_message('missing-return', node=node, args=(node.name)) - @utils.check_messages('openerp-exception-warning') + @utils.check_messages('external-request-timeout') + def visit_import(self, node): + self._from_imports.update({ + alias or name: "%s" % name + for name, alias in node.names + }) + + @utils.check_messages('openerp-exception-warning', 'external-request-timeout') def visit_importfrom(self, node): if node.modname == 'openerp.exceptions': for (import_name, import_as_name) in node.names: if import_name == 'Warning' and import_as_name != 'UserError': self.add_message('openerp-exception-warning', node=node) + self._from_imports.update({ + alias or name: "%s.%s" % (node.modname, name) + for name, alias in node.names}) @utils.check_messages('class-camelcase') def visit_classdef(self, node): diff --git a/pylint_odoo/test/main.py b/pylint_odoo/test/main.py index 26fd1122..c726db83 100644 --- a/pylint_odoo/test/main.py +++ b/pylint_odoo/test/main.py @@ -30,6 +30,7 @@ 'duplicate-po-message-definition': 2, 'duplicate-xml-fields': 9, 'duplicate-xml-record-id': 2, + 'external-request-timeout': 47, 'file-not-used': 6, 'incoherent-interpreter-exec-perm': 3, 'invalid-commit': 4, diff --git a/pylint_odoo/test_repo/broken_module/models/broken_model.py b/pylint_odoo/test_repo/broken_module/models/broken_model.py index cd43027f..319dc76a 100644 --- a/pylint_odoo/test_repo/broken_module/models/broken_model.py +++ b/pylint_odoo/test_repo/broken_module/models/broken_model.py @@ -3,6 +3,38 @@ import psycopg2 from psycopg2 import sql from psycopg2.sql import SQL, Identifier +import requests +from requests import ( + delete, get, head, options, patch, post, put, request) +from requests import ( + delete as delete_r, get as get_r, head as head_r, + options as options_r, patch as patch_r, post as post_r, + put as put_r, request as request_r) + +import urllib +from urllib.request import urlopen +from urllib.request import urlopen as urlopen_r + +import suds.client +from suds.client import Client +from suds.client import Client as Client_r + +import http.client +from http.client import HTTPConnection, HTTPSConnection +from http.client import ( + HTTPConnection as HTTPConnection_r, + HTTPSConnection as HTTPSConnection_r, +) + +import smtplib +import smtplib as smtplib_r +from smtplib import SMTP +from smtplib import SMTP as SMTP_r + +import serial +import serial as serial_r +from serial import Serial +from serial import Serial as Serial_r from openerp import fields, models, _ from openerp.exceptions import Warning as UserError @@ -14,6 +46,10 @@ import openerp.addons.broken_module as broken_module2 import openerp.addons.broken_module.broken_model as broken_model2 +import odoo.addons.iap.models.iap.jsonrpc +from odoo.addons.iap.models.iap import jsonrpc +from odoo.addons.iap.models import iap + other_field = fields.Char() @@ -537,6 +573,133 @@ def func(self, a): length = len(a) return length + def requests_test(self): + # requests without timeout + requests.delete('http://localhost') + requests.get('http://localhost') + requests.head('http://localhost') + requests.options('http://localhost') + requests.patch('http://localhost') + requests.post('http://localhost') + requests.put('http://localhost') + requests.request('call', 'http://localhost') + + delete_r('http://localhost') + get_r('http://localhost') + head_r('http://localhost') + options_r('http://localhost') + patch_r('http://localhost') + post_r('http://localhost') + put_r('http://localhost') + request_r('call', 'http://localhost') + + delete('http://localhost') + get('http://localhost') + head('http://localhost') + options('http://localhost') + patch('http://localhost') + post('http://localhost') + put('http://localhost') + request('call', 'http://localhost') + + # requests valid cases + requests.delete('http://localhost', timeout=10) + requests.get('http://localhost', timeout=10) + requests.head('http://localhost', timeout=10) + requests.options('http://localhost', timeout=10) + requests.patch('http://localhost', timeout=10) + requests.post('http://localhost', timeout=10) + requests.put('http://localhost', timeout=10) + requests.request('call', 'http://localhost', timeout=10) + + delete_r('http://localhost', timeout=10) + get_r('http://localhost', timeout=10) + head_r('http://localhost', timeout=10) + options_r('http://localhost', timeout=10) + patch_r('http://localhost', timeout=10) + post_r('http://localhost', timeout=10) + put_r('http://localhost', timeout=10) + request_r('call', 'http://localhost', timeout=10) + + delete('http://localhost', timeout=10) + get('http://localhost', timeout=10) + head('http://localhost', timeout=10) + options('http://localhost', timeout=10) + patch('http://localhost', timeout=10) + post('http://localhost', timeout=10) + put('http://localhost', timeout=10) + request('call', 'http://localhost', timeout=10) + + # urllib without timeout + urllib.request.urlopen('http://localhost') + urlopen('http://localhost') + urlopen_r('http://localhost') + + # urllib valid cases + urllib.request.urlopen('http://localhost', timeout=10) + urlopen('http://localhost', timeout=10) + urlopen_r('http://localhost', timeout=10) + + # suds without timeout + suds.client.Client('http://localhost') + Client('http://localhost') + Client_r('http://localhost') + + # suds valid cases + suds.client.Client('http://localhost', timeout=10) + Client('http://localhost', timeout=10) + Client_r('http://localhost', timeout=10) + + # http.client without timeout + http.client.HTTPConnection('http://localhost') + http.client.HTTPSConnection('http://localhost') + HTTPConnection('http://localhost') + HTTPSConnection('http://localhost') + HTTPConnection_r('http://localhost') + HTTPSConnection_r('http://localhost') + + # http.client valid cases + http.client.HTTPConnection('http://localhost', timeout=10) + http.client.HTTPSConnection('http://localhost', timeout=10) + HTTPConnection('http://localhost', timeout=10) + HTTPSConnection('http://localhost', timeout=10) + HTTPConnection_r('http://localhost', timeout=10) + HTTPSConnection_r('http://localhost', timeout=10) + + # smtplib without timeout + smtplib.SMTP('http://localhost') + smtplib_r.SMTP('http://localhost') + SMTP('http://localhost') + SMTP_r('http://localhost') + + # smtplib valid cases + smtplib.SMTP('http://localhost', timeout=10) + smtplib_r.SMTP('http://localhost', timeout=10) + SMTP('http://localhost', timeout=10) + SMTP_r('http://localhost', timeout=10) + + # Serial without timeout + serial.Serial('/dev/ttyS1') + serial_r.Serial('/dev/ttyS1') + Serial('/dev/ttyS1') + Serial_r('/dev/ttyS1') + + # serial valid cases + serial.Serial('/dev/ttyS1', timeout=10) + serial_r.Serial('/dev/ttyS1', timeout=10) + Serial('/dev/ttyS1', timeout=10) + Serial_r('/dev/ttyS1', timeout=10) + + # odoo.addons.iap without timeout + odoo.addons.iap.models.iap.jsonrpc('http://localhost') + jsonrpc('http://localhost') + iap.jsonrpc('http://localhost') + + # odoo.addons.iap valid cases + odoo.addons.iap.models.iap.jsonrpc('http://localhost', timeout=10) + jsonrpc('http://localhost', timeout=10) + iap.jsonrpc('http://localhost', timeout=10) + class NoOdoo(object): length = 0