chore(api): stop injecting SessionId into merged spec - #152
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| 'SessionId', | ||
| ), | ||
| } | ||
| return auth |
There was a problem hiding this comment.
Removing the SessionId scheme here silently breaks the hand-written session_id surface that this SDK's custom templates add on top of the generated code.
After this change:
Configuration.__init__still acceptssession_id=and still doesself.api_keys["SessionId"] = session_id(line 240-241)- the
session_idproperty/setter still exist (lines 543-557) and still document "sent asX-Session-Id" - the class docstring still advertises it (line 145-147) as a workspace/sandbox scoping example
…but nothing consumes it anymore. auth_settings() no longer emits a SessionId entry, and _auth_settings in query_api.py / results_api.py no longer lists it, so update_params_for_auth (api_client.py:649-651) never finds a setting to apply. X-Session-Id is never sent.
That is a silent failure mode, not a loud one: existing callers doing
conf = hotdata.Configuration(api_key=..., workspace_id="ws_abc", session_id="sb_xyz")get no error, no warning, and conf.session_id still reads back "sb_xyz" — but their queries and result reads now run unscoped against the workspace instead of isolated in the sandbox. Sandbox isolation quietly disappears.
This needs to be resolved one of two ways:
- If dropping SessionId is intended — remove the
session_idkwarg, property, setter, and docstring references from.openapi-generator-templates/configuration.mustache(lines 166-170, 212, 284, 331-337, 663-676) so the regeneratedconfiguration.pyno longer exposes a dead parameter, and note the removal as a breaking change in the CHANGELOG rather than under### Changed. - If SessionId is still a real header — restore it in the upstream spec so the generator re-emits both the
auth_settings()entry and the per-operation_auth_settingsmembership.
Separately worth confirming while you're here: the session_id docstring points users at SandboxesApi.create_sandbox, but there is no sandboxes_api.py in hotdata/api/. That predates this PR, but it suggests option 1 is the intent — in which case the template cleanup belongs in this change so the public API and the wire behavior stay in sync.
There was a problem hiding this comment.
Review
Blocking Issues
hotdata/configuration.py:601 — session_id becomes a silent no-op
This PR removes the SessionId security scheme from the generated code (auth_settings() in configuration.py, the AuthSettings TypedDict, and _auth_settings in query_api.py / results_api.py), but the session_id public surface added by this repo's custom templates is left in place:
hotdata/configuration.py:191—session_id: Optional[str]=Nonekwarghotdata/configuration.py:240-241— still writesself.api_keys["SessionId"]hotdata/configuration.py:543-557—session_idproperty + setter, documented as "sent asX-Session-Id"hotdata/configuration.py:145-147— class docstring advertises sandbox scoping viasession_id
Nothing consumes that value now. update_params_for_auth (hotdata/api_client.py:649-651) looks each requested auth name up in configuration.auth_settings(); SessionId is absent from both the operation lists and the returned dict, so X-Session-Id is never sent.
Callers passing session_id="sb_xyz" get no error and can still read the value back off the config, but their /query and results requests silently run unscoped against the workspace instead of isolated in the sandbox. Silent loss of scoping is the problem here, not the removal itself.
Action Required
Pick one and make the code consistent:
- Removal is intended — strip the
session_idkwarg, property, setter, and docstring references from.openapi-generator-templates/configuration.mustache(lines 166-170, 212, 284, 331-337, 663-676) so regeneration stops exposing a dead parameter, and record this as a breaking change inCHANGELOG.mdrather than a plain### Changedentry. X-Session-Idis still live — restore the scheme in the upstream spec so the generator re-emits theauth_settings()entry and the per-operation_auth_settingsmembership.
Option 1 looks likelier given the session_id docstring points at SandboxesApi.create_sandbox and no sandboxes_api.py exists in hotdata/api/. Either way, the template change belongs in this PR so the public API and the wire behavior do not diverge.
|
Took option 1 — Removed the kwarg, property, setter, and docstrings from
Heads up on downstream: |
| @pytest.mark.parametrize("kwarg", _typed_api_key_kwargs()) | ||
| def test_typed_api_key_kwarg_reaches_auth_settings(kwarg: str) -> None: | ||
| """A convenience kwarg that never shows up in auth_settings() is a no-op.""" | ||
| config = _config(**{kwarg: "value-for-" + kwarg}) | ||
|
|
||
| settings = config.auth_settings() | ||
| wire_values = {setting["value"] for setting in settings.values()} | ||
| assert "value-for-" + kwarg in wire_values, ( | ||
| f"Configuration(...{kwarg}=...) is accepted but auth_settings() never emits it, " | ||
| "so the value is silently dropped instead of being sent." | ||
| ) |
There was a problem hiding this comment.
nit: this tripwire only covers half the failure mode (not blocking).
The module docstring says the goal is to pin the invariant generally "so the next scheme removal fails here instead of shipping a silent no-op", but this test only checks the config-level half: kwarg → auth_settings(). The per-operation half is only covered for SessionId specifically, by test_session_header_is_not_sent.
auth_settings() is generated from the spec's securitySchemes, while each operation's _auth_settings list comes from that operation's security. Those move independently. If a future spec keeps a scheme declared under securitySchemes but drops it from every operation's security, auth_settings() still emits it, this test still passes — and the kwarg is once again a silent no-op that never reaches the wire. That's exactly the shape of the bug this file exists to catch.
A general version would assert scheme membership in the operations too, e.g. alongside the existing test:
_SCHEME_FOR_KWARG = {"workspace_id": "WorkspaceId"}
@pytest.mark.parametrize("kwarg", _typed_api_key_kwargs())
def test_typed_api_key_kwarg_is_used_by_some_operation(kwarg: str) -> None:
scheme = _SCHEME_FOR_KWARG[kwarg]
sources = "\n".join(
p.read_text() for p in (REPO_ROOT / "hotdata" / "api").glob("*.py")
)
assert f"'{scheme}'" in sources, (
f"{scheme} is in auth_settings() but no operation lists it in _auth_settings, "
"so the kwarg is never sent."
)Nothing wrong with what's here — it just leaves the door open on the side that isn't spec-declaration.
There was a problem hiding this comment.
Prior blocking issue resolved: the session_id kwarg, property, and docstring references are gone from .openapi-generator-templates/configuration.mustache as well as the generated hotdata/configuration.py, the CHANGELOG entry moved under ### Removed and is marked breaking, and tests/test_config_auth_surface.py pins the kwarg/wire invariant so the next scheme removal fails loudly. Verified no session_id / SessionId / X-Session-Id references remain outside the test and changelog, and that the new tests match the actual _query_serialize / _list_results_serialize signatures and param_serialize's 5-tuple return.
Note: I could not execute pytest in this environment, so the new test file was reviewed by reading rather than by running it.
One non-blocking nit left inline on the test coverage.
1 participant
Auto-generated from the updated HotData OpenAPI spec.
Source: https://github.com/hotdata-dev/www.hotdata.dev/pull/283