-
Notifications
You must be signed in to change notification settings - Fork 807
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: Ensure external refs are propagated to generated code #1389
Conversation
71813ff
to
520ad55
Compare
pkg/codegen/operations.go
Outdated
@@ -817,6 +821,19 @@ func GenerateResponseDefinitions(operationID string, responses map[string]*opena | |||
NameTag: tag, | |||
Schema: contentSchema, | |||
} | |||
|
|||
// TODO finish comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO
pkg/codegen/operations.go
Outdated
// TODO finish comment | ||
parts := strings.SplitN(parentRef, "#", 2) | ||
if parentRef != "" && len(parts) == 2 { | ||
// TODO finish comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO
pkg/codegen/operations.go
Outdated
rcd.Schema = Schema{ | ||
RefType: fmt.Sprintf("%s.%s", pack.Name, contentSchema.GoType), | ||
} | ||
} // TODO log when not found |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not as part of this one
e7cef18
to
f4d7d4c
Compare
I'm getting the following error when trying this out:
|
14ef705
to
3e25b53
Compare
520ad55
to
1ef78fc
Compare
pkg/codegen/operations.go
Outdated
parts := strings.SplitN(parentRef, "#", 2) | ||
if parentRef != "" && len(parts) == 2 { | ||
// TODO finish comment | ||
// if a locally-defined, but `$ref`'d parent, then we need to make sure that we make this an external ref |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rephrase
f8b232b
to
55e3ce2
Compare
As noted in #1378, there are cases where a complex set of `$ref`s between multiple files can lead to broken generated code, which does not correctly import the package that has been prepared for the external reference. We can handle this by looking up any references, where there is a `.Ref` passed into the type, and then iterate through relevant children. This requires we handle the updating in-place for these by using a bit of pointer + indexing fun. This also adds a relevant test case to validate the fix. Closes #1378.
55e3ce2
to
0692acc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my limited understanding of the internals of this project, LGTM. :D
// ensureExternalRefsInRequestBodyDefinitions ensures that when an externalRef (`$ref` that points to a file that isn't the current spec) is encountered, we make sure we update our underlying `RefType` to make sure that we point to that type. | ||
// This only happens if we have a non-empty `ref` passed in, and that `ref` isn't pointing to something in our file | ||
// NOTE that the pointer here allows us to pass in a reference and edit in-place | ||
func ensureExternalRefsInRequestBodyDefinitions(defs *[]RequestBodyDefinition, ref string) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be easier to understand if the param was defs []*RequestBodyDefinition
, since the underlying def could be changed directly in that case, and you wouldn't need the (*defs)[i] = rbd
. Or is there something I'm missing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a fair shout.
I went for this solution, as I could just pass it in from operations.go
:
ensureExternalRefsInRequestBodyDefinitions(&bodyDefinitions, pathItem.Ref)
Whereas if I went with that solution, I'd need to create a new slice that had a pointer to each value within bodyDefinitions
🤔
As noted in #1378, there are cases where a complex set of
$ref
sbetween multiple files can lead to broken generated code, which does not
correctly import the package that has been prepared for the external
reference.
We can handle this by looking up any references, where there is a
.Ref
passed into the type, and then iterate through relevant children.
This requires we handle the updating in-place for these by using a bit
of pointer + indexing fun.
This also adds a relevant test case to validate the fix.
Closes #1378.
Note that this won't fix every case - we'll review it in #1440