feat: custom functions

[P0lip]

Closes #311

Checklist

• Tests added / updated
• Docs added / updated

Does this PR introduce a breaking change?

• Yes
• No

Here it is.
The PR is likely to introduce a breaking change, as I shuffled some methods around and perhaps removed a few.
There is still cleanup left to be done, mostly around the usage of deprecated methods in our code as well as tests.
I'm not really sure whether we are good with making breaking changes, as we are not planning to release 5.0 anytime soon, and the code is meant to be shipped as a part of 4.1, so I suspect I might need to revert a few changes and be more careful with the cleanup.

The code is ready to be reviewed, I covered all the cases that I could think of.
I'll write a word on the updates I had to make to tests as well as the rulesets functions.

A note on functions.
This is the final syntax I implemented:

"functions": [
"foo",
["bar", { "type": "string" }]
]

There are a couple of fixtures and tests covering these, so you can take a look to get a grasp of that in action.
The syntax follows the other JS-based tooling such as Babel etc., therefore we should be aligned with others.
The object pattern felt very awkward in practice.

@philsturgeon
I believe the custom functions deserve their own document, thoughts?

Moreover, I'm planning to start a developer guide or something similar, as everything gets more complicated and you need to be wary of certain patterns.

I'm planning to do that in the scope of this PR, but beforehand, if you could at least take a look at the code and the functions concept I eventually went with, that would be great.

[philsturgeon]

Part of the plan with Spectral v4.1 was to add loadRulesets and custom functions, but something I didn't emphasize enough was that this was meant to be used throughout Spectral itself. We have some rather confusing code for our internal rulesets, and loadRulesets, formats, etc is meant to make it a bit more simple.

People should be able to include spectral:oas and have it load all the rules, without needing to specify oas2 or oas3 (thanks formats!) but the way our oas2 and oas3 are build don't allow for it.

We should be able to have the current oas2 ruleset look like this:

  "extends": ["spectral:oas2", "spectral:oas3"],
  "formats": ["oas2", "oas3"],
  "rules": {
    "operation-2xx-response": {

Then oas2 and oas3 specific stuff can stay where it is. They can use the same functionDir because all the functions are the same for 2 and 3, but this hopefully means we can get rid of export const rules = async () => readRulesFromRulesets(require.resolve('./index.json')); and this stuff:

export const commonOasFunctions = (): FunctionCollection => {
  return {
    oasPathParam: require('./functions/oasPathParam').oasPathParam,
    oasOp2xxResponse: require('./functions/oasOp2xxResponse').oasOp2xxResponse,
    oasOpSecurityDefined: require('./functions/oasOpSecurityDefined').oasOpSecurityDefined, // used in oas2/oas3 differently see their rulesets for details
    oasOpIdUnique: require('./functions/oasOpIdUnique').oasOpIdUnique,
    oasOpFormDataConsumeCheck: require('./functions/oasOpFormDataConsumeCheck').oasOpFormDataConsumeCheck,
    oasOpParams: require('./functions/oasOpParams').oasOpParams,
  };
};

This will involve a bunch of change in our tests.

It would be ok to split this work into another issue and follow up PR, but it would still need to be done for the 4.1 launch, otherwise we are suggesting features we dont use, and carrying around this extra baggage instead of using our nice new functionality. :)

[philsturgeon]

Other cleanup, lets get rid of this:

  if (parseInt(spec.data.swagger) === 2) {
    command.log('Adding OpenAPI 2.0 (Swagger) functions');
    spectral.addFunctions(oas2Functions());
  } else if (parseInt(spec.data.openapi) === 3) {
    command.log('Adding OpenAPI 3.x functions');
    spectral.addFunctions(oas3Functions());
  }

They're the same thing anyway! We don't need to check the object to detect format, formats do that, and now functions are gonna be loaded through the ruleset right?

[philsturgeon]

One last thing, we are getting deprecation warnings in the CLI because we use this old stuff:

▶ ./bin/run lint pets-mini.yaml --fail-severity=error
Adding OpenAPI 2.0 (Swagger) functions
Method `addRules` has been deprecated since version 4.1, use `loadRuleset` instead.
    at /Users/phil/src/spectral/src/cli/commands/lint.ts:213:12
OpenAPI 2.0 (Swagger) detected

:D

[P0lip]

@philsturgeon I'm aware of deprecation warnings, but we don't have any alternative solutions just yet 🤔 We'd really need to get that setRuleset thingy in.

[P0lip]

The code should be pretty much ready. I will just add a few extra test cases and write docs, so hold on with the reviews yet.

[P0lip]

Docs added 😅

[P0lip]

@XVincentX https://dev.azure.com/vncz/vncz/_build/results?buildId=1117 any clue why azure jobs get stuck at $ copyfiles -u 1 "dist/rulesets/oas*/functions/*.js" ./ all the time?

[philsturgeon]

So begin the conflicts!

[marbemac]

I feel like we should move the relevant bits of this over to contributing, and delete this file.

[marbemac]

Again, I'm not sure that this is what it should be doing?

We have a json schema rule that should be used to validate target data.

This JSON schema should describe the functionOptions available in this function. Eventually we'll use it to render documentation for the ruleset and custom functions, and I guess we could also use it to validate the functionOptions the end user passes in?

[P0lip]

This JSON schema should describe the functionOptions available in this function. Eventually we'll use it to render documentation for the ruleset and custom functions, and I guess we could also use it to validate the functionOptions the end user passes in?

Yeah, we validate the function options that are passed in.

[marbemac]

This is a very constrictive API - we'll never be able to change it or add any options without introducing a breaking change. I suggest something like this instead:

public async loadRulesets(uris: string[]) {

With this, we can easily add an optional opts or other argument after uris in the future, as needed. I also pluralized the method, since it supports multiple.

[P0lip]

Yeah, you are right. I'll pull over the code I wrote herehttps://github.com//pull/460 it has a very similar APIs. I had to change it because of the reasons you mentioned.

[marbemac]

This is confusing. The truthy function is defined further up, but I can still use it in my ruleset. Can I not use any of the functions defined in the oas2 ruleset in my own ruleset file?

[P0lip]

Can I not use any of the functions defined in the oas2 ruleset in my own ruleset file?

You can use it.

[P0lip]

I'm not really sure how to explain it any better 🤔
As you could have all observed already, writing docs is not my biggest asset 😆

[P0lip]

I feel like we should move the relevant bits of this over to contributing, and delete this file.

It's up to you all.
If the information is not that useful we could even get rid of it entirely, so that we don't have to update it in future.
I wrote it because I thought it might be helpful for others, but if you believe it makes more harm than good, it's okay, I'm totally okay with removing it.

[P0lip]

I feel like this is ready for another pass.
Regarding documentation - I suppose we can polish it in another PR.

[philsturgeon]

Note that this breaks the readme

We’re gonna look at docs in another PR

[philsturgeon]

The docs you’ve written are excellent but need a little polish. I’ll take care of this in another PR on Monday, using the comments here to start and just giving it a general once over.

[marbemac]

Ah, I was just polishing a few bits up - will open a separate PR :).

Awesome to get this merged, great job @P0lip !