Mermade / oas-kit

Convert Swagger 2.0 definitions to OpenAPI 3.0 and resolve/validate/lint
https://mermade.org.uk/openapi-converter
BSD 3-Clause "New" or "Revised" License
707 stars 129 forks source link

New rule action for object keys #128

Closed pderaaij closed 5 years ago

pderaaij commented 5 years ago

Detailed description

For my custom rule set that I am creating I would love to have a rule that checks the keys of an object according to a pattern. For example, having a custom rule that ensures the property names are in snake_case.

This is especially tricky in the schema object as fields in the schema object are a schema object on its own. I've played around with several options within the current abilities but came short every time.

Possible implementation

I would like to introduce a new rule action that targets the keys of an object, applying a pattern to each and every key.

In my local environment I have this as a rule definition

 {
       "name":"schema-property-name-snake-case",
       "object": "schema",
       "enabled": true,
       "description": "parameter must be in snake_case",
       "keyPattern": { "property": "properties", "value": "^([a-z_])*$" }
   }

and introduced the following to the linter

             if (rule.keyPattern) {
                const { property, value } = rule.patternForKey;
                const target = (property) ? object[property] : false;

                if (target) {
                    const re = new RegExp(value);
                    const keys = Object.keys(target).forEach(key => {
                        options.context.push((options.context[options.context.length - 1] + '/' + key).split('//').join('/'));
                        ensure(rule, () => {
                            should(re.test(key)).be.exactly(true, rule.description);
                        });
                        options.context.pop();
                    });
                }
            }

I am curious if this is something we would like to add or is a bad approach.

** Continuing from: https://github.com/wework/speccy/issues/242

MikeRalphson commented 5 years ago

Let me check recent changes to our linter here don't already cover that, I may have added the functionality by porting https://github.com/wework/speccy/pull/239 or https://github.com/wework/speccy/pull/232 ...

pderaaij commented 5 years ago

With https://github.com/wework/speccy/pull/239 I couldn't get it working. Haven't tried it with the other

MikeRalphson commented 5 years ago

I'll try with your example rule and see what I can come up with.

MikeRalphson commented 5 years ago

When you say:

This is especially tricky in the schema object as fields in the schema object are a schema object on its own.

Do you mean you want to recurse into every sub-schema of a schema object? You may not realise that oas-kit's validator walks all properties of a schema object which are themselves schema objects, including arrays like oneOf etc and calls the linter on each one.

pderaaij commented 5 years ago

Well, that is part of the problem I think. I am relative new, so I might see it wrong, but:

in oas-validator/index.js on line 113 it states:

if (state.options.lint) state.options.linter('schema',schema,'schema',state.options);

So, as it recursively goes through the subschema it never propagates the key of the object, it justs calls it schema. Therefore I can't create a rule that looks at $key, as the linter never receives the key. This especially a problem in the following definition:

components:
  schemas:
    Health:
      type: object
      properties:
        instance_id:
          description: The id of the instance it's running on
          type: string
        report_as_of:
          description: Data of the health report
          type: string
          format: date-time

I haven't found a way to figure out a rule that validates if the elements in properties are matching a regex.

So yes, it does recursively walk through the schema object. There seems to be no way to get the key to use that as the target for the rule.

MikeRalphson commented 5 years ago

OK, should be able to fix that, we have a semver major release coming up anyway.

pderaaij commented 5 years ago

Cool! You want to change it in the validator or create a new rule action for it?

MikeRalphson commented 5 years ago

Yep, I've changed it in the validator.

pderaaij commented 5 years ago

That is a perfect solution as well. Will do as test run at the office tomorrow.

MikeRalphson commented 5 years ago

Great, let me know if any changes are needed to the linter. Thanks for the help.

bottoy commented 5 years ago

Hi @MikeRalphson , can you look as well to the responses object - suffers the same issue? Thx

Speccy rules on the "responses" object are not triggered as the validator only works on the 'response' object. We check that the response code is a number so the pattern rule of @pderaaij would be perfect for this...

It would be something like (oas-validator/index.js near line 778)

if (options.lint) options.linter('responses',op.responses,'responses',options);

I think. Although the parameters are somewhat guessed ...

pderaaij commented 5 years ago

I got it working by using this rule:

       {
            "name":"schema-property-name-snake-case",
            "object": "schema",
            "enabled": true,
            "description": "parameter must be in snake_case",
            "pattern": { "property": "$key", "omit": "properties/", "value": "^([a-z_])*$" }
        }

The omit is necessary as the key propagated is in the format of properties/<property>.

MikeRalphson commented 5 years ago

@bottoy thanks - that's a really good spot, will be going into the next release!

MikeRalphson commented 5 years ago

@pderaaij excellent. May I add your rule(s) to either a new Wiki page or a contrib.yaml rules file?

pderaaij commented 5 years ago

Feel free to add!

pderaaij commented 5 years ago

I've created #134 to enable rule that actually validates that the response key is an integer. Without this change I wasn't able to get a proper rule to validate this.

MikeRalphson commented 5 years ago

Successfully published: