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 #164045

Conversation

fw-bot
Copy link
Contributor

@fw-bot fw-bot commented Apr 30, 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.

First proposal: #156673

Forward-Port-Of: #163634
Forward-Port-Of: #160708

ryv-odoo and others added 6 commits April 30, 2024 17:45
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.

X-original-commit: e79d689
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.

X-original-commit: 1885531
…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.

X-original-commit: ecc0191
Non-attachment binary fields need to be flushed before reading their
size, since the latter relies on the database's binary size function.

X-original-commit: fb3d451
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

X-original-commit: 400790e
Co-authored-by: Renaud Thiry <reth@odoo.com>
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.

X-original-commit: 4daf34a
@robodoo
Copy link
Contributor

robodoo commented Apr 30, 2024

@fw-bot
Copy link
Contributor Author

fw-bot commented Apr 30, 2024

This PR targets saas-17.2 and is part of the forward-port chain. Further PRs will be created up to master.

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

@robodoo robodoo added the forwardport This PR was created by @fw-bot label Apr 30, 2024
@C3POdoo C3POdoo added the RD research & development, internal work label Apr 30, 2024
robodoo pushed a commit that referenced this pull request May 8, 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.

X-original-commit: e79d689
Part-of: #164045
robodoo pushed a commit that referenced this pull request May 8, 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.

X-original-commit: 1885531
Part-of: #164045
robodoo pushed a commit that referenced this pull request May 8, 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.

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

X-original-commit: fb3d451
Part-of: #164045
robodoo pushed a commit that referenced this pull request May 8, 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

X-original-commit: 400790e
Part-of: #164045
Co-authored-by: Renaud Thiry <reth@odoo.com>
robodoo pushed a commit that referenced this pull request May 8, 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 #164045

X-original-commit: 4daf34a
Signed-off-by: Raphael Collet <rco@odoo.com>
Signed-off-by: Rémy Voet (ryv) <ryv@odoo.com>
@robodoo robodoo closed this May 8, 2024
@fw-bot fw-bot deleted the saas-17.2-17.0-example-binary-store-compute-reth-ryv-1jft-fw branch May 22, 2024 10:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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

4 participants