parser-js icon indicating copy to clipboard operation
parser-js copied to clipboard

Integrate stoplight/spectral in parser

Open magicmatatjahu opened this issue 3 years ago • 10 comments

The whole validation/parsing process in our parser consists of 3 steps:

  • validation via AJV based on spec JSON Schema
  • custom validation(s) which cannot be done with AJV because we have to compare values between different parts of the specification, e.g. security in servers and corresponding values in components/securityRequirements. Here we also perform additional actions like applying traits etc
  • creating from validated AsyncAPI spec the models - the instance of the AsyncApiDocument class

The above operations are not complicated and are sufficient for now (however, we do not validate all edge cases from point 2, such as lack of payload's/header's example validation etc.). However, we are missing one thing that every good linter/parser/validator has, support for custom rules. I am thinking about using spectral in our parser to add such functionality. The advantage of this solution would be the complete elimination of the first two steps, because it would be done by the spectral itself and only our parser would be a wrapper for it. At the end we still need to models/intent API.

But to go in this direction we should first test spectral and see if we can support all the cases we have in AsyncAPI like:

  • validating extensions based on their schema
  • validating bindings based on version and schema
  • validating and parsing custom schema formats like Avro, Raml etc.
  • validating custom ref behaviour if we decide to change current $ref logic.

Currently spectral has in source code support for AsyncAPI with some additional rules. You can check it here https://github.com/stoplightio/spectral/blob/develop/packages/rulesets/src/asyncapi/index.ts Here comes the problem because in order to have control over these rules we should ask the spectral people if we can extract these rules from their source code and move them to our organization under @asyncapi/spectral package.

Then, with every spec release we could add a new rules, without waiting for an update from stoplight. Additionally, such a package would have two advantages:

  • people using spectral alone would have the ability to validate AsyncAPI with the spectral CLI (only have to fetch that new rules)
  • we as an AsyncAPI organization would have the ability to use those rules in our parser during validation + support in our CLI/ServerAPI/Studio.

I'm not a fan of reinventing the wheel (unless there is a lack of functionality on the market or no feedback from the package maintainers) but we should try to reuse spectral in our parser instead of validating in our own way. Of course if Spectral will not meet our requirements then we should think about our own linter, but lets see.

What do you think about it?

cc @smoya @jonaslagoni @fmvilas @derberg

magicmatatjahu avatar Feb 21 '22 16:02 magicmatatjahu

I believe everything you aim to do will be possible if the project takes a few Spectral modules as dependencies. In terms of owning the AsyncAPI ruleset built into Spectral, that's worth mentioning on their Discord server, I think. However, rulesets are composable. AsyncAPI could host their own ruleset at a URL endpoint, and folks could use that. Here's an example of how that looks: https://github.com/kevinswiber/spectral-custom-function-example

kevinswiber avatar Feb 23 '22 19:02 kevinswiber

@kevinswiber Thanks for comment! I tested spectral in my POC and as you wrote, spectral should handle all of our use cases. Also rather to host our ruleset as URL (but we should think of that) we should have @asyncapi/spectral package with that ruleset and using cdn we can handle that use case with URL :)

magicmatatjahu avatar Feb 28 '22 12:02 magicmatatjahu

I'm so excited about the possibility to use Spectral (I've never used it before, but read about it's potential)! Simplifying our parse logic, and boosting our possibilities to validate our documents into a next step where we just build validation rules is something that sounds like a big achievement. It would be awesome if we can get a more detailed idea of what we would need to do in order to move forward (in a form of GH Issues maybe) so we can include this for the next major as well (as this also affects other possible big changes we considered such as rewriting the parser to TS).

Thanks @magicmatatjahu for taking the time 👍

smoya avatar Mar 02 '22 15:03 smoya

thanks, @magicmatatjahu. This looks promising. I have been playing around with spectral to see how it works with bindings, and there are a few issues that I need to mention here.

  1. https://github.com/stoplightio/spectral/issues/2093
  2. If we validate an object against its schema, there will be no customized message for what exactly is wrong with the object. In other words, it will just give the info of the Spectral rule that the object is being validated under, not the schema rule that the object is violating. (maybe it's just how it's supposed to work?)

KhudaDad414 avatar Mar 16 '22 07:03 KhudaDad414

@KhudaDad414 Thanks for that comment!

I know about these errors, but it won't be a big problem for us. I would opt that binding and extension schemas should be added to the parser as registry and then a spectral rule be created based on them with custom logic. We should have something like binding and extension registry (like we have with custom parsers). In this month I will try to create a draft PR with this idea, then you will see what I mean :)

magicmatatjahu avatar Mar 16 '22 10:03 magicmatatjahu

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

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 :heart:

github-actions[bot] avatar Jul 15 '22 00:07 github-actions[bot]

@magicmatatjahu would you mind removing stale label?

smoya avatar Jul 18 '22 08:07 smoya

For the record, here is the list of tasks regarding Spectral rules that should be implemented: https://github.com/stoplightio/spectral/issues/2100

smoya avatar Jul 27 '22 14:07 smoya

In reference to https://github.com/asyncapi/parser-js/issues/481#issuecomment-1198369348

can you share what the new error would look like after integrating spectral? some examples? I understand you want to take 1:1 what spectral offers and this is confusing for me

From the issue description, I understand spectral goal for us is to replace custom validation - make sense 💯 But in the linked issue, you also refer to severity and governance. So do you also mean that you want to allow users to use spectral through the parser? so they provide spectral config files to our parser? I mean, is your idea to not only use spectral for our spec-related validation?

also, not clear to me how you want to replace AJV. Spectral uses AJV internally or?

derberg avatar Aug 16 '22 10:08 derberg

@derberg

can you share what the new error would look like after integrating spectral? some examples? I understand you want to take 1:1 what spectral offers and this is confusing for me

You can look on error(s) shape here https://github.com/stoplightio/spectral/blob/develop/packages/rulesets/src/asyncapi/tests/asyncapi-operation-operationId-uniqueness.test.ts#L114 next to the message, path and severity you have also range (as I remember) to determine in which place you have error - similar to our location. It is similar to our shape, but we have some variations related to the refs etc. and in spectral we have generic shape for everything.

From the issue description, I understand spectral goal for us is to replace custom validation - make sense 💯 But in the linked issue, you also refer to severity and governance. So do you also mean that you want to allow users to use spectral through the parser? so they provide spectral config files to our parser? I mean, is your idea to not only use spectral for our spec-related validation?

Yes, I wanna introduce custom config for spectral :) Of course we need to "hardcode" base rules to validate required things like whole spec based on JSON Schema, uniqueness of operationID etc, but people will have possibility to write their own rules to apply company governance.

also, not clear to me how you want to replace AJV. Spectral uses AJV internally or?

Spectral has built-in some validation function, to check if given value exists etc and one of these function is function based on AJV to validate some value/JSON on JSON Schema - check our rule to validate AsyncAPI based on our spec JSON Schema: https://github.com/stoplightio/spectral/blob/develop/packages/rulesets/src/asyncapi/functions/asyncApi2DocumentSchema.ts#L104

Let me know if I have cleared up any doubts.

magicmatatjahu avatar Aug 22 '22 10:08 magicmatatjahu

Issue is resolved in v2 ParserJS - now in https://github.com/asyncapi/parser-js/tree/next-major branch

magicmatatjahu avatar Oct 25 '22 08:10 magicmatatjahu