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

SPUBLISH not working for Reactive/Coroutines client when using pubsub connection #2971

Closed
jkrauze opened this issue Aug 22, 2024 · 4 comments

Comments

@jkrauze
Copy link
Contributor

jkrauze commented Aug 22, 2024

Bug Report

https://github.com/redis/lettuce/pull/2838/files#r1727231070

As per comment I left on that PR, the spublish() command in RedisPubSubReactiveCommandsImpl points to publish command instead of spublish.

This causes spublish() command to point to a wrong underlying command when using a pubsub connection (initiated with .connectPubSub() method).

Please note that the spublish() works fine when we use a regular connection.

Current Behavior

redisClient.connectPubSub().coroutines().spublish("test", "test")

this calls PUBLISH test test

redisClient.connect().coroutines().spublish("test", "test")

this calls SPUBLISH test test

Expected behavior/code

both should call SPUBLISH test test

Environment

  • Lettuce version(s): 6.4.0.RELEASE
  • Redis version: 7.0

Possible Solution

correct the typo

Additional context

This was referenced Aug 22, 2024
@jkrauze
Copy link
Contributor Author

jkrauze commented Aug 22, 2024

opened a PR fixing this typo

@jkrauze jkrauze changed the title SPUBLISH not working for Reactive/Coroutines client SPUBLISH not working for Reactive/Coroutines client when using pubsub connection Aug 22, 2024
@tishun
Copy link
Collaborator

tishun commented Aug 25, 2024

Wondering why the integration tests have not caught this issue. Please, give me a little time to verify that.

tishun pushed a commit that referenced this issue Aug 25, 2024
@tishun
Copy link
Collaborator

tishun commented Aug 26, 2024

Wondering why the integration tests have not caught this issue. Please, give me a little time to verify that.

Well the answer is we did not have tests for that :/

tishun pushed a commit to tishun/lettuce-core that referenced this issue Aug 26, 2024
tishun added a commit that referenced this issue Aug 26, 2024
@tishun
Copy link
Collaborator

tishun commented Aug 26, 2024

@jkrauze thank you for the contribution!

@tishun tishun closed this as completed Aug 26, 2024
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

No branches or pull requests

2 participants