Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
julienrf opened this issue Nov 20, 2020 · 6 comments · Fixed by #327
Closed
Labels
Milestone

Comments

@julienrf
Copy link
Contributor

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
Copy link
Contributor Author

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

@gmcelhoe
Copy link

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
Copy link

cen1 commented 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.

@julienrf
Copy link
Contributor Author

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
Copy link
Contributor

joschi commented Feb 28, 2021

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
Copy link
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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
5 participants