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

nil key should also be namespaced #106

Open
ghprince opened this issue Jul 7, 2015 · 4 comments
Open

nil key should also be namespaced #106

ghprince opened this issue Jul 7, 2015 · 4 comments

Comments

@ghprince
Copy link

ghprince commented Jul 7, 2015

currently nil key is treated as empty-string key without namespace. which is confusing.

redis = Redis.new
namespaced = Redis::Namespace.new(:ns)
namespaced.set nil, 'foo'
namespaced.set '', 'bar'
redis.keys # => ["", "ns:"]
@yaauie
Copy link
Member

yaauie commented Jul 7, 2015

The redis ruby adapter's documentation explicitly calls out that keys should be strings, so I'm very wary of defining behaviour here where it is undefined upstream:

  # Set the string value of a key.
  #
  # @param [String] key
  # @param [String] value
  # @param [Hash] options
  #   - `:ex => Fixnum`: Set the specified expire time, in seconds.
  #   - `:px => Fixnum`: Set the specified expire time, in milliseconds.
  #   - `:nx => true`: Only set the key if it does not already exist.
  #   - `:xx => true`: Only set the key if it already exist.
  # @return [String, Boolean] `"OK"` or true, false if `:nx => true` or `:xx => true`
  def set(key, value, options = {})

-- https://github.com/redis/redis-rb/blob/v3.2.1/lib/redis.rb#L654

@ghprince
Copy link
Author

ghprince commented Jul 8, 2015

I believe this is the line where redis-rb convert the key from nil to empty string. (it basically converts every command to string before sending to Redis because that's the only thing it recognize)
https://github.com/redis/redis-rb/blob/v3.2.1/lib/redis/connection/command_helper.rb#L18

# @param [String] key could also mean give me anything, I'll try to convert it to String, and if I can't, I'll raise an exception (that's quite a common behaviour for a dynamic typing language like Ruby)

for example, # @param [String] value says the value should be a String as well, and within that method, to_s is called on value.
https://github.com/redis/redis-rb/blob/v3.2.1/lib/redis.rb#L681

@yaauie
Copy link
Member

yaauie commented Jul 8, 2015

As I said earlier, I'm wary of defining behaviour based on the observed (but undefined) behaviour of private api's, but this behaviour is also surprising, which is why I left this ticket open. I'll circle back to it in the coming days, when I have time to consider all of the potential ramifications of your proposed fix (#107) and/or any alternates.


That said, I'd like to address a couple of your arguments:

# @param [String] key could also mean give me anything

No, it doesn't; in yardoc (which is how the redis ruby adapter gem documents itself), the way to require an object that responds to to_s would be # @param [#to_s] key.

for example, # @param [String] value says the value should be a String as well, and within that method, to_s is called on value.
https://github.com/redis/redis-rb/blob/v3.2.1/lib/redis.rb#L681

The documentation declares the contract of the public API for a library; in Semantic Versioning, the implementation details are free to change without a major version release (and thus an obvious warning to the consumers of a library), so long as the method behaves in the same manner from an external standpoint. The fact that version 3.2.1 of the redis ruby adapter sends #to_s to the value argument that its documentation already requires be a String is immaterial, and since the redis ruby adapter has no tests to validate the behaviour when a non-String key is given, the behaviour is undefined and can change out of under us without warning.

@ghprince
Copy link
Author

ghprince commented Jul 9, 2015

ah I see! Thanks a lot for such a great explanation! Very clear! 👍

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 a pull request may close this issue.

2 participants