feat!: address managed databases by id only, never by name (#39, #55) - #59
Conversation
c087c89 to
4d3f6c4
Compare
When an API key may create/upload/query but is forbidden from reading /databases (403), ensure_managed_database now creates the database (the operation the key is permitted to make) instead of failing, caches the returned record for the run, and hands that record to subsequent load/add/list operations so no further read is attempted. Builds on the resolve-once cache from #39. Requires hotdata-framework 0.9.0 (managed-table ops accept a resolved ManagedDatabase and skip the read probe). The pin bump is intentionally deferred until 0.9.0 is published; until then the object-passing is exercised via fakes modelling that contract.
…hrough (#55) 0.9.0 is released on PyPI; bump the pin so create-scoped keys can pass the resolved ManagedDatabase into load/add/list ops and skip the read probe. Verified against real 0.9.0: _as_managed_database returns the object as-is and the full suite is green.
ed8f353 to
8865559
Compare
rohan-hotdata
changed the title
|
Hey @rohan-hotdata , I am noticing that we are using database name to try and identify names, so we have this name collision logic now instead ManagedDatabase. I don't think that's necessary. Let's not try and resolve by name, lets be explicit and require ids. |
|
Also implicates this work: |
Hotdata database names are not unique -- the `name` field is a display label
(description), not an identifier. The destination previously listed databases
and matched on the name for every operation, so a collision could silently
read from, write to, or drop the wrong database. Resolve strictly by id
instead:
- Bind an existing database by id via `GET /databases/{id}` (no listing, no
name scan); every op addresses it by the resolved `ManagedDatabase` record.
- With no `database_id` and `create_database_if_missing`, create the database
(labelled `database_name`) and log its new id so it can be pinned for
subsequent runs -- without a pinned id, each run creates a new database.
- Create/upload/query-scoped keys bootstrap for free: the create path issues
no read, so a key forbidden from reading `/databases` only makes the create
it is permitted to make.
Adds `database_id` (param / `HOTDATA_DATABASE_ID` / `[destination.hotdata]`),
keeping `database_name` as a create-only label. Removes the by-name resolution
and collision guard entirely. Supersedes the interim collision-safe path (#39)
and reworks the #55 create-scoped bootstrap. Requires hotdata-framework>=0.9.0.
BREAKING CHANGE: managed databases are addressed by id, not name. To load into
the same database across runs, pin its `database_id` (printed on first-run
create). Bumps to 0.11.0.
rohan-hotdata
changed the title
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| if db is None: | ||
| database_id = self._configured_database_id() | ||
| if database_id: | ||
| db = self._bind_by_id(database_id) |
There was a problem hiding this comment.
nit: (not blocking) With a pinned database_id, the dev_mode / refresh="drop_sources" path now dead-ends. drop_storage() → drop_managed_database() deletes the pinned database and clears the run cache, but config.database_id still holds the (now-deleted) id. The subsequent initialize_storage() → ensure_managed_database(create_if_missing=True) takes this database_id branch, _bind_by_id gets a 404, and the resulting KeyError is not caught in initialize_storage (only HotdataTerminalError is), so it propagates as an opaque KeyError instead of recreating (the old by-name path recreated) or raising a clear "pinned database was dropped" error.
This is a narrow combination (pinning an id while also using dev_mode/refresh), and under id-first semantics recreating with the same id isn't possible anyway — but a clearer error beats a raw KeyError. Worth a follow-up.
There was a problem hiding this comment.
Reviewed the id-first redesign. Addressing-by-id, resolve-once caching, create-and-log, and the create-scoped bootstrap all look correct and are well covered by the rewritten tests. One non-blocking nit left inline about the dev_mode/refresh + pinned-id edge case.
…hangelog - ensure_managed_database: on the create path, a configured database_id that 404s (e.g. dropped by dev_mode/refresh) now raises a clear HotdataTerminalError instead of an opaque KeyError -- a server-assigned id can't be recreated, so the message tells the user to unset database_id or pin an existing one. The probe path (create_if_missing=False) still gets KeyError -> "not initialized". Addresses claude[bot]'s review nit on PR #59. - CHANGELOG: promote the Unreleased notes to a dated [0.11.0] section so the release-metadata check matches the version bump.
There was a problem hiding this comment.
Verified the prior review nit is resolved: a pinned database_id that 404s on the create path now raises a clear HotdataTerminalError (caught by initialize_storage) instead of an opaque KeyError. The id-first rework is consistent across call sites, tests, docs, changelog, and version bump.
…irst The read-showcasing demos did run() then read via dataset() in one process. Under id-first the read has no id to resolve (its config differs from the write's), so they crashed with "no managed database resolved". Provision the managed database up front and pin its id for both write and read (use --database-id to reuse an existing one). This also models the correct id-first workflow. demo.py (write-only) is unchanged. Demo-only; no src behavior change.
55d0c18
|
|
||
| def _is_not_found(exc: Exception) -> bool: | ||
| """True when ``exc`` wraps a 404 (the bound database id does not exist).""" | ||
| return getattr(getattr(exc, "__cause__", None), "status", None) == 404 |
There was a problem hiding this comment.
super nit: (not blocking) _is_not_found only inspects the immediate __cause__. This works today because get_database is called directly on DatabasesApi and the 404 ApiException lands one level down (confirmed by the tests and your prod validation). But the same module's _chain_messages in sql_client.py deliberately walks the whole __cause__ chain, noting the framework can bury the real status deeper. If the framework ever adds a wrapping layer around get_database errors, a dropped-id 404 would slip past this check and re-surface as an opaque HotdataTerminalError — defeating the clear "database_id was dropped" message this PR just added. Consider walking the chain here too for consistency and durability.
There was a problem hiding this comment.
id-first rework looks solid. The prior nit (dropped pinned database_id surfacing as an opaque KeyError) is resolved with a clear HotdataTerminalError and a covering test. Addressing is consistent across client/job_client/sql_client/ibis_backend/demos/tests. One non-blocking super nit left inline.
_is_not_found only inspected the immediate __cause__. That matches where the framework surfaces the 404 today, but if it ever wraps get_database errors in an extra layer the dropped-id 404 would slip past and re-surface as an opaque HotdataTerminalError, losing the clear "database_id was dropped" message. Walk the whole chain (mirroring sql_client._chain_messages) so it stays robust. Addresses claude[bot]'s review nit on PR #59.
There was a problem hiding this comment.
id-first refactor is clean and internally consistent; both prior nits (clear error on a dropped pinned database_id, and walking the full __cause__ chain in _is_not_found) are addressed in code. Call sites, tests, docs, CHANGELOG, and the version/pin bump all line up. LGTM.
Bring the docs in line with 0.11.0's id-first addressing (#59): README.md - Read-back: reads resolve the managed database by id, so configure database_id on the reading pipeline (drops the stale "no database IDs" claim that now errors). - hard_delete: marked conditional (⚠️ ) — deletes propagate only with a primary_key; keyless merge drops flagged rows without removing existing ones. - Intro tagline and "Verify a load" now address the database by id, so the whole README tells one addressing story (avoids the ambiguous duplicate-name case). docs/sql-client-spec.md - Note that the write→read example needs a pinned database_id under id-first. Docs-only; found via a full claim-audit of the docs rather than a string search.
Bring the docs in line with 0.11.0's id-first addressing (#59): README.md - Read-back: reads resolve the managed database by id, so configure database_id on the reading pipeline (drops the stale "no database IDs" claim that now errors). - hard_delete: marked conditional (⚠️ ) — deletes propagate only on the default `upsert` merge strategy with a primary_key; keyless merge and insert-only drop flagged rows without removing existing ones. - Intro tagline and "Verify a load" now address the database by id, so the whole README tells one addressing story (avoids the ambiguous duplicate-name case). docs/sql-client-spec.md - Note that the write→read example needs a pinned database_id under id-first. Docs-only; found via a full claim-audit of the docs rather than a string search.
Bring the docs in line with 0.11.0's id-first addressing (#59): README.md - Read-back: reads resolve the managed database by id, so configure database_id on the reading pipeline (drops the stale "no database IDs" claim that now errors). - hard_delete: marked conditional (⚠️ ) — deletes propagate only on the default `upsert` merge strategy with a primary_key; keyless merge and insert-only drop flagged rows without removing existing ones. - Intro tagline and "Verify a load" now address the database by id, so the whole README tells one addressing story (avoids the ambiguous duplicate-name case). docs/sql-client-spec.md - Note that the write→read example needs a pinned database_id under id-first. Docs-only; found via a full claim-audit of the docs rather than a string search.
Bring the docs in line with 0.11.0's id-first addressing (#59): README.md - Read-back: reads resolve the managed database by id, so configure database_id on the reading pipeline (drops the stale "no database IDs" claim that now errors). - hard_delete: marked conditional (⚠️ ) — deletes propagate only on the default `upsert` merge strategy with a primary_key; keyless merge and insert-only drop flagged rows without removing existing ones. - Intro tagline and "Verify a load" now address the database by id, so the whole README tells one addressing story (avoids the ambiguous duplicate-name case). docs/sql-client-spec.md - Note that the write→read example needs a pinned database_id under id-first. Docs-only; found via a full claim-audit of the docs rather than a string search.
2 participants
What & why
Adopts @eddietejeda's review direction: stop resolving managed databases by name; address them by id. Hotdata database names are not unique — the
namefield is a display label (description), not an identifier — so the destination should never look one up by name.Previously the destination listed databases and matched on the name for every operation. A name collision could silently read from, write to, or drop the wrong database. This PR removes that by-name path entirely and resolves strictly by id.
Behaviour
GET /databases/{id}once per run; every op (load/add/list/query/drop) addresses the resolvedManagedDatabaserecord. No listing, no name scan.database_idandcreate_database_if_missing, the database is created (labelleddatabase_name) and its new id is logged at WARNING so it can be pinned:/databasesonly makes the create it is permitted to make. No 403 probe needed.Config surface
New
database_id—hotdata(database_id=...)/HOTDATA_DATABASE_ID/[destination.hotdata] database_id. This mirrors how every other dlt destination takes its connection identity from config (Postgres DSN, MotherDuckmd:, LanceDBuri): you point at a specific database, set once in config, not edited per run.database_namestays as a create-only label.Relationship to prior work
list_databasesscan + ambiguity check are removed. What's kept from Harden managed-database resolution: name lookups are collision-prone (id-only follow-up) #39 is the id/object addressing.get_database(id)is id-only andlist_databases()has no name filter, so there was never a server-side name lookup to keep. Requireshotdata-framework>=0.9.0(managed-table ops accept a resolvedManagedDatabase).Breaking change → 0.11.0
Managed databases are addressed by id, not name. To load into the same database across runs, pin its
database_id(printed on first-run create); without it, each run creates a new database. (Also carries the earlierworkspace_id-out-of-credentials and api-key-env-resolution changes already noted in the CHANGELOG.)Testing
pytest: 124 passed, 1 skipped.test_client.pyrewritten for id-first (bind-by-id, create-and-log, resolve-once, create-scoped-no-read); e2e models the real "create once, pin the id" flow.Reviewer note
A one-off transient
the upload transfer to storage failed before any responseappeared once during prod validation — a storage-transport hiccup (the retry loaded cleanly), unrelated to this change.