-
Notifications
You must be signed in to change notification settings - Fork 134
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
Add APQ support #555
Add APQ support #555
Changes from 13 commits
9f842be
ed54915
57920cd
b25557c
058d2e6
cf7c7c0
d199fce
bbdf13c
5429f64
97312ee
77a6d7e
0b47264
2b77d59
4fadbff
ddc0268
3f50777
c8b6254
6be7d9c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,2 +1,3 @@ | ||
<wpf:ResourceDictionary xml:space="preserve" xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml" xmlns:s="clr-namespace:System;assembly=mscorlib" xmlns:ss="urn:shemas-jetbrains-com:settings-storage-xaml" xmlns:wpf="http://schemas.microsoft.com/winfx/2006/xaml/presentation"> | ||
<s:String x:Key="/Default/CodeStyle/Naming/CSharpNaming/Abbreviations/=APQ/@EntryIndexedValue">APQ</s:String> | ||
<s:String x:Key="/Default/CodeStyle/Naming/CSharpNaming/Abbreviations/=QL/@EntryIndexedValue">QL</s:String></wpf:ResourceDictionary> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,15 +1,34 @@ | ||
#if NET6_0_OR_GREATER | ||
using System.Diagnostics.CodeAnalysis; | ||
|
||
namespace GraphQL; | ||
|
||
/// <summary> | ||
/// Value record for a GraphQL query string | ||
/// Value object representing a GraphQL query string and storing the corresponding APQ hash. <br /> | ||
/// Use this to hold query strings you want to use more than once. | ||
/// </summary> | ||
/// <param name="Text">the actual query string</param> | ||
public readonly record struct GraphQLQuery([StringSyntax("GraphQL")] string Text) | ||
public class GraphQLQuery : IEquatable<GraphQLQuery> | ||
rose-a marked this conversation as resolved.
Show resolved
Hide resolved
|
||
{ | ||
/// <summary> | ||
/// The actual query string | ||
/// </summary> | ||
public string Text { get; } | ||
|
||
/// <summary> | ||
/// The SHA256 hash used for the automatic persisted queries feature (APQ) | ||
/// </summary> | ||
public string Sha256Hash { get; } | ||
|
||
public GraphQLQuery([StringSyntax("GraphQL")] string text) | ||
{ | ||
Text = text; | ||
Sha256Hash = Hash.Compute(Text); | ||
} | ||
|
||
public static implicit operator string(GraphQLQuery query) | ||
=> query.Text; | ||
}; | ||
#endif | ||
|
||
public bool Equals(GraphQLQuery other) => Sha256Hash == other.Sha256Hash; | ||
|
||
public override bool Equals(object? obj) => obj is GraphQLQuery other && Equals(other); | ||
|
||
public override int GetHashCode() => Sha256Hash.GetHashCode(); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,44 @@ | ||
using System.Buffers; | ||
using System.Diagnostics; | ||
using System.Security.Cryptography; | ||
using System.Text; | ||
|
||
namespace GraphQL; | ||
|
||
internal static class Hash | ||
{ | ||
private static SHA256? _sha256; | ||
|
||
internal static string Compute(string query) | ||
{ | ||
int expected = Encoding.UTF8.GetByteCount(query); | ||
byte[]? inputBytes = ArrayPool<byte>.Shared.Rent(expected); | ||
int written = Encoding.UTF8.GetBytes(query, 0, query.Length, inputBytes, 0); | ||
Comment on lines
+15
to
+16
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This could be rewritten to use Span / stackalloc also. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you please elaborate? 😉 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I saw that stackalloc was used on lines 21-27. Stackalloc could be used for the call to GetBytes as well. This is faster than ArrayPool. https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/operators/stackalloc The easy recommended pattern is to write code like this: int length = 1000;
Span<byte> buffer = length <= 1024 ? stackalloc byte[length] : new byte[length]; This assumes that typically the buffer length is relatively small, and in such cases, is ideal. It also includes a fallback for large buffer requirements; but then you'd probably lose the ArrayPool (?). And if it was known that the query would commonly be larger than you'd want to allocate on the stack, then maybe the cost of the heap allocation for the long queries might be worse than the benefits from stackalloc. Or you'd have to write a separate if block for the ArrayPool path vs not. I'm not really sure; maybe better off leaving it as-is. Another possible optimization would be to guess at the length, rather than using GetByteCount. You'd have to write a fallback routine in case the encoded string length was longer than the preallocated buffer though, and you might need to use the underlying Encoder rather than the static method. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think I'll leave it as it is. Using |
||
Debug.Assert(written == expected, (string)$"Encoding.UTF8.GetBytes returned unexpected bytes: {written} instead of {expected}"); | ||
|
||
var shaShared = Interlocked.Exchange(ref _sha256, null) ?? SHA256.Create(); | ||
|
||
#if NET5_0_OR_GREATER | ||
Span<byte> bytes = stackalloc byte[32]; | ||
if (!shaShared.TryComputeHash(inputBytes.AsSpan().Slice(0, written), bytes, out int bytesWritten)) // bytesWritten ignored since it is always 32 | ||
throw new InvalidOperationException("Too small buffer for hash"); | ||
#else | ||
byte[] bytes = shaShared.ComputeHash(inputBytes, 0, written); | ||
#endif | ||
|
||
ArrayPool<byte>.Shared.Return(inputBytes); | ||
Interlocked.CompareExchange(ref _sha256, shaShared, null); | ||
|
||
#if NET5_0_OR_GREATER | ||
return Convert.ToHexString(bytes); | ||
#else | ||
var builder = new StringBuilder(bytes.Length * 2); | ||
foreach (byte item in bytes) | ||
{ | ||
builder.Append(item.ToString("x2")); | ||
} | ||
|
||
return builder.ToString(); | ||
#endif | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This interface is needed to pass non-generic instance of response into
GraphQLHttpClientOptions.DisableAPQ
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you should put the content in there, too... then people can evaluate plaintext error messages...
Always having the raw content in there might be beneficial for general debugging, too, cause then people could cast their responses to
IGraphQLHttpResponse
and get the raw body of the response...There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then I would put entire
HttpResponseMessage
instead of pulling discrete properties from it. Also note that content may be already unaccessable since stream was read inSendHttpRequestAsync
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#559 , not a goal for this PR