From 3f75215d1488ee1bbc7469e696b6ebfe6a9b98ce Mon Sep 17 00:00:00 2001 From: Austin DeNoble Date: Tue, 7 Nov 2023 14:22:29 -0500 Subject: [PATCH] Update validator to handle typebox literal values (#154) _This revision is dependent on a previous branches / PRs. These will need to be reviewed and merged first:_ - https://github.com/pinecone-io/pinecone-ts-client/pull/153 ## Problem When replacing the OpenAPI generated core in a previous revision (https://github.com/pinecone-io/pinecone-ts-client/pull/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 () // 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) ``` --- .../__tests__/createIndex.validation.test.ts | 4 +- src/control/listIndexes.ts | 3 +- src/validator.ts | 48 +++++++++++++++++-- 3 files changed, 46 insertions(+), 9 deletions(-) diff --git a/src/control/__tests__/createIndex.validation.test.ts b/src/control/__tests__/createIndex.validation.test.ts index 7d397fdd..d49f1007 100644 --- a/src/control/__tests__/createIndex.validation.test.ts +++ b/src/control/__tests__/createIndex.validation.test.ts @@ -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'." ); }); @@ -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'." ); }); diff --git a/src/control/listIndexes.ts b/src/control/listIndexes.ts index f861daec..d8cd81a8 100644 --- a/src/control/listIndexes.ts +++ b/src/control/listIndexes.ts @@ -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. @@ -27,7 +26,7 @@ export type IndexListDescription = { export type IndexList = Array; export const listIndexes = (api: IndexOperationsApi) => { - return async (): Promise> => { + return async (): Promise => { const response = await api.listIndexes(); return response.databases || []; diff --git a/src/validator.ts b/src/validator.ts index d2251575..f6138abb 100644 --- a/src/validator.ts +++ b/src/validator.ts @@ -97,12 +97,46 @@ const typeErrors = ( messageParts: Array ) => { const typeErrorsList: Array = []; + const anyOfConstPropErrors: Array = 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 } = {}; + 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); @@ -181,13 +215,17 @@ const validationErrors = ( }; export const errorFormatter = (subject: string, errors: Array) => { - 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