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

fix: handle deprecating relation for polyrelation #27

Merged

Conversation

notchairmk
Copy link
Member

@notchairmk notchairmk commented Dec 23, 2024

The purpose of this PR is to support deprecating struct fields marked with relation tags in favor of polyrelation. For example, this struct fails to unmarshal in it's current form:

type Apple struct {}
type Orange struct {}

type Fruit struct {
  Apple  *Apple
  Orange *Orange
}

type Meal struct {
  ID          string  `jsonapi:"primary,meals"`
  Fruit       *Apple  `jsonapi:"relation,fruit"`
  FruitChoice *Fruit  `jsonapi:"polyrelation,fruit"`
}

This will work if the fruit struct is only ever of the following form:

type Fruit struct {
  Apple  *Apple
}

This is what we have in go-tfe in a handful of places. For example https://github.com/hashicorp/go-tfe/blob/98975f447700482a79e4d7d177cd406ff8132f1d/run_trigger.go#L59-L61

Most of the usages in go-tfe support polyrelation with a deprecated relation tag, but only one type in the associated polyrelation struct. See https://github.com/hashicorp/go-tfe/blob/98975f447700482a79e4d7d177cd406ff8132f1d/run_trigger.go#L59-L61

The issue is that as soon as you try to add another type to the polyrelation/choice struct, we start to run into unmarshal failures. This can be seen in the error handling workaround for DataRetentionPolicy in go-tfe Organization and Workspace: https://github.com/hashicorp/go-tfe/blob/98975f447700482a79e4d7d177cd406ff8132f1d/organization.go#L500-L505
Essentially what's happening here is that we aren't actually allowing a deprecated field to be adopted over time, but forces the library user to immediately adopt the breaking change.

This is a prerequisite to support hashicorp/go-tfe#1016

brandonc
brandonc previously approved these changes Jan 7, 2025
Comment on lines +501 to +504
if pFieldType, ok := polyrelationFields[args[1]]; ok && fieldValue.Type() != pFieldType {
continue
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the solution makes sense but is not at all obvious to the casual reader. Would you mind adding a comment about what this achieves?

Suggested change
if pFieldType, ok := polyrelationFields[args[1]]; ok && fieldValue.Type() != pFieldType {
continue
}
// Skipping nonmatching types when a polymorphic relation of this name exists enables
// a migration path from relation to polyrelation.
if pFieldType, ok := polyrelationFields[args[1]]; ok && fieldValue.Type() != pFieldType {
continue
}

@brandonc brandonc merged commit 1dc4f04 into hashicorp:main Jan 7, 2025
5 checks passed
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