Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HttpStatus Code on GRAPHQL_VALIDATION error #967

Closed
gao-artur opened this issue Dec 8, 2022 · 7 comments · Fixed by #1142
Closed

HttpStatus Code on GRAPHQL_VALIDATION error #967

gao-artur opened this issue Dec 8, 2022 · 7 comments · Fixed by #1142
Labels
enhancement New feature or request question Further information is requested spec compliance The change is intended to solve the problem of inconsistency with the official GraphQL specification

Comments

@gao-artur
Copy link
Contributor

Not sure if it's a bug or a misunderstanding of the spec. I'm migrating from 5.1.1 to 7.2.0 and found that in some cases, the HttpStatus Code changed from 200 to 400. For example, when the required field is missing from the query, I'm getting now BadRequest (400)

{
  "errors": [
    {
      "message": "Argument 'testInput' has invalid value. Missing required field 'name' of type 'String'.",
      "locations": [
        {
          "line": 1,
          "column": 52
        }
      ],
      "extensions": {
        "code": "GRAPHQL_VALIDATION",
        "codes": [
          "ARGUMENTS_OF_CORRECT_TYPE"
        ],
        "number": "5.6.1",
      }
    }
  ]
}

The spec for application/json says:

Note: A status code in the 4xx or 5xx ranges or status code 203 (and maybe others) could originate from intermediary servers; since the client cannot determine if an application/json response with arbitrary status code is a well-formed GraphQL response (because it cannot trust the source) the server must use 200 status code to guarantee to the client that the response has not been generated or modified by an intermediary.

And also:

Note: For compatibility with legacy servers, this specification allows the use of 4xx or 5xx status codes for failed requests where the response uses the application/json media type, but it is strongly discouraged. To use 4xx and 5xx status codes, please use the application/graphql-response+json media type.

@Shane32
Copy link
Member

Shane32 commented Dec 8, 2022

The latest 7.2.0 follows the draft GraphQL over HTTP spec, which returns application/graphql-response+json and returns 400 in the case of a validation error.

If the GraphQL response does not contain the {data} entry then the server MUST reply with a 4xx or 5xx status code as appropriate.

See: https://github.com/graphql/graphql-over-http/blob/main/spec/GraphQLOverHTTP.md#applicationgraphql-responsejson

Both the response content type and status code response are configurable.

See the ValidationErrorsReturnBadRequest option and the DefaultResponseContentType properties of the options. For example:

app.UseGraphQL("/graphql", options =>
{
    options.DefaultResponseContentType = MediaTypeHeaderValueMs.Parse("application/json");
    options.ValidationErrorsReturnBadRequest = false;
});

Note that if the HTTP Accept header is provided to the request, the response will attempt to use a matching content type -- but the selection of the response content type does not alter the response status code. So in the default configuration, 400 errors may be returned with the application/json content type if the application/json content type was requested via the Accept header.

@sungam3r
Copy link
Member

sungam3r commented Dec 8, 2022

@gao-artur You provided irrelevant parts of the spec. The problem is not about intermediary servers or content type. The problem is the data itself. @Shane32 pointed in the right direction. Since v7 server returns 4xx if data entry is empty, i.e. execution was not started at all, that means some validation error or any other arbitrary server-side error occurred BEFORE actual execution of GraphQL query.

@sungam3r sungam3r added question Further information is requested spec compliance The change is intended to solve the problem of inconsistency with the official GraphQL specification labels Dec 8, 2022
@Shane32
Copy link
Member

Shane32 commented Dec 8, 2022

@sungam3r You may wish to read this section again https://github.com/graphql/graphql-over-http/blob/main/spec/GraphQLOverHTTP.md#applicationjson

@gao-artur is correct that 400 is "strongly discouraged" for well-formed GraphQL responses with the "application/json" content type.

This section only applies when the response body is to use the application/json media type.

The server SHOULD use the 200 status code, independent of any GraphQL request error or GraphQL field error raised.

We are catering to the application/graphql-response+json content type and although we vary the response content type based on the Accept header, we do not have code that sets the status code based on which response content type was selected.

@sungam3r
Copy link
Member

sungam3r commented Dec 8, 2022

OK, too many combinations to remember.

@gao-artur
Copy link
Contributor Author

Thanks, this explains the observed behavior and provided configuration indeed solves my issue. But I think this should be the default behavior for application/json as the other one is discouraged

@Shane32 Shane32 added the enhancement New feature or request label Dec 8, 2022
@Shane32
Copy link
Member

Shane32 commented Dec 8, 2022

We can look at adding this feature. (Specifically, that the response status code is dependent not only on the response but the selected content type.) It would likely have to be in the next major version since it probably requires a redesign of some of the protected methods of the middleware. I think ultimately we want to determine the response content type earlier in the middleware logic to allow for adjusting behavior throughout the pipeline, for example to support subscriptions over SSE.

@Shane32
Copy link
Member

Shane32 commented Aug 9, 2024

Done for v8. By default it will follow GraphQL over HTTP spec and return 200 for application/json responses when there are validation errors.

@Shane32 Shane32 closed this as completed Aug 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request question Further information is requested spec compliance The change is intended to solve the problem of inconsistency with the official GraphQL specification
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants