sinclairzx81 / typebox

Json Schema Type Builder with Static Type Resolution for TypeScript
Other
4.77k stars 152 forks source link

fix: surface deepest error from failed union validation #779

Closed aleclarson closed 6 months ago

aleclarson commented 6 months ago

The "Expected union value" error message is practically useless, so I think it‘s a better DX to surface the deepest validation error (based on property path).

sinclairzx81 commented 6 months ago

@aleclarson Hi, Thanks for the PR

Union errors have been quite difficult to get right, but not sure about surfacing only the deepest error. I do think TypeBox could provide more information about the nature of the error, so what are your thoughts on updating the ValueError interface to contain all sub errors?

Relevant updates below.

// ...Update ValueError to contain an array of sub errors.

export interface ValueError {
  type: ValueErrorType
  schema: TSchema
  path: string
  value: unknown     
  message: string     // Expected union value
  errors: ValueError[] // Union errors go here.
}

// ...Update Create method to accept optional array of ValueError

function Create(errorType: ValueErrorType, schema: TSchema, path: string, value: unknown, errors: ValueError[] = []): ValueError {
  return { type: errorType, schema, path, value, message: GetErrorFunction()({ errorType, path, schema, value }), errors }
}

// ... Update FromUnion to push union errors on sub errors.

function* FromUnion(schema: TUnion, references: TSchema[], path: string, value: any): IterableIterator<ValueError> {
  let unionErrors: ValueError[] = []
  for (const subschema of schema.anyOf) {
    const errors = [...Visit(subschema, references, path, value)]
    if (errors.length === 0) return // matched
    for(const error of errors) unionErrors.push(error)
  }
  if (unionErrors.length > 0) {
    yield Create(ValueErrorType.Union, schema, path, value, unionErrors) // errors here
  }
}

So, the above would ensure that TypeBox emits the same error types in same order (so non-breaking), while communicating that the error was a Union related with all internal errors obtainable from the sub error array.

What are your thoughts?

aleclarson commented 6 months ago

I think that makes more sense, as it allows for more advanced use cases than just my own, and I can easily work with your proposal to get what I want. 👍