search

sylvainlaurent / swagger-validator-maven-plugin

A maven plugin that validates swagger files in yaml and json formats
Apache License 2.0
11 stars 6 forks source link

Required properties in the models with inheritance #19

Closed oyeli closed 5 years ago

oyeli commented 5 years ago

There are such definitions in our spec: Foo: type: object properties: req2: type: string opt1: type: string

Bar: allOf:

We created issue https://github.com/OAI/OpenAPI-Specification/issues/1870 to clarify if above spec is valid. And got a positive answer. But only properties defined in the Bar entity are considered during the required properties check in the swagger-validator plugin. And I got the validation error. Could I add properties from all ref-models from "allOf" definition section when VisitableModel is created in SemanticValidationService?

giuliopulina commented 5 years ago

Hi @oyeli, I see your point and I think you are right. Honestly, I never thought about this possibility. I had a look at the codebase and I think this is not an easy task. Anyway, if you have some ideas and want to submit a PR, I can help reviewing :)

oyeli commented 5 years ago

Hi @giuliopulina! I want to submit PR for this issue. Could you please help me with the review: https://github.com/oyeli/swagger-validator-maven-plugin/commit/23adb4fc602772d2b4245ac0faa2b964e20100cb ? I also add a small fix regarding consumes and produces sections. For example we have a spec:

 swagger: '2.0'
info:
  version: '2'
  title: My spec
basePath: /path/2
produces:
- application/json
consumes:
- application/json
........
paths:
  /actions1:
    post:
      consumes: []
  /actions2:
    post:

In case of "actions1" it's an error to specify "consumes[]", because post operation must have a payload. But in case of "actions2" there is no errors, because "consumes" is inherited from general section. But now we can't differ these two cases because in the OperationWrapper consumes (and produces as well) is initialized as new ArrayList<>(). I've left consumes and produces sections in OperationWraper non-initialized to be able to distinguish these two cases. Please look at com.github.sylvainlaurent.maven.swaggervalidator.semantic.node.path#OperationWrapper in the PR. What do you think about it?

giuliopulina commented 5 years ago

Hi @oyeli, I had a look at your commit and I think that is good:

1) Consumes/Produces change: I think you are right on it. It would be great if you can add a couple of unit tests (both valid and invalid cases) for this validation. I think that, to be fully compliant with the spec, the validator should also check for validity of the mime type as specified here, but I don't know if there is any MimeType validator available in Java language:) 2) Required properties validation enhancement: I liked the idea to remove RequiredPropertiesValidator and merge the validation with InheritanceChainPropertiesValidator, it really simplifies things. 3) Cyclic model reference validation: it seems OK to me.

oyeli commented 5 years ago

Hi @giuliopulina! I've added MIME type validator with consumes and produces sections validation and tests for several cases. I've also added opportunity to specify custom MIME types in pom-configuration. Could you please look at my changes https://github.com/oyeli/swagger-validator-maven-plugin/commit/09a67866b9f49b6c646101ac572e6dca3407b5e7?

sylvainlaurent commented 5 years ago

PR #20 has been merged and release 1.2.6 published to maven central (may take a few hours). Thanks everyone!