From 847327b515ec51c0de3c5b59fba921607c35bb16 Mon Sep 17 00:00:00 2001 From: Alexander Turenko Date: Wed, 26 Aug 2020 07:03:14 +0300 Subject: [PATCH 1/6] refactoring: extract msgpack unpacker tuning code We will need a bit more logic in unpacker configuration code to support msgpack-1.0.0, so it is convenient to factor out it from actual creation of the unpacker instance. No behaviour is changed here. Part of #155 --- tarantool/response.py | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/tarantool/response.py b/tarantool/response.py index 9516cd39..7006cb11 100644 --- a/tarantool/response.py +++ b/tarantool/response.py @@ -50,15 +50,21 @@ def __init__(self, conn, response): # created in the __new__(). # super(Response, self).__init__() + unpacker_kwargs = dict() + + # Decode msgpack arrays into Python lists (not tuples). + unpacker_kwargs['use_list'] = True + + # Use raw=False instead of encoding='utf-8'. if msgpack.version >= (0, 5, 2) and conn.encoding == 'utf-8': # Get rid of the following warning. # > PendingDeprecationWarning: encoding is deprecated, # > Use raw=False instead. - unpacker = msgpack.Unpacker(use_list=True, raw=False) + unpacker_kwargs['raw'] = False elif conn.encoding is not None: - unpacker = msgpack.Unpacker(use_list=True, encoding=conn.encoding) - else: - unpacker = msgpack.Unpacker(use_list=True) + unpacker_kwargs['encoding'] = conn.encoding + + unpacker = msgpack.Unpacker(**unpacker_kwargs) unpacker.feed(response) header = unpacker.unpack() From 30d54f0fee1d09517b0546753f94484ebdc22cbc Mon Sep 17 00:00:00 2001 From: Alexander Turenko Date: Wed, 26 Aug 2020 07:52:27 +0300 Subject: [PATCH 2/6] Support msgpack-1.0.0 (unpacking part) msgpack-1.0.0 changes default value of 'raw' and 'strict_map_key' options. We should handle different versions of the library and provide the same behaviour across them. This version of the msgpack library also drops support of 'encoding' option. We already use raw=True/False msgpack option to implement encoding=None/'utf-8' connector option, so the only change we should do about this is to explicitly forbid other encoding values. Unlikely using of text encodings other than UTF-8 is popular. Part of #155 --- tarantool/connection.py | 7 +++++++ tarantool/response.py | 17 +++++++++++++++++ 2 files changed, 24 insertions(+) diff --git a/tarantool/connection.py b/tarantool/connection.py index 6f330896..325a664f 100644 --- a/tarantool/connection.py +++ b/tarantool/connection.py @@ -52,6 +52,7 @@ NetworkError, DatabaseError, InterfaceError, + ConfigurationError, SchemaError, NetworkWarning, SchemaReloadException, @@ -79,6 +80,7 @@ class Connection(object): Error = tarantool.error DatabaseError = DatabaseError InterfaceError = InterfaceError + ConfigurationError = ConfigurationError SchemaError = SchemaError NetworkError = NetworkError @@ -101,6 +103,11 @@ def __init__(self, host, port, creates network connection. if False than you have to call connect() manualy. ''' + + if msgpack.version >= (1, 0, 0) and encoding not in (None, 'utf-8'): + raise ConfigurationError("Only None and 'utf-8' encoding option " + + "values are supported with msgpack>=1.0.0") + if os.name == 'nt': libc = ctypes.WinDLL( ctypes.util.find_library('Ws2_32'), use_last_error=True diff --git a/tarantool/response.py b/tarantool/response.py index 7006cb11..d8b479c1 100644 --- a/tarantool/response.py +++ b/tarantool/response.py @@ -64,6 +64,23 @@ def __init__(self, conn, response): elif conn.encoding is not None: unpacker_kwargs['encoding'] = conn.encoding + # raw=False is default since msgpack-1.0.0. + # + # The option decodes mp_str to bytes, not a Unicode + # string (when True). + if msgpack.version >= (1, 0, 0) and conn.encoding is None: + unpacker_kwargs['raw'] = True + + # encoding option is not supported since msgpack-1.0.0, + # but it is handled in the Connection constructor. + assert(msgpack.version < (1, 0, 0) or conn.encoding in (None, 'utf-8')) + + # strict_map_key=True is default since msgpack-1.0.0. + # + # The option forbids non-string keys in a map (when True). + if msgpack.version >= (1, 0, 0): + unpacker_kwargs['strict_map_key'] = False + unpacker = msgpack.Unpacker(**unpacker_kwargs) unpacker.feed(response) From 02998f5246b013dcc16a2a29a474745921af4ce1 Mon Sep 17 00:00:00 2001 From: Alexander Turenko Date: Wed, 26 Aug 2020 07:58:00 +0300 Subject: [PATCH 3/6] refactoring: wrap msgpack.dumps() into own method We'll need to tune msgpack options in order to support msgpack-1.0.0 and it is convenient to do so in one place. Aside of this, reusing of an msgpack.Packer() instance looks good for performance matters, however I guess that an actual difference is negligible. No behaviour is changed. Part of #155 --- tarantool/request.py | 83 +++++++++++++++++++++++--------------------- 1 file changed, 44 insertions(+), 39 deletions(-) diff --git a/tarantool/request.py b/tarantool/request.py index 2e45c8d9..55cbeecc 100644 --- a/tarantool/request.py +++ b/tarantool/request.py @@ -65,6 +65,11 @@ def __init__(self, conn): self._sync = None self._body = '' + self.packer = msgpack.Packer() + + def _dumps(self, src): + return self.packer.pack(src) + def __bytes__(self): return self.header(len(self._body)) + self._body @@ -82,11 +87,11 @@ def sync(self): def header(self, length): self._sync = self.conn.generate_sync() - header = msgpack.dumps({IPROTO_CODE: self.request_type, - IPROTO_SYNC: self._sync, - IPROTO_SCHEMA_ID: self.conn.schema_version}) + header = self._dumps({IPROTO_CODE: self.request_type, + IPROTO_SYNC: self._sync, + IPROTO_SCHEMA_ID: self.conn.schema_version}) - return msgpack.dumps(length + len(header)) + header + return self._dumps(length + len(header)) + header class RequestInsert(Request): @@ -102,8 +107,8 @@ def __init__(self, conn, space_no, values): super(RequestInsert, self).__init__(conn) assert isinstance(values, (tuple, list)) - request_body = msgpack.dumps({IPROTO_SPACE_ID: space_no, - IPROTO_TUPLE: values}) + request_body = self._dumps({IPROTO_SPACE_ID: space_no, + IPROTO_TUPLE: values}) self._body = request_body @@ -131,19 +136,19 @@ def sha1(values): hash2 = sha1((hash1,)) scramble = sha1((salt, hash2)) scramble = strxor(hash1, scramble) - request_body = msgpack.dumps({IPROTO_USER_NAME: user, - IPROTO_TUPLE: ("chap-sha1", scramble)}) + request_body = self._dumps({IPROTO_USER_NAME: user, + IPROTO_TUPLE: ("chap-sha1", scramble)}) self._body = request_body def header(self, length): self._sync = self.conn.generate_sync() # Set IPROTO_SCHEMA_ID: 0 to avoid SchemaReloadException # It is ok to use 0 in auth every time. - header = msgpack.dumps({IPROTO_CODE: self.request_type, - IPROTO_SYNC: self._sync, - IPROTO_SCHEMA_ID: 0}) + header = self._dumps({IPROTO_CODE: self.request_type, + IPROTO_SYNC: self._sync, + IPROTO_SCHEMA_ID: 0}) - return msgpack.dumps(length + len(header)) + header + return self._dumps(length + len(header)) + header class RequestReplace(Request): @@ -159,8 +164,8 @@ def __init__(self, conn, space_no, values): super(RequestReplace, self).__init__(conn) assert isinstance(values, (tuple, list)) - request_body = msgpack.dumps({IPROTO_SPACE_ID: space_no, - IPROTO_TUPLE: values}) + request_body = self._dumps({IPROTO_SPACE_ID: space_no, + IPROTO_TUPLE: values}) self._body = request_body @@ -177,9 +182,9 @@ def __init__(self, conn, space_no, index_no, key): ''' super(RequestDelete, self).__init__(conn) - request_body = msgpack.dumps({IPROTO_SPACE_ID: space_no, - IPROTO_INDEX_ID: index_no, - IPROTO_KEY: key}) + request_body = self._dumps({IPROTO_SPACE_ID: space_no, + IPROTO_INDEX_ID: index_no, + IPROTO_KEY: key}) self._body = request_body @@ -193,12 +198,12 @@ class RequestSelect(Request): # pylint: disable=W0231 def __init__(self, conn, space_no, index_no, key, offset, limit, iterator): super(RequestSelect, self).__init__(conn) - request_body = msgpack.dumps({IPROTO_SPACE_ID: space_no, - IPROTO_INDEX_ID: index_no, - IPROTO_OFFSET: offset, - IPROTO_LIMIT: limit, - IPROTO_ITERATOR: iterator, - IPROTO_KEY: key}) + request_body = self._dumps({IPROTO_SPACE_ID: space_no, + IPROTO_INDEX_ID: index_no, + IPROTO_OFFSET: offset, + IPROTO_LIMIT: limit, + IPROTO_ITERATOR: iterator, + IPROTO_KEY: key}) self._body = request_body @@ -214,10 +219,10 @@ class RequestUpdate(Request): def __init__(self, conn, space_no, index_no, key, op_list): super(RequestUpdate, self).__init__(conn) - request_body = msgpack.dumps({IPROTO_SPACE_ID: space_no, - IPROTO_INDEX_ID: index_no, - IPROTO_KEY: key, - IPROTO_TUPLE: op_list}) + request_body = self._dumps({IPROTO_SPACE_ID: space_no, + IPROTO_INDEX_ID: index_no, + IPROTO_KEY: key, + IPROTO_TUPLE: op_list}) self._body = request_body @@ -235,8 +240,8 @@ def __init__(self, conn, name, args, call_16): super(RequestCall, self).__init__(conn) assert isinstance(args, (list, tuple)) - request_body = msgpack.dumps({IPROTO_FUNCTION_NAME: name, - IPROTO_TUPLE: args}) + request_body = self._dumps({IPROTO_FUNCTION_NAME: name, + IPROTO_TUPLE: args}) self._body = request_body @@ -252,8 +257,8 @@ def __init__(self, conn, name, args): super(RequestEval, self).__init__(conn) assert isinstance(args, (list, tuple)) - request_body = msgpack.dumps({IPROTO_EXPR: name, - IPROTO_TUPLE: args}) + request_body = self._dumps({IPROTO_EXPR: name, + IPROTO_TUPLE: args}) self._body = request_body @@ -280,10 +285,10 @@ class RequestUpsert(Request): def __init__(self, conn, space_no, index_no, tuple_value, op_list): super(RequestUpsert, self).__init__(conn) - request_body = msgpack.dumps({IPROTO_SPACE_ID: space_no, - IPROTO_INDEX_ID: index_no, - IPROTO_TUPLE: tuple_value, - IPROTO_OPS: op_list}) + request_body = self._dumps({IPROTO_SPACE_ID: space_no, + IPROTO_INDEX_ID: index_no, + IPROTO_TUPLE: tuple_value, + IPROTO_OPS: op_list}) self._body = request_body @@ -297,7 +302,7 @@ class RequestJoin(Request): # pylint: disable=W0231 def __init__(self, conn, server_uuid): super(RequestJoin, self).__init__(conn) - request_body = msgpack.dumps({IPROTO_SERVER_UUID: server_uuid}) + request_body = self._dumps({IPROTO_SERVER_UUID: server_uuid}) self._body = request_body @@ -312,7 +317,7 @@ def __init__(self, conn, cluster_uuid, server_uuid, vclock): super(RequestSubscribe, self).__init__(conn) assert isinstance(vclock, dict) - request_body = msgpack.dumps({ + request_body = self._dumps({ IPROTO_CLUSTER_UUID: cluster_uuid, IPROTO_SERVER_UUID: server_uuid, IPROTO_VCLOCK: vclock @@ -329,6 +334,6 @@ class RequestOK(Request): # pylint: disable=W0231 def __init__(self, conn, sync): super(RequestOK, self).__init__(conn) - request_body = msgpack.dumps({IPROTO_CODE: self.request_type, - IPROTO_SYNC: sync}) + request_body = self._dumps({IPROTO_CODE: self.request_type, + IPROTO_SYNC: sync}) self._body = request_body From 45d530cb7264a5f382e832f429a4be9652044d5b Mon Sep 17 00:00:00 2001 From: Alexander Turenko Date: Wed, 26 Aug 2020 08:29:48 +0300 Subject: [PATCH 4/6] Support msgpack-1.0.0 (packing part) The default value of the 'use_bin_type' option was changed in msgpack-1.0.0. We should support different versions of the library and provide the same behaviour across them. Aside of this, 'encoding' option was dropped since this version of the msgpack library, but we didn't actually support it for packing. Binary strings are packed as is (as mp_str), Unicode strings are encoded as UTF-8 (as mp_str too) and nothing is changed regarding this. Fixes #155 --- tarantool/request.py | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/tarantool/request.py b/tarantool/request.py index 55cbeecc..b7651c51 100644 --- a/tarantool/request.py +++ b/tarantool/request.py @@ -65,7 +65,19 @@ def __init__(self, conn): self._sync = None self._body = '' - self.packer = msgpack.Packer() + packer_kwargs = dict() + + # use_bin_type=True is default since msgpack-1.0.0. + # + # The option controls whether to pack binary (non-unicode) + # string values as mp_bin or as mp_str. + # + # The default behaviour of the connector is to pack both + # bytes and Unicode strings as mp_str. + if msgpack.version >= (1, 0, 0): + packer_kwargs['use_bin_type'] = False + + self.packer = msgpack.Packer(**packer_kwargs) def _dumps(self, src): return self.packer.pack(src) From 542ba75e0a3940b18dc09565862a68ec5460aa7e Mon Sep 17 00:00:00 2001 From: Alexander Turenko Date: Sat, 29 Aug 2020 01:52:33 +0300 Subject: [PATCH 5/6] Eliminate deprecation warning re use_bin_type When 'use_bin_type' option is not set explicitly, msgpack-0.5.0 (and only it, not 0.5.1 or newer) warns a user that its default value will be changed in a future. The main reason of the change is to allow test_03_discovery_bad_good_addresses() test from test_mesh.py to pass on msgpack-0.5.0: it counts deprecation warnings produced by a call and so affected if unexpected warnings are emitted. This commit is prerequisite to enable msgpack-0.5.0 in CI, which is done in the next commit. Follows up #155 --- tarantool/request.py | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/tarantool/request.py b/tarantool/request.py index b7651c51..e4c1acc7 100644 --- a/tarantool/request.py +++ b/tarantool/request.py @@ -74,8 +74,19 @@ def __init__(self, conn): # # The default behaviour of the connector is to pack both # bytes and Unicode strings as mp_str. - if msgpack.version >= (1, 0, 0): - packer_kwargs['use_bin_type'] = False + # + # msgpack-0.5.0 (and only this version) warns when the + # option is unset: + # + # | FutureWarning: use_bin_type option is not specified. + # | Default value of the option will be changed in future + # | version. + # + # The option is supported since msgpack-0.4.0, so we can + # just always set it for all msgpack versions to get rid + # of the warning on msgpack-0.5.0 and to keep our + # behaviour on msgpack-1.0.0. + packer_kwargs['use_bin_type'] = False self.packer = msgpack.Packer(**packer_kwargs) From 053aa8830856e9874b299c7a096c3131dc3b7885 Mon Sep 17 00:00:00 2001 From: Alexander Turenko Date: Fri, 28 Aug 2020 18:43:35 +0300 Subject: [PATCH 6/6] travis-ci: verify different msgpack library versions Also bump msgpack PyPI package name in requirements.txt. See the note about the library name change in [1]: | Package name on PyPI was changed to msgpack from 0.5. I choose versions for testing this way: we have 0.4.0 in requirements.txt, so it is the baseline. We should test at least 0.4.0, 0.5.0, 0.6.0 and 1.0.0. I also added 0.6.2, because this version is installed on my Gentoo box and because it is the last release before 1.0.0. [1]: https://github.com/msgpack/msgpack-python/blob/8fb709f2e0438862020d8810fa70a81fb5dac7d4/README.md Follows up #155 --- .travis.yml | 9 +++++++++ requirements.txt | 2 +- test.sh | 7 ++++++- 3 files changed, 16 insertions(+), 2 deletions(-) diff --git a/.travis.yml b/.travis.yml index 784cb107..268506fe 100644 --- a/.travis.yml +++ b/.travis.yml @@ -8,6 +8,15 @@ cache: false env: matrix: - TARGET=test + PYTHON_MSGPACK=msgpack-python==0.4.0 + - TARGET=test + PYTHON_MSGPACK=msgpack==0.5.0 + - TARGET=test + PYTHON_MSGPACK=msgpack==0.6.0 + - TARGET=test + PYTHON_MSGPACK=msgpack==0.6.2 + - TARGET=test + PYTHON_MSGPACK=msgpack==1.0.0 - OS=el DIST=6 - OS=el DIST=7 - OS=fedora DIST=28 diff --git a/requirements.txt b/requirements.txt index 5a1556c1..14261dfd 100644 --- a/requirements.txt +++ b/requirements.txt @@ -1 +1 @@ -msgpack-python>=0.4.0 +msgpack>=0.4.0 diff --git a/test.sh b/test.sh index a8c1b577..ce701592 100755 --- a/test.sh +++ b/test.sh @@ -9,8 +9,13 @@ echo "deb http://download.tarantool.org/tarantool/2x/ubuntu/ ${release} main" | sudo apt-get update > /dev/null sudo apt-get -q -y install tarantool +# Install module requirements. +# +# Keep it in sync with requirements.txt. +pip install "${PYTHON_MSGPACK:-msgpack==1.0.0}" +python -c 'import msgpack; print(msgpack.version)' + # Install testing dependencies. -pip install -r requirements.txt pip install pyyaml # Run tests.