Check the change of "required" for properties in Response schema

[guillaume-pansier]

Hello,

I would like to have the RequiredStatusChange fail for requests and responses, as for now there is no errors for the following contract change:

"MyEntityName": {
      "required": [
        "myProperty1",
-       "myProperty2"
      ],

I checked the implementation, and it bypasses the check for responses:

 if (oldParameter.IsRequired() == newParameter.IsRequired() || context.Direction == DataDirection.Response)
                return;

The description of the rule, "Checks whether an existing property's required status is changed from the previous specification.", doesn't mention anything about this bypass. Furthermore, I believe changing the "required" status in a response is a breaking change as well.

I'll see if I can investigate and push a PR later on.

[bubianchi-criteo]

Thank you for reporting.
The code you mention is found in ParameterComparator and concerns an operation parameter either in the path, query, header or cookie.
I think you are more concerned about required properties in schema here, aren't you?

[bubianchi-criteo]

The ResponseComparator uses eventually the SchemaComparator (see OpenApiDocumentComparator constructor that checks only if there is no new required property in the schema. An AddedRequiredProperty is reported in this case.

The SchemaComparator should report a new RemovedRequiredProperty message for properties that were removed from the required list.

In a Response,

• a RemovedRequiredProperty should be always considered as a breaking change error, since the existing clients could consider as an error a missing required property
• a AddedRequiredProperty is only an informative message since it should be ignored by the existing clients.

In a Request, it is more subtle:

• a RemovedRequiredProperty can be an acceptable (non-breaking) change error as long as the property is still in the schema and the server will ensure a backward compatible behavior.
• a AddedRequiredProperty is always a breaking change error since the existing won't provide the property.

[guillaume-pansier]

You're right, the problem I had was with the schema, it wasn't about parameters.

If I understand correctly what you're suggesting, there should be a new rule added to list of already existing rules right ?

I tried with a fork of the repository, so that we can discuss on something concrete.
The behavior seems OK to me, except for one thing: if a required property get removed entirely, the tool will now report 2 warnings: one about the "removed property", one about the "removed required". It seems a bit redundant to me, but maybe it's acceptable ?