Subscription messages contain both `data` and `errors`

[mihai-dinculescu]

Describe the bug
Successful messages contain an empty array errors property, while error messages contain a null data property.

{
  "data": {
    "users": {
      "name": "stream user"
    }
  },
  "errors": []
}

{
  "data": {
    "users": null
  },
  "errors": [
    {
      "message": "some field error from handler",
      "locations": [
        {
          "line": 2,
          "column": 3
        }
      ],
      "path": [
        "users"
      ],
      "extensions": "some additional string"
    }
  ]
}

To Reproduce
Run the warp_subscriptions example

Expected behavior
Succesful messages should contain only data. Error messages should contain only errors.

Additional context
Introduced by #721

[ccbrown]

This isn't (entirely) a bug. The result of executing a GraphQL operation is not binary "success" or "error". You can have data present and have errors. In fact, the spec requires that the data key is always present if execution occurs (as opposed to an error happening during parsing or validation, in which case a different message type is sent back altogether).

It probably makes sense to omit the empty errors array in your first example, and just emit this:

{
  "data": {
    "users": {
      "name": "stream user"
    }
  }
}

But your second example is correct, and removing the data key would violate the spec.

[ccbrown]

PR to omit empty errors from serialization: #732

The spec does say:

If no errors were encountered during the requested operation, the errors entry should not be present in the result.

It says "should" and not "must", so clients should be fine either way, but it's definitely worth changing.

[mihai-dinculescu]

PR to omit empty errors from serialization: #732

The spec does say:

If no errors were encountered during the requested operation, the errors entry should not be present in the result.

It says "should" and not "must", so clients should be fine either way, but it's definitely worth changing.

That looks like an inconsistency in the spec. Just above that, under Response Format, the spec takes a more prescriptive approach:

If the operation encountered any errors, the response map must contain an entry with key errors. The value of this entry is described in the “Errors” section. If the operation completed without encountering any errors, this entry must not be present.

[mihai-dinculescu]

But your second example is correct, and removing the data key would violate the spec.

The caveat here is if the operation started executing or not.

Data spec

If an error was encountered before execution begins, the data entry should not be present in the result.

Response Format spec is even more prescriptive

If the operation included execution, the response map must contain an entry with key data. The value of this entry is described in the “Data” section. If the operation failed before execution, due to a syntax error, missing information, or validation error, this entry must not be present.

[ccbrown]

The message type this issue is about is only used if execution has started. If execution has not started, an entirely different message type is used (which does not contain a "data" field). That's the difference between the graphql-ws "data" and "error" messages:

juniper/juniper_graphql_ws/src/server_message.rs

Lines 109 to 123 in 6788ef2

	Data {
	/// The id of the operation that the data is for.
	id: String,
	/// The data and errors that occurred during execution.
	payload: DataPayload<S>,
	},
	/// Error contains an error that occurs before execution, such as validation errors.
	Error {
	/// The id of the operation that triggered this error.
	id: String,
	/// The error(s).
	payload: ErrorPayload,
	},

[ccbrown]

Looks like you're right about the success case. That fix has been merged though, so I think we're now 100% spec compliant. Let me know if anything else looks off.

[mihai-dinculescu]

The message type this issue is about is only used if execution has started. If execution has not started, an entirely different message type is used (which does not contain a "data" field). That's the difference between the graphql-ws "data" and "error" messages:

juniper/juniper_graphql_ws/src/server_message.rs

Lines 109 to 123 in 6788ef2

	Data {
	/// The id of the operation that the data is for.
	id: String,
	/// The data and errors that occurred during execution.
	payload: DataPayload<S>,
	},
	/// Error contains an error that occurs before execution, such as validation errors.
	Error {
	/// The id of the operation that triggered this error.
	id: String,
	/// The error(s).
	payload: ErrorPayload,
	},

Right! I've got the two confused. All good!