zallek / swagger-diff

Compute the diff between two swagger API specifications.
https://zallek.github.io/swagger-diff/
58 stars 30 forks source link

Separate rule for add-required-object-property #18

Open stevenhabex opened 7 years ago

stevenhabex commented 7 years ago

Very nice work, thanks for your efforts.

We have the following situation : we are using object ExportConfiguration in our API. It is being exposed as an output parameter. We added a new property to this ExportConfiguration object which is required.

Because of the fact we set the rule "add-required-object-property" to cause an error, it will return an error, which makes sense. However, since this object is only being used as an output parameter in the API and it is never being used as an incoming parameter, it does not really break the API.

Does it make sense for the swagger-diff library to have separate rules "add-required-object-property-on-input-parameter" and "add-required-object-property-on-output-parameter"? Or do you suggest another workaround?

zallek commented 7 years ago

Could be useful. Though, could you explain what are the required propreties in outputs for ? The idea of required properties for outputs is strange isn't ?

Mean while you can disable the rule https://github.com/zallek/swagger-diff#configure-specific-rules

koenj commented 7 years ago

Now there is only rule, named "add-required-object-property".

It makes sense to impose this rule on input parameters: adding a required property on an input object, breaks the api because older clients will not provide this property.

However, in our opinion, it does not make sense to impose this rule on an output parameter: adding a property (even a required one) on an output object, does not break the api; older clients just ignore this property in deserializing and do not care about the new property.

What is your opinion? In our opinion, there are 2 solutions:

  1. do not impose this rule on output parameters
  2. split the "add-required-object-property" rule into 2: "add-required-object-property-on-input-parameter" and "add-required-object-property-on-output-parameter" which can both be set to mimic the current behavior.
koenj commented 7 years ago

If you take a stand on this, I'm willing to create a PullRequest to implement it.

Koen

zallek commented 7 years ago

Sorry @koenj @stevenhabex for the delay answer. I understand the problem.

2 rules are inconvenient for output object required property:

Though I'm agree it could be nice to separate input from outputs.

The problem is that currently for objects in definitions we don't know if there are used as input or output. This rule can be easily implemented for objects in parameters inlined but not so much the parameters which linked to a definition. Example:

screen shot 2017-01-31 at 19 29 50

One solution would be to dereference internals references. The problem with that is that it duplicates the definition everywhere it's being used. So if 2 different operations are using the same definition which changes. It creates 2 duplicate diffs. That's why currently definitions are deferenced.

Another solution could be to recursively tag definitions used as input (by just adding _usedAsInput: true on definitions). It would require to loop on all input paramters, tag referenced definitions and their sub definitions (If a definition reference an other definition). With this tag we could easily in the rule test if it's used as input).

PS: If you want to work on a PR for that, I would be with pleasure :)