Improve keyword query parameters (#835)

 * API docs: clarify precedence between keyword and dict query parameters
 * Fix: parameters of internal functions could clash with keyword parameters.
   E.g., `db` and `imp_user` could not be used as keyword query parameters
   causing a not necessarily easy to understand error message.
   This got fixed by merging keyword and dict parameters at the top-most level.
* Add tests for query parameter precedence

Author: robsdedude

neo4j/_async/work/result.py

@@ -106,24 +106,19 @@ def _qid(self):
         else:
             return self._raw_qid
 
-    async def _tx_ready_run(self, query, parameters, **kwargs):
+    async def _tx_ready_run(self, query, parameters):
         # BEGIN+RUN does not carry any extra on the RUN message.
         # BEGIN {extra}
         # RUN "query" {parameters} {extra}
-        await self._run(
-            query, parameters, None, None, None, None, **kwargs
-        )
+        await self._run(query, parameters, None, None, None, None)
 
     async def _run(
-        self, query, parameters, db, imp_user, access_mode, bookmarks,
-        **kwargs
+        self, query, parameters, db, imp_user, access_mode, bookmarks
     ):
         query_text = str(query)  # Query or string object
         query_metadata = getattr(query, "metadata", None)
         query_timeout = getattr(query, "timeout", None)
 
-        parameters = dict(parameters or {}, **kwargs)
-
         self._metadata = {
             "query": query_text,
             "parameters": parameters,

neo4j/_async/work/session.py

@@ -258,7 +258,8 @@ async def run(
 
         :param query: cypher query
         :param parameters: dictionary of parameters
-        :param kwargs: additional keyword parameters
+        :param kwargs: additional keyword parameters.
+            These take precedence over parameters passed as ``parameters``.
 
         :raises SessionError: if the session has been closed.
 
@@ -286,10 +287,11 @@ async def run(
             self._result_error
         )
         bookmarks = await self._get_all_bookmarks()
+        parameters = dict(parameters or {}, **kwargs)
         await self._auto_result._run(
             query, parameters, self._config.database,
             self._config.impersonated_user, self._config.default_access_mode,
-            bookmarks, **kwargs
+            bookmarks
         )
 
         return self._auto_result

neo4j/_async/work/transaction.py

@@ -119,7 +119,8 @@ async def run(
 
         :param query: cypher query
         :param parameters: dictionary of parameters
-        :param kwparameters: additional keyword parameters
+        :param kwparameters: additional keyword parameters.
+            These take precedence over parameters passed as ``parameters``.
 
         :raise TransactionError: if the transaction is already closed
 
@@ -147,7 +148,8 @@ async def run(
         )
         self._results.append(result)
 
-        await result._tx_ready_run(query, parameters, **kwparameters)
+        parameters = dict(parameters or {}, **kwparameters)
+        await result._tx_ready_run(query, parameters)
 
         return result
 

neo4j/_sync/work/result.py

@@ -106,24 +106,19 @@ def _qid(self):
         else:
             return self._raw_qid
 
-    def _tx_ready_run(self, query, parameters, **kwargs):
+    def _tx_ready_run(self, query, parameters):
         # BEGIN+RUN does not carry any extra on the RUN message.
         # BEGIN {extra}
         # RUN "query" {parameters} {extra}
-        self._run(
-            query, parameters, None, None, None, None, **kwargs
-        )
+        self._run(query, parameters, None, None, None, None)
 
     def _run(
-        self, query, parameters, db, imp_user, access_mode, bookmarks,
-        **kwargs
+        self, query, parameters, db, imp_user, access_mode, bookmarks
     ):
         query_text = str(query)  # Query or string object
         query_metadata = getattr(query, "metadata", None)
         query_timeout = getattr(query, "timeout", None)
 
-        parameters = dict(parameters or {}, **kwargs)
-
         self._metadata = {
             "query": query_text,
             "parameters": parameters,

neo4j/_sync/work/session.py

@@ -258,7 +258,8 @@ def run(
 
         :param query: cypher query
         :param parameters: dictionary of parameters
-        :param kwargs: additional keyword parameters
+        :param kwargs: additional keyword parameters.
+            These take precedence over parameters passed as ``parameters``.
 
         :raises SessionError: if the session has been closed.
 
@@ -286,10 +287,11 @@ def run(
             self._result_error
         )
         bookmarks = self._get_all_bookmarks()
+        parameters = dict(parameters or {}, **kwargs)
         self._auto_result._run(
             query, parameters, self._config.database,
             self._config.impersonated_user, self._config.default_access_mode,
-            bookmarks, **kwargs
+            bookmarks
         )
 
         return self._auto_result

neo4j/_sync/work/transaction.py

@@ -119,7 +119,8 @@ def run(
 
         :param query: cypher query
         :param parameters: dictionary of parameters
-        :param kwparameters: additional keyword parameters
+        :param kwparameters: additional keyword parameters.
+            These take precedence over parameters passed as ``parameters``.
 
         :raise TransactionError: if the transaction is already closed
 
@@ -147,7 +148,8 @@ def run(
         )
         self._results.append(result)
 
-        result._tx_ready_run(query, parameters, **kwparameters)
+        parameters = dict(parameters or {}, **kwparameters)
+        result._tx_ready_run(query, parameters)
 
         return result
 

tests/unit/async_/work/test_session.py

@@ -319,9 +319,7 @@ async def test_session_tx_type(fake_pool):
 ))
 @pytest.mark.parametrize("run_type", ("auto", "unmanaged", "managed"))
 @mark_async_test
-async def test_session_run_with_parameters(
-    fake_pool, parameters, run_type, mocker
-):
+async def test_session_run_with_parameters(fake_pool, parameters, run_type):
     async with AsyncSession(fake_pool, SessionConfig()) as session:
         if run_type == "auto":
             await session.run("RETURN $x", **parameters)
@@ -337,12 +335,59 @@ async def work(tx):
 
     assert len(fake_pool.acquired_connection_mocks) == 1
     connection_mock = fake_pool.acquired_connection_mocks[0]
-    assert connection_mock.run.called_once()
+    connection_mock.run.assert_called_once()
     call = connection_mock.run.call_args
     assert call.args[0] == "RETURN $x"
     assert call.kwargs["parameters"] == parameters
 
 
+@pytest.mark.parametrize(
+    ("params", "kw_params", "expected_params"),
+    (
+        ({"x": 1}, {}, {"x": 1}),
+        ({}, {"x": 1}, {"x": 1}),
+        ({"x": 1}, {"y": 2}, {"x": 1, "y": 2}),
+        ({"x": 1}, {"x": 2}, {"x": 2}),
+        ({"x": 1}, {"x": 2}, {"x": 2}),
+        ({"x": 1, "y": 3}, {"x": 2}, {"x": 2, "y": 3}),
+        ({"x": 1}, {"x": 2, "y": 3}, {"x": 2, "y": 3}),
+        # potentially internally used keyword arguments
+        ({}, {"timeout": 2}, {"timeout": 2}),
+        ({"timeout": 2}, {}, {"timeout": 2}),
+        ({}, {"imp_user": "hans"}, {"imp_user": "hans"}),
+        ({"imp_user": "hans"}, {}, {"imp_user": "hans"}),
+        ({}, {"db": "neo4j"}, {"db": "neo4j"}),
+        ({"db": "neo4j"}, {}, {"db": "neo4j"}),
+        ({}, {"database": "neo4j"}, {"database": "neo4j"}),
+        ({"database": "neo4j"}, {}, {"database": "neo4j"}),
+    )
+)
+@pytest.mark.parametrize("run_type", ("auto", "unmanaged", "managed"))
+@mark_async_test
+async def test_session_run_parameter_precedence(
+    fake_pool, params, kw_params, expected_params, run_type
+):
+    async with AsyncSession(fake_pool, SessionConfig()) as session:
+        if run_type == "auto":
+            await session.run("RETURN $x", params, **kw_params)
+        elif run_type == "unmanaged":
+            tx = await session.begin_transaction()
+            await tx.run("RETURN $x", params, **kw_params)
+        elif run_type == "managed":
+            async def work(tx):
+                await tx.run("RETURN $x", params, **kw_params)
+            await session.execute_write(work)
+        else:
+            raise ValueError(run_type)
+
+    assert len(fake_pool.acquired_connection_mocks) == 1
+    connection_mock = fake_pool.acquired_connection_mocks[0]
+    connection_mock.run.assert_called_once()
+    call = connection_mock.run.call_args
+    assert call.args[0] == "RETURN $x"
+    assert call.kwargs["parameters"] == expected_params
+
+
 @pytest.mark.parametrize("db", (None, "adb"))
 @pytest.mark.parametrize("routing", (True, False))
 # no home db resolution when connected to Neo4j 4.3 or earlier
@@ -446,7 +491,7 @@ async def bmm_gat_all_bookmarks():
 
     assert len(fake_pool.acquired_connection_mocks) == 1
     connection_mock = fake_pool.acquired_connection_mocks[0]
-    assert connection_mock.run.called_once()
+    connection_mock.run.assert_called_once()
     connection_run_call_kwargs = connection_mock.run.call_args.kwargs
     assert (set(connection_run_call_kwargs["bookmarks"])
             == {"all", "bookmarks", *(additional_session_bookmarks or [])})

tests/unit/sync/work/test_session.py

@@ -319,9 +319,7 @@ def test_session_tx_type(fake_pool):
 ))
 @pytest.mark.parametrize("run_type", ("auto", "unmanaged", "managed"))
 @mark_sync_test
-def test_session_run_with_parameters(
-    fake_pool, parameters, run_type, mocker
-):
+def test_session_run_with_parameters(fake_pool, parameters, run_type):
     with Session(fake_pool, SessionConfig()) as session:
         if run_type == "auto":
             session.run("RETURN $x", **parameters)
@@ -337,12 +335,59 @@ def work(tx):
 
     assert len(fake_pool.acquired_connection_mocks) == 1
     connection_mock = fake_pool.acquired_connection_mocks[0]
-    assert connection_mock.run.called_once()
+    connection_mock.run.assert_called_once()
     call = connection_mock.run.call_args
     assert call.args[0] == "RETURN $x"
     assert call.kwargs["parameters"] == parameters
 
 
+@pytest.mark.parametrize(
+    ("params", "kw_params", "expected_params"),
+    (
+        ({"x": 1}, {}, {"x": 1}),
+        ({}, {"x": 1}, {"x": 1}),
+        ({"x": 1}, {"y": 2}, {"x": 1, "y": 2}),
+        ({"x": 1}, {"x": 2}, {"x": 2}),
+        ({"x": 1}, {"x": 2}, {"x": 2}),
+        ({"x": 1, "y": 3}, {"x": 2}, {"x": 2, "y": 3}),
+        ({"x": 1}, {"x": 2, "y": 3}, {"x": 2, "y": 3}),
+        # potentially internally used keyword arguments
+        ({}, {"timeout": 2}, {"timeout": 2}),
+        ({"timeout": 2}, {}, {"timeout": 2}),
+        ({}, {"imp_user": "hans"}, {"imp_user": "hans"}),
+        ({"imp_user": "hans"}, {}, {"imp_user": "hans"}),
+        ({}, {"db": "neo4j"}, {"db": "neo4j"}),
+        ({"db": "neo4j"}, {}, {"db": "neo4j"}),
+        ({}, {"database": "neo4j"}, {"database": "neo4j"}),
+        ({"database": "neo4j"}, {}, {"database": "neo4j"}),
+    )
+)
+@pytest.mark.parametrize("run_type", ("auto", "unmanaged", "managed"))
+@mark_sync_test
+def test_session_run_parameter_precedence(
+    fake_pool, params, kw_params, expected_params, run_type
+):
+    with Session(fake_pool, SessionConfig()) as session:
+        if run_type == "auto":
+            session.run("RETURN $x", params, **kw_params)
+        elif run_type == "unmanaged":
+            tx = session.begin_transaction()
+            tx.run("RETURN $x", params, **kw_params)
+        elif run_type == "managed":
+            def work(tx):
+                tx.run("RETURN $x", params, **kw_params)
+            session.execute_write(work)
+        else:
+            raise ValueError(run_type)
+
+    assert len(fake_pool.acquired_connection_mocks) == 1
+    connection_mock = fake_pool.acquired_connection_mocks[0]
+    connection_mock.run.assert_called_once()
+    call = connection_mock.run.call_args
+    assert call.args[0] == "RETURN $x"
+    assert call.kwargs["parameters"] == expected_params
+
+
 @pytest.mark.parametrize("db", (None, "adb"))
 @pytest.mark.parametrize("routing", (True, False))
 # no home db resolution when connected to Neo4j 4.3 or earlier
@@ -446,7 +491,7 @@ def bmm_gat_all_bookmarks():
 
     assert len(fake_pool.acquired_connection_mocks) == 1
     connection_mock = fake_pool.acquired_connection_mocks[0]
-    assert connection_mock.run.called_once()
+    connection_mock.run.assert_called_once()
     connection_run_call_kwargs = connection_mock.run.call_args.kwargs
     assert (set(connection_run_call_kwargs["bookmarks"])
             == {"all", "bookmarks", *(additional_session_bookmarks or [])})