From aed9b0ac8f8692a5e04b976aa8c6dd54ec767f82 Mon Sep 17 00:00:00 2001 From: Georgy Moiseev Date: Tue, 19 Sep 2023 17:36:56 +0300 Subject: [PATCH 1/2] internal: more careful crud response process In most cases, crud API response process assumes that there are always two response values, which is rather uncommon. The reason of that is internal crud wrappers [1]. This patch reworks response process a bit to more carefully process this unusual case and to not confuse developers. 1. https://github.com/tarantool/crud/blob/53457477974fed42351cbd87f566d11e9f7e39bb/crud/common/schema.lua#L88 --- tarantool/connection.py | 51 +++++++++++++++++++---------------------- 1 file changed, 23 insertions(+), 28 deletions(-) diff --git a/tarantool/connection.py b/tarantool/connection.py index ea089c08..2fab182b 100644 --- a/tarantool/connection.py +++ b/tarantool/connection.py @@ -2158,7 +2158,7 @@ def crud_insert(self, space_name: str, values: Union[tuple, list], crud_resp = call_crud(self, "crud.insert", space_name, values, opts) - if crud_resp[1] is not None: + if len(crud_resp) > 1 and crud_resp[1] is not None: raise CrudModuleError(None, CrudError(crud_resp[1])) return CrudResult(crud_resp[0]) @@ -2192,7 +2192,7 @@ def crud_insert_object(self, space_name: str, values: dict, crud_resp = call_crud(self, "crud.insert_object", space_name, values, opts) - if crud_resp[1] is not None: + if len(crud_resp) > 1 and crud_resp[1] is not None: raise CrudModuleError(None, CrudError(crud_resp[1])) return CrudResult(crud_resp[0]) @@ -2230,7 +2230,7 @@ def crud_insert_many(self, space_name: str, values: Union[tuple, list], if crud_resp[0] is not None: res = CrudResult(crud_resp[0]) - if crud_resp[1] is not None: + if len(crud_resp) > 1 and crud_resp[1] is not None: errs = [] for err in crud_resp[1]: errs.append(CrudError(err)) @@ -2271,7 +2271,7 @@ def crud_insert_object_many(self, space_name: str, values: Union[tuple, list], if crud_resp[0] is not None: res = CrudResult(crud_resp[0]) - if crud_resp[1] is not None: + if len(crud_resp) > 1 and crud_resp[1] is not None: errs = [] for err in crud_resp[1]: errs.append(CrudError(err)) @@ -2306,7 +2306,7 @@ def crud_get(self, space_name: str, key: int, opts: Optional[dict] = None) -> Cr crud_resp = call_crud(self, "crud.get", space_name, key, opts) - if crud_resp[1] is not None: + if len(crud_resp) > 1 and crud_resp[1] is not None: raise CrudModuleError(None, CrudError(crud_resp[1])) return CrudResult(crud_resp[0]) @@ -2345,7 +2345,7 @@ def crud_update(self, space_name: str, key: int, operations: Optional[list] = No crud_resp = call_crud(self, "crud.update", space_name, key, operations, opts) - if crud_resp[1] is not None: + if len(crud_resp) > 1 and crud_resp[1] is not None: raise CrudModuleError(None, CrudError(crud_resp[1])) return CrudResult(crud_resp[0]) @@ -2377,7 +2377,7 @@ def crud_delete(self, space_name: str, key: int, opts: Optional[dict] = None) -> crud_resp = call_crud(self, "crud.delete", space_name, key, opts) - if crud_resp[1] is not None: + if len(crud_resp) > 1 and crud_resp[1] is not None: raise CrudModuleError(None, CrudError(crud_resp[1])) return CrudResult(crud_resp[0]) @@ -2411,7 +2411,7 @@ def crud_replace(self, space_name: str, values: Union[tuple, list], crud_resp = call_crud(self, "crud.replace", space_name, values, opts) - if crud_resp[1] is not None: + if len(crud_resp) > 1 and crud_resp[1] is not None: raise CrudModuleError(None, CrudError(crud_resp[1])) return CrudResult(crud_resp[0]) @@ -2445,7 +2445,7 @@ def crud_replace_object(self, space_name: str, values: dict, crud_resp = call_crud(self, "crud.replace_object", space_name, values, opts) - if crud_resp[1] is not None: + if len(crud_resp) > 1 and crud_resp[1] is not None: raise CrudModuleError(None, CrudError(crud_resp[1])) return CrudResult(crud_resp[0]) @@ -2483,7 +2483,7 @@ def crud_replace_many(self, space_name: str, values: Union[tuple, list], if crud_resp[0] is not None: res = CrudResult(crud_resp[0]) - if crud_resp[1] is not None: + if len(crud_resp) > 1 and crud_resp[1] is not None: errs = [] for err in crud_resp[1]: errs.append(CrudError(err)) @@ -2524,7 +2524,7 @@ def crud_replace_object_many(self, space_name: str, values: Union[tuple, list], if crud_resp[0] is not None: res = CrudResult(crud_resp[0]) - if crud_resp[1] is not None: + if len(crud_resp) > 1 and crud_resp[1] is not None: errs = [] for err in crud_resp[1]: errs.append(CrudError(err)) @@ -2567,7 +2567,7 @@ def crud_upsert(self, space_name: str, values: Union[tuple, list], crud_resp = call_crud(self, "crud.upsert", space_name, values, operations, opts) - if crud_resp[1] is not None: + if len(crud_resp) > 1 and crud_resp[1] is not None: raise CrudModuleError(None, CrudError(crud_resp[1])) return CrudResult(crud_resp[0]) @@ -2608,7 +2608,7 @@ def crud_upsert_object(self, space_name: str, values: dict, crud_resp = call_crud(self, "crud.upsert_object", space_name, values, operations, opts) - if crud_resp[1] is not None: + if len(crud_resp) > 1 and crud_resp[1] is not None: raise CrudModuleError(None, CrudError(crud_resp[1])) return CrudResult(crud_resp[0]) @@ -2646,7 +2646,7 @@ def crud_upsert_many(self, space_name: str, values_operation: Union[tuple, list] if crud_resp[0] is not None: res = CrudResult(crud_resp[0]) - if crud_resp[1] is not None: + if len(crud_resp) > 1 and crud_resp[1] is not None: errs = [] for err in crud_resp[1]: errs.append(CrudError(err)) @@ -2687,7 +2687,7 @@ def crud_upsert_object_many(self, space_name: str, values_operation: Union[tuple if crud_resp[0] is not None: res = CrudResult(crud_resp[0]) - if crud_resp[1] is not None: + if len(crud_resp) > 1 and crud_resp[1] is not None: errs = [] for err in crud_resp[1]: errs.append(CrudError(err)) @@ -2726,7 +2726,7 @@ def crud_select(self, space_name: str, conditions: Optional[list] = None, crud_resp = call_crud(self, "crud.select", space_name, conditions, opts) - if crud_resp[1] is not None: + if len(crud_resp) > 1 and crud_resp[1] is not None: raise CrudModuleError(None, CrudError(crud_resp[1])) return CrudResult(crud_resp[0]) @@ -2758,7 +2758,7 @@ def crud_min(self, space_name: str, index_name: str, opts: Optional[dict] = None crud_resp = call_crud(self, "crud.min", space_name, index_name, opts) - if crud_resp[1] is not None: + if len(crud_resp) > 1 and crud_resp[1] is not None: raise CrudModuleError(None, CrudError(crud_resp[1])) return CrudResult(crud_resp[0]) @@ -2790,7 +2790,7 @@ def crud_max(self, space_name: str, index_name: str, opts: Optional[dict] = None crud_resp = call_crud(self, "crud.max", space_name, index_name, opts) - if crud_resp[1] is not None: + if len(crud_resp) > 1 and crud_resp[1] is not None: raise CrudModuleError(None, CrudError(crud_resp[1])) return CrudResult(crud_resp[0]) @@ -2819,9 +2819,7 @@ def crud_truncate(self, space_name: str, opts: Optional[dict] = None) -> bool: crud_resp = call_crud(self, "crud.truncate", space_name, opts) - # In absence of an error, crud does not give - # variable err as nil (as in most cases). - if len(crud_resp) != 1: + if len(crud_resp) > 1 and crud_resp[1] is not None: raise CrudModuleError(None, CrudError(crud_resp[1])) return crud_resp[0] @@ -2850,9 +2848,7 @@ def crud_len(self, space_name: str, opts: Optional[dict] = None) -> int: crud_resp = call_crud(self, "crud.len", space_name, opts) - # In absence of an error, crud does not give - # variable err as nil (as in most cases). - if len(crud_resp) != 1: + if len(crud_resp) > 1 and crud_resp[1] is not None: raise CrudModuleError(None, CrudError(crud_resp[1])) return crud_resp[0] @@ -2877,9 +2873,7 @@ def crud_storage_info(self, opts: Optional[dict] = None) -> dict: crud_resp = call_crud(self, "crud.storage_info", opts) - # In absence of an error, crud does not give - # variable err as nil (as in most cases). - if len(crud_resp) != 1: + if len(crud_resp) > 1 and crud_resp[1] is not None: raise CrudModuleError(None, CrudError(crud_resp[1])) return crud_resp[0] @@ -2915,7 +2909,7 @@ def crud_count(self, space_name: str, conditions: Optional[list] = None, crud_resp = call_crud(self, "crud.count", space_name, conditions, opts) - if crud_resp[1] is not None: + if len(crud_resp) > 1 and crud_resp[1] is not None: raise CrudModuleError(None, CrudError(crud_resp[1])) return crud_resp[0] @@ -2938,6 +2932,7 @@ def crud_stats(self, space_name: str = None) -> CrudResult: crud_resp = call_crud(self, "crud.stats", space_name) + # There are no errors in `crud.stats`. res = None if len(crud_resp.data[0]) > 0: res = CrudResult(crud_resp.data[0]) From fdf297eebcb8934fa1e5644740f0774421535def Mon Sep 17 00:00:00 2001 From: Georgy Moiseev Date: Tue, 19 Sep 2023 17:41:31 +0300 Subject: [PATCH 2/2] api: exception rethrow in crud Before this patch, all DatabaseError exception, except for ER_NO_SUCH_PROC and ER_ACCESS_DENIED ones, were silently ignored. It resulted in "UnboundLocalError: local variable 'crud_resp' referenced before assignment" instead of a proper error. --- CHANGELOG.md | 5 +++++ tarantool/crud.py | 1 + test/suites/crud_mock_server.lua | 21 +++++++++++++++++++++ test/suites/test_crud.py | 21 +++++++++++++++++++++ 4 files changed, 48 insertions(+) create mode 100644 test/suites/crud_mock_server.lua diff --git a/CHANGELOG.md b/CHANGELOG.md index 13e58a2b..8555c6ac 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,11 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## Unreleased + +### Fixed +- Exception rethrow in crud API (PR #310). + ## 1.1.1 - 2023-07-19 ### Changed diff --git a/tarantool/crud.py b/tarantool/crud.py index d10726c5..3a642564 100644 --- a/tarantool/crud.py +++ b/tarantool/crud.py @@ -69,5 +69,6 @@ def call_crud(conn, *args): if exc.code in (ER_NO_SUCH_PROC, ER_ACCESS_DENIED): exc_msg = ". Ensure that you're calling crud.router and user has sufficient grants" raise DatabaseError(exc.code, exc.message + exc_msg, extra_info=exc.extra_info) from exc + raise exc return crud_resp diff --git a/test/suites/crud_mock_server.lua b/test/suites/crud_mock_server.lua new file mode 100644 index 00000000..252059df --- /dev/null +++ b/test/suites/crud_mock_server.lua @@ -0,0 +1,21 @@ +#!/usr/bin/env tarantool + +local admin_listen = os.getenv("ADMIN") +local primary_listen = os.getenv("LISTEN") + +require('console').listen(admin_listen) +box.cfg{ + listen = primary_listen, + memtx_memory = 0.1 * 1024^3, -- 0.1 GiB + pid_file = "box.pid", +} + +box.schema.user.grant('guest', 'execute', 'universe', nil, {if_not_exists = true}) + +local function mock_replace() + error('Unexpected connection error') +end + +rawset(_G, 'crud', {replace = mock_replace}) + +rawset(_G, 'ready', true) diff --git a/test/suites/test_crud.py b/test/suites/test_crud.py index d1d7e186..2bbc5513 100644 --- a/test/suites/test_crud.py +++ b/test/suites/test_crud.py @@ -22,6 +22,14 @@ def create_server(): return srv +def create_mock_server(): + srv = TarantoolServer() + srv.script = 'test/suites/crud_mock_server.lua' + srv.start() + + return srv + + @unittest.skipIf(sys.platform.startswith("win"), "Crud tests on windows platform are not supported: " "complexity of the vshard replicaset configuration") @@ -33,14 +41,19 @@ def setUpClass(cls): print('-' * 70, file=sys.stderr) # Create server and extract helpful fields for tests. cls.srv = create_server() + cls.mock_srv = create_mock_server() cls.host = cls.srv.host cls.port = cls.srv.args['primary'] + cls.mock_host = cls.mock_srv.host + cls.mock_port = cls.mock_srv.args['primary'] def setUp(self): time.sleep(1) # Open connections to instance. self.conn = tarantool.Connection(host=self.host, port=self.port, user='guest', password='', fetch_schema=False) + self.mock_conn = tarantool.Connection(host=self.mock_host, port=self.mock_port, + user='guest', password='', fetch_schema=False) self.conn_mesh = tarantool.MeshConnection(host=self.host, port=self.port, user='guest', password='', fetch_schema=False) self.conn_pool = tarantool.ConnectionPool([{'host': self.host, 'port': self.port}], @@ -736,9 +749,15 @@ def test_crud_module_via_pool_connection(self): # Exception try testing. self._exception_operation_with_crud(testing_function, case, mode=tarantool.Mode.RW) + def test_error_rethrow(self): + self.assertRaisesRegex( + DatabaseError, "Unexpected connection error", + lambda: self.mock_conn.crud_replace('tester', [2, 100, 'Alice'], {'timeout': 10})) + def tearDown(self): # Close connections to instance. self.conn.close() + self.mock_conn.close() self.conn_mesh.close() self.conn_pool.close() @@ -747,3 +766,5 @@ def tearDownClass(cls): # Stop instance. cls.srv.stop() cls.srv.clean() + cls.mock_srv.stop() + cls.mock_srv.clean()