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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Recursive Child elements do not generate correctly for OpenAPI #3049

Closed
hkf57 opened this issue Dec 7, 2022 · 5 comments 路 Fixed by #3090
Closed

Recursive Child elements do not generate correctly for OpenAPI #3049

hkf57 opened this issue Dec 7, 2022 · 5 comments 路 Fixed by #3090

Comments

@hkf57
Copy link

hkf57 commented Dec 7, 2022

馃悰 Bug Report

Recursive child members do not render properly in OpenAPI

To Reproduce

For a given proto, generate the openapiv2 output (I used buf):

message Foo{
  repeated Foo children = 1;
}

Expected behavior

"Foo": {
"type": "object",
"properties": {
  "children": {
    "type": "array"
    "items": {
      "type": "object",
      "$ref": "#/Foo"
     }
    }
}

Actual Behavior

"Foo": {
"type": "object",
"properties": {
  "children": {
    "type": "array"
    "items": {
      "$ref": "#/Foo"
     }
    }
}

However, on the other hand, if your repeated section is NOT a recursive one, e.g. for the below proto:

message Bar {
  repeated Foo children = 1;
}

the generated openAPI spec looks like:

"Bar": {
"type": "object",
"properties": {
  "children": {
    "type": "array"
    "items": {
      "$ref": "#/Foo"
     }
    }
}

Which is totally valid.

I've tried both cases and it seems like even adding a type to the second example will work, so it looks like it can be added across the board for children.

Your Environment

Alpine 3.15, but reproducible everywhere

@johanbrandhorst
Copy link
Collaborator

Hi, thanks for your issue. I'm confused, aren't the two examples you show here identical? Why do you expect there to be a type field in the items object? There are plenty of examples in https://swagger.io/specification/v2 of items with just a ref field. Could you please clarify why this is wrong for the recursive type?

@hkf57
Copy link
Author

hkf57 commented Dec 8, 2022

Hi @johanbrandhorst, I think you're talking about the Actual Behaviour section, and the difference is only the top line.

Please see here for the requirement inside an array: https://swagger.io/specification/v2/#itemsObject

maybe, it is required for non-recursive references too. But the validator I'm using (the one inside https://github.com/opticdev/optic) seems to think it's fine.

@johanbrandhorst
Copy link
Collaborator

I did look at that definition, and it does state that type is required in the items object, but as you can see in multiple examples and evidently your validator too, it's not de facto required when using references. Are you experiencing an error associated with this behavior? Why should the non-recursive reference be OK and the recursive reference not be? I don't get it.

@hkf57
Copy link
Author

hkf57 commented Dec 14, 2022

@johanbrandhorst then it would be an inconsistency in the validator

Nonetheless it is still valid that the type is required if the items object has the type of array.

@johanbrandhorst
Copy link
Collaborator

I agree with that interpretation of the spec, I'm just surprised that their examples are... invalid code? In any case, it probably shouldn't hurt for us to add this. Would you be interested in contributing a fix?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants