search

Tufin / oasdiff

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

Diff tool gets confused when a new inline enum value is added and a new enum type is added at the sime time #514

Closed blva closed 4 months ago

blva commented 5 months ago

Describe the bug Diff tool gets confused when a new inline enum value is added and a new enum type is added at the sime time To Reproduce Steps to reproduce the behavior:

  1. oasdiff breaking data/enums/response-enum-one-of-inline.yaml data/enums/response-enum-one-of-inline-2.yaml
  2. Spec 1: 
    openapi: 3.0.1
info:
title: Test API
version: "2.0"
tags:
- name: Tests
description: Test tag.
paths:
/api/v2/changeOfResponseFieldValueTiedToEnumTest:
get:
  tags:
  - Tests
  summary: This is a test
  description: Test description.
  operationId: getTest
  requestBody:
    description: Test.
    content:
      application/json:
        schema:
          $ref: '#/components/schemas/GroupOfRequestObjects'
    required: true
  responses:
    "200":
      description: OK
  security:
  - DigestAuth: []
components:
schemas:
GroupOfRequestObjects:
  type: object
  description: Enum values
  oneOf:
  - $ref: "#/components/schemas/ResponseEnumInline"
  - $ref: "#/components/schemas/ResponseEnumInline2"
ResponseEnumInline:
  type: object
  description: Enum values
  properties:
    eventTypeName:
      description: Incident that triggered this alert.
      type: object
      oneOf:
      - enum:
        - CREDIT_CARD_ABOUT_TO_EXPIRE
        title: Billing Event Types
        type: string
      - enum:
        - CPS_SNAPSHOT_STARTED
        - CPS_SNAPSHOT_SUCCESSFUL
        - CPS_SNAPSHOT_FAILED
        - CPS_SNAPSHOT_FALLBACK_SUCCESSFUL
        - CPS_SNAPSHOT_FALLBACK_FAILED
        - CPS_RESTORE_SUCCESSFUL
        - CPS_EXPORT_SUCCESSFUL
        - CPS_RESTORE_FAILED
        - CPS_EXPORT_FAILED
        - CPS_SNAPSHOT_DOWNLOAD_REQUEST_FAILED
        - CPS_OPLOG_CAUGHT_UP
        title: Cps Backup Event Types
        type: string
ResponseEnumInline2:
  type: object
  description: Enum values
  properties:
    eventTypeName2:
      description: Incident that triggered this alert.
      type: object
      oneOf:
      - enum:
        - CREDIT_CARD_ABOUT_TO_EXPIRE
        title: Billing Event Types
        type: string
DigestAuth:
  type: http
  scheme: digest
  3. Spec 2: 
    openapi: 3.0.1
info:
title: Test API
version: "2.0"
tags:
- name: Tests
description: Test tag.
paths:
/api/v2/changeOfResponseFieldValueTiedToEnumTest:
get:
  tags:
  - Tests
  summary: This is a test
  description: Test description.
  operationId: getTest
  requestBody:
    description: Test.
    content:
      application/json:
        schema:
          $ref: '#/components/schemas/GroupOfRequestObjects'
    required: true
  responses:
    "200":
      description: OK
  security:
  - DigestAuth: []
components:
schemas:
GroupOfRequestObjects:
  type: object
  description: Enum values
  oneOf:
  - $ref: "#/components/schemas/ResponseEnumInline"
  - $ref: "#/components/schemas/ResponseEnumInline2"
ResponseEnumInline:
  type: object
  description: Enum values
  properties:
    eventTypeName:
      description: Incident that triggered this alert.
      type: object
      oneOf:
      - title: Billing Event Types
        type: string
        enum:
        - CREDIT_CARD_ABOUT_TO_EXPIRE
      - title: Cps Backup Event Types
        type: string
        enum:
        - CPS_SNAPSHOT_STARTED
        - CPS_SNAPSHOT_SUCCESSFUL
        - CPS_SNAPSHOT_FAILED
        - CPS_SNAPSHOT_FALLBACK_SUCCESSFUL
        - CPS_SNAPSHOT_FALLBACK_FAILED
        - CPS_RESTORE_SUCCESSFUL
        - CPS_NEW_EVENT_1
        - CPS_NEW_EVENT_2
        - CPS_EXPORT_SUCCESSFUL
        - CPS_RESTORE_FAILED
        - CPS_EXPORT_FAILED
        - CPS_SNAPSHOT_DOWNLOAD_REQUEST_FAILED
        - CPS_OPLOG_CAUGHT_UP
      - title: New Events 
        type: string
        enum:
        - NEW_EVENT_1
        - NEW_EVENT_2 
ResponseEnumInline2:
  type: object
  description: Enum values
  properties:
    eventTypeName2:
      description: Incident that triggered this alert.
      type: object
      oneOf:
      - enum:
        - CREDIT_CARD_ABOUT_TO_EXPIRE
        title: Billing Event Types
        type: string
DigestAuth:
  type: http
  scheme: digest
  4. Output
❯ oasdiff changelog data/enums/response-enum-one-of-inline.yaml data/enums/response-enum-one-of-inline-2.yaml
2 changes: 1 error, 0 warning, 1 info
error   [request-property-one-of-removed] at data/enums/response-enum-one-of-inline-2.yaml
        in API GET /api/v2/changeOfResponseFieldValueTiedToEnumTest
                removed 'BaseSchema[1]:Cps Backup Event Types' from the '/oneOf[#/components/schemas/ResponseEnumInline]/eventTypeName' request property 'oneOf' list

info    [request-property-one-of-added] at data/enums/response-enum-one-of-inline-2.yaml
        in API GET /api/v2/changeOfResponseFieldValueTiedToEnumTest
                added 'RevisionSchema[1]:Cps Backup Event Types, RevisionSchema[2]:New Events' to the '/oneOf[#/components/schemas/ResponseEnumInline]/eventTypeName' request property 'oneOf' list

Expected behavior

info    [request-property-one-of-added] at data/enums/response-enum-one-of-inline-2.yaml
        in API GET /api/v2/changeOfResponseFieldValueTiedToEnumTest
                added 'RevisionSchema[2]:New Events' to the '/oneOf[#/components/schemas/ResponseEnumInline]/eventTypeName' request property 'oneOf' list
info    [request-property-enum-value-added] at data/enums/response-enum-one-of-inline-2.yaml
        in API GET /api/v2/changeOfResponseFieldValueTiedToEnumTest
                added the new 'CPS_NEW_EVENT_1' enum value to the request property '/oneOf[#/components/schemas/ResponseEnumInline]/eventTypeName/oneOf[#2]/'

info    [request-property-enum-value-added] at data/enums/response-enum-one-of-inline-2.yaml
        in API GET /api/v2/changeOfResponseFieldValueTiedToEnumTest
                added the new 'CPS_NEW_EVENT_2' enum value to the request property '/oneOf[#/components/schemas/ResponseEnumInline]/eventTypeName/oneOf[#2]/'

Desktop (please complete the following information):

Additional context

When I isolate both the changes, then the output is correct, if I do

            # - CPS_NEW_EVENT_1
            # - CPS_NEW_EVENT_2

I get the new event changelog correctly

info    [request-property-one-of-added] at data/enums/response-enum-one-of-inline-2.yaml
        in API GET /api/v2/changeOfResponseFieldValueTiedToEnumTest
                added 'RevisionSchema[2]:New Events' to the '/oneOf[#/components/schemas/ResponseEnumInline]/eventTypeName' request property 'oneOf' list

If I do

         # - title: New Events 
          #   type: string
          #   enum:
          #   - NEW_EVENT_1
          #   - NEW_EVENT_2

I get the CPS new event log correctly

❯ oasdiff changelog data/enums/response-enum-one-of-inline.yaml data/enums/response-enum-one-of-inline-2.yaml
2 changes: 0 error, 0 warning, 2 info
info    [request-property-enum-value-added] at data/enums/response-enum-one-of-inline-2.yaml
        in API GET /api/v2/changeOfResponseFieldValueTiedToEnumTest
                added the new 'CPS_NEW_EVENT_1' enum value to the request property '/oneOf[#/components/schemas/ResponseEnumInline]/eventTypeName/oneOf[#2]/'

info    [request-property-enum-value-added] at data/enums/response-enum-one-of-inline-2.yaml
        in API GET /api/v2/changeOfResponseFieldValueTiedToEnumTest
                added the new 'CPS_NEW_EVENT_2' enum value to the request property '/oneOf[#/components/schemas/ResponseEnumInline]/eventTypeName/oneOf[#2]/'

This is something in the diff core of the tool, where we start checking RevisionSchemas without considering the content for some reason in this corner case.

reuvenharrison commented 5 months ago

Thanks @blva. I replicated this behavior.

reuvenharrison commented 5 months ago

Apparently, this is the expected behavior based on the current diff algorithm.

Explanation: The sub-schemas under eventTypeName/oneOf were changed:

Note that these sub-schemas are defined “inline”, meaning that they are not references to schemas under “Components”. When oasdiff compares “inline” schemas it cannot determine which pairs of schemas correspond to each-other between base and revision.

For example, when comparing this list of schemas:

to this list of schemas:

oasdiff doesn't know if Schema3 corresponds to Schema1 or to Schema2, or perhaps it is a new schema that corresponds to neither of the old ones.

oasdiff actually tries to minimize the error by matching identical schemas, so in your case:

So oasdiff knows that “Billing Event Types” is unchanged, but it can’t determine whether the new “Cps Backup Event Types” is a modified version of the old one or whether it’s a new schema. The same logic applies to “New Events”.

In your case, there is a hint, a title, which we could use to enhance the matching but we can’t rely on it because it is optional and, moreover, it could also be modified.

So here’s a question to you, and the community: Do you think that we should enhance the matching algorithm to compare schemas with the same title to each-other?

blva commented 4 months ago

Hey @reuvenharrison, sorry to get back at this in delay, but I do believe we should enhance and match the same titles. But I understand that this would be then a feature request based on your explanation of the current algorithm limitation. Thanks for doing this fix I'll test this out! Also the core issue is that the actual behaviour detects a breaking change false positive when it should be just info level messages.

reuvenharrison commented 4 months ago

The fix is already available in the latest release. Please give it a try and let me know how it works.