Clients are receiving a subscription response even with context.skip

[bbugh]

Describe the bug

Client callbacks for subscriptions are being called for non-returning subscription methods, even when context.skip is used.

Versions

graphql version: 1.11.5
rails: 6.0.3.2
graphql-batch: 0.4.3
graphql-fragment_cache: 1.0.3
actioncable: 5.2.4-4

GraphQL schema

Include relevant types and fields (in Ruby is best, in GraphQL IDL is ok).
Are you using interpreter? Any custom instrumentation, etc?

class GraphSchema < GraphQL::Schema
  use GraphQL::Execution::Interpreter
  use GraphQL::Analysis::AST
  use GraphQL::Subscriptions::ActionCableSubscriptions
end

class Types::SubscriptionType < Types::BaseObject
  field :project_added, Types::ProjectType, "XXX", null: false # or true, neither work
  field :notification, Types::NotificationType, "XXX", null: true, subscription_scope: :current_user

  def project_added
    if context.query.subscription_update?
      object
    else
      context.skip
    end
  end

  def notification
    context.skip
  end
end

GraphQL query

The ActionCable websocket request:

{
  "command": "message",
  "identifier": "{\"channel\":\"GraphqlChannel\",\"channelId\":\"1750efbc531\"}",
  "data": "{\"query\":\"subscription projectAdded {\\n  projectAdded {\\n    id\\n    name\\n    clientName\\n    status\\n    __typename\\n  }\\n}\\n\",\"variables\":{},\"operationName\":\"name\",\"action\":\"execute\"}"
}

The query from data (no variables):

subscription projectAdded {
  projectAdded {
    id
    name
    clientName
    status
    __typename
  }
}

The ActionCable websocket response

{
  "identifier": "{\"channel\":\"GraphqlChannel\",\"channelId\":\"1750efbc531\"}",
  "message": {
    "result": {
      "data": {}
    },
    "more": true
  }
}

Steps to reproduce

Not sure other than the provided code.

Expected behavior

The context.skip should prevent the client from receiving and acting on a message.

Actual behavior

The client is receiving the message and running code that it should not be running. For example, the notification subscription tries to run the code for when a notification subscription is received, but no data arrives:

// NotificationListener.vue
        query: NOTIFICATION_SUBSCRIPTION,
        result (data) {
          console.log('notification listener', data)

Console output, data is empty here. Previously, this would not be called when the subscription was first active.

notification listener  {…}
                         data: {}
​    					   <prototype>: Object { … }
​						 <prototype>: Object { … }

Additional context

This seems to be the same issue as #3180, but the fix there didn't work. I believe it's related to the breaking change in #2536.

[bbugh]

Removing the method doesn't work either. This still results in the same issue:

  field :notification, Types::NotificationType, "Sent a notification", null: true, subscription_scope: :current_user

  # def notification
  #   context.skip
  # end

[rmosolgo]

Thanks for those details. I misunderstood what you meant by empty response. I think skip is working as expected -- the field that returned context.skip was not present in the response. (The response was an empty object, {}.)

Were you previously using Execution::Interpreter, or did adding that plugin cause this issue? I ask because there's a difference between how the two runtimes handle empty subscription responses. (For example, in the tests:

graphql-ruby/spec/graphql/subscriptions_spec.rb

Line 286 in af617b9

empty_response = TESTING_INTERPRETER ? {} : nil

)

I'm not sure I want to change the empty response from {} to nil. I just haven't heard of other issues with that, and I think it makes more sense to work like this.

If it turns out it's a compatibility issue from switching to Interpreter, and there's really no way to handle it on the client, you could hack a fix in your server, something like:

result = MySchema.execute(...)
if result.query.subscription? && !result.query.subscription_update? 
 # don't transmit anything, the client expects no response 
else 
  transmit(result)
end 

Does that help at all?

[bbugh]

Thanks for the reply! We were not previously using the interpreter. The only change we made to result in this was upgrading to 1.11.5 from 1.9.10, and then adding the two lines for the AST and the new Interpreter:

+  use GraphQL::Execution::Interpreter
+  use GraphQL::Analysis::AST

If I can make the argument, I think that returning an empty object for data is not the right approach to signaling "no response".

For example, {} is truthy in JavaScript and JS doesn't have an empty? check for objects. Most things will check for data's truthiness, which will always be true because {} is truthy in JS and Ruby and other languages. Short of saying "does this contain any keys?" there's no way to check if the data is empty or not.

If I remember correctly from the spec, data is not required - although I think sending data: null would be better than an empty string, at least any data checks on a result object would correctly demonstrate that there was no data.

For example, your ActionCableLink code checks whether data is truthy - which it always will be because the server returns a truthy result to data:

graphql-ruby/javascript_client/src/subscriptions/ActionCableLink.ts

Lines 44 to 47 in af617b9

	received: function(payload) {
	if (payload.result.data || payload.result.errors) {
	observer.next(payload.result)
	}

That code could be the part of our issue here, since we're using ActionCableLink, but I don't think it's the full picture.

While we could hack it, I think this is actually a design issue from #2536.

[tienle]

We have faced the same issue with the initial response is being returned as data: {} . It takes us a while to figure it out this unexpected behavior. I think it should rather return nothing or null than {}.

[choznerol]

I think it should rather return nothing or null than {}.

Agree. I also thought :no_update means transmitting nothing at all.

Take our case, for example, we are following the client setup Apollo 2 – ActionCable and the current behavior will be like below:

const Example = () => {
  const { loading, error, data } = useSubscription(THE_NUMBER)

  console.log(JSON.stringify(loading, error, data))
  // { loading: true, data: null }               // 1st render, UI shows 'Loading...' 👍
  // { loading: false, data: {} }                // 2nd render, UI shows nothing 😳
  // { loading: false, data: { theNumber: 42 }   // 3rd render, UI shows 42 👍

  return loading ? <div>Loading...</div> : <pre{data.theNumber}</pre>;
}

I'm guessing this is a real-world example for the above-mentioned problem of {} being truthy in JS.

For example, your ActionCableLink code checks whether data is truthy - which it always will be because the server returns a truthy result to data:

graphql-ruby/javascript_client/src/subscriptions/ActionCableLink.ts

Lines 44 to 47 in af617b9

	received: function(payload) {
	if (payload.result.data || payload.result.errors) {
	observer.next(payload.result)
	}

[jqr]

I believe the underlying issue, "when skip/NO_UPDATE is used don't send any message over ActionCable" is fixed by #3713.

If I have it right, the skip behavior that Subscriptions implements is not used by ActionCableSubscriptions due to its own implementation complexities. The fix was just to just port that trivial behavior into ActionCableSubscriptions.

[rmosolgo]

#3713 is released in 1.13.0 -- thanks @jqr! Please open a new issue if you find this issue on that version.