From 1a1ce16265dff9b7f0d055b5ad6fcfd5af723d1e Mon Sep 17 00:00:00 2001 From: Victor Feyens Date: Mon, 19 Dec 2022 14:59:57 +0000 Subject: [PATCH] [IMP] core,*: check routes decorators When overriding an existing controller route, developers can easily c/p the route definition and call super() in the overridden method when the route attributes are automatically deducted by odoo from the parent route. Removing those redefined attributes simplifies the routes definition, clearly highlighting what's changed by the override. Also reduces unexpected behavior when modifying the base route without noticing/considering the redefined attributes in a overridden route, which overrides the changes made to the base route when the sub-module is installed. This commit adds a test to catch routes attributes redefinition, and clean existing routes. closes odoo/odoo#108512 Related: odoo/enterprise#35176 Signed-off-by: Victor Feyens (vfe) --- addons/auth_signup/controllers/main.py | 2 +- addons/http_routing/controllers/main.py | 2 +- addons/hw_posbox_homepage/controllers/main.py | 2 +- addons/sale_timesheet/controllers/portal.py | 6 +-- addons/web/controllers/export.py | 4 +- addons/website/controllers/main.py | 2 +- .../controllers/main.py | 2 +- .../controllers/community.py | 3 +- .../controllers/community.py | 5 +- .../controllers/community.py | 3 +- addons/website_livechat/controllers/main.py | 2 +- .../website_sale_digital/controllers/main.py | 4 +- .../website_sale_loyalty/controllers/main.py | 2 +- addons/website_slides/controllers/mail.py | 2 +- .../controllers/slides.py | 2 +- odoo/addons/test_lint/tests/__init__.py | 1 + odoo/addons/test_lint/tests/test_routes.py | 48 +++++++++++++++++++ odoo/http.py | 26 +++++++--- 18 files changed, 87 insertions(+), 31 deletions(-) create mode 100644 odoo/addons/test_lint/tests/test_routes.py diff --git a/addons/auth_signup/controllers/main.py b/addons/auth_signup/controllers/main.py index 0348edb4511..41210d1d6d7 100644 --- a/addons/auth_signup/controllers/main.py +++ b/addons/auth_signup/controllers/main.py @@ -162,7 +162,7 @@ class AuthSignupHome(Home): raise SignupError(_('Authentication Failed.')) class AuthBaseSetup(BaseSetup): - @http.route('/base_setup/data', type='json', auth='user') + @http.route() def base_setup_data(self, **kwargs): res = super().base_setup_data(**kwargs) res.update({'resend_invitation': True}) diff --git a/addons/http_routing/controllers/main.py b/addons/http_routing/controllers/main.py index 351d031f48d..4776731c8f8 100644 --- a/addons/http_routing/controllers/main.py +++ b/addons/http_routing/controllers/main.py @@ -20,6 +20,6 @@ class Routing(Home): class SessionWebsite(Session): - @http.route('/web/session/logout', type='http', auth="none", website=True, multilang=False, sitemap=False) + @http.route('/web/session/logout', website=True, multilang=False, sitemap=False) def logout(self, redirect='/web'): return super().logout(redirect=redirect) diff --git a/addons/hw_posbox_homepage/controllers/main.py b/addons/hw_posbox_homepage/controllers/main.py index 2a3ea05dec1..718718cb9f7 100644 --- a/addons/hw_posbox_homepage/controllers/main.py +++ b/addons/hw_posbox_homepage/controllers/main.py @@ -97,7 +97,7 @@ class IoTboxHomepage(Home): 'system': platform.system(), } - @http.route('/', type='http', auth='none') + @http.route() def index(self): wifi = Path.home() / 'wifi_network.txt' remote_server = Path.home() / 'odoo-remote-server.conf' diff --git a/addons/sale_timesheet/controllers/portal.py b/addons/sale_timesheet/controllers/portal.py index 6f5f97d1a1b..e5318ec4362 100644 --- a/addons/sale_timesheet/controllers/portal.py +++ b/addons/sale_timesheet/controllers/portal.py @@ -133,6 +133,6 @@ class SaleTimesheetCustomerPortal(TimesheetCustomerPortal): }) return values - @http.route(['/my/timesheets', '/my/timesheets/page/'], type='http', auth="user", website=True) - def portal_my_timesheets(self, page=1, sortby=None, filterby=None, search=None, search_in='all', groupby='sol', **kw): - return super().portal_my_timesheets(page, sortby, filterby, search, search_in, groupby, **kw) + @http.route() + def portal_my_timesheets(self, *args, groupby='sol', **kw): + return super().portal_my_timesheets(*args, groupby=groupby, **kw) diff --git a/addons/web/controllers/export.py b/addons/web/controllers/export.py index e2dc55fe214..5fdfbc25353 100644 --- a/addons/web/controllers/export.py +++ b/addons/web/controllers/export.py @@ -510,7 +510,7 @@ class ExportFormat(object): class CSVExport(ExportFormat, http.Controller): @http.route('/web/export/csv', type='http', auth="user") - def index(self, data): + def web_export_csv(self, data): try: return self.base(data) except Exception as exc: @@ -554,7 +554,7 @@ class CSVExport(ExportFormat, http.Controller): class ExcelExport(ExportFormat, http.Controller): @http.route('/web/export/xlsx', type='http', auth="user") - def index(self, data): + def web_export_xlsx(self, data): try: return self.base(data) except Exception as exc: diff --git a/addons/website/controllers/main.py b/addons/website/controllers/main.py index 1552ef7885d..87ab0819082 100644 --- a/addons/website/controllers/main.py +++ b/addons/website/controllers/main.py @@ -74,7 +74,7 @@ class QueryURL(object): class Website(Home): - @http.route('/', type='http', auth="public", website=True, sitemap=True) + @http.route('/', auth="public", website=True, sitemap=True) def index(self, **kw): """ The goal of this controller is to make sure we don't serve a 404 as the website homepage. As this is the website entry point, serving a 404 diff --git a/addons/website_crm_partner_assign/controllers/main.py b/addons/website_crm_partner_assign/controllers/main.py index 0b1d33d6cfd..59dab32111e 100644 --- a/addons/website_crm_partner_assign/controllers/main.py +++ b/addons/website_crm_partner_assign/controllers/main.py @@ -308,7 +308,7 @@ class WebsiteCrmPartnerAssign(WebsitePartnerPage): # Do not use semantic controller due to sudo() - @http.route(['/partners/'], type='http', auth="public", website=True) + @http.route() def partners_detail(self, partner_id, **post): _, partner_id = unslug(partner_id) current_grade, current_country = None, None diff --git a/addons/website_event_meet/controllers/community.py b/addons/website_event_meet/controllers/community.py index b194f1dda3b..e6e1ba629b0 100644 --- a/addons/website_event_meet/controllers/community.py +++ b/addons/website_event_meet/controllers/community.py @@ -28,8 +28,7 @@ class WebsiteEventMeetController(EventCommunityController): # MAIN PAGE # ------------------------------------------------------------ - @http.route(["/event//community"], type="http", - auth="public", website=True, sitemap=True) + @http.route() def community(self, event, page=1, lang=None, **kwargs): """Display the meeting rooms of the event on the frontend side. diff --git a/addons/website_event_meet_quiz/controllers/community.py b/addons/website_event_meet_quiz/controllers/community.py index 7cd974a19f3..91b93aec273 100644 --- a/addons/website_event_meet_quiz/controllers/community.py +++ b/addons/website_event_meet_quiz/controllers/community.py @@ -3,14 +3,13 @@ from werkzeug.exceptions import Forbidden -from odoo import http from odoo.addons.website_event.controllers.community import EventCommunityController -from odoo.http import request +from odoo.http import request, route class WebsiteEventTrackQuizMeetController(EventCommunityController): - @http.route(['/event//community'], type='http', auth="public", website=True, sitemap=False) + @route() def community(self, event, page=1, lang=None, **kwargs): # website_event_track_quiz values = self._get_community_leaderboard_render_values(event, kwargs.get('search'), page) diff --git a/addons/website_event_track_quiz/controllers/community.py b/addons/website_event_track_quiz/controllers/community.py index 7748a2dc1db..6957d93cd7c 100644 --- a/addons/website_event_track_quiz/controllers/community.py +++ b/addons/website_event_track_quiz/controllers/community.py @@ -27,8 +27,7 @@ class WebsiteEventTrackQuizCommunityController(EventCommunityController): values = self._get_community_leaderboard_render_values(event, None, None) return request.render('website_event_track_quiz.event_leaderboard', values) - @http.route('/event//community', - type='http', auth="public", website=True, sitemap=False) + @http.route() def community(self, event, **kwargs): values = self._get_community_leaderboard_render_values(event, None, None) return request.render('website_event_track_quiz.event_leaderboard', values) diff --git a/addons/website_livechat/controllers/main.py b/addons/website_livechat/controllers/main.py index 8678526e6b9..0f24a22bfe8 100644 --- a/addons/website_livechat/controllers/main.py +++ b/addons/website_livechat/controllers/main.py @@ -59,7 +59,7 @@ class WebsiteLivechat(LivechatController): } return request.render("website_livechat.channel_page", values) - @http.route('/im_livechat/get_session', type="json", auth='public', cors="*") + @http.route() def get_session(self, channel_id, anonymous_name, previous_operator_id=None, chatbot_script_id=None, persisted=True, **kwargs): """ Override to use visitor name instead of 'Visitor' whenever a visitor start a livechat session. """ visitor_sudo = request.env['website.visitor']._get_visitor_from_request() diff --git a/addons/website_sale_digital/controllers/main.py b/addons/website_sale_digital/controllers/main.py index 732a15dbced..d1a4b777085 100644 --- a/addons/website_sale_digital/controllers/main.py +++ b/addons/website_sale_digital/controllers/main.py @@ -14,9 +14,7 @@ from odoo.addons.sale.controllers.portal import CustomerPortal class WebsiteSaleDigital(CustomerPortal): orders_page = '/my/orders' - @http.route([ - '/my/orders/', - ], type='http', auth='public', website=True) + @http.route() def portal_order_page(self, order_id=None, **post): response = super(WebsiteSaleDigital, self).portal_order_page(order_id=order_id, **post) if not 'sale_order' in response.qcontext: diff --git a/addons/website_sale_loyalty/controllers/main.py b/addons/website_sale_loyalty/controllers/main.py index 2ebf38ed683..db725e6ba9b 100644 --- a/addons/website_sale_loyalty/controllers/main.py +++ b/addons/website_sale_loyalty/controllers/main.py @@ -35,7 +35,7 @@ class WebsiteSale(main.WebsiteSale): order._auto_apply_rewards() return res - @http.route(['/shop/cart'], type='http', auth="public", website=True) + @http.route() def cart(self, **post): order = request.website.sale_get_order() if order: diff --git a/addons/website_slides/controllers/mail.py b/addons/website_slides/controllers/mail.py index 7c52fa46b35..54b6ab7dd17 100644 --- a/addons/website_slides/controllers/mail.py +++ b/addons/website_slides/controllers/mail.py @@ -20,7 +20,7 @@ class SlidesPortalChatter(PortalChatter): return True return super()._portal_post_has_content(res_model, res_id, message, attachment_ids=attachment_ids, **kw) - @http.route(['/mail/chatter_post'], type='json', methods=['POST'], auth='public', website=True) + @http.route() def portal_chatter_post(self, res_model, res_id, message, **kw): result = super(SlidesPortalChatter, self).portal_chatter_post(res_model, res_id, message, **kw) if result and res_model == 'slide.channel': diff --git a/addons/website_slides_survey/controllers/slides.py b/addons/website_slides_survey/controllers/slides.py index 577afcb795d..503468ffa4a 100644 --- a/addons/website_slides_survey/controllers/slides.py +++ b/addons/website_slides_survey/controllers/slides.py @@ -42,7 +42,7 @@ class WebsiteSlidesSurvey(WebsiteSlides): # Overrides # ------------------------------------------------------------ - @http.route(['/slides/add_slide'], type='json', auth='user', methods=['POST'], website=True) + @http.route() def create_slide(self, *args, **post): create_new_survey = post['slide_category'] == "certification" and post.get('survey') and not post['survey']['id'] linked_survey_id = int(post.get('survey', {}).get('id') or 0) diff --git a/odoo/addons/test_lint/tests/__init__.py b/odoo/addons/test_lint/tests/__init__.py index f7908d8a6e3..334dedcefe4 100644 --- a/odoo/addons/test_lint/tests/__init__.py +++ b/odoo/addons/test_lint/tests/__init__.py @@ -11,3 +11,4 @@ from . import test_onchange_domains from . import test_override_signatures from . import test_pofile from . import test_pylint +from . import test_routes diff --git a/odoo/addons/test_lint/tests/test_routes.py b/odoo/addons/test_lint/tests/test_routes.py new file mode 100644 index 00000000000..c9479adcf23 --- /dev/null +++ b/odoo/addons/test_lint/tests/test_routes.py @@ -0,0 +1,48 @@ +# Part of Odoo. See LICENSE file for full copyright and licensing details. + +import logging +from pprint import pformat +from unittest.mock import patch + +from odoo import http +from odoo.tests import tagged, TransactionCase + +_logger = logging.getLogger(__name__) + + +@tagged('post_install', '-at_install') +class RoutesLinter(TransactionCase): + + def test_routes_definition(self): + """Forbid redefinition of same-value attributes in an inherited route. + + Makes it easier to know what an inherited route really modifies and + investigate unexpected behavior. + """ + _check_and_complete_route_definition = http._check_and_complete_route_definition + + def extended_check(controller_cls, submethod, merged_routing): + if 'type' in merged_routing: + # merged_routing contains non default 'type' value + # => current method is an inherited route. + useless_overrides = { + key: value + for key, value in submethod.original_routing.items() + if key not in ('routes', 'type') + if merged_routing.get(key) == value + } + if useless_overrides: + _logger.warning( + "The endpoint %s is duplicating the existing routing configuration : %s", + f'{controller_cls.__module__}.{controller_cls.__name__}.{submethod.__name__}', + pformat(useless_overrides), + ) + + _check_and_complete_route_definition(controller_cls, submethod, merged_routing) + + installed_modules = set(self.env['ir.module.module'].search([ + ('state', '=', 'installed'), + ]).mapped('name')) + with patch('odoo.http._check_and_complete_route_definition', extended_check): + for _ in http._generate_routing_rules(installed_modules, nodb_only=False): + pass diff --git a/odoo/http.py b/odoo/http.py index ff39250c4d3..386e89e7931 100644 --- a/odoo/http.py +++ b/odoo/http.py @@ -797,13 +797,7 @@ def _generate_routing_rules(modules, nodb_only, converters=None): _logger.warning("The endpoint %s is not decorated by @route(), decorating it myself.", f'{cls.__module__}.{cls.__name__}.{method_name}') submethod = route()(submethod) - # Ensure "type" is defined on each method's own routing, - # also ensure overrides don't change the routing type. - default_type = submethod.original_routing.get('type', 'http') - routing_type = merged_routing.setdefault('type', default_type) - if submethod.original_routing.get('type') not in (None, routing_type): - _logger.warning("The endpoint %s changes the route type, using the original type: %r.", f'{cls.__module__}.{cls.__name__}.{method_name}', routing_type) - submethod.original_routing['type'] = routing_type + _check_and_complete_route_definition(cls, submethod, merged_routing) merged_routing.update(submethod.original_routing) @@ -827,6 +821,24 @@ def _generate_routing_rules(modules, nodb_only, converters=None): yield (url, endpoint) +def _check_and_complete_route_definition(controller_cls, submethod, merged_routing): + """Verify and complete the route definition. + + * Ensure 'type' is defined on each method's own routing. + * also ensure overrides don't change the routing type. + + :param submethod: route method + :param dict merged_routing: accumulated routing values (defaults + submethod ancestor methods) + """ + default_type = submethod.original_routing.get('type', 'http') + routing_type = merged_routing.setdefault('type', default_type) + if submethod.original_routing.get('type') not in (None, routing_type): + _logger.warning( + "The endpoint %s changes the route type, using the original type: %r.", + f'{controller_cls.__module__}.{controller_cls.__name__}.{submethod.__name__}', + routing_type) + submethod.original_routing['type'] = routing_type + # ========================================================= # Session # =========================================================