add middlewares to fill the cache #337
Conversation
arschles
added
storage
Codecov Report
@@ Coverage Diff @@
## master #337 +/- ##
=========================================
+ Coverage 37.64% 37.75% +0.1%
=========================================
Files 102 102
Lines 2757 2757
=========================================
+ Hits 1038 1041 +3
+ Misses 1615 1613 -2
+ Partials 104 103 -1
Continue to review full report at Codecov.
|
| return err | ||
| } | ||
| defer ref.Clear() | ||
| version, err := ref.Read() |
There was a problem hiding this comment.
Is this meant to escape this block? Currently it is never used besides the deferred zip close below.
There was a problem hiding this comment.
good call - this is supposed to be versionInfo, err = ref.Read(). change incoming
| lggr.SystemErr(err) | ||
| return err | ||
| } | ||
| c.Set("versionInfo", version) |
There was a problem hiding this comment.
Is this meant to set the versionInfo param to the string version from 23?
There was a problem hiding this comment.
this should set to versionInfo, thanks for catching this!
There was a problem hiding this comment.
Personally I'm not a fan of overloading context values because it has no safety guarantees and it's hard to test.
I'd suggest either making the middleware's next function be type safe (similar to what we discussed in #328) or we just implement a download.Protocol that eliminates the need for a middleware in the first place.
Something like this
func (p *protocol) Info(ctx context.Context, mod, ver string) (RevInfo, error) {
v, err := p.storage.Get(ctx, mod, ver)
if errors.ErrNotExist(err) {
v, err = p.filleCache(ctx, mod, ver)
}
return v.Info, err
}
func (p *protocol) fillCache(ctx, mod, ver) (Version, error) {
i, err := p.fetcher.Fetch(ctx, mod, ver)
if err != nil { return {}, err }
p.storage.Save(i) // handleErr
return i, err
}and so on.
Happy to hear your reasoning though, let me know!
| } | ||
| c.Set("versionInfo", version) | ||
|
|
||
| if err := getterSaver.Save( |
There was a problem hiding this comment.
This means we always save back to Storage on every http call even if cache already existed no?
There was a problem hiding this comment.
yup, that's a big bug on my part, thanks for catching
pkg/download/params_middleware.go
Outdated
|
|
||
| // ParamsMiddleware fetches module and version params from c and calls next with "value" | ||
| // and "version" strings in the context. Access them with c.Value("module") and c.Value("version") | ||
| func ParamsMiddleware(next buffalo.Handler) buffalo.Handler { |
There was a problem hiding this comment.
Why do we need middleware for this if we can just call path.GetAllParams from inside a handler?
There was a problem hiding this comment.
PS. if it's the error handling part, paths.GetAllParams shouldn't really return an error. It can safely panic because that means we got something seriously screwed up in our http paths.
There was a problem hiding this comment.
@marwan-at-work that's fair. I was trying to remove a bit of error handling code from all the handlers here. I'll move this into the cache fill middleware handler
|
@marwan-at-work RE your comment on overloading context - I was following the buffalo middleware docs here. I def understand the type-safety argument and normally I'd be for it, but I think it's a tough tradeoff in this case because we exchange a bit of "magic" (setting Would you be ok with submitting just the download protocol bits from #336 in a new PR so we can get that in separately from the EDIT: I see that #336 is almost ready to merge |
|
Closing in favor of #342 |
also adding a middleware to fill in the module and version names
cc/ @marwan-at-work - this could be an alternative or complement (not sure?) to the download protocol you introduced in #336
this makes progress toward #290 because the cache fill middleware can wrap the zip, mod and info endpoints to give them sync. cache filling "for free"