feat: attach a registered source into a database's scope, and read back when it expires - #97
Conversation
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.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| identifier, | ||
| AttachDatabaseCatalogRequest(connection_id=connection_id, alias=alias), | ||
| ) | ||
| except ApiException as e: |
There was a problem hiding this comment.
Translate a 404 from attach_database_catalog into KeyError here, the way detach_catalog does at line 303.
The docstring at line 245 promises KeyError when the workspace has no database with that id. This except block handles 409 only, so every other status becomes RuntimeError. attach_catalog never reaches _database_detail on that path, because the attach call fails first.
Failure scenario: a caller passes a deleted database id. The attach endpoint answers 404. attach_catalog raises RuntimeError. A caller following the docstring wraps the call in except KeyError. The RuntimeError escapes and crashes the caller.
No test covers attach_catalog against a 404, so the mismatch is unguarded. Correct the docstring instead if RuntimeError is the intended contract.
| 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 () |
There was a problem hiding this comment.
nit: read detail.attachments directly rather than through getattr (not blocking).
The PR description records that attachments exists on the detail response at the declared floor, so the default never applies today. If the SDK renames or drops the field, the default turns that into an empty list: database_attachments reports "nothing attached" for a database that has attachments, and attach_catalog raises "reports it is not attached" after an attach that landed. Direct access fails loudly instead. or () still covers a None value.
| raise RuntimeError( | ||
| f"attaching connection {connection_id!r} to database {identifier!r} reported " | ||
| "no error, but the database reports it is not attached." | ||
| ) |
There was a problem hiding this comment.
nit: name confirm=False in this message (not blocking).
The read-back can also come back empty because the detail response lags behind the attach. A caller cannot tell that case from an attach that did not land, and the message points at no escape hatch. Error text in this module carries its remedy already: _database_detail names hotdata_list_managed_databases at line 175.
| return database_id.id if isinstance(database_id, ManagedDatabase) else database_id | ||
|
|
||
|
|
||
| def _database_detail(client: HotdataClient, database_id: str | ManagedDatabase) -> Any: |
There was a problem hiding this comment.
nit: annotate the return with the SDK detail model rather than Any (not blocking). Fix this only if hotdata ships type information for DatabasesApi.get_database, since disallow_any_unimported is set.
resolve_database_by_id passes this value straight into managed_database_from_detail. Under Any, mypy no longer checks that argument, and mypy runs with strict = true in pyproject.toml. The inline call this replaced was checked.
| ``alias`` is the catalog name the attached tables answer to in SQL, so a query reads | ||
| ``<alias>.<schema>.<table>``. 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. |
There was a problem hiding this comment.
super nit: this explanation of alias contradicts attach_catalog (not blocking). The text says alias is None when the server chose the name. attach_catalog returns the attachment read back from the server, and test_attach_reports_the_alias_that_landed_not_the_one_requested has a server-chosen alias arriving as pg_main. Say instead that the API declares alias optional.
There was a problem hiding this comment.
Review
Blocking Issues
hotdata_langchain/databases.py:254—attach_catalogdoes not translate a 404 fromattach_database_catalogintoKeyError. The docstring at line 245 promisesKeyErrorfor an unknown database id. The code raisesRuntimeErrorinstead, because the except block handles 409 only.detach_catalogtranslates 404 at line 303, so the two helpers disagree. No test coversattach_catalogagainst a 404.
Action Required
Handle 404 in the attach_catalog except block the way detach_catalog does, and add a test for it. Correct the docstring instead if RuntimeError is the intended contract.
Four non-blocking comments are inline.
CI note: Lint and types, Test (Python 3.13) and Test (Python 3.14) were still pending when this review started. Their results are unknown here.
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.
|
All five findings addressed in e6fb5be. Blocking — 404 on attach. Correct, and it was a real contract break: the docstring promised
Re-run locally: 622 passed (up 3), mypy clean with the real annotation, ruff clean, and the One note on the CI caveat in the review: |
| 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." |
There was a problem hiding this comment.
super nit: say "registered in this workspace" rather than "registered in it" (not blocking).
A connection is registered in the workspace, not in the database. detach_catalog's docstring at line 302 states that: "The connection itself stays registered in the workspace". The attach message points a reader looking for the connection at the wrong place.
There was a problem hiding this comment.
Prior blocking finding is fixed: attach_catalog now translates a 404 into KeyError, and two tests cover it. The four nits are addressed as well. One super nit inline, not blocking.
CI checks were still queued or in progress at review time, so this approval does not rest on their results.
…base The attach 404 message said "no connection <id> 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.
rohan-hotdata
changed the title
1 participant
Refs #90.
Every tool set in this package is scoped to exactly one instant database — resolved once at
build time, one
X-Database-Idper query. There was no way to bring a second source into thatscope without leaving the library for the raw SDK. This adds
hl.attach_catalog,hl.detach_catalogandhl.database_attachments.Where the capability was
Not missing — one layer down:
attach_database_catalog?hotdatahotdata-frameworkhotdata-langchainHotdataClientexposes itsApiClientas a public.apiproperty and builds its ownDatabasesApi(self._api)from it internally, so going throughDatabasesApi(client.api).attach_database_catalog(...)is the same construction the frameworkmakes rather than a way around it — and it is the route
resolve_database_by_idalready takes.That keeps this shippable now. Waiting for the method to land on the client would mean a
framework release plus a floor bump to
>=0.14.0, and the floor only moved to 0.13.0 lastrelease. It is also the second place this package reaches past the client, which is a gap in the
layer below rather than good use of an escape hatch — filed separately on
sdk-python-framework, and when it lands these become thin delegations.Two decisions that came out of the API, not preference
Confirmation is on by default. Both endpoints answer
204with no body, mapping400/404(and409on attach) toApiErrorResponse— so a refusal raises and there isnothing ambiguous to inspect.
Noneis the success signal. What a status code cannot cover isa 204 that did not do the work, and that shape is measured on this platform:
delete_managed_tablereports success while leaving a connection registration behind (#36). Ithas not been observed for an attach, so this is a cheap guard against a known failure mode
rather than a fix for something seen. Each helper re-reads and raises;
confirm=Falseskips theextra request, and the returned
aliasis then the one requested rather than the one thatlanded, since there is nothing to read it from.
A 409 is resolved by reading, not assumed. Rather than guessing that it means "already
attached", the attach re-reads: if the connection is attached, that existing attachment is
returned and the call is a no-op; if it is not, the original message is raised. Safe whichever
way the platform uses 409, and it makes re-running a provisioning step idempotent.
The confirmation matches on the specific
connection_idrather than on the attachment listbeing empty, so detaching one of two attachments succeeds. Both cases are tested.
Why
database_attachmentsis part of this and not a follow-upIt is what makes an attach verifiable.
ManagedDatabasecarriesid,descriptionanddefault_connection_id; the detail response it is built from also carriesattachments,default_catalog,default_schemaandexpires_at, andmanaged_database_from_detaildiscards all four.
That function lives in
hotdata_framework, so this package cannot fix the discarding in place —it re-reads the detail response instead and maps
attachmentsinto a frozenCatalogAttachment,so callers do not depend on an SDK model. This satisfies #90's second ask without fixing its
cause, which is why I have not closed that issue: the upstream half is in the framework ticket.
Also collapsed the duplicated
GET /databases/{id}error handling into_database_detail, whichresolve_database_by_idnow shares — it drops from twelve lines to one, and the three call sitescannot drift apart on 404 translation.
Docs
reach across databases … Each set queries its own" now names the platform's refusal explicitly
rather than reading as a blanket impossibility. And "Nothing on the database record
distinguishes the two" was made imprecise by this very change: the detail response does carry
attachments, it isManagedDatabasethat drops them. Reworded to say which record carrieswhat, and why
information_schemais still the right source — attachments say which sourcesare attached, not which catalogs hold tables.
docs/engine-contract.md— replaces its guess that attachment was "presumably thesupported route — not verified here" with the measured table. The guess was half right:
attachment is the route, and it does not work for another instant database. Two instant
databases cannot see each other by any route, including via
result_id. Marked per thatdoc's convention: the endpoints are verified, and the helpers added here are not yet
exercised against a live workspace.
[Unreleased].Also here: reading back when a database expires
hl.database_expiryandhl.database_expiries. Same shape of gap as the attachments one, andfound the same way.
A lifetime is written as a string, an RFC 3339 timestamp or a relative window such as
"24h",and the server resolves it to an instant: the request field is
str, the response field isdatetime. So the resolved time is only knowable by reading it back, and nothing here could —ManagedDatabasecarries noexpires_at. A caller could set a TTL and never learn which secondit landed on, or whether a database still had one.
database_expiriesreturns the workspace keyed by database id, at one request per page of thelisting rather than one per database, because
DatabaseSummaryalready carries the field. Adatabase with no TTL maps to
None, so "lives forever" stays distinguishable from "not in thisworkspace", which a missing key would not separate.
It reads
DatabasesApi.list_databasesdirectly rather than going throughclient.list_managed_databases(), which has three problems for this purpose: it returnslist[ManagedDatabase]and so dropsexpires_at, it callsget_databaseonce per database, andit wraps each of those in
except ApiException: pass, so a database whose detail read fails issilently omitted. That method is unchanged here and its defects are noted on #36 rather than
fixed in this PR.
The listing is paginated (
limit,cursor,search), so the cursor is followed. A single-pageread would report a subset as though it were the whole workspace. The loop stops on an empty page
and caps at 10,000 records, so a server that keeps returning a cursor cannot spin it.
Neither is on a tool. Reaping runs from the TTL or from an explicit cleanup step, so a model
has no decision to make with the value, and an unexplained timestamp in a tool result is surface
it can only misuse — the same line #91 drew for
partition_by. Adding it to the tool later staysadditive; removing it later would change what every model sees.
Verification
--resolution lowest-directThe floor job earned its place here. This imports a new SDK model, and
hotdata's declared flooris
>=0.8.0, so I checked the surface at the floor directly rather than trusting the tests —they mock
DatabasesApiand would pass whether or not the real method existed. Athotdata0.9.0 /
hotdata-framework0.13.0 /langchain-core1.0.0,attach_database_catalog,detach_database_catalogandattachmentson the detail response all exist.Not in scope
be reachable by a model is Provisioning boundary: agent-driven database, table, and index lifecycle #61's open question and nothing here answers it.
question, not a wrapping one.
demo or an integration check against a real workspace before anyone relies on it.
Follow-up
docs/ai-native-layer-roadmap.mdsays "attach_database_catalogmay already be the supportedroute for the cross-database case; unverified". That is now wrong in a specific way — it is not
the route for the cross-database case — but the file has unrelated uncommitted changes in my
working tree, so I left it out rather than sweep them in. Wants a one-line fix.