Conversation
ahx
left a comment
ahx
left a comment
There was a problem hiding this comment.
Hi. Thanks for the PR. I was hesitant to add caching outside of the "register" interface, but this makes total sense.
lib/openapi_first/file_loader.rb
Outdated
| end | ||
|
|
||
| body | ||
| def clear_cache |
There was a problem hiding this comment.
Did you had a situation where you had to call this method? – If not, I'd like to remove this and maybe add an interface on a higher level.
There was a problem hiding this comment.
I do (I should have called this out).
In the Rails app I am working on, it is set up to watch the yml files for reloading. So if a change is made to a schema, the entire server doesn't have to restart. Before reloading the OpenAPI definition, you would need to clear this cache.
I figured that this may be a general "need" so included this.
|
Can you update the changelog (just under „unreleased“ please? I’ll merge after that. |
| - `OpenapiFirst::FileLoader` will now cache the contents of files that have been loaded. This cache | ||
| is keyed on the file's path. If you need to reload your OpenAPI definition for tests or server hot | ||
| reloading, you can run `OpenapiFirst::FileLoader.clear_cache!`. |
There was a problem hiding this comment.
Is this something you'd like documented in the README as well? Until now, FileLoader was an implementation detail.
Maybe this should be OpenapiFirst.clear_cache! or something like that?
There was a problem hiding this comment.
Yes, the changelog should not mention internal APIs, but just what was behavior was changed / improved. I'll merge this PR and might rephrase this part in the next release (till friday).
|
Thanks for the work on this. I think this is a great improval. |
2 participants
I noticed that when loading an Openapi document with
$refs to other yaml files, files were being loaded more than once.For example, when loading the splitted-train-travel-api from the specs
You'll see the same file loaded multiple times (note: I put a log message in
OpenapiFirst::FileLoader#loadto test this)I'm honestly not sure why this is happening. I thought that this may be because a yaml file could be referenced multiple times in the same document, but this appears to still be happening for files that are only referenced a single time. I would have expected the
JSONSchemer::CachedResolverto handle most of this, but perhaps the caching is only taking place after all of the nested$refs have been loaded.This PR adds a cache to the
FileLoader. It is thread safe and follows the check-lock-check pattern to reduce overhead from the mutex. With this in place, loading the same document results in the files only being loaded a single time.I saw #426, so perhaps this is a way to mitigate some of those memory issues.
A quick memory benchmark for
Before
After