From 7c5441063fe7ab3a4c43a769a452c2e154471914 Mon Sep 17 00:00:00 2001 From: Rohan Dsouza Date: Thu, 3 Sep 2026 17:49:20 +0530 Subject: [PATCH 1/3] feat: attach a registered source into an instant database's scope Every tool set here is scoped to exactly one instant database, and there was no way to bring a second source into that scope without leaving the library for the raw SDK. Adds attach_catalog, detach_catalog and database_attachments. attach_database_catalog and detach_database_catalog are on the generated SDK and on no version of HotdataClient, so these go through DatabasesApi(client.api) -- the same construction the framework uses internally, and the route resolve_database_by_id already takes. Both endpoints answer 204 with no body, so a refusal raises and there is nothing ambiguous to inspect. What a status cannot cover is a 204 that did not do the work, so each helper re-reads the database and raises if the attachment did not land or a detached connection is still listed, behind confirm=True. A 409 on attach is resolved by reading rather than assumed to mean "already attached", so re-running a provisioning step is a no-op. ManagedDatabase carries only id, description and default_connection_id, while the detail response also carries attachments -- so database_attachments reads what the resolved record drops, and is what makes an attach verifiable at all. Also records the measured cross-database boundary in docs/engine-contract.md, replacing its guess that attachment was "presumably the supported route": it is the route, and it does not work for another instant database. --- CHANGELOG.md | 34 ++++ README.md | 60 ++++++- docs/engine-contract.md | 25 ++- hotdata_langchain/__init__.py | 8 + hotdata_langchain/databases.py | 183 +++++++++++++++++++-- tests/test_attachments.py | 289 +++++++++++++++++++++++++++++++++ 6 files changed, 580 insertions(+), 19 deletions(-) create mode 100644 tests/test_attachments.py diff --git a/CHANGELOG.md b/CHANGELOG.md index d16ec80..217881b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,40 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Added + +- **`hl.attach_catalog` and `hl.detach_catalog`.** An instant database could only ever read its + own tables from this package: every tool set is scoped to one database and there was no way to + bring a second source into that scope without dropping out of the library to the raw SDK. + Attaching a registered connection makes its tables addressable as `..` + alongside the database's own, which the SQL tool description already knows how to describe — + it just had no way to produce that state. `attach_catalog` returns the attachment that landed + rather than the one requested, because the alias is the server's choice when it is not given + and SQL has to address the name that exists. + + Both endpoints answer 204 with no body, so a refusal raises and there is nothing ambiguous + to inspect. What a status code cannot cover is a 204 that did not do the work, so both helpers + re-read the database and raise if the attachment did not land, or if a detached connection is + still listed. That shape is not hypothetical on this platform — `delete_managed_table` reports + success while leaving a registration behind — though it has not been observed for an attach. + Pass `confirm=False` to skip the extra request. + + A 409 from the attach is resolved by reading rather than assumed to mean "already attached": if + the connection turns out to be attached, its existing attachment is returned, so re-running a + provisioning step is a no-op instead of an error. + + Attaching one instant database into another is refused by the platform + (`Connection '' is scoped to another database`); the refusal is passed through with its + message intact. A registered data source is what attaches. + +- **`hl.database_attachments` and `CatalogAttachment`.** `ManagedDatabase` carries only `id`, + `description` and `default_connection_id`, so a caller holding a resolved record could not ask + what was attached to it. This reads the fields that record drops. + +These are Python helpers and deliberately not tools. Whether provisioning of this kind should be +agent-callable is the open question in +[#61](https://github.com/hotdata-dev/hotdata-langchain/issues/61), and nothing here settles it. + ## [0.15.0] - 2026-09-01 ### Added diff --git a/README.md b/README.md index ad685e1..6922b0b 100644 --- a/README.md +++ b/README.md @@ -424,8 +424,11 @@ SQL does what the tool does. An instant database's tables read as `default..
`. An **attached** source's do not — its tables answer to the attachment's alias, and `default..
` is not -found there. Nothing on the database record distinguishes the two, so there is no constant -the tools can assume. +found there. A resolved `ManagedDatabase` distinguishes nothing here, since it carries neither +the attachments nor the default catalog, and `default_catalog` reports `default` whether or not +anything answers to that name. `hl.database_attachments` will name what is attached, but that +is not the same question: it says which sources are attached, not which catalogs actually hold +tables. So there is no constant the tools can assume. `make_hotdata_tools` therefore reads the catalog from `information_schema` once, when the tools are built, and states it in the SQL tool's description — so the model is told the real @@ -783,8 +786,57 @@ the instant-database tools act on the workspace, so naming one database in them `management_tools=False` on the extra sets is worth it for the same reason: listing, creating and loading databases are workspace-wide, so a second copy of them is redundant surface. -Note that a query cannot reach across databases: `SELECT ... FROM other_db.public.t` from within -one database's scope fails with `table not found`. Each set queries its own. +Note that a query cannot reach from one instant database into another: `SELECT ... FROM +other_db.public.t` from within one database's scope fails with `table not found`, and the +platform refuses attaching one instant database into another to get around it. Each set queries +its own. A *registered data source* is a different matter — see below. + +## Attaching another source into a database's scope + +A registered connection can be attached into an instant database, which puts its tables in the +same query scope as the database's own. That is how one query reads across the boundary: + +```python +attachment = hl.attach_catalog(client, db, connection_id="conn...", alias="warehouse") +print(attachment.alias) # the catalog name SQL has to address +``` + +Its tables are then addressable as `warehouse.public.orders` alongside the database's own +tables, and a query can join the two. `hl.detach_catalog(client, db, connection_id="conn...")` +removes the attachment; the connection stays registered and its data is untouched, so this is +reversible. + +**Pass `alias=` if you need to know the name in advance.** Left unset, the server chooses it — +so `attach_catalog` returns the attachment that landed rather than echoing what was asked for, +and that is the name to address. + +Both calls answer 204 with no body, so a refusal raises and there is nothing ambiguous to read. +What that does not cover is a 204 that did not take effect, so each one re-reads the database +afterwards and raises if the attachment did not land, or if a detached connection is still +listed. Pass `confirm=False` to skip the extra request — the returned `alias` is then the one +you asked for rather than the one that landed. + +Attaching a connection that is already attached is a no-op rather than an error, so re-running a +provisioning step is safe. + +To ask what a database already has attached: + +```python +for one in hl.database_attachments(client, db): + print(one.connection_id, one.alias) +``` + +This is a separate call because `ManagedDatabase` carries only `id`, `description` and +`default_connection_id` — a resolved record cannot answer the question on its own. + +A database with something attached exposes more than one catalog, and the SQL tool's description +changes to match: instead of naming a single catalog it tells the model to read `table_catalog` +from `information_schema.tables` to see which one holds a table. Nothing extra is needed to make +that happen — build the tools after attaching. + +These are Python helpers, not tools. Whether an agent should be able to attach a source itself +is [#61](https://github.com/hotdata-dev/hotdata-langchain/issues/61)'s open question, and this +does not answer it. ## Controlling result size diff --git a/docs/engine-contract.md b/docs/engine-contract.md index 29c96a0..24d4cc6 100644 --- a/docs/engine-contract.md +++ b/docs/engine-contract.md @@ -385,10 +385,27 @@ record — nothing in `information_schema` marks the generated column as generat call; the same client read from two different instant databases in one session. - **Cross-database references inside a single query fail** by default: `SELECT id FROM f1_db.public.drivers` from within another database's scope gives - `table 'f1_db.public.drivers' not found`. `DatabasesApi` does expose - `attach_database_catalog`/`detach_database_catalog`, and `bm25_search`'s scope resolution - translates "an attachment alias or `default`", so attachment is presumably the supported route - — **not verified here**. + `table 'f1_db.public.drivers' not found`. + +- **Attachment is the supported route across that boundary, but only for a registered source** + (verified 2026-08-31/09-01 against the live workspace, through `DatabasesApi` directly): + + | Attempt | Result | + |---|---| + | attach instant database A into instant database B | **refused** — `Connection '' is scoped to another database and cannot be attached here` | + | load a `result_id` from A's query into a table in A | accepted, 1 row landed | + | load that same `result_id` into B | **refused** — `Result '' not found` | + | attach a **Postgres connection** into an instant database | accepted | + | `SELECT * FROM .public.drivers` through that alias | **859 rows**, from inside the instant database's scope | + + So two instant databases cannot see each other by any route, and the earlier guess that + attachment was "presumably the supported route" was half right: it is the route, and it does + not work for another instant database. `ResultsApi.get_result` requires an `x_database_id`, + which is the same boundary expressed in a signature. + + `hl.attach_catalog`/`hl.detach_catalog` wrap that same endpoint pair. The endpoints are + verified as above; **the helpers themselves have not been exercised against a live + workspace.** - **Database names are not unique.** `name` is a display label; `resolve_managed_database` tries the id first and then scans `list_databases()` matching on name. Ids are the only safe handle, so this package never calls that resolver: `resolve_database_by_id` goes straight to diff --git a/hotdata_langchain/__init__.py b/hotdata_langchain/__init__.py index 1a47d7f..955bffd 100644 --- a/hotdata_langchain/__init__.py +++ b/hotdata_langchain/__init__.py @@ -18,8 +18,12 @@ from hotdata_langchain._sql import DISTANCE_FUNCTIONS, DistanceMetric from hotdata_langchain.databases import ( + CatalogAttachment, LoadMode, + attach_catalog, create_managed_database, + database_attachments, + detach_catalog, list_managed_databases_json, load_managed_table, load_result_summary, @@ -111,6 +115,7 @@ "SEARCH_NOUNS", "SEMANTIC", "TEXT", + "CatalogAttachment", "DistanceMetric", "Fusion", "HotdataClient", @@ -126,11 +131,14 @@ "TablePartitionKey", "TableSortKey", "__version__", + "attach_catalog", "bm25_search_json", "bm25_search_sql", "capabilities_by_column", "create_managed_database", + "database_attachments", "describe_tables_json", + "detach_catalog", "engine_error_message", "error_feedback", "execute_sql_json", diff --git a/hotdata_langchain/databases.py b/hotdata_langchain/databases.py index 550dab9..2031498 100644 --- a/hotdata_langchain/databases.py +++ b/hotdata_langchain/databases.py @@ -8,6 +8,7 @@ import socket import tempfile from collections.abc import Sequence +from dataclasses import dataclass from pathlib import Path from typing import Any, Literal from urllib.parse import urlsplit @@ -15,6 +16,7 @@ from hotdata.api.databases_api import DatabasesApi from hotdata.exceptions import ApiException +from hotdata.models.attach_database_catalog_request import AttachDatabaseCatalogRequest from hotdata_framework import ( DEFAULT_SCHEMA, HotdataClient, @@ -27,6 +29,21 @@ logger = logging.getLogger(__name__) + +@dataclass(frozen=True) +class CatalogAttachment: + """A data source attached into an instant database's query scope. + + ``alias`` is the catalog name the attached tables answer to in SQL, so a query reads + ``..
``. It is ``None`` when the attachment was made without one + and the server chose the name, which is why :func:`attach_catalog` reports back what + landed rather than echoing what was asked for. + """ + + connection_id: str + alias: str | None + + CATALOG_QUERY = ( "SELECT DISTINCT table_catalog FROM information_schema.tables " "WHERE table_schema <> 'information_schema'" @@ -69,17 +86,7 @@ def resolve_database_by_id( """ if isinstance(database_id, ManagedDatabase): return database_id - try: - detail = DatabasesApi(client.api).get_database(database_id) - except ApiException as e: - if e.status == 404: - raise KeyError( - f"no instant database with id {database_id!r} in this workspace. " - "Ids are listed by hotdata_list_managed_databases; a database name is " - "not accepted here, because names are not unique." - ) from e - raise RuntimeError(api_error_message(e)) from e - return managed_database_from_detail(detail) + return managed_database_from_detail(_database_detail(client, database_id)) def query_scope(database: ManagedDatabase | None) -> ManagedDatabase | None: @@ -153,6 +160,160 @@ def query_catalogs(client: HotdataClient, database: ManagedDatabase) -> list[str return catalogs +def _database_id(database_id: str | ManagedDatabase) -> str: + return database_id.id if isinstance(database_id, ManagedDatabase) else database_id + + +def _database_detail(client: HotdataClient, database_id: str | ManagedDatabase) -> Any: + identifier = _database_id(database_id) + try: + return DatabasesApi(client.api).get_database(identifier) + except ApiException as e: + if e.status == 404: + raise KeyError( + f"no instant database with id {identifier!r} in this workspace. " + "Ids are listed by hotdata_list_managed_databases; a database name is " + "not accepted here, because names are not unique." + ) from e + raise RuntimeError(api_error_message(e)) from e + + +def database_attachments( + client: HotdataClient, + database_id: str | ManagedDatabase, +) -> list[CatalogAttachment]: + """Return the data sources attached into ``database_id``'s query scope. + + ``GET /databases/{id}`` reports these, but ``ManagedDatabase`` carries only ``id``, + ``description`` and ``default_connection_id`` — so a caller holding a resolved record + cannot ask what is attached to it. This re-reads the detail response for the fields + that record drops. + + A database with nothing attached returns an empty list. Raises ``KeyError`` when the + workspace has no database with that id. + """ + detail = _database_detail(client, database_id) + return [ + CatalogAttachment(connection_id=str(one.connection_id), alias=one.alias) + for one in getattr(detail, "attachments", None) or () + ] + + +def _attached_connection( + client: HotdataClient, + identifier: str, + connection_id: str, +) -> CatalogAttachment | None: + """Return ``connection_id``'s attachment on ``identifier``, or ``None`` if absent.""" + for one in database_attachments(client, identifier): + if one.connection_id == connection_id: + return one + return None + + +def attach_catalog( + client: HotdataClient, + database_id: str | ManagedDatabase, + *, + connection_id: str, + alias: str | None = None, + confirm: bool = True, +) -> CatalogAttachment: + """Attach a registered connection into an instant database, as a second catalog. + + This is what makes one database read across a boundary: the attached source's tables + become addressable as ``..
`` inside ``database_id``'s scope, + alongside its own. ``connection_id`` is a registered data source, not another instant + database — the platform refuses a managed database's own connection here with + "scoped to another database and cannot be attached". + + ``alias`` names the catalog in SQL. Left unset the server chooses it, so read the + returned attachment rather than assuming the name. + + The endpoint answers 204 with no body, so a failure raises rather than returning + anything to inspect. ``confirm`` adds a read-back on top of that, which covers the one + case the status code cannot: a 204 that did not do the work. That shape is not + hypothetical here — ``delete_managed_table`` reports success while leaving a + registration behind — though it has not been observed for an attach. Pass + ``confirm=False`` to skip the extra request, and note that the returned ``alias`` is + then the one asked for rather than the one that landed. + + A 409 is resolved by reading rather than assumed: if the connection turns out to be + attached already, its existing attachment is returned, so re-running a provisioning + step is a no-op instead of an error. + + Raises ``KeyError`` when the workspace has no database with that id, and + ``RuntimeError`` when the attach is refused or reports success without landing. + """ + identifier = _database_id(database_id) + try: + DatabasesApi(client.api).attach_database_catalog( + identifier, + AttachDatabaseCatalogRequest(connection_id=connection_id, alias=alias), + ) + except ApiException as e: + if e.status == 409: + existing = _attached_connection(client, identifier, connection_id) + if existing is not None: + logger.debug( + "connection %s is already attached to database %s as %r", + connection_id, + identifier, + existing.alias, + ) + return existing + raise RuntimeError(api_error_message(e)) from e + + if not confirm: + return CatalogAttachment(connection_id=connection_id, alias=alias) + landed = _attached_connection(client, identifier, connection_id) + if landed is None: + raise RuntimeError( + f"attaching connection {connection_id!r} to database {identifier!r} reported " + "no error, but the database reports it is not attached." + ) + return landed + + +def detach_catalog( + client: HotdataClient, + database_id: str | ManagedDatabase, + *, + connection_id: str, + confirm: bool = True, +) -> None: + """Detach a connection from an instant database's query scope. + + Removes the attachment only. The connection itself stays registered in the workspace + and any data it holds is untouched, so this is reversible by attaching again — unlike + deleting a table, which burns its name. + + ``confirm`` re-reads the database and raises if the connection is still attached, for + the same reason :func:`attach_catalog` does: 204 says the call was accepted, not that + it took effect. Pass ``confirm=False`` to skip the extra request. + + A 404 covers both "no such database" and "that connection is not attached to it", and + the two are not distinguishable from the response, so the ``KeyError`` names both + rather than asserting one. + """ + identifier = _database_id(database_id) + try: + DatabasesApi(client.api).detach_database_catalog(identifier, connection_id) + except ApiException as e: + if e.status == 404: + raise KeyError( + f"no instant database with id {identifier!r} in this workspace, or no " + f"connection {connection_id!r} attached to it." + ) from e + raise RuntimeError(api_error_message(e)) from e + + if confirm and _attached_connection(client, identifier, connection_id) is not None: + raise RuntimeError( + f"detaching connection {connection_id!r} from database {identifier!r} " + "reported no error, but the database still reports it as attached." + ) + + def list_managed_databases_json(client: HotdataClient) -> str: """List this workspace's instant databases as JSON, each with its ``id`` and ``name``. diff --git a/tests/test_attachments.py b/tests/test_attachments.py new file mode 100644 index 0000000..de74204 --- /dev/null +++ b/tests/test_attachments.py @@ -0,0 +1,289 @@ +"""Attaching a registered connection into an instant database's query scope. + +A managed database cannot see another managed database — the platform refuses its +connection with "scoped to another database". A *registered* source attaches as designed, +which is the route across the boundary. + +Both endpoints answer 204 with no body, so a refusal raises and there is no ambiguous +return to read. What the status cannot cover is a 204 that did not do the work, which is +what the read-back guards against and what most of these tests exercise. +""" + +from __future__ import annotations + +from collections.abc import Iterator +from types import SimpleNamespace +from unittest.mock import MagicMock, patch + +import pytest +from hotdata.exceptions import ApiException +from hotdata_framework import ManagedDatabase + +from hotdata_langchain.databases import ( + CatalogAttachment, + attach_catalog, + database_attachments, + detach_catalog, +) + +CONNECTION = "connpg000000000000000000000001" + + +def detail_with(managed_db: ManagedDatabase, *attachments: SimpleNamespace) -> SimpleNamespace: + """A ``GET /databases/{id}`` response carrying the fields ``ManagedDatabase`` drops.""" + return SimpleNamespace( + id=managed_db.id, + name=managed_db.description, + default_connection_id=managed_db.default_connection_id, + default_catalog="default", + default_schema="public", + attachments=list(attachments), + ) + + +def attachment(connection_id: str = CONNECTION, alias: str | None = "warehouse") -> SimpleNamespace: + return SimpleNamespace(connection_id=connection_id, alias=alias) + + +@pytest.fixture +def attached(managed_db: ManagedDatabase) -> Iterator[MagicMock]: + """Patch the raw databases API, reporting nothing attached until a test says otherwise.""" + with patch("hotdata_langchain.databases.DatabasesApi") as api: + api.return_value.get_database.return_value = detail_with(managed_db) + yield api + + +# --- reading back what the resolved record drops ------------------------------------- + + +def test_attachments_are_readable_even_though_the_resolved_record_drops_them( + mock_client: MagicMock, managed_db: ManagedDatabase, attached: MagicMock +) -> None: + """``ManagedDatabase`` carries no attachments field at all, so this is the only route.""" + assert not hasattr(managed_db, "attachments") + attached.return_value.get_database.return_value = detail_with(managed_db, attachment()) + + assert database_attachments(mock_client, managed_db.id) == [ + CatalogAttachment(connection_id=CONNECTION, alias="warehouse") + ] + + +def test_a_database_with_nothing_attached_reports_an_empty_list( + mock_client: MagicMock, managed_db: ManagedDatabase, attached: MagicMock +) -> None: + assert database_attachments(mock_client, managed_db.id) == [] + + +def test_attachments_accepts_an_already_resolved_record( + mock_client: MagicMock, managed_db: ManagedDatabase, attached: MagicMock +) -> None: + database_attachments(mock_client, managed_db) + attached.return_value.get_database.assert_called_once_with(managed_db.id) + + +def test_attachments_raises_keyerror_for_an_unknown_database( + mock_client: MagicMock, attached: MagicMock +) -> None: + attached.return_value.get_database.side_effect = ApiException(status=404, reason="Not Found") + with pytest.raises(KeyError, match="not accepted here"): + database_attachments(mock_client, "dbid000000000000000000000000x") + + +# --- attaching ----------------------------------------------------------------------- + + +def test_attach_sends_the_connection_and_the_requested_alias( + mock_client: MagicMock, managed_db: ManagedDatabase, attached: MagicMock +) -> None: + attached.return_value.get_database.return_value = detail_with(managed_db, attachment()) + + attach_catalog(mock_client, managed_db.id, connection_id=CONNECTION, alias="warehouse") + + database_id, request = attached.return_value.attach_database_catalog.call_args.args + assert database_id == managed_db.id + assert request.connection_id == CONNECTION + assert request.alias == "warehouse" + + +def test_attach_reports_the_alias_that_landed_not_the_one_requested( + mock_client: MagicMock, managed_db: ManagedDatabase, attached: MagicMock +) -> None: + """Left unset the server names the catalog, and SQL has to address that name.""" + attached.return_value.get_database.return_value = detail_with( + managed_db, attachment(alias="pg_main") + ) + + landed = attach_catalog(mock_client, managed_db.id, connection_id=CONNECTION) + + assert landed == CatalogAttachment(connection_id=CONNECTION, alias="pg_main") + assert attached.return_value.attach_database_catalog.call_args.args[1].alias is None + + +def test_attach_raises_when_the_call_succeeds_but_nothing_was_attached( + mock_client: MagicMock, managed_db: ManagedDatabase, attached: MagicMock +) -> None: + """A 204 says the call was accepted, not that it took effect.""" + attached.return_value.get_database.return_value = detail_with(managed_db) + + with pytest.raises(RuntimeError, match="reports it is not attached"): + attach_catalog(mock_client, managed_db.id, connection_id=CONNECTION) + + +def test_attach_ignores_an_unrelated_connection_already_attached( + mock_client: MagicMock, managed_db: ManagedDatabase, attached: MagicMock +) -> None: + attached.return_value.get_database.return_value = detail_with( + managed_db, attachment(connection_id="connother00000000000000000001", alias="other") + ) + + with pytest.raises(RuntimeError, match="reports it is not attached"): + attach_catalog(mock_client, managed_db.id, connection_id=CONNECTION) + + +def test_attach_surfaces_the_platforms_refusal_message( + mock_client: MagicMock, managed_db: ManagedDatabase, attached: MagicMock +) -> None: + """Attaching a managed database's own connection is refused, and the reason matters.""" + attached.return_value.attach_database_catalog.side_effect = ApiException( + status=400, + reason="Bad Request", + body="Connection 'conn123' is scoped to another database and cannot be attached here", + ) + + with pytest.raises(RuntimeError, match="scoped to another database"): + attach_catalog(mock_client, managed_db.id, connection_id=CONNECTION) + + +def test_attach_accepts_an_already_resolved_record( + mock_client: MagicMock, managed_db: ManagedDatabase, attached: MagicMock +) -> None: + attached.return_value.get_database.return_value = detail_with(managed_db, attachment()) + + attach_catalog(mock_client, managed_db, connection_id=CONNECTION) + + assert attached.return_value.attach_database_catalog.call_args.args[0] == managed_db.id + + +# --- detaching ----------------------------------------------------------------------- + + +def test_detach_passes_the_database_and_connection_ids( + mock_client: MagicMock, managed_db: ManagedDatabase, attached: MagicMock +) -> None: + detach_catalog(mock_client, managed_db.id, connection_id=CONNECTION) + attached.return_value.detach_database_catalog.assert_called_once_with(managed_db.id, CONNECTION) + + +def test_detach_translates_a_404_into_a_keyerror( + mock_client: MagicMock, managed_db: ManagedDatabase, attached: MagicMock +) -> None: + attached.return_value.detach_database_catalog.side_effect = ApiException( + status=404, reason="Not Found" + ) + with pytest.raises(KeyError, match="attached to it"): + detach_catalog(mock_client, managed_db.id, connection_id=CONNECTION) + + +def test_detach_surfaces_a_non_404_failure_as_a_runtimeerror( + mock_client: MagicMock, managed_db: ManagedDatabase, attached: MagicMock +) -> None: + attached.return_value.detach_database_catalog.side_effect = ApiException( + status=403, reason="Forbidden", body="workspace does not permit detaching" + ) + with pytest.raises(RuntimeError, match="workspace does not permit detaching"): + detach_catalog(mock_client, managed_db.id, connection_id=CONNECTION) + + +# --- confirmation, and what 204 does not tell you ------------------------------------ + + +def test_attach_skips_the_read_back_when_confirmation_is_off( + mock_client: MagicMock, managed_db: ManagedDatabase, attached: MagicMock +) -> None: + """The only GET here would be the confirmation, so its absence is the assertion.""" + landed = attach_catalog( + mock_client, managed_db.id, connection_id=CONNECTION, alias="warehouse", confirm=False + ) + + assert landed == CatalogAttachment(connection_id=CONNECTION, alias="warehouse") + attached.return_value.get_database.assert_not_called() + + +def test_attach_without_confirmation_reports_the_requested_alias_not_the_landed_one( + mock_client: MagicMock, managed_db: ManagedDatabase, attached: MagicMock +) -> None: + """Unconfirmed, there is nothing to read the server's choice from.""" + attached.return_value.get_database.return_value = detail_with( + managed_db, attachment(alias="pg_main") + ) + + landed = attach_catalog(mock_client, managed_db.id, connection_id=CONNECTION, confirm=False) + + assert landed.alias is None + + +def test_attach_treats_an_already_attached_connection_as_a_no_op( + mock_client: MagicMock, managed_db: ManagedDatabase, attached: MagicMock +) -> None: + """Re-running a provisioning step should not fail on the attach it already did.""" + attached.return_value.attach_database_catalog.side_effect = ApiException( + status=409, reason="Conflict", body="already attached" + ) + attached.return_value.get_database.return_value = detail_with( + managed_db, attachment(alias="warehouse") + ) + + landed = attach_catalog(mock_client, managed_db.id, connection_id=CONNECTION) + + assert landed == CatalogAttachment(connection_id=CONNECTION, alias="warehouse") + + +def test_a_conflict_that_is_not_an_existing_attachment_still_raises( + mock_client: MagicMock, managed_db: ManagedDatabase, attached: MagicMock +) -> None: + """409 is resolved by reading, not assumed to mean 'already attached'.""" + attached.return_value.attach_database_catalog.side_effect = ApiException( + status=409, reason="Conflict", body="alias 'warehouse' is already in use" + ) + attached.return_value.get_database.return_value = detail_with(managed_db) + + with pytest.raises(RuntimeError, match="already in use"): + attach_catalog(mock_client, managed_db.id, connection_id=CONNECTION) + + +def test_detach_raises_when_the_connection_is_still_attached_afterwards( + mock_client: MagicMock, managed_db: ManagedDatabase, attached: MagicMock +) -> None: + """204 says the call was accepted, not that it took effect.""" + attached.return_value.get_database.return_value = detail_with(managed_db, attachment()) + + with pytest.raises(RuntimeError, match="still reports it as attached"): + detach_catalog(mock_client, managed_db.id, connection_id=CONNECTION) + + +def test_detach_confirms_by_reading_the_database_back( + mock_client: MagicMock, managed_db: ManagedDatabase, attached: MagicMock +) -> None: + detach_catalog(mock_client, managed_db.id, connection_id=CONNECTION) + attached.return_value.get_database.assert_called_once_with(managed_db.id) + + +def test_detach_skips_the_read_back_when_confirmation_is_off( + mock_client: MagicMock, managed_db: ManagedDatabase, attached: MagicMock +) -> None: + attached.return_value.get_database.return_value = detail_with(managed_db, attachment()) + + detach_catalog(mock_client, managed_db.id, connection_id=CONNECTION, confirm=False) + + attached.return_value.get_database.assert_not_called() + + +def test_detach_leaving_an_unrelated_attachment_in_place_succeeds( + mock_client: MagicMock, managed_db: ManagedDatabase, attached: MagicMock +) -> None: + """Confirmation matches on the connection detached, not on the list being empty.""" + attached.return_value.get_database.return_value = detail_with( + managed_db, attachment(connection_id="connother00000000000000000001", alias="other") + ) + + detach_catalog(mock_client, managed_db.id, connection_id=CONNECTION) From e6fb5bef06125247c8190395a6e3966979772827 Mon Sep 17 00:00:00 2001 From: Rohan Dsouza Date: Thu, 3 Sep 2026 17:59:41 +0530 Subject: [PATCH 2/3] fix: translate a 404 from the attach the way the detach does attach_catalog's docstring promised KeyError for an unknown database id, but its except block handled 409 only, so every other status became RuntimeError. The read-back that would have raised KeyError is never reached on that path: the attach call fails first. detach_catalog already translated 404, so the two helpers disagreed on the same failure. A 404 here does not say whether the database or the connection is missing, so the message names both, as detach's does. Also from review: - Read detail.attachments directly rather than through getattr. The field is required on DatabaseDetailResponse, so the default never applies -- and if the SDK dropped it, the default would turn that into "nothing attached" for a database that has attachments, and an unlanded-attach error after an attach that landed. - Annotate _database_detail as DatabaseDetailResponse rather than Any. hotdata ships py.typed and mypy runs strict with disallow_any_unimported, so Any was silently dropping the check on managed_database_from_detail's argument that the inline call had. - Name confirm=False in the unlanded-attach message, matching this module's habit of carrying the remedy in the error text. - Fix CatalogAttachment's alias docstring, which contradicted attach_catalog by saying alias is None when the server names the catalog. --- hotdata_langchain/databases.py | 41 +++++++++++++++++++++---------- tests/test_attachments.py | 45 ++++++++++++++++++++++++++++++++++ 2 files changed, 73 insertions(+), 13 deletions(-) diff --git a/hotdata_langchain/databases.py b/hotdata_langchain/databases.py index 2031498..5328aa8 100644 --- a/hotdata_langchain/databases.py +++ b/hotdata_langchain/databases.py @@ -17,6 +17,7 @@ from hotdata.api.databases_api import DatabasesApi from hotdata.exceptions import ApiException from hotdata.models.attach_database_catalog_request import AttachDatabaseCatalogRequest +from hotdata.models.database_detail_response import DatabaseDetailResponse from hotdata_framework import ( DEFAULT_SCHEMA, HotdataClient, @@ -35,9 +36,10 @@ class CatalogAttachment: """A data source attached into an instant database's query scope. ``alias`` is the catalog name the attached tables answer to in SQL, so a query reads - ``..
``. It is ``None`` when the attachment was made without one - and the server chose the name, which is why :func:`attach_catalog` reports back what - landed rather than echoing what was asked for. + ``..
``. The API declares it optional, so it can be ``None``. + Attaching without one leaves the naming to the server, which is why + :func:`attach_catalog` reports back the attachment it read rather than echoing what + was asked for. """ connection_id: str @@ -164,17 +166,23 @@ def _database_id(database_id: str | ManagedDatabase) -> str: return database_id.id if isinstance(database_id, ManagedDatabase) else database_id -def _database_detail(client: HotdataClient, database_id: str | ManagedDatabase) -> Any: +def _no_such_database(identifier: str) -> KeyError: + return KeyError( + f"no instant database with id {identifier!r} in this workspace. " + "Ids are listed by hotdata_list_managed_databases; a database name is " + "not accepted here, because names are not unique." + ) + + +def _database_detail( + client: HotdataClient, database_id: str | ManagedDatabase +) -> DatabaseDetailResponse: identifier = _database_id(database_id) try: return DatabasesApi(client.api).get_database(identifier) except ApiException as e: if e.status == 404: - raise KeyError( - f"no instant database with id {identifier!r} in this workspace. " - "Ids are listed by hotdata_list_managed_databases; a database name is " - "not accepted here, because names are not unique." - ) from e + raise _no_such_database(identifier) from e raise RuntimeError(api_error_message(e)) from e @@ -195,7 +203,7 @@ def database_attachments( detail = _database_detail(client, database_id) return [ CatalogAttachment(connection_id=str(one.connection_id), alias=one.alias) - for one in getattr(detail, "attachments", None) or () + for one in detail.attachments or () ] @@ -242,8 +250,9 @@ def attach_catalog( attached already, its existing attachment is returned, so re-running a provisioning step is a no-op instead of an error. - Raises ``KeyError`` when the workspace has no database with that id, and - ``RuntimeError`` when the attach is refused or reports success without landing. + Raises ``KeyError`` when the database or the connection does not exist — a 404 here + does not say which, so the message names both — and ``RuntimeError`` when the attach + is refused or reports success without landing. """ identifier = _database_id(database_id) try: @@ -252,6 +261,11 @@ def attach_catalog( AttachDatabaseCatalogRequest(connection_id=connection_id, alias=alias), ) except ApiException as e: + if e.status == 404: + raise KeyError( + f"no instant database with id {identifier!r} in this workspace, or no " + f"connection {connection_id!r} registered in it." + ) from e if e.status == 409: existing = _attached_connection(client, identifier, connection_id) if existing is not None: @@ -270,7 +284,8 @@ def attach_catalog( if landed is None: raise RuntimeError( f"attaching connection {connection_id!r} to database {identifier!r} reported " - "no error, but the database reports it is not attached." + "no error, but the database reports it is not attached. Pass confirm=False to " + "accept the call's own result instead of this read-back." ) return landed diff --git a/tests/test_attachments.py b/tests/test_attachments.py index de74204..4ab2133 100644 --- a/tests/test_attachments.py +++ b/tests/test_attachments.py @@ -287,3 +287,48 @@ def test_detach_leaving_an_unrelated_attachment_in_place_succeeds( ) detach_catalog(mock_client, managed_db.id, connection_id=CONNECTION) + + +# --- the 404 contract, which attach and detach must state the same way --------------- + + +def test_attach_translates_a_404_into_a_keyerror( + mock_client: MagicMock, managed_db: ManagedDatabase, attached: MagicMock +) -> None: + """The attach fails before the read-back, so this path never reaches _database_detail.""" + attached.return_value.attach_database_catalog.side_effect = ApiException( + status=404, reason="Not Found" + ) + + with pytest.raises(KeyError) as excinfo: + attach_catalog(mock_client, "dbid000000000000000000000000x", connection_id=CONNECTION) + + message = str(excinfo.value) + assert "no instant database" in message + assert CONNECTION in message + + +def test_attach_and_detach_agree_on_which_error_a_404_is( + mock_client: MagicMock, managed_db: ManagedDatabase, attached: MagicMock +) -> None: + """A caller wrapping both in one try block should not need two except clauses.""" + attached.return_value.attach_database_catalog.side_effect = ApiException( + status=404, reason="Not Found" + ) + attached.return_value.detach_database_catalog.side_effect = ApiException( + status=404, reason="Not Found" + ) + + for call in (attach_catalog, detach_catalog): + with pytest.raises(KeyError): + call(mock_client, managed_db.id, connection_id=CONNECTION) + + +def test_the_unlanded_attach_message_names_the_way_out( + mock_client: MagicMock, managed_db: ManagedDatabase, attached: MagicMock +) -> None: + """Error text in this module carries its own remedy.""" + attached.return_value.get_database.return_value = detail_with(managed_db) + + with pytest.raises(RuntimeError, match="confirm=False"): + attach_catalog(mock_client, managed_db.id, connection_id=CONNECTION) From 604bf6a72ab3425fea4a157e4a3f0e377460b28e Mon Sep 17 00:00:00 2001 From: Rohan Dsouza Date: Thu, 3 Sep 2026 18:21:45 +0530 Subject: [PATCH 3/3] fix: say a connection is registered in the workspace, not in the database The attach 404 message said "no connection registered in it", where "it" is the database. A connection is registered in the workspace and then attached into a database, which detach_catalog's own docstring already states. The message pointed a reader looking for the connection at the wrong place. --- hotdata_langchain/databases.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hotdata_langchain/databases.py b/hotdata_langchain/databases.py index 5328aa8..a654f9c 100644 --- a/hotdata_langchain/databases.py +++ b/hotdata_langchain/databases.py @@ -264,7 +264,7 @@ def attach_catalog( if e.status == 404: raise KeyError( f"no instant database with id {identifier!r} in this workspace, or no " - f"connection {connection_id!r} registered in it." + f"connection {connection_id!r} registered in this workspace." ) from e if e.status == 409: existing = _attached_connection(client, identifier, connection_id)