Skip to content
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

Update validator to handle typebox literal values #154

Merged
merged 5 commits into from
Nov 7, 2023

Conversation

austin-denoble
Copy link
Contributor

@austin-denoble austin-denoble commented Nov 3, 2023

This revision is dependent on a previous branches / PRs. These will need to be reviewed and merged first:

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

The errors emitted from typebox look like this:
Screenshot 2023-11-03 at 7 31 11 PM

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

  • 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)

@austin-denoble austin-denoble changed the base branch from spruce to adenoble/update-openapi-spec November 6, 2023 18:52
Copy link
Collaborator

@jhamon jhamon left a comment

Choose a reason for hiding this comment

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

Thanks for crawling through the rats nest to try to make this better.

src/validator.ts Outdated
const constValueErrors = propErrorGroups[property];

typeErrorsList.push(
`property '${property}' is a constant which must be equal to one of: ` +
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we could make this slightly more concise.

Suggested change
`property '${property}' is a constant which must be equal to one of: ` +
`property '${property}' must be equal to one of: ` +

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea, thanks!

Base automatically changed from adenoble/update-openapi-spec to spruce November 7, 2023 18:36
@austin-denoble austin-denoble force-pushed the adenoble/update-validator-for-literals branch from 2ad2a18 to c655eae Compare November 7, 2023 18:52
@@ -27,7 +26,7 @@ export type IndexListDescription = {
export type IndexList = Array<IndexListDescription>;

export const listIndexes = (api: IndexOperationsApi) => {
return async (): Promise<Array<IndexListMeta>> => {
return async (): Promise<IndexList> => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Snuck this update in after merging the last PR.

@austin-denoble austin-denoble merged commit 74ca598 into spruce Nov 7, 2023
16 of 19 checks passed
@austin-denoble austin-denoble deleted the adenoble/update-validator-for-literals branch November 7, 2023 19:22
austin-denoble added a commit that referenced this pull request Nov 7, 2023
_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.
austin-denoble added a commit that referenced this pull request Dec 27, 2023
_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)
```
austin-denoble added a commit that referenced this pull request Dec 27, 2023
_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.
austin-denoble added a commit that referenced this pull request Jan 13, 2024
_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)
```
austin-denoble added a commit that referenced this pull request Jan 13, 2024
_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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants