Avoid returning HTTP 500 response code

[glen-84]

Is there an existing issue for this?

• [X] I have searched the existing issues

Product

Hot Chocolate

Describe the bug

There are at least 2 cases where an HTTP 500 response code is being used where it should not be.

1. When a global ID is malformed (for example).
   • This seems like it should be part of validation, and instead return a 400 Bad Request.
2. When authn/z is unsuccessful.
   • According to the spec, this should return a 401 or 403 code, or perhaps a 200 for mixed responses where a data property is present.

   If the client is not permitted to issue the GraphQL request then the server SHOULD reply with 403, 401 or similar appropriate status code.

HTTP 500 errors are often logged and considered to be issues that need to be resolved, so it's far from ideal when this code is used for common responses.

Steps to reproduce

n/a

Relevant log output

No response

Additional Context?

Related issues: #2513 (notable comments: first, second) #3044 #5434 (my comment)

Version

13.8.1

[michaelstaib]

This seems like it should be part of validation, and instead return a 400 Bad Request.

This is wrong. Validation is the GraphQL validation as in the core spec. The actual coercion of the argument value is happening in the execution. The moment that something breaks in the execution we need to do a 200 as in this case data would become null we still implement an older draft that specified a 500 when data is null or does not exist.

We will change that soon to a 200.

[michaelstaib]

I am discussing 2 at the moment. In general I think this is meant for things that happen before the GraphQL execute. You could for instance still have partial data ... it would lead to inconsistent status codes .... this should also be a 200.

The moment execution has started the only outcome can be a 2xx.

[michaelstaib]

Just double checked with Benjie and the authentication is meant as I explained... this is also how we will implement it ... there will be no 500 once the execution started.

[glen-84]

This is wrong. Validation is the GraphQL validation as in the core spec. The actual coercion of the argument value is happening in the execution.

Oh, I thought that it would check the format during the validation phase, but I guess IDs are just strings, Relay IDs are a layer on top of that (outside of the spec)?

The moment that something breaks in the execution we need to do a 200 as in this case data would become null

Is this true even if there is just one operation in the request? I suppose it's more consistent to always return 200, instead of that depending on the number of operations.

---

To summarize:

• Expect to only see 4xx or 5xx codes before execution.
• Developers should not need to use workarounds like https://github.com/ChilliCream/graphql-platform/issues/5434#issuecomment-1306399587, especially once the 500 is changed to a 200. They should instead rely on checking the error code in the errors array.

[onionhammer]

Can we put together a sort-of cross tabulation chart on this to make it easier to understand what to expect?

i.e.

If a user has an [Authorize] attribute on a field, the status code would be

	Validation	BeforeResolver	AfterResolver
User not authenticated	?	?	?
User not authorized	?	?	?