fix: collision-safe, resolve-once managed-database addressing (#39) - #58
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| def table_is_synced(self, database: str, table: str, *, schema: str) -> bool: | ||
| db = self._resolve(database) | ||
| for managed_table in self._request_with_retry( | ||
| lambda: self._runtime.list_managed_tables(db.id, schema=schema) | ||
| ): | ||
| if managed_table.table == table: | ||
| return managed_table.synced | ||
| return False |
There was a problem hiding this comment.
super nit: table_is_synced and _table_is_synced_for (below) are the same loop — resolve + iterate list_managed_tables for a matching .synced. table_is_synced could resolve then delegate to _table_is_synced_for so the scan logic lives in one place. (not blocking)
There was a problem hiding this comment.
Collision-safe resolution and the resolve-once/id-addressed rework look correct and are well-covered by the rewritten tests (ambiguity raises, resolve-once cache reuse, id-addressing on ensure/load/fetch/execute, drop clears cache). One non-blocking super-nit left inline.
Hotdata database names are not unique, and the destination resolved the name on every operation via hotdata-framework's first-match list scan, so a name collision could silently read from, write to, or drop the wrong database. Resolution now lists the workspace's databases, raises a clear error when a name matches more than one, resolves the name to its record once per run (cached on the shared config), and addresses every subsequent operation (load/add/list/query/drop) by id. The cache holds the full ManagedDatabase, which also seats the follow-up that lets a create-scoped key skip the read probe (#55, pending the hotdata-framework 0.9.0 passthrough).
c087c89 to
4d3f6c4
Compare
| def table_is_synced(self, database: str, table: str, *, schema: str) -> bool: | ||
| db = self._resolve(database) | ||
| for managed_table in self._request_with_retry( | ||
| lambda: self._runtime.list_managed_tables(db.id, schema=schema) | ||
| ): | ||
| if managed_table.table == table: | ||
| return managed_table.synced | ||
| return False |
There was a problem hiding this comment.
super nit: this public table_is_synced isn't called anywhere — the read path uses fetch_table, which goes through the private _table_is_synced_for. So it's dead code and duplicates the same scan. Simplest resolution (which also closes the earlier duplication note) is to just delete it. (not blocking)
1 participant
Fixes #39.
Problem
The destination is name-keyed: every managed-database operation independently resolves
config.database_name→ id viahotdata-framework'sresolve_managed_database, which falls back to alist_databases()scan and takes the firstname ==match. Hotdata database names are not unique, so on a collision the destination could silently read from, write to, or drop the wrong database — with no error. The scan also repeats on every op (ensure, per-table add/load, state reads,has_dataset, drop), each independently vulnerable.Changes
HotdataClient._collision_safe_resolve): lists the workspace's databases, and raisesHotdataTerminalError(naming the ambiguous ids) when a name matches more than one, instead of silently picking one. An id matches exactly and is unambiguous.bind_run_cache+_resolve): the name is resolved to its record once per run, cached on the sharedHotdataClientConfigurationinstance (which the job client, its load jobs, and the read client all share), and every subsequent op —ensure/load/add/list/fetch/execute_sql/drop— addresses the database by id.dropclears the cache.The cache holds the full
ManagedDatabase(id +default_connection_id), which also seats the #55 follow-up: oncehotdata-framework0.9.0 (theManagedDatabasepassthrough, hotdata-dev/sdk-python-framework#52) is pinned, the destination catches the 403→create and passes the cached object (instead of the id) so a create-scoped key skips the read probe.Scope
list, so it fails at resolution exactly as before — this closes Harden managed-database resolution: name lookups are collision-prone (id-only follow-up) #39 for read-capable keys and lays the Create/upload/query-scoped API keys can't bootstrap a managed database (resolve-before-create requires read access) #55 seam.Cost note
Collision detection uses the framework's
list_managed_databases()(1 list + N gets, since the list summary omitsdefault_connection_id). With the per-run cache this runs once per run, then every later op is cache-served. A lighter summaries-only list on the framework/API side would drop it to a single call — tracked as an upstream follow-up.Testing
tests/test_client.pyto the id-scoped model; added ambiguous-name-raises, resolve-once/cache-reuse, resolve-by-id, drop-clears-cache, and id-addressing on load/ensure/fetch/execute). e2eInMemoryBackendandtest_job_clientfakes updated to the real contract (list_managed_databases/bind_run_cache).ruffclean on changed/new files.