| Message ID | 20250606214915.1240248-1-twilson@redhat.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Ilya Maximets |
| Headers | show |
| Series | [ovs-dev] python: Add uuid/convert references to uuid for Row.__str__. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/cirrus-robot | success | cirrus build: passed |
| ovsrobot/github-robot-_Build_and_Test | fail | github build: failed |
On 6/6/25 11:49 PM, Terry Wilson via dev wrote: > Row stringification happens a lot in client logs and it is far > more useful to have the logged Row's uuid printed. This also > adds converting referenced Row objects, and references within > set and map columns to UUIDs. > > Signed-off-by: Terry Wilson <twilson@redhat.com> > --- > python/ovs/db/idl.py | 32 ++++++++++++++++++++++++++++---- > tests/test-ovsdb.py | 3 +++ > 2 files changed, 31 insertions(+), 4 deletions(-) Hi, Terry. Thanks for the patch! This looks like a goos improvement indeed, printing out pointers instead of map content is indeed not user friendly. I think, there is some part missing though, I tried to print out the _Server database row and got this: Database(uuid=<1> cid=[] connected=true index=[] leader=true model=standalone name=_Server schema=[{"cksum":"3009684573 744""name":"_Server""tables":{"Database":{"columns":{"cid":{"type":{"key":"uuid""min":0}}"connected":{"type":"boolean"}"index":{"type":{"key":"integer""min":0}}"leader":{"type":"boolean"}"model":{"type":{"key":{"enum":["set"["clustered""relay""standalone"]]"type":"string"}}}"name":{"type":"string"}"schema":{"type":{"key":"string""min":0}}"sid":{"type":{"key":"uuid""min":0}}}}}"version":"1.2.0"}] sid=[]) It all looks good, but there is no delimiter between map elements, e.g. "cksum":"3009684573 744""name":"_Server". I'd expect a space or a comma between the entries. Can this be done? A couple nits below. > > diff --git a/python/ovs/db/idl.py b/python/ovs/db/idl.py > index f01e21b5e..4188d1842 100644 > --- a/python/ovs/db/idl.py > +++ b/python/ovs/db/idl.py > @@ -1229,6 +1229,29 @@ def _row_to_uuid(value): > return value > > > +def _rows_to_uuid_str(value): > + if isinstance(value, collections.abc.Mapping): > + try: > + k, v = next(iter(value.items())) > + # Pass through early without iterating if not Rows Period at the end of a comment. > + if isinstance(k, Row) or isinstance(v, Row): > + return {str(_row_to_uuid(x)): str(_row_to_uuid(y)) > + for x, y in value.items()} > + except StopIteration: > + # Empty, return default ditto. > + pass > + elif (isinstance(value, collections.abc.Iterable) > + and not isinstance(value, str)): > + try: > + # Pass through early without iterating if not Rows ditto. > + if value and isinstance(value[0], Row): > + return type(value)(str(_row_to_uuid(x)) for x in value) > + except TypeError: > + # Weird Iterable, pass through ditto. > + pass > + return str(_row_to_uuid(value)) > + > + > @functools.total_ordering > class Row(object): > """A row within an IDL. > @@ -1334,11 +1357,12 @@ class Row(object): > return int(self.__dict__['uuid']) > > def __str__(self): > - return "{table}({data})".format( > + return "{table}(uuid={uuid}, {data})".format( > table=self._table.name, > - data=", ".join("{col}={val}".format(col=c, val=getattr(self, c)) > - for c in sorted(self._table.columns) > - if hasattr(self, c))) > + uuid=self.uuid, > + data=", ".join("{col}={val}".format( > + col=c, val=_rows_to_uuid_str(getattr(self, c))) > + for c in sorted(self._table.columns) if hasattr(self, c))) > > def _uuid_to_row(self, atom, base): > if base.ref_table: > diff --git a/tests/test-ovsdb.py b/tests/test-ovsdb.py > index b690b4f07..32e1894e1 100644 > --- a/tests/test-ovsdb.py > +++ b/tests/test-ovsdb.py > @@ -214,6 +214,9 @@ def do_parse_schema(schema_string): > > > def get_simple_printable_row_string(row, columns): > + # NOTE(twilson):This turns out to be a particularly good place to test that > + # Row object stringification doesn't crash on a large variety of row types > + assert(str(row)) flake8 complains that this should be 'assert str(row)' instead. It would be nice if we could just use the result here instead of table-specific printing functions, but that will require a large amount of test changes, so it may be good to do sometime later. Best regards, Ilya Maximets.
On 6/18/25 4:21 PM, Ilya Maximets wrote: > On 6/6/25 11:49 PM, Terry Wilson via dev wrote: >> Row stringification happens a lot in client logs and it is far >> more useful to have the logged Row's uuid printed. This also >> adds converting referenced Row objects, and references within >> set and map columns to UUIDs. >> >> Signed-off-by: Terry Wilson <twilson@redhat.com> >> --- >> python/ovs/db/idl.py | 32 ++++++++++++++++++++++++++++---- >> tests/test-ovsdb.py | 3 +++ >> 2 files changed, 31 insertions(+), 4 deletions(-) > > Hi, Terry. Thanks for the patch! > > This looks like a goos improvement indeed, printing out pointers > instead of map content is indeed not user friendly. > > I think, there is some part missing though, I tried to print out > the _Server database row and got this: > > Database(uuid=<1> cid=[] connected=true index=[] leader=true model=standalone name=_Server schema=[{"cksum":"3009684573 744""name":"_Server""tables":{"Database":{"columns":{"cid":{"type":{"key":"uuid""min":0}}"connected":{"type":"boolean"}"index":{"type":{"key":"integer""min":0}}"leader":{"type":"boolean"}"model":{"type":{"key":{"enum":["set"["clustered""relay""standalone"]]"type":"string"}}}"name":{"type":"string"}"schema":{"type":{"key":"string""min":0}}"sid":{"type":{"key":"uuid""min":0}}}}}"version":"1.2.0"}] sid=[]) > > It all looks good, but there is no delimiter between map elements, > e.g. "cksum":"3009684573 744""name":"_Server". I'd expect a space > or a comma between the entries. Can this be done? Uff, sorry, I messed up while printing out things and passed the text through regexes in the get_simple_printable_row_string() that's why commas disappeared. So, ignore this part. > > A couple nits below. > >> >> diff --git a/python/ovs/db/idl.py b/python/ovs/db/idl.py >> index f01e21b5e..4188d1842 100644 >> --- a/python/ovs/db/idl.py >> +++ b/python/ovs/db/idl.py >> @@ -1229,6 +1229,29 @@ def _row_to_uuid(value): >> return value >> >> >> +def _rows_to_uuid_str(value): >> + if isinstance(value, collections.abc.Mapping): >> + try: >> + k, v = next(iter(value.items())) >> + # Pass through early without iterating if not Rows > > Period at the end of a comment. > >> + if isinstance(k, Row) or isinstance(v, Row): >> + return {str(_row_to_uuid(x)): str(_row_to_uuid(y)) >> + for x, y in value.items()} >> + except StopIteration: >> + # Empty, return default > > ditto. > >> + pass >> + elif (isinstance(value, collections.abc.Iterable) >> + and not isinstance(value, str)): >> + try: >> + # Pass through early without iterating if not Rows > > ditto. > >> + if value and isinstance(value[0], Row): >> + return type(value)(str(_row_to_uuid(x)) for x in value) >> + except TypeError: >> + # Weird Iterable, pass through > > ditto. > >> + pass >> + return str(_row_to_uuid(value)) >> + >> + >> @functools.total_ordering >> class Row(object): >> """A row within an IDL. >> @@ -1334,11 +1357,12 @@ class Row(object): >> return int(self.__dict__['uuid']) >> >> def __str__(self): >> - return "{table}({data})".format( >> + return "{table}(uuid={uuid}, {data})".format( >> table=self._table.name, >> - data=", ".join("{col}={val}".format(col=c, val=getattr(self, c)) >> - for c in sorted(self._table.columns) >> - if hasattr(self, c))) >> + uuid=self.uuid, >> + data=", ".join("{col}={val}".format( >> + col=c, val=_rows_to_uuid_str(getattr(self, c))) >> + for c in sorted(self._table.columns) if hasattr(self, c))) >> >> def _uuid_to_row(self, atom, base): >> if base.ref_table: >> diff --git a/tests/test-ovsdb.py b/tests/test-ovsdb.py >> index b690b4f07..32e1894e1 100644 >> --- a/tests/test-ovsdb.py >> +++ b/tests/test-ovsdb.py >> @@ -214,6 +214,9 @@ def do_parse_schema(schema_string): >> >> >> def get_simple_printable_row_string(row, columns): >> + # NOTE(twilson):This turns out to be a particularly good place to test that >> + # Row object stringification doesn't crash on a large variety of row types >> + assert(str(row)) > > flake8 complains that this should be 'assert str(row)' instead. > > It would be nice if we could just use the result here instead of > table-specific printing functions, but that will require a large > amount of test changes, so it may be good to do sometime later. > > Best regards, Ilya Maximets.
diff --git a/python/ovs/db/idl.py b/python/ovs/db/idl.py index f01e21b5e..4188d1842 100644 --- a/python/ovs/db/idl.py +++ b/python/ovs/db/idl.py @@ -1229,6 +1229,29 @@ def _row_to_uuid(value): return value +def _rows_to_uuid_str(value): + if isinstance(value, collections.abc.Mapping): + try: + k, v = next(iter(value.items())) + # Pass through early without iterating if not Rows + if isinstance(k, Row) or isinstance(v, Row): + return {str(_row_to_uuid(x)): str(_row_to_uuid(y)) + for x, y in value.items()} + except StopIteration: + # Empty, return default + pass + elif (isinstance(value, collections.abc.Iterable) + and not isinstance(value, str)): + try: + # Pass through early without iterating if not Rows + if value and isinstance(value[0], Row): + return type(value)(str(_row_to_uuid(x)) for x in value) + except TypeError: + # Weird Iterable, pass through + pass + return str(_row_to_uuid(value)) + + @functools.total_ordering class Row(object): """A row within an IDL. @@ -1334,11 +1357,12 @@ class Row(object): return int(self.__dict__['uuid']) def __str__(self): - return "{table}({data})".format( + return "{table}(uuid={uuid}, {data})".format( table=self._table.name, - data=", ".join("{col}={val}".format(col=c, val=getattr(self, c)) - for c in sorted(self._table.columns) - if hasattr(self, c))) + uuid=self.uuid, + data=", ".join("{col}={val}".format( + col=c, val=_rows_to_uuid_str(getattr(self, c))) + for c in sorted(self._table.columns) if hasattr(self, c))) def _uuid_to_row(self, atom, base): if base.ref_table: diff --git a/tests/test-ovsdb.py b/tests/test-ovsdb.py index b690b4f07..32e1894e1 100644 --- a/tests/test-ovsdb.py +++ b/tests/test-ovsdb.py @@ -214,6 +214,9 @@ def do_parse_schema(schema_string): def get_simple_printable_row_string(row, columns): + # NOTE(twilson):This turns out to be a particularly good place to test that + # Row object stringification doesn't crash on a large variety of row types + assert(str(row)) s = "" for column in columns: if hasattr(row, column) and not (type(getattr(row, column))
Row stringification happens a lot in client logs and it is far more useful to have the logged Row's uuid printed. This also adds converting referenced Row objects, and references within set and map columns to UUIDs. Signed-off-by: Terry Wilson <twilson@redhat.com> --- python/ovs/db/idl.py | 32 ++++++++++++++++++++++++++++---- tests/test-ovsdb.py | 3 +++ 2 files changed, 31 insertions(+), 4 deletions(-)