har-to-k6
har-to-k6 copied to clipboard
Thrown error does not provide data where the validation error has happened
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 (likerequest
,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 anumber
. In some cases it would be a tuple[number, number]
. For example, when throwing validation errors forheaders
we would like to know the indexes of both theentries
andheaders
arrays respectively. With the addition oftype
property or checking for thename
property the user will know what type to expect fromindex
.~~ -
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!
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
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 👍
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?