Improve error handling in parser

[magicmatatjahu]

Reason/Context

On the Slack, there was a discussion in which several proposals were proposed to improve the handling of errors in parser.

Description

Proposals:

1. Add custom-validation-error new error type for custom validation like: validate variables of server name etc...

2. Create the predefinied classes for error types like in models:

class ValidationError extends ParserError {
  super(...args) {
     args.type = "validation-errors";
     super(args);
  }

  // additional logic
}

3. Think about subtypes for custom-validation-errors, for channels, server-variables etc. It would be great if it will be implemented with HTTP Problems RFC.

NOTE: this is improvement not for 1.0.0 but for the next version.

[derberg]

I'm with the idea of introducing more error types!

I also think we need to make it super clear what is our approach to validation vs throwing errors. Should we always throw errors and have no warnings? or should we always throw errors and leave it up to the parser users to decide if a particular error type they want to silent or not.

Why?
This cases:

• https://github.com/asyncapi/tck/blob/master/tests/asyncapi-2.0/Parameter%20Object/valid-parameter-not-defined.yaml
• https://github.com/asyncapi/tck/blob/master/tests/asyncapi-2.0/Server%20Variable%20Object/valid-variable-not-defined.yaml

now we are throwing errors that some Parameter or Variable object is not provided. The thing is that it is not mandatory to have those objects. Tck shows parser doesn't work as expected.

We might need to maybe introduce some kind of strict flag and throw such special error only if strict: true

[derberg]

We discussed it during an open meeting. We had an agreement that having a kind of silent flag is good as long as it is false by default. We just did not discuss in detail what can be silent. For now, my suggestion is to use the spec as a "decision maker" here, so if something is described by the spec and is not done in the document should be a normal error, others (even if important for code gen, but not for docs) should be available for silent. But now when I think about it, silent instead of boolean could be an array of error types, that you can pass and silent 🤔 We will have to make a good proposal here when we start coding

[github-actions]

This issue has been automatically marked as stale because it has not had recent activity 😴
It will be closed in 30 days if no further activity occurs. To unstale this issue, add a comment with detailed explanation.
Thank you for your contributions ❤️

[github-actions]

This issue has been automatically marked as stale because it has not had recent activity 😴
It will be closed in 60 days if no further activity occurs. To unstale this issue, add a comment with detailed explanation.
Thank you for your contributions ❤️

[github-actions]

This issue has been automatically marked as stale because it has not had recent activity 😴
It will be closed in 60 days if no further activity occurs. To unstale this issue, add a comment with detailed explanation.
Thank you for your contributions ❤️

[github-actions]

This issue has been automatically marked as stale because it has not had recent activity 😴
It will be closed in 60 days if no further activity occurs. To unstale this issue, add a comment with detailed explanation.
Thank you for your contributions ❤️

[github-actions]

This issue has been automatically marked as stale because it has not had recent activity 😴
It will be closed in 60 days if no further activity occurs. To unstale this issue, add a comment with detailed explanation.
Thank you for your contributions ❤️

[github-actions]

This issue has been automatically marked as stale because it has not had recent activity 😴

It will be closed in 120 days if no further activity occurs. To unstale this issue, add a comment with a detailed explanation.

There can be many reasons why some specific issue has no activity. The most probable cause is lack of time, not lack of interest. AsyncAPI Initiative is a Linux Foundation project not owned by a single for-profit company. It is a community-driven initiative ruled under open governance model.

Let us figure out together how to push this issue forward. Connect with us through one of many communication channels we established here.

Thank you for your patience ❤️

[BOLT04]

Hi @magicmatatjahu and @derberg 🙂, this looks like an interesting improvement, but I have some questions.

It would be great if it will be implemented with HTTP Problems RFC

I think taking inspiration on the HTTP Problems RFC is a good idea, but I'm not sure parser-js should return errors with all the fields of that spec, mainly the status field. This is because it would add a "dependency" on HTTP, and the parser can be used in another context, so HTTP status codes don't mean a lot to the CLI, IMO.

I'm not aware if this feature is on the CLI or other AsyncAPI tools, but would it make sense to have the parser auto fix these warnings or errors? Kinda like eslint --fix, so the AsyncAPI CLI would call the parser to fix these issues? Maybe not all errors can be fixed automatically, but it would be interesting to know if fixing errors while editing the AsyncAPI document is taking too much time currently and can be improved.

But now when I think about it, silent instead of boolean could be an array of error types, that you can pass and silent 🤔

I think it's common for it to be a flag, at least when this silent is implemented in CLIs I think. But now that you mentioned it, it could very well be better using an array. I'm assuming this new option would be added to ParserOptions, so its type is very important since it's part of the library's interface.
IMO, it's easier to evolve a library if we use an object, simply because it's extensible 🙂. There isn't much we can do if we go with a boolean or array and later decide to change it, other than making a breaking change. @derberg what do you think about having an object, and inside it have that array of error types to ignore? We could add properties later for new features without breaking changes.

If anyone could share their thoughts on this that would be awesome 👍

[derberg]

I have to admit this one is pretty old and I don't remember much 😅

I think taking inspiration on the HTTP Problems RFC is a good idea, but I'm not sure parser-js should return errors with all the fields of that spec, mainly the status field. This is because it would add a "dependency" on HTTP, and the parser can be used in another context, so HTTP status codes don't mean a lot to the CLI, IMO.

I don't think status makes a lot of sense, but I also think the intention here is to follow HTTP Problems RFC to the extend it makes sense.

Kinda like eslint --fix

The flag could be easily added to the validate command, but the question is if there is actually anything that could be auto-fixed really. I don't think it is possible. At least there is no error that comes to my mind on Monday morning that could be fixed with some autofix.

what do you think about having an object, and inside it have that array of error types to ignore

Yup, object it is. My only real concern here is where to draw a line between a spec-related validation and a linter. I'm just not 100% sure if we should go in this direction at all. Maybe we should just remove the validation I added some time ago and it should belong to a separate "linter" solution. Maybe validation in parser should be simple, error only and strict-spec-only. Do you know what I mean? I'm kinda afraid that with one silent error we open a door for more and more, etc.

[magicmatatjahu]

@derberg @BOLT04

About --fix flag, as Łukasz wrote, there is no error to be easily fixed, e.g. in eslint --fix is only to fix some problems with indentation, white spaces etc, and in AsyncAPI doc errors are related to the wrong structure so --fix won't help, because we can't change the content of the spec if something is wrong.

Maybe we should just remove the validation I added some time ago and it should belong to a separate "linter" solution. Maybe validation in parser should be simple, error only and strict-spec-only

I had the same thought. I wanna test Spectral from the Stoplight but I don't have time. I wonder if we could reuse the logic related with rulesets and integrate them into our parser, then all the custom logic associated with validating duplicate tags, existing servers in channels etc (things that we cannot validate by JSON Schema) could be implemented using appropriate rules in the linter. Additionally, users would be able to add their own rules.

[derberg]

I know that @kevinswiber did some research on Spectral + AsyncAPI (https://github.com/kevinswiber/spectral-function-past-tense) and also plans to write an article about it.

the only problem is that just @stoplight/spectral-rulesets is almost 500kb 😅
so integrating it with our parser is not the best idea, but integrating it with CLI and Studio as separate functionalities - sure, would be awesome from DX-perspective.

[magicmatatjahu]

Our js parser alone weighs something like 700-800 kbs after minification, just to remind you 😏 Running spectral as a separate tool involves validating the whole spec twice, once on the parser-js side and once on the spectral side - with medium sized specs this can be a serious problem due to memory consumption and speed. Also spectral only supports 2.0.0 version of AsyncAPI and that's another problem.

In general, 500kb doesn't necessarily mean that it's actually added to the final bundle, because it's not the minified code but the weight of the code in the package.

[derberg]

In general, 500kb doesn't necessarily mean that it's actually added to the final bundle

something to check for sure

Also spectral only supports 2.0.0 version of AsyncAPI and that's another problem.

not a problem IMHO. They keep asyncapi format in @stoplight/spectral-formats afaik and I bet they will be happy if we could support them with regular updates of the format. I think it is easier than maintaining our own linter.

Anyway, I think we are going away from the initial goal of the issue 😄

[github-actions]

This issue has been automatically marked as stale because it has not had recent activity 😴

It will be closed in 120 days if no further activity occurs. To unstale this issue, add a comment with a detailed explanation.

There can be many reasons why some specific issue has no activity. The most probable cause is lack of time, not lack of interest. AsyncAPI Initiative is a Linux Foundation project not owned by a single for-profit company. It is a community-driven initiative ruled under open governance model.

Let us figure out together how to push this issue forward. Connect with us through one of many communication channels we established here.

Thank you for your patience ❤️

[github-actions]

This issue has been automatically marked as stale because it has not had recent activity 😴

It will be closed in 120 days if no further activity occurs. To unstale this issue, add a comment with a detailed explanation.

There can be many reasons why some specific issue has no activity. The most probable cause is lack of time, not lack of interest. AsyncAPI Initiative is a Linux Foundation project not owned by a single for-profit company. It is a community-driven initiative ruled under open governance model.

Let us figure out together how to push this issue forward. Connect with us through one of many communication channels we established here.

Thank you for your patience ❤️

[github-actions]

This issue has been automatically marked as stale because it has not had recent activity 😴

It will be closed in 120 days if no further activity occurs. To unstale this issue, add a comment with a detailed explanation.

There can be many reasons why some specific issue has no activity. The most probable cause is lack of time, not lack of interest. AsyncAPI Initiative is a Linux Foundation project not owned by a single for-profit company. It is a community-driven initiative ruled under open governance model.

Let us figure out together how to push this issue forward. Connect with us through one of many communication channels we established here.

Thank you for your patience ❤️

[magicmatatjahu]

In Parser v2 we have improved error handling, but we still need that library https://github.com/asyncapi/problem to use.

[github-actions]

This issue has been automatically marked as stale because it has not had recent activity 😴

It will be closed in 120 days if no further activity occurs. To unstale this issue, add a comment with a detailed explanation.

There can be many reasons why some specific issue has no activity. The most probable cause is lack of time, not lack of interest. AsyncAPI Initiative is a Linux Foundation project not owned by a single for-profit company. It is a community-driven initiative ruled under open governance model.

Let us figure out together how to push this issue forward. Connect with us through one of many communication channels we established here.

Thank you for your patience ❤️