Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FW] Bunch of fixes for Binary/Image fields #163634

Conversation

fw-bot
Copy link
Contributor

@fw-bot fw-bot commented Apr 26, 2024

[FIX] core: add invalidation of Environment's _cache_key.

Changing the environment in method create() to force bin_size=False
looks harmless, but it actually breaks many tests, in particular in
module account. The reason is that company_dependent fields are read at
the wrong place in the cache. And this is because env._cache_key can
be polluted with old data.

Make sure that _cache_key is cleared when resetting all the lazy
properties on the environment. Only the change in res_user.py makes it
work, but let's not tempt the devil.

Side note: I hate caches.

[FIX] test_new_api: test's change that already works

Add some test cases, and also remove useless flush/invalidate.

[FIX] core: fix create/write on binary fields

When invoking create() or write() with a binary field, the cache of the
field was incorrect if bin_size=True was in context. Force context with
bin_size=False when putting a binary value in cache. It is particularly
important to have coherent values in the cache for web_save.

Also, because an environment with bin_size=False won't return the same
context cache key as one with bin_size=None, it leads to have a cache
inconstistency when we write with bin_size=False. Change Environment
method cache_key() to return the same cache key when bin_size is absent,
bin_size=None or bin_size=False.

Tests on binary fields have been updated to not rely on flush and
invalidate. We also created specific tests for the binary write() path.

[FIX] core: cache inconsistency when assigning related resized image field

After writing or creating on a related Image field, its cache contains
the full-size image instead of the resized one (according to its
attributes max_width and max_height). Fix it by re-setting the
the resized image on the cache at the end of the inverse method.

[FIX] core: flush non-attachment binary field when necessary

Non-attachment binary field needs to be flushed before reading their size,
since the latter relies on the database's binary size function.

[FIX] core: fix no-attachment binary for web_save

When bin_size=True is in the context, the computed no-attachment binary
was incorrectly saved to the database. The row is updated with the size
of the binary instead of the value itself (check compute_value() +
cache.set).

Fix this problem by avoiding setting the cache of the bin_size value
as dirty.

Moreover, the binary size is computed with pg_size_pretty for the
no-attachment binary fields. Also, in compute_value() we called
b64decode on the value that was previously encoded in base64 by
_compute_datas(). But in our case, we never use _compute_datas()
because it is not stored as an attachment. Then b64decode() doesn't
make sense in this case.

These 2 bugs are now covered by testing web_save(), where cache
consistency is required. It was first reported for this method.

[FIX] core: flush with the correct context

There is a typo in ddfc2aa :
self.with_context(context={}), it adds "context":{} to the context
itself, instead of clearing it. It breaks the new tests of binary
fields, and it is important to flush with an empty context to write
the correct value of context-dependent fields (like binary fields).

Fix this typo.

Forward-Port-Of: #160708

@robodoo
Copy link
Contributor

robodoo commented Apr 26, 2024

@fw-bot
Copy link
Contributor Author

fw-bot commented Apr 26, 2024

@ryv-odoo @rco-odoo cherrypicking of pull request #160708 failed.

stderr:

20:56:04.677921 git.c:463               trace: built-in: git cherry-pick a0cd4b26276b4c55e3bb550511e2b417521086e7
error: Cherry-picking is not possible because you have unmerged files.
hint: Fix them up in the work tree, and then use 'git add/rm <file>'
hint: as appropriate to mark resolution and make a commit.
fatal: cherry-pick failed
----------
status:

Either perform the forward-port manually (and push to this branch, proceeding as usual) or close this PR (maybe?).

In the former case, you may want to edit this PR message as well.

⚠️ after resolving this conflict, you will need to merge it via @robodoo.

More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port

@C3POdoo C3POdoo added the RD research & development, internal work label Apr 26, 2024
@robodoo robodoo added forwardport This PR was created by @fw-bot conflict There was an error while creating this forward-port PR labels Apr 26, 2024
ryv-odoo and others added 5 commits April 29, 2024 09:29
Changing the environment in method create() to force bin_size=False
looks harmless, but it actually breaks many tests, in particular in
module account.  The reason is that company_dependent fields are read at
the wrong place in the cache.  And this is because `env._cache_key` can
be polluted with old data.

Make sure that `_cache_key` is cleared when resetting all the lazy
properties on the environment.  Only the change in res_user.py makes it
work, but let's not tempt the devil.

Side note: I hate caches.
When invoking create() or write() with a binary field, the cache of the
field was incorrect if bin_size=True was in context.  Force context with
bin_size=False when putting a binary value in cache.  It is particularly
important to have coherent values in the cache for `web_save`.

Also, because an environment with bin_size=False won't return the same
context cache key as one with bin_size=None, it leads to have a cache
inconstistency when we write with bin_size=False.  Change Environment
method cache_key() to return the same cache key when bin_size is absent,
bin_size=None or bin_size=False.

Tests on binary fields have been updated to not rely on flush and
invalidate.  We also created specific tests for write() on binary
fields.
…field

After writing or creating on a related Image field, its cache contains
the full-size image instead of the resized one (according to its
attributes max_width and max_height).  Fix the cache with the resized
image at the end of the inverse method.
Non-attachment binary fields need to be flushed before reading their
size, since the latter relies on the database's binary size function.
When bin_size=True is in the context, a computed non-attachment binary
is incorrectly saved to the database.  The row is actually updated with
the size of the binary instead of the value itself.

This commit fixes the problem by avoiding setting the cache with the
bin_size value as dirty.

Moreover, the binary size is computed with `pg_size_pretty` for
non-attachment binary fields.  Also, method compute_value() calls
b64decode() on the value that was previously encoded in base64 by
_compute_datas().  But _compute_datas() is specific to attachments, and
is not used in this case.  Thus b64decode() doesn't make sense.

These 3 bugs are now covered by testing web_save(), where cache
consistency is required. It was first reported for this method.

Closes odoo#156673

Co-authored-by: Renaud Thiry <reth@odoo.com>
@ryv-odoo ryv-odoo force-pushed the saas-17.1-17.0-example-binary-store-compute-reth-ryv-zXDN-fw branch from d58cd90 to 0e86d78 Compare April 29, 2024 07:43
@C3POdoo C3POdoo requested review from a team, xmo-odoo and HydrionBurst and removed request for a team April 29, 2024 07:46
There is a typo in ddfc2aa :
`self.with_context(context={})`, it adds "context":{} to the context
itself, instead of clearing it. It breaks the new tests of binary
fields, and it is important to flush with an empty context to write
the correct value of context-dependent fields (like binary fields).

Fix this typo.
@ryv-odoo ryv-odoo force-pushed the saas-17.1-17.0-example-binary-store-compute-reth-ryv-zXDN-fw branch from ba62cb9 to d364d67 Compare April 29, 2024 15:00
@ryv-odoo
Copy link
Contributor

@robodoo r+

Hello @odoo/rd-security, can someone override this one ?

@robodoo
Copy link
Contributor

robodoo commented Apr 30, 2024

@ryv-odoo you may want to rebuild or fix this PR as it has failed CI.

@mart-e
Copy link
Contributor

mart-e commented Apr 30, 2024

@robodoo override=ci/security

robodoo pushed a commit that referenced this pull request Apr 30, 2024
Changing the environment in method create() to force bin_size=False
looks harmless, but it actually breaks many tests, in particular in
module account.  The reason is that company_dependent fields are read at
the wrong place in the cache.  And this is because `env._cache_key` can
be polluted with old data.

Make sure that `_cache_key` is cleared when resetting all the lazy
properties on the environment.  Only the change in res_user.py makes it
work, but let's not tempt the devil.

Side note: I hate caches.

Part-of: #163634
robodoo pushed a commit that referenced this pull request Apr 30, 2024
When invoking create() or write() with a binary field, the cache of the
field was incorrect if bin_size=True was in context.  Force context with
bin_size=False when putting a binary value in cache.  It is particularly
important to have coherent values in the cache for `web_save`.

Also, because an environment with bin_size=False won't return the same
context cache key as one with bin_size=None, it leads to have a cache
inconstistency when we write with bin_size=False.  Change Environment
method cache_key() to return the same cache key when bin_size is absent,
bin_size=None or bin_size=False.

Tests on binary fields have been updated to not rely on flush and
invalidate.  We also created specific tests for write() on binary
fields.

Part-of: #163634
robodoo pushed a commit that referenced this pull request Apr 30, 2024
…field

After writing or creating on a related Image field, its cache contains
the full-size image instead of the resized one (according to its
attributes max_width and max_height).  Fix the cache with the resized
image at the end of the inverse method.

Part-of: #163634
robodoo pushed a commit that referenced this pull request Apr 30, 2024
Non-attachment binary fields need to be flushed before reading their
size, since the latter relies on the database's binary size function.

Part-of: #163634
robodoo pushed a commit that referenced this pull request Apr 30, 2024
When bin_size=True is in the context, a computed non-attachment binary
is incorrectly saved to the database.  The row is actually updated with
the size of the binary instead of the value itself.

This commit fixes the problem by avoiding setting the cache with the
bin_size value as dirty.

Moreover, the binary size is computed with `pg_size_pretty` for
non-attachment binary fields.  Also, method compute_value() calls
b64decode() on the value that was previously encoded in base64 by
_compute_datas().  But _compute_datas() is specific to attachments, and
is not used in this case.  Thus b64decode() doesn't make sense.

These 3 bugs are now covered by testing web_save(), where cache
consistency is required. It was first reported for this method.

Closes #156673

Part-of: #163634
Co-authored-by: Renaud Thiry <reth@odoo.com>
robodoo pushed a commit that referenced this pull request Apr 30, 2024
There is a typo in ddfc2aa :
`self.with_context(context={})`, it adds "context":{} to the context
itself, instead of clearing it. It breaks the new tests of binary
fields, and it is important to flush with an empty context to write
the correct value of context-dependent fields (like binary fields).

Fix this typo.

closes #163634

Signed-off-by: Raphael Collet <rco@odoo.com>
Signed-off-by: Rémy Voet (ryv) <ryv@odoo.com>
@robodoo robodoo closed this Apr 30, 2024
@fw-bot fw-bot deleted the saas-17.1-17.0-example-binary-store-compute-reth-ryv-zXDN-fw branch May 14, 2024 15:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
conflict There was an error while creating this forward-port PR forwardport This PR was created by @fw-bot RD research & development, internal work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants