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

Wrapper/Model to handle anyOf over VirtualCircuit and VrfVirtualCircuit #58

Open
VamshikShetty opened this issue Jan 30, 2023 · 3 comments

Comments

@VamshikShetty
Copy link
Contributor

We have multiple models/schema representing anyOf structure between VirtualCircuit and VrfVirtualCircuit.

VirtualCircuitList in non-stitched oas 3.0 as following schema structure :

properties:
  virtual_circuits:
    items:
      anyOf:
      - $ref: './VirtualCircuit.yaml'
      - $ref: './VrfVirtualCircuit.yaml'
    type: array
type: object

ref : https://github.com/equinix-labs/metal-java/blob/main/spec/oas3.fetched/openapi/public/components/schemas/VirtualCircuitList.yaml

Whereas after stitching it with openapi generator we get following :

VirtualCircuitList:
      properties:
        virtual_circuits:
          items:
            $ref: '#/components/schemas/VirtualCircuitList_virtual_circuits_inner'
          type: array
      type: object

ref : https://github.com/equinix-labs/metal-java/blob/main/spec/oas3.stitched/oas3.stitched.metal.yaml#L13421

Same is visible in create request and response schema :

  requestBody:
    content:
      application/json:
        schema:
          oneOf:
          - $ref: '../../../../../components/schemas/VirtualCircuitCreateInput.yaml'
          - $ref: '../../../../../components/schemas/VrfVirtualCircuitCreateInput.yaml'
    description: Virtual Circuit details
    required: true
  responses:
    "201":
      content:
        application/json:
          schema:
            oneOf:
            - $ref: '../../../../../components/schemas/VirtualCircuit.yaml'
            - $ref: '../../../../../components/schemas/VrfVirtualCircuit.yaml'

ref: https://github.com/equinix-labs/metal-java/blob/main/spec/oas3.fetched/openapi/public/paths/connections/connection_id/ports/port_id/virtual-circuits.yaml#L62

Stitched flavour :

      requestBody:
        content:
          application/json:
            schema:
              $ref: '#/components/schemas/createInterconnectionPortVirtualCircuit_request'
        description: Virtual Circuit details
        required: true
      responses:
        "201":
          content:
            application/json:
              schema:
                $ref: '#/components/schemas/createInterconnectionPortVirtualCircuit_201_response'

ref : https://github.com/equinix-labs/metal-java/blob/main/spec/oas3.stitched/oas3.stitched.metal.yaml#L1088

createInterconnectionPortVirtualCircuit_201_response and VirtualCircuitList_virtual_circuits_inner in stitched spec represent the same schema modelling and is generated as a result of anyOf not being wrapped in a model before consumption. This creates inconsistent flavours when java bindings are generated.

Such example exists in multiple other places and should be rectified :

  1. IPReservationList_ip_addresses_inner : https://github.com/equinix-labs/metal-java/blob/main/spec/oas3.stitched/oas3.stitched.metal.yaml#L14129
  2. MetalGatewayList_metal_gateways_inner : https://github.com/equinix-labs/metal-java/blob/main/spec/oas3.stitched/oas3.stitched.metal.yaml#L14192
  3. findIPAddressById_200_response : https://github.com/equinix-labs/metal-java/blob/main/spec/oas3.stitched/oas3.stitched.metal.yaml#L13986
  4. InstancesBatchCreateInput_batches_inner_allOf : https://github.com/equinix-labs/metal-java/blob/main/spec/oas3.stitched/oas3.stitched.metal.yaml#L14133
@displague
Copy link
Member

@ctreatma Is this something that you noticed improvement with in the attempt to use redoc-cli for stitching?

@ctreatma
Copy link
Contributor

ctreatma commented Jan 30, 2023

No, switching to redoc for bundling wouldn't have an impact here. For this sort of thing, we need to extract the anyOf, oneOf, etc., to separate files so that we can replace these inline, complex models with appropriately-named external references to make the common functionality more explicit.

For example, we could create a file components/schemas/OneOfVirtualCircuit.yaml (name is an example, not necessarily committing to that) with the following contents:

oneOf:
- $ref: '../../../../../components/schemas/VirtualCircuitCreate.yaml'
- $ref: '../../../../../components/schemas/VrfVirtualCircuitCreate.yaml'

Given that file, we would update the VirtualCircuitList schema and the operation snippet in the issue description as follows:

properties:
  virtual_circuits:
    items:
      $ref: './OneOfVirtualCircuit.yaml
    type: array
type: object
requestBody:
  content:
    application/json:
      schema:
          oneOf:
          - $ref: '../../../../../components/schemas/VirtualCircuitCreateInput.yaml'
          - $ref: '../../../../../components/schemas/VrfVirtualCircuitCreateInput.yaml'
  description: Virtual Circuit details
  required: true
responses:
  "201":
    content:
      application/json:
        schema:
          $ref: '../../../../../components/schemas/OneOfVirtualCircuit.yaml'

That change would replace both the VirtualCircuitList_virtual_circuits_inner and createInterconnectionPortVirtualCircuit_201_response types with OneOfVirtualCircuit.

@ctreatma
Copy link
Contributor

More broadly, we need to establish a pattern/practice of avoiding nested anyOf, oneOf, allOf, etc.; anywhere we use those it's an indication that there's a model that needs its own name.

ctreatma added a commit that referenced this issue Mar 2, 2023
This updates the virtual circuit schemas to address #58.

The existing VirtualCircuit schema is renamed to VlanVirtualCircuit,
and a new VirtualCircuit schema is added that represents either a
VlanVirtualCircuit or a VrfVirtualCircuit.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants