diff mbox

[ovs-dev,v1] Python-IDL: getattr after mutate fix

Message ID 20161012143239.19959-1-abiswas@us.ibm.com
State Superseded
Delegated to: Russell Bryant
Headers show

Commit Message

Amitabha Biswas Oct. 12, 2016, 2:32 p.m. UTC
This commit returns the updated column value when getattr is done
after a mutate operation is performed (but before the commit). It
addresses the bug reported in
http://openvswitch.org/pipermail/dev/2016-September/080120.html

Signed-off-by: Amitabha Biswas <abiswas@us.ibm.com>

Reported-by: Richard Theis <rtheis@us.ibm.com>
---
 python/ovs/db/idl.py | 38 ++++++++++++++++++++++++--------------
 tests/ovsdb-idl.at   | 19 ++++++++++++-------
 tests/test-ovsdb.py  | 36 ++++++++++++++++++++++++++++++++++++
 3 files changed, 72 insertions(+), 21 deletions(-)

Comments

Russell Bryant Oct. 12, 2016, 8:16 p.m. UTC | #1
On Wed, Oct 12, 2016 at 10:32 AM, Amitabha Biswas <azbiswas@gmail.com>
wrote:

> This commit returns the updated column value when getattr is done
> after a mutate operation is performed (but before the commit). It
> addresses the bug reported in
> http://openvswitch.org/pipermail/dev/2016-September/080120.html


You can also record this with a "Reported-at" header:

Reported-at: http://openvswitch.org/pipermail/dev/2016-September/080120.html

It would also be good to record the related commit that this fixes.

Fixes: a59912a0ee8e ("python: Add support for partial map and partial set
updates")

Signed-off-by: Amitabha Biswas <abiswas@us.ibm.com>
>

Your Signed-off-by doesn't match the commit author (your gmail address).
Can you re-submit with the two matching?


> Reported-by: Richard Theis <rtheis@us.ibm.com>


Other than the Signed-off-by issue, I think this is ready to apply.

Thanks!
Amitabha Biswas Oct. 12, 2016, 9:37 p.m. UTC | #2
Thanks! I reposted the patch with the update to the commit message.

Amitabha

> On Oct 12, 2016, at 1:16 PM, Russell Bryant <russell@ovn.org> wrote:
> 
> 
> 
> On Wed, Oct 12, 2016 at 10:32 AM, Amitabha Biswas <azbiswas@gmail.com <mailto:azbiswas@gmail.com>> wrote:
> This commit returns the updated column value when getattr is done
> after a mutate operation is performed (but before the commit). It
> addresses the bug reported in
> http://openvswitch.org/pipermail/dev/2016-September/080120.html <http://openvswitch.org/pipermail/dev/2016-September/080120.html>
> 
> You can also record this with a "Reported-at" header:
> 
> Reported-at: http://openvswitch.org/pipermail/dev/2016-September/080120.html <http://openvswitch.org/pipermail/dev/2016-September/080120.html>
> 
> It would also be good to record the related commit that this fixes.
> 
> Fixes: a59912a0ee8e ("python: Add support for partial map and partial set updates")
> 
> Signed-off-by: Amitabha Biswas <abiswas@us.ibm.com <mailto:abiswas@us.ibm.com>>
> 
> Your Signed-off-by doesn't match the commit author (your gmail address).  Can you re-submit with the two matching?
>  
> Reported-by: Richard Theis <rtheis@us.ibm.com <mailto:rtheis@us.ibm.com>>
> 
> Other than the Signed-off-by issue, I think this is ready to apply.
> 
> Thanks!
> 
> -- 
> Russell Bryant
diff mbox

Patch

diff --git a/python/ovs/db/idl.py b/python/ovs/db/idl.py
index 187e902..0178050 100644
--- a/python/ovs/db/idl.py
+++ b/python/ovs/db/idl.py
@@ -770,6 +770,7 @@  class Row(object):
         assert self._changes is not None
         assert self._mutations is not None
 
+        column = self._table.columns[column_name]
         datum = self._changes.get(column_name)
         inserts = None
         if '_inserts' in self._mutations.keys():
@@ -784,24 +785,33 @@  class Row(object):
                                          (self.__class__.__name__,
                                           column_name))
                 else:
-                    datum = inserts
-            if column_name in self._data:
+                    datum = data.Datum.from_python(column.type,
+                                                   inserts,
+                                                   _row_to_uuid)
+            elif column_name in self._data:
                 datum = self._data[column_name]
-                try:
+                if column.type.is_set():
+                    dlist = datum.as_list()
                     if inserts is not None:
-                        datum.extend(inserts)
+                        dlist.extend(list(inserts))
                     if removes is not None:
-                        datum = [x for x in datum if x not in removes]
-                except error.Error:
-                    pass
-                try:
+                        removes_datum = data.Datum.from_python(column.type,
+                                                              removes,
+                                                              _row_to_uuid)
+                        removes_list = removes_datum.as_list()
+                        dlist = [x for x in dlist if x not in removes_list]
+                    datum = data.Datum.from_python(column.type, dlist,
+                                                   _row_to_uuid)
+                elif column.type.is_map():
+                    dmap = datum.to_python(_uuid_to_row)
                     if inserts is not None:
-                        datum.merge(inserts)
+                        dmap.update(inserts)
                     if removes is not None:
-                        for key in removes.keys():
-                            del datum[key]
-                except error.Error:
-                    pass
+                        for key in removes:
+                            if key not in (inserts or {}):
+                                del dmap[key]
+                    datum = data.Datum.from_python(column.type, dmap,
+                                                   _row_to_uuid)
             else:
                 if inserts is None:
                     raise AttributeError("%s instance has no attribute '%s'" %
@@ -870,7 +880,7 @@  class Row(object):
             vlog.err("attempting to write bad value to column %s (%s)"
                      % (column_name, e))
             return
-        if column_name in self._data:
+        if self._data and column_name in self._data:
             # Remove existing key/value before updating.
             removes = self._mutations.setdefault('_removes', {})
             column_value = removes.setdefault(column_name, set())
diff --git a/tests/ovsdb-idl.at b/tests/ovsdb-idl.at
index e57a3a4..6326d32 100644
--- a/tests/ovsdb-idl.at
+++ b/tests/ovsdb-idl.at
@@ -1112,15 +1112,18 @@  OVSDB_CHECK_IDL_PY([partial-map idl],
 [['["idltest", {"op":"insert", "table":"simple2",
                 "row":{"name":"myString1","smap":["map",[["key1","value1"],["key2","value2"]]]} }]']
 ],
-  [?simple2:name,smap,imap 'partialmapinsertelement' 'partialmapinsertmultipleelements' 'partialmapdelelements'],
+  [?simple2:name,smap,imap 'partialmapinsertelement' 'partialmapinsertmultipleelements' 'partialmapdelelements' 'partialmapmutatenew'],
 [[000: name=myString1 smap=[(key1 value1) (key2 value2)] imap=[]
 001: commit, status=success
 002: name=String2 smap=[(key1 myList1) (key2 value2)] imap=[(3 myids2)]
 003: commit, status=success
-004: name=String2 smap=[(key1 myList1) (key2 myList2) (key3 myList3)] imap=[(3 myids2)]
+004: name=String2 smap=[(key1 myList1) (key2 myList2) (key3 myList3) (key4 myList4)] imap=[(3 myids2)]
 005: commit, status=success
 006: name=String2 smap=[(key2 myList2)] imap=[(3 myids2)]
-007: done
+007: commit, status=success
+008: name=String2 smap=[(key2 myList2)] imap=[(3 myids2)]
+008: name=String2New smap=[(key1 newList1) (key2 newList2)] imap=[]
+009: done
 ]])
 
 m4_define([OVSDB_CHECK_IDL_PARTIAL_UPDATE_SET_COLUMN],
@@ -1166,7 +1169,7 @@  OVSDB_CHECK_IDL_PY([partial-set idl],
                {"op":"mutate", "table":"simple3", "where":[["_uuid", "==", ["named-uuid", "newrow"]]],
                     "mutations": [["uset", "insert", ["set", [["uuid", "000d2f6a-76af-412f-b59d-e7bcd3e84eff"]]]]]}]']
 ],
-  ['partialrenamesetadd' 'partialduplicateadd' 'partialsetdel' 'partialsetref' 'partialsetoverrideops' 'partialsetmutatenew'],
+  ['partialrenamesetadd' 'partialduplicateadd' 'partialsetdel' 'partialsetref' 'partialsetoverrideops' 'partialsetadddelete' 'partialsetmutatenew'],
 [[000: name=mySet1 uset=[<0> <1>]
 001: commit, status=success
 002: name=String2 uset=[<0> <1> <2>]
@@ -1179,9 +1182,11 @@  OVSDB_CHECK_IDL_PY([partial-set idl],
 009: commit, status=success
 010: name=String2 uset=[<3>]
 011: commit, status=success
-012: name=String2 uset=[<3>]
-012: name=String3 uset=[<4>]
-013: done
+012: name=String2 uset=[<4> <5>]
+013: commit, status=success
+014: name=String2 uset=[<4> <5>]
+014: name=String3 uset=[<6>]
+015: done
 ]])
 
 m4_define([OVSDB_CHECK_IDL_NOTIFY_PY],
diff --git a/tests/test-ovsdb.py b/tests/test-ovsdb.py
index b27ad28..185215a 100644
--- a/tests/test-ovsdb.py
+++ b/tests/test-ovsdb.py
@@ -431,38 +431,63 @@  def idl_set(idl, commands, step):
             l1.k = [l1]
         elif name == 'partialmapinsertelement':
             row = idltest_find_simple2(idl, 'myString1')
+            len_smap = len(getattr(row, 'smap'))
             row.setkey('smap', 'key1', 'myList1')
+            len_imap = len(getattr(row, 'imap'))
             row.setkey('imap', 3, 'myids2')
             row.__setattr__('name', 'String2')
+            assert len(getattr(row, 'smap')) == len_smap
+            assert len(getattr(row, 'imap')) == len_imap + 1
         elif name == 'partialmapinsertmultipleelements':
             row = idltest_find_simple2(idl, 'String2')
+            len_smap = len(getattr(row, 'smap'))
             row.setkey('smap', 'key2', 'myList2')
             row.setkey('smap', 'key3', 'myList3')
+            row.setkey('smap', 'key4', 'myList4')
+            assert len(getattr(row, 'smap')) == len_smap + 2
         elif name == 'partialmapdelelements':
             row = idltest_find_simple2(idl, 'String2')
+            len_smap = len(getattr(row, 'smap'))
             row.delkey('smap', 'key1', 'myList1')
             row.delkey('smap', 'key2', 'wrongvalue')
             row.delkey('smap', 'key3')
+            row.delkey('smap', 'key4')
+            assert len(getattr(row, 'smap')) == len_smap - 3
+        elif name == 'partialmapmutatenew':
+            new_row2 = txn.insert(idl.tables["simple2"])
+            setattr(new_row2, 'name', 'String2New')
+            new_row2.setkey('smap', 'key1', 'newList1')
+            assert len(getattr(new_row2, 'smap')) == 1
+            new_row2.setkey('smap', 'key2', 'newList2')
+            assert len(getattr(new_row2, 'smap')) == 2
         elif name == 'partialrenamesetadd':
             row = idltest_find_simple3(idl, 'mySet1')
+            old_size = len(getattr(row, 'uset', []))
             row.addvalue('uset',
                          uuid.UUID("001e43d2-dd3f-4616-ab6a-83a490bb0991"))
             row.__setattr__('name', 'String2')
+            assert len(getattr(row, 'uset', [])) == old_size + 1
         elif name == 'partialduplicateadd':
             row = idltest_find_simple3(idl, 'String2')
+            old_size = len(getattr(row, 'uset', []))
             row.addvalue('uset',
                          uuid.UUID("0026b3ba-571b-4729-8227-d860a5210ab8"))
             row.addvalue('uset',
                          uuid.UUID("0026b3ba-571b-4729-8227-d860a5210ab8"))
+            assert len(getattr(row, 'uset', [])) == old_size + 1
         elif name == 'partialsetdel':
             row = idltest_find_simple3(idl, 'String2')
+            old_size = len(getattr(row, 'uset', []))
             row.delvalue('uset',
                          uuid.UUID("001e43d2-dd3f-4616-ab6a-83a490bb0991"))
+            assert len(getattr(row, 'uset', [])) == old_size - 1
         elif name == 'partialsetref':
             new_row = txn.insert(idl.tables["simple4"])
             new_row.__setattr__('name', 'test')
             row = idltest_find_simple3(idl, 'String2')
+            old_size = len(getattr(row, 'uref', []))
             row.addvalue('uref', new_row.uuid)
+            assert len(getattr(row, 'uref', [])) == old_size + 1
         elif name == 'partialsetoverrideops':
             row = idltest_find_simple3(idl, 'String2')
             row.addvalue('uset',
@@ -471,12 +496,23 @@  def idl_set(idl, commands, step):
                          uuid.UUID("0026b3ba-571b-4729-8227-d860a5210ab8"))
             row.__setattr__('uset',
                 [uuid.UUID("0026b3ba-571b-4729-8227-d860a5210ab8")])
+            assert len(getattr(row, 'uset', [])) == 1
+        elif name == 'partialsetadddelete':
+            row = idltest_find_simple3(idl, 'String2')
+            row.addvalue('uset',
+                         uuid.UUID('b6272353-af9c-40b7-90fe-32a43e6518a1'))
+            row.addvalue('uset',
+                         uuid.UUID('1d6a71a2-dffb-426e-b2fa-b727091f9901'))
+            row.delvalue('uset',
+                         uuid.UUID('0026b3ba-571b-4729-8227-d860a5210ab8'))
+            assert len(getattr(row, 'uset', [])) == 2
         elif name == 'partialsetmutatenew':
             new_row41 = txn.insert(idl.tables["simple4"])
             new_row41.__setattr__('name', 'new_row41')
             new_row3 = txn.insert(idl.tables["simple3"])
             setattr(new_row3, 'name', 'String3')
             new_row3.addvalue('uset', new_row41.uuid)
+            assert len(getattr(new_row3, 'uset', [])) == 1
         else:
             sys.stderr.write("unknown command %s\n" % name)
             sys.exit(1)