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

Openapi parser #22

Merged
merged 42 commits into from
Oct 5, 2023
Merged

Openapi parser #22

merged 42 commits into from
Oct 5, 2023

Conversation

spinillos
Copy link
Member

@spinillos spinillos commented Sep 20, 2023

Contributes to: #4

First approach for openapi parser

@spinillos spinillos requested a review from a team as a code owner September 20, 2023 10:28
@spinillos spinillos marked this pull request as draft September 20, 2023 10:29
@spinillos spinillos changed the title Openapi gen Openapi parser Sep 20, 2023
@spinillos spinillos mentioned this pull request Sep 21, 2023
@spinillos spinillos marked this pull request as ready for review September 27, 2023 12:33
@@ -43,6 +43,7 @@ type Op string
const (
MinLengthOp Op = "minLength"
MaxLengthOp Op = "maxLength"
MultipleOfOp Op = "multipleOf"
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to support this operator? 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried to support the things that exist in open-api. I think that we aren't using that in the cue schemas.

internal/ast/types.go Outdated Show resolved Hide resolved
@@ -97,7 +97,8 @@ func (jenny RawTypes) formatEnumDef(def ast.Object) string {

buffer.WriteString("const (\n")
for _, val := range enumType.Values {
buffer.WriteString(fmt.Sprintf("\t%s %s = %#v\n", tools.UpperCamelCase(val.Name), enumName, val.Value))
name := tools.CleanupNames(tools.UpperCamelCase(val.Name))
Copy link
Member

Choose a reason for hiding this comment

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

I'm surprised to see "enum value name" cleaning/formatting here.
It feels like this should be done somewhere else since we're likely to do it for any target language: in a compiler pass such as this one maybe? (or another one)

Copy link
Member Author

Choose a reason for hiding this comment

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

I found this issue when we have integers. It was setting things like:

0 Cursor = 0
1 Cursor = 1

I set the enum name as prefix to avoid invalid values but fon any reason, after do some sync it wasn't necessary anymore 😅.

This cleanup is because I was setting the values of the enum as names and there were some with punctuation marks generating invalid names.

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense!

If we want to keep this kind of cleanup, I still think it should be done as a compiler pass though, instead of directly within the jennies :)

internal/openapi/generator.go Show resolved Hide resolved
internal/openapi/generator.go Outdated Show resolved Hide resolved
internal/openapi/generator.go Outdated Show resolved Hide resolved
internal/openapi/generator.go Outdated Show resolved Hide resolved
internal/openapi/utils.go Outdated Show resolved Hide resolved

func getArgs(v *float64, t string) []any {
args := []any{v}
if t == openapi3.TypeInteger {
Copy link
Member

Choose a reason for hiding this comment

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

Does that mean that if t != openapi3.TypeInterger, then args will contain a pointer to a float?

If so, I think we should dereference the pointer first: args := []any{*v}

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, if it isn't an integer, we want to retrieve the float value. These values are *float4 by default and if we don't map them to integers when the type is integer, the builder will compare integers with floats.


func (g *generator) walkAnyOf(schema *openapi3.Schema) (ast.Type, error) {
return g.walkDisjunctions(schema.AnyOf)
}
Copy link
Member

Choose a reason for hiding this comment

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

allOf, oneOf and anyOf have different semantics, we can't treat them as just a disjunction. See: https://swagger.io/docs/specification/data-models/oneof-anyof-allof-not/

I think that for the time being, it's okay if we only support oneOf and explicitly return an error if a anyOf or allOf is found.

But even for oneOf, we still should read and extract the discriminator data that might be there: https://github.com/OAI/OpenAPI-Specification/blob/main/versions/3.0.3.md#discriminator-object

Copy link
Member Author

Choose a reason for hiding this comment

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

I put them as the same way from the beginning but its something that I wanted to do it later 😅. But yep, its wrong

@K-Phoen
Copy link
Member

K-Phoen commented Sep 28, 2023

Tests? 😇

@K-Phoen
Copy link
Member

K-Phoen commented Oct 5, 2023

Let's merge it!

@spinillos spinillos merged commit 2889cc6 into main Oct 5, 2023
2 checks passed
@spinillos spinillos deleted the openapi-gen branch October 5, 2023 15:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants