Update pinecone-generated-ts-fetch code along with associated types and unit tests #153

austin-denoble · 2023-11-01T19:37:12Z

Problem

We need to update the generated client code (pinecone-generated-ts-fetch) to align with upcoming API changes.

Solution

Regenerate and replace the contents of /pinecone-generated-ts-fetch.
Update relevant types and unit tests, primarily createIndex and listIndexes.
Add new unit tests to the createIndex.validation suite focused on new required fields in CreateIndexOptions.
Bonus: Fix for vectorOperationsProvider - TS wasn't properly inferring types within buildVectorOperationsConfig, and we were passing undefined to indexConfigurationParameters.basePath.

Type of Change

Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

…pdate relevant typescript types, fix bug in vectorOperationsProvider, update related unit tests

austin-denoble · 2023-11-01T19:50:19Z

src/control/createIndex.ts

+  /**
+   * The public cloud where you would like your index hosted
+   */
+  cloud: 'gcp' | 'aws' | 'azure';


This is aligned with the CreateRequestCloudEnum in the generated code: https://github.com/pinecone-io/pinecone-ts-client/pull/153/files#diff-678ee0ec5845c7d940569446c8f0a4210f8e23c95500729b0788439fd9759749R124

Maybe it makes more sense to just export and reuse that directly?

It would be nice to adjust the spec to somehow export these directly in the compiled code. But this seems okay for now.

austin-denoble · 2023-11-01T19:51:08Z

src/control/listIndexes.ts

-/**
- * A partial description of indexes in your project.
- *
- * For full information about each index, see
- * { @link Pinecone.describeIndex }
- */
-export type PartialIndexDescription = {
-  /** The name of the index */
-  name: string;
-};


My assumption is now that the intent is for the API to return the full objects, we don't want to keep the partial type around. Let me know if that's not the case.

Yes, exactly. This was just a placeholder until we could return the full object.

austin-denoble · 2023-11-01T19:52:18Z

src/control/listIndexes.ts

-    // will allow us us to add more information in the future
-    // in a non-breaking way.
-    return names.map((n) => ({ name: n }));
+    return response.databases || [];


I tried messing with the spec, but this || is mainly to handle the case where the response is an empty array. I could not get the OpenAPI spec to handle that and just return the empty array rather than undefined, so we basically do that here so we don't have to handle an optional response.

Open to feedback.

Even if the spec generation stuff were easier to massage, it seems smart to be defensive like this.

austin-denoble · 2023-11-01T19:53:54Z

src/control/types.ts

+export const CloudSchema = Type.Union([
+  Type.Literal('gcp'),
+  Type.Literal('aws'),
+  Type.Literal('azure'),
+]);


There might be a better way of doing this, open to feedback. This also isn't handled gracefully by errorFormatter in validator.ts. I'd like to cut a ticket and update if using literals here is deemed the way to go.

The specific changes to validator.ts to properly handle errors related to this schema are addressed here: #154

austin-denoble · 2023-11-01T19:56:23Z

src/data/vectorOperationsProvider.ts

    const indexConfigurationParameters: ConfigurationParameters = {
-      basePath: config.hostUrl,
+      basePath: config.indexHostUrl,


I had updated the name of this field before merging the previous PR (#148), and we apparently didn't have any type safety here as config seemed to be any. The provider was essentially giving basePath: undefined to each instance of Configuration. 🤦‍♂️

Nice to see some type safety added here. But the index part of the indexHostUrl name seems a bit redundant if the class is called IndexConfiguration

austin-denoble · 2023-11-01T20:00:30Z

src/control/__tests__/createIndex.validation.test.ts

+
+      expect(toThrow).rejects.toThrowError(PineconeArgumentError);
+      expect(toThrow).rejects.toThrowError(
+        'The argument to createIndex accepts multiple types. Either 1) 2) 3)'


This is the errorFormatter issue mentioned below (/src/control/types.ts): https://github.com/pinecone-io/pinecone-ts-client/pull/153/files#diff-cf36f0ee9dc687ce8159fbecd0992953a0c2e646eac44cca76cc2a2ba8246db3R30

We haven't used typebox literals anywhere else and the errorFormatter doesn't play nice.

jhamon

This all looks pretty reasonable to me. I haven't looked at your other PR about validation / enums / literals but this seems fine to merge into spruce and continue iterating on in follow-up PRs.

jhamon · 2023-11-06T20:10:23Z

src/control/listIndexes.ts

-/**
- * A partial description of indexes in your project.
- *
- * For full information about each index, see
- * { @link Pinecone.describeIndex }
- */
-export type PartialIndexDescription = {
-  /** The name of the index */
-  name: string;
-};


Yes, exactly. This was just a placeholder until we could return the full object.

jhamon · 2023-11-06T20:11:34Z

src/control/listIndexes.ts

-    // will allow us us to add more information in the future
-    // in a non-breaking way.
-    return names.map((n) => ({ name: n }));
+    return response.databases || [];


Even if the spec generation stuff were easier to massage, it seems smart to be defensive like this.

jhamon · 2023-11-06T20:15:42Z

src/data/vectorOperationsProvider.ts

    const indexConfigurationParameters: ConfigurationParameters = {
-      basePath: config.hostUrl,
+      basePath: config.indexHostUrl,


Nice to see some type safety added here. But the index part of the indexHostUrl name seems a bit redundant if the class is called IndexConfiguration

…feedback

_This revision is dependent on a previous branches / PRs. These will need to be reviewed and merged first:_ - #153 ## Problem When replacing the OpenAPI generated core in a previous revision (#153), our `validator` module which handles errors emitted from typebox was not handling literal types correctly. We added new union of literal types to support `cloud` values: ``` export const CloudSchema = Type.Union([ Type.Literal('gcp'), Type.Literal('aws'), Type.Literal('azure'), ]); ``` When passing an incorrect value for the `cloud` field, validator would not handle things gracefully: ![Screenshot 2023-11-03 at 7 28 21 PM](https://github.com/pinecone-io/pinecone-ts-client/assets/119623786/c2485c24-c898-49eb-81cc-260e3532f299) The errors emitted from typebox look like this: ![Screenshot 2023-11-03 at 7 31 11 PM](https://github.com/pinecone-io/pinecone-ts-client/assets/119623786/2ebbeeca-8a6a-4867-b04e-3cbfb2d1150f) ## Solution This took a lot of trial and error and testing, but I think this is a reasonable approach. I'm sure there's a lot more bike shedding and refactoring we could do in `validator`, but for now it feels most sensible to try and cater to the typebox schemas we support. Previously, at the top level of `errorFormatter` we were pulling out all errors that contained `anyOf` in the schema. We return early with the assumption that these errors are specific to the shape of the top level arguments being validated. Generally, these types of errors will not have an `instancePath`. It is also possible to have `anyOf` errors relating to a specific property, which in our case is the `cloud` field on `createIndex`. - At the top-level of `errorFormatter()` rename `anyOfErrors` to `anyOfArgumentErrors`, and update the filter to exclude errors with keyword of `const` or `type`. - Update `typeErrors()` to handle possible `anyOf` errors with keyword `const` and `instancePath.length > 1` as these are errors specific to a property whose type is some set of literal values. - Update broken unit tests. ## Type of Change - [X] Bug fix (non-breaking change which fixes an issue) ## Test Plan Pull this PR down and test using the repl: ``` $ npm run repl $ await init() // call createIndex and test that passing invalid cloud values throws correctly $ await client.createIndex({ name: 'test-index', dimension: 10, cloud: 'gg', region: 'us-east4', capacityMode: 'pod' }) // Uncaught: // PineconeArgumentError: The argument to createIndex had type errors: property 'cloud' is a constant which must be equal to one of: 'gcp', 'aws', 'azure'. // at /Users/austin/workspace/pinecone-ts-client/dist/validator.js:239:19 // at /Users/austin/workspace/pinecone-ts-client/dist/control/createIndex.js:67:21 // at step (/Users/austin/workspace/pinecone-ts-client/dist/control/createIndex.js:33:23) // at Object.next (/Users/austin/workspace/pinecone-ts-client/dist/control/createIndex.js:14:53) // at /Users/austin/workspace/pinecone-ts-client/dist/control/createIndex.js:8:71 // at new Promise (<anonymous>) // at __awaiter (/Users/austin/workspace/pinecone-ts-client/dist/control/createIndex.js:4:12) // at Pinecone._createIndex (/Users/austin/workspace/pinecone-ts-client/dist/control/createIndex.js:62:40) // at Pinecone.createIndex (/Users/austin/workspace/pinecone-ts-client/dist/pinecone.js:243:21) ```

_This revision is dependent on a previous branches / PRs. These will need to be reviewed and merged first:_ - #153 - #154 ## Problem We want to be able to track usage specific to the spruce release of the client. ## Solution - Update `buildUserAgent` logic to check for `packageInfo.release`, and append to the user agent if present. - Update `release-spruce-dev` and `release-spruce` workflows to add `@spruce` to `version.json` during these specific releases. ## Type of Change - [X] New feature (non-breaking change which adds functionality) ## Test Plan We will need to release through either the `release-spruce` or `release-spruce-dev` workflows and validate the `version.json` file has the proper info, and the `release` is appended to the user agent header.

…nd unit tests (#153) ## Problem We need to update the generated client code (`pinecone-generated-ts-fetch`) to align with upcoming API changes. ## Solution - Regenerate and replace the contents of `/pinecone-generated-ts-fetch`. - Update relevant types and unit tests, primarily `createIndex` and `listIndexes`. - Add new unit tests to the `createIndex.validation` suite focused on new required fields in `CreateIndexOptions`. - **Bonus:** Fix for `vectorOperationsProvider` - TS wasn't properly inferring types within `buildVectorOperationsConfig`, and we were passing `undefined` to `indexConfigurationParameters.basePath`. ## Type of Change - [X] Breaking change (fix or feature that would cause existing functionality to not work as expected) - [X] This change requires a documentation update

_This revision is dependent on a previous branches / PRs. These will need to be reviewed and merged first:_ - #153 ## Problem When replacing the OpenAPI generated core in a previous revision (#153), our `validator` module which handles errors emitted from typebox was not handling literal types correctly. We added new union of literal types to support `cloud` values: ``` export const CloudSchema = Type.Union([ Type.Literal('gcp'), Type.Literal('aws'), Type.Literal('azure'), ]); ``` When passing an incorrect value for the `cloud` field, validator would not handle things gracefully: ![Screenshot 2023-11-03 at 7 28 21 PM](https://github.com/pinecone-io/pinecone-ts-client/assets/119623786/c2485c24-c898-49eb-81cc-260e3532f299) The errors emitted from typebox look like this: ![Screenshot 2023-11-03 at 7 31 11 PM](https://github.com/pinecone-io/pinecone-ts-client/assets/119623786/2ebbeeca-8a6a-4867-b04e-3cbfb2d1150f) ## Solution This took a lot of trial and error and testing, but I think this is a reasonable approach. I'm sure there's a lot more bike shedding and refactoring we could do in `validator`, but for now it feels most sensible to try and cater to the typebox schemas we support. Previously, at the top level of `errorFormatter` we were pulling out all errors that contained `anyOf` in the schema. We return early with the assumption that these errors are specific to the shape of the top level arguments being validated. Generally, these types of errors will not have an `instancePath`. It is also possible to have `anyOf` errors relating to a specific property, which in our case is the `cloud` field on `createIndex`. - At the top-level of `errorFormatter()` rename `anyOfErrors` to `anyOfArgumentErrors`, and update the filter to exclude errors with keyword of `const` or `type`. - Update `typeErrors()` to handle possible `anyOf` errors with keyword `const` and `instancePath.length > 1` as these are errors specific to a property whose type is some set of literal values. - Update broken unit tests. ## Type of Change - [X] Bug fix (non-breaking change which fixes an issue) ## Test Plan Pull this PR down and test using the repl: ``` $ npm run repl $ await init() // call createIndex and test that passing invalid cloud values throws correctly $ await client.createIndex({ name: 'test-index', dimension: 10, cloud: 'gg', region: 'us-east4', capacityMode: 'pod' }) // Uncaught: // PineconeArgumentError: The argument to createIndex had type errors: property 'cloud' is a constant which must be equal to one of: 'gcp', 'aws', 'azure'. // at /Users/austin/workspace/pinecone-ts-client/dist/validator.js:239:19 // at /Users/austin/workspace/pinecone-ts-client/dist/control/createIndex.js:67:21 // at step (/Users/austin/workspace/pinecone-ts-client/dist/control/createIndex.js:33:23) // at Object.next (/Users/austin/workspace/pinecone-ts-client/dist/control/createIndex.js:14:53) // at /Users/austin/workspace/pinecone-ts-client/dist/control/createIndex.js:8:71 // at new Promise (<anonymous>) // at __awaiter (/Users/austin/workspace/pinecone-ts-client/dist/control/createIndex.js:4:12) // at Pinecone._createIndex (/Users/austin/workspace/pinecone-ts-client/dist/control/createIndex.js:62:40) // at Pinecone.createIndex (/Users/austin/workspace/pinecone-ts-client/dist/pinecone.js:243:21) ```

_This revision is dependent on a previous branches / PRs. These will need to be reviewed and merged first:_ - #153 - #154 ## Problem We want to be able to track usage specific to the spruce release of the client. ## Solution - Update `buildUserAgent` logic to check for `packageInfo.release`, and append to the user agent if present. - Update `release-spruce-dev` and `release-spruce` workflows to add `@spruce` to `version.json` during these specific releases. ## Type of Change - [X] New feature (non-breaking change which adds functionality) ## Test Plan We will need to release through either the `release-spruce` or `release-spruce-dev` workflows and validate the `version.json` file has the proper info, and the `release` is appended to the user agent header.

…nd unit tests (#153) ## Problem We need to update the generated client code (`pinecone-generated-ts-fetch`) to align with upcoming API changes. ## Solution - Regenerate and replace the contents of `/pinecone-generated-ts-fetch`. - Update relevant types and unit tests, primarily `createIndex` and `listIndexes`. - Add new unit tests to the `createIndex.validation` suite focused on new required fields in `CreateIndexOptions`. - **Bonus:** Fix for `vectorOperationsProvider` - TS wasn't properly inferring types within `buildVectorOperationsConfig`, and we were passing `undefined` to `indexConfigurationParameters.basePath`. ## Type of Change - [X] Breaking change (fix or feature that would cause existing functionality to not work as expected) - [X] This change requires a documentation update

_This revision is dependent on a previous branches / PRs. These will need to be reviewed and merged first:_ - #153 ## Problem When replacing the OpenAPI generated core in a previous revision (#153), our `validator` module which handles errors emitted from typebox was not handling literal types correctly. We added new union of literal types to support `cloud` values: ``` export const CloudSchema = Type.Union([ Type.Literal('gcp'), Type.Literal('aws'), Type.Literal('azure'), ]); ``` When passing an incorrect value for the `cloud` field, validator would not handle things gracefully: ![Screenshot 2023-11-03 at 7 28 21 PM](https://github.com/pinecone-io/pinecone-ts-client/assets/119623786/c2485c24-c898-49eb-81cc-260e3532f299) The errors emitted from typebox look like this: ![Screenshot 2023-11-03 at 7 31 11 PM](https://github.com/pinecone-io/pinecone-ts-client/assets/119623786/2ebbeeca-8a6a-4867-b04e-3cbfb2d1150f) ## Solution This took a lot of trial and error and testing, but I think this is a reasonable approach. I'm sure there's a lot more bike shedding and refactoring we could do in `validator`, but for now it feels most sensible to try and cater to the typebox schemas we support. Previously, at the top level of `errorFormatter` we were pulling out all errors that contained `anyOf` in the schema. We return early with the assumption that these errors are specific to the shape of the top level arguments being validated. Generally, these types of errors will not have an `instancePath`. It is also possible to have `anyOf` errors relating to a specific property, which in our case is the `cloud` field on `createIndex`. - At the top-level of `errorFormatter()` rename `anyOfErrors` to `anyOfArgumentErrors`, and update the filter to exclude errors with keyword of `const` or `type`. - Update `typeErrors()` to handle possible `anyOf` errors with keyword `const` and `instancePath.length > 1` as these are errors specific to a property whose type is some set of literal values. - Update broken unit tests. ## Type of Change - [X] Bug fix (non-breaking change which fixes an issue) ## Test Plan Pull this PR down and test using the repl: ``` $ npm run repl $ await init() // call createIndex and test that passing invalid cloud values throws correctly $ await client.createIndex({ name: 'test-index', dimension: 10, cloud: 'gg', region: 'us-east4', capacityMode: 'pod' }) // Uncaught: // PineconeArgumentError: The argument to createIndex had type errors: property 'cloud' is a constant which must be equal to one of: 'gcp', 'aws', 'azure'. // at /Users/austin/workspace/pinecone-ts-client/dist/validator.js:239:19 // at /Users/austin/workspace/pinecone-ts-client/dist/control/createIndex.js:67:21 // at step (/Users/austin/workspace/pinecone-ts-client/dist/control/createIndex.js:33:23) // at Object.next (/Users/austin/workspace/pinecone-ts-client/dist/control/createIndex.js:14:53) // at /Users/austin/workspace/pinecone-ts-client/dist/control/createIndex.js:8:71 // at new Promise (<anonymous>) // at __awaiter (/Users/austin/workspace/pinecone-ts-client/dist/control/createIndex.js:4:12) // at Pinecone._createIndex (/Users/austin/workspace/pinecone-ts-client/dist/control/createIndex.js:62:40) // at Pinecone.createIndex (/Users/austin/workspace/pinecone-ts-client/dist/pinecone.js:243:21) ```

_This revision is dependent on a previous branches / PRs. These will need to be reviewed and merged first:_ - #153 - #154 We want to be able to track usage specific to the spruce release of the client. - Update `buildUserAgent` logic to check for `packageInfo.release`, and append to the user agent if present. - Update `release-spruce-dev` and `release-spruce` workflows to add `@spruce` to `version.json` during these specific releases. - [X] New feature (non-breaking change which adds functionality) We will need to release through either the `release-spruce` or `release-spruce-dev` workflows and validate the `version.json` file has the proper info, and the `release` is appended to the user agent header.

re-generate and update openapi core in pinecone-generated-ts-fetch, u…

20de0e0

…pdate relevant typescript types, fix bug in vectorOperationsProvider, update related unit tests

austin-denoble requested review from jhamon and rohanshah18 November 1, 2023 19:37

austin-denoble commented Nov 1, 2023

View reviewed changes

This was referenced Nov 3, 2023

Update validator to handle typebox literal values #154

Merged

Add @spruce release to user-agent #155

Merged

jhamon approved these changes Nov 6, 2023

View reviewed changes

regenerate openapi core to support listIndexes, add new type, review …

ac4e902

…feedback

austin-denoble merged commit 5bedda5 into spruce Nov 7, 2023
16 of 19 checks passed

austin-denoble deleted the adenoble/update-openapi-spec branch November 7, 2023 18:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update pinecone-generated-ts-fetch code along with associated types and unit tests #153

Update pinecone-generated-ts-fetch code along with associated types and unit tests #153

austin-denoble commented Nov 1, 2023

austin-denoble Nov 1, 2023

jhamon Nov 6, 2023

austin-denoble Nov 1, 2023

jhamon Nov 6, 2023

austin-denoble Nov 1, 2023

jhamon Nov 6, 2023

austin-denoble Nov 1, 2023

austin-denoble Nov 6, 2023

austin-denoble Nov 1, 2023

jhamon Nov 6, 2023

austin-denoble Nov 1, 2023

jhamon left a comment

jhamon Nov 6, 2023

jhamon Nov 6, 2023

jhamon Nov 6, 2023

Update pinecone-generated-ts-fetch code along with associated types and unit tests #153

Update pinecone-generated-ts-fetch code along with associated types and unit tests #153

Conversation

austin-denoble commented Nov 1, 2023

Problem

Solution

Type of Change

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhamon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment