chore(api): stop injecting SessionId into merged spec - #102
Conversation
| }; | ||
| req_builder = req_builder.header("X-Session-Id", value); | ||
| }; | ||
| if let Some(token) = configuration.resolve_bearer_token().await { |
There was a problem hiding this comment.
Dropping the X-Session-Id block here (and in results_api::get_result / list_results) leaves the SDK internally inconsistent, because the hand-written mirrors of these same requests still send it:
src/query.rs:618(send_query, the pathClient::queryactually uses) → sendsX-Session-Idsrc/client.rs:468(Client::submit_query) → sendsX-Session-Idsrc/arrow.rs:305(apply_apikey_headers, used byget_result_arrow/stream_result_arrow) → sendsX-Session-Id
After this change there is no X-Session-Id emission left anywhere in src/apis, so a single Client::query() call now submits with the session header (send_query) and then polls GET /v1/results/{id} without it (wait_for_result → generated get_result). Same for JSON vs Arrow result fetches: the Arrow path is session-scoped, the generated path is not.
This also silently narrows public API surface: ClientBuilder::session_id (src/client.rs:126) and the HOTDATA_SESSION_ID env var still install the key into configuration.api_keys and their docs still say "Installed as the X-Session-Id header when present" — which is now only true for the hand-written paths. If the backend still scopes results by session, a session-scoped client's get_result / list_results calls are no longer scoped.
Please resolve one way or the other before merge:
- If session auth is genuinely gone from the API — finish the removal in the same PR: drop
ClientBuilder::session_id,SESSION_ID_HEADER, theHOTDATA_SESSION_IDlookup, theSESSION_ID_HEADERentries insrc/query.rs/src/arrow.rs/src/client.rs::submit_query, the now-wrongarrow.rstests and doc comments that say "mirroring the generatedget_result" (src/arrow.rs:301,src/arrow.rs:496,src/query.rs:639), and theX-Session-Idrationale in.openapi-generator-templates/README.md:7. Record it in the changelog as Breaking sincesession_id()is public API. - If session scoping is still enforced server-side — the spec change is wrong and this regen should not ship.
|
|
||
| ### Changed | ||
|
|
||
| - chore(api): stop injecting SessionId into merged spec |
There was a problem hiding this comment.
nit: (not blocking) this entry is the raw commit subject and describes the spec pipeline ("merged spec"), not what changes for a user of this crate. Every other entry in this file is written from the caller's point of view. Something like "query, get_result, and list_results no longer send the X-Session-Id header" would tell a reader whether they are affected.
There was a problem hiding this comment.
Review
Blocking Issues
src/apis/query_api.rs:57,src/apis/results_api.rs:76,src/apis/results_api.rs:150— removing theX-Session-Idemission leaves no session-header emission anywhere insrc/apis, while the hand-written mirrors of the same requests still send it:src/query.rs:618(send_query),src/client.rs:468(submit_query),src/arrow.rs:305(Arrow result fetch). Net effect: oneClient::query()call now submits with the session header and then pollsGET /v1/results/{id}without it, and JSON vs Arrow result fetches disagree on scoping.src/client.rs:126—ClientBuilder::session_idand theHOTDATA_SESSION_IDenv var are public API. They still install the key and still document "Installed as theX-Session-Idheader when present", which is now only true on the hand-written paths. If the backend still scopes results by session, session-scopedget_result/list_resultscalls are silently no longer scoped.- Stale statements left behind:
src/arrow.rs:301andsrc/query.rs:639say the helpers mirror the generated ops'isKeyInHeaderblocks; the test atsrc/arrow.rs:496asserts behavior described as "matching the generatedget_result". Neither holds after this change..openapi-generator-templates/README.md:7still citesX-Session-Idas the reason for the template override.
Action Required
Pick one and land it in this PR:
- Session auth is gone from the API → finish the removal: drop
ClientBuilder::session_id,SESSION_ID_HEADER, theHOTDATA_SESSION_IDlookup, the session entries insrc/query.rs/src/arrow.rs/Client::submit_query, and the now-wrong tests and doc comments listed above. Record it as a Breaking changelog entry, sincesession_id()is public. - Session scoping is still enforced server-side → the upstream spec change is wrong and this regen should not ship; fix it at the source (the linked www.hotdata.dev PR 283).
Also see the non-blocking changelog-wording comment.
|
|
||
| /// Apply the `X-Workspace-Id` / `X-Session-Id` API-key headers, mirroring the | ||
| /// generated op's `isKeyInHeader` blocks. | ||
| /// Apply the `X-Workspace-Id` API-key header, mirroring the generated op's |
There was a problem hiding this comment.
super nit: (not blocking) two doc comments elsewhere still describe session scoping and were missed by this sweep:
src/query.rs:473—execute_query's doc still says the request is built wire-identically to the generated op with the "same workspace/session scope headers".src/uploads.rs:33— the module doc still saysput_to_storagesends "NONE of the SDK's bearer / workspace / session headers". The matching wording intests/presigned_uploads.rswas updated in this PR, so these two now disagree.
Both are just s|workspace/session|workspace| / dropping the / session clause.
There was a problem hiding this comment.
Prior blocking issue (incomplete X-Session-Id removal) is resolved: the sweep now covers ClientBuilder::session_id, SESSION_ID_HEADER, HOTDATA_SESSION_ID, the hand-written emission sites in src/query.rs / src/arrow.rs / Client::submit_query, the arrow tests, and the docs. No X-Session-Id references remain outside the changelog, so submit and poll paths are consistent again. Changelog and doc-comment nits also addressed.
Auto-generated from the updated HotData OpenAPI spec.
Source: https://github.com/hotdata-dev/www.hotdata.dev/pull/283