The proposed solution to problem #787 #819

RobinTail · 2023-02-14T00:12:20Z

Related to the discussion #787

Should enable the developer using zod as a tool within Endpoint's handler without considering its possible parsing/validation errors as IO related ones, leading to the status code 500 instead of 400 in that case.

@TheWisestOne

The approach

I am making an InputValidationError, consistent with OutputValidationError.
I separate the input parsing from the execution.
Validation errors (ZodError) catched before running middlewares and Endpoint handlers are transformed into InputValidationError

It also contains originalError prop for the further processing if needed.
However the message prop is already prepared for being human readable.

Thus, your ResultHandler now can be sure that any ZodError is actually NOT input validation error and can respond with 500.

Inspired by #794.

ToDo

needs more tests
need to expose error processing helpers in index ✅

RobinTail · 2023-02-14T00:24:55Z

@TheWisestOne , please review

TheWisestOne

Seems good to me!

I have 2 comments:

EndpointHandlerError is a very generic name, and might be better renamed EndpointHandlerZodError or something like that to clarify that any other errors thrown by the endpoint don't get wrapped in this error class
From my understanding, the flow of execution / error handling now looks like:

parse middleware input / run middleware -> any errors go to ResultHandler directly
parse handler input -> any errors go to ResultHandler directly
run endpoint handler -> any zod errors get wrapped in EndpointHandlerError
parse output -> any zod errors get wrapped in OutputValidationError

It seems a little arbitrary when we wrap up errors into these wrapper classes vs. let the result handler deal with the raw error. I am fine with the current state, just want to raise that it is not all that consistent, and it might be worth thinking about if there's a way to make it more consistent.

One proposal that comes to mind to resolve 2. above would be to sort of "merge" our two approaches:
We would do a try/catch around the parseAsync call rather than around the this.handler call, and in the catch block we would throw a new InputValidationError, which gets passed on to the ResultHandler. Then, the ResultHandler code that currently checks for a ZodError and decides to throw a 400 can instead check for an InputValidationError.
In this world, parsing of both input and output have the potential to wrap zod errors, but the endpoint handler itself always throws raw errors to the ResultHandler

src/endpoint.ts

RobinTail · 2023-02-19T18:23:33Z

EndpointHandlerError is a very generic name, and might be better renamed EndpointHandlerZodError or something like that to clarify that any other errors thrown by the endpoint don't get wrapped in this error class.

@TheWisestOne, yes, agree. That need to be fixed.

RobinTail · 2023-02-19T18:47:37Z

and in the catch block we would throw a new InputValidationError

there was a reason why I tried to refrain from making another validation error — it would be a breaking change for existing custom ResultHandlers made by developers.
Since ZodError is for a long time works as a synonym for the input validation error.

But I also agree with your point on consistency.

Need to think on it a little bit.
Sorry that it takes a bit long.

TheWisestOne · 2023-02-21T14:16:42Z

and in the catch block we would throw a new InputValidationError

there was a reason why I tried to refrain from making another validation error — it would be a breaking change for existing custom ResultHandlers made by developers. Since ZodError is for a long time works as a synonym for the input validation error.

But I also agree with your point on consistency.

Need to think on it a little bit. Sorry that it takes a bit long.

ah, an interesting point. One thing to note is that my attempt at this fix (#794) actually is non-breaking for people using an instanceof ZodError check in their ResultHandler.
This is because:
throw createHttpError(400, makeErrorFromAnything(e)); still creates an error that is an instance of a ZodError if e was a ZodError. I still see some of the benifits of your approach, but just wanted to point out that we might be able to do something non-breaking for existing ResultHandlers

RobinTail · 2023-02-26T19:37:53Z

I'm going to tackle this problem once again keeping in mind that the breaking change is acceptable, since I'm going to make v9 with another one.

RobinTail · 2023-02-26T19:43:27Z

I wanna go with the InputValidationError, for of consistency.

RobinTail · 2023-02-26T19:45:12Z

I wanna leave the utilization of createHttpError for the developer's decision on the final implementation and customizations.

…to problem-787

…nd InputValidationError.

RobinTail · 2023-02-26T21:12:56Z

src/endpoint.ts

-          ? new z.ZodError(
-              e.issues.map(({ path, ...rest }) => ({
-                ...rest,
-                path: (["output"] as typeof path).concat(path),


this trick is moved to the helper

RobinTail · 2023-02-26T21:27:29Z

OK. Now I'm happy with it.
I hope it will also fulfills your needs, @TheWisestOne

Anyway, I tested that throwing ZodError inside the Endpoint is not considered as IO validation issue.
So you could use zod for your database validations.

This reverts commit c71689b.

* The proposed solution. * Changing the name of the error. * Adding originalError prop. * The proposed solution. * Changing the name of the error. * Adding originalError prop. * Ref: the solution based on the consistency of OutputValidationError and InputValidationError. * Exposing getMessageFromError() and getStatusCodeFromError() helpers. * Enabling workflows. * Testing the edge cases. * Testing the inheritance of InputValidationError. * Adding the specific test for the problem #787. * Revert "Enabling workflows." This reverts commit c71689b.

* Remove request body from `DELETE` operations in OpenAPI spec (#821) * Remove request body from delete operations in OpenAPI spec * Remove `DELETE` request `body` from `defaultInputSources` + revert `src/open-api.ts` change from previous commit * Fix test name, add couple todos. * Testing fallback input sources for unknown request methods. * Updating the workflows to run in this branch. --------- Co-authored-by: Robin Tail <robin_tail@me.com> * Readme: reflecting the changes, including the config type description. * Changelog: version 9.0.0-beta1, describing the potentially breaking changes. * V9 is for Brianna Ghey. * Updating the workflows to support the future branch of v8. * Updating the Security policy. * 9.0.0-beta1 * Solving the problem #787 (#819) * The proposed solution. * Changing the name of the error. * Adding originalError prop. * The proposed solution. * Changing the name of the error. * Adding originalError prop. * Ref: the solution based on the consistency of OutputValidationError and InputValidationError. * Exposing getMessageFromError() and getStatusCodeFromError() helpers. * Enabling workflows. * Testing the edge cases. * Testing the inheritance of InputValidationError. * Adding the specific test for the problem #787. * Revert "Enabling workflows." This reverts commit c71689b. * Handling the edge case when the issues array is empty. * Changelog: v9.0.0-beta2, describing the potenitally breaking changes. * Readme: updating the instructions on creating a custom ResultHandler. * 9.0.0-beta2 * Changelog: the response may actually not be json. * Changelog: a note on base for 9.0.0-beta1. * Changelog: upcoming v9.0.0-beta3. * 9.0.0-beta3 * Breaking: removing createApiResponse(). * Ref: moving the array initializer for scopes and tags. * Replacing mimeTypes parameter with hasUpload, moving mime types initialization to constructor. * Ref: figuring out hasUpload inside the constructor. * Ref: single method for getMimeTypes() with variants. * Ref: moving response schema and status codes initializations into constructor. * Ref: single method for getStatusCode() with variants. * Ref: removing getOutputSchema() - redundant. * Ref: single method for getSchema() with variants and overloads. * Ref: extracting variant types. * Fix: using ZodTypeAny where anything is implied. * Changelog: upcoming 9.0.0-beta4. * Minor: fix typo. * 9.0.0-beta4 --------- Co-authored-by: Aleksandr Sidorenko <mcmerph@gmail.com>

The proposed solution.

d6fdb4e

RobinTail changed the title ~~The proposed solution.~~ The proposed solution to problem #787 Feb 14, 2023

RobinTail marked this pull request as ready for review February 14, 2023 00:24

TheWisestOne reviewed Feb 14, 2023

View reviewed changes

src/endpoint.ts Outdated Show resolved Hide resolved

RobinTail added 3 commits February 19, 2023 19:23

Merge branch 'master' into problem-787

b46202c

Changing the name of the error.

d2394bc

Adding originalError prop.

67f7b49

RobinTail added 5 commits February 26, 2023 20:48

Merge branch 'master' into problem-787

ef6f7f0

The proposed solution.

74bb230

Changing the name of the error.

4526a62

Adding originalError prop.

3041d05

Merge branch 'problem-787' of github.com:RobinTail/express-zod-api in…

a009e0d

…to problem-787

RobinTail changed the base branch from master to v9-beta February 26, 2023 19:49

RobinTail added 4 commits February 26, 2023 21:32

Ref: the solution based on the consistency of OutputValidationError a…

a40fda5

…nd InputValidationError.

Exposing getMessageFromError() and getStatusCodeFromError() helpers.

74993ac

Enabling workflows.

c71689b

Testing the edge cases.

8627e82

RobinTail commented Feb 26, 2023

View reviewed changes

RobinTail added 2 commits February 26, 2023 22:16

Testing the inheritance of InputValidationError.

5b8ac4f

Adding the specific test for the problem #787.

da071a7

Revert "Enabling workflows."

5245ca1

This reverts commit c71689b.

RobinTail merged commit bc5229c into v9-beta Feb 26, 2023

RobinTail deleted the problem-787 branch February 26, 2023 21:29

RobinTail mentioned this pull request Feb 26, 2023

Update input validation to explicitly throw an input validation error on failure #794

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

The proposed solution to problem #787 #819

The proposed solution to problem #787 #819

RobinTail commented Feb 14, 2023 •

edited

Loading

RobinTail commented Feb 14, 2023

TheWisestOne left a comment •

edited

Loading

RobinTail commented Feb 19, 2023

RobinTail commented Feb 19, 2023 •

edited

Loading

TheWisestOne commented Feb 21, 2023 •

edited

Loading

RobinTail commented Feb 26, 2023

RobinTail commented Feb 26, 2023

RobinTail commented Feb 26, 2023

RobinTail Feb 26, 2023

RobinTail commented Feb 26, 2023

The proposed solution to problem #787 #819

The proposed solution to problem #787 #819

Conversation

RobinTail commented Feb 14, 2023 • edited Loading

The approach

ToDo

RobinTail commented Feb 14, 2023

TheWisestOne left a comment • edited Loading

Choose a reason for hiding this comment

RobinTail commented Feb 19, 2023

RobinTail commented Feb 19, 2023 • edited Loading

TheWisestOne commented Feb 21, 2023 • edited Loading

RobinTail commented Feb 26, 2023

RobinTail commented Feb 26, 2023

RobinTail commented Feb 26, 2023

RobinTail Feb 26, 2023

Choose a reason for hiding this comment

RobinTail commented Feb 26, 2023

RobinTail commented Feb 14, 2023 •

edited

Loading

TheWisestOne left a comment •

edited

Loading

RobinTail commented Feb 19, 2023 •

edited

Loading

TheWisestOne commented Feb 21, 2023 •

edited

Loading