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

Upgrading oapi library #107

Merged
merged 10 commits into from
Feb 6, 2023
Merged

Upgrading oapi library #107

merged 10 commits into from
Feb 6, 2023

Conversation

spinillos
Copy link
Member

@spinillos spinillos commented Feb 6, 2023

Important: The library introduced a bug checking pointers equality and some cue files fail. I created a PR in their repo to fix that. If they don't respond in time, we can use my fork to make it work.

It upgrades oapi library to v1.12.4. I'm going to try to clarify the changes...

The oapi library introduce these changes:

  • Adds GoDecl in some comments: It means that it set the name of each field/struct at the beginning of each comment. That's "correct" because it makes GoLint happy but it breaks most of our comments. It works different when is group or not 🤷🏻‍♀️.
  • Generic fields are now structs with functions to map them: It adds a lot of boilerplate to map the struct into their different types and it adds new imports. Previously we have *interface{} that it did the work.
  • Fixes map structs: We don't need to iterate the Go file to detect the maps with "AdditionalProperties" to fix them.
  • "Generic" functions and autogenerated structs use underscores: We started to have structs names with underscores 🙃. Some of our *interface{} became structs that its ok.

Changes that I added:

  • decoderCompactor() isn't necessary anymore.
  • It adds a UseGoDeclInComments to allow default configuration in comments. By default it leaves the comments "as" we put in the cue file. Its not perfect but its accurate 😞.
  • It iterates the Go file to detect the structs that previously were *interface{} and leave them as this. Also deletes the related functions and models affected.

Pending stuff:

  • Autogenerated structs that aren't generic uses underscores and we should fix them. If this PR its ok to merge, I'll create an issue to fix this case.

"Generic" structs

When I was talking about generic structs I mean the ones that accept multiple values like:

// TODO docs
refresh?: string | false @grafanamaturity(NeedsExpertReview)

It was generating the following:

// DashboardRefresh0 defines model for .
type DashboardRefresh0 = interface{}

// DashboardRefresh1 defines model for .
type DashboardRefresh1 = string

// Dashboard_Refresh TODO docs
type Dashboard_Refresh struct {
	union json.RawMessage
}

// AsDashboardRefresh0 returns the union data inside the Dashboard_Refresh as a DashboardRefresh0
func (t Dashboard_Refresh) AsDashboardRefresh0() (DashboardRefresh0, error) {
	var body DashboardRefresh0
	err := json.Unmarshal(t.union, &body)
	return body, err
}

// FromDashboardRefresh0 overwrites any union data inside the Dashboard_Refresh as the provided DashboardRefresh0
func (t *Dashboard_Refresh) FromDashboardRefresh0(v DashboardRefresh0) error {
	b, err := json.Marshal(v)
	t.union = b
	return err
}

// MergeDashboardRefresh0 performs a merge with any union data inside the Dashboard_Refresh, using the provided DashboardRefresh0
func (t *Dashboard_Refresh) MergeDashboardRefresh0(v DashboardRefresh0) error {
	b, err := json.Marshal(v)
	if err != nil {
		return err
	}

	merged, err := runtime.JsonMerge(b, t.union)
	t.union = merged
	return err
}

// AsDashboardRefresh1 returns the union data inside the Dashboard_Refresh as a DashboardRefresh1
func (t Dashboard_Refresh) AsDashboardRefresh1() (DashboardRefresh1, error) {
	var body DashboardRefresh1
	err := json.Unmarshal(t.union, &body)
	return body, err
}

// FromDashboardRefresh1 overwrites any union data inside the Dashboard_Refresh as the provided DashboardRefresh1
func (t *Dashboard_Refresh) FromDashboardRefresh1(v DashboardRefresh1) error {
	b, err := json.Marshal(v)
	t.union = b
	return err
}

// MergeDashboardRefresh1 performs a merge with any union data inside the Dashboard_Refresh, using the provided DashboardRefresh1
func (t *Dashboard_Refresh) MergeDashboardRefresh1(v DashboardRefresh1) error {
	b, err := json.Marshal(v)
	if err != nil {
		return err
	}

	merged, err := runtime.JsonMerge(b, t.union)
	t.union = merged
	return err
}

func (t Dashboard_Refresh) MarshalJSON() ([]byte, error) {
	b, err := t.union.MarshalJSON()
	return b, err
}

func (t *Dashboard_Refresh) UnmarshalJSON(b []byte) error {
	err := t.union.UnmarshalJSON(b)
	return err
}

and used like:

// TODO docs
Refresh *Dashboard_Refresh `json:"refresh,omitempty"`

And after all changes, we are going to have the previous behaviour:

// TODO docs
Refresh *interface{} `json:"refresh,omitempty"`

@sdboyer
Copy link
Contributor

sdboyer commented Feb 6, 2023

cc @IfSentient

Copy link
Contributor

@sdboyer sdboyer left a comment

Choose a reason for hiding this comment

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

This looks good! Thanks for all your work on this, i know it's been quite a journey 😅

@@ -4,7 +4,7 @@ package generate
import "github.com/grafana/thema"

thema.#Lineage
name: "pointers"
Copy link
Contributor

Choose a reason for hiding this comment

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

uuuuggghhh there's still a bug in the txtar framework i wrote - something is leaking across goroutines and causing flipping in the expected name used for lineages.

return is
}

// It fixes the "generic" fields. It happens when a value in cue could be different structs.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// It fixes the "generic" fields. It happens when a value in cue could be different structs.
// fixRawData fixes "generic" fields. It happens when a value in cue could be different structs.

@sdboyer sdboyer merged commit 8cc8cfa into main Feb 6, 2023
mildwonkey pushed a commit that referenced this pull request May 1, 2023
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.

None yet

2 participants