diff --git a/python/packages/duckdb/README.md b/python/packages/duckdb/README.md index a9d0271dc40..c6acff3025a 100644 --- a/python/packages/duckdb/README.md +++ b/python/packages/duckdb/README.md @@ -70,13 +70,14 @@ by DuckDB, not by this connector. Alternatively, use `DuckDBStore(client=duckdb.connect(...))` or `DuckDBCollection(Record, client=...)` to **borrow** an existing connection. -The caller owns and closes that connection; `client` cannot be combined with -`connection_string`, `config`, or `.env` options. Collections obtained from a -store borrow its client, so keep the store open while using them. All connector -operations on one store/collection run serially on one worker thread, not on -the event loop. Coordinate any *external* use of an injected connection -yourself. Cancelling an awaiter does not stop a DuckDB query already running -on that thread; `aclose()` waits for it to finish. +The caller owns and closes that connection; `client` takes precedence over +`connection_string`, `config`, and `.env` options, which are ignored when a +client is supplied. Collections obtained from a store borrow its client, so keep +the store open while using them. All connector operations on one store/collection +run serially on one worker thread, not on the event loop. Coordinate any +*external* use of an injected connection yourself. Cancelling an awaiter does +not stop a DuckDB query already running on that thread; `aclose()` waits for it +to finish. ## Example diff --git a/python/packages/duckdb/agent_framework_duckdb/_vector_store.py b/python/packages/duckdb/agent_framework_duckdb/_vector_store.py index fd7d5b5262e..f9a9dd44acc 100644 --- a/python/packages/duckdb/agent_framework_duckdb/_vector_store.py +++ b/python/packages/duckdb/agent_framework_duckdb/_vector_store.py @@ -196,8 +196,6 @@ def _create_client( env_file_encoding: str | None, ) -> _Client: if client is not None: - if any(value is not None for value in (connection_string, config, env_file_path, env_file_encoding)): - raise ValueError("client cannot be combined with connection_string, config, or .env options.") if not isinstance(client, duckdb.DuckDBPyConnection): raise TypeError("client must be a DuckDBPyConnection.") return _Client(None, client=client, config=None) @@ -403,7 +401,8 @@ def __init__( Args: record_type: Registered typed model or ``dict`` with an explicit definition. connection_string: Persistent filename or DuckDB URI. Defaults to ``agent-framework.duckdb``. - client: Borrowed DuckDB connection; mutually exclusive with connection settings. + client: Borrowed DuckDB connection; takes precedence over all connection settings. When supplied, + ``connection_string``, ``config``, ``env_file_path``, and ``env_file_encoding`` are ignored. config: DuckDB connection settings, optionally wrapping secret values in ``SecretString``. definition: Explicit definition for dictionary records. collection_name: Table name, overriding the model definition. @@ -718,7 +717,8 @@ def __init__( Args: connection_string: On-disk filename or DuckDB URI; defaults to ``agent-framework.duckdb``. - client: Borrowed DuckDB connection; cannot be combined with other connection settings. + client: Borrowed DuckDB connection; takes precedence over all connection settings. When supplied, + ``connection_string``, ``config``, ``env_file_path``, and ``env_file_encoding`` are ignored. config: Additional DuckDB connection settings. Wrap credential values in ``SecretString``. embedding_generator: Default local embedding client for collections. env_file_path: Optional selected .env file for ``DUCKDB_CONNECTION_STRING``. diff --git a/python/packages/duckdb/tests/duckdb/test_settings.py b/python/packages/duckdb/tests/duckdb/test_settings.py index e248d3a7809..f5bd0479e24 100644 --- a/python/packages/duckdb/tests/duckdb/test_settings.py +++ b/python/packages/duckdb/tests/duckdb/test_settings.py @@ -176,14 +176,74 @@ async def test_connection_failure_does_not_chain_unwrapped_secrets(): {"env_file_encoding": "utf-16"}, ], ) -def test_injected_client_rejects_conflicting_settings(constructor, kwargs): +def test_injected_client_ignores_conflicting_settings(constructor, kwargs): connection = duckdb.connect(":memory:") try: - with ( - patch.object(module, "load_settings", side_effect=AssertionError("settings must not be loaded")), - pytest.raises(ValueError, match="cannot be combined"), - ): - constructor(client=connection, **kwargs) + with patch.object(module, "load_settings", side_effect=AssertionError("settings must not be loaded")): + instance = constructor(client=connection, **kwargs) + assert instance._client._connection is connection + assert instance._client._address is None + assert instance._client.owned is False + assert instance.managed_client is False + finally: + connection.close() + + +def test_injected_client_valid_config_ignored(constructor): + connection = duckdb.connect(":memory:") + try: + instance = constructor(client=connection, config={"threads": "2"}) + assert instance._client._connection is connection + finally: + connection.close() + + +def test_injected_client_invalid_config_ignored(constructor): + connection = duckdb.connect(":memory:") + try: + instance = constructor(client=connection, config={"threads": object()}) + assert instance._client._connection is connection + finally: + connection.close() + + +def test_injected_client_invalid_env_path_ignored(constructor, tmp_path): + connection = duckdb.connect(":memory:") + try: + instance = constructor(client=connection, env_file_path=str(tmp_path / "missing.env")) + assert instance._client._connection is connection + finally: + connection.close() + + +def test_injected_client_invalid_env_encoding_ignored(constructor): + connection = duckdb.connect(":memory:") + try: + instance = constructor(client=connection, env_file_encoding="bogus-encoding") + assert instance._client._connection is connection + finally: + connection.close() + + +def test_injected_client_environment_variables_ignored(constructor, monkeypatch): + monkeypatch.setenv("DUCKDB_CONNECTION_STRING", "md:should-not-be-opened") + connection = duckdb.connect(":memory:") + try: + with patch.object(module, "load_settings", side_effect=AssertionError("settings must not be loaded")): + instance = constructor(client=connection) + assert instance._client._connection is connection + finally: + connection.close() + + +async def test_duckdb_connect_not_called_when_client_injected(constructor): + connection = duckdb.connect(":memory:") + try: + with patch.object(module.duckdb, "connect", side_effect=AssertionError("duckdb.connect must not be called")): + instance = constructor(client=connection) + assert instance._client._connection is connection + result = await instance._client.run(lambda c: c.execute("SELECT 1").fetchone()) + assert result == (1,) finally: connection.close()