pengyuthon/odoo_source
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

[FIX] website: prevent assets to be invalidated in multi domain

Browse Source 
== 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) <qsm@odoo.com>
Signed-off-by: Romain Derie (rde) <rde@odoo.com>
This commit is contained in:
Romain Derie
2023-05-03 13:57:11 +02:00
parent 8577abc4c1
commit 2391d0994a
3 changed files with 87 additions and 2 deletions
Download Patch File Download Diff File Expand all files Collapse all files
addons/website/models/website.py
+21 -2
View File
@@ -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)
addons/website/tests/__init__.py
+1
View File
@@ -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
addons/website/tests/test_assets.py
+65
View File
@@ -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()