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.

Sign up for GitHub

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

Jump to bottom

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 opened this issue Apr 3, 2024 · 4 comments · Fixed by #527
Closed

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

blva opened this issue Apr 3, 2024 · 4 comments · Fixed by #527
Assignees
@reuvenharrison
Labels
bug Something isn't working

Comments

@blva
Copy link
Contributor

blva commented Apr 3, 2024

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
  1. 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
  1. 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):

  • OS: MacOS

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.
@blva blva added the bug Something isn't working label Apr 3, 2024
@reuvenharrison
Copy link
Collaborator

Thanks @blva.
I replicated this behavior.

@reuvenharrison
Copy link
Collaborator

reuvenharrison commented Apr 9, 2024 •
edited
Loading

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

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

  • Base (response-enum-one-of-inline.yaml): there are 2 enums
  • Revision (response-enum-one-of-inline-2.yaml): there are 3 enums

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:

  • Schema1
  • Schema2

to this list of schemas:

  • Schema3
  • Schema4
  • Schema5

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:

  • “Billing Event Types” is unchanged
  • “Cps Backup Event Types” is modified
  • “New Events” is added

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?

@reuvenharrison reuvenharrison self-assigned this Apr 9, 2024
@reuvenharrison reuvenharrison linked a pull request Apr 15, 2024 that will close this issue
Compare inline schemas under AllOf/AnyOf/OneOf by title #527
Merged
@blva
Copy link
Contributor Author

blva commented Apr 26, 2024 •
edited
Loading

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

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

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

@reuvenharrison reuvenharrison

Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Compare inline schemas under AllOf/AnyOf/OneOf by title Tufin/oasdiff
2 participants
@reuvenharrison @blva