Skip to content

Fail gracefully with a default return value when 0 keys are are provided to a command expecting at least 1 key #1052

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

Merged
merged 3 commits into from
Nov 5, 2018

Conversation

andymccurdy
Copy link
Contributor

This is an alternative implementation to #941

Some commands like MGET, DEL, etc. accept 1 or more keys but fail if the user supplies 0 keys. From a user point of view, it would be nice if these commands simply returned an empty value when called with 0 keys. For example, MGET could return an empty list and DEL could return 0 indicating 0 keys were deleted. However, commands cannot simply return an empty value because of how they interact with pipelines.

This PR introduces a well named option EMPTY_ERROR. If a command detects one of these 0-key scenarios, it can pass the EMPTY_ERROR option with the default value that should be returned to the application. This includes the plumbing to work with pipelines.

Currently only MGET is implemented as an example. Other commands like DEL should also get this functionality if merged.

The question we need to answer is should we actually do this.

allow commands that expect 1 or more keys to fail gracefully when 0 keys are provided
@RoeyPrat
Copy link

RoeyPrat commented Nov 4, 2018

@andymccurdy I like this feature, but it gives me pause.

On one hand, I believe this is easier for the dev, perhaps even more pythonic.
It also matches the part of my brain that still works in SQL (select * from X where Y is in []).

On the other hand, this is a serious behaviour change from Redis.
I'm not that worried about backwards compatibility, though that also might be an issue.

I thought it would be nice to have two classes, one that is more pythonic, and easier to use, and one that strictly does what redis does. Then I noticed that we kind of already have that (Redis vs StrictRedis).

IMHO StrictRedis should be just that - strict, while features like this should be implemented elsewhere.
Maybe Redis class is a good fit for this, maybe not. If you think we need a new class for this, let's call it LiberalRedis or something.

@andymccurdy
Copy link
Contributor Author

andymccurdy commented Nov 4, 2018

@RoeyPrat

re: Backwards Compatibility: It seems like now is the time to get these kinds of changes in. There will be several compatibility issues with v3 and I'd prefer to get all this kind of stuff in at the same time.

re: StrictRedis vs. Redis: This is a longer discussion, but my hope is that we remove the StrictRedis/Redis distinction for v3. Some history: StrictRedis was created because I initially implemented some commands that deviated away from the server's expected syntax. This lead to lots of questions/bug reports, so I created StrictRedis to adhere to the server's command syntax and updated all the docs to encourage its use. The Redis class was kept for backwards compatibility. Sadly, this didn't really fix the bug report/question problem -- there's still quite a few bug reports about ZADD/SETEX/etc. because some people continue to use the Redis class.

The docs have encouraged the use of StrictRedis for years now. I'd like to have only one class to make things less confusing to users. My plan is to delete the Redis class, rename StrictRedis -> Redis, and create a StrictRedis alias of Redis. People using StrictRedis today will see no difference and continue to function as they do today without any changes. People using the Redis class will be affected if they use the small group of commands that it overrides. Providing better documentation on this stuff should help reduce the issues going forward.

All this is to say that my feeling has evolved into favoring user happiness and being "more pythonic" over strictly adhering to what the server expects, provided that we:

  1. Have great documentation so users know what to expect
  2. Provide enhancements over the server's behavior that are lossless, meaning these types of changes are only adding syntactic sugar or create an experience that's superior to the server without hiding any information that the user might need or want.

IMO, this change meets the lossless requirement. It's a better user experience and providing the default return value doesn't hide information or require the need for any special case code.

@RoeyPrat
Copy link

RoeyPrat commented Nov 5, 2018

@andymccurdy Ok. that sounds like a good plan.

@andymccurdy andymccurdy merged commit 5f61ad9 into master Nov 5, 2018
@tzickel
Copy link

tzickel commented Nov 6, 2018

@RoeyPrat the constructor could also have another flag for throwing exceptions (like before) or silently handling this.

Also, might be time to start mapping which other commands like MGET need to be patched like this

@andymccurdy andymccurdy deleted the error_defaults branch November 9, 2018 03:28
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.

3 participants