Skip to content

Commit

Permalink
Remove potentially-buggy positive_id (#384)
Browse files Browse the repository at this point in the history
positive_id was added in 2004 in f7b96ae (ZODB.utils grows a new
function positive_id(), which returns the id() of an object as a
non-negative integer ...) to workaround change in behaviour Python2.3 ->
2.4 for `'%x' % <negative-number>`. And nowdays `positive_id` is used
only in one place in Connection.__repr__ to mimic approximately what
Python already gives for a default __repr__ out of the box.

On https://github.com/zopefoundation/ZODB/actions/runs/5067242751/jobs/9098062772
we recently hit

    ZODB.POSException.InvalidObjectReference: <unprintable InvalidObjectReference object>

for the following code:

    if self._jar.get_connection(database_name) is not obj._p_jar:
        raise InvalidObjectReference(
            "Attempt to store a reference to an object from "
            "a separate connection to the same database or "
            "multidatabase", self._jar, obj,
        )

    ( https://github.com/zopefoundation/ZODB/blob/5.8.0-24-g8a7e3162d/src/ZODB/serialize.py#L366-L371 )

for which the "unprintable ..." part is produced by `Lib/traceback.py`
who tries to do `str(ex)` and returns `unprintable ...` if that str
failed:

    https://github.com/python/cpython/blob/v3.8.16-18-g9f89c471fb1/Lib/traceback.py#L153-L157

Since one of the reasons for that failure might have been a bit
non-trivial code and assert in positive_id, let's remove it and rely on
builtin python behaviour for Connection repr.

repr for Connection:

before this patch:

    <Connection at 0x7fef09989d10>

after this patch:

    <ZODB.Connection.Connection object at 0x7fef09989d10>

/reviewed-by @dataflake, @d-maurer 
/reviewed-at #384
  • Loading branch information
navytux committed May 26, 2023
1 parent 8a7e316 commit 12d1a05
Show file tree
Hide file tree
Showing 5 changed files with 9 additions and 42 deletions.
10 changes: 0 additions & 10 deletions src/ZODB/Connection.py
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,6 @@
from ZODB.serialize import ObjectWriter
from ZODB.utils import oid_repr
from ZODB.utils import p64
from ZODB.utils import positive_id
from ZODB.utils import u64
from ZODB.utils import z64

Expand Down Expand Up @@ -947,15 +946,6 @@ def _release_resources(self):
c._cache = PickleCache(self, 0, 0)
c.close(False)

##########################################################################
# Python protocol

def __repr__(self):
return '<Connection at %08x>' % (positive_id(self),)

# Python protocol
##########################################################################

##########################################################################
# DEPRECATION candidates

Expand Down
8 changes: 4 additions & 4 deletions src/ZODB/cross-database-references.rst
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ It isn't valid to create references outside a multi database:
...
InvalidObjectReference:
('Attempt to store an object from a foreign database connection',
<Connection at ...>,
<ZODB.Connection.Connection object at ...>,
<ZODB.tests.testcrossdatabasereferences.MyClass...>)

>>> tm.abort()
Expand Down Expand Up @@ -92,7 +92,7 @@ reachable from multiple databases:
InvalidObjectReference:
("A new object is reachable from multiple databases. Won't try to
guess which one was correct!",
<Connection at ...>,
<ZODB.Connection.Connection object at ...>,
<ZODB.tests.testcrossdatabasereferences.MyClass...>)

>>> tm.abort()
Expand Down Expand Up @@ -120,7 +120,7 @@ This doesn't work with a savepoint:
InvalidObjectReference:
("A new object is reachable from multiple databases. Won't try to guess
which one was correct!",
<Connection at ...>,
<ZODB.Connection.Connection object at ...>,
<ZODB.tests.testcrossdatabasereferences.MyClass...>)

>>> tm.abort()
Expand Down Expand Up @@ -167,7 +167,7 @@ the other way around.
...
InvalidObjectReference:
("Database '2' doesn't allow implicit cross-database references",
<Connection at ...>,
<ZODB.Connection.Connection object at ...>,
{'x': {}})

>>> transaction.abort()
Expand Down
6 changes: 3 additions & 3 deletions src/ZODB/tests/multidb.txt
Original file line number Diff line number Diff line change
Expand Up @@ -86,20 +86,20 @@ You can (still) get a connection to a database this way:
>>> tm = transaction.TransactionManager()
>>> cn = db.open(transaction_manager=tm)
>>> cn # doctest: +ELLIPSIS
<Connection at ...>
<ZODB.Connection.Connection object at ...>

This is the only connection in this collection right now:

>>> cn.connections # doctest: +ELLIPSIS
{'root': <Connection at ...>}
{'root': <ZODB.Connection.Connection object at ...>}

Getting a connection to a different database from an existing connection in the
same database collection (this enables 'connection binding' within a given
thread/transaction/context ...):

>>> cn2 = cn.get_connection('notroot')
>>> cn2 # doctest: +ELLIPSIS
<Connection at ...>
<ZODB.Connection.Connection object at ...>

The second connection gets the same transaction manager as the first:

Expand Down
4 changes: 2 additions & 2 deletions src/ZODB/tests/testcrossdatabasereferences.py
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ def test_must_use_consistent_connections():
InvalidObjectReference:
('Attempt to store a reference to an object from a separate connection to
the same database or multidatabase',
<Connection at ...>,
<ZODB.Connection.Connection object at ...>,
<ZODB.tests.testcrossdatabasereferences.MyClass object at ...>)
>>> tm.abort()
Expand All @@ -80,7 +80,7 @@ def test_must_use_consistent_connections():
InvalidObjectReference:
('Attempt to store a reference to an object from a separate connection
to the same database or multidatabase',
<Connection at ...>,
<ZODB.Connection.Connection object at ...>,
<ZODB.tests.testcrossdatabasereferences.MyClass object at ...>)
>>> tm.abort()
Expand Down
23 changes: 0 additions & 23 deletions src/ZODB/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,6 @@
'oid_repr',
'serial_repr',
'tid_repr',
'positive_id',
'readable_tid_repr',
'get_pickle_metadata',
'locked',
Expand Down Expand Up @@ -184,28 +183,6 @@ def readable_tid_repr(tid):
result = "%s %s" % (result, TimeStamp(tid))
return result

# Addresses can "look negative" on some boxes, some of the time. If you
# feed a "negative address" to an %x format, Python 2.3 displays it as
# unsigned, but produces a FutureWarning, because Python 2.4 will display
# it as signed. So when you want to prodce an address, use positive_id() to
# obtain it.
# _ADDRESS_MASK is 2**(number_of_bits_in_a_native_pointer). Adding this to
# a negative address gives a positive int with the same hex representation as
# the significant bits in the original.


_ADDRESS_MASK = 256 ** struct.calcsize('P')


def positive_id(obj):
"""Return id(obj) as a non-negative integer."""

result = id(obj)
if result < 0:
result += _ADDRESS_MASK
assert result > 0
return result

# Given a ZODB pickle, return pair of strings (module_name, class_name).
# Do this without importing the module or class object.
# See ZODB/serialize.py's module docstring for the only docs that exist about
Expand Down

0 comments on commit 12d1a05

Please sign in to comment.