GraphQLTransportWS: Always return an id upon errors #1845
Conversation
|
|
From what I can tell connection errors do not include the id in the old spec |
|
@SvenW the error you referred to in the new spec is a response to the 
This is the equivalent of I can't seem to find anywhere how an error due to connection is meant to look like though in the new spec |
|
According to the spec, it seems that we need to use HTTP codes to reject
https://github.com/enisdenjo/graphql-ws/blob/master/PROTOCOL.md#connectioninit |
Yeah, you're right. We need to handle the authentication errors for the connection_init events as well with the correct code when closing the socket. However from what I can tell the |
|
@SvenW I agree, although I don't think this will solve the linked issue. Or have you tested and it does solve it? |
Well it solves it but not completely according to the spec. Let me see if I can find some time during the day and solve the unauthenticated part as well |
… close the socket with 4403 on error
From what I can tell graphql-transport-ws always closes the socket on |
SvenW
changed the title
| override def error[E](id: Option[String], e: E): GraphQLWSOutput = | ||
| override def error[E](id: Option[String], e: E): GraphQLWSOutput = { | ||
| val outputId = id match { | ||
| case None => Some(UUID.randomUUID().toString) |
There was a problem hiding this comment.
Nit: It would be better to use the zio.Random construct, though you'll then need to push the id generation up where you will still be in ZIO land
There was a problem hiding this comment.
@SvenW can you just update that one? Then it's good to go 🙏
I think as long as we're within the spec and it works that's fine. Have you tested whether this solves the original issue? |
I've tested it with the AuthExampleApp and it seems to behave as expected, both with error messages and with the connection_init event with authorization errors. |
SvenW
force-pushed
the
fix/graphql-transport-ws-init
branch
from
fa8d11c to
8af118f
Compare
Fixes #1844