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

[WIP] fix(rulesets): do not mix up irrelevant rules within rulesets #677

Closed
wants to merge 1 commit into from
Closed

Conversation

nulltoken
Copy link
Contributor

@nulltoken nulltoken commented Oct 12, 2019

Checklist

  • Tests added / updated
  • Docs added / updated

Does this PR introduce a breaking change?

  • Yes
  • No
  • Maybe (depending if modifying the content of a core built-in ruleset is tracked as a breaking change)

The doco states Spectral comes with two built in rulesets - one for OpenAPI v2 (spectral:oas2), and one for OpenAPI v3 (spectral:oas3).

However, it looks like those built-in rulesets do not only contain relevant rules.

  • spectral:oas2 contains rules that only target an oas3 document
  • spectral:oas3 contains rules that only target an oas2 document

Whereas spectral:oas extends both spectral:oas2 and spectral:oas3, surprisingly, the code shows that

  • spectral:oas2 also extends spectral:oas
  • spectral:oas3 also extends spectral:oas

This proposed change (tries and) fixes this so that

  • spectral:oas2 contains rules that only target an oas2 document (or support both oas2 and oas3 documents)
  • spectral:oas3 contains rules that only target an oas3 document (or support both oas2 and oas3 documents)
  • no functional change to spectral:oas which contains the union of spectral:oas2 and spectral:oas3
oas oas2 oas3
Before 53 rules 53 rules 53 rules
After 53 rules 39 rules 41 rules

⚠️ I realize that this is an awful lot of code to initiate a discussion, but my main goal was to see how much Spectral internally relied on those surprising oas -> oas2 -> oas / oas -> oas3 -> oas extends and if my spike would put under the light meaningful failures of some tests.

Screenshots

This test demonstrates the initial finding. Once the patch is applied, it doesn't fail any more.

image

@nulltoken nulltoken changed the title fix(rulesets): do not mix up irrelevant rules within rulesets [WIP] fix(rulesets): do not mix up irrelevant rules within rulesets Oct 12, 2019
@nulltoken
Copy link
Contributor Author

I realize that this is an awful lot of code to initiate a discussion, but my main goal was to see how much Spectral internally relied on those surprising oas -> oas2 -> oas / oas -> oas3 -> oas extends and if my spike would put under the light meaningful failures of some tests.

Ah!!! The build points out to some failures. Time to dig in 😉

@P0lip
Copy link
Contributor

P0lip commented Oct 12, 2019

The long term plan was to remove spectral oas2 and oas3 rulesets and keep oas only, which as you noticed contains all rules. I'll write a brief recap of the story behind the change in the morning.

@nulltoken
Copy link
Contributor Author

nulltoken commented Oct 13, 2019

The long term plan was to remove spectral oas2 and oas3 rulesets and keep oas only, which as you noticed contains all rules.

Ah! This explains a lot of things, then. To be honest, I was starting to feel a little bit uneasy to see so little tests failing as I was working through the code with a machete.

Considering this, I'm going to do three things:

  • Close this PR which is no longer relevant
  • Open a new one to make it more obvious for the new contributor (from the code standpoint) that oas2 and oas3 are only here for backward compatibility.
  • Update a few things in Detect unused OAS2 definitions and OAS3 component schemas #635 to not make it harder to get rid of oas2 and oas3 later.

I'll write a brief recap of the story behind the change in the morning.

👍

@P0lip
Copy link
Contributor

P0lip commented Oct 13, 2019

@nulltoken
So yeah, here is the long story short.
Until 4.1 version (where formats were introduced), Spectral had no way to effectively target a given specification. therefore if you wanted to lint an OAS2 document, you had to create a new spectral instance, and load the OAS2 ruleset, which extended the OAS ruleset, as it does now.
If you had another document, written in OAS3 this time around, and wanted to lint it, you had to create yet another instance of Spectral and load the OAS3 ruleset, which similarly to the OAS2 ruleset, extended OAS ruleset.
This is all due to the fact rules could not be registered again specification, so all of them apply to linted document.

Making OAS2 and OAS3 rulesets private was not really an option for us at the time formats feature was released, as it was a minor release, hence we couldn't afford introducing any breaking changes.

I hope it makes sense. Let me know if you have any further questions, happy to answer them.

Open a new one to make it more obvious for the new contributor (from the code standpoint) that oas2 and oas3 are only here for backward compatibility.

Yeah, that would be awesome!

Update a few things in #635 to not make it harder to get rid of oas2 and oas3 later.

Ditto, if you are eager to look into this, that'd be great!
We can discuss potential solutions in this PR if you already have some thoughts you'd like to share with us.

@philsturgeon
Copy link
Contributor

I think we could probably close this down and focus on #561 which will delete a lot of the old confusing things. Now that we have formats and oas2- and oas3- on everything we can make one mega-ruleset.

@nulltoken
Copy link
Contributor Author

Closing in favor of #561

@nulltoken nulltoken closed this Oct 16, 2019
@nulltoken nulltoken mentioned this pull request Oct 16, 2019
7 tasks
@nulltoken nulltoken deleted the ntk/rulesets branch March 5, 2020 07:39
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

Successfully merging this pull request may close these issues.

3 participants