From 2391d0994ae564347ac01772811f5cfc22e951ff Mon Sep 17 00:00:00 2001 From: Romain Derie Date: Wed, 12 Apr 2023 15:28:12 +0000 Subject: [PATCH] [FIX] website: prevent assets to be invalidated in multi domain == Issue == A business code error was detected by the internal team on our production. The cache and assets where invalidated !WAY! too often for the past months. It was hard to figure but finally the error was tracked down to be located in the assets retrieval stack of our code when a database is accessed through multiple different domains. In our production use case, whenever one was accessing `odoo.com/web` after someone accessed `accounts.odoo.com/web`, the assets would be invalidated and recomputed, again and again, whenever someone accessed the backend on a domain after someone else did with another domain. Obviously, on our production, this could be occuring multiple time per minute. Technically, this is because the "assets retrieval stack" had a mismatch in multiple endpoint when trying to find if a current website was involved (serving for the frontend). Some business method were using `env.context.get('website_id')` while others were using `env['website'].get_current_website(fallback=False)`. From there, when the code was called without a `website_id` in the context, `get_current_website()` would still return a `website_id` when called from `http://odoo.com` as there is a website having its domain set to it. `get_current_website()` is then finding it and returning it. But it would not when the user is on `http://accounts.odoo.com`. Since we have a custom scss override (done through our website builder, basically an ir.asset linked to a "url type" attachment: `/website/static/src/scss/options/colors/user_color_palette.scss`) for our website to define the website colors which is shadowing the scss file from disk. So, depending of the host/domain, either the real file disk for this URL or the ir.asset linked to our website for this URL would be fetched to generate the bundle hash (which is basically the last modification date of the files/attachments). Obviously, the file on disk and the ir.assets have a different last modification date. The system would then consider the assets as outdated and would regenerate it. You can see it in the logs where the attachment id of the assets URL would get higher and higher everytime you access the DB through another domain. Using `get_current_website(fallback=False)`: - `_get_related_assets()` https://github.com/odoo/odoo/blame/30d3b97b5ece379d9ddcbceda9d12c03dc7f4a48/addons/website/models/ir_asset.py#L14 - `filter_duplicate()` https://github.com/odoo/odoo/blame/30d3b97b5ece379d9ddcbceda9d12c03dc7f4a48/addons/website/models/ir_asset.py#L41 - .. Using `get_current_website()`: - `_get_custom_attachment()` https://github.com/odoo/odoo/blame/30d3b97b5ece379d9ddcbceda9d12c03dc7f4a48/addons/website/models/assets.py#L162 - .. Using `context.get('website_id')`: - `_get_asset_url_values()` https://github.com/odoo/odoo/blame/30d3b97b5ece379d9ddcbceda9d12c03dc7f4a48/addons/website/models/ir_qweb.py#L23 - .. == Fix == A fix could have been to aligned those to use the same way of retrieving the website but it would be too fragile (definitely some other places where the same bug is involved but not yet found). What is done in this commit is something we wanted to do for a long time (see [1]) but was based purely on guess and feeling rather than concrete bug / use case, but now that we found a real use case, we will do it: - It doesn't seems to make sense to consider the request host/domain when we are in the backend - Same for the forced session, those should only impact the frontend calls. But this seems to have too much impact in stable to be changed, as it would require to check every caller to also check for the session if it makes sense. This will be done in master as not really needed to prevent the critical bug fixed here. - When something wants to alter the backend with a website, it should explicitely be passed in the context, which is still considered regardless if it's a backend/frontend call. - If something needs to consider the forced website in session in the backend, it should explicitely check it, not relying on `get_current_website()`. == Step to reproduce == - Start a db with website installed - Enter the website builder in edit mode and change the "Theme Colors"'s first "Color Presets"'s background color (it is white by default). - Set the website domain to `http://127.0.0.1:8069/` - Go to `http://127.0.0.1:8069/web` and login - Go to `http://127.0.0.2:8069/web` and login - Now start refreshing those 2 pages one after each other. Everytime you will refresh the page, it will take a very long time (~5-10 seconds) before loading the page, and monitoring the logs will show something about invalidating the cache and huge query count. == Benchmark == For the explained "multiple domain access" case, the backend /web will now be loaded in less than 10ms and with ~10 SQL Queries when website is installed, while it was taking ~4 seconds and ~200 Sql Queries before the fix. Before the fix: ``` odoo.modules.registry: At least one model cache has been invalidated, signaling through the database GET host1.com/web HTTP/1.1 200 - 222 0.135 3.840 <-- 222 Queries, ~4s odoo.modules.registry: At least one model cache has been invalidated, signaling through the database GET host2.com/web HTTP/1.1 200 - 181 0.101 3.692 <-- 181 Queries, ~4s odoo.modules.registry: At least one model cache has been invalidated, signaling through the database GET host1.com/web HTTP/1.1 200 - 215 0.121 3.704 <-- 215 Queries, ~4s odoo.modules.registry: At least one model cache has been invalidated, signaling through the database GET host2.com/web HTTP/1.1 200 - 181 0.100 3.616 <-- 181 Queries, ~4s ``` After the fix: ``` odoo.modules.registry: At least one model cache has been invalidated, signaling through the database GET host1.com/web HTTP/1.1 200 - 101 0.043 0.353 <-- 101 Queries, ~0.3s GET host2.com/web HTTP/1.1 200 - 11 0.004 0.007 <-- 11 Queries, ~10ms GET host1.com/web HTTP/1.1 200 - 11 0.003 0.005 <-- 11 Queries, ~10ms GET host2.com/web HTTP/1.1 200 - 11 0.003 0.008 <-- 11 Queries, ~10ms ``` [1]: https://github.com/odoo/odoo/pull/94161#discussion_r904780031 (Also other PR/task but couldn't find those.) closes odoo/odoo#120364 X-original-commit: 28dd35eb3c681b630f0b3c109a7d8209f9fa42d8 Signed-off-by: Quentin Smetz (qsm) Signed-off-by: Romain Derie (rde) --- addons/website/models/website.py | 23 +++++++++- addons/website/tests/__init__.py | 1 + addons/website/tests/test_assets.py | 65 +++++++++++++++++++++++++++++ 3 files changed, 87 insertions(+), 2 deletions(-) create mode 100644 addons/website/tests/test_assets.py diff --git a/addons/website/models/website.py b/addons/website/models/website.py index 42b20802b86..8f680b1a73e 100644 --- a/addons/website/models/website.py +++ b/addons/website/models/website.py @@ -930,6 +930,15 @@ class Website(models.Model): @api.model def get_current_website(self, fallback=True): + """ The current website is returned in the following order: + - the website forced in session `force_website_id` + - the website set in context + - (if frontend or fallback) the website matching the request's "domain" + - arbitrary the first website found in the database if `fallback` is set + to `True` + - empty browse record + """ + is_frontend_request = request and getattr(request, 'is_frontend', False) if request and request.session.get('force_website_id'): website_id = self.browse(request.session['force_website_id']).exists() if not website_id: @@ -942,12 +951,22 @@ class Website(models.Model): if website_id: return self.browse(website_id) - if not request and not fallback: + if not is_frontend_request and not fallback: + # It's important than backend requests with no fallback requested + # don't go through return self.browse(False) + # Reaching this point means that: + # - We didn't find a website in the session or in the context. + # - And we are either: + # - in a frontend context + # - in a backend context (or early in the dispatch stack) and a + # fallback website is requested. + # We will now try to find a website matching the request host/domain (if + # there is one on request) or return a random one. + # The format of `httprequest.host` is `domain:port` domain_name = request and request.httprequest.host or '' - website_id = self._get_current_website_id(domain_name, fallback=fallback) return self.browse(website_id) diff --git a/addons/website/tests/__init__.py b/addons/website/tests/__init__.py index 63fccfa6a24..b44bdd38129 100644 --- a/addons/website/tests/__init__.py +++ b/addons/website/tests/__init__.py @@ -1,5 +1,6 @@ # -*- coding: utf-8 -*- # Part of Odoo. See LICENSE file for full copyright and licensing details. +from . import test_assets from . import test_attachment from . import test_auth_signup_uninvited from . import test_automatic_editor diff --git a/addons/website/tests/test_assets.py b/addons/website/tests/test_assets.py new file mode 100644 index 00000000000..5dbacafa878 --- /dev/null +++ b/addons/website/tests/test_assets.py @@ -0,0 +1,65 @@ +# -*- coding: utf-8 -*- +# Part of Odoo. See LICENSE file for full copyright and licensing details. + +import odoo.tests + +from odoo.tools import config + + +@odoo.tests.common.tagged('post_install', '-at_install') +class TestWebsiteAssets(odoo.tests.HttpCase): + + def test_01_multi_domain_assets_generation(self): + Website = self.env['website'] + Attachment = self.env['ir.attachment'] + # Simulate single website DBs: make sure other website do not interfer + # (We can't delete those, constraint will most likely be raised) + Website.search([]).write({'domain': 'inactive.test'}) + # Don't use HOST, hardcode it so it doesn't get changed one day and make + # the test useless + domain_1 = "http://127.0.0.1:%s" % config['http_port'] + domain_2 = "http://localhost:%s" % config['http_port'] + Website.browse(1).domain = domain_1 + + self.authenticate('admin', 'admin') + self.env['web_editor.assets'].with_context(website_id=1).make_scss_customization( + '/website/static/src/scss/options/colors/user_color_palette.scss', + {"o-cc1-bg": "'400'"}, + ) + + def get_last_backend_asset_attach_id(): + return Attachment.search([ + ('name', '=', 'web.assets_backend.min.js'), + ], order="id desc", limit=1).id + + def check_asset(): + self.assertEqual(last_backend_asset_attach_id, get_last_backend_asset_attach_id()) + + last_backend_asset_attach_id = get_last_backend_asset_attach_id() + + # The first call will generate the assets and populate the cache and + # take ~100 SQL Queries (~cold state). + # Any later call to `/web`, regardless of the domain, will take only + # ~10 SQL Queries (hot state). + # Without the calls the `check_asset()` (which would raise early and + # would not call other `url_open()`) and before the fix coming with this + # test, here is the logs: + # "GET /web HTTP/1.1" 200 - 222 0.135 3.840 <-- 222 Queries, ~4s + # "GET /web HTTP/1.1" 200 - 181 0.101 3.692 <-- 181 Queries, ~4s + # "GET /web HTTP/1.1" 200 - 215 0.121 3.704 <-- 215 Queries, ~4s + # "GET /web HTTP/1.1" 200 - 181 0.100 3.616 <-- 181 Queries, ~4s + # After the fix, here is the logs: + # "GET /web HTTP/1.1" 200 - 101 0.043 0.353 <-- 101 Queries, ~0.3s + # "GET /web HTTP/1.1" 200 - 11 0.004 0.007 <-- 11 Queries, ~10ms + # "GET /web HTTP/1.1" 200 - 11 0.003 0.005 <-- 11 Queries, ~10ms + # "GET /web HTTP/1.1" 200 - 11 0.003 0.008 <-- 11 Queries, ~10ms + self.url_open(domain_1 + '/web') + check_asset() + self.url_open(domain_2 + '/web') + check_asset() + self.url_open(domain_1 + '/web') + check_asset() + self.url_open(domain_2 + '/web') + check_asset() + self.url_open(domain_1 + '/web') + check_asset()