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

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

Open
stevenhabex opened this issue Nov 25, 2016 · 4 comments
Open

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

stevenhabex opened this issue Nov 25, 2016 · 4 comments
Labels

Comments

@stevenhabex
Copy link

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
Copy link
Owner

zallek commented Nov 26, 2016

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
Copy link

koenj commented Jan 4, 2017

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
Copy link

koenj commented Jan 31, 2017

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

Koen

@zallek
Copy link
Owner

zallek commented Jan 31, 2017

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 :)

@zallek zallek added the feature label Jan 31, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants