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

Support empty operation name #34

Merged
merged 1 commit into from
Aug 3, 2023

Conversation

ilyasgaraev
Copy link
Contributor

Issue: an empty operation name causes the error in the redis-client library (dependency of Redis gem). Only relevant to Redis v5.

Reason:
Let's check the path from the graphql-anycable to redis-client:

  1. GraphQL::Subscriptions::AnyCableSubscriptions#write_subscription (the fixed method) builds data hash with the operation_name key;
  2. operation_name can be nil, because it's not a required attribute, and a subscription without a name is still valid;
  3. graphql-anycable passes the data hash via the mapped_hmset method;
  4. mapped_hmset sends HMSET command via redis_client;
  5. redis_client checks the attributes here and triggers the error because one of the attributes (operation_name) is nil.

Here you can find a reason why redis-client doesn't support nil.

The issue is reproducible with Redis v5, you can remove the fix and the following error will be triggered in spec:

GraphQL::AnyCable with empty operation name subscribes channel to stream updates from GraphQL subscription
     Failure/Error: pipeline.mapped_hmset(SUBSCRIPTION_PREFIX + subscription_id, data)
     
     TypeError:
       Unsupported command argument type: NilClass

@Envek Envek merged commit 2f3217e into anycable:master Aug 3, 2023
3 checks passed
@Envek
Copy link
Member

Envek commented Aug 3, 2023

Thanks a lot for the pull request and such a detailed explanation, appreciate it.

Released in 1.1.6, please enjoy!

Envek added a commit that referenced this pull request May 7, 2024
…dling

* master:
  Switch to RubyGems Trusted publishing in CI release workflow [ci skip]
  add emergency actions TO README
  fix: consider redis_prefix in GraphQL::AnyCable::Cleaner
  add ability to set scan_count and added a little refactoring
  ability to fetch subscription stats
  use "around" instead "before"
  little refactoring
  add changes to README
  remove override redis_prefix and add an extra spec
  feat: Add configurable graphql redis_prefix
  Support empty operation name with redis.rb v5 (#34)
  Use arrays to fix deprecation warnings with redis gem 4.8 (#29)
  Use real Redis in tests instead of fakeredis gem (#32)
  Better `config.use_client_provided_uniq_id` deprecation (#27)
  Delay the deprecation warning of use_client_provided_uniq_id (#26)
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 this pull request may close these issues.

2 participants