chore(databases): clarify default_connection_id usage and catalog attachment rules - #197
hotdata-automation[bot] wants to merge 1 commit into
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
|
||
| ### Changed | ||
|
|
||
| - chore(databases): clarify default_connection_id usage and catalog attachment rules |
There was a problem hiding this comment.
This PR removes two public methods from ConnectionsApi: purge_connection_cache and purge_table_cache. The CHANGELOG records only a ### Changed chore line, so the removal ships undocumented.
Failure scenario: a caller on 0.11.0 calls connections_api.purge_connection_cache(connection_id). After upgrading, that call raises AttributeError. The caller reads the CHANGELOG, finds no mention of the removal, and has no migration note.
The repo already documents this exact kind of change. The 0.11.0 entry lists ConnectionsApi.check_connection_health under ### Removed with a **Breaking:** prefix.
Add a ### Removed section naming both methods as breaking. Add an ### Added section for the new surface in the same PR: DatabasesApi.get_database_lineage, DatabasesApi.lookup_database_by_name, DatabasesApi.set_database_table_constant_per_key, ConnectionsApi.set_managed_table_constant_per_key, and the six new models.
| created: Optional[StrictBool] = Field(default=None, description="Whether this call brought the database into existence. Only `false` when `if_not_exists` found a database already carrying the requested name, in which case nothing was created and the existing one is returned. The response status says the same thing — `201` against `200` — but generated clients often surface only the body, so it is stated here as well. Always sent. It is declared optional so that a client built against a newer version of this API still accepts a response from a deployment that predates the field. Absent therefore means \"this deployment cannot say\", which is not the same as `false` — test for the two values explicitly rather than for truthiness.") | ||
| default_catalog: StrictStr = Field(description="Name the database's default catalog answers to inside its query scope (`default` unless overridden at create time).") | ||
| default_connection_id: StrictStr = Field(description="Internal id of the connection that backs this database's `default` catalog. Workspace-level connection endpoints (list, get, health, delete, cache purge) refuse to act on this id — it is exposed only for the managed-tables load endpoint (`POST /v1/connections/{id}/schemas/{s}/tables/{t}/loads`) so callers can load data into tables declared at database-create time. Addressing it directly in SQL is not the recommended path — use `default` inside an `X-Database-Id` scope instead.") | ||
| default_connection_id: StrictStr = Field(description="Id of the connection that backs this database's `default` catalog. Two uses: pass it as `connection_id` to `POST /v1/databases/{other}/catalogs` to attach this database's catalog into another database, and as the connection in the managed-tables load endpoint (`POST /v1/connections/{id}/schemas/{s}/tables/{t}/loads`) to load tables declared at create time. Other connection endpoints (list, get, health, delete, cache purge) refuse to act on it. In SQL, address the catalog as `default` inside an `X-Database-Id` scope, not by this id.") |
There was a problem hiding this comment.
super nit: the description lists health and cache purge as connection endpoints that refuse this id. Neither endpoint exists in the SDK any more. check_connection_health was removed in 0.11.0, and this PR removes purge_connection_cache. A reader looking for those endpoints will not find them.
Fix belongs in the upstream OpenAPI spec, since this file is generated. (not blocking)
There was a problem hiding this comment.
Review
The full diff was omitted from the review context. I fetched it with gh pr diff and read these files from the checkout: hotdata/api/connections_api.py, hotdata/api/databases_api.py, hotdata/models/database_lineage_response.py, hotdata/models/forked_from_info.py, hotdata/models/lineage_ancestor_info.py, hotdata/models/lineage_fork_info.py, hotdata/models/managed_table_constant_per_key_response.py, hotdata/models/update_managed_table_request.py, CHANGELOG.md, tests/integration/test_connections_read.py, tests/integration/test_managed_tables_lifecycle.py, .github/workflows/integration-tests.yml.
The generated model and serializer code is consistent with the generator output. Both blocking findings concern the removal of the cache-purge endpoints.
Blocking Issues
1. CHANGELOG omits a breaking removal — CHANGELOG.md:12
ConnectionsApi.purge_connection_cache and ConnectionsApi.purge_table_cache are removed. The CHANGELOG records only a Changed chore line. See the inline comment for detail.
2. Integration test calls a removed method — tests/integration/test_connections_read.py:32
The test calls connections_api.purge_connection_cache(connection_id). That method no longer exists on ConnectionsApi.
Failure scenario: the test currently carries @pytest.mark.skip for an unrelated reason — a degraded seeded prod connection. The skip hides the break, so Integration Tests / integration stays green. When someone restores the seeded connection and lifts the skip, the test fails with AttributeError: ConnectionsApi object has no attribute purge_connection_cache, not with a connection error. The scenario-parity job cannot catch this, because it only checks that a scenario test file exists.
Delete line 32 and its comment on line 31. Update the module docstring on line 4, which still says the scenario covers "cache purge".
Action Required
- Add a
Removedsection to the[Unreleased]CHANGELOG entry, naming both removed methods as breaking. Add anAddedsection for the new endpoints and models. - Remove the
purge_connection_cachecall and the stale docstring text fromtests/integration/test_connections_read.py. - Update the docstring in
tests/integration/test_managed_tables_lifecycle.py:17, which explains why the scenario omits apurge_table_cachestep. The method is gone, so the explanation no longer applies.
Auto-generated from the updated HotData OpenAPI spec.
Source: https://github.com/hotdata-dev/www/pull/430