Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Thrown error does not provide data where the validation error has happened #92

Open
EdvinasDaugirdas opened this issue Dec 2, 2021 · 3 comments

Comments

@EdvinasDaugirdas
Copy link
Contributor

EdvinasDaugirdas commented Dec 2, 2021

In the current implementation we're throwing errors without returning any structured data to determine where the validation error has happened. That information is being returned via the error message, which is not always ideal.

Example: A user provided a HAR config with an invalid request url.

{
  "log": {
    "entries": [
      {
        "request": {
          "method": "POST",
          "url": true
        }
      }
    ]
  }
}

Such a HAR config would throw InvalidArchiveError with a message of Invalid request url (0): must be string and with a set property name equal to InvalidRequestUrl.

This implementation has a couple of issues:


The error message provides too much detail when not used in a CLI. Currently error messages almost always provide the index where the validation error happened. This probably leads to more confusion than actually helping the user if we're displaying error messages outside of a CLI.

A possible solution would be to remove the index part of the error message when validating HAR configs and only augmenting them with the index when used in a CLI.

This means the error message of our example would become:

  • Programmatic/Browser usage: Invalid request url must be string
  • CLI usage: Invalid request url must be string. Occurred in an entry with an index of 0.

Errors do not provide any structured data to determine where the validation error happened. Knowing where the validation error has happened would add more flexbility to the user to determine what to do with a given error.

Currently the only additional information that we provide is a name property (like InvalidRequestUrl or InvalidRequestQuery).

I'm suggesting extending the error object with two additional properties:

  • type - specifies the type of validation error. Since we're validating different types of data (like request, headers, cookies, queryString, etc.) it would be beneficial to know the type of validation error.
  • index - specifies the index of validation error. Usually this property would be a number. In some cases it would be a tuple [number, number]. For example, when throwing validation errors for headers we would like to know the indexes of both the entries and headers arrays respectively. With the addition of typeproperty or checking for the name property the user will know what type to expect from index.
  • path - pinpoints where the validation error has happened. Example: { ..., path: 'entries[4].request.headers[0].name'}.

Let me know your thoughts on the given suggestions. Thanks!

@w1kman
Copy link
Contributor

w1kman commented Dec 2, 2021

TL;DR; Would be nice to add json path to the affected node. Perhaps we could make us of inheritance (since we are dealing with a node tree (parent/child).


So if InvalidRequestUrl is thrown, wouldn't one expect to see InvalidRequestUrl: must be string. Received <typeof request.url>. or at least InvalidRequestUrl: must be string? (spelling out Invalid request url seems redundant, since that part of the error is implied by the name of the error).

JSON Path

I would suggest that we "pin point" the problem with json path (rather than indexes).
Eg. { ..., path: 'entries[4].request.headers[0].name'}

Error inheritance

When it comes to ValidationError.type, I don't know if it would be easier to just keep indicating type with the error constructor (name).
Let's say we are throwing a InvalidRequestQuery (in combination with a json path), what would the type be?

Sharing some thoughts... Since we have a tree structure, one could make use of the structure when creating error types.
That would give us granular errors that uses the tree structure for inheritance (as a bonus, names could probably be less verbose).
Perhaps stupid?

RequestError extends ValidationError
HeadersError extends RequestError

const error = new HeadersError('Ermagherd!')

error instanceof ValidationError // It's a validation error
error instanceof RequestError // It's a request validation error
error instanceof HeadersError // It's a request headers validation error

@EdvinasDaugirdas
Copy link
Contributor Author

Thanks for your suggestions! ❤️

I really like the JSON Path solution, definitely better than what I had in mind 😅

In terms of error inheritance I had the same thought and while I agree that it's probably more correct semantically I did not want to introduce a breaking change if I did not need to. It felt like error inheritance solution achieved the same goal just using a different API, so extending the original InvalidArchiveError with a type property seemed less intrusive.

If my thought process does not make any sense pls let me know 👍

@legander
Copy link
Collaborator

legander commented Dec 2, 2021

I really like these ideas that you guys have put forward 🙌

Here are some thoughts and questions.

Having both verbose and less verbose error message sounds great!
Would it make sense for the choice to get verbose messages or not to be explicitly set when compiling rather implicit depending on runtime? Alternatively packing both a "stracktrace" style error + a "non programmer readable" version with the error?

Regarding the error type/specific error classes, what purpose would this information serve and how would it be used? We type out those error messages by hand so they should be pretty useful alone, if we add JSONpath on top of that it seems to me like the error message + JSONpath will be more than enough to highlight/help to pinpoint where the error is?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants