chore(databases): clarify case-sensitivity rules for name matching - #188
chore(databases): clarify case-sensitivity rules for name matching#188hotdata-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 case-sensitivity rules for name matching |
There was a problem hiding this comment.
Blocking: this PR removes three public SDK symbols, and the changelog records none of them.
Removed by this diff:
ConnectionsApi.purge_connection_cacheConnectionsApi.purge_table_cacheCreateConnectionRequest.skip_discovery
The ### Removed section directly above (lines 10-20) already documents comparable retirements with a **Breaking:** prefix, so the convention exists in this file.
Failure scenario: a user on 0.10.0 upgrades to the next release, calls connections_api.purge_connection_cache(conn_id), and gets AttributeError. A user passing skip_discovery=True to CreateConnectionRequest gets a pydantic validation error. Neither user has any release note pointing at the cause.
Add entries under ### Removed for the two endpoints and the field.
| """ # noqa: E501 | ||
| 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.") |
There was a problem hiding this comment.
super nit: this description names health and cache purge as workspace-level connection endpoints, but both were removed from the SDK — check_connection_health in an earlier change, purge_connection_cache and purge_table_cache in this one. The text now points at endpoints that do not exist. Fix in the OpenAPI spec upstream, not here. (not blocking)
There was a problem hiding this comment.
Review
Context disclosure: the full diff was omitted from the review prompt. The diff was fetched with gh pr diff 188. The following files were read from the checkout: CHANGELOG.md, hotdata/models/managed_table_constant_per_key_response.py, hotdata/models/database_lineage_response.py, hotdata/models/create_database_response.py, hotdata/models/create_database_request.py, hotdata/api/databases_api.py, hotdata/api/connections_api.py, and tests/integration/test_connections_read.py.
CI note: at review start only Integration Tests / scenario-parity reported success. The other four checks were queued or in progress. No test result is claimed here.
Blocking Issues
tests/integration/test_connections_read.py:32calls a method this PR deletes.
The line is connections_api.purge_connection_cache(connection_id). This PR removes purge_connection_cache from ConnectionsApi. The test currently carries @pytest.mark.skip at line 15, so the call never runs and CI stays green.
Failure scenario: the skip reason at lines 16-18 says "Re-enable once the seeded connection is restored." Whoever re-enables the test gets AttributeError: 'ConnectionsApi' object has no attribute 'purge_connection_cache', not the connection assertion the test is about.
Delete line 32 and the comment on line 31. Update the module docstring on lines 4-5, which still lists "cache purge" as a step in this scenario.
CHANGELOG.md:24records no breaking removals.
See the inline comment. Three public symbols are removed with no entry under the Removed heading.
Action Required
- Remove the
purge_connection_cachecall and its comment fromtests/integration/test_connections_read.py, and correct that file's module docstring. - Add
Removedentries inCHANGELOG.mdforConnectionsApi.purge_connection_cache,ConnectionsApi.purge_table_cache, andCreateConnectionRequest.skip_discovery.
Note on scope
The PR title and description say "clarify case-sensitivity rules for name matching". The diff does much more: it adds three endpoints (get_database_lineage, lookup_database_by_name, set_database_table_constant_per_key), adds set_managed_table_constant_per_key to ConnectionsApi, adds five models, adds the if_not_exists request field and the created response field, and removes the two purge endpoints. Widen the description so a reviewer knows what to look at.
The generated model and API code itself reads correctly. create_database maps both 200 and 201 to CreateDatabaseResponse, which if_not_exists needs.
Auto-generated from the updated HotData OpenAPI spec.
Source: https://github.com/hotdata-dev/www/pull/420