Skip to content

Commit

Permalink
[FIX] core: fix create/write on binary fields
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
ryv-odoo authored and rco-odoo committed Apr 26, 2024
1 parent ddd97a3 commit fe287f0
Show file tree
Hide file tree
Showing 5 changed files with 83 additions and 17 deletions.
1 change: 1 addition & 0 deletions odoo/addons/test_new_api/models/test_new_api.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down
93 changes: 77 additions & 16 deletions odoo/addons/test_new_api/tests/test_new_fields.py
Original file line number Diff line number Diff line change
Expand Up @@ -2596,6 +2596,8 @@ def test_94_image(self):
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,
Expand All @@ -2610,6 +2612,8 @@ def test_94_image(self):
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',
Expand All @@ -2625,6 +2629,8 @@ def test_94_image(self):
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,
Expand All @@ -2639,6 +2645,8 @@ def test_94_image(self):
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({
Expand All @@ -2649,6 +2657,7 @@ def test_94_image(self):
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,
Expand All @@ -2657,6 +2666,7 @@ def test_94_image(self):
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({
Expand All @@ -2667,6 +2677,7 @@ def test_94_image(self):
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,
Expand All @@ -2675,6 +2686,7 @@ def test_94_image(self):
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)
Expand Down Expand Up @@ -2703,62 +2715,53 @@ def test_94_image(self):
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)

assertBinaryValue(record, binary_value)
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)

assertBinaryValue(record_no_bin_size, binary_value)
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)

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
# 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)
assertBinaryValue(record_bin_size, binary_size)

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

Expand All @@ -2767,6 +2770,64 @@ def assertBinaryValue(record, value):
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"},
Expand Down
2 changes: 2 additions & 0 deletions odoo/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -815,6 +815,8 @@ def get(key, get_context=self.context.get):
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:
Expand Down
1 change: 1 addition & 0 deletions odoo/fields.py
Original file line number Diff line number Diff line change
Expand Up @@ -2458,6 +2458,7 @@ def create(self, record_values):
])

def write(self, records, value):
records = records.with_context(bin_size=False)
if not self.attachment:
super().write(records, value)
return
Expand Down
3 changes: 2 additions & 1 deletion odoo/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -4797,13 +4797,14 @@ def _create(self, data_list):
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'])
Expand Down

0 comments on commit fe287f0

Please sign in to comment.