From d988030134887f518bf29605f9f0b74c9bf12539 Mon Sep 17 00:00:00 2001 From: Xavier-Do Date: Wed, 8 Feb 2023 18:48:44 +0100 Subject: [PATCH] [REF] base: don't add id to assets bundle url 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 --- addons/test_website/tests/test_qweb.py | 4 +- addons/web/controllers/binary.py | 38 +++++------ addons/web/tests/test_assets.py | 1 + addons/website/models/ir_asset.py | 2 +- addons/website/tests/test_performance.py | 16 ++--- odoo/addons/base/models/assetsbundle.py | 65 +++++++++---------- .../tests/test_assetsbundle.py | 31 +++++---- 7 files changed, 74 insertions(+), 83 deletions(-) diff --git a/addons/test_website/tests/test_qweb.py b/addons/test_website/tests/test_qweb.py index 7e62156d815..31a2463d91f 100644 --- a/addons/test_website/tests/test_qweb.py +++ b/addons/test_website/tests/test_qweb.py @@ -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) diff --git a/addons/web/controllers/binary.py b/addons/web/controllers/binary.py index 0a51a304b74..509e3947906 100644 --- a/addons/web/controllers/binary.py +++ b/addons/web/controllers/binary.py @@ -83,29 +83,23 @@ class Binary(http.Controller): res.headers['Content-Security-Policy'] = "default-src 'none'" return res - @http.route(['/web/assets/debug/', - '/web/assets/debug//', - '/web/assets//', - '/web/assets/-/', - '/web/assets/-//'], 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//', + '/web/assets//', + '/web/assets///'], 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: diff --git a/addons/web/tests/test_assets.py b/addons/web/tests/test_assets.py index a1dd4d37977..250e12aaa15 100644 --- a/addons/web/tests/test_assets.py +++ b/addons/web/tests/test_assets.py @@ -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): diff --git a/addons/website/models/ir_asset.py b/addons/website/models/ir_asset.py index 154b6a4de7a..231b6922680 100644 --- a/addons/website/models/ir_asset.py +++ b/addons/website/models/ir_asset.py @@ -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): diff --git a/addons/website/tests/test_performance.py b/addons/website/tests/test_performance.py index 0c120ded144..bd4ffa1b3e8 100644 --- a/addons/website/tests/test_performance.py +++ b/addons/website/tests/test_performance.py @@ -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) diff --git a/odoo/addons/base/models/assetsbundle.py b/odoo/addons/base/models/assetsbundle.py index 82a626acb6b..77ace9a9a01 100644 --- a/odoo/addons/base/models/assetsbundle.py +++ b/odoo/addons/base/models/assetsbundle.py @@ -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 diff --git a/odoo/addons/test_assetsbundle/tests/test_assetsbundle.py b/odoo/addons/test_assetsbundle/tests/test_assetsbundle.py index a202d1f0c41..41874774b5a 100644 --- a/odoo/addons/test_assetsbundle/tests/test_assetsbundle.py +++ b/odoo/addons/test_assetsbundle/tests/test_assetsbundle.py @@ -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()), ("""