3
\$\begingroup\$

I have some code to perform a redis lookup, but if the key to look up either doesn't exist or there's an issue connecting to Redis at all, it will use a default value.

The logic currently looks like this:

begin
 redis = Redis.new(:url => url)
 returned_value = redis.get(key)
 if returned_value == nil and defined?(default) != nil
   default
 else
   returned_value
 end
rescue Exception => e
  if default
    debug "Connection to redis failed with #{e} - Returning default value of #{default}"
    default
  else
    raise(Puppet::Error, "connection to redis server failed - #{e}")
  end
end

It feels like there might be a simpler way of doing this, right now this code feels a little wordy.

Any suggestions?

\$\endgroup\$
2
\$\begingroup\$

First of all NEVER rescue ExceptionError. If you want a to rescue from any runtime exception, just omit exception class, otherwise you might catch SignalException which happens upon ruby process received some signal (e.g. SIGTERM) and instead of behave correctly you will silently skip that.

So, your rescue part should look actually like this:

begin
  # ...
rescue SocketError, Redis::CannotConnectError => e
  raise Puppet::Error, "connection to redis server failed - #{e}" unless default
  debug "Connection to redis failed with #{e} - Returning default value of #{default}"
  default
end

Now your main block can be also improved:

begin
  Redis.new(:url => url).get(key) || default
rescue
  # ...
end

Notice that redis returns either String or Nil, e.g.:

Redis.current.set(:x, 123)
Redis.current.get(:x) # => "123"

Redis.current.set(:x, false)
Redis.current.get(:x) # => "false"

So, summing all above together, you can form that into a method:

def redis_get(key, url:, default: nil)
  Redis.new(:url => url).get(key) || default
rescue SocketError, Redis::CannotConnectError => e
  raise Puppet::Error, "connection to redis server failed - #{e}" unless default
  debug "Connection to redis failed with #{e}; Return default: #{default}"
  default
end
\$\endgroup\$
  • \$\begingroup\$ Fantastic, that looks a lot cleaner. However my spec failed with a test where the redis server connection could not be found, so I added SocketError to the rescue then everything was green. I added that as an edit to the answer, other wise thanks for your fix, I'll keep this in mind next time :) \$\endgroup\$ – Peter Souter Jun 22 '17 at 12:25
  • \$\begingroup\$ Yeah, it's totally fine to add SocketError as well. :D \$\endgroup\$ – Aleksey V Zapparov Jun 26 '17 at 19:47
  • \$\begingroup\$ Sorry, I saw that I can simply approve too late. Anyway. One note to your edit - there were no need in begin block. You can (and in this case should) rescue method's body... No need for extra level of depth. And for the sake of history, one might want to rescue SocketError as it's got raised when socket can't get ip address. e.g. when you specify :url => "redis://surprise" and surprise can't be resolved. So it's not actually redis connection issue, it' general networking issue. \$\endgroup\$ – Aleksey V Zapparov Jun 26 '17 at 19:56
  • \$\begingroup\$ Cool, works perfectly, accepted! :) \$\endgroup\$ – Peter Souter Jul 11 '17 at 18:30

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.