Tufin / oasdiff

OpenAPI Diff and Breaking Changes
https://www.oasdiff.com
Apache License 2.0
763 stars 63 forks source link

Adding deprecated: true to an operation yields a parse error #552

Closed tibbe closed 5 months ago

tibbe commented 6 months ago

Describe the bug Adding deprecated: true to an operation yields a parse error. As per OAS 3 (see "Deprecated Operations" in https://swagger.io/docs/specification/paths-and-operations/) it should be possible to add deprecated: true to an operation. This annotation gets added automatically to any FastAPI-generated OpenAPI specs when setting deprecated=True on a route:

@router.get("/authorized/organizations/{organization_id}/link_invitations", deprecated=True):
def get_link_invitations(...):
    ...

To Reproduce Steps to reproduce the behavior:

Run

oasdiff breaking openapi-old.json openapi-new.json

Using these two files:

openapi-old.json:

{
  "components": {
    "schemas": {}
  },
  "info": {
    "title": "FastAPI",
    "version": "0.1.0"
  },
  "openapi": "3.1.0",
  "paths": {
    "/authorized/organizations/{organization_id}/link_invitations": {
      "delete": {
        "deprecated": true,
        "operationId": "mobile-api.deleteOrganizationLinkInvitation",
        "parameters": [
          {
            "in": "path",
            "name": "organization_id",
            "required": true,
            "schema": {
              "title": "Organization Id",
              "type": "string"
            }
          },
          {
            "in": "query",
            "name": "invitation_id",
            "required": true,
            "schema": {
              "title": "Invitation Id",
              "type": "string"
            }
          }
        ],
        "responses": {
          "204": {
            "description": "Successful Response"
          }
        },
        "summary": "Delete Organization Link Invitation",
        "tags": [
          "mobile-api",
          "organizations"
        ]
      }
    }
  }
}

openapi-new.json:

{
  "components": {
    "schemas": {}
  },
  "info": {
    "title": "FastAPI",
    "version": "0.1.0"
  },
  "openapi": "3.1.0",
  "paths": {}
}

Output:

1 breaking changes: 1 error, 0 warning
error   [api-path-sunset-parse] at openapi-old.json
        in API DELETE /authorized/organizations/{organization_id}/link_invitations
                api-path-sunset-parse

Expected behavior

No parse error and no output of type "error" (i.e. there are no breaking changes) but I would settle for just not getting an "api-path-sunset-parse" error, as it should be OK to set deprecated: true without setting anything else (e.g. some "sunset" flag) as per OAS 3.

Desktop (please complete the following information):

Additional context oasdiff version 1.10.14

reuvenharrison commented 5 months ago

Hi @tibbe, When removing an endpoint and marking it as deprecated, oasdiff requires an additional field x-sunset, like this:

    "/authorized/organizations/{organization_id}/link_invitations": {
      "delete": {
        "deprecated": true,
        "x-sunset": "2024-12-31",

Adding this field will prevent the api-path-sunset-parse error that you are receiving. This behavior is documented here: https://github.com/Tufin/oasdiff/blob/main/docs/API-DEPRECATION.md

Please share your feedback.

tibbe commented 5 months ago

Hi @tibbe, When removing an endpoint and marking it as deprecated, oasdiff requires an additional field x-sunset, like this:

    "/authorized/organizations/{organization_id}/link_invitations": {
      "delete": {
        "deprecated": true,
        "x-sunset": "2024-12-31",

Adding this field will prevent the api-path-sunset-parse error that you are receiving. This behavior is documented here: https://github.com/Tufin/oasdiff/blob/main/docs/API-DEPRECATION.md

Please share your feedback.

That's not really feasible in all the cases where the OpenAPI spec is programatically generated, like in the case of FastAPI, as the user likely has no control over what gets put in the generated spec, especially so for non-standard fields like x-sunset (hence the "x-" part).

I don't think oasdiff should require the input spec to contain non-standard fields. It's fine if it also accepts them but shouldn't fail if they're not present.

reuvenharrison commented 5 months ago

I agree. That's a good point.

reuvenharrison commented 5 months ago

Thanks for reporting this. Please let me know if it is working as expected now. Details: https://github.com/Tufin/oasdiff/blob/main/docs/DEPRECATION.md

tibbe commented 5 months ago

Thanks!