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

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

Closed
wants to merge 4 commits into from

Conversation

ctreatma
Copy link
Contributor

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.

@ctreatma ctreatma changed the title fix: update virtual networks to reference resource schemas instead of href feat: update virtual networks to reference resource schemas instead of href Jan 29, 2024
@displague
Copy link
Member

displague commented Feb 5, 2024

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
Copy link
Contributor Author

ctreatma commented Feb 5, 2024

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.

Copy link
Member

@displague displague left a comment

Choose a reason for hiding this comment

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

lgtm

@ctreatma
Copy link
Contributor Author

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

@ctreatma ctreatma closed this Feb 16, 2024
@ctreatma ctreatma deleted the moar-spec-fixes branch October 30, 2024 14:37
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.

2 participants