Fiserv / Support

To create support tickets for tenants
9 stars 4 forks source link

Support discriminator References as a Dropdown #684

Closed wwfiserv closed 8 hours ago

wwfiserv commented 5 months ago

Product

Commerce Hub

Page

https://github.com/Fiserv/Commerce-Hub/pull/952

Description

OpenAPI 3.0 has been the standard since 2017 and CH has been using it to write our YAML since Dev Studio launch. We are currently trying to improve our YAML to allow the discriminators (https://github.com/OAI/OpenAPI-Specification/blob/main/versions/3.0.0.md#discriminator-object) that have always been present in our API and allow it to display the dependent objects (source, network) as oneOf, so that the API Explorer will display the schemas and create a functional Postman collection.

  discriminator: {propertyName: sourceType}
  oneOf:
  - $ref: '#/components/schemas/PaymentCard'
  - $ref: '#/components/schemas/PaymentEMV'
  - $ref: '#/components/schemas/PaymentTrack'
  - $ref: '#/components/schemas/ApplePay'
  - $ref: '#/components/schemas/GooglePay'
  - $ref: '#/components/schemas/PaymentToken'
  - $ref: '#/components/schemas/PaymentSession'

  discriminator: {propertyName: network}
  oneOf:
  - $ref: '#/components/schemas/AmericanExpress'
  - $ref: '#/components/schemas/Discover'
  - $ref: '#/components/schemas/Mastercard'
  - $ref: '#/components/schemas/Visa'
  - $ref: '#/components/schemas/Debit'

The YAML validator throws and error, and based on @russnicoletti it's because Dev Studio doesn't support 3.0.

-------------------------YAML VALIDATOR FAILED -------------------------- 71 File :Commerce-Hub/reference/1.6.0/openapi.yaml with Converting circular structure to JSON 72 --> starting at object with constructor 'Object' 73 | property 'oneOf' -> object with constructor 'Array' 74 | index 0 -> object with constructor 'Object' 75 | property 'allOf' -> object with constructor 'Array' 76 --- index 0 closes the circle

We need to enhance the API Explorer to allows us to define the additional objects to be displayed without causing this problem. See Adyen as an example.

image

image

wwfiserv commented 3 months ago

Similar issue https://github.com/Fiserv/Support/issues/705

minh-pham1 commented 2 months ago

Hello William, going through some old Github Issues. The validator here isn't actually having issue with OpenAPI3.0, the issue may have been misdiagnosed. The circular dependency refers to the fact that schemas/PaymentCard schema requires allOf: {$ref: '#/components/schemas/Source'}. At the same time, schemas/SourceoneOf: $ref: '#/components/schemas/PaymentCard'` (plus some other schemas) which creates an infinite loop/circular dependency upon each other.

wwfiserv commented 2 months ago

@minh-pham1 but that is the correct usage of the allOf and discriminator if you look at the linked GitHub OpenAPI document I included. It also works fine in Swagger and shows the appropriate options.

components:
  schemas:
    Pet:
      type: object
      required:
      - pet_type
      properties:
        pet_type:
          type: string
      discriminator:
        propertyName: pet_type
        mapping:
          cachorro: Dog
    Cat:
      allOf:
      - $ref: '#/components/schemas/Pet'
      - type: object
        # all other properties specific to a `Cat`
        properties:
          name:
            type: string
    Dog:
      allOf:
      - $ref: '#/components/schemas/Pet'
      - type: object
        # all other properties specific to a `Dog`
        properties:
          bark:
            type: string
    Lizard:
      allOf:
      - $ref: '#/components/schemas/Pet'
      - type: object
        # all other properties specific to a `Lizard`
        properties:
          lovesRocks:
            type: boolean
minh-pham1 commented 2 months ago

Hi William, also reading through the OpenAPI documentation to refresh myself a bit. Of course, I could be very wrong as I don't work with OpenAPI too much so please feel free to correct me or point me to resources/other examples.

In this case, the parent class is Pet which doesn't require it to be oneOf the children classes (which makes sense because how can the parent be inheriting from its child).

In your openapi spec, from a developer standpoint I believe that the Source field (which is the parent) is defined to be oneOf PaymentCard, PaymentEMV, etc. The problem is, PaymentCard for example is defined as allOf: Source, which is indicating that its parent/super class is Source.

It's basically creating an object that looks like

PaymentCard: {
   x-model-version: 1.0
   ref -> Source: { 
      x-model-version: 1.0
      description: Payment
      ref -> PaymentCard: {
         x-model-version: 1.0
         ref -> Source: { .....

Just as an inside note, we utilize showdown.Converter and @apidevtools/swagger-parser for our API validator if that means anything. This was designed by a previous engineer so I'm not completely sure on why these package and its exact implementation but it's certainly something we can look into to refine and improve with more details.

Sorry about the inconvenience and I would like to say I am really appreciative of all these enhancements and discussions which will help improve the experience and quality of Developer Studio for everyone as a whole (along with my own personal learning).

wwfiserv commented 2 months ago

@minh-pham1 I see what you mean, just doublechecked in Swagger and it's getting that endless loop as well. I think when I was reading the GitHub page I was combining the first use case and the second. Now I have to see if there is another way for me to implement this, without breaking the CH API as we are using the same YAML.

minh-pham1 commented 2 months ago

Yeah I'm guessing you'll either need a third type like Source -> oneOf: PaymentCard, PaymentEMV, etc. and PaymentCard -> allOf: PaymentSource (instead of Source) which just has the description or specific subsets of other schemas which don't loop back.

wwfiserv commented 2 months ago

@minh-pham1 I have updated the requirements, this is also being tracked in the following Jira ticket COM-4066

stale[bot] commented 2 weeks ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.