False positive breaking change reported when removing an optional field from a response

[julienrf]

I’ve noticed that removing an optional field from a response is flagged as a breaking change.

Old OpenAPI document

{
  "openapi": "3.0.0",
  "info": {
    "title": "API to manipulate a counter",
    "version": "1.0.0"
  },
  "paths": {
    "/counter": {
      "get": {
        "responses": {
          "200": {
            "description": "The counter current value",
            "content": {
              "application/json": {
                "schema": {
                  "$ref": "#/components/schemas/counter.Counter"
                }
              }
            }
          },
          "400": {
            "description": "Client error",
            "content": {
              "application/json": {
                "schema": {
                  "$ref": "#/components/schemas/endpoints.Errors"
                }
              }
            }
          },
          "500": {
            "description": "Server error",
            "content": {
              "application/json": {
                "schema": {
                  "$ref": "#/components/schemas/endpoints.Errors"
                }
              }
            }
          }
        }
      },
      "post": {
        "responses": {
          "200": {
            "description": "The counter current value",
            "content": {
              "application/json": {
                "schema": {
                  "$ref": "#/components/schemas/counter.Counter"
                }
              }
            }
          },
          "400": {
            "description": "Client error",
            "content": {
              "application/json": {
                "schema": {
                  "$ref": "#/components/schemas/endpoints.Errors"
                }
              }
            }
          },
          "500": {
            "description": "Server error",
            "content": {
              "application/json": {
                "schema": {
                  "$ref": "#/components/schemas/endpoints.Errors"
                }
              }
            }
          }
        },
        "requestBody": {
          "content": {
            "application/json": {
              "schema": {
                "$ref": "#/components/schemas/counter.Operation"
              }
            }
          },
          "description": "The operation to apply to the counter"
        }
      }
    }
  },
  "components": {
    "schemas": {
      "counter.Operation.Set": {
        "type": "object",
        "properties": {
          "type": {
            "type": "string",
            "enum": [
              "Set"
            ],
            "example": "Set"
          },
          "value": {
            "type": "integer",
            "format": "int32"
          }
        },
        "required": [
          "type",
          "value"
        ]
      },
      "counter.Counter": {
        "type": "object",
        "properties": {
          "value": {
            "type": "integer",
            "format": "int32"
          },
          "timestamp": {
            "type": "string",
            "format": "date-time"
          }
        },
        "required": [
          "value"
        ]
      },
      "counter.Operation": {
        "oneOf": [
          {
            "$ref": "#/components/schemas/counter.Operation.Add"
          },
          {
            "$ref": "#/components/schemas/counter.Operation.Set"
          }
        ],
        "discriminator": {
          "propertyName": "type",
          "mapping": {
            "Add": "#/components/schemas/counter.Operation.Add",
            "Set": "#/components/schemas/counter.Operation.Set"
          }
        }
      },
      "counter.Operation.Add": {
        "type": "object",
        "properties": {
          "type": {
            "type": "string",
            "enum": [
              "Add"
            ],
            "example": "Add"
          },
          "delta": {
            "type": "integer",
            "format": "int32"
          }
        },
        "required": [
          "type",
          "delta"
        ]
      },
      "endpoints.Errors": {
        "type": "array",
        "items": {
          "type": "string"
        }
      }
    },
    "securitySchemes": {}
  }
}

Note the presence of the optional field timestamp in the counter.Counter component.

New OpenAPI document

{
  "openapi": "3.0.0",
  "info": {
    "title": "API to manipulate a counter",
    "version": "1.0.0"
  },
  "paths": {
    "/counter": {
      "get": {
        "responses": {
          "200": {
            "description": "The counter current value",
            "content": {
              "application/json": {
                "schema": {
                  "$ref": "#/components/schemas/counter.Counter"
                }
              }
            }
          },
          "400": {
            "description": "Client error",
            "content": {
              "application/json": {
                "schema": {
                  "$ref": "#/components/schemas/endpoints.Errors"
                }
              }
            }
          },
          "500": {
            "description": "Server error",
            "content": {
              "application/json": {
                "schema": {
                  "$ref": "#/components/schemas/endpoints.Errors"
                }
              }
            }
          }
        }
      },
      "post": {
        "responses": {
          "200": {
            "description": "The counter current value",
            "content": {
              "application/json": {
                "schema": {
                  "$ref": "#/components/schemas/counter.Counter"
                }
              }
            }
          },
          "400": {
            "description": "Client error",
            "content": {
              "application/json": {
                "schema": {
                  "$ref": "#/components/schemas/endpoints.Errors"
                }
              }
            }
          },
          "500": {
            "description": "Server error",
            "content": {
              "application/json": {
                "schema": {
                  "$ref": "#/components/schemas/endpoints.Errors"
                }
              }
            }
          }
        },
        "requestBody": {
          "content": {
            "application/json": {
              "schema": {
                "$ref": "#/components/schemas/counter.Operation"
              }
            }
          },
          "description": "The operation to apply to the counter"
        }
      }
    }
  },
  "components": {
    "schemas": {
      "counter.Operation.Set": {
        "type": "object",
        "properties": {
          "type": {
            "type": "string",
            "enum": [
              "Set"
            ],
            "example": "Set"
          },
          "value": {
            "type": "integer",
            "format": "int32"
          }
        },
        "required": [
          "type",
          "value"
        ]
      },
      "counter.Counter": {
        "type": "object",
        "properties": {
          "value": {
            "type": "integer",
            "format": "int32"
          }
        },
        "required": [
          "value"
        ]
      },
      "counter.Operation": {
        "oneOf": [
          {
            "$ref": "#/components/schemas/counter.Operation.Add"
          },
          {
            "$ref": "#/components/schemas/counter.Operation.Set"
          }
        ],
        "discriminator": {
          "propertyName": "type",
          "mapping": {
            "Add": "#/components/schemas/counter.Operation.Add",
            "Set": "#/components/schemas/counter.Operation.Set"
          }
        }
      },
      "counter.Operation.Add": {
        "type": "object",
        "properties": {
          "type": {
            "type": "string",
            "enum": [
              "Add"
            ],
            "example": "Add"
          },
          "delta": {
            "type": "integer",
            "format": "int32"
          }
        },
        "required": [
          "type",
          "delta"
        ]
      },
      "endpoints.Errors": {
        "type": "array",
        "items": {
          "type": "string"
        }
      }
    },
    "securitySchemes": {}
  }
}

Note that the field timestamp is absent from the counter.Counter component.

I expect such a change to be backward compatible: clients already know how to deal with the absence of the field timestamp since this one was optional in the first place.

[julienrf]

Would you be interested if I contribute a PR to fix this issue?

[gmcelhoe]

There is another way to look at -- if "optional" meant that sometimes the field is returned and sometimes it isn't (based on the attributes of the request or environment), then removing the field could break existing clients that were coded to expect it (even though the spec says that they shouldn't have).

So although you can assert in your case it is not a compatibility problem in your client, it does represent a potential compatibility problem for other clients.

[cen1]

It can be even more complicated. For example, Confluent Schema Registry defines several compatibility types: https://docs.confluent.io/platform/current/schema-registry/avro.html#compatibility-types

Nothing to do with OpenAPI but a related concept worth looking at.

[julienrf]

if "optional" meant that sometimes the field is returned and sometimes it isn't (based on the attributes of the request or environment), then removing the field could break existing clients that were coded to expect it (even though the spec says that they shouldn't have).

Shouldn’t this be modeled with a oneOf then?

[joschi]

Diff:

--- core/src/test/resources/issue-198-1.json	2021-02-28 18:07:23.000000000 +0100
+++ core/src/test/resources/issue-198-2.json	2021-02-28 18:07:46.000000000 +0100
@@ -114,10 +114,6 @@
           "value": {
             "type": "integer",
             "format": "int32"
-          },
-          "timestamp": {
-            "type": "string",
-            "format": "date-time"
           }
         },
         "required": [

[orange-buffalo]

We are experiencing the same issue as OP and also believe it is a backwards compatible change.
Would the maintainers be open to accepting a PR to fix this?

[myoffe]

Hi, I've found this issue because the current behavior is the opposite of how I expect openapi-diff to behave.
My scenario is like @gmcelhoe wrote - we have a response DTO that has a field that is sometimes returned, and sometimes not, depending on a request parameter.
anyOf is not a good solution because if you have multiple optional fields of this nature, you'd need to specify variants for every possible combination, and that's not possible to maintain.

At the very least this behavior should be configurable.
This applies to requests as well.

My suggestion is to add new configuration properties for request and response which control whether this is considered a breaking change or not.
In my opinion their default should be true, but assuming this will break existing flows, I'm fine with setting it to true in our own scripts.

p.s. oasdiff treats this case as a warning (which you can configure to fail the check) and bump.sh treats this as a breaking change.

[DrSatyr]

@myoffe, I agree with you on this topic. All decisions regarding breaking/non-breaking should be configurable from a user perspective, as this is a "holy war" question. Please look at this discussion, I've collected all concerns raised in this repo regarding this topic. So at the moment, we have BackwardIncompatibleProp.java. Please check it out, may be it can address your particular case.

[myoffe]

Thanks @DrSatyr I've checked the props file, I did not find something that can control this behavior.