Response parser removes 'required' properties

[Alex]

lib/swagger-mock-validator/validate-spec-and-mock/validate-parsed-mock-response-body.ts removes the 'required' property from schemas before performing validation. I see this was intentional all the way back to the first release of this tool, but it seems like a bug to me.

If, in my swagger spec, I mark a property as required, but it is not in my mock interaction response, this seems like it should fail validation. My spec states something is required, but it is not provided - thus, it is invalid. As a side effect of this, almost no contract-breaking changes in your swagger files response models can trigger a failure. If the provider renames a field, or removes a field, or adds a new field, all of these will just be treated as optional changes, so the validation will pass. In reality, removing a required field from an API is a breaking change and will break my application.

[Ben Sayers]

Hi @valistar

Yes we are intentionally removing the required property from request schemas as part of our validation process. I'll explain the thinking behind this via an example:

Imagine we have a User Service that exposes an API that allows consumers to get user details (e.g. GET /user/{id}). Let's say this endpoint returns a user entity that contains 20 properties, all of them required properties.

Let's also imagine we have a Billing Service that is written by a seperate team, lives in a seperate code repository to the User Service and is deployed independently. Let's say one of the features of the Billing Service is that it sends the user an email reminding them when a payment is due soon. In order to implement this the Billing Service calls the User Service API to get 3 properties of the user: first name, last name and email address.

Finally, let's imagine the Billing Service has tests for this functionality and in those tests the User Service API is mocked out. The swagger-mock-validator tool is used in both the User Service and Billing Service to ensure this mock is compatible with the User Service Swagger file anytime either service is changed.

If the swagger-mock-validator does not remove the required property from the user entity schema there would be 2 negative consequences in this scenario:

• The Billing Service mock would have to include all 20 of the required properties, even though only 3 are actually needed.
• If the User Service was to add a new required property to the user entity it would cause the User Service build to fail because the mock will not contain this extra property (a false positive). In order to make this change the User Service would first have to go ask the Billing Service team to add this new property to their mock. If the user entity had "additionalProperties" set to false on the user object it would complicate the process to make this change even further.

Now let me explore the the side effects you raised:

• If the User Service adds a new property (required or optional), assuming the Billing Service has been written to follow Postel's law then nothing will be broken by this change.
• If the User Service removes a property then the behaviour of swagger-mock-validator will depend on what the user entity schema has set in "additionalProperties". If it is false then when the User Service tries to validate the Billing Service mock it will fail because the Billing Service mock contains the removed field and additional properties are not permitted by the schema. If it is true then, unfortunately, swagger-mock-validator will consider this change valid even though it is not (a false negative).
• A rename is the same as removing one property and adding another

Since neither option is flawless we made the tradeoff decision to remove the required property from request schemas and as a result we have this false negative case. We believe this is less disruptive to teams using this tool compared to the false positive case.

I'd make these two suggestions to help reduce the impact of the false negative case:

• If possible, always set additional properties to false on all response schema objects. This avoids the false negative case completely.
• Using a tool to detect backward incompatible changes to the swagger file. We have been working on a tool called openapi-diff that will detect breaking changes between two swagger/openapi files that may help, although it is still a work in progress.

Let me know if you still consider this an issue and we'll try and come up with a solution for you.

[Alex]

I hadn't thought about setting additionalProperties to false, this should work for us. Thanks for the response.

[Alex]

By design