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.

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

Issue #204 - Redis Get response in case of missing value #238

Merged
merged 2 commits into from Jul 22, 2020
Merged

Issue #204 - Redis Get response in case of missing value #238

merged 2 commits into from Jul 22, 2020

Conversation

christgf
Copy link

Signed-off-by: George Christou christgf@gmail.com

Which problem is this PR solving?

Resolves #204 - Redis Get response in case of missing value

Short description of the changes

When returning results from Redis, an empty cache key is handled as an error, which is inconsistent with the caching API interface. The change ensures cache misses don't result in errors, as proposed.
Please note there's currently no test fixture for the Redis cache implementation.

* Handle err == redis.Nil as a cache miss instead of an error
Signed-off-by: George Christou <christgf@gmail.com>
cache/redis/redis.go Outdated Show resolved Hide resolved
* Happy path should be last statement
Signed-off-by: George Christou <christgf@gmail.com>
@christgf christgf requested a review from mantzas July 21, 2020 17:19
Copy link

@mantzas mantzas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, thanks very much for the contribution.

@codecov
Copy link

codecov bot commented Jul 21, 2020

Codecov Report

Merging #238 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #238   +/-   ##
=======================================
  Coverage   73.01%   73.01%           
=======================================
  Files          57       57           
  Lines        3332     3332           
=======================================
  Hits         2433     2433           
  Misses        837      837           
  Partials       62       62           

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 2e67018...db834f3. Read the comment docs.

Copy link

@tpaschalis tpaschalis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for taking the time to open your first PR in Patron!

It looks good, just one remark; I think that we can remove this const Nil = redis.Nil as well.

It was used to export the redis.Nil error so that the end-user could implement the cache miss check herself, but this is no longer necessary!

@christgf
Copy link
Author

Thank you for taking the time to open your first PR in Patron!

It looks good, just one remark; I think that we can remove this const Nil = redis.Nil as well.

It was used to export the redis.Nil error so that the end-user could implement the cache miss check herself, but this is no longer necessary!

Actually @tpaschalis, it seems that this still serves its original purpose, and removing it would require we import redis/v7 in the Redis cache implementation (which is where the redis.Nil check is needed).
I may be missing your point altogether?

@tpaschalis
Copy link

tpaschalis commented Jul 21, 2020

Actually @tpaschalis, it seems that this still serves its original purpose, and removing it would require we import redis/v7 in the Redis cache implementation (which is where the redis.Nil check is needed).
I may be missing your point altogether?

On the other hand, you're right, let's keep the imports tidied up!

@tpaschalis tpaschalis self-requested a review July 21, 2020 18:33
Copy link

@tpaschalis tpaschalis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Copy link

@drakos74 drakos74 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice!!!

@mantzas mantzas merged commit f8ebb41 into beatlabs:master Jul 22, 2020
Stefos pushed a commit to Stefos/patron that referenced this pull request Oct 8, 2020
…tlabs#238)

Signed-off-by: George Christou <christgf@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Handle redis Get response gracefully in case of missing value
4 participants