From 87a1ebb338277e45d2367344c212d4867405b7a4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Voet=20=28ryv=29?= Date: Mon, 8 Apr 2024 09:02:26 +0200 Subject: [PATCH] [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 write() on binary fields. Part-of: odoo/odoo#160708 --- .../test_new_api/models/test_new_api.py | 1 + .../test_new_api/tests/test_new_fields.py | 93 +++++++++++++++---- odoo/api.py | 2 + odoo/fields.py | 1 + odoo/models.py | 3 +- 5 files changed, 83 insertions(+), 17 deletions(-) diff --git a/odoo/addons/test_new_api/models/test_new_api.py b/odoo/addons/test_new_api/models/test_new_api.py index e961886c86c..2a4c6a3a0fc 100644 --- a/odoo/addons/test_new_api/models/test_new_api.py +++ b/odoo/addons/test_new_api/models/test_new_api.py @@ -931,6 +931,7 @@ class ModelImage(models.Model): image_512 = fields.Image("Image 512", related='image', max_width=512, max_height=512, store=True, readonly=False) image_256 = fields.Image("Image 256", related='image', max_width=256, max_height=256, store=False, readonly=False) image_128 = fields.Image("Image 128", max_width=128, max_height=128) + image_64 = fields.Image("Image 64", related='image', max_width=64, max_height=64, store=True, attachment=False) class BinarySvg(models.Model): diff --git a/odoo/addons/test_new_api/tests/test_new_fields.py b/odoo/addons/test_new_api/tests/test_new_fields.py index 49a4e52c397..0b191ae34a3 100644 --- a/odoo/addons/test_new_api/tests/test_new_fields.py +++ b/odoo/addons/test_new_api/tests/test_new_fields.py @@ -2596,6 +2596,8 @@ class TestFields(TransactionCaseWithUserDemo): self.assertEqual(Image.open(io.BytesIO(base64.b64decode(record.image_512))).size, (512, 256)) # test create related no store (resize, width limited) self.assertEqual(Image.open(io.BytesIO(base64.b64decode(record.image_256))).size, (256, 128)) + # test create related store on column (resize, width limited) + self.assertEqual(Image.open(io.BytesIO(base64.b64decode(record.image_64))).size, (64, 32)) record.write({ 'image': image_h, @@ -2610,6 +2612,8 @@ class TestFields(TransactionCaseWithUserDemo): self.assertEqual(Image.open(io.BytesIO(base64.b64decode(record.image_512))).size, (256, 512)) # test write related no store (resize, height limited) self.assertEqual(Image.open(io.BytesIO(base64.b64decode(record.image_256))).size, (128, 256)) + # test write related store on column (resize, width limited) + self.assertEqual(Image.open(io.BytesIO(base64.b64decode(record.image_64))).size, (32, 64)) record = self.env['test_new_api.model_image'].create({ 'name': 'image', @@ -2625,6 +2629,8 @@ class TestFields(TransactionCaseWithUserDemo): self.assertEqual(Image.open(io.BytesIO(base64.b64decode(record.image_512))).size, (256, 512)) # test create related no store (resize, height limited) self.assertEqual(Image.open(io.BytesIO(base64.b64decode(record.image_256))).size, (128, 256)) + # test create related store on column (resize, width limited) + self.assertEqual(Image.open(io.BytesIO(base64.b64decode(record.image_64))).size, (32, 64)) record.write({ 'image': image_w, @@ -2639,6 +2645,8 @@ class TestFields(TransactionCaseWithUserDemo): self.assertEqual(Image.open(io.BytesIO(base64.b64decode(record.image_512))).size, (512, 256)) # test write related store (resize, width limited) self.assertEqual(Image.open(io.BytesIO(base64.b64decode(record.image_256))).size, (256, 128)) + # test write related store on column (resize, width limited) + self.assertEqual(Image.open(io.BytesIO(base64.b64decode(record.image_64))).size, (64, 32)) # test create inverse store record = self.env['test_new_api.model_image'].create({ @@ -2649,6 +2657,7 @@ class TestFields(TransactionCaseWithUserDemo): self.assertEqual(Image.open(io.BytesIO(base64.b64decode(record.image_512))).size, (512, 256)) self.assertEqual(Image.open(io.BytesIO(base64.b64decode(record.image))).size, (4000, 2000)) self.assertEqual(Image.open(io.BytesIO(base64.b64decode(record.image_256))).size, (256, 128)) + self.assertEqual(Image.open(io.BytesIO(base64.b64decode(record.image_64))).size, (64, 32)) # test write inverse store record.write({ 'image_512': image_h, @@ -2657,6 +2666,7 @@ class TestFields(TransactionCaseWithUserDemo): self.assertEqual(Image.open(io.BytesIO(base64.b64decode(record.image_512))).size, (256, 512)) self.assertEqual(Image.open(io.BytesIO(base64.b64decode(record.image))).size, (2000, 4000)) self.assertEqual(Image.open(io.BytesIO(base64.b64decode(record.image_256))).size, (128, 256)) + self.assertEqual(Image.open(io.BytesIO(base64.b64decode(record.image_64))).size, (32, 64)) # test create inverse no store record = self.env['test_new_api.model_image'].with_context(image_no_postprocess=True).create({ @@ -2667,6 +2677,7 @@ class TestFields(TransactionCaseWithUserDemo): self.assertEqual(Image.open(io.BytesIO(base64.b64decode(record.image_512))).size, (512, 256)) self.assertEqual(Image.open(io.BytesIO(base64.b64decode(record.image))).size, (4000, 2000)) self.assertEqual(Image.open(io.BytesIO(base64.b64decode(record.image_256))).size, (256, 128)) + self.assertEqual(Image.open(io.BytesIO(base64.b64decode(record.image_64))).size, (64, 32)) # test write inverse no store record.write({ 'image_256': image_h, @@ -2675,6 +2686,7 @@ class TestFields(TransactionCaseWithUserDemo): self.assertEqual(Image.open(io.BytesIO(base64.b64decode(record.image_512))).size, (256, 512)) self.assertEqual(Image.open(io.BytesIO(base64.b64decode(record.image))).size, (2000, 4000)) self.assertEqual(Image.open(io.BytesIO(base64.b64decode(record.image_256))).size, (128, 256)) + self.assertEqual(Image.open(io.BytesIO(base64.b64decode(record.image_64))).size, (32, 64)) # test bin_size record_bin_size = record.with_context(bin_size=True) @@ -2703,18 +2715,16 @@ class TestFields(TransactionCaseWithUserDemo): with self.assertQueryCount(0): new_record.image = image_w - def test_95_binary_bin_size(self): + def test_95_binary_bin_size_create(self): binary_value = base64.b64encode(b'content') binary_size = b'7.00 bytes' def assertBinaryValue(record, value): for field in ('binary', 'binary_related_store', 'binary_related_no_store'): - self.assertEqual(record[field], value) + self.assertEqual(record[field], value, f'Incorrect result for {field}') - # created, flushed, and first read without context + # created and first read without context record = self.env['test_new_api.model_binary'].create({'binary': binary_value}) - self.env.flush_all() - self.env.invalidate_all() record_no_bin_size = record.with_context(bin_size=False) record_bin_size = record.with_context(bin_size=True) @@ -2722,10 +2732,8 @@ class TestFields(TransactionCaseWithUserDemo): assertBinaryValue(record_no_bin_size, binary_value) assertBinaryValue(record_bin_size, binary_size) - # created, flushed, and first read with bin_size=False + # created and first read with bin_size=False record_no_bin_size = self.env['test_new_api.model_binary'].with_context(bin_size=False).create({'binary': binary_value}) - self.env.flush_all() - self.env.invalidate_all() record = self.env['test_new_api.model_binary'].browse(record.id) record_bin_size = record.with_context(bin_size=True) @@ -2733,10 +2741,8 @@ class TestFields(TransactionCaseWithUserDemo): assertBinaryValue(record, binary_value) assertBinaryValue(record_bin_size, binary_size) - # created, flushed, and first read with bin_size=True + # created and first read with bin_size=True record_bin_size = self.env['test_new_api.model_binary'].with_context(bin_size=True).create({'binary': binary_value}) - self.env.flush_all() - self.env.invalidate_all() record = self.env['test_new_api.model_binary'].browse(record.id) record_no_bin_size = record.with_context(bin_size=False) @@ -2744,12 +2750,11 @@ class TestFields(TransactionCaseWithUserDemo): assertBinaryValue(record_no_bin_size, binary_value) assertBinaryValue(record, binary_value) - # created without context and flushed with bin_size + # created without context and flushed/invalidated with bin_size=True record = self.env['test_new_api.model_binary'].create({'binary': binary_value}) + record.with_context(bin_size=True).env.invalidate_all() record_no_bin_size = record.with_context(bin_size=False) record_bin_size = record.with_context(bin_size=True) - self.env.flush_all() - self.env.invalidate_all() assertBinaryValue(record, binary_value) assertBinaryValue(record_no_bin_size, binary_value) @@ -2757,8 +2762,6 @@ class TestFields(TransactionCaseWithUserDemo): # check computed binary field with arbitrary Python value record = self.env['test_new_api.model_binary'].create({}) - self.env.flush_all() - self.env.invalidate_all() record_no_bin_size = record.with_context(bin_size=False) record_bin_size = record.with_context(bin_size=True) @@ -2767,6 +2770,64 @@ class TestFields(TransactionCaseWithUserDemo): self.assertEqual(record_no_bin_size.binary_computed, expected_value) self.assertEqual(record_bin_size.binary_computed, expected_value) + def test_95_binary_bin_size_write(self): + binary_value = base64.b64encode(b'content') + binary_size = b'7.00 bytes' + + def assertBinaryValue(record, value): + for field in ('binary', 'binary_related_store', 'binary_related_no_store'): + self.assertEqual(record[field], value, f'Incorrect result for {field}') + + # created and written without context + record = self.env['test_new_api.model_binary'].create({}) + record.write({'binary': binary_value}) + record_no_bin_size = record.with_context(bin_size=False) + record_bin_size = record.with_context(bin_size=True) + + assertBinaryValue(record, binary_value) + assertBinaryValue(record_no_bin_size, binary_value) + assertBinaryValue(record_bin_size, binary_size) + + # created without context, written with bin_size=False + record = self.env['test_new_api.model_binary'].create({}) + record.with_context(bin_size=False).write({'binary': binary_value}) + record_bin_size = record.with_context(bin_size=True) + + assertBinaryValue(record_no_bin_size, binary_value) + assertBinaryValue(record, binary_value) + assertBinaryValue(record_bin_size, binary_size) + + # created without context, written with bin_size=True + record = self.env['test_new_api.model_binary'].create({}) + record.with_context(bin_size=True).write({'binary': binary_value}) + record_no_bin_size = record.with_context(bin_size=False) + + assertBinaryValue(record_bin_size, binary_size) + assertBinaryValue(record_no_bin_size, binary_value) + assertBinaryValue(record, binary_value) + + # created without context and flushed with bin_size=True + record = self.env['test_new_api.model_binary'].create({}) + record.write({'binary': binary_value}) + record.with_context(bin_size=True).env.invalidate_all() + record_no_bin_size = record.with_context(bin_size=False) + record_bin_size = record.with_context(bin_size=True) + + assertBinaryValue(record, binary_value) + assertBinaryValue(record_no_bin_size, binary_value) + assertBinaryValue(record_bin_size, binary_size) + + # created and written without context, flushed without bin_size + record = self.env['test_new_api.model_binary'].create({}) + record.write({'binary': binary_value}) + record.env.invalidate_all() + record_no_bin_size = record.with_context(bin_size=False) + record_bin_size = record.with_context(bin_size=True) + + assertBinaryValue(record, binary_value) + assertBinaryValue(record_no_bin_size, binary_value) + assertBinaryValue(record_bin_size, binary_size) + def test_96_order_m2o(self): belgium, congo = self.env['test_new_api.country'].create([ {'name': "Duchy of Brabant"}, diff --git a/odoo/api.py b/odoo/api.py index a695ef2993f..32c55f35ada 100644 --- a/odoo/api.py +++ b/odoo/api.py @@ -815,6 +815,8 @@ class Environment(Mapping): return get_context('lang') or None elif key == 'active_test': return get_context('active_test', field.context.get('active_test', True)) + elif key.startswith('bin_size'): + return bool(get_context(key)) else: val = get_context(key) if type(val) is list: diff --git a/odoo/fields.py b/odoo/fields.py index 5b3f2f42434..000faaa1a63 100644 --- a/odoo/fields.py +++ b/odoo/fields.py @@ -2458,6 +2458,7 @@ class Binary(Field): ]) def write(self, records, value): + records = records.with_context(bin_size=False) if not self.attachment: super().write(records, value) return diff --git a/odoo/models.py b/odoo/models.py index d56fc811ca0..48868dbea44 100644 --- a/odoo/models.py +++ b/odoo/models.py @@ -4797,13 +4797,14 @@ class BaseModel(metaclass=MetaModel): ids.extend(id_ for id_, in cr.fetchall()) # put the new records in cache, and update inverse fields, for many2one + # (using bin_size=False to put binary values in the right place) # # cachetoclear is an optimization to avoid modified()'s cost until other_fields are processed cachetoclear = [] records = self.browse(ids) inverses_update = defaultdict(list) # {(field, value): ids} common_set_vals = set(LOG_ACCESS_COLUMNS + ['id', 'parent_path']) - for data, record in zip(data_list, records): + for data, record in zip(data_list, records.with_context(bin_size=False)): data['record'] = record # DLE P104: test_inherit.py, test_50_search_one2many vals = dict({k: v for d in data['inherited'].values() for k, v in d.items()}, **data['stored'])