Add check for duplicate operation ids

[marcelstoer]

Fixes #68

[coveralls]

Coverage increased (+0.07%) to 97.664% when pulling 4e6ed4b on marcelstoer:feat/dup-operationId into f8113cf on BigstickCarpet:master.

[marcelstoer]

James, what is your plan with the 4.x branch? Shall I create another PR for this to get back-ported?

[marcelstoer]

@BigstickCarpet any feedback yet?

[JamesMessinger]

@marcelstoer - I somehow completely missed this PR, so thank you for pinging me on it. It looks great! Thanks for adding a test too 👍

[JamesMessinger]

whoops! The browser tests failed because Array.includes() isn't supported on older browsers. I'll fix that and retry the tests

[marcelstoer]

No worries, how about that question about the 4.x branch above?

The browser tests failed

Those you did manually? Travis succeeded...

[JamesMessinger]

The browser tests (SauceLabs) don't run for PRs because Travis CI doesn't set environment variables (e.g. my SauceLabs credentials) for PR builds. So browser tests only run once the PR is merged into Master.

As for the 4.x branch... It's no longer actively supported/maintained. It's mostly just there for archival purposes, and maybe hotfixes. Is there a particular reason you're using 4.x instead of 5.x?

[marcelstoer]

Is there a particular reason you're using 4.x instead of 5.x?

I'm not, was just asking out of curiosity. Every project has a different policy.

[JamesMessinger]

ah, ok. Well let's just leave this on 5.x then. I don't think anybody's even using the 4.x branch anymore.

[argos83]

This change broke our builds. I wonder if this could have been released as a major version, or if not a minor version, but not a patch. I seems to me that adding validation logic should be considered as adding a feature. Wdyt?

[marcelstoer]

I would want this change to break my builds. It means that my Swagger files haven apparently been invalid so far but it went unnoticed. Or did your builds break for different reasons?

Can't comment on how the maintainer chooses to apply the version scheme.

I seems to me that adding validation logic should be considered as adding a feature.

True, but I guess you could also argue that it's a bug if a Swagger parser/validator doesn't check for duplicate IDs.

[BenSayers]

I am part of a large organisation who has thousands of builds that depend on libraries like this. When backward incompatible changes like this are released under a patch version and builds are unexpectedly broken it creates significant disruption. I completely agree we want to know when our Swagger files are invalid, however I don't believe the right way to achieve this is to without warning suddenly force everyone to deal with this, despite what other priorities they have.

[JamesMessinger]

@BenSayers - I apologize for the disruption. I didn't think this would be a breaking change, since it was just adding a check for something that's already part of the Swagger 2.0 spec. But you're right, I should have been more careful and released it as a major or minor version bump, not a patch.

For future reference, you can disable the Swagger spec validation in Swagger Parser by setting the validate.spec option to false. It will still validate your API against the Swagger schema. It just won't run the additional spec-compliancy checks.

var options = {
    validate: {
        spec: false
    },
};

SwaggerParser.validate("my-api.yaml", options, callback);