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

Keys are not consistently converted #170

Closed
jgeiger opened this issue Aug 9, 2019 · 4 comments
Closed

Keys are not consistently converted #170

jgeiger opened this issue Aug 9, 2019 · 4 comments
Labels

Comments

@jgeiger
Copy link

jgeiger commented Aug 9, 2019

When using a key in Redis, 1 and "1" are equivalent (at least in Redis 3.x). While there are a few cases where this was fixed for some methods in #157 and #48, it should be a global fix for all key access.

I've recently run into this issue for lpush and lrange. I'll look into a PR, but this is more of a notice for anyone who may be seeing this issue in their code.

@meagar
Copy link

meagar commented Sep 4, 2019

Similar problem for hget/hset, the results are very inconsistent:

r = MockRedis.new

r.hset(123, 'foo', 'bar') # => true
r.keys # => [] # expected [123] or ['123'], but not empty
r.hgetall(123) # => { 'foo' => 'bar' } # content is still queryable 

r.flushall # => "OK"
r.keys # => [] # as expected, the store should be empty
r.hgetall(123) # => { 'foo' => 'bar' } # numeric keys still map to values after `flushall`

@sds sds added the bug label Sep 7, 2019
@JeffLuckett
Copy link

There are also issues with using symbols as keys in Redis hashes. See below:

{14:01}[2.5.1]~ ➭ irb
irb(main):001:0> require 'mock_redis'
=> true
irb(main):002:0> mr = MockRedis.new
=> #<MockRedis:0x00007f8540035b20 @options={:scheme=>"redis", :host=>"127.0.0.1", :port=>6379, :path=>nil, :timeout=>5.0, :password=>nil, :db=>0, :time_class=>Time}, @db=#<MockRedis::PipelinedWrapper:0x00007f853e9b7380 @db=#<MockRedis::TransactionWrapper:0x00007f853e9b7d80 @db=#<MockRedis::ExpireWrapper:0x00007f853e9b7e70 @db=#<MockRedis::MultiDbWrapper:0x00007f8540034c48 @db_index=0, @prototype_db=#<MockRedis::Database:0x00007f85400348b0 @base=#<MockRedis:0x00007f8540035b20 ...>, @data={}, @expire_times=[]>, @databases={0=>#<MockRedis::Database:0x00007f85400351e8 @base=#<MockRedis:0x00007f8540035b20 ...>, @data={}, @expire_times=[]>}>>, @transaction_futures=[], @multi_stack=[], @multi_block_given=false>, @pipelined_futures=[], @nesting_level=0>>
irb(main):003:0> mr.hset(:foo, :bar, :bas)
=> true
irb(main):004:0> mr.hgetall(:foo)
=> {"bar"=>"bas"}
irb(main):005:0> mr.data
=> {:foo=>{"bar"=>"bas"}}
irb(main):006:0> mr.del(:foo)
=> 0
irb(main):007:0> mr.data
=> {:foo=>{"bar"=>"bas"}}

Note the del operation didn't behave as expected, because del converts all keys to strings. However, you can see in my mr.data call that the key is a symbol.

PR to follow.

@JeffLuckett
Copy link

JeffLuckett commented Oct 8, 2019

This is addressed in PR: #173.

Until this PR is merged, and a new Gem version is cut, I'm working around this in my own project with the following monkey patch.

# This library is only used in test.
if Rails.env.test?
  class MockRedis
    module HashMethods
      private

      def with_hash_at(key, &blk)
        version_check
        with_thing_at(key.to_s, :assert_hashy, proc { {} }, &blk)
      end

      def version_check
        return unless Gem.loaded_specs['mock_redis'].version > Gem::Version.new('0.21.0')
        Rails.logger.warn 'Check if PR (https://github.com/sds/mock_redis/pull/173) is merged, and part of current ' \
                          'version.  If so, remove this code.'
      end
    end
  end
end

@sds
Copy link
Owner

sds commented Oct 10, 2019

Fixed in #173.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants