Fix: Ensure external refs are propagated to generated code#1389
Merged
jamietanna merged 1 commit intomasterfrom Jan 25, 2024
Merged
Fix: Ensure external refs are propagated to generated code#1389jamietanna merged 1 commit intomasterfrom
jamietanna merged 1 commit intomasterfrom
Conversation
jamietanna
force-pushed
the
feature/external-ref-strict
branch
from
71813ff to
520ad55
Compare
jamietanna
commented
Dec 18, 2023
pkg/codegen/operations.go
Outdated
| Schema: contentSchema, | ||
| } | ||
|
|
||
| // TODO finish comment |
jamietanna
commented
Dec 18, 2023
pkg/codegen/operations.go
Outdated
| // TODO finish comment | ||
| parts := strings.SplitN(parentRef, "#", 2) | ||
| if parentRef != "" && len(parts) == 2 { | ||
| // TODO finish comment |
jamietanna
commented
Dec 18, 2023
pkg/codegen/operations.go
Outdated
| rcd.Schema = Schema{ | ||
| RefType: fmt.Sprintf("%s.%s", pack.Name, contentSchema.GoType), | ||
| } | ||
| } // TODO log when not found |
Member
Author
There was a problem hiding this comment.
Not as part of this one
jamietanna
force-pushed
the
feature/external-ref-1378
branch
from
e7cef18 to
f4d7d4c
Compare
Closed
|
I'm getting the following error when trying this out: |
jamietanna
force-pushed
the
feature/external-ref-1378
branch
from
14ef705 to
3e25b53
Compare
jamietanna
force-pushed
the
feature/external-ref-strict
branch
from
520ad55 to
1ef78fc
Compare
jamietanna
commented
Jan 19, 2024
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 |
jamietanna
commented
Jan 19, 2024
This was referenced Jan 25, 2024
jamietanna
force-pushed
the
feature/external-ref-1378
branch
from
f8b232b to
55e3ce2
Compare
jamietanna
changed the title
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.
jamietanna
force-pushed
the
feature/external-ref-1378
branch
from
55e3ce2 to
0692acc
Compare
jamietanna
changed the base branch from
feature/external-ref-strict
to
master
markuswustenberg
approved these changes
Jan 25, 2024
markuswustenberg
left a comment
markuswustenberg
left a comment
There was a problem hiding this comment.
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.
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?
Member
Author
There was a problem hiding this comment.
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 🤔
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
As noted in #1378, there are cases where a complex set of
$refsbetween 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
.Refpassed 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