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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

cache loaded remote schemas #117

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 8 additions & 3 deletions openapi3/swagger_loader.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,10 +30,11 @@ type SwaggerLoader struct {
Context context.Context
LoadSwaggerFromURIFunc func(loader *SwaggerLoader, url *url.URL) (*Swagger, error)
visited map[interface{}]struct{}
loadedRemoteSchemas map[url.URL]*Swagger
}

func NewSwaggerLoader() *SwaggerLoader {
return &SwaggerLoader{}
return &SwaggerLoader{loadedRemoteSchemas: map[url.URL]*Swagger{}}
Copy link
Collaborator

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.

Copy link
Collaborator

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.

Copy link
Collaborator

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!

}

func (swaggerLoader *SwaggerLoader) LoadSwaggerFromURI(location *url.URL) (*Swagger, error) {
Expand Down Expand Up @@ -222,9 +223,13 @@ func (swaggerLoader *SwaggerLoader) resolveComponent(swagger *Swagger, ref strin
return nil, "", nil, fmt.Errorf("Error while resolving path: %v", err)
}

if swagger, err = swaggerLoader.LoadSwaggerFromURI(resolvedPath); err != nil {
return nil, "", nil, fmt.Errorf("Error while resolving reference '%s': %v", ref, err)
if swg2, ok := swaggerLoader.loadedRemoteSchemas[*parsedURL]; !ok {
if swg2, err = swaggerLoader.LoadSwaggerFromURI(resolvedPath); err != nil {
return nil, "", nil, fmt.Errorf("Error while resolving reference '%s': %v", ref, err)
}
swaggerLoader.loadedRemoteSchemas[*parsedURL] = swg2
}
swagger = swaggerLoader.loadedRemoteSchemas[*parsedURL]
ref = fmt.Sprintf("#%s", fragment)
componentPath = resolvedPath
}
Expand Down
43 changes: 37 additions & 6 deletions openapi3/swagger_loader_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,10 +184,10 @@ func TestResolveSchemaExternalRef(t *testing.T) {
},
},
}
loader := &openapi3.SwaggerLoader{
IsExternalRefsAllowed: true,
LoadSwaggerFromURIFunc: multipleSourceLoader.LoadSwaggerFromURI,
}
loader := openapi3.NewSwaggerLoader()
loader.IsExternalRefsAllowed = true
loader.LoadSwaggerFromURIFunc = multipleSourceLoader.LoadSwaggerFromURI

doc, err := loader.LoadSwaggerFromURI(rootLocation)
require.NoError(t, err)
err = doc.Validate(loader.Context)
Expand Down Expand Up @@ -307,10 +307,10 @@ func TestLoadFromRemoteURL(t *testing.T) {

loader := openapi3.NewSwaggerLoader()
loader.IsExternalRefsAllowed = true
url, err := url.Parse("http://" + addr + "/test.openapi.json")
remote, err := url.Parse("http://" + addr + "/test.openapi.json")
require.NoError(t, err)

swagger, err := loader.LoadSwaggerFromURI(url)
swagger, err := loader.LoadSwaggerFromURI(remote)
require.NoError(t, err)

require.Equal(t, "string", swagger.Components.Schemas["TestSchema"].Value.Type)
Expand Down Expand Up @@ -423,3 +423,34 @@ func TestLoadYamlFileWithExternalSchemaRef(t *testing.T) {

require.NotNil(t, swagger.Components.Schemas["AnotherTestSchema"].Value.Type)
}

type hitCntFS struct {
fs http.Dir
hits map[string]int
}

func (fs hitCntFS) Open(fn string) (http.File, error) {
fs.hits[fn] = fs.hits[fn] + 1
return fs.fs.Open(fn)
}

func TestRemoteURLCaching(t *testing.T) {

sfs := hitCntFS{fs: "testdata", hits: map[string]int{}}
fs := http.FileServer(sfs)
ts := createTestServer(fs)
ts.Start()
defer ts.Close()

loader := openapi3.NewSwaggerLoader()
loader.IsExternalRefsAllowed = true
remote, err := url.Parse("http://" + addr + "/test.refcache.openapi.yml")
require.NoError(t, err)

_, err = loader.LoadSwaggerFromURI(remote)
require.NoError(t, err)

require.Contains(t, sfs.hits, "/test.refcache.openapi.yml")
require.Contains(t, sfs.hits, "/components.openapi.yml")
require.Equal(t, 1, sfs.hits["/components.openapi.yml"], "expcting 1 load of referenced schema")
}
15 changes: 15 additions & 0 deletions openapi3/testdata/test.refcache.openapi.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
---
openapi: 3.0.0
info:
title: 'OAI Specification w/ refs in YAML. Multiple refs to the same remote schema'
version: '1'
paths: {}
components:
schemas:
AnotherTestSchema:
type: object
properties:
ref1:
"$ref": http://localhost:7965/components.openapi.yml#/components/schemas/CustomTestSchema
ref2:
"$ref": http://localhost:7965/components.openapi.yml#/components/schemas/Name