1045 - AddedOptionalProperty

[holm0563]

Can you please explain this logic?

Adding a property is never a breaking change on a response.

Adding a Optional Property would not be a breaking change on a request.

However adding a Required Property would be a breaking change on a request.

Happy to help fix this if I'm aligned with the expectations.

https://github.com/oasdiff/oasdiff/blob/main/docs/BREAKING-CHANGES-EXAMPLES.md

[holm0563]

In reference to: https://github.com/criteo/openapi-comparator/blob/main/documentation/rules/1045.md

[NextFire]

The vast majority of the rules come from https://github.com/Azure/openapi-diff/blob/main/docs/rules/

To determine whether they are breaking or not, you can often assess them from the perspective of users of client or validation libraries.

Adding a property is never a breaking change on a response.

Properties that are added to a response will generally be ignored during deserialization (e.g. https://docs.pydantic.dev/latest/concepts/models/#extra-data)

Adding a Optional Property would not be a breaking change on a request.

Because it is optional, if the new attribute is missing in the request when using an old client, it will still be fine.

However adding a Required Property would be a breaking change on a request.

The server expects the new property, so if the client is not upgraded to include it in the requests, the server will respond with a 400 error.

[holm0563]

It sounds like we are in agreement? However the rules of this library are:

1045 - AddedOptionalProperty
Description: Checks whether a property is added to the model from the previous specification. The model includes all the models that referenced by any request or response.

Cause: This is considered a breaking change.

[gricher-crto]

Adding an optional property in a response can be a breaking change when additionalProperties is set to false.

[holm0563]

See
https://github.com/oasdiff/oasdiff/blob/main/docs/BREAKING-CHANGES-EXAMPLES.md

Examples of breaking changes
...
[adding a new required property in request body is breaking]
...

Examples of non-breaking changes
[adding a media-type to response is not breaking]
[adding a new required property in response body is not breaking]
[adding a new required property under AllOf in response body is not breaking]
[adding a new required read-only property in request body is not breaking]
[adding a non-existent required property in request body is not breaking]
[adding a required property to response is not breaking]
....

Additionalproperties provide flexibility around API data not explicitly defined in your schema. Think of additionalproperties as the rules in an OpenAPI definition for handling unstructured data. Translated into code, additionalproperties become a dictionary, map, hashmap, or associative array, depending on the programming language.

https://www.apimatic.io/openapi/additionalproperties

I don't think additional properties changes how the defined properties behave.

[gricher-crto]

The additionalProperties keyword can also be a boolean (true or false, defaulting to true meaning any kind of additional property is allowed). It comes from the JsonSchema definition formalism : setting the additionalProperties to false means no additional properties will be allowed, cf https://json-schema.org/understanding-json-schema/reference/object#additionalproperties

I agree that adding a new property in response body is not breaking, if and only if additionalProperties is not false.

I think the current status of this rule (as breaking) probably comes from the fact that there was a time when some commonly-used tooling to generate clients (e.g., OpenApiGenerator I think, for some languages at least) were assuming additionalProperties was false by default. In practice, this means the client code was actual throwing exception when trying to parse object with extra properties. Even if the addition of a property should not have been a breaking change on paper, it was breaking in practice.

Given the above, do you see any issue leaving the rule as it is, since any user can select a chosen list of rules?

[holm0563]

Thanks for the great response.

I think this library should match the standards which does say additionalProperties defaults to true.

Do you have more information about "since any user can select a chosen list of rules?". Currently it seems hard to change this behavior and know if this rule should be thrown given that you would also have to verify additionalProperties is not set to false. If we had more severity levels (info, additive, warning, breaking change, error) it would be easier to select rules. See: #71

I've updated the PR to include additional tests I found that also seem to break with the current logic. I also fixed some of the new tests I had to include a check for additionalProperties not being false as mentioned.

[holm0563]

I have more changes I'll hopefully publish soon to more cleanly address this.

[holm0563]

See #73 and the change log for enhancements proposed in the readme file