-
-
Notifications
You must be signed in to change notification settings - Fork 435
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
cache loaded remote schemas #117
Conversation
…e they are loaded and resolved. this avoids hundreds of roundtrips to remote server for schemas that contain multiple references
bd0b49a
to
7b543ac
Compare
} | ||
|
||
func NewSwaggerLoader() *SwaggerLoader { | ||
return &SwaggerLoader{} | ||
return &SwaggerLoader{loadedRemoteSchemas: map[url.URL]*Swagger{}} |
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.
Note that since this is only initialized/cleared once at creation there is the possibility that you could introduce unwanted behavior for existing users. For example, if you re-use the loader instance to reload an OpenAPI doc and expect to pull in the updated references as well this will fail after the proposed change.
I just checked and this is not a problem for API Sprout since it always creates a new loader. Just calling out that it might be problematic for others.
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.
Could you mention this in the docs (for example as the documentation for this very function?). It's better than buried in a PR comments :)
Also, could you not initialize the map here but instead check for nil
in resolveComponent
below? This would save allocation in the case the user does not rely on remote schemas.
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.
Ah well @danielgtaylor you're not the author... Thanks for pointing this out!
I understand this can be a major PITA and am very curious how this is supposed to be handled by OpenAPI / JSON Schema specs? Introducing remote refs immutability may break things, introduce races... so I'm leaning towards "these just have to be immutable". I just can't find the part of the documentation mentioning this? |
@bonnyr Hi! I think this should get in. Do you mind rebasing? Thanks Actually there now is a |
See https://pkg.go.dev/github.com/getkin/kin-openapi/openapi3#ReadFromURIFunc and specifically https://pkg.go.dev/github.com/getkin/kin-openapi/openapi3#URIMapCache which is used by default: var DefaultReadFromURI = URIMapCache(ReadFromURIs(ReadFromHTTP(http.DefaultClient), ReadFromFile)) |
when resolving components for loaded schemas and when external resolution is enabled,
kin-openapi will read the remote schema, parse it and resolve it in order to satisfy the reference attribute in the calling schema, but it does so every time a reference needs resolving leading
to unnecessary network traffic request for the same remote schema.
For example:
In the above case, external resolution for components.openapi.yml is performed when parsing the
fragment above only to be done again for ref2. This is recursive and if components.openapi.yml
includes other references they are resolved the same way.
This very quickly leads to major network traffic which at the very least is unnecessary.
This pull request caches the loaded schemas in the swagger loader and returns resolved schemas from the cache once they are loaded.
Added tests to ensure the schemas are loaded only once.