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

[REF] base: don't add id to assets bundle url

Browse Source 
The main motivation is to be able to generate assets bundle outside
the t-call-assets call.
The need of an id in the url makes it mandatory to have an attachment
when adding the url in the page. Without this restriction, we can guess
the url without generating the assets.

This can also have other useful side effect:
There are corner case when a worked could have an invalid url in
cache because, if the transaction is rollbacked or if another request
generates the same attachment at the same time. This should be
partially solved by removing the id: The url remains valid even if the
attachment does not exist.

Note that the extra part of the url was made explicit, always there and
taking one / to remove complexity and ambiguity.

Note that an additional query appeared in .test_50_perf_sql_web_assets
because of the search, this but two of them were in _find_record. One of
them was an `exist`, not making much sense since we are not getting the
id from the attachment url anymore but from a search, and the other one
was prefetch of the "public field" since the call to _find_record does
not go in other cases (xmlid, website published, access token, ....). A
attachment of a asset is always public, and this part of the security
was moved to the search domain. The final result is one less query:
- one query to search
- one query to read the fields (_get_stream_from) (the prefetch could
actually be set to avoid prefetching everything)

Part-of: odoo/odoo#131353
This commit is contained in:
Xavier-Do
2023-10-27 11:34:52 +00:00
parent 781dcdf396
commit d988030134
7 changed files with 74 additions and 83 deletions
Download Patch File Download Diff File Expand all files Collapse all files
addons/test_website/tests/test_qweb.py
+2 -2
View File
@@ -43,8 +43,8 @@ class TestQweb(TransactionCaseWithUserDemo):
html = html.strip()
html = re.sub(r'\?unique=[^"]+', '', html).encode('utf8')
css_attachement = demo_env['ir.attachment'].search([('url', '=like', f'/web/assets/%-{asset_version_css}/{website.id}/test_website.test_bundle.%')])
js_attachement = demo_env['ir.attachment'].search([('url', '=like', f'/web/assets/%-{asset_version_js}/{website.id}/test_website.test_bundle.%')])
css_attachement = demo_env['ir.attachment'].search([('url', '=like', f'/web/assets/{asset_version_css}/w-{website.id}_ltr/test_website.test_bundle.%')])
js_attachement = demo_env['ir.attachment'].search([('url', '=like', f'/web/assets/{asset_version_js}/w-{website.id}_-/test_website.test_bundle.%')])
self.assertEqual(len(css_attachement), 1)
self.assertEqual(len(js_attachement), 1)
addons/web/controllers/binary.py
+16 -22
View File
@@ -83,29 +83,23 @@ class Binary(http.Controller):
res.headers['Content-Security-Policy'] = "default-src 'none'"
return res
@http.route(['/web/assets/debug/<string:filename>',
'/web/assets/debug/<path:extra>/<string:filename>',
'/web/assets/<int:id>/<string:filename>',
'/web/assets/<int:id>-<string:unique>/<string:filename>',
'/web/assets/<int:id>-<string:unique>/<path:extra>/<string:filename>'], type='http', auth="public")
# pylint: disable=redefined-builtin,invalid-name
def content_assets(self, id=None, filename=None, unique=False, extra=None, nocache=False):
if not id:
domain = [('url', '!=', False)]
if extra:
domain += [('url', '=like', f'/web/assets/%/{extra}/{filename}')]
else:
domain += [
('url', '=like', f'/web/assets/%/{filename}'),
('url', 'not like', f'/web/assets/%/%/{filename}')
]
attachment = request.env['ir.attachment'].sudo().search(domain, limit=1)
if not attachment:
raise request.not_found()
id = attachment.id
@http.route(['/web/assets/debug/<path:extra>/<string:filename>',
'/web/assets/<path:extra>/<string:filename>',
'/web/assets/<string:unique>/<path:extra>/<string:filename>'], type='http', auth="public")
def content_assets(self, filename=None, unique='%', extra='-', nocache=False):
url = f'/web/assets/{unique}/{extra}/{filename}'
domain = [
('public', '=', True),
('url', '!=', False),
('url', '=like', url),
('res_model', '=', 'ir.ui.view'),
('res_id', '=', 0),
]
attachment = request.env['ir.attachment'].sudo().search(domain, limit=1)
if not attachment:
raise request.not_found()
with replace_exceptions(UserError, by=request.not_found()):
record = request.env['ir.binary']._find_record(res_id=int(id))
stream = request.env['ir.binary']._get_stream_from(record, 'raw', filename)
stream = request.env['ir.binary']._get_stream_from(attachment, 'raw', filename)
send_file_kwargs = {'as_attachment': False}
if unique:
addons/web/tests/test_assets.py
+1
View File
@@ -98,6 +98,7 @@ class TestAssetsGenerateTime(TestAssetsGenerateTimeCommon):
class TestLoad(HttpCase):
def test_assets_already_exists(self):
self.authenticate('admin', 'admin')
# TODO xdo adapt this test. url open won't generate attachment anymore even if not pregenerated
_save_attachment = odoo.addons.base.models.assetsbundle.AssetsBundle.save_attachment
def save_attachment(bundle, extension, content):
addons/website/models/ir_asset.py
+1 -1
View File
@@ -19,7 +19,7 @@ class IrAsset(models.Model):
extra = super()._get_asset_extra(extra, **params)
if extra == '%':
return extra
website_id_path = website_id and ('%s/' % website_id) or ''
website_id_path = website_id and ('website-%s+' % website_id) or ''
return website_id_path + extra
def _get_related_assets(self, domain, website_id=None, **params):
addons/website/tests/test_performance.py
+6 -10
View File
@@ -61,7 +61,6 @@ class UtilPerf(HttpCase):
sql_into_log_before = copy.deepcopy(self.cr.sql_into_log)
self.url_open(url)
sql_count = self.cr.sql_log_count - sql_count_before - EXTRA_REQUEST
if table_count:
sql_from_tables = {'base_registry_signaling': 1} # see EXTRA_REQUEST
@@ -283,13 +282,10 @@ class TestWebsitePerformancePost(UtilPerf):
assets_url = self.env['ir.attachment'].search([('url', '=like', '/web/assets/%/web.assets_frontend_lazy%.js')], limit=1).url
select_tables_perf = {
'base_registry_signaling': 1,
'ir_attachment': 3,
# All 3 coming from the /web/assets and ir.binary stack
# 1. `_find_record()` performs an access right check through
# `exists()` which perform a request on the ir.attachment.
# 2. `validate_access` reads `public` field of ir.attachment with
# prefetch=False (so only that field)
# 3. `_record_to_stream` reads the other attachment fields
'ir_attachment': 2,
# All 2 coming from the /web/assets and ir.binary stack
# 1. `search() the attachment`
# 2. `_record_to_stream` reads the other attachment fields
}
self._check_url_hot_query(assets_url, 4, select_tables_perf)
self.assertEqual(self._get_url_hot_query(assets_url, cache=False), 4)
self._check_url_hot_query(assets_url, 3, select_tables_perf)
self.assertEqual(self._get_url_hot_query(assets_url, cache=False), 3)
odoo/addons/base/models/assetsbundle.py
+31 -34
View File
@@ -118,7 +118,7 @@ class AssetsBundle(object):
css_attachments = self.css(is_minified=not self.is_debug_assets) or []
for attachment in css_attachments:
if self.is_debug_assets:
href = self.get_debug_asset_url(extra='rtl/' if self.rtl else '',
href = self.get_debug_asset_url(extra='rtl' if self.rtl else 'ltr',
name=css_attachments.name,
extension='')
else:
@@ -166,13 +166,14 @@ class AssetsBundle(object):
self._checksum_cache[asset_type] = hashlib.sha512(unique_descriptor.encode()).hexdigest()[:64]
return self._checksum_cache[asset_type]
def get_asset_url(self, attachment_id='%', unique='%', extra='', name='%', sep=".", extension='%'):
def get_asset_url(self, unique='%', extra='-', name='%', sep=".", extension='%'):
extra = self.env['ir.asset']._get_asset_extra(extra, **self.assets_params)
return f"/web/assets/{attachment_id}-{unique}/{extra}{name}{sep}{extension}"
return f"/web/assets/{unique}/{extra}/{name}{sep}{extension}"
def get_debug_asset_url(self, extra='', name='%', extension='%'):
def get_debug_asset_url(self, extra='-', name='%', extension='%'):
extra = self.env['ir.asset']._get_asset_extra(extra, **self.assets_params)
return f"/web/assets/debug/{extra}{name}{extension}"
return f"/web/assets/debug/{extra}/{name}{extension}"
def _unlink_attachments(self, attachments):
""" Unlinks attachments without actually calling unlink, so that the ORM cache is not cleared.
@@ -188,7 +189,7 @@ class AssetsBundle(object):
for fpath in to_delete:
attachments._file_delete(fpath)
def clean_attachments(self, extension):
def clean_attachments(self, extension, keep_url):
""" Takes care of deleting any outdated ir.attachment records associated to a bundle before
saving a fresh one.
@@ -200,15 +201,14 @@ class AssetsBundle(object):
"""
ira = self.env['ir.attachment']
is_css = extension in ['css', 'min.css', 'css.map']
url = self.get_asset_url(
extra='%s' % ('rtl/' if is_css and self.rtl else ''),
to_clean_pattern = self.get_asset_url(
extra='rtl' if is_css and self.rtl else 'ltr' if is_css else '-',
name=self.name,
extension=extension,
)
domain = [
('url', '=like', url),
'!', ('url', '=like', self.get_asset_url(unique=self.get_version('css' if is_css else 'js'), sep='%'))
('url', '=like', to_clean_pattern),
('url', '!=', keep_url)
]
attachments = ira.sudo().search(domain)
# avoid to invalidate cache if it's already empty (mainly useful for test)
@@ -228,14 +228,14 @@ class AssetsBundle(object):
by file name and only return the one with the max id for each group.
:param extension: file extension (js, min.js, css)
:param ignore_version: if ignore_version, the url contains a version => web/assets/%-%/name.extension
:param ignore_version: if ignore_version, the url contains a version => web/assets/%/name.extension
(the second '%' corresponds to the version),
else: the url contains a version equal to that of the self.get_version(type)
=> web/assets/%-self.get_version(type)/name.extension.
=> web/assets/self.get_version(type)/name.extension.
"""
is_css = extension in ['css', 'min.css', 'css.map']
unique = "%" if ignore_version else self.get_version('css' if is_css else 'js')
extra = '%s' % ('rtl/' if is_css and self.rtl else '')
extra = 'rtl' if is_css and self.rtl else 'ltr' if is_css else '-'
url_pattern = self.get_asset_url(
unique=unique,
extra=extra, # not sure about css.map
@@ -269,6 +269,12 @@ class AssetsBundle(object):
if similar_attachment_ids:
similar = self.env['ir.attachment'].sudo().browse(similar_attachment_ids)
_logger.info('Found a similar attachment for %s, copying from %s', url_pattern, similar.url)
url = self.get_asset_url(
unique=unique,
extra=extra,
name=self.name,
extension=extension,
)
values = {
'name': similar.name,
'mimetype': similar.mimetype,
@@ -277,18 +283,10 @@ class AssetsBundle(object):
'type': 'binary',
'public': True,
'raw': similar.raw,
'url': url,
}
self.add_post_rollback()
attachment = self.env['ir.attachment'].with_user(SUPERUSER_ID).create(values)
url = self.get_asset_url(
attachment_id=attachment.id,
unique=unique,
extra=extra,
name=self.name,
extension=extension,
)
attachment.url = url
attachment_id = attachment.id
if self.env.context.get('commit_assetsbundle') is True:
self.env.cr.commit()
@@ -327,6 +325,12 @@ class AssetsBundle(object):
'application/json' if extension in ['js.map', 'css.map'] else
'application/javascript'
)
url = self.get_asset_url(
unique=self.get_version('css' if is_css else 'js'),
extra='rtl' if is_css and self.rtl else 'ltr' if is_css else '-',
name=self.name,
extension=extension,
)
values = {
'name': fname,
'mimetype': mimetype,
@@ -335,22 +339,15 @@ class AssetsBundle(object):
'type': 'binary',
'public': True,
'raw': content.encode('utf8'),
'url': url,
}
self.add_post_rollback()
attachment = ira.with_user(SUPERUSER_ID).create(values)
url = self.get_asset_url(
attachment_id=attachment.id,
unique=self.get_version('css' if is_css else 'js'),
extra='%s' % ('rtl/' if extension in ['css', 'min.css'] and self.rtl else ''),
name=self.name,
extension=extension,
)
attachment.url = url
if self.env.context.get('commit_assetsbundle') is True:
self.env.cr.commit()
self.clean_attachments(extension)
self.clean_attachments(extension, url)
# For end-user assets (common and backend), send a message on the bus
# to invite the user to refresh their browser
@@ -602,7 +599,7 @@ class AssetsBundle(object):
sourcemap_attachment = self.get_attachments('css.map') \
or self.save_attachment('css.map', '')
debug_asset_url = self.get_debug_asset_url(name=self.name,
extra='rtl/' if self.rtl else '')
extra='rtl' if self.rtl else 'ltr')
generator = SourceMapGenerator(
source_root="/".join(
[".." for i in range(0, len(debug_asset_url.split("/")) - 2)]
@@ -1030,7 +1027,7 @@ class PreprocessedCSS(StylesheetAsset):
def __init__(self, *args, **kw):
super().__init__(*args, **kw)
self.html_url_args = tuple(self.url.rsplit('/', 1))
self.html_url_format = '%%s/%s%s/%%s.css' % ('rtl/' if self.rtl else '', self.bundle.name)
self.html_url_format = '%%s/%s%s/%%s.css' % ('rtl' if self.rtl else 'ltr', self.bundle.name)
def get_command(self):
raise NotImplementedError
odoo/addons/test_assetsbundle/tests/test_assetsbundle.py
+17 -14
View File
@@ -27,6 +27,7 @@ from odoo.tools.misc import file_path
GETMTINE = os.path.getmtime
# ruff: noqa: S320
class TestAddonPaths(TransactionCase):
def test_operations(self):
@@ -72,7 +73,7 @@ class TestAddonPaths(TransactionCase):
# insert with a duplicate of 'd' before 'd'
asset_paths.insert([
('/home/user/odoo/addons/web/b', '/web/b', 1),
('/home/user/odoo/addons/web/d', '/web/d', 1)
('/home/user/odoo/addons/web/d', '/web/d', 1),
], 'bundle4', 1)
self.assertEqual(asset_paths.list, [
@@ -88,7 +89,7 @@ class TestAddonPaths(TransactionCase):
asset_paths.remove([
('/home/user/odoo/addons/web/c', '/web/c', 1),
('/home/user/odoo/addons/web/d', '/web/d', 1),
('/home/user/odoo/addons/web/g', '/web/g', 1)
('/home/user/odoo/addons/web/g', '/web/g', 1),
], 'bundle5')
self.assertEqual(asset_paths.list, [
('/home/user/odoo/addons/web/a', '/web/a', 'bundle1', 1),
@@ -163,8 +164,10 @@ class TestJavascriptAssetsBundle(FileTouchable):
""" Returns all ir.attachments associated to a bundle, regardless of the verion.
"""
bundle = self.jsbundle_name if extension in ['js', 'min.js'] else self.cssbundle_name
rtl = 'rtl/' if rtl and extension in ['css', 'min.css'] else ''
url = f'/web/assets/%-%/{rtl}{bundle}.{extension}'
extra = '-'
if extension in ['css', 'min.css']:
extra = 'rtl' if rtl else 'ltr'
url = f'/web/assets/%/{extra}/{bundle}.{extension}'
domain = [('url', '=like', url)]
return self.env['ir.attachment'].search(domain)
@@ -389,7 +392,7 @@ class TestJavascriptAssetsBundle(FileTouchable):
debug_bundle = self._get_asset(self.cssbundle_name, debug_assets=True)
content = debug_bundle.get_links()
# there should be a minified file
self.assertEqual(content[0][0], '/web/assets/debug/test_assetsbundle.bundle2.css')
self.assertEqual(content[0][0], '/web/assets/debug/ltr/test_assetsbundle.bundle2.css')
# there should be one css asset created in debug mode
self.assertEqual(len(self._any_ira_for_bundle('css')), 1,
@@ -485,7 +488,7 @@ class TestJavascriptAssetsBundle(FileTouchable):
# Check two bundles are available, one for ltr and one for rtl
css_bundles = self.env['ir.attachment'].search([
('url', '=like', '/web/assets/%-%/{0}%.{1}'.format(self.cssbundle_name, 'min.css'))
('url', '=like', f'/web/assets/%/{self.cssbundle_name}%.min.css'),
])
self.assertEqual(len(css_bundles), 2)
@@ -529,7 +532,7 @@ class TestJavascriptAssetsBundle(FileTouchable):
# check if the previous attachment is correctly cleaned
css_bundles = self.env['ir.attachment'].search([
('url', '=like', '/web/assets/%-%/{0}%.{1}'.format(self.cssbundle_name, 'min.css'))
('url', '=like', f'/web/assets/%/{self.cssbundle_name}%.min.css'),
])
self.assertEqual(len(css_bundles), 2)
@@ -549,7 +552,7 @@ class TestJavascriptAssetsBundle(FileTouchable):
rtl_version0 = rtl_bundle0.get_version('css')
css_bundles = self.env['ir.attachment'].search([
('url', '=like', '/web/assets/%-%/{0}%.{1}'.format(self.cssbundle_name, 'min.css'))
('url', '=like', f'/web/assets/%/{self.cssbundle_name}%.min.css'),
])
self.assertEqual(len(css_bundles), 2)
@@ -582,7 +585,7 @@ class TestJavascriptAssetsBundle(FileTouchable):
# check if the previous attachment are correctly cleaned
css_bundles = self.env['ir.attachment'].search([
('url', '=like', '/web/assets/%-%/{0}%.{1}'.format(self.cssbundle_name, 'min.css'))
('url', '=like', f'/web/assets/%/{self.cssbundle_name}%.min.css'),
])
self.assertEqual(len(css_bundles), 2)
@@ -593,19 +596,19 @@ class TestJavascriptAssetsBundle(FileTouchable):
content = debug_bundle.get_links()
# there should be an css assets bundle in /debug/rtl if user's lang direction is rtl and debug=assets
self.assertEqual('/web/assets/debug/rtl/{0}.css'.format(self.cssbundle_name), content[0][0],
self.assertEqual(f'/web/assets/debug/rtl/{self.cssbundle_name}.css', content[0][0],
"there should be an css assets bundle in /debug/rtl if user's lang direction is rtl and debug=assets")
# there should be an css assets bundle created in /rtl if user's lang direction is rtl and debug=assets
css_bundle = self.env['ir.attachment'].search([
('url', '=like', '/web/assets/%-%/rtl/{0}.css'.format(self.cssbundle_name))
('url', '=like', f'/web/assets/%/rtl/{self.cssbundle_name}.css'),
])
self.assertEqual(len(css_bundle), 1,
"there should be an css assets bundle created in /rtl if user's lang direction is rtl and debug=assets")
def test_20_external_lib_assets(self):
html = self.env['ir.ui.view']._render_template('test_assetsbundle.template2')
attachments = self.env['ir.attachment'].search([('url', '=like', '/web/assets/%-%/test_assetsbundle.bundle4.%')])
attachments = self.env['ir.attachment'].search([('url', '=like', '/web/assets/%/test_assetsbundle.bundle4.%')])
self.assertEqual(len(attachments), 2)
format_data = {
@@ -633,8 +636,8 @@ class TestJavascriptAssetsBundle(FileTouchable):
self.assertEqual(len(attachments), 1)
format_data = {
"css": '/web/assets/debug/test_assetsbundle.bundle4.css',
"js": '/web/assets/debug/test_assetsbundle.bundle4.js',
"css": '/web/assets/debug/ltr/test_assetsbundle.bundle4.css',
"js": '/web/assets/debug/-/test_assetsbundle.bundle4.js',
}
self.assertEqual(str(html.strip()), ("""<!DOCTYPE html>
<html>