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

add patch 09 & 10 #63

Merged
merged 2 commits into from
Feb 14, 2023
Merged

add patch 09 & 10 #63

merged 2 commits into from
Feb 14, 2023

Conversation

VamshikShetty
Copy link
Contributor

@VamshikShetty VamshikShetty commented Feb 14, 2023

This PR contains following changes :

  1. Introduction of patch 09 : Href schema model poses an issue where it is valid for most other models in anyOf scenario, hence by disabling allow additional properties we can fix this. This topic was discussed in a previous thread : Fix patches #61 (comment)

  2. Introduction of patch 10, issue in InterconnectionPort schema :
    InterconnectionPort schema contains virtual_circuits attribute as a property:

properties:
  .
  .
  virtual_circuits:
    $ref: './VirtualCircuitList.yaml'
  .
  .
type: object

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

Whereas VirtualCircuitList represents a complex anyOf array for attribute named virtual_circuits :

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

This creates a incorrect response expectation which looks like this :

{
  "interconnectionport": {
    "virtual_circuits": {
      "virtual_circuits": [...]
    }
  }
}

Patch 10 create a file called InterconnectionPortCore.yaml file without virtual_circuits and uses allOf to merge this new file and VirtualCircuitList.yaml to create InterconnectionPort.yaml which gives the correct response flavour.

switch_id:
description: A switch 'short ID'
type: string
virtual_circuits:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be more correct here to patch the VirtualCircuitList.yaml file to specify a list schema:

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking through the schema more, I see there's a lot of precedent for the current structure of VirtualCircuitList, and changing that would require a bigger patch, so I'm on board with what you have here.

One thing to note is that we're working on a patch to address #58, and that will likely include a name change for the VirtualCircuit model.

required:
- href
type: object
+additionalProperties: false
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the right change IMO, but we should bear in mind that fixing it will likely uncover additional schema issues.

@ctreatma ctreatma merged commit 2294ee2 into main Feb 14, 2023
@cprivitere cprivitere deleted the add-patch-09-10 branch August 16, 2023 20:43
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

Successfully merging this pull request may close these issues.

3 participants