Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Provide a way to retrieve raw extensions data #183

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion src/ZODB/Connection.py
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@
from transaction.interfaces import ISavepointDataManager
from transaction.interfaces import IDataManagerSavepoint
from transaction.interfaces import ISynchronizer
from zope.interface import implementer
from zope.interface import implementer, alsoProvides

import transaction

Expand Down Expand Up @@ -1312,6 +1312,13 @@ def __init__(self, user=u'', description=u'', extension=b''):
description = description.encode('utf-8')
self.description = description

# Provide .extension_bytes if we are given extension in its raw form.
# If not - leave created TransactionMetaData instance without
# .extension_bytes attribute at all.
if isinstance(extension, bytes):
self.extension_bytes = extension
alsoProvides(self, ZODB.interfaces.IStorageTransactionMetaDataRaw)

if not isinstance(extension, dict):
extension = _compat.loads(extension) if extension else {}
self.extension = extension
Expand Down
9 changes: 1 addition & 8 deletions src/ZODB/FileStorage/FileStorage.py
Original file line number Diff line number Diff line change
Expand Up @@ -1988,15 +1988,8 @@ def __next__(self):

if h.status != "u":
pos = tpos + h.headerlen()
e = {}
if h.elen:
try:
e = loads(h.ext)
except:
pass

result = TransactionRecord(h.tid, h.status, h.user, h.descr,
e, pos, tend, self._file, tpos)
h.ext, pos, tend, self._file, tpos)

# Read the (intentionally redundant) transaction length
self._file.seek(tend)
Expand Down
12 changes: 7 additions & 5 deletions src/ZODB/fsrecover.py
Original file line number Diff line number Diff line change
Expand Up @@ -143,12 +143,14 @@ def read_txn_header(f, pos, file_size, outp, ltid):
pos = tpos+(23+ul+dl+el)
user = f.read(ul)
description = f.read(dl)
if el:
try: e = loads(f.read(el))
except: e = {}
else: e = {}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jimfulton says:

The original code is flawed. IDK (IDR) why we have a bare except.

To any tests fail wo it?

I don't know whether any test fails without it, but since it is FS recover to me it looks logical that if e.g. extension is corrupt, it is better to drop it, but still try to recover rest of transaction metadata.

In any way I would preserve current semantic within this patch and try to do one step at a time but well.

BTW this code is from you from 2001:

2d0d8c08#diff-b18ad2268d655f3885a3b21941d8f8f9R209

ext = f.read(el)
# reset extension to {} if it cannot be unpickled
try:
loads(ext)
except:
ext = b''

result = TransactionRecord(tid, status, user, description, e, pos, tend,
result = TransactionRecord(tid, status, user, description, ext, pos, tend,
f, tpos)
pos = tend

Expand Down
18 changes: 18 additions & 0 deletions src/ZODB/interfaces.py
Original file line number Diff line number Diff line change
Expand Up @@ -540,6 +540,9 @@ class IStorageTransactionMetaData(Interface):

Note that unlike transaction.interfaces.ITransaction, the ``user``
and ``description`` attributes are bytes, not text.

An instance of IStorageTransactionMetaData may also optionally provide
IStorageTransactionMetaDataRaw with raw form of transaction meta data.
"""
user = Attribute("Bytes transaction user")
description = Attribute("Bytes transaction Description")
Expand Down Expand Up @@ -567,6 +570,21 @@ def data(ob):
See set_data.
"""

class IStorageTransactionMetaDataRaw(IStorageTransactionMetaData):
"""Provide storage transaction meta data in raw form.

IStorageTransactionMetaDataRaw is additional interface which may
be optionally provided by an instance of IStorageTransactionMetaData.
If so, it provides information about raw form of transaction's meta data as
saved on storage.

NOTE as IStorageTransactionMetaData already provides ``user`` and
``description`` as raw bytes, not text, IStorageTransactionMetaDataRaw
amends it only for ``extension`` part.
"""

extension_bytes = Attribute("Transaction's extension data as raw bytes.")


class IStorage(Interface):
"""A storage is responsible for storing and retrieving data of objects.
Expand Down
25 changes: 23 additions & 2 deletions src/ZODB/tests/IteratorStorage.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@
from ZODB.tests.MinPO import MinPO
from ZODB.tests.StorageTestBase import zodb_pickle, zodb_unpickle
from ZODB.utils import U64, p64, load_current
from ZODB.interfaces import IStorageTransactionMetaDataRaw
from ZODB._compat import loads

import ZODB.blob

Expand Down Expand Up @@ -49,6 +51,10 @@ def iter_verify(self, txniter, revids, val0):

class IteratorStorage(IteratorCompare):

# override this in tests for storages that have support for
# .extension_bytes in iterated transactions.
supports_extension_bytes = False

def checkSimpleIteration(self):
# Store a bunch of revisions of a single object
self._oid = oid = self._storage.new_oid()
Expand Down Expand Up @@ -86,11 +92,22 @@ def checkUndoZombie(self):

def checkTransactionExtensionFromIterator(self):
oid = self._storage.new_oid()
revid = self._dostore(oid, data=MinPO(1))
ext_dict = {"hello": "world"}
revid = self._dostore(oid, data=MinPO(1), extension=ext_dict)
iter = self._storage.iterator()
count = 0
for txn in iter:
self.assertEqual(txn.extension, {})
self.assertEqual(txn.extension, ext_dict)
# missing: not None - something unique - not to allow
# implementations to erroneously return None in .extension_bytes .
missing = object()
ext_bytes = getattr(txn, "extension_bytes", missing)
self.assertEqual(self.supports_extension_bytes, ext_bytes is not missing)
self.assertEqual(self.supports_extension_bytes,
IStorageTransactionMetaDataRaw.providedBy(txn))
if ext_bytes is not missing:
ext_dict_fromraw = loads(ext_bytes)
self.assertEqual(ext_dict_fromraw, ext_dict)
count +=1
self.assertEqual(count, 1)

Expand Down Expand Up @@ -207,6 +224,7 @@ class IteratorDeepCompare(object):

def compare(self, storage1, storage2):
eq = self.assertEqual
missing = object()
iter1 = storage1.iterator()
iter2 = storage2.iterator()
for txn1, txn2 in zip(iter1, iter2):
Expand All @@ -215,6 +233,9 @@ def compare(self, storage1, storage2):
eq(txn1.user, txn2.user)
eq(txn1.description, txn2.description)
eq(txn1.extension, txn2.extension)
rawext1 = getattr(txn1, "extension_bytes", missing)
rawext2 = getattr(txn2, "extension_bytes", missing)
eq(rawext1, rawext2)
itxn1 = iter(txn1)
itxn2 = iter(txn2)
for rec1, rec2 in zip(itxn1, itxn2):
Expand Down
4 changes: 3 additions & 1 deletion src/ZODB/tests/StorageTestBase.py
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,7 @@ def tearDown(self):
ZODB.tests.util.TestCase.tearDown(self)

def _dostore(self, oid=None, revid=None, data=None,
already_pickled=0, user=None, description=None):
already_pickled=0, user=None, description=None, extension=None):
"""Do a complete storage transaction. The defaults are:

- oid=None, ask the storage for a new oid
Expand All @@ -149,6 +149,8 @@ def _dostore(self, oid=None, revid=None, data=None,
t.user = user
if description is not None:
t.description = description
if extension is not None:
t.extension = extension
try:
self._storage.tpc_begin(t)
# Store an object
Expand Down
11 changes: 10 additions & 1 deletion src/ZODB/tests/testFileStorage.py
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,8 @@ class FileStorageTests(
ReadOnlyStorage.ReadOnlyStorage
):

supports_extension_bytes = True

def open(self, **kwargs):
self._storage = ZODB.FileStorage.FileStorage('FileStorageTests.fs',
**kwargs)
Expand All @@ -77,7 +79,14 @@ def checkLongMetadata(self):
except POSException.StorageError:
pass
else:
self.fail("expect long user field to raise error")
self.fail("expect long description field to raise error")
try:
self._dostore(extension={s: 1})
except POSException.StorageError:
pass
else:
self.fail("expect long extension field to raise error")

Copy link
Contributor Author

@navytux navytux Jan 14, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jimfulton says:

What is this testing?

There is already-existing FileStorage test which checks if a commit with long metadata fails:

https://github.com/zopefoundation/ZODB/blob/de1f24ca/src/ZODB/tests/testFileStorage.py#L67

Since the commits in FileStorage tests are performed via _dostore() and my patch extends _dostore() to also accept extension, I natrally extend this test as well. (I grepped all places where _dostore was used and found it this way).


def check_use_fsIndex(self):

Expand Down