Handle None return from feature store .all()

[thieman]

Fixes a bug where self._store.all() returns None without throwing an Exception. This would go on to immediately fail on line 328 as None does not have .items(). Instead, this makes us go through the established error handler and bail out with FeatureFlagsState(False)

cc @mattbriancon

[eli-darkly]

Hmm. I agree that the current implementation is a problem, but I'm not sure if this is the best place to fix it - especially if that means having to raise and catch another exception. The other option would be to just change the feature store so it returns an empty dict, rather than None, for these error conditions. That would be consistent with the behavior of the single-item get method, where if there is a Redis error it will just behave the same as if no such item existed (though it will log the error).

[eli-darkly]

On the other hand... your approach will result in setting the valid property of the state object to False, which is appropriate since this is an error condition. So, OK. Thanks for catching this!

[eli-darkly]

Actually I have one request: could you change the exception message from "flags_map is None, aborting" to "feature store error"? That would be more descriptive - no one should need to know this particular variable name. The details of the error should have already been logged by the feature store at this point.

[thieman]

Potentially dumb question: is the condition on this line actually an error / invalid state? Is there a different valid state the store is expected to be in if it's initialized but empty? https://github.com/launchdarkly/python-client/blob/master/ldclient/redis_feature_store.py#L72

[eli-darkly]

is the condition on this line actually an error / invalid state?

Well, this is more confusing than I thought.

I was thinking that hgetall should return an empty dict, rather than None or "" in that case. My assumption was that the hash key would still exist even if there are no items. However, looking again at how we've implemented init (lines 47-51), it seems that where there are no items the hash key will not exist.

Unfortunately, the redis-py documentation says nothing at all about how hgetall is supposed to behave in this case. So I took a look at the source code and it's still not entirely clear to me, but this line suggests that it should return an empty dict. I will verify that experimentally.

[eli-darkly]

Indeed, hgetall always returns a dict even if the key does not exist, assuming there wasn't some other kind of Redis error. So I would say that yes, if flags_map is None or "" then that is an error condition.