Skip to content

Commit

Permalink
Update validator to handle typebox literal values (#154)
Browse files Browse the repository at this point in the history
_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)
```
  • Loading branch information
austin-denoble committed Dec 27, 2023
1 parent 97c1b9d commit 3f75215
Show file tree
Hide file tree
Showing 3 changed files with 46 additions and 9 deletions.
4 changes: 2 additions & 2 deletions src/control/__tests__/createIndex.validation.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,7 @@ describe('createIndex argument validations', () => {

expect(toThrow).rejects.toThrowError(PineconeArgumentError);
expect(toThrow).rejects.toThrowError(
'The argument to createIndex accepts multiple types. Either 1) 2) 3)'
"The argument to createIndex had type errors: property 'cloud' must be equal to one of: 'gcp', 'aws', 'azure'."
);
});

Expand All @@ -196,7 +196,7 @@ describe('createIndex argument validations', () => {

expect(toThrow).rejects.toThrowError(PineconeArgumentError);
expect(toThrow).rejects.toThrowError(
'The argument to createIndex accepts multiple types. Either 1) 2) 3)'
"The argument to createIndex had type errors: property 'cloud' must be equal to one of: 'gcp', 'aws', 'azure'."
);
});

Expand Down
3 changes: 1 addition & 2 deletions src/control/listIndexes.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import { IndexOperationsApi } from '../pinecone-generated-ts-fetch';
import type { IndexListMeta } from '../pinecone-generated-ts-fetch';

/**
* A description of indexes in your project.
Expand Down Expand Up @@ -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> => {
const response = await api.listIndexes();

return response.databases || [];
Expand Down
48 changes: 43 additions & 5 deletions src/validator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -97,12 +97,46 @@ const typeErrors = (
messageParts: Array<string>
) => {
const typeErrorsList: Array<string> = [];
const anyOfConstPropErrors: Array<ErrorObject> = errors.filter(
(error) =>
error.schemaPath.indexOf('anyOf') > -1 &&
error.keyword === 'const' &&
error.instancePath.length > 0
);
let errorCount = 0;

// handle possible literal types first
const propErrorGroups: { [key: string]: Array<ErrorObject> } = {};
if (anyOfConstPropErrors.length > 0) {
for (const error of anyOfConstPropErrors) {
const constValue = error.instancePath.slice(1);

if (propErrorGroups[constValue]) {
propErrorGroups[constValue].push(error);
} else {
propErrorGroups[constValue] = [error];
}
}
const properties = Object.keys(propErrorGroups);

properties.forEach((property) => {
const constValueErrors = propErrorGroups[property];

typeErrorsList.push(
`property '${property}' must be equal to one of: ` +
Object.values(constValueErrors)
.map((group) => `'${group.params.allowedValue}'`)
.join(', ')
);
});
}

// typebox also emits type errors for each value of a literal so we want to exclude these
const anyOfKeys = Object.keys(propErrorGroups);
for (let i = 0; i < errors.length; i++) {
const e = errors[i];

if (e.keyword === 'type') {
if (e.keyword === 'type' && !anyOfKeys.includes(e.instancePath.slice(1))) {
errorCount += 1;
if (errorCount <= maxErrors) {
formatIndividualError(e, typeErrorsList);
Expand Down Expand Up @@ -181,13 +215,17 @@ const validationErrors = (
};

export const errorFormatter = (subject: string, errors: Array<ErrorObject>) => {
const anyOfErrors = errors.filter(
const anyOfArgumentErrors = errors.filter(
(error) =>
error.schemaPath.indexOf('anyOf') > -1 && error.keyword !== 'anyOf'
error.schemaPath.indexOf('anyOf') > -1 &&
error.keyword !== 'anyOf' &&
error.keyword !== 'const' &&
error.keyword !== 'type'
);
if (anyOfErrors.length > 0) {

if (anyOfArgumentErrors.length > 0) {
const groups = {};
for (const error of anyOfErrors) {
for (const error of anyOfArgumentErrors) {
const schemaPathMatch = schemaPathGroupNumberRegex.exec(error.schemaPath);
const groupNumber = schemaPathMatch ? schemaPathMatch[1] : 'unknown';
// Remove the anyOf portion of the schema path to avoid infinite loop
Expand Down

0 comments on commit 3f75215

Please sign in to comment.