-
Notifications
You must be signed in to change notification settings - Fork 0
chore(query-runs): clarify user_public_id documentation #103
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -106,7 +106,7 @@ pub struct QueryRunInfo { | |
| skip_serializing_if = "Option::is_none" | ||
| )] | ||
| 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. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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) |
||
| #[serde( | ||
| rename = "user_public_id", | ||
| default, | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Blocking — this drops the
X-Session-Idheader fromquery, and the same block is removed fromget_result/list_resultsinsrc/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 onquery,get_result, andlist_resultswith no compile-time or runtime signal.Please resolve the divergence one way or the other before merging: either drop
SESSION_ID_HEADERfrom the two hand-writtenapply_apikey_headershelpers and fromsubmit_query(updating those doc comments and thearrow.rstests), or keep it and rewrite the "mirrors the generated op" comments to state that the divergence is deliberate and why. Ifsession_idis no longer meaningful anywhere, it should be deprecated rather than left as a silent no-op.