Skip to content

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

Closed
#327
Closed
False positive breaking change reported when removing an optional field from a response#198
#327
Labels
bug
Milestone
2.1.0

Description

@julienrf
julienrf
opened on Nov 20, 2020

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.

Activity

  1. julienrf commented on Dec 17, 2020

    @julienrf
    julienrf
    on Dec 17, 2020
    ContributorAuthor

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

  2. gmcelhoe commented on Jan 14, 2021

    @gmcelhoe
    gmcelhoe
    on Jan 14, 2021

    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.

  3. cen1 commented on Jan 14, 2021

    @cen1
    cen1
    on Jan 14, 2021

    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.

  4. julienrf commented on Jan 15, 2021

    @julienrf
    julienrf
    on Jan 15, 2021
    ContributorAuthor

    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?

  5. joschi commented on Feb 28, 2021

    @joschi
    joschi
    on Feb 28, 2021
    Contributor

    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": [

  6. orange-buffalo commented on Jan 10, 2022

    @orange-buffalo
    orange-buffalo
    on Jan 10, 2022
    Contributor

    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?

  7. joschi
    added
    bug
    on Feb 28, 2022
  8. joschi
    added this to the 2.1.0 milestone on Feb 28, 2022
  9. joschi
    closed this as completedin #327on Feb 28, 2022

  10. myoffe commented on Mar 26, 2025

    @myoffe
    myoffe
    on Mar 26, 2025

    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.

  11. DrSatyr commented on Mar 26, 2025

    @DrSatyr
    DrSatyr
    on Mar 26, 2025
    Collaborator

    @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.

  12. myoffe commented on Mar 26, 2025

    @myoffe
    myoffe
    on Mar 26, 2025

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    bug

    Type

    No type

    Projects

    No projects

      Milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions