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

tools/importer-rest-api-specs - correctly find all referenced models to mixin #4411

Closed
wants to merge 2 commits into from

Conversation

teowa
Copy link
Contributor

@teowa teowa commented Sep 12, 2024

resolve issue in PR #4374
supersede #4386

This PR optimize the findAndMergeLocalMixins to find all specific models being referenced.

Current status: findAndMergeLocalMixins will mixin the whole swagger doc that being referenced in current file. Mixin the whole swagger file and then perform the analysis.Flatten will sometimes introduce un-relative models, as mentioned in #4374 (comment).

More detail: networkManagerSecurityAdminConfiguration.json refer to the neworkManager.json,
findAndMergeLocalMixins will mixin the whole neworkManager.json. And later the analysis.Flatten will introduce network.json#ProvisioingState

The conflict will happen for networkManagerSecurityAdminConfiguration.json#ProvisioingState and network.json#ProvisioingState. But actually networkManagerSecurityAdminConfiguration.json only use the ProvisioingState in its own file, but another ProvisioningState is introduced while parsing networkManagerSecurityAdminConfiguration.json, through the route
networkManagerSecurityAdminConfiguration.json -> whole networkManager.json -> network.json#ProvisioningState

Note although networkManagerSecurityAdminConfiguration.json -> network.json, the whole network.json file will be added by findAndMergeLocalMixins, the ProvisioningState will not be overwritten. So the conflict is happend by the effect of both findAndMergeLocalMixins and analysis.Flatten

expandedSwaggerDoc, err = findAndMergeLocalMixins(expandedSwaggerDoc, directory, fileName)
if err != nil {
return nil, fmt.Errorf("could not mixin remote swagger files referenced by %q: %+v", filePath, err)
}
flattenedExpandedOpts := &analysis.FlattenOpts{
Minimal: false,
Verbose: true,
Expand: false,
RemoveUnused: false,
//ContinueOnError: true,
BasePath: expandedSwaggerDoc.SpecFilePath(),
Spec: analysis.New(expandedSwaggerDoc.Spec()),
}
if err := analysis.Flatten(*flattenedExpandedOpts); err != nil {
return nil, fmt.Errorf("flattening expanded swagger file %q: %+v", filePath, err)
}

@teowa teowa changed the title tools/importer-rest-api-specs - correctly find all referenced model to mixin tools/importer-rest-api-specs - correctly find all referenced models to mixin Sep 12, 2024
@teowa
Copy link
Contributor Author

teowa commented Sep 12, 2024

@manicminer
Copy link
Contributor

@teowa Thanks for suggesting this and for the implementation. As we are busy refactoring the importer-rest-api-specs tool, I have taken this idea and incorporated it into an updated Swagger file parser. Due to other unfortunate bugs in the go-openapi libraries, we have had to take on full responsibility for parsing remote references, and we've been able to do this in a way that achieves the goal here of only loading models that are explicitly referenced, and not loading in the entire remote swagger file. Please see https://github.com/hashicorp/pandora/pull/4307/files#diff-be8846c315219d09fdcf129da305752193df70c3acb8eed2a9bc414473863156 for the final version of this.

As such, I'll close this off as this has been superseded by #4307 - thanks!

@manicminer manicminer closed this Oct 1, 2024
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