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

From redis-rb 5.0.0, sadd and srem always return integers #281

Closed
mikevoets opened this issue Oct 15, 2023 · 2 comments · Fixed by #314
Closed

From redis-rb 5.0.0, sadd and srem always return integers #281

mikevoets opened this issue Oct 15, 2023 · 2 comments · Fixed by #314
Labels
bug contributions-welcome Contributions from anyone are welcome

Comments

@mikevoets
Copy link

In this gem it still returns true or false. This has changed from redis-rb version 5, see https://github.com/redis/redis-rb/blob/master/CHANGELOG.md.

As a workaround, I started using sadd? and srem? to align the behaviour of this gem and redis-rb.

@sds sds added bug contributions-welcome Contributions from anyone are welcome labels Nov 2, 2023
@CoderMiguel
Copy link

I started a fork to work on this last night and realized the extent of this issue is pretty deep. There were a lot of changes in the upgrade to version 5 that will have a lot of impact on this gem. That I think ultimately needs to be a larger conversation than just fixing this bug.

One of the biggest questions would be does this gem want to maintain backwards compatibility with version 4 or not.
Some others would include, is fixing this bug alone enough or would all of the methods effected need to be handled before the changes will be committed?

In the meantime, may I offer the following as a workaround for those that are using >= version 5:

class MockRedis
  module SetMethods
    def sadd(key, members)
      # members_class = members.class
      members = Array(members).map(&:to_s)
      assert_has_args(members, 'sadd')

      with_set_at(key) do |s|
        size_before = s.size
        if members.size > 1
          members.reverse_each { |m| s << m }
          s.size - size_before
        else
          added = !!s.add?(members.first)
          # if members_class == Array
            s.size - size_before
          # else
            # added
          # end
        end
      end
    end

    def srem(key, members)
      with_set_at(key) do |s|
        if members.is_a?(Array)
          orig_size = s.size
          members = members.map(&:to_s)
          s.delete_if { |m| members.include?(m) }
          orig_size - s.size
        else
          # !!s.delete?(members.to_s)
          s.delete?(members.to_s) ? 1 : 0
        end
      end
    end
  end
end

Comments about the above suggestion

If the specs are updated to expect the new output. This passes the bulk of the tests in the gem, all failures for the srem and sadd specs are from breaking changes in version 5. All but one of the failures coming from it_should_behave_like 'a set-only command':

    let(:redis_gem_v5?) { Redis::VERSION.to_i == 5 }
    let(:positive_response) { redis_gem_v5? ? 1 : true }
    let(:negative_response) { redis_gem_v5? ? 0 : false }
    
    it 'returns positive response if member is in the set' do
      expect(@redises.srem(@key, 'bert')).to eq(positive_response)
    end

    it 'returns negative response if member is not in the set' do
      expect(@redises.srem(@key, 'cookiemonster')).to eq(negative_response)
    end

@sds
Copy link
Owner

sds commented Jan 10, 2024

Thanks for the research and providing a workaround!

I'm supportive of dropping support for older Redis gem versions as part of fixing in the project itself. At some point the ecosystem needs to move forward.

@Palladinium Palladinium mentioned this issue Nov 14, 2024
@sds sds closed this as completed in #314 Nov 27, 2024
sds pushed a commit that referenced this issue Nov 27, 2024
This is an attempt to fix #281.
This PR was also tested using the test suite of our service.

There are a few decisions I made regarding this PR, please let me know
if it's not appropriate:

1. Error from Redis now contains the redis connection URL
- To fix this, I created `MockRedis::Error` module which decorate
`Redis::CommandError` and related errors with the connection URL
- I replace most of the `Redis::CommandError` with the new module
wrapper
2. Some methods now return integer as output instead of boolean and
`exists_returns_integer` is removed
  - I replace the output as is
3. Interfaces of most data structures (Hash, Set, ...) now validate type
to be 4 main primitive types
- `assert_type` is added to validate the arguments. Currently it doesn't
look very clean, so any improvements would be great.
4. `sadd`, `srem` now support multiple arguments
5. Transaction: `multi` now needs to call with a block and
`discard`/`exec` cannot be called outside that block anymore
- I tried to refactor the logic to make the tests pass, but I'm not
entirely confident that it works correctly
sds pushed a commit that referenced this issue Nov 27, 2024
Currently failing due to needing redis-rb v5 (see also #281 and #293)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug contributions-welcome Contributions from anyone are welcome
Projects
None yet
3 participants