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

Types extension #55

Merged
merged 26 commits into from
Dec 4, 2019
Merged

Types extension #55

merged 26 commits into from
Dec 4, 2019

Conversation

knidarkness
Copy link
Contributor

@knidarkness knidarkness commented Oct 24, 2019

This PR introduces a concept of extending and redefining types and rules.

Description

From this version, user can add custom fields into default objects in all levels, which will then validate using either existing validators (for existing types, either with custom rules, created by user).

To extend or define a type, user should create a file and pass it as a typeExtension parameter in the config. It should follow such pattern (quite similar to usual reducers):

module.exports = (types) => ({
  ...types,
  OpenAPIParameter: {
    ...types.OpenAPIParameter,
    resolveType: (node) => (node.allOf ? 'OpenAPIParameterWithAllOf' : 'OpenAPIParameter'),
  },
  OpenAPIParameterWithAllOf: {
    name: 'OpenAPIParameterWithAllOf',
    properties: {
      allOf: 'OpenAPIParameterPartial',
    },
  },
  OpenAPIParameterPartial: {
    ...types.OpenAPIParameter,
    name: 'OpenAPIParameterPartial',
  },
});

Here, user adds field allOf to Parameter type.

null value in the properties object is allowed and means that given property is a leaf in the type-tree.

To extend ruleset user can create another file and set it as customRules param of config with such contents:

class ValidateOpenAPIParameterPartial {
  static get rule() {
    return 'parameterPartial';
  }

  OpenAPIParameterPartial() {
    return {
      onEnter: (node, definition, ctx) => {
        const result = [];
        let validators = {};
        if (Object.keys(node).length === 1 && node.description) {
          validators = {
            description: ctx.getRule('oas3-schema/parameter').validators.description,
          };
        } else {
          validators = {
            ...ctx.getRule('oas3-schema/parameter').validators,
          };
        }
        if (node.in && node.in !== 'header') { // just as an example
          ctx.path.push('in');
          result.push(ctx.createError('Only header parameters can be extended with allOf', 'key'));
          ctx.path.pop();
        }
        const fieldMessages = ctx.validateFieldsHelper(validators);
        result.push(...fieldMessages);
        return result;
      },
    };
  }
}
class Other {
  static get rule() {
    return 'parameterWithAllOf';
  }

  OpenAPIParameterWithAllOf() {
    return {
      onEnter: (node, definition, ctx) => {
        const result = [];
        if (node.allOf.length > 2) {
          ctx.path.push('allOf');
          result.push(ctx.createError('Do not use more that 2 items in allOf for OpenAPI Parameter', 'key' /* whaterver else */));
          ctx.path.pop();
        }
        return result;
      },
    };
  }
}
module.exports = [
  ValidateOpenAPIParameterPartial, Other,
];

User should provide configuration file .openapi-cli.yaml in the root of the definition directory.

typeExtension: typeExtension.js # relative path to the extended types
customRules: customRules.js # relative path to the custom visitors
rules:
  parameterPartial: warning # allowed values are 'error', 'warning' and 'off'
  parameterWithAllOf: off # by default custom rules are 'on' and set to 'error' severity

@knidarkness knidarkness marked this pull request as ready for review October 28, 2019 08:34
src/resolver.js Outdated Show resolved Hide resolved
Co-Authored-By: Adam Altman <adam@rebilly.com>
Copy link
Member

@RomanHotsiy RomanHotsiy left a comment

Choose a reason for hiding this comment

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

Added a few comments

src/loader/index.js Outdated Show resolved Hide resolved
src/loader/index.js Outdated Show resolved Hide resolved

for (let i = 0; i < alteredFields.length; i++) {
const pathToDefaultType = `${__dirname}/../types/${extension.properties[alteredFields[i]]}.js`;
let alteredType;
Copy link
Member

Choose a reason for hiding this comment

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

I think you can assign the value directly on this line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

agree

class parameterAllOf {
constructor(config) {
this.config = { ...config };
switch (this.config.level) {
Copy link
Member

Choose a reason for hiding this comment

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

Do we need every custom rule to add this code? Maybe we can provide some base class with this behaviour handled. Also, where is this level used later? Is it propagated down the visitor invocations?

Also, I thing we should export constants for those 1, 2, 3, 4 numbers :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted to make custom rules completely 0-dependency. We can just inherit from AbstractVisitor class and don't do this manually

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, numbers are in constants elsewhere

Copy link
Member

Choose a reason for hiding this comment

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

Hmm. I think 0-dep makes sense. Maybe, instead we can just keep string values and adjust other related code to accept them instead of/ additionally to number values?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmmm, yes, that might work fine

src/visitors/rules/custom/parameterAllOf.js Outdated Show resolved Hide resolved

OpenAPIParameter() {
return {
onEnter: (node, _, ctx, unresolved, traverseTools) => {
Copy link
Member

Choose a reason for hiding this comment

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

how to add an error from this validator? Could you add an example of validating the description field to be a string (using some custom code)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

for now we allow types (re)definition, not so much custom validators. we can (not sure if already) load additional rules (with are the same syntax/structure) as our own ones and run them on types we want

@knidarkness knidarkness self-assigned this Nov 27, 2019
@knidarkness knidarkness merged commit 6ea8e07 into master Dec 4, 2019
@knidarkness knidarkness deleted the types-extension branch December 4, 2019 14:57
@knidarkness knidarkness restored the types-extension branch December 4, 2019 14:57
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.

None yet

3 participants