equinix / equinix-sdk-go

Golang client for Equinix APIs
https://deploy.equinix.com/labs/equinix-sdk-go
MIT License
2 stars 4 forks source link

feat: update virtual networks to reference resource schemas instead of href #28

Closed ctreatma closed 4 months ago

ctreatma commented 5 months ago

While converting the metal_vlan Terraform resource and datasource to this SDK, a number of issues were uncovered in the VirtualNetwork schema. A number of properties relevant to virtual networks were declared as $refs to the Href component, which only has one named property, href. References to the Href component make it difficult to access attributes of nested resources; this updates those references to point to more specific components instead.

displague commented 5 months ago

Should this be documented somewhere in this project, that the preference is for Hrefs to refer to concrete types? I imagine we have other upstream references to Href rather than types.

Are these concrete types defined such that Href may be the only field present? If not, we could see exceptions thrown in downstream tools during deserialization. Something like error while deserializing, id is required but missing

Could we end up needing a Device selector type, OneOf [Device, DeviceHref]

ctreatma commented 5 months ago

Should this be documented somewhere in this project, that the preference is for Hrefs to refer to concrete types? I imagine we have other upstream references to Href rather than types

I'm not sure about documenting it in this project; the whole Href approach is specific to the Metal API and we tend to see better detail in other specs. There are cases where Href might be useful but the Href schema itself would need to change to empower those, and once it does it would naturally push the spec in a better direction.

Are these concrete types defined such that Href may be the only field present? If not, we could see exceptions thrown in downstream tools during deserialization. Something like error while deserializing, id is required but missing

Could we end up needing a Device selector type, OneOf [Device, DeviceHref]

In general, yes, our response schemas avoid required fields so that clients have better flexibility in deserializing responses. There are cases where we already have OneOf schemas in which an Href could be a useful fallback, but in its current state the Href schema can't be used safely in those situations because it allows arbitrary additional properties.

For example, we currently have Metal Gateway that is OneOf [VlanMetalGateway, VrfMetalGateway], and we have to use include parameters to ensure that any response that has gateways in it has enough detail to match one of those schemas.

If we tried changing Metal Gateway to OneOf [VlanMetalGateway, VrfMetalGateway, Href] right now, we would introduce deserialization errors because anything that matches VlanMetalGateway or VrfMetalGateway will also match Href. [The Href schema had to be patched in order to adopt a ThingOrHref pattern in metal-java](https://github.com/equinix-labs/metal-java/blob/main/patches/spec.fetched.json/09-set-additional-prop-to-false-in-href.patch, but since it's used in many places that change is higher risk. Once we have a clearer path for Metal API schema changes we can aim to replace usage of Href with appropriate schemas and then update Href to disallow additional properties so that it can be used as a fallback

Even in this case, though, it would be possible to get deserialization errors if a response comes back with properties other than "href" but doesn't have enough fields needed to match either VlanMetalGateway or VrfMetalGateway. If the goal is to avoid deserialization errors, then the best approach is to not have any required fields in response schemas (which probably implies avoiding OneOf in responses as well); the tradeoff there is that we lose some type information because there would be just a MetalGateway schema that supports all properties of VlanMetalGateway and VrfMetalGateway.

ctreatma commented 4 months ago

Closing this because I think it would be better to fix this at the source and reduce our reliance on SDK-local spec patches