[FIX] fields: setting a one2many field deletes all its lines (#13082)

Setting the value of a one2many field (`rec.line_ids = ...`) converts the new
recordset to a list of commands, and passes it to `write`.

Currently the list of commands is like: `[(5,), (0, 0, vals), (1, id, vals), (4, id), ...]`.
Notice that the command `(5,)` is inappropriate here: it deletes all the
existing lines of the record. Reintroducing the same lines with `(4, id)`
commands crashes, because the corresponding `id` is no longer in database.

The fix serializes the recordset as a list of commands like
`[(6, 0, ids), (0, 0, vals), (1, id, vals), ...]`.
Only the lines that are not kept will be deleted in this case.

openerp/addons/test_new_api/tests/__init__.py

@@ -5,6 +5,7 @@ from . import test_new_fields
 from . import test_onchange
 from . import test_field_conversions
 from . import test_attributes
+from . import test_one2many
 from . import test_ui
 from . import test_no_infinite_recursion
 from . import test_domain

openerp/addons/test_new_api/tests/test_onchange.py

@@ -176,7 +176,7 @@ class TestOnChange(common.TransactionCase):
         self.assertEqual(values, {
             'name': partner.name,
             'partner': partner.id,
-            'lines': [(5,), (4, line.id)],
+            'lines': [(6, 0, [line.id])],
         })
 
         # modify 'partner'

openerp/addons/test_new_api/tests/test_one2many.py

@@ -0,0 +1,65 @@
+# -*- coding: utf-8 -*-
+from openerp.tests.common import TransactionCase
+
+
+class One2manyCase(TransactionCase):
+    def setUp(self):
+        super(One2manyCase, self).setUp()
+        self.Line = self.env["test_new_api.multi.line"]
+        self.multi = self.env["test_new_api.multi"].create({
+            "name": "What is up?"
+        })
+
+    def operations(self):
+        """Run operations on o2m fields to check all works fine."""
+        # Check the lines first
+        self.assertItemsEqual(
+            self.multi.lines.mapped('name'),
+            map(str, range(10)))
+        # Modify the first line and drop the last one
+        self.multi.lines[0].name = "hello"
+        self.multi.lines = self.multi.lines[:-1]
+        self.assertEqual(len(self.multi.lines), 9)
+        self.assertIn("hello", self.multi.lines.mapped('name'))
+        # Invalidate the cache and check again; this crashes if the value
+        # of self.multi.lines in cache contains new records
+        self.multi.invalidate_cache()
+        self.assertEqual(len(self.multi.lines), 9)
+        self.assertIn("hello", self.multi.lines.mapped('name'))
+
+    def test_new_one_by_one(self):
+        """Check lines created with ``new()`` and appended one by one."""
+        for name in range(10):
+            self.multi.lines |= self.Line.new({"name": str(name)})
+        self.operations()
+
+    def test_new_single(self):
+        """Check lines created with ``new()`` and added in one step."""
+        self.multi.lines = self.Line.browse(
+            [self.Line.new({"name": str(name)}).id for name in range(10)]
+        )
+        self.operations()
+
+    def test_create_one_by_one(self):
+        """Check lines created with ``create()`` and appended one by one."""
+        for name in range(10):
+            self.multi.lines |= self.Line.create({"name": str(name)})
+        self.operations()
+
+    def test_create_single(self):
+        """Check lines created with ``create()`` and added in one step."""
+        self.multi.lines = self.Line.browse(
+            [self.Line.create({"name": str(name)}).id for name in range(10)]
+        )
+        self.operations()
+
+    def test_rpcstyle_one_by_one(self):
+        """Check lines created with RPC style and appended one by one."""
+        for name in range(10):
+            self.multi.lines = [(0, 0, {"name": str(name)})]
+        self.operations()
+
+    def test_rpcstyle_single(self):
+        """Check lines created with RPC style and added in one step"""
+        self.multi.lines = [(0, 0, {'name': str(name)}) for name in range(10)]
+        self.operations()

openerp/fields.py

@@ -861,9 +861,11 @@ class Field(object):
             env.invalidate(spec)
 
         else:
-            # simply write to the database, and update cache
+            # Write to database
             record.write({self.name: self.convert_to_write(value)})
-            record._cache[self] = value
+            # Update the cache unless value contains a new record
+            if all(getattr(value, '_ids', ())):
+                record._cache[self] = value
 
     ############################################################################
     #
@@ -1795,7 +1797,7 @@ class _RelationalMulti(_Relational):
 
     def convert_to_write(self, value):
         # make result with new and existing records
-        result = [(5,)]
+        result = [(6, 0, [])]
         for record in value:
             if not record.id:
                 values = dict(record._cache)
@@ -1806,7 +1808,7 @@ class _RelationalMulti(_Relational):
                 values = record._convert_to_write(values)
                 result.append((1, record.id, values))
             else:
-                result.append((4, record.id))
+                result[0][2].append(record.id)
         return result
 
     def convert_to_onchange(self, value, fnames=None):

openerp/models.py

@@ -3807,8 +3807,7 @@ class BaseModel(object):
           ``(6, _, ids)``
               replaces all existing records in the set by the ``ids`` list,
               equivalent to using the command ``5`` followed by a command
-              ``4`` for each ``id`` in ``ids``. Can not be used on
-              :class:`~openerp.fields.One2many`.
+              ``4`` for each ``id`` in ``ids``.
 
           .. note:: Values marked as ``_`` in the list above are ignored and
                     can be anything, generally ``0`` or ``False``.

openerp/osv/fields.py

@@ -863,7 +863,11 @@ class one2many(_column):
                     ids2 = act[2] or [0]
                     cr.execute('select id from '+_table+' where '+self._fields_id+'=%s and id <> ALL (%s)', (id,ids2))
                     ids3 = map(lambda x:x[0], cr.fetchall())
-                    obj.write(cr, user, ids3, {self._fields_id:False}, context=context or {})
+                    inverse_field = obj._fields.get(self._fields_id)
+                    if getattr(inverse_field, "ondelete", None) == "cascade":
+                        obj.unlink(cr, user, ids3, context=context)
+                    else:
+                        obj.write(cr, user, ids3, {self._fields_id: False}, context=context or {})
         return result
 
     def search(self, cr, obj, args, name, value, offset=0, limit=None, uid=None, operator='like', context=None):