#1434 Added support for ZMSCORE new in Redis 6.2 RC

[jiekun]

Pull Request check-list

Please make sure to review and check all of these items:

• Does $ tox pass with this change (including linting)?
• Does travis tests pass with this change (enable it first in your forked repo and wait for the travis build to finish)?
• Is the new or changed code fully tested?
• Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?

NOTE: these things are not required to open a PR and can be done
afterwards / while the PR is open.

Description of change

Added support for zmscore command.
#1434
#1435

>>> r.zmscore(key="test", members=["member1", "member2", "invalid_member"])
[1.0, 2.0, None]

[codecov-io]

Codecov Report

Merging #1437 (77eadf3) into master (15dafb1) will increase coverage by 0.55%.
The diff coverage is 80.00%.

@@            Coverage Diff             @@
##           master    #1437      +/-   ##
==========================================
+ Coverage   85.90%   86.45%   +0.55%     
==========================================
  Files           6        6              
  Lines        2894     2909      +15     
==========================================
+ Hits         2486     2515      +29     
+ Misses        408      394      -14     

Impacted Files	Coverage Δ
redis/client.py	86.80% <80.00%> (+0.87%)	⬆️

---

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 15dafb1...77eadf3. Read the comment docs.

[adamserafini]

One comment: AFAICT, I think the new test introduced here never runs on CI because the base docker image for redis needs to bumped to something >= 6.2: https://github.com/andymccurdy/redis-py/blob/master/docker/base/Dockerfile#L1

The test is currently always skipped because @skip_if_server_version_lt('6.1.240') will be true.

[jiekun]

One comment: AFAICT, I think the new test introduced here never runs on CI because the base docker image for redis needs to bumped to something >= 6.2: https://github.com/andymccurdy/redis-py/blob/master/docker/base/Dockerfile#L1

The test is currently always skipped because @skip_if_server_version_lt('6.1.240') will be true.

Yes I see. So when I wrote the test, I removed the decorator and run it locally(tox) with version 6.2RC.
But you are right I think we need to update the docker image thing for Codecov testing.

[chayim]

This PR does that, it's merged in. Can you please re-sync with master, and then tests can run! Thank you again.

[chayim]

Suggested change

	if not isinstance(members, list) or len(members) < 1:
	if not members:

[jiekun]

So we should check if it's None or not and if it's a list type data?

[chayim]

According to the zmscore docs ZMSCORE requires a minimum of one member. This either means that we either:

1. Validate that there's a non-zero length list (as above).
2. Validate that the item passed in is a string or a dictionary.

I'd rather we not overload the members argument, and instead always pass in a list. For the simple case, the list has one item (i.e ['foo']). Validating that we were passed a list I think is overkill, as the docs and the function signature are indicative of the requirement.

[chayim]

Suggested change

	def zmscore(self, key, members):
	def zmscore(self, key, members=[]):

[jiekun]

Sorry for the late reply.

I am ok with a default value proposal (for compatible reason). But not sure if using a mutable type is fine. This contains risk that the default value might be modified.

So maybe I should change it with a default value None and modify the function body accordingly?

[chayim]

Hi @2014BDuck thanks for your submission! Everything passes with the current unit tests.

[jiekun]

@chayim Hi sorry for the late reply. I would prefer using an immutable type as a default value, what do you think?

[chayim]

I think members should merely be required, and we shouldn't do a check on it. It should be a list. For what it's worth in the future I'm pretty pro type hints, but we're not there.

…

--c

[jiekun]

@chayim OK then. I am going to add a list-type default value now.

[jiekun]

Ah sorry for the change. I am still thinking if we could just keep not using default value for the argument members

And we can stick with the same validation you suggested:

if not members:
    raise DataError()

And, use empty list as default won't stop communicating with redis server when user pass, for example "asdfasdf" or a dict contain something.

So the code is clean, same effect as we use empty list as default.

@chayim

[chayim]

For ZMSCORE, you have to have at a minimum one member <https://redis.io/commands/zmscore>. and it's a list of members. We should pass in a list, in order to not allow for overloadable constructs. A list of length one, is still a list.

[jiekun]

For ZMSCORE, you have to have at a minimum one member https://redis.io/commands/zmscore. and it's a list of members. We should pass in a list, in order to not allow for overloadable constructs. A list of length one, is still a list.

Yes it requires a minimum one member so we don't need a default value, as setting a [] as default won't have different behaviour as the current one. Like I said it still won't pass the validation. And if user don't pass a list with length >= 1, the suggested changes won't help.

So the only thing it can help is to tell user that it should be a list. However user can notice this requirement in the function doc.

[chayim]

Two difference things are at play. The first is that yes - we should document accordingly. Secondly, given that the only viable input is a list, we should accept a list. If you want to validate the minimum length of the list, to support the client use - I can support that! However, we can't accept a default value of None, as the command doesn't support that.

[jiekun]

Yes I see.

So currently the default value is removed, which means user must pass members in.

# user cannot do
r.zmscore("key")

# user can do:
r.zmscore("key", ["a", "b"])             # case 0
r.zmscore("key", "invalid_value")        # case 1
r.zmscore("key", [])                     # case 2
r.zmscore("key", {"invalide": "kv"})     # case 3

we should accept a list.

And with a [] as default value, user can still call this method with all 4 cases' input. The [] is not helpful in limiting users' input.

Currently, this method can do:

• Users must send a members argument.

Can't:

• Limiting users to send a list input (The possible way is to validate by isinstance, which is suggested to change at the very beginning)

My point is, the default value won't affect any validation logic in the function body. No matter what default value we set, users can pass anything they want. So why not just don't set the default value and require an argument to be passed in. And we will have 2 different ways to handle the rest:

1. validate if it's a list with item: if isinstance(members, list) and len(members) > 0:.
2. A less solid validation: if members: and hand it over to Redis server. (Currently using)

I think either way we don't need the default value.

[chayim]

There are really only two cases present - the right items being specified, or not. The problem with validating the existence of a list, is that it doesn't prevent a user from passing in an invalid list - hence my being against that valiadation.

Take as an example:

x = ['a', 'b']
y = ['a', 'b', 999, {'hello': 'world'}
z = ['a', None]
assert isinstance(x, list)
assert isinstance(y, list)
assert isinstance(z, list)

etc.

In this example, True will always be accurate, as everything is a list. I can appreciate the isinstance check on a list, but I wouldn't do anything beyond that. The use will receive a ResponseError should they provide invalid input to redis. Were we to sanitize the inputs in this function, we'd be responsible for raising a DataError (or to be fair an AttributeError) exception in all cases. I'd rather allow redis to do what it does and surface accordingly.

The following is a valid case, and I could understand generally doing this. However, I feel that type hints are a better answer.

x = ['some','list','of','things']
if not isinstance(x, list):
    raise AttributeError("x must be a list.")

[jiekun]

OK, I am more clear about the responsibility of redis-py and redis server now.

But I can't agree with using a mutable object as a default value. It's not recommended in any way and I never do this. Type hint could do this, but using [] as default value is just absolutely a different (and not secure) thing.

So for the function signature part: Use None as default(not preferred as we discussed) or just don't set a default value. Since user need to understand that they have to pass a list, I think we should keep the current code without a default value.

And for the body part, I've removed the isinstance() validation for a long time. I think we could just be specific on the function signature now if you have a better idea.

[jiekun]

By the way, we have warnings for unresolved reference on the Commands class (for the execute_command method) after the refactor in #1534 . Is it awarded and planed to fix?

[chayim]

Hey @2014BDuck I'll take a look at the warning in another issue - or happily accept a PR. As always, thanks for this contribution, it's great to see!