From b8c0a0706f54c3e58eb26ea9f3403afcf1df4245 Mon Sep 17 00:00:00 2001 From: James Munro Date: Fri, 22 Mar 2024 17:06:52 +0000 Subject: [PATCH] msgpack>0.5, check instead of assert, test msgpack padding, typos and style fixes --- environment_unix.yml | 3 ++- python/arcticdb/_msgpack_compat.py | 5 ++--- python/arcticdb/version_store/_normalization.py | 5 ++--- python/tests/unit/arcticdb/test_msgpack_compact.py | 11 +++++++++-- .../unit/arcticdb/version_store/test_normalization.py | 2 +- setup.cfg | 3 ++- 6 files changed, 18 insertions(+), 11 deletions(-) diff --git a/environment_unix.yml b/environment_unix.yml index 7e9ab9d3fe0..ae770c3182d 100644 --- a/environment_unix.yml +++ b/environment_unix.yml @@ -66,7 +66,8 @@ dependencies: - werkzeug - moto - mock - - msgpack-python + # msgpack 0.5.0 is required for strict_types argument, needed for correct pickling fallback + - msgpack-python >= 0.5.0 # Pinned to avoid test disruption (in phase with the pin in setup.cfg) # See: https://github.com/man-group/ArcticDB/pull/291 - hypothesis < 6.73 diff --git a/python/arcticdb/_msgpack_compat.py b/python/arcticdb/_msgpack_compat.py index 5773ca401fb..9201328b97a 100644 --- a/python/arcticdb/_msgpack_compat.py +++ b/python/arcticdb/_msgpack_compat.py @@ -5,7 +5,7 @@ This module implements a backwards compatible version of msgpack functions. """ import msgpack -from arcticdb.log import version as log +from arcticdb.preconditions import check from arcticdb.exceptions import ArcticNativeException ExtType = msgpack.ExtType @@ -20,7 +20,6 @@ def _check_valid_msgpack(): packer_module in ("msgpack._packer", "msgpack.fallback", "msgpack._cmsgpack") ): return - log.info("Unsupported msgpack variant, got: {}, {}".format(pack_module, packer_module)) raise ArcticNativeException("Unsupported msgpack variant, got: {}, {}".format(pack_module, packer_module)) @@ -44,7 +43,7 @@ def padded_packb(obj, **kwargs): nbytes = packer.getbuffer().nbytes pad = -nbytes % 8 # next multiple of 8 bytes [packer.pack(None) for _ in range(pad)] # None is packed as single byte b`\xc0` - assert packer.getbuffer().nbytes % 8 == 0 + check(packer.getbuffer().nbytes % 8 == 0, 'Error in ArcticDB padded_packb. Padding failed. nbytes={}', packer.getbuffer().nbytes) return packer.bytes(), nbytes diff --git a/python/arcticdb/version_store/_normalization.py b/python/arcticdb/version_store/_normalization.py index fd620f91d44..f9caadbe6f3 100644 --- a/python/arcticdb/version_store/_normalization.py +++ b/python/arcticdb/version_store/_normalization.py @@ -1225,10 +1225,9 @@ def normalize_metadata(d): packed = _msgpack_metadata._msgpack_packb(d) size = len(packed) if size > _MAX_USER_DEFINED_META: - raise ArcticDbNotYetImplemented("User defined metadata cannot exceed {}B".format(_MAX_USER_DEFINED_META)) + raise ArcticDbNotYetImplemented(f'User defined metadata cannot exceed {_MAX_USER_DEFINED_META}B') if size > _WARN_USER_DEFINED_META: - log.warn('User defined metadata is above warning size ({0}B), metadata cannot exceed {1}B. Current size: {2}B.'\ - .format(_WARN_USER_DEFINED_META, _MAX_USER_DEFINED_META, size)) + log.warn(f'User defined metadata is above warning size ({_WARN_USER_DEFINED_META}B), metadata cannot exceed {_MAX_USER_DEFINED_META}B. Current size: {size}B.') udm = UserDefinedMetadata() udm.inline_payload = packed diff --git a/python/tests/unit/arcticdb/test_msgpack_compact.py b/python/tests/unit/arcticdb/test_msgpack_compact.py index a05df23227b..ba93001809c 100644 --- a/python/tests/unit/arcticdb/test_msgpack_compact.py +++ b/python/tests/unit/arcticdb/test_msgpack_compact.py @@ -19,7 +19,7 @@ def test_packb_raises_on_tuple(): def test_padded_packb_small(): packed, nbytes = padded_packb(1) assert len(packed) % 8 == 0 - assert len(packed) > nbytes + assert len(packed) >= nbytes assert len(packed) - nbytes < 8 assert packed[:nbytes] == msgpack.packb(1, use_bin_type=True, strict_types=True) @@ -34,7 +34,7 @@ def test_padded_packb_list(): def test_padded_packb_string(): - aas = 'A' * 1_000_005 # not dividisble by 8 + aas = 'A' * 1_000_005 # not divisible by 8 packed, nbytes = padded_packb(aas) assert len(packed) % 8 == 0 assert len(packed) >= nbytes @@ -42,6 +42,13 @@ def test_padded_packb_string(): assert packed[:nbytes] == msgpack.packb(aas, use_bin_type=True, strict_types=True) +def test_padded_packb_padding(): + # padded_packb behaviour relies on 1 byte for None assumption from msgpack spec + packed, nbytes = padded_packb(None) + assert nbytes == 1 # 1 byte of content + assert packed == b'\xc0\xc0\xc0\xc0\xc0\xc0\xc0\xc0' # 7 bytes of padding, 8 total + + def test_unpackb(): # serializes without strict_types packed = msgpack.packb({(1, 2): "a"}) diff --git a/python/tests/unit/arcticdb/version_store/test_normalization.py b/python/tests/unit/arcticdb/version_store/test_normalization.py index a64c3e58017..7ffe0ea5746 100644 --- a/python/tests/unit/arcticdb/version_store/test_normalization.py +++ b/python/tests/unit/arcticdb/version_store/test_normalization.py @@ -369,7 +369,7 @@ def test_ndarray_arbitrary_shape(): def test_dict_with_tuples(): - # This has to be pickeld because msgpack doesn't differentiate between tuples and lists + # This has to be pickled because msgpack doesn't differentiate between tuples and lists data = {(1, 2): [1, 24, 55]} norm = CompositeNormalizer(use_norm_failure_handler_known_types=False, fallback_normalizer=test_msgpack_normalizer) df, norm_meta = norm.normalize(data) diff --git a/setup.cfg b/setup.cfg index 8fbe8e4c37a..b9639790584 100644 --- a/setup.cfg +++ b/setup.cfg @@ -39,7 +39,8 @@ install_requires = attrs dataclasses ; python_version < '3.7' protobuf >=3.5.0.post1, < 5 # Per https://github.com/grpc/grpc/blob/v1.45.3/requirements.txt - msgpack >=0.5.0 # 0.5.0 is needed for strict_types argument, which is required for our pickling behaviour + msgpack >=0.5.0 # msgpack 0.5.0 is required for strict_types argument, needed for correct pickling fallback + pyyaml packaging