Add API version and custom headers #41
Conversation
pinecone_sdk/src/pinecone/mod.rs
Outdated
| let client = reqwest::Client::builder() | ||
| .default_headers(headers) | ||
| .build() | ||
| .unwrap_or(reqwest::Client::new()); |
There was a problem hiding this comment.
are we able to do this once when the client is initially created? I believe this function is called every time we make a call to an OpenAPI endpoint, so probably better to not create the additional headers map every single time if possible.
There was a problem hiding this comment.
I think if we fail to build the client with headers it should error instead of silently defaulting to basic client
pinecone_sdk/src/pinecone/mod.rs
Outdated
| // create reqwest headers | ||
| let mut headers = reqwest::header::HeaderMap::new(); | ||
| for (k, v) in additional_headers.iter() { | ||
| let key = reqwest::header::HeaderName::from_bytes(k.as_bytes()).unwrap(); | ||
| let value = reqwest::header::HeaderValue::from_str(v).unwrap(); | ||
| headers.insert(key, value); | ||
| } |
There was a problem hiding this comment.
This feels a bit heavy. There is some chatter about supporting HashMap -> HeaderMap conversions a bit more naturally. Refs: seanmonstar/reqwest#555 and hyperium/http#326.
Can you see if you can work out a better way to do this?
There was a problem hiding this comment.
I've updated it to use map rather than iterating and appending each user-defined header, would this be a better approach?
There was a problem hiding this comment.
How about this:
let headers: reqwest::header::HeaderMap =
(&additional_headers)
.try_into()
.map_err(|_| PineconeError::InvalidHeadersError {
message: "Provided headers are not valid".to_string(),
})?;
pinecone_sdk/src/pinecone/mod.rs
Outdated
| let client = reqwest::Client::builder() | ||
| .default_headers(headers) | ||
| .build() | ||
| .unwrap_or(reqwest::Client::new()); |
There was a problem hiding this comment.
I think if we fail to build the client with headers it should error instead of silently defaulting to basic client
codegen/proto_build/src/main.rs
Outdated
| let version: &str; | ||
| if args.len() == 3 { | ||
| out_dir = &args[1]; | ||
| version = &args[2]; | ||
| println!("OUT_DIR: {:?}", out_dir); | ||
| println!("version: {:?}", version); | ||
| } else { | ||
| return Err("missing out_dir argument".into()); | ||
| return Err("Required 2 arguments: out_dir version".into()); |
There was a problem hiding this comment.
We might want to update to use a CLI framework, but don't need to do in this PR
justfile
Outdated
|
|
||
| # Generate all OpenAPI and protobuf code | ||
| build-client: build-apis build-openapi build-proto | ||
| echo "/// Pinecone API version\npub const API_VERSION: &str = \"{{api_version}}\";" > pinecone_sdk/src/version.rs |
There was a problem hiding this comment.
I'm wondering if the command to generate the version.rs file should be it's own just command. 🤔
I'm not sure, technically we want to make sure this is populated every time so maybe moving into it's own action and calling for each build command might be good?
pinecone_sdk/src/pinecone/mod.rs
Outdated
| })?; | ||
|
|
||
| // add X-Pinecone-Api-Version header if not present | ||
| if !headers.contains_key(PINECONE_API_VERSION_KEY) { |
There was a problem hiding this comment.
Nice, I appreciate allowing overrides of this via additional_headers. 👍
Problem
The SDK currently does not support adding a custom header for requests, which is necessary if we want to include API versions.
Solution
An API version number is added to the justfile and propagated throughout the sdk, to code generation and to
PineconeClientto be sent with requests.Type of Change
Test Plan
All existing test cases pass.