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

feat: allow spectral rules to be defined in the validaterc #202

Merged
merged 1 commit into from
Oct 15, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 24 additions & 0 deletions src/.defaultsForValidator.js
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,30 @@ const defaults = {
'schemas': {
'json_or_param_binary_string': 'warning'
}
},
'spectral': {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: what'll happen if this new section is missing from the validaterc? Just want to ensure that not having this new section will not break current users.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We also need to update the init command to generate the new section we're adding here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since i'm calling getConfigObject if there is no spectral section in the validaterc, it will take the rules from the spectral rules from .defaultsForValidator.js and these changes also allow the init command to generate the new spectral section

'rules': {
'no-eval-in-markdown': "warning",
'no-script-tags-in-markdown': "warning",
'openapi-tags': "warning",
'operation-description': "warning",
'operation-tags': "warning",
'operation-tag-defined': "warning",
'path-keys-no-trailing-slash': "warning",
'typed-enum': "warning",
'oas2-api-host': "warning",
'oas2-api-schemes': "warning",
'oas2-host-trailing-slash': "warning",
'oas2-valid-example': "warning",
'oas2-valid-definition-example': "error",
'oas2-anyOf': "warning",
'oas2-oneOf': "warning",
'oas3-api-servers': "warning",
'oas3-examples-value-or-externalValue': "warning",
'oas3-server-trailing-slash': "warning",
'oas3-valid-example': "error",
'oas3-valid-schema-example': "error"
}
}
};

Expand Down
2 changes: 1 addition & 1 deletion src/cli-validator/runValidator.js
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,7 @@ const processInput = async function(program) {
// or the default ruleset
const spectral = new Spectral();
try {
await spectralValidator.setup(spectral);
await spectralValidator.setup(spectral, configObject);
} catch (err) {
return Promise.reject(err);
}
Expand Down
6 changes: 6 additions & 0 deletions src/cli-validator/utils/processConfiguration.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ const defaultConfig = require('../../.defaultsForValidator');
// global objects
const readFile = util.promisify(fs.readFile);
const defaultObject = defaultConfig.defaults;
// Clear spectral rules from defaultObject, only use the explicit values in the validaterc file
defaultObject.spectral.rules = {};
const deprecatedRuleObject = defaultConfig.deprecated;
const configOptions = defaultConfig.options;

Expand Down Expand Up @@ -42,6 +44,10 @@ const validateConfigObject = function(configObject, chalk) {
const allowedSpecs = Object.keys(defaultObject);
const userSpecs = Object.keys(configObject);
userSpecs.forEach(spec => {
// Do not check "spectral" spec rules
if (spec === 'spectral') {
return;
}
if (!allowedSpecs.includes(spec)) {
validObject = false;
configErrors.push({
Expand Down
2 changes: 1 addition & 1 deletion src/lib/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,8 @@ module.exports = async function(input, defaultMode = false) {
const spectral = new Spectral();

try {
await spectralValidator.setup(spectral);
configObject = await config.get(defaultMode, chalk);
await spectralValidator.setup(spectral, configObject);
} catch (err) {
return Promise.reject(err);
}
Expand Down
2 changes: 1 addition & 1 deletion src/spectral/rulesets/.defaultsForSpectral.yaml
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
extends: [[spectral:oas, off]]
formats: [oas2, oas3]
functionsDir": ../functions
functionsDir: ../functions
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Surprised spectral didn't complain about that

rules:
no-eval-in-markdown: true
no-script-tags-in-markdown: true
Expand Down
21 changes: 16 additions & 5 deletions src/spectral/utils/spectral-validator.js
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
const MessageCarrier = require('../../plugins/utils/messageCarrier');
const config = require('../../cli-validator/utils/processConfiguration');
const { isOpenApiv2, isOpenApiv3 } = require('@stoplight/spectral');
const { mergeRules } = require('@stoplight/spectral/dist/rulesets');
// default spectral ruleset file
const defaultSpectralRuleset =
const defaultSpectralRulesetURI =
__dirname + '/../rulesets/.defaultsForSpectral.yaml';

const parseResults = function(results, debug) {
Expand Down Expand Up @@ -46,7 +47,7 @@ const parseResults = function(results, debug) {
};

// setup: registers the oas2/oas3 formats, and attempts to load the ruleset file
const setup = async function(spectral) {
const setup = async function(spectral, configObject) {
if (!spectral) {
const message =
'Error (spectral-validator): An instance of spectral has not been initialized.';
Expand All @@ -57,12 +58,22 @@ const setup = async function(spectral) {
spectral.registerFormat('oas3', isOpenApiv3);

// load the spectral ruleset, either a user's or the default ruleset
const spectralRuleset = await config.getSpectralRuleset(
defaultSpectralRuleset
const spectralRulesetURI = await config.getSpectralRuleset(
defaultSpectralRulesetURI
);

// Combine user ruleset with the default ruleset
// The defined user ruleset will take precendence over the default ruleset
// Any rules specified in both will have the user defined rule severity override the default rule severity
await spectral.loadRuleset([defaultSpectralRulesetURI, spectralRulesetURI]);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this handles the case in which users define their own configuration with the Spectral format? But we don't let them override our default Spectral rules with that format?

I think this is what we want but as someone not very familiar with Spectral, I find it a little confusing. I appreciate the comments here, they are helpful, but will you add a couple more to provide context for why this is there?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Matthew correct me if I'm wrong but with this PR this is what the user can do now:

  1. They can override the severity level and disable/enable our spectral rules from the validaterc file.
  2. They can add any new custom rules to the validator using a custom rule set. Even if they don't include our spectral rules in their ruleset file, our spectral rules will still be loaded.
  3. If a user wants to override our spectral rules using their ruleset file, they would have to:
    • remove that spectral rule from the validaterc file
    • add a rule with the same name to their ruleset file

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Correct
  2. Correct
  3. Correct, but I realized that the omitting the spectral rule from the validaterc file did not remove it and still overrode the severity the user would specify.

This was due to the config.get() function getting the defaultObject when there was no validaterc found, this default object specified the same rules as the default validaterc and would take priority. So I added a new flag to prevent this defaultObject from overriding it so it acts as you should expect it to like 3.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sounds good although I'm slightly confused because if there is no validaterc file, the default object would be used, and the default object doesn't include the spectral rules. So it should be the same no spectral rules being defined, right?


// Combine default/user ruleset with the validaterc spectral rules
// The validaterc rules will take precendence in the case of duplicate rules
const userRules = Object.assign({}, spectral.rules); // Clone rules
try {
return await spectral.loadRuleset(spectralRuleset);
return await spectral.setRules(
mergeRules(userRules, configObject.spectral.rules)
);
} catch (err) {
return Promise.reject(err);
}
Expand Down
24 changes: 24 additions & 0 deletions test/spectral/mockFiles/mockConfig/.mockSpectral.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
extends: [[spectral:oas, off]]
formats: [oas2, oas3]
functionsDir: ../functions
rules:
no-eval-in-markdown: error
no-script-tags-in-markdown: true
openapi-tags: true
operation-description: true
operation-tags: true
operation-tag-defined: true
path-keys-no-trailing-slash: true
typed-enum: true
oas2-api-host: true
oas2-api-schemes: true
oas2-host-trailing-slash: true
oas2-valid-example: true
oas2-valid-definition-example: true
oas2-anyOf: true
oas2-oneOf: true
oas3-api-servers: true
oas3-examples-value-or-externalValue: true
oas3-server-trailing-slash: true
oas3-valid-example: true
oas3-valid-schema-example: true
160 changes: 160 additions & 0 deletions test/spectral/tests/spectral-validator.test.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,11 @@
const { getCapturedText } = require('../../test-utils');
const spectralValidator = require('../../../src/spectral/utils/spectral-validator');
const inCodeValidator = require('../../../src/lib');
const oas3InMemory = require('../mockFiles/oas3/enabled-rules-in-memory');
const path = require('path');
const config = require('../../../src/cli-validator/utils/processConfiguration');
const defaultConfig = require('../../../src/.defaultsForValidator');
const defaultObject = defaultConfig.defaults;

describe('spectral - test spectral-validator.js', function() {
let consoleSpy;
Expand Down Expand Up @@ -108,3 +114,157 @@ describe('spectral - test spectral-validator.js', function() {
);
});
});

describe('spectral - test config file changes with .spectral.yml', function() {
let validationResults;
let errors;
let warnings;

beforeAll(async () => {
// Set config to mock .spectral.yml file before running
const mockPath = path.join(
__dirname,
'../mockFiles/mockConfig/.mockSpectral.yaml'
);
const mockConfig = jest
.spyOn(config, 'getSpectralRuleset')
.mockReturnValue(mockPath);

// Below is used from enabled-rules.test.js
// set up mock user input
const defaultMode = false;
validationResults = await inCodeValidator(oas3InMemory, defaultMode);

// Ensure mockConfig was called and revert it to its original state
expect(mockConfig).toHaveBeenCalled();
mockConfig.mockRestore();

// should produce an object with `errors` and `warnings` keys that should
// both be non-empty
expect(validationResults.errors.length).toBeGreaterThan(0);
expect(validationResults.warnings.length).toBeGreaterThan(0);

errors = validationResults.errors.map(error => error.message);
warnings = validationResults.warnings.map(warn => warn.message);
expect(errors.length).toBeGreaterThan(0);
expect(warnings.length).toBeGreaterThan(0);
});

// One test should be an error instead of a warning compared to the enable-rules.test.js
it('test no-eval-in-markdown rule using mockFiles/oas3/enabled-rules-in-memory', function() {
expect(errors).toContain(
'Markdown descriptions should not contain `eval(`.'
);
expect(warnings).not.toContain(
'Markdown descriptions should not contain `eval(`.'
);
});

// Other tests should be their default severity levels
it('test no-script-tags-in-markdown rule using mockFiles/oas3/enabled-rules-in-memory', function() {
expect(errors).not.toContain(
'Markdown descriptions should not contain `<script>` tags.'
);
expect(warnings).toContain(
'Markdown descriptions should not contain `<script>` tags.'
);
});
});

describe('spectral - test config file changes with .validaterc', function() {
let validationResults;
let errors;
let warnings;

beforeAll(async () => {
// Create a mockObject from the defaultObject and mock config.get()
const mockObject = Object.assign({}, defaultObject);
mockObject.spectral.rules['no-script-tags-in-markdown'] = 'error';
const mockConfig = jest.spyOn(config, 'get').mockReturnValue(mockObject);

// Below is used from enabled-rules.test.js
// set up mock user input
const defaultMode = false;
validationResults = await inCodeValidator(oas3InMemory, defaultMode);

// Ensure mockConfig was called and revert it to its original state
expect(mockConfig).toHaveBeenCalled();
mockConfig.mockRestore();

// should produce an object with `errors` and `warnings` keys that should
// both be non-empty
expect(validationResults.errors.length).toBeGreaterThan(0);
expect(validationResults.warnings.length).toBeGreaterThan(0);

errors = validationResults.errors.map(error => error.message);
warnings = validationResults.warnings.map(warn => warn.message);
expect(errors.length).toBeGreaterThan(0);
expect(warnings.length).toBeGreaterThan(0);
});

// One test should be an error instead of a warning compared to the enable-rules.test.js
it('test no-script-tags-in-markdown rule using mockFiles/oas3/enabled-rules-in-memory', function() {
expect(errors).toContain(
'Markdown descriptions should not contain `<script>` tags.'
);
expect(warnings).not.toContain(
'Markdown descriptions should not contain `<script>` tags.'
);
});

// Other tests should be their default severity levels
it('test no-eval-in-markdown rule using mockFiles/oas3/enabled-rules-in-memory', function() {
expect(errors).not.toContain(
'Markdown descriptions should not contain `eval(`.'
);
expect(warnings).toContain(
'Markdown descriptions should not contain `eval(`.'
);
});
});

describe('spectral - test config file changes with .validaterc, all rules off', function() {
let validationResults;

beforeAll(async () => {
// Create a mockObject from the defaultObject and mock config.get()
const mockObject = Object.assign({}, defaultObject);
mockObject.spectral.rules = {
'no-eval-in-markdown': 'off',
'no-script-tags-in-markdown': 'off',
'openapi-tags': 'off',
'operation-description': 'off',
'operation-tags': 'off',
'operation-tag-defined': 'off',
'path-keys-no-trailing-slash': 'off',
'typed-enum': 'off',
'oas2-api-host': 'off',
'oas2-api-schemes': 'off',
'oas2-host-trailing-slash': 'off',
'oas2-valid-example': 'off',
'oas2-valid-definition-example': 'off',
'oas2-anyOf': 'off',
'oas2-oneOf': 'off',
'oas3-api-servers': 'off',
'oas3-examples-value-or-externalValue': 'off',
'oas3-server-trailing-slash': 'off',
'oas3-valid-example': 'off',
'oas3-valid-schema-example': 'off'
};
const mockConfig = jest.spyOn(config, 'get').mockReturnValue(mockObject);

// set up mock user input
const defaultMode = false;
validationResults = await inCodeValidator(oas3InMemory, defaultMode);

// Ensure mockConfig was called and revert it to its original state
expect(mockConfig).toHaveBeenCalled();
mockConfig.mockRestore();
});

// There should be no errors and 2 warnings for a non-spectral rule
it('test no spectral errors and no spectral warnings', function() {
expect(validationResults.errors.length).toBe(0);
expect(validationResults.warnings.length).toBe(2);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are the two warnings from a non-spectral rule?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, there is a warning for Definition was declared but never used in document, i'm clarifying the comment for this

});
});