fix: make sendError payload format respect WebSocket subprotocol

[tsushanth]

Problem

sendError in lib/subscription-connection.js unconditionally wraps the error in an array:

this.sendMessage(this.protocolMessageTypes.GQL_ERROR, id, [convertedError])

This is correct for the graphql-transport-ws protocol (the newer spec), but breaks clients using the legacy subscriptions-transport-ws protocol. In mercurius/Fastify, that legacy protocol is identified by socket.protocol === 'graphql-ws' — and it expects a single error object as the payload, not an array.

Sending an array to a subscriptions-transport-ws client causes the client to fail to parse the error, surfacing as an unhandled error or silent failure rather than a proper GraphQL error.

Fix

The active subprotocol is already available on this.socket.protocol and is used elsewhere in the same class for protocol-branching logic. The fix is a one-line conditional:

const payload = this.socket.protocol === 'graphql-ws' ? convertedError : [convertedError]

• 'graphql-ws' → legacy subscriptions-transport-ws → single error object
• anything else (e.g. 'graphql-transport-ws') → newer spec → array

Closes #1132

[mcollina]

Thanks for opening a PR! Can you please add a unit test?

[tsushanth]

Added two unit tests in test/subscription-connection.test.js that verify sendError payload format per each protocol:

• graphql-ws (subscriptions-transport-ws): expects a plain error object
• graphql-transport-ws: expects errors wrapped in an array

Running the full test suite also surfaced three existing tests that were asserting the old (broken) array format for graphql-ws connections — those expectations have been corrected to match the spec as well (subscription.test.js, subscription-hooks.test.js, query-depth.test.js).

[tsushanth]

Hi @mcollina — the unit tests you asked for are in test/subscription-connection.test.js, added in the latest commit. Two tests cover sendError payload format per protocol:

• sendError sends a plain object for graphql-ws protocol (subscriptions-transport-ws)
• sendError wraps error in array for graphql-transport-ws protocol

Also updated three existing tests in subscription.test.js, subscription-hooks.test.js, and query-depth.test.js that were asserting the old (incorrect) array-wrapped format for graphql-ws, so the full test suite passes. Let me know if you'd like anything adjusted!

[mcollina]

lgtm

[leorossi]

The spec direction here looks right to me — subscriptions-transport-ws does want a single error object, and graphql-transport-ws wants the array. But I think the protocol check needs to use the resolved protocol rather than the raw one, or the change lands inconsistently.

socket.protocol is the subprotocol the client actually negotiated. Mercurius elsewhere resolves that against wsDefaultSubprotocol:

• subscription-connection.js:51 — this.protocolMessageTypes = getProtocolByName(socket.protocol, this.wsDefaultSubprotocol)
• subscription-protocol.js:39 — isValidClientProtocol() explicitly accepts an empty client protocol when a default is configured

So on a server configured with wsDefaultSubprotocol: 'graphql-ws', a client that omits Sec-WebSocket-Protocol is handled as legacy graphql-ws for every message type — start, data, ka — but with this patch still receives the array error shape, while a client that sends the header receives the object. Same server, same protocol semantics, two payload shapes.

I applied the diff to 16.9.0 and drove two clients against one server (wsDefaultSubprotocol: 'graphql-ws'), triggering an error via an unknown message type:

• client sending Sec-WebSocket-Protocol: graphql-ws → payload: { ... }
• client sending no subprotocol → payload: [{ ... }]

Minimal fix, since the signals object is already exported:

const { GRAPHQL_WS_PROTOCOL_SIGNALS } = require('./subscription-protocol')

sendError (err, id) {
  const convertedError = toGraphQLError(err)
  const payload = this.protocolMessageTypes === GRAPHQL_WS_PROTOCOL_SIGNALS
    ? convertedError
    : [convertedError]
  this.sendMessage(this.protocolMessageTypes.GQL_ERROR, id, payload)
}

That reuses the resolution already done in the constructor, so it stays correct however wsDefaultSubprotocol is configured. Might be worth a test case for the header-less client too, since the existing ones all set protocol explicitly.

Separately on versioning: this changes an observable wire format for clients that have been parsing payload[0] against every released version so far, so it reads as breaking to me. Would you consider landing it behind an option in 16.x with the current behaviour as the default, and flipping the default in 17? That would let downstreams move client and server independently rather than atomically.

[leorossi]

@tsushanth this is a test that shows the edge case

test('sendError uses one payload shape regardless of how the protocol was resolved', async (t) => {
  const app = fastify()
  t.after(() => app.close())

  app.register(mercurius, {
    schema: `
      type Query {
        _placeholder: String
      }

      type Subscription {
        onMessage: String!
      }
    `,
    resolvers: {
      Query: {},
      Subscription: {
        onMessage: {
          async * subscribe () {
            yield { onMessage: 'hello' }
          }
        }
      }
    },
    subscription: {
      wsDefaultSubprotocol: GRAPHQL_WS
    }
  })

  await app.listen({ port: 0 })
  const url = 'ws://localhost:' + (app.server.address()).port + '/graphql'

  // An unrecognised message type reaches the default branch of handleMessage(),
  // which calls sendError() directly. Every other sendError call site behaves
  // the same way.
  async function errorPayload (subprotocol) {
    const ws = subprotocol ? new WebSocket(url, subprotocol) : new WebSocket(url)

    ws.on('error', (error) => {
      t.assert.fail('must not error ' + error)
    })

    await once(ws, 'open')
    ws.send(JSON.stringify({ type: 'connection_init' }))
    ws.send(JSON.stringify({ type: 'not-a-real-message-type', id: 1 }))

    let payload
    for await (const [message] of on(ws, 'message')) {
      const data = JSON.parse(message.toString())
      if (data.type === 'error') {
        payload = data.payload
        break
      }
    }

    ws.close()
    return payload
  }

  const negotiated = await errorPayload(GRAPHQL_WS)
  const defaulted = await errorPayload()

  t.assert.strictEqual(
    Array.isArray(negotiated),
    Array.isArray(defaulted),
    'a client that omits the subprotocol is resolvultSubprotocol, so it must receive the same error payload shape as one that negotiates it explicitly'
  )
})

Add it tosubscription-connection.test.js and that would be green both on main and with the proposed fix

[tsushanth]

Fixed in the latest push — switched from this.socket.protocol === 'graphql-ws' to this.protocolMessageTypes === GRAPHQL_WS_PROTOCOL_SIGNALS so the resolved default subprotocol is used consistently.

Also added your exact test case (the one you posted above) to test/subscription-connection.test.js — it was failing on main and passes with the fix.

[mcollina]

lgtm

[tsushanth]

Friendly ping — this has been approved by @mcollina for a week now and CI is green. Happy to rebase or make any changes if needed before merge.

[mcollina]

This is semver-major. I'll land it whenever we cut a new major.