Skip to content

Add API version and custom headers #41

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 7 commits into from
Jul 25, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion codegen/build-oas.sh
Original file line number Diff line number Diff line change
@@ -1,13 +1,20 @@
#!/bin/bash

version=$1

if [ -z "$version" ]; then
echo "Version is required"
exit 1
fi

pushd codegen/apis
just build
popd

outdir="openapi"

docker run --rm -v $(pwd):/workspace openapitools/openapi-generator-cli:v7.6.0 generate \
--input-spec /workspace/codegen/apis/_build/2024-07/control_2024-07.oas.yaml \
--input-spec /workspace/codegen/apis/_build/$version/control_$version.oas.yaml \
--generator-name rust \
--output /workspace/$outdir \
--additional-properties "packageVersion=0.0.1"
9 changes: 8 additions & 1 deletion codegen/build-proto.sh
Original file line number Diff line number Diff line change
@@ -1,14 +1,21 @@
#!/bin/bash

version=$1

SCRIPT_DIR=$(dirname "$(realpath "$0")")
outdir="protos"

if [ -z "$version" ]; then
echo "Version is required"
exit 1
fi

OUT_DIR=$SCRIPT_DIR/../$outdir

pushd $SCRIPT_DIR/apis
just build
popd

pushd $SCRIPT_DIR/proto_build
cargo run -- $OUT_DIR
cargo run -- $OUT_DIR $version
popd
10 changes: 7 additions & 3 deletions codegen/proto_build/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,18 @@ fn main() -> Result<(), Box<dyn Error>> {
let args: Vec<String> = std::env::args().collect();

let out_dir: &str;
if args.len() > 1 {
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());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might want to update to use a CLI framework, but don't need to do in this PR

}

let proto_path: &Path = "../apis/_build/2024-07/data_2024-07.proto".as_ref();
let proto_path = format!("../apis/_build/{version}/data_{version}.proto");
let proto_path: &Path = proto_path.as_ref();

// directory the main .proto file resides in
let proto_dir = proto_path
Expand Down
17 changes: 14 additions & 3 deletions justfile
Original file line number Diff line number Diff line change
@@ -1,6 +1,17 @@
api_version := "2024-07"

# Build the OpenAPI and Protobuf definitions in `codegen/apis`
build-apis:
cd codegen/apis && just build

# Generate the control plane OpenAPI code based on the yaml files in `codegen/apis/_build`
build-openapi:
./codegen/build-oas.sh
cargo build -p openapi
./codegen/build-oas.sh {{api_version}}

# Generate the data plane protobuf code based on the yaml files in `codegen/apis/_build`
build-proto:
./codegen/build-proto.sh
./codegen/build-proto.sh {{api_version}}

# 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

3 changes: 3 additions & 0 deletions pinecone_sdk/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,3 +14,6 @@ pub mod pinecone;

/// Utility modules.
pub mod utils;

/// Version information.
pub mod version;
96 changes: 52 additions & 44 deletions pinecone_sdk/src/pinecone/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,9 @@
use serde_json;
use std::collections::HashMap;

/// The `PINECONE_API_VERSION_KEY` is the key for the Pinecone API version header.
pub const PINECONE_API_VERSION_KEY: &str = "x-pinecone-api-version";

/// Control plane module.
pub mod control;

Expand All @@ -18,7 +21,7 @@
api_key: String,
controller_url: String,
additional_headers: HashMap<String, String>,
source_tag: Option<String>,

Check warning on line 24 in pinecone_sdk/src/pinecone/mod.rs

View workflow job for this annotation

GitHub Actions / test (ubuntu-latest, stable)

field `source_tag` is never read

Check warning on line 24 in pinecone_sdk/src/pinecone/mod.rs

View workflow job for this annotation

GitHub Actions / test (ubuntu-latest, stable)

field `source_tag` is never read
user_agent: Option<String>,
}

Expand Down Expand Up @@ -74,9 +77,8 @@
let controller_host = control_plane_host.unwrap_or(env_controller);

// get additional headers
let additional_headers = match additional_headers {
Some(headers) => headers,
None => match std::env::var("PINECONE_ADDITIONAL_HEADERS") {
let additional_headers =
additional_headers.unwrap_or(match std::env::var("PINECONE_ADDITIONAL_HEADERS") {
Ok(headers) => match serde_json::from_str(&headers) {
Ok(headers) => headers,
Err(_) => {
Expand All @@ -87,8 +89,7 @@
}
},
Err(_) => HashMap::new(),
},
};
});

// create config
let config = Config {
Expand All @@ -113,13 +114,37 @@

/// Returns the OpenAPI configuration object.
pub fn openapi_config(&self) -> Configuration {
// create reqwest headers
// init an empty one if self.additional_headers is empty, otherwise make it
let mut headers = reqwest::header::HeaderMap::new();
for (k, v) in self.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);
}

// add X-Pinecone-Api-Version header if not present
if !headers.contains_key(PINECONE_API_VERSION_KEY) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, I appreciate allowing overrides of this via additional_headers. 👍

headers.insert(
reqwest::header::HeaderName::from_static(PINECONE_API_VERSION_KEY),
reqwest::header::HeaderValue::from_static(crate::version::API_VERSION),
);
}

// create reqwest client
let client = reqwest::Client::builder()
.default_headers(headers)
.build()
.unwrap_or(reqwest::Client::new());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think if we fail to build the client with headers it should error instead of silently defaulting to basic client


Configuration {
base_path: self.controller_url.clone(),
user_agent: self.user_agent.clone(),
api_key: Some(ApiKey {
prefix: None,
key: self.api_key.clone(),
}),
client,
..Default::default()
}
}
Expand Down Expand Up @@ -161,13 +186,9 @@
let mock_controller_host = "mock-arg-controller-host";

temp_env::with_var("PINECONE_API_KEY", Some(mock_api_key), || {
let pinecone = PineconeClient::new(
None,
Some(mock_controller_host),
Some(HashMap::new()),
None,
)
.expect("Expected to successfully create Pinecone instance");
let pinecone =
PineconeClient::new(None, Some(mock_controller_host), Some(HashMap::new()), None)
.expect("Expected to successfully create Pinecone instance");

assert_eq!(pinecone.api_key, mock_api_key);
assert_eq!(pinecone.controller_url, mock_controller_host);
Expand All @@ -187,13 +208,11 @@
let mock_controller_host = "mock-arg-controller-host";

temp_env::with_var_unset("PINECONE_API_KEY", || {
let pinecone = PineconeClient::new(
None,
Some(mock_controller_host),
Some(HashMap::new()),
None,
)
.expect_err("Expected to fail creating Pinecone instance due to missing API key");
let pinecone =
PineconeClient::new(None, Some(mock_controller_host), Some(HashMap::new()), None)
.expect_err(
"Expected to fail creating Pinecone instance due to missing API key",
);

assert!(matches!(pinecone, PineconeError::APIKeyMissingError { .. }));
});
Expand Down Expand Up @@ -227,13 +246,9 @@
"PINECONE_CONTROLLER_HOST",
Some(mock_controller_host),
|| {
let pinecone = PineconeClient::new(
Some(mock_api_key),
None,
Some(HashMap::new()),
None,
)
.expect("Expected to successfully create Pinecone instance with env host");
let pinecone =
PineconeClient::new(Some(mock_api_key), None, Some(HashMap::new()), None)
.expect("Expected to successfully create Pinecone instance with env host");

assert_eq!(pinecone.controller_url, mock_controller_host);
},
Expand Down Expand Up @@ -301,13 +316,11 @@
"PINECONE_ADDITIONAL_HEADERS",
Some(serde_json::to_string(&mock_headers).unwrap().as_str()),
|| {
let pinecone = PineconeClient::new(
Some(mock_api_key),
Some(mock_controller_host),
None,
None,
)
.expect("Expected to successfully create Pinecone instance with env headers");
let pinecone =
PineconeClient::new(Some(mock_api_key), Some(mock_controller_host), None, None)
.expect(
"Expected to successfully create Pinecone instance with env headers",
);

assert_eq!(pinecone.additional_headers, mock_headers);
},
Expand All @@ -322,13 +335,11 @@
let mock_controller_host = "mock-arg-controller-host";

temp_env::with_var("PINECONE_ADDITIONAL_HEADERS", Some("invalid-json"), || {
let pinecone = PineconeClient::new(
Some(mock_api_key),
Some(mock_controller_host),
None,
None,
)
.expect_err("Expected to fail creating Pinecone instance due to invalid headers");
let pinecone =
PineconeClient::new(Some(mock_api_key), Some(mock_controller_host), None, None)
.expect_err(
"Expected to fail creating Pinecone instance due to invalid headers",
);

assert!(matches!(
pinecone,
Expand Down Expand Up @@ -377,10 +388,7 @@
temp_env::with_vars(
[
("PINECONE_API_KEY", Some(mock_env_api_key)),
(
"PINECONE_CONTROLLER_HOST",
Some(mock_env_controller_host),
),
("PINECONE_CONTROLLER_HOST", Some(mock_env_controller_host)),
(
"PINECONE_ADDITIONAL_HEADERS",
Some(serde_json::to_string(&mock_env_headers).unwrap().as_str()),
Expand Down
2 changes: 2 additions & 0 deletions pinecone_sdk/src/version.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
/// Pinecone API version
pub const API_VERSION: &str = "2024-07";
35 changes: 28 additions & 7 deletions pinecone_sdk/tests/integration_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ use pinecone_sdk::pinecone::data::{Kind, Metadata, Namespace, SparseValues, Valu
use pinecone_sdk::pinecone::PineconeClient;
use pinecone_sdk::utils::errors::PineconeError;
use rand::Rng;
use std::collections::BTreeMap;
use std::collections::{BTreeMap, HashMap};
use std::time::Duration;
use std::vec;

Expand Down Expand Up @@ -435,7 +435,7 @@ async fn test_list_collections() -> Result<(), PineconeError> {
}

#[tokio::test]
async fn test_delete_collection_err() -> Result<(), PineconeError> {
async fn test_delete_collection_invalid_collection() -> Result<(), PineconeError> {
let pinecone =
PineconeClient::new(None, None, None, None).expect("Failed to create Pinecone instance");

Expand All @@ -447,6 +447,27 @@ async fn test_delete_collection_err() -> Result<(), PineconeError> {
Ok(())
}

#[tokio::test]
async fn test_list_collections_invalid_api_version() -> Result<(), PineconeError> {
let headers: HashMap<String, String> = [(
pinecone_sdk::pinecone::PINECONE_API_VERSION_KEY.to_string(),
"invalid".to_string(),
)]
.iter()
.cloned()
.collect();

let pinecone = PineconeClient::new(None, None, Some(headers), None)
.expect("Failed to create Pinecone instance");

let _ = pinecone
.list_collections()
.await
.expect_err("Expected to fail listing collections due to invalid api version");

Ok(())
}

#[tokio::test]
async fn test_index() -> Result<(), PineconeError> {
let pinecone = PineconeClient::new(None, None, None, None).unwrap();
Expand Down Expand Up @@ -982,10 +1003,7 @@ async fn test_fetch_vectors() -> Result<(), PineconeError> {
std::thread::sleep(std::time::Duration::from_secs(5));

let fetch_response = index
.fetch(
&["1".to_string(), "2".to_string()],
namespace,
)
.fetch(&["1".to_string(), "2".to_string()], namespace)
.await
.expect("Failed to fetch vectors");

Expand Down Expand Up @@ -1034,7 +1052,10 @@ async fn test_fetch_no_match() -> Result<(), PineconeError> {
.expect("Failed to target index");

let fetch_response = index
.fetch(&["invalid-id1".to_string(), "invalid-id2".to_string()], &Default::default())
.fetch(
&["invalid-id1".to_string(), "invalid-id2".to_string()],
&Default::default(),
)
.await
.expect("Failed to fetch vectors");

Expand Down
Loading