From b34dcee26ee027fff11764af618ebf1f88eeea0d Mon Sep 17 00:00:00 2001 From: Michael Staib Date: Thu, 19 Nov 2020 23:23:10 +0100 Subject: [PATCH] Do not return operation context to pool when tasks are still running. (#2616) --- .../InternalServiceCollectionExtensions.cs | 6 +- .../Processing/OperationContext.Pooling.cs | 2 + .../Processing/OperationContextPoolPolicy.cs | 14 +- .../src/Execution/Processing/ResolverTask.cs | 3 +- .../test/Execution.Tests/CodeFirstTests.cs | 30 ++- ...FieldsWithDifferentCasingAreNotMerged.snap | 23 ++ .../Language.SyntaxTree/StringValueNode.cs | 18 +- .../AST/StringValueNodeTests.cs | 255 ++++++++++++++++++ 8 files changed, 327 insertions(+), 24 deletions(-) create mode 100644 src/HotChocolate/Core/test/Execution.Tests/__snapshots__/CodeFirstTests.EnsureThatFieldsWithDifferentCasingAreNotMerged.snap create mode 100644 src/HotChocolate/Language/test/Language.Tests/AST/StringValueNodeTests.cs diff --git a/src/HotChocolate/Core/src/Execution/DependencyInjection/InternalServiceCollectionExtensions.cs b/src/HotChocolate/Core/src/Execution/DependencyInjection/InternalServiceCollectionExtensions.cs index fd9d868dc85..86f6800edfd 100644 --- a/src/HotChocolate/Core/src/Execution/DependencyInjection/InternalServiceCollectionExtensions.cs +++ b/src/HotChocolate/Core/src/Execution/DependencyInjection/InternalServiceCollectionExtensions.cs @@ -99,9 +99,9 @@ internal static class InternalServiceCollectionExtensions this IServiceCollection services) { services.TryAddSingleton( - sp => new DefaultDocumentCache()); + _ => new DefaultDocumentCache()); services.TryAddSingleton( - sp => new DefaultPreparedOperationCache()); + _ => new DefaultPreparedOperationCache()); return services; } @@ -109,7 +109,7 @@ internal static class InternalServiceCollectionExtensions this IServiceCollection services) { services.TryAddSingleton( - sp => new MD5DocumentHashProvider()); + _ => new MD5DocumentHashProvider()); return services; } diff --git a/src/HotChocolate/Core/src/Execution/Processing/OperationContext.Pooling.cs b/src/HotChocolate/Core/src/Execution/Processing/OperationContext.Pooling.cs index b48078af8e5..4c8c46ae889 100644 --- a/src/HotChocolate/Core/src/Execution/Processing/OperationContext.Pooling.cs +++ b/src/HotChocolate/Core/src/Execution/Processing/OperationContext.Pooling.cs @@ -24,6 +24,8 @@ internal sealed partial class OperationContext _resultHelper = new ResultHelper(resultPool); } + public bool IsPooled => _isPooled; + public void Initialize( IRequestContext requestContext, IServiceProvider scopedServices, diff --git a/src/HotChocolate/Core/src/Execution/Processing/OperationContextPoolPolicy.cs b/src/HotChocolate/Core/src/Execution/Processing/OperationContextPoolPolicy.cs index 03057629ac9..e508cdc663a 100644 --- a/src/HotChocolate/Core/src/Execution/Processing/OperationContextPoolPolicy.cs +++ b/src/HotChocolate/Core/src/Execution/Processing/OperationContextPoolPolicy.cs @@ -17,8 +17,18 @@ public OperationContextPoolPolicy(Func factory) public bool Return(OperationContext obj) { - obj.Clean(); - return true; + if (obj.IsPooled) + { + return true; + } + + if (obj.Execution.TaskStats.IsCompleted) + { + obj.Clean(); + return true; + } + + return false; } } } diff --git a/src/HotChocolate/Core/src/Execution/Processing/ResolverTask.cs b/src/HotChocolate/Core/src/Execution/Processing/ResolverTask.cs index 14edd7fdcee..5be89291b79 100644 --- a/src/HotChocolate/Core/src/Execution/Processing/ResolverTask.cs +++ b/src/HotChocolate/Core/src/Execution/Processing/ResolverTask.cs @@ -53,8 +53,9 @@ private async ValueTask TryExecuteAndCompleteAsync(CancellationToken cancellatio else { _operationContext.Execution.TaskStats.TaskCompleted(); - _operationContext.Execution.TaskPool.Return(this); } + + _operationContext.Execution.TaskPool.Return(this); } } diff --git a/src/HotChocolate/Core/test/Execution.Tests/CodeFirstTests.cs b/src/HotChocolate/Core/test/Execution.Tests/CodeFirstTests.cs index 5edaae69930..fd84a58c1d5 100644 --- a/src/HotChocolate/Core/test/Execution.Tests/CodeFirstTests.cs +++ b/src/HotChocolate/Core/test/Execution.Tests/CodeFirstTests.cs @@ -268,12 +268,22 @@ public async Task ExecuteExplicitAsyncField() [Fact] public async Task CannotCreateRootValue() { - IExecutionResult result = - await new ServiceCollection() - .AddGraphQL() - .AddQueryType() - .ExecuteRequestAsync("{ hello }") - .MatchSnapshotAsync(); + await new ServiceCollection() + .AddGraphQL() + .AddQueryType() + .ExecuteRequestAsync("{ hello }") + .MatchSnapshotAsync(); + } + + // https://github.com/ChilliCream/hotchocolate/issues/2617 + [Fact] + public async Task EnsureThatFieldsWithDifferentCasingAreNotMerged() + { + await new ServiceCollection() + .AddGraphQL() + .AddQueryType() + .BuildSchemaAsync() + .MatchSnapshotAsync(); } private static Schema CreateSchema() @@ -515,5 +525,13 @@ private QueryPrivateConstructor() public string Hello() => "Hello"; } + + public class QueryFieldCasing + { + public string YourFieldName { get; set; } + + [GraphQLDeprecated("This is deprecated")] + public string YourFieldname { get; set; } + } } } diff --git a/src/HotChocolate/Core/test/Execution.Tests/__snapshots__/CodeFirstTests.EnsureThatFieldsWithDifferentCasingAreNotMerged.snap b/src/HotChocolate/Core/test/Execution.Tests/__snapshots__/CodeFirstTests.EnsureThatFieldsWithDifferentCasingAreNotMerged.snap new file mode 100644 index 00000000000..31fd1492374 --- /dev/null +++ b/src/HotChocolate/Core/test/Execution.Tests/__snapshots__/CodeFirstTests.EnsureThatFieldsWithDifferentCasingAreNotMerged.snap @@ -0,0 +1,23 @@ +schema { + query: QueryFieldCasing +} + +type QueryFieldCasing { + yourFieldName: String + yourFieldname: String @deprecated(reason: "This is deprecated") +} + +"The `@defer` directive may be provided for fragment spreads and inline fragments to inform the executor to delay the execution of the current fragment to indicate deprioritization of the current fragment. A query with `@defer` directive will cause the request to potentially return multiple responses, where non-deferred data is delivered in the initial response and data deferred is delivered in a subsequent response. `@include` and `@skip` take precedence over `@defer`." +directive @defer("If this argument label has a value other than null, it will be passed on to the result of this defer directive. This label is intended to give client applications a way to identify to which fragment a deferred result belongs to." label: String "Deferred when true." if: Boolean) on FRAGMENT_SPREAD | INLINE_FRAGMENT + +"The @deprecated directive is used within the type system definition language to indicate deprecated portions of a GraphQL service’s schema,such as deprecated fields on a type or deprecated enum values." +directive @deprecated("Deprecations include a reason for why it is deprecated, which is formatted using Markdown syntax (as specified by CommonMark)." reason: String = "No longer supported") on FIELD_DEFINITION | ENUM_VALUE + +"Directs the executor to include this field or fragment only when the `if` argument is true." +directive @include("Included when true." if: Boolean!) on FIELD | FRAGMENT_SPREAD | INLINE_FRAGMENT + +"Directs the executor to skip this field or fragment when the `if` argument is true." +directive @skip("Skipped when true." if: Boolean!) on FIELD | FRAGMENT_SPREAD | INLINE_FRAGMENT + +"The `@stream` directive may be provided for a field of `List` type so that the backend can leverage technology such as asynchronous iterators to provide a partial list in the initial response, and additional list items in subsequent responses. `@include` and `@skip` take precedence over `@stream`." +directive @stream("If this argument label has a value other than null, it will be passed on to the result of this stream directive. This label is intended to give client applications a way to identify to which fragment a streamed result belongs to." label: String "The initial elements that shall be send down to the consumer." initialCount: Int! "Streamed when true." if: Boolean!) on FIELD diff --git a/src/HotChocolate/Language/src/Language.SyntaxTree/StringValueNode.cs b/src/HotChocolate/Language/src/Language.SyntaxTree/StringValueNode.cs index bd10e1baacc..39f7cc31932 100644 --- a/src/HotChocolate/Language/src/Language.SyntaxTree/StringValueNode.cs +++ b/src/HotChocolate/Language/src/Language.SyntaxTree/StringValueNode.cs @@ -233,19 +233,13 @@ public ReadOnlySpan AsSpan() return _memory.Span; } - public StringValueNode WithLocation(Location? location) - { - return new(location, Value, Block); - } + public StringValueNode WithLocation(Location? location) => + new(location, Value, Block); - public StringValueNode WithValue(string value) - { - return new(Location, value, false); - } + public StringValueNode WithValue(string value) => + new(Location, value, false); - public StringValueNode WithValue(string value, bool block) - { - return new(Location, value, block); - } + public StringValueNode WithValue(string value, bool block) => + new(Location, value, block); } } diff --git a/src/HotChocolate/Language/test/Language.Tests/AST/StringValueNodeTests.cs b/src/HotChocolate/Language/test/Language.Tests/AST/StringValueNodeTests.cs new file mode 100644 index 00000000000..81b1dd373d3 --- /dev/null +++ b/src/HotChocolate/Language/test/Language.Tests/AST/StringValueNodeTests.cs @@ -0,0 +1,255 @@ +using System.Text; +using Xunit; + +namespace HotChocolate.Language +{ + public class StringValueNodeTests + { + [Fact] + public void Create_StringValueNode_1() + { + // arrange + // act + var value = new StringValueNode("abc"); + + // assert + Assert.Equal("abc", value.Value); + Assert.False(value.Block); + Assert.Equal(SyntaxKind.StringValue, value.Kind); + Assert.Null(value.Location); + Assert.Empty(value.GetNodes()); + } + + [Fact] + public void Create_StringValueNode_2_Location_Is_Null() + { + // arrange + // act + var value = new StringValueNode(null, "abc", true); + + // assert + Assert.Equal("abc", value.Value); + Assert.True(value.Block); + Assert.Equal(SyntaxKind.StringValue, value.Kind); + Assert.Null(value.Location); + Assert.Empty(value.GetNodes()); + } + + [Fact] + public void Create_StringValueNode_2_With_Location() + { + // arrange + var location = new Location(1, 1, 1, 1); + + // act + var value = new StringValueNode(location, "abc", true); + + // assert + Assert.Equal("abc", value.Value); + Assert.True(value.Block); + Assert.Equal(SyntaxKind.StringValue, value.Kind); + Assert.Same(location, value.Location); + Assert.Empty(value.GetNodes()); + } + + [Fact] + public void Create_StringValueNode_3_Location_Is_Null() + { + // arrange + var stringValue = Encoding.UTF8.GetBytes("abc"); + + // act + var value = new StringValueNode(null, stringValue, true); + + // assert + Assert.Equal("abc", value.Value); + Assert.True(value.Block); + Assert.Equal(SyntaxKind.StringValue, value.Kind); + Assert.Null(value.Location); + Assert.Empty(value.GetNodes()); + } + + [Fact] + public void Create_StringValueNode_3_With_Location() + { + // arrange + var location = new Location(1, 1, 1, 1); + var stringValue = Encoding.UTF8.GetBytes("abc"); + + // act + var value = new StringValueNode(location, stringValue, true); + + // assert + Assert.Equal("abc", value.Value); + Assert.True(value.Block); + Assert.Equal(SyntaxKind.StringValue, value.Kind); + Assert.Same(location, value.Location); + Assert.Empty(value.GetNodes()); + } + + [Fact] + public void StringValueNode_Equals_To_Null() + { + // arrange + var location = new Location(1, 1, 1, 1); + var stringValue = Encoding.UTF8.GetBytes("abc"); + var value = new StringValueNode(location, stringValue, true); + + // act + var equals = value.Equals(null); + + // assert + Assert.False(equals); + } + + [Fact] + public void StringValueNode_Equals_To_Same() + { + // arrange + var location = new Location(1, 1, 1, 1); + var stringValue = Encoding.UTF8.GetBytes("abc"); + var value = new StringValueNode(location, stringValue, true); + + // act + var equals = value.Equals(value); + + // assert + Assert.True(equals); + } + + [InlineData("abc", true)] + [InlineData("def", false)] + [Theory] + public void StringValueNode_Equals_To_Other(string value, bool expected) + { + // arrange + var location = new Location(1, 1, 1, 1); + var value1 = new StringValueNode(location, "abc", true); + var value2 = new StringValueNode(location, value, true); + + // act + var equals = value1.Equals(value2); + + // assert + Assert.Equal(expected, equals); + } + + [Fact] + public void ValueNode_Equals_To_Null() + { + // arrange + var location = new Location(1, 1, 1, 1); + var stringValue = Encoding.UTF8.GetBytes("abc"); + var value = new StringValueNode(location, stringValue, true); + + // act + var equals = value.Equals((IValueNode)null); + + // assert + Assert.False(equals); + } + + [Fact] + public void ValueNode_Equals_To_Same() + { + // arrange + var location = new Location(1, 1, 1, 1); + var stringValue = Encoding.UTF8.GetBytes("abc"); + var value = new StringValueNode(location, stringValue, true); + + // act + var equals = value.Equals((IValueNode)value); + + // assert + Assert.True(equals); + } + + [InlineData("abc", true)] + [InlineData("def", false)] + [Theory] + public void ValueNode_Equals_To_Other(string value, bool expected) + { + // arrange + var location = new Location(1, 1, 1, 1); + var value1 = new StringValueNode(location, "abc", true); + var value2 = new StringValueNode(location, value, true); + + // act + var equals = value1.Equals((IValueNode)value2); + + // assert + Assert.Equal(expected, equals); + } + + [Fact] + public void ValueNode_Equals_To_Int() + { + // arrange + var location = new Location(1, 1, 1, 1); + var stringValue = Encoding.UTF8.GetBytes("abc"); + var value = new StringValueNode(location, stringValue, true); + + // act + var equals = value.Equals(new IntValueNode(123)); + + // assert + Assert.False(equals); + } + + [Fact] + public void WithLocation() + { + // arrange + var location = new Location(1, 1, 1, 1); + var value = new StringValueNode("abc"); + + // act + value = value.WithLocation(location); + + + // assert + Assert.Equal("abc", value.Value); + Assert.False(value.Block); + Assert.Equal(SyntaxKind.StringValue, value.Kind); + Assert.Same(location, value.Location); + Assert.Empty(value.GetNodes()); + } + + [Fact] + public void WithValue_1() + { + // arrange + var value = new StringValueNode("abc"); + + // act + value = value.WithValue("def"); + + + // assert + Assert.Equal("def", value.Value); + Assert.False(value.Block); + Assert.Equal(SyntaxKind.StringValue, value.Kind); + Assert.Null(value.Location); + Assert.Empty(value.GetNodes()); + } + + [Fact] + public void WithValue_2() + { + // arrange + var value = new StringValueNode("abc"); + Assert.False(value.Block); + + // act + value = value.WithValue("def", true); + + + // assert + Assert.Equal("def", value.Value); + Assert.True(value.Block); + Assert.Equal(SyntaxKind.StringValue, value.Kind); + Assert.Null(value.Location); + Assert.Empty(value.GetNodes()); + } + } +}