fix: Provide correct error message on the validation error.

[Bram-dc]

The previous error message for each validation error was the entire zod error message instead of the issue message.

[kibertoad]

@Bram-dc somewhat related - could you please add a type for request validation errors? structure changed, and it's hard to map response in a custom error handler type for an error now

[Bram-dc]

@Bram-dc somewhat related - could you please add a type for request validation errors? structure changed, and it's hard to map response in a custom error handler type for an error now

Do you mean something like this?

[kibertoad]

@Bram-dc almost, although it is not the structure that we are actually getting in the error handler. I've pushed the test for this case.

See screenshot for the debugger:

Wonder if we need to adjust the type or the error

[kibertoad]

@Bram-dc I think the new type is for the issue entry, not for the error per se

[Bram-dc]

Yes it is indeed.

[Bram-dc]

if (error.validation && isZodFastifySchemaValidationError(error.validation))

[kibertoad]

@Bram-dc And for these we actually do not need to provide method and url for every entry, as they are going to be repeated for every single entry; and as you have said, they are available on the request itself anyway. original idea was to have them on the parent error, but it seems that we don't actually need them there

[Bram-dc]

if (error.validation && isZodFastifySchemaValidationError(error.validation))

error.validation is an array

[Bram-dc]

Should I remove them?

[kibertoad]

@Bram-dc What I'm looking for is a typeguard that could accurately narrow down error to what it actually is. Value of error.validation should be correctly derived from that.

And yeah, let's drop the url and method, but instead provide type guards for both request and response errors, so that these could be correctly mapped by hand in the custom error handler.

[Bram-dc]

I have a solution, give me a few minutes

[Bram-dc]

export class ZodFastifySchemaValidationError implements FastifySchemaValidationError {
  constructor(
    public message: string,
    public keyword: ZodIssueCode,
    public instancePath: string,
    public schemaPath: string,
    public params: {
      issue: ZodIssue;
      zodError: ZodError;
    },
  ) {}
}

export const isFastifyErrorZodFastiySchemaValidationError = (
  error: FastifyError,
): error is FastifyError & { validation: ZodFastifySchemaValidationError[] } =>
  error.validation &&
  error.validation.every((issue) => issue instanceof ZodFastifySchemaValidationError);

[Bram-dc]

Do we want both isZodFastiySchemaValidationError and isFastifyErrorZodFastiySchemaValidationError?

I also need some help with naming of these 2 functions.

edit: no its not only isFastifyErrorZodFastiySchemaValidationError will do, still no like the naming yet.

[Bram-dc]

maybe name it hasZodSchemaValidationErrors and only allow FastifyError as input?

[kibertoad]

@Bram-dc I would avoid instanceof checks, they do not work reliably across realms. Very liberal check that checks for presence of a few fields by name should be sufficient, probability of a collision with a similarly shaped error is pretty low. Alternatively we can put a simple is[ErrorName] field within the error for a quick check.

How about isFastifyRequestValidationError and isFastifyResponseValidationError?

[kibertoad]

only allow FastifyError as input?

That would work if fastify already narrows down all the errors it receives within the errorHandler to FastifyError. We need to accept as input what it gives us there

[Bram-dc]

@Bram-dc I would avoid instanceof checks, they do not work reliably across realms.

Can I do error instanceof Error or is that also not reliable?
And from there continue narrowing

[kibertoad]

instanceof Error is not reliable either. If you want a more reliable check, use this:

export function isError(maybeError: unknown): maybeError is Error {
  return (
    maybeError instanceof Error || Object.prototype.toString.call(maybeError) === '[object Error]'
  )
}

(this is how Node.js itself used to do the check)

[Bram-dc]

What about this, (I removed it, my bad)

export class ZodFastifySchemaValidationError implements FastifySchemaValidationError {
  public name = 'ZodFastifySchemaValidationError';

  constructor(
    public message: string,
    public keyword: ZodIssueCode,
    public instancePath: string,
    public schemaPath: string,
    public params: {
      issue: ZodIssue
      zodError: ZodError
    },
  ) {}
}


export const isZodFastifySchemaValidationError = (error: unknown): error is ZodFastifySchemaValidationError =>
    error instanceof Error && error.name === 'ZodFastifySchemaValidationError';

export const hasZodFastifySchemaValidationErrors = (
  error: unknown,
): error is FastifyError & { validation: ZodFastifySchemaValidationError[] } =>
    error instanceof Error &&
  'validation' in error &&
  Array.isArray(error.validation) &&
  error.validation.every(isZodFastifySchemaValidationError);

[Bram-dc]

Than this?

export class ZodFastifySchemaValidationError implements FastifySchemaValidationError {
  public name = 'ZodFastifySchemaValidationError';

  constructor(
    public message: string,
    public keyword: ZodIssueCode,
    public instancePath: string,
    public schemaPath: string,
    public params: {
      issue: ZodIssue;
      zodError: ZodError;
    },
  ) {}
}

export const isZodFastifySchemaValidationError = (
  error: unknown,
): error is ZodFastifySchemaValidationError =>
  typeof error === 'object' &&
  error !== null &&
  'name' in error &&
  error.name === 'ZodFastifySchemaValidationError';

export const hasZodFastifySchemaValidationErrors = (
  error: unknown,
): error is FastifyError & { validation: ZodFastifySchemaValidationError[] } =>
  typeof error === 'object' &&
  error !== null &&
  'validation' in error &&
  Array.isArray(error.validation) &&
  error.validation.every(isZodFastifySchemaValidationError);

[Bram-dc]

I was looking at ZodError and saw this. Is this reliable?

[kibertoad]

@Bram-dc instanceof always relies on being in the same realm. if an error is thrown from a package imported from another package, it will not work reliably, so I wouldn't trust that. Latest version that you've suggested looks good, although I'm not sure we need to iterate entire array, probability of someone mixing different entities there is very low

[Bram-dc]

Last thing, do you think it is better if ZodFastifySchemaValidationError was a type or a class? FastifySchemaValidationError itself is also just a type.

[Bram-dc]

If it is a class we should extend Error and add the ZodError as a cause instead of a param.

[kibertoad]

No strong opinion, either works

[Bram-dc]

🚀

[kibertoad]

thank you! I will release a new version tomorrow

[kibertoad]

@Bram-dc In a hindsight I see that the error structure that we have is not ideal, we are leaking implementation details:

      {
        "details": {
          "issues": [
            {
              "instancePath": "/name",
              "keyword": "invalid_type",
              "message": "Required",
              "name": "ZodFastifySchemaValidationError",
              "params": {
                "issue": {
                  "code": "invalid_type",
                  "expected": "string",
                  "message": "Required",
                  "path": [
                    "name",
                  ],
                  "received": "undefined",
                },
                "zodError": {
                  "issues": [
                    {
                      "code": "invalid_type",
                      "expected": "string",
                      "message": "Required",
                      "path": [
                        "name",
                      ],
                      "received": "undefined",
                    },
                  ],
                  "name": "ZodError",
                },
              },
              "schemaPath": "#/name/invalid_type",
            },
          ],
          "method": "POST",
          "url": "/",
        },
        "error": "Response Validation Error",
        "message": "Request doesn't match the schema",
        "statusCode": 400,
      }

Ideally neither ZodFastifySchemaValidationError, nor zodError, nor ZodError should be included in the exposed entity. Wonder if we could hide the name (e. g. by making it non-enumerable, or at least rename it to remove reference to Zod). Probably also rename zodError to validationError; but do we actually need it? doesn't issue field already include all the same data?

[Bram-dc]

@kibertoad What about this? #120