Fix default behavior for 401/403 http response codes #429
Conversation
| /// </summary> | ||
| public Func<HttpResponseMessage, bool> IsValidResponseToDeserialize { get; set; } = r => r.IsSuccessStatusCode || r.StatusCode == HttpStatusCode.BadRequest; | ||
| public Func<HttpResponseMessage, bool> IsValidResponseToDeserialize { get; set; } = r => r.IsSuccessStatusCode || r.StatusCode == HttpStatusCode.BadRequest || r.Content.Headers.ContentType.MediaType == "application/graphql+json"; |
There was a problem hiding this comment.
This tries to parse all responses in which the server manages to set the content type header to application/graphql+json (including 5xx responses). Are u sure this is a "bulletproof" assumption?
I think we should at least catch parsing exceptions in GraphQLHttpClient and wrap those up in a GraphQLHttpRequestException to pass the raw response content to the user (this probably won't hurt, anyway).
There was a problem hiding this comment.
If content type is application/graphql+json then the response can be deserialized as a valid graphql response.
Regarding 5xx error codes from server - see https://github.com/graphql/graphql-over-http/blob/main/spec/GraphQLOverHTTP.md#applicationgraphqljson :
|
What's missing, too, is that this client doesn't set the Accept request header as defined in the spec draft, so the server will only respond with We should set it to |
|
OK, will do tomorrow. |
| @@ -38,6 +39,9 @@ public virtual HttpRequestMessage ToHttpRequestMessage(GraphQLHttpClientOptions | |||
| { | |||
| Content = new StringContent(serializer.SerializeToString(this), Encoding.UTF8, options.MediaType) | |||
| }; | |||
| message.Headers.Accept.Add(new MediaTypeWithQualityHeaderValue("application/graphql+json")); | |||
There was a problem hiding this comment.
MediaTypeWithQualityHeaderValue does not accept application/graphql+json; charset=utf-8. It throws System.FormatException : The format of value 'application/graphql+json; charset=utf-8' is invalid. I added Accept-Charset header that should work the same.
This PR aligns behavior with server project that returns 401/403.