Shopify / Shopify/semian

Redis adapter causes errors with Redis::Cluster due to monkey-patching

Open Beginner friendly
#1,036 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
Ruby
Stars
1.5k
Forks
88
Avg merge
3d 19h
Merged PRs (30d)
5

Description

Semian::RedisV5Client#translate_error! overrides Redis::Client.translate_error! via prepend but doesn't preserve the original method signature. This might cause an ArgumentError when used alongside the redis-clustering gem.

This can happen even if you do not pass the semian argument to a Redis::Cluster class

The original signature:

      def translate_error!(error, mapping: ERROR_MAPPING)
        redis_error = translate_error_class(error.class, mapping: mapping)
        raise redis_error, error.message, error.backtrace
      end

Semian's patch:

https://github.com/Shopify/semian/blob/568d0736a5a64a262bea0f9c50508f91132a0ef5/lib/semian/redis/v5.rb#L54-L70

Now when Redis::Cluster::Client calls its translate_error!, it would delegate to Redis::Client:

        def translate_error!(error, mapping: ERROR_MAPPING)
          case error
          when RedisClient::Cluster::ErrorCollection
            # nothing in particular
          else
            Redis::Client.translate_error!(error, mapping: mapping) # <---- DELEGATES HERE
          end
        end

But the Semian's patch breaks the API, which causes a ArgumentError: wrong number of arguments (given 2, expected 1).

Tested with:
semian 0.27.1 (latest)
redis 5.4.1 (latest)
redis-clustering 5.4.1 (latest)
redis-client 0.26.4 (latest)

But likely it's been an issue for longer.

Contributor guide

No contributing guide indexed for this repository

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with lib/semian/redis/v5.rb, especially Semian::RedisV5Client#translate_error!, and compare its method signature with Redis::Client.translate_error! and the Redis::Cluster delegation described in the issue. Verify the adapter remains compatible with the mapping keyword and reproduce the reported ArgumentError using the listed Redis and redis-clustering versions.

Written by the indexing model from the issue text.

Assessment

Tech stack
redis, ruby
Domain
backend
Issue type
Bug
Difficulty
2/5
Estimated time
1-3 hours
Activity status
Stale
Clarity
Clearly specified
Newbie friendliness
64/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.