Saturday, June 10, 2023
11 changes · master
Code cleanup and technical improvements
This change reorganizes how Odoo caches website and web interface asset bundles such as JavaScript and CSS. It aims to reduce unnecessary cache duplication, speed up page loading after website changes, and make cache refreshes more targeted to lower the risk of broad performance disruptions.
Original PR description
## Warning If changing the ormcache keys and placement was the main goal of this pr, a `self.env['ir.qweb'].clear_caches()` was removed This is maybe the change that can have the must impact and may…
## Warning
If changing the ormcache keys and placement was the main goal of this pr, a `self.env['ir.qweb'].clear_caches()` was removed
This is maybe the change that can have the must impact and may create unexpected bug. It was replaced by more precise actions. See the corresponding **Remove cache invalidation** section for more info.
## Introduction
The current ormcache for assets bundle are not always _ideal_.
- Some cache key can have multiple values leading to the same result (example, lang defines rtl)
- Some cache key are useless (too many context key related to templates logic)
- Some cache key are not totally enough and works by "chance"
- Part of the computation stored in the ormcache is actually fast to generate (node attributes)
## 1. Improve _get_asset_content cache
### Move cache from _get_asset_content to _get_asset_paths
The `_get_asset_content` cache has many cache key that are related to a post-processing of the `_get_asset_paths` result. (node attributes)
`_get_asset_paths` can be slow because of file system access, but the post processing is actually just filtering files, generating node attributes for external assets, ....
Moving the cache to _get_asset_content will have the benefit to create less duplicates entries in the ormcache as well as less cache miss.
To simplify the `css` and `js` parameters are removed since they _only_ filter the output of get_paths, the heavy part of globing the file will be done before that. Anyway, they are both `True` when called from `_get_asset_content`, and the only other call, in `_get_related_bundle` don't really need to filter them since it is not a critical part regarding performance, and the result will remain the same.
### Better `_get_asset_paths` cache key
The initial orm cache key was using `_get_template_cache_keys`, a little overkill and creating duplicates entries again. The only context key needed is website_id for `_get_related_assets`.
The context key is actually not enough, the website_id depends on more parameters than that:
- request.session.get('force_website_id')
- request.httprequest.host
- existing websites
The idea here is to call `get_current_website` instead of using all parameters that could define the website.
In the same spirit of `_get_template_cache_keys` `_get_assets_params` can be overridden to give extra params that are useful to list assets path. Those params are computed before entering the method `_get_asset_paths`.
The only parameter here is the website_id.
Other parameter like `defer_load`, `lazy_load`, `media` are not useful since the cache was moved but this will actually change even further in next point.
### Result
Before:
```
@tools.ormcache('bundle', 'defer_load', 'lazy_load', 'media', 'tuple(self.env.context.get(k) for k in self._get_template_cache_keys())')
```
After
```
@tools.conditional(
'xml' not in tools.config['dev_mode'],
tools.ormcache('bundle', 'tuple(sorted(assets_params.items()))'),
)
```
The conditional part is actually to make it consistent with the assets_node, and enforce an expected result to discover new files in dev mode (was working before only by cache invalidation side effect) The final cache key is actually : (`bundle_name`, `website_id`)
## 2. Improve _generate_asset_nodes cache
The main reason of the orm cache is the slowness of the validation of the assets. This includes:
- listing files (_get_asset_content, see previous point)
- **computing version**
The cache key was, like the previous point, depending on
- `debug`
The only relevant value for debug is _contains assets_
We don't need to make the difference between debug='', debug='1', debug='test', and 'debug=assets', 'debug=tests,assets', ...
- `defer_load`, `lazy_load`,
Those values are only useful to generate html node, a leight-weight operation that does not really needs to be in cache.
The main idea to remove them from the ormcache key is simply to generate the nodes outside the ormcached values.
- `media`
The idea is the same as `defer_load` and `lazy_load`, but `media` was also used in the generation and it looks useless if we have the media on the node. Since media is not used to generate the attachment url, it doesn't make sence to use it in the generation. Also, no t-call-asset with a media= was found in the master code base.
- `async_load`
This one is similar to `defer_load` and `lazy_load` but it looks like it wasn't used anymore. This was simply removed.
- context.get('lang')
The only information needed is the direction, rtl or ltr. This means en and fr languages, despite sharing the same css assets, will duplicate the ormcache entries.
- `_get_template_cache_keys`
Only the lang and webiste where really relevant in this flow. Other keys are actually useless in this flow.
As explained in the previous point, some information used in the generation where not in the orm cache key
- `self.env.user.lang` if there is no lang in the context
- `request.session.get('force_website_id')`
- `request.httprequest.host`
- ...
The proposed solution is to:
- extract any information needed from the context, request, environment before entering the ormcache, reduce it to the minimal possible set of values needed
```
rtl = self.env['res.lang']._lang_get_direction(self.env.context.get('lang') or self.env.user.lang) == 'rtl'
assets_params = self.env['ir.asset']._get_assets_params() # website_id
debug_assets = debug and 'assets' in debug
```
and remove a leightweight part of the logic
```
def _get_asset_nodes(self, bundle, css=True, js=True, debug=False, defer_load=False, lazy_load=False, media=None):
links = self._get_asset_links(bundle, css=css, js=js, debug=debug)
return self._links_to_nodes(links, defer_load=defer_load, lazy_load=lazy_load, media=media)
```
Where _get_asset_links is the heavy cached part, and _links_to_nodes is the lightweight part generating the nodes based on the `defer_load`, ....
Additional notes:
- data-asset-version and data-asset-bundle are removed from the node since they don't seem to be used anymore since 65d70acdbfb1027f0937b5af7c4f3e9a571a4c6e
- async_load is removed since there is no occurence of this in the code.
- a small hack is still needed to pass javascript content instead of links, this is only to manage css compile error and will hopefully be removed in the future.
- a context key is still in use to generate the bundle, the `commit_assetsbundle` but it has no impact on content and will hopefully be removed in the future.
## Add test for ormcache hit/miss
In this context, hit/miss is about having the same cache key for the same result. This test demonstrates the current state, were entries are create in the ormcache only if the key is really different and will lead to a different result.
## Remove cache invalidation
This cache invalidation is quite aggressive since every time an asset bundle is updated, all workers will clear their cache.
The concerned cache by this clear_cache is `_generate_asset_nodes_cache` trough `_get_asset_nodes`.
The cache is ignored, both in dev=xml and debug=assets.
This clear cache was made conditional in https://github.com/odoo/odoo/commit/553ea82f8135454b90888bf200372744729d7b20 but this does not solve an issue we can have in production.
Lets imagine a clean starting state
- all sources are updated
- all workers are restarted.
The orm caches are all empty, but since the sources changed, all bundles will be recomputed. This means that every bundle updated in database with save_attachement will invalidate the cache of all workers. Rendering a pdf report of any kind using a specific bundle will invalidate all cache. Starting a debug=assets for the first time will invalidate all cache, even if the cache is not used in this case.
But for a regenerated bundle we would expect the ormcache to be:
- empty (did not generate the same bundle yet)
- have the same value (concurrent generation of the same bundle)
Having a different value would mean that the bundle was generated with another version of the sources. In this case it is maybe even better not to invalidate the cache since it could lead to an invalidation war between two workers.
The only case where invalidating this cache is useful is when a bundle changes, Usually if an ir_asset is created, modified, ...
There is still another rare but possible possibility to have a 404 if the transaction is roll backed after populating the assets node cache.
In this case, we only need to clear the cache locally in case of rollback.
## Copy bundle when possible (2nd commit)
If get_attachments fails, the next step will be to generate the attachments from scratch, a slow operations.
When creating a new website, all assets bundle would actually be similar to their version without website, but the url is different.
This can be visible because the first loading of /web is slow after creating a new website: the website_id is forced in the session and the `assets_backend` are regenerated, identical to the original ones.
This commit proposes to try to find an attachments with different extra but the same uniquifier when possible and copy it's content.
Note that just returning the other attachment url may work, but it would be confusing to randomly have links to assets coming from another website_id. This would also be a problem if the original attachment is unlinked, forcing to recompute it for other websites.
Good to now, since the content is the same, no duplication of the content should appear in the file store, just an entry in the database.
## Third commit
Because of all previous changes, we can simplify even futher. The main idea is to avoid to be forced to call `_get_asset_content` before `_get_asset_bundle` (or `AssetsBundle`). We can get rid of `AssetsBundleMultiWebsite` by making the small change of behaviour between the two work by calling an `ir.asset` method `_get_asset_extra` to generate the extra parameter. This in fine allows to have less overrides.
The "remains" (external assets url) of get_assets_content are also stored on the assets_bundle.Miscellaneous changes
Steps to reproduce ================== - Switch odoo to french - Go to projects - Click on the three dots -> Got duplicate key in t-foreach: Violet Cause of the issue ================== Since [0], the colors have been renamed. In french, both Purple and Violet translates to Violet. Color is a LazyTranslatedString and its string representation is the translation. Solution ======== Use the index for the t-key [0]: https://github.com/odoo/odoo/commit/ef313061301948693bfbe
Original PR description
Steps to reproduce ================== - Switch odoo to french - Go to projects - Click on the three dots -> Got duplicate key in t-foreach: Violet Cause of the issue ================== Since [0], the colors have been renamed. In french, both Purple and Violet translates to Violet. Color is a LazyTranslatedString and its string representation is the translation. Solution ======== Use the index for the t-key [0]: https://github.com/odoo/odoo/commit/ef313061301948693bfbeb16e8ccca786e5251f4#diff-df052c03baba52d267dcf7714eb1a1d03f1eddf85f096ff1c8d016f12f71a6b5L40-L53 opw-3358597 Forward-Port-Of: odoo/odoo#124458
The confirmation dialog component uses `t-esc` thereby preventing the use of markup for the body prop. With this commit, we use `t-out` instead and thereby allowing markup. The same applies for the AlertDialog [IMP] web: enhance extensibility of form/list confirmation dialog when deleting records To extend the functionality of the delete confirmation in the form and list controllers, a lot of code has to be copied. This commit separates the props into a getter, making it easier to be ex
Original PR description
The confirmation dialog component uses `t-esc` thereby preventing the use of markup for the body prop. With this commit, we use `t-out` instead and thereby allowing markup. The same applies for the AlertDialog [IMP] web: enhance extensibility of form/list confirmation dialog when deleting records To extend the functionality of the delete confirmation in the form and list controllers, a lot of code has to be copied. This commit separates the props into a getter, making it easier to be extended. Forward-Port-Of: odoo/odoo#124221 Forward-Port-Of: odoo/odoo#123532
Allow timesheeting on sub-tasks with no project_id set. Instead, refer to the project_id set on its parent_id, and so on, recursively. task-3336215 --- I confirm I have signed the CLA and read the PR guidelines at www.odoo.com/submit-pr Forward-Port-Of: odoo/odoo#122520
Original PR description
Allow timesheeting on sub-tasks with no project_id set. Instead, refer to the project_id set on its parent_id, and so on, recursively. task-3336215 --- I confirm I have signed the CLA and read the PR guidelines at www.odoo.com/submit-pr Forward-Port-Of: odoo/odoo#122520
Desired behavior after PR is merged: Sign the CLA to make more contribution --- I confirm I have signed the CLA and read the PR guidelines at www.odoo.com/submit-pr Forward-Port-Of: odoo/odoo#124562
Original PR description
Desired behavior after PR is merged: Sign the CLA to make more contribution --- I confirm I have signed the CLA and read the PR guidelines at www.odoo.com/submit-pr Forward-Port-Of: odoo/odoo#124562
Send a request to any json-rpc route with a HTTP header `Content-Type: application/json-rpc` but send non-json or non-jsonrpc data in the body. The application crashes with a 500 Internal Server Error instead of a 400 Bad Request one. Closes: #122048 Description of the issue/feature this PR addresses: Current behavior before PR: Desired behavior after PR is merged: --- I confirm I have signed the CLA and read the PR guidelines at www.odoo.com/submit-pr Forward-Port-Of: odo
Original PR description
Send a request to any json-rpc route with a HTTP header `Content-Type: application/json-rpc` but send non-json or non-jsonrpc data in the body. The application crashes with a 500 Internal Server Error instead of a 400 Bad Request one. Closes: #122048 Description of the issue/feature this PR addresses: Current behavior before PR: Desired behavior after PR is merged: --- I confirm I have signed the CLA and read the PR guidelines at www.odoo.com/submit-pr Forward-Port-Of: odoo/odoo#122437
Task: 3336403 Forward-Port-Of: odoo/enterprise#41334
Original PR description
Task: 3336403 Forward-Port-Of: odoo/enterprise#41334
* Allow today's timesheet validation. * Allow users to timesheet today, even if last validation is today. * Adapt Error Message when no timesheet to validate. task-3284604 Forward-Port-Of: odoo/enterprise#42239 Forward-Port-Of: odoo/enterprise#41118
Original PR description
* Allow today's timesheet validation. * Allow users to timesheet today, even if last validation is today. * Adapt Error Message when no timesheet to validate. task-3284604 Forward-Port-Of: odoo/enterprise#42239 Forward-Port-Of: odoo/enterprise#41118
Fixes 2 different bug, more details in each commit - all timesheets > on searching project/task filter > traceback - my timesheet > list view > group by "project" > start > create a project > stop > Traceback Task-3292566 --- I confirm I have signed the CLA and read the PR guidelines at www.odoo.com/submit-pr Forward-Port-Of: odoo/enterprise#40786
Original PR description
Fixes 2 different bug, more details in each commit - all timesheets > on searching project/task filter > traceback - my timesheet > list view > group by "project" > start > create a project > stop > Traceback Task-3292566 --- I confirm I have signed the CLA and read the PR guidelines at www.odoo.com/submit-pr Forward-Port-Of: odoo/enterprise#40786
The associated community PR makes a change to the confirmation dialog template, which is extended in documents_spreadsheet. This commit fixes the xpath target Forward-Port-Of: odoo/enterprise#42279
Original PR description
The associated community PR makes a change to the confirmation dialog template, which is extended in documents_spreadsheet. This commit fixes the xpath target Forward-Port-Of: odoo/enterprise#42279
While creating a post on 'social_twitter' if we upload a corrupted image or when the API sends improper response after clicking on post button it will show image uploading status failed in the Kanban view below and when we click on retry we will encouter 'AttributeError: 'UserError' object has no attribute 'name'' . Steps to produce: 1) In 'social_twitter' click on new post. 2) Add a particular mesage and upload a corrupted image. 3) Now click on post. 5) In the Knaban view below it will s
Original PR description
While creating a post on 'social_twitter' if we upload a corrupted image or when the API sends improper response after clicking on post button it will show image uploading status failed in the Kanban…
While creating a post on 'social_twitter' if we upload a corrupted image or when the API sends improper response after clicking on post button it will show image uploading status failed in the Kanban view below and when we click on retry we will encouter 'AttributeError: 'UserError' object has no attribute 'name'' .
Steps to produce:
1) In 'social_twitter' click on new post.
2) Add a particular mesage and upload a corrupted image. 3) Now click on post.
5) In the Knaban view below it will show image uploading status 'Failed'. 6) Click on retry.
By following above steps you will be able to encounter error.
Traceback:
```
UserError: We could not upload your image, try reducing its size and posting it again (error: ).
File "home/odoo/src/enterprise/saas-16.2/social_twitter/models/social_live_post.py", line 92, in _post_twitter
images_attachments_ids = account._format_attachments_to_images_twitter(post.image_ids)
File "home/odoo/src/enterprise/saas-16.2/social_twitter/models/social_account.py", line 167, in _format_attachments_to_images_twitter
return self._format_images_twitter([{
File "home/odoo/src/enterprise/saas-16.2/social_twitter/models/social_account.py", line 193, in _format_images_twitter
media_id = self._init_twitter_upload(image)
File "home/odoo/src/enterprise/saas-16.2/social_twitter/models/social_account.py", line 221, in _init_twitter_upload
raise UserError(_("We could not upload your image, try reducing its size and posting it again (error: %s).", generic_api_error))
AttributeError: 'UserError' object has no attribute 'name'
File "odoo/http.py", line 2115, in __call__
response = request._serve_db()
File "odoo/http.py", line 1698, in _serve_db
return service_model.retrying(self._serve_ir_http, self.env)
File "odoo/service/model.py", line 134, in retrying
result = func()
File "odoo/http.py", line 1725, in _serve_ir_http
response = self.dispatcher.dispatch(rule.endpoint, args)
File "odoo/http.py", line 1922, in dispatch
result = self.request.registry['ir.http']._dispatch(endpoint)
File "odoo/addons/base/models/ir_http.py", line 154, in _dispatch
result = endpoint(**request.params)
File "odoo/http.py", line 715, in route_wrapper
result = endpoint(self, *args, **params_ok)
File "addons/web/controllers/dataset.py", line 32, in call_button
action = self._call_kw(model, method, args, kwargs)
File "addons/web/controllers/dataset.py", line 24, in _call_kw
return call_kw(request.env[model], method, args, kwargs)
File "odoo/api.py", line 461, in call_kw
result = _call_kw_multi(method, model, args, kwargs)
File "odoo/api.py", line 448, in _call_kw_multi
result = method(recs, *args, **kwargs)
File "home/odoo/src/enterprise/saas-16.2/social/models/social_live_post.py", line 74, in action_retry_post
self._post()
File "home/odoo/src/enterprise/saas-16.2/social_youtube/models/social_live_post.py", line 73, in _post
super(SocialLivePostYoutube, (self - youtube_live_posts))._post()
File "home/odoo/src/enterprise/saas-16.2/social_twitter/models/social_live_post.py", line 78, in _post
twitter_live_posts._post_twitter()
File "home/odoo/src/enterprise/saas-16.2/social_twitter/models/social_live_post.py", line 96, in _post_twitter
'failure_reason': e.name
```
This commit will fix the Attribute error by using a proper value for 'failure_reason'. Also the file size of image can be upto 20 MB according to our current R & D.
sentry- 4147783585
Forward-Port-Of: odoo/enterprise#40754