Cannot stringify invalid message; isMessage validation for execution result is too strict

[n1ru4l]

A stream payload published by an AsyncIterable returned by execute (from [email protected]) cannot be stringified

{
  id: '7aef51de-707e-49e5-8cbf-df1bff572e5f',
  type: 'next',
  payload: { data: 'Friend', path: [ 'streamTest', 2 ], hasNext: true }
}

It fails silently with the following message, I had to dig into node_modules and add a bunch of console.logs before I had access to this message.

Error: Cannot stringify invalid message
    at Object.stringifyMessage (/Users/laurin/projects/stream-defer-demo/node_modules/graphql-ws/cjs/message.js:87:15)

It would be nice if such an error would not be swallowed by default but instead forwarded to the console. The user could then overwrite the default behavior.

Aside from that, this message object (returned from execute) should be accepted as a valid message. I don't understand why exactly the message sent by the server must be validated - as the server constructs it should be fine? Also, the current validation seems to be rather inflexible and must be updated every time graphql supports a new execution result variation. The responsibility for returning a correct result should be within execute.

Reference code:

type Query {
  greetings: [String]
}

query StreamTestQuery {
  streamTest @stream(initialCount: 1)
}

const Query = new GraphQLObjectType({
  name: "Query",
  fields: {
    streamTest: {
      type: GraphQLList(GraphQLString),
      resolve: async function* () {
        for (const item of ["Hi", "My", "Friend"]) {
          yield item;
          await sleep(1000);
        }
      },
    },
})

[enisdenjo]

Nothing should be failing silently. Are you sure the socket is not closed with the error in the close event reason? Keep in mind that the client will retry a few times if the connection gets closed (you might not see the error immediately).

If its indeed failing silently, please make a test or a repro and I will be more than happy to look at it and bring up a fix! (Please make an additional issue)

Furthermore, the server validates messages because they should follow the Protocol's shapes. I'd say the validation for the next message is very forgiving - it requires the data or the errors field to be an object in the payload:

graphql-ws/src/message.ts

Lines 120 to 127 in 0d2f974

	case MessageType.Next:
	return (
	hasOwnStringProperty(val, 'id') &&
	hasOwnObjectProperty(val, 'payload') &&
	// ExecutionResult
	(hasOwnObjectProperty(val.payload, 'data') ||
	hasOwnObjectProperty(val.payload, 'errors'))
	);

Message you provided is failing because the data field is a string. How does a query that gives you just a string in the data object look like? (just curious 😄)

It does make sense to support whatever for the data field - fix is coming!

[n1ru4l]

The example above shows such a query, with the schema and Query Object type 😅

You are right, the client connection gets closed and it will retry connecting. However, since this is a error in general related to the server, I would actually also expect to see that issue logged to the console.

The close reason for socket is also not detailed enough imho. like is the message sent by the client invalid or the one sent back and what exactly is invalid? For a nice DX A more detailed message could be returned.

I would propose to not explicitly check whether payload has a data or errors property. That seems to be an implementation detail of execute to me

[enisdenjo]

You can hook up onto the closed event for the client and log all socket closes.

I check just the basics, the simplest form of graphql-js's ExecuteResult (any data or object errors). The message has a super simple form, so it should be rather easy to detect why the message is malformed - hence vague error.

I plan on making the client retry connections on some errors only - classify the fatal ones and report them immediately.

Furthermore, all of server-side error handling issues will go away as soon as #64 lands. You will have full control of error handling and reporting then. Once this lands, tracking if the malformed issue originates from the back or front should be easy. Will keep your insights in mind though!

[n1ru4l]

defer and stream AsyncIterables returned from execute will emit a final { hasNext: false } execution result. See src/execution/tests/stream-test.js here: graphql/graphql-js@dec5554#diff-a370dc23e95e90e2b04fe67a3a933e6eb09296c64b4eae7509cd3ae93edaf2ae

[enisdenjo]

Oh! Well, thats enough for me. I'll just make it so that the validation makes sure that the payload is present. You can pass whatever you want.

[enisdenjo]

There, you can now pass whatever you want for the payload. 😄

[n1ru4l]

Awesome 👏 Thanks for considering this!

[enisdenjo]

Thank you for bringing it up!

[enisdenjo]

BTW, the client now fails immediately for some fatal close event codes. This should improve the DX by bringing a problem up sooner.

[enisdenjo]

🎉 This issue has been resolved in version 1.13.1 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀