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.

Sign up for GitHub

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

Fix default behavior for 401/403 http response codes #429

Merged
merged 4 commits into from
Aug 1, 2022
Merged

Fix default behavior for 401/403 http response codes #429

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
89ba50d
Fix default behavior for 401/403 http response codes
sungam3r Jul 26, 2022
b5f5f84
Add Accept and Accept-Charset headers
sungam3r Jul 31, 2022
491d21f
fix version
sungam3r Jul 31, 2022
fb107eb
add note
sungam3r Jul 31, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion src/GraphQL.Client/GraphQLHttpClientOptions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,8 +52,10 @@ public class GraphQLHttpClientOptions

/// <summary>
/// Delegate to determine if GraphQL response may be properly deserialized into <see cref="GraphQLResponse{T}"/>.
/// Note that compatible to the draft graphql-over-http spec GraphQL Server MAY return 4xx status codes (401/403, etc.)
/// with well-formed GraphQL response containing errors collection.
/// </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";
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :
изображение


/// <summary>
/// This callback is called after successfully establishing a websocket connection but before any regular request is made.
Expand Down
4 changes: 4 additions & 0 deletions src/GraphQL.Client/GraphQLHttpRequest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
using System.Net.Http.Headers;
using System.Runtime.Serialization;
using System.Text;
using GraphQL.Client.Abstractions;
Expand Down Expand Up @@ -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"));
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

message.Headers.Accept.Add(new MediaTypeWithQualityHeaderValue("application/json"));
message.Headers.AcceptCharset.Add(new StringWithQualityHeaderValue("utf-8"));

#pragma warning disable CS0618 // Type or member is obsolete
PreprocessHttpRequestMessage(message);
Expand Down
9 changes: 7 additions & 2 deletions tests/GraphQL.Integration.Tests/QueryAndMutationTests/Base.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
using System.Net;
using System.Net.Http.Headers;
using FluentAssertions;
using GraphQL.Client.Abstractions;
using GraphQL.Client.Http;
Expand Down Expand Up @@ -173,9 +174,13 @@ public async void PreprocessHttpRequestMessageIsCalled()
#pragma warning restore CS0618 // Type or member is obsolete
};

var defaultHeaders = StarWarsClient.HttpClient.DefaultRequestHeaders;
var expectedHeaders = new HttpRequestMessage().Headers;
expectedHeaders.UserAgent.Add(new ProductInfoHeaderValue("GraphQL.Client", "1.0.0.0"));
expectedHeaders.Accept.Add(new MediaTypeWithQualityHeaderValue("application/graphql+json"));
expectedHeaders.Accept.Add(new MediaTypeWithQualityHeaderValue("application/json"));
expectedHeaders.AcceptCharset.Add(new StringWithQualityHeaderValue("utf-8"));
var response = await StarWarsClient.SendQueryAsync(graphQLRequest, () => new { Human = new { Name = string.Empty } });
callbackTester.Should().HaveBeenInvokedWithPayload().Which.Headers.Should().BeEquivalentTo(defaultHeaders);
callbackTester.Should().HaveBeenInvokedWithPayload().Which.Headers.Should().BeEquivalentTo(expectedHeaders);
Assert.Null(response.Errors);
Assert.Equal("Luke", response.Data.Human.Name);
}
Expand Down