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

meta openapi:example work at APIExpr level too #3330

Merged
merged 1 commit into from
Aug 30, 2023

Conversation

antipopp
Copy link
Contributor

As mentioned in my issue #3316 this meta doesn't work as the documentation says.

// - "swagger:example" DEPRECATED, use "openapi:example" instead
//
// - "openapi:example" specifies whether to generate random example. Defaults to
// true. Applicable to API (applies to all attributes) or individual attributes.
//
//	var _ = API("MyAPI", func() {
//	    Meta("openapi:example", "false")
//	})

I've looked into the code for a while and really couldn't find a better way than this. The problem is that examples are generated on the Attribute expression, and the meta will work only there. My solution checks for the API meta during the generation of the openapi schema and sets the random generator seed to nil, and then adding the check to the Example method that created the example.

I found the codebase kinda confusing and I won't be surprised if there's a better, cleaner way, but since nobody was looking into it I tried myself.

Copy link
Member

@raphael raphael left a comment

Choose a reason for hiding this comment

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

Great first stab! A few things that would make this a bit better:

  1. It would suffice to change the generator just once in the New function of builder.go.
  2. The metadata check could simply call MustGenerate(api.Meta) instead of testing inline
  3. I would suggest setting the Randomizer of the ExampleGenerator to nil instead of the generator itself. The test to skip generation would be changed to if r.Randomizer == nil. We could document the behavior in expr/random.go (there are currently two possible randomizer types)

@antipopp
Copy link
Contributor Author

Let me know for other suggestions, somehow I totally missed the New function while I was traversing the whole flow when looking into it :)

About the inline check, there is already a MustGenerate function that checks if the whole schema needs to be generated. I made a new function that checks only for the example meta instead of mixing the two things. Hopefully you meant it in this way.

Copy link
Member

@raphael raphael left a comment

Choose a reason for hiding this comment

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

Looks good - left one question!

expr/random.go Outdated Show resolved Hide resolved
http/codegen/openapi/v3/builder.go Outdated Show resolved Hide resolved
dix

set Randomizer to nil in New function

check for nil Randomizer

document behaviour for Randomizer set to nil

updated comment for randomizer

fix

removed new lines

fix
@raphael raphael merged commit b11ee3c into goadesign:v3 Aug 30, 2023
5 checks passed
@raphael
Copy link
Member

raphael commented Aug 30, 2023

Thank you!

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