idealley / feathers-hooks-rediscache

Set of caching hooks and routes for feathersjs.

MIT License

38 stars 12 forks source link

fix: handle redis connection errors #61

Closed SassNinja closed 5 years ago

SassNinja commented 5 years ago

Summary

(If you have not already please refer to the contributing guideline as described here)

[x] Tell us about the problem your pull request is solving.
[x] Are there any open issues that are related to this?
[ ] Is this PR dependent on PRs in other repos?

If so, please mention them to keep the conversations linked together.

Other Information

This PR fixes the bad or rather missing error handling of this hook when connection to redis is not possible. First of all an error event listener was missing what threw an error if redis is not available. But also the hooks itself didn't catch the case.

What I need is:

don't throw error if redis connection not possible (either on startup or at runtime)
reconnect if redis is available
don't block requests if redis not available and return uncached response instead

Therefore I've implemented a cache strategy, catch it in the hooks (if (!client) {}) and added some console.log Tests are included as well.

Closes https://github.com/idealley/feathers-hooks-rediscache/issues/58

@idealley it would be great if you can review this on the weekend, too.

If both of my PRs are merged I'm able to use your hooks in production in a much much better way :)

idealley commented 5 years ago

Could you check please, by pulling the new master? Tests are not passing on my machine with latest master, Travis as well.

Thank you for all the work.

Sam

SassNinja commented 5 years ago

You're right, I think due to my adjustments of the redisClient (not immediately available anymore) some tests fail. I'll fix it (will wait until client is ready).

idealley commented 5 years ago

Do you have a status on this?

SassNinja commented 5 years ago

Sorry, haven't had time yet to fix it (lot of work currently)

Will take care of it by the end of the week!

codecov[bot] commented 5 years ago

Codecov Report

Merging #61 into master will decrease coverage by 0.34%. The diff coverage is 100%.

@@            Coverage Diff            @@
##           master     #61      +/-   ##
=========================================
- Coverage   96.85%   96.5%   -0.35%     
=========================================
  Files           6       6              
  Lines         127     143      +16     
=========================================
+ Hits          123     138      +15     
- Misses          4       5       +1

Impacted Files	Coverage Δ
src/hooks/redis.js	`100% <100%> (ø)`	:arrow_up:
src/redisClient.js	`100% <100%> (ø)`	:arrow_up:
src/routes/cache.js	`92.1% <0%> (-2.19%)`	:arrow_down:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update b0dc8de...1f75a7f. Read the comment docs.

coveralls commented 5 years ago

Coverage decreased (-1.5%) to 91.139% when pulling e302aa74bd5ae0080ac96196c82630590cebed62 on SassNinja:fix/connection-error into b0dc8de5d537e8cea390bed115bf2d126183eb1d on idealley:master.

coveralls commented 5 years ago

Coverage increased (+0.8%) to 93.45% when pulling 1f75a7fffce78e9fe53e128079559300b2a97164 on SassNinja:fix/connection-error into b0dc8de5d537e8cea390bed115bf2d126183eb1d on idealley:master.

SassNinja commented 5 years ago

@idealley it took me some time to understand your tests, but I've been able to fix the issue. The tests are running again.

Would be great if you'll review my both PRs now and let me know what you think.

idealley commented 5 years ago

Thank you, looks good to me!