chore(query-runs): clarify user_public_id documentation - #103
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| }; | ||
| 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.
Blocking — this drops the X-Session-Id header from query, and the same block is removed from get_result/list_results in src/apis/results_api.rs. After this PR, grep -rn "X-Session-Id" src/apis/ returns nothing: no generated operation sends the session header anymore.
That leaves the crate internally inconsistent, because the hand-written twins of these exact operations still send it:
src/client.rs:468—Client::submit_query(the hand-writtenPOST /v1/query) still emitsX-Session-Id, so it now sends different auth headers than the generatedqueryfor the same endpoint.src/query.rs:641-656—apply_apikey_headers, whose doc comment says it is "mirroring the generated op'sisKeyInHeaderblocks". That statement is now false.src/arrow.rs:301-321—apply_apikey_headers, documented as "mirroring the generatedget_resultisKeyInHeaderblocks so a session-scoped client ... behaves identically on the Arrow path". Also now false, and the unit test atsrc/arrow.rs:494-526asserts the header is forwarded "matching the generatedget_result" — the rationale that test encodes no longer holds.
Also, ClientBuilder::session_id (src/client.rs:126-131) still exists and still installs the API key, but it is now inert for every generated endpoint. A caller who builds a session-scoped client silently loses session scoping on query, get_result, and list_results with no compile-time or runtime signal.
Please resolve the divergence one way or the other before merging: either drop SESSION_ID_HEADER from the two hand-written apply_apikey_headers helpers and from submit_query (updating those doc comments and the arrow.rs tests), or keep it and rewrite the "mirrors the generated op" comments to state that the divergence is deliberate and why. If session_id is no longer meaningful anywhere, it should be deprecated rather than left as a silent no-op.
|
|
||
| ### Changed | ||
|
|
||
| - chore(query-runs): clarify user_public_id documentation |
There was a problem hiding this comment.
Blocking — this entry only records the user_public_id doc wording, but the same PR removes the SessionId security scheme from three operations (query, get_result, list_results), so the generated client stops sending X-Session-Id on all of them. That is a user-facing behavior change for anyone using ClientBuilder::session_id, and it is currently invisible in the changelog under a chore(...) heading.
Please add a separate entry describing the removal, e.g.:
- **Breaking:** `X-Session-Id` is no longer sent on `query`, `get_result`, or `list_results` — the `SessionId` security scheme was removed from these operations upstream.
Callers reading only the changelog would otherwise upgrade expecting a docs-only change.
| )] | ||
| pub trace_id: Option<Option<String>>, | ||
| /// Caller identity derived from the Authorization Bearer token (SHA-256 hash). Format: `user_{first_10_hex_chars}`. | ||
| /// Who ran this query: the account id from the access token the request was made with. Use it to group a caller's query history. Requests made with a credential that identifies no account instead record an opaque `user_`-prefixed identifier, which is stable for that credential but cannot be resolved to an account. |
There was a problem hiding this comment.
super nit: the doc comment has a stray double space ("query history. Requests made") where the upstream description's paragraph break was flattened into a single line. It renders as one run-on paragraph in rustdoc. Worth fixing in the generator template / spec so the two sentences stay visually separated. (not blocking)
There was a problem hiding this comment.
Review
Blocking Issues
-
X-Session-Idsilently dropped from all generated operations, leaving hand-written paths divergent —src/apis/query_api.rs:57andsrc/apis/results_api.rs:79,153. TheSessionIdsecurity scheme is removed fromquery,get_result, andlist_results; after this PR no generated operation sends the header. But the hand-written twins of those same operations still do, and their doc comments/tests explicitly claim to mirror the generated ops:src/client.rs:468—Client::submit_querystill sendsX-Session-IdforPOST /v1/query, diverging from generatedquery.src/query.rs:641— "mirroring the generated op'sisKeyInHeaderblocks" — no longer true.src/arrow.rs:301— "mirroring the generatedget_resultisKeyInHeaderblocks so a session-scoped client ... behaves identically on the Arrow path" — no longer true, and the test atsrc/arrow.rs:494asserts the session header is forwarded "matching the generatedget_result".
ClientBuilder::session_id(src/client.rs:126) still installs the API key but is now inert for every generated endpoint, so session scoping is lost with no signal to the caller. -
Behavior change not disclosed in the changelog —
CHANGELOG.md:12. The only entry is theuser_public_iddoc wording; the removal of session auth from three operations is unrecorded.
Action Required
- Reconcile the hand-written header helpers with the new spec: either drop
SESSION_ID_HEADERfromapply_apikey_headersinsrc/query.rsandsrc/arrow.rsand fromsubmit_queryinsrc/client.rs(updating the doc comments and thearrow.rssession-header tests), or keep them and rewrite the "mirrors the generated op" comments to state the divergence is intentional and why. - Decide the fate of
ClientBuilder::session_id: if no endpoint consumesX-Session-Idanymore, deprecate it rather than leaving a silent no-op. - Add a changelog entry for the
X-Session-Idremoval, flagged as breaking for session-scoped clients. .openapi-generator-templates/README.md:7still documentsX-Session-Idas one of the supported schemes — update it if the scheme is gone for good.
|
Reworded the changelog entry from the raw commit subject to a caller-facing description (a3ad306), which addresses the non-blocking nit from the earlier review. Worth flagging for whoever merges: this is a behavior change, not a docs clarification, so the PR title understates it. The upstream source is runtimedb#1000 — The two blocking issues from the earlier review were both about the The remaining super-nit (stray double space in the |
There was a problem hiding this comment.
Review
Cycle 1's two blocking findings were wrong and I'm withdrawing them: they were read off gh pr diff, which diffs against a stale merge base. The X-Session-Id removal from query/get_result/list_results and the corresponding docs changes already landed on main in #102 (chore(api): stop injecting SessionId into merged spec), together with the removal of ClientBuilder::session_id/SESSION_ID_HEADER from the hand-written paths and the Breaking ### Removed changelog entry at CHANGELOG.md:12-15. grep -rn 'session' src/ on the merged tree turns up nothing session-related, so there is no divergence between the generated and hand-written operations, and the changelog already covers the behavior change.
Against current main, this PR is docs-only:
CHANGELOG.md | 7 +++++++
docs/QueryRunInfo.md | 2 +-
src/models/query_run_info.rs | 2 +-
The regenerated user_public_id description matches between the model doc comment and docs/QueryRunInfo.md, and the changelog entry accurately describes the server-side semantics change. Approving.
1 participant
Auto-generated from the updated HotData OpenAPI spec.
Source: https://github.com/hotdata-dev/www.hotdata.dev/pull/289