-
Notifications
You must be signed in to change notification settings - Fork 0
chore(api): stop injecting SessionId into merged spec #102
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
Merged
Merged
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -61,7 +61,7 @@ use crate::apis::configuration::Configuration; | |
| use crate::apis::query_api::QueryError as GeneratedQueryError; | ||
| use crate::apis::results_api::GetResultError; | ||
| use crate::apis::{query_runs_api, results_api, Error, ResponseContent}; | ||
| use crate::client::{SESSION_ID_HEADER, WORKSPACE_ID_HEADER}; | ||
| use crate::client::WORKSPACE_ID_HEADER; | ||
| use crate::http::{backoff_delay, is_pre_response_transport_error, parse_retry_after}; | ||
| use crate::models::{AsyncQueryResponse, QueryRequest, QueryResponse, ResultsFormatQuery}; | ||
| use crate::status::ResultStatus; | ||
|
|
@@ -470,8 +470,8 @@ struct RawResponse { | |
| /// truncation per `qc`. | ||
| /// | ||
| /// Mirrors `hotdata.query.QueryApi.query` from the Python SDK. The request is | ||
| /// built wire-identically to the generated `query` op (same workspace/session | ||
| /// scope headers, bearer auth, `base_path`/`/v1` join, JSON body); building it | ||
| /// built wire-identically to the generated `query` op (same workspace scope | ||
| /// header, bearer auth, `base_path`/`/v1` join, JSON body); building it | ||
| /// here rather than calling the generated op lets the retry path read the | ||
| /// `Retry-After` header (the generated op discards response headers). | ||
| pub(crate) async fn execute_query( | ||
|
|
@@ -636,21 +636,19 @@ async fn send_query( | |
| }) | ||
| } | ||
|
|
||
| /// 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 | ||
|
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: (not blocking) two doc comments elsewhere still describe session scoping and were missed by this sweep:
Both are just |
||
| /// `isKeyInHeader` block. | ||
| fn apply_apikey_headers( | ||
| mut req_builder: reqwest::RequestBuilder, | ||
| config: &Configuration, | ||
| ) -> reqwest::RequestBuilder { | ||
| for header in [WORKSPACE_ID_HEADER, SESSION_ID_HEADER] { | ||
| if let Some(apikey) = config.api_keys.get(header) { | ||
| let key = apikey.key.clone(); | ||
| let value = match apikey.prefix { | ||
| Some(ref prefix) => format!("{prefix} {key}"), | ||
| None => key, | ||
| }; | ||
| req_builder = req_builder.header(header, value); | ||
| } | ||
| if let Some(apikey) = config.api_keys.get(WORKSPACE_ID_HEADER) { | ||
| let key = apikey.key.clone(); | ||
| let value = match apikey.prefix { | ||
| Some(ref prefix) => format!("{prefix} {key}"), | ||
| None => key, | ||
| }; | ||
| req_builder = req_builder.header(WORKSPACE_ID_HEADER, value); | ||
| } | ||
| req_builder | ||
| } | ||
|
|
||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Dropping the
X-Session-Idblock here (and inresults_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-IdAfter this change there is no
X-Session-Idemission left anywhere insrc/apis, so a singleClient::query()call now submits with the session header (send_query) and then pollsGET /v1/results/{id}without it (wait_for_result→ generatedget_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 theHOTDATA_SESSION_IDenv var still install the key intoconfiguration.api_keysand their docs still say "Installed as theX-Session-Idheader when present" — which is now only true for the hand-written paths. If the backend still scopes results by session, a session-scoped client'sget_result/list_resultscalls are no longer scoped.Please resolve one way or the other before merge:
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.