-
Notifications
You must be signed in to change notification settings - Fork 350
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 middleware #264
Cache middleware #264
Conversation
Codecov Report
@@ Coverage Diff @@
## master #264 +/- ##
==========================================
+ Coverage 92.14% 95.81% +3.67%
==========================================
Files 26 24 -2
Lines 624 574 -50
==========================================
- Hits 575 550 -25
+ Misses 49 24 -25
Continue to review full report at Codecov.
|
Adding optional redix dependency doesn't seem a good idea in long term. For example, when new redix major release with breaking change may require upgrading tesla to support it (or support both old and new version). A project using tesla cache and redix directly... may need to have own middleware if they cannot upgrade tesla for redix update. What about renaming it to Also we may introduce common func signature for separate caching library (e.g. with Redis) |
@chulkilee It's a can of worms of dependencies ;) IF tesla gets this http cache middleware in core it should definitely ship with ets backend, the redis one is so simple it could even go to wiki to be copy-pasted (or live as tesla_cache_redix package if needed of course) |
@teamon, do you need help picking this up? I could use it for my work. I recommendation would be to use a separate caching library (https://github.com/cabol/nebulex for example). |
@jtarchie Yes! 😍 Ideally, |
I'd have to take a look at everything. HTTP caching is hard. I appreciate that you model this after someone else's implementation, though. Can you clarify your vision for the cache layer so it is generic enough? |
I don't really have a full vision, more like a bunch of goals:
|
I have a similar project. Basically trying to graphql a restful API, and the |
@teamon I would like to take a stab at this branch, as I'm experimenting with it in a few side-projects. With the advances on the OpenAPI branch, that may also look a good addition overall. From those experiments, something I've noticed is that there is no way to set options to default Stores, like what redix or
Last but not least: should I make a branch and target this branch with added code, tests etc? |
That would be amazing! I think that by default, tesla could ship with Having said that, I think passing configuration options like Feel free to start from this branch, but maybe rebase with master before moving forward. I'd be happy to replace this branch with yours later. Let me know if you have any questions, thanks. |
Hey @teamon, I'm working on https://github.com/andrewhr/tesla/tree/cache fork. Code is rebased, got TTL args and a very simple ETS implementation (replacing the Redis one). I've also made a try on Talking about API, I'm not sure if I really like the result. The Store behaviour feels a bit inflated, with a total of 4 args for Maybe we're just good with Singleton store modules. Looking forward for your thoughts, I appreciate your time helping me here 🙇 |
I've opened a fresh PR so we can talk about specific parts - #508 |
Preface
This is a rewrite of the excellent faraday-http-cache by Plataformatec, so all credit goes to @lucasmazza and other contributors.
Description
This middleware implements HTTP cache (more or less) like a web browser does:
Cache-Control
headers to make decisions (no-cache
,no-store
,max-age
etc)Vary
header as a cache key (if present)ETag
& related and does response validationAge
,Date
andExpires
headers to determine cached response freshnessPOST /user/1
invalidates/user/1
)Implemented decision flow
This is still very experimental and has NOT yet been used in production environment.
To-Do
ttl
to store (very useful with redis)Store.ETS
File organization
At the moment everything is in single file (
cache.ex
) and single test file (cache_test.exs
).Let's keep it that way until there is a fully functional implementation. I'm not yet sure if this should be added to tesla's core or should it be a separate package. I also don't like few things about the implementation (i.e. the
Request
/Response
wrapping seems redundant) which are direct rewrites from ruby's version and I believe they could be more elixirish (make it work before making it pretty)