Number types in hgetall not always equal to hmset.

[jonathanslenders]

I cannot tell whether the bug in in txredisapi, in redis or our code.

Set the value '4', as a string in this hash mapping

redis.hmset('key', { "a": "4" })

Retrieve the hash again.

redis.hgetall('key')

This returns { "a": 4}, so the string turned to an integer.

[fiorix]

not sure, too, but look:

redis 127.0.0.1:6379> hmset "key" "a" "4" "b" 2
OK
redis 127.0.0.1:6379> hgetall "key"

1. "a"
2. "4"
3. "b"
4. "2"

and there's this (we do convert things in txredisapi):

https://github.com/fiorix/txredisapi/blob/master/txredisapi.py#L338

which i think was introduced by one of Jeethu Rao's patches.

On 2013-01-07, at 11:56 AM, Jonathan Slenders notifications@github.com wrote:

I cannot tell whether the bug in in txredisapi, in redis or our code.

Set the value '4', as a string in this hash mapping

redis.hmset('key', { "a": "4" })

Retrieve the hash again.

redis.hgetall('key')

This returns { "a": 4}, so the string turned to an integer.

—
Reply to this email directly or view it on GitHub.

[jonathanslenders]

So Redis only knows about bytestrings, and txredisapi does a clever guess? Right? (And the double quotes are for parameter grouping?)

That's nice, but not always needed or desired. For instance, we use json.dumps/loads to serialize hash values, because Python's json library knows about more primary types, like False/True and None.

For saving, we do:
hmset("key", { k, json.dumps(v) for k,v in original_dict.values() ])

For loading;
result = yield hgetall("key")
result = { k, json.loads(v) for k,v in result.values() ])

Thanks for your feedback, florix!

[jeethu]

This behaviour has been the same for a long time, my patch was to speed things up a bit by not trying to call int() (and incur the overhead of an exception) on every piece of data returned unless it's likely a number.

Redis bulk replies are always strings, it's just that txredisapi tries to convert values which look like integers and floats to integers and floats. In my opinion, this is the right thing to do.

Rather than changing the behaviour of the library, might I propose a simple workaround?

result = yield hgetall("key")
result = {k:json.loads(str(v)) for k,v in result.items()}

[jonathanslenders]

That works for me, but not sure whether it's entirely the good approach. It's workable though.

[fiorix]

Thanks

[jonathanslenders]

Hi fiorix, we found a use case where this doesn't work.

If we store the string "+22", the number 22 will be returned. Converting it back to str will turn it into "22". This is definitely not what we expect. I seriously suggest removing the automatic conversion, because probably there are a lot more issues and cases that we don't know about yet.

[fiorix]

I agree... but again, this is a change that breaks people's implementation all over the place. Perhaps it's time to introduce both this change and the one with the password on the connection methods at the same time.

By the way, the only way I see to not have automatic conversion is to only allow strings in and out.

[williamsjj]

I'd really like to see this implemented. Since redis doesn't support return datatypes, magic translation under the hood is always going to bite a lot of people.

[fiorix]

I need more time for this - and I'm currently on vacation abroad, coming back in 2 weeks.

[iksteen]

Any chance of this getting another look? I've commented out the magic casting code in a local copy because we work with decimal.Decimal so interpreting the string value using float() is a no-go for us. IMO, 'magic' behaviour is pretty undesirable for a low-level database driver.

[fiorix]

Sure, send a patch and I'll review. Even if it breaks backwards compatibility (which I think it will) we can debate about how to fix it properly.

[fiorix]

Perfect @jeethu. Now the magic is optional.