Skip to content

Commit

Permalink
Fix for middleware injection optimization
Browse files Browse the repository at this point in the history
  • Loading branch information
benmccallum committed Oct 19, 2021
1 parent 22a0d62 commit b455544
Show file tree
Hide file tree
Showing 7 changed files with 67 additions and 42 deletions.
2 changes: 1 addition & 1 deletion src/Directory.Build.props
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
<PackageIconUrl>https://github.com/benmccallum/fairybread/raw/master/logo-400x400.png</PackageIconUrl>
<PackageLicenseUrl>https://github.com/benmccallum/fairybread/blob/master/LICENSE</PackageLicenseUrl>

<Version>7.1.0</Version>
<Version>7.1.1</Version>

<HotChocolateVersion>11.0.9</HotChocolateVersion>

Expand Down
2 changes: 2 additions & 0 deletions src/FairyBread.Tests/FairyBread.Tests.csproj
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@
<ItemGroup>
<PackageReference Include="FluentValidation.DependencyInjectionExtensions" Version="10.0.0" />
<PackageReference Include="HotChocolate.Execution" Version="$(HotChocolateVersion)" />
<PackageReference Include="HotChocolate.Types.CursorPagination" Version="$(HotChocolateVersion)" />
<PackageReference Include="HotChocolate.Data" Version="$(HotChocolateVersion)" />
<PackageReference Include="Microsoft.NET.Test.Sdk" Version="16.10.0" />
<PackageReference Include="Moq" Version="4.16.1" />
<PackageReference Include="Shouldly" Version="4.0.3" />
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,13 @@
listArgA: 0, 0,
listArgB: 0,0,
listArgC: hello,
listOfListArgC: hello
listOfListArgC: hello,
filterSortAndPagingArgs: {
nodes: [
{
a: A
}
]
}
}
}
27 changes: 20 additions & 7 deletions src/FairyBread.Tests/ValidationMiddlewareInjectorTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
using System.Threading.Tasks;
using FluentValidation;
using HotChocolate;
using HotChocolate.Data;
using HotChocolate.Execution;
using HotChocolate.Types;
using Microsoft.Extensions.DependencyInjection;
Expand Down Expand Up @@ -39,6 +40,8 @@ private static async Task<IRequestExecutor> GetRequestExecutorAsync(
.AddGraphQL()
.AddQueryType<QueryIType>()
.AddType<TestInputType>()
.AddSorting()
.AddFiltering()
.AddFairyBread(options =>
{
configureOptions?.Invoke(options);
Expand All @@ -63,7 +66,7 @@ public async Task Works(bool registerValidators)
options.ThrowIfNoValidatorsFound = false;
}
});

var query = "query { " +
"noArgs " +
"scalarArgsA(a: 0, b: false) " +
Expand All @@ -80,6 +83,7 @@ public async Task Works(bool registerValidators)
"listArgB(items: [0, 0]) " +
"listArgC(items: [0, 0]) " +
"listOfListArgC(items: [[0, 0], [0, 0]]) " +
"filterSortAndPagingArgs(first: 10) { nodes { a } }" +
"}";

// Act
Expand All @@ -106,6 +110,11 @@ public class QueryI
public string ArrayArgA(int?[] items) => string.Join(", ", items);

public string ListArgA(List<int?> items) => string.Join(", ", items);

[UsePaging]
[UseFiltering]
[UseSorting]
public IEnumerable<FooI> GetFilterSortAndPagingArgs() => new FooI[] { new FooI() };
}

public class QueryIType
Expand All @@ -125,21 +134,21 @@ protected override void Configure(IObjectTypeDescriptor<QueryI> descriptor)
.Argument("a", arg => arg.Type<NonNullType<IntType>>())
.Argument("b", arg => arg.Type<NonNullType<BooleanType>>())
.Type<StringType>()
.Resolver(ctx => "hello");
.Resolve(ctx => "hello");

descriptor
.Field("nullableScalarArgsB")
.Argument("a", arg => arg.Type<IntType>())
.Argument("b", arg => arg.Type<BooleanType>())
.Type<StringType>()
.ResolveWith<QueryIType>(x => x.NullableScalarArgsBResolver(default, default));;
.ResolveWith<QueryIType>(x => x.NullableScalarArgsBResolver(default, default)); ;

descriptor
.Field("nullableScalarArgsC")
.Argument("a", arg => arg.Type<IntType>())
.Argument("b", arg => arg.Type<BooleanType>())
.Type<StringType>()
.Resolver(ctx => "hello");
.Resolve(ctx => "hello");

descriptor
.Field("objectArgB")
Expand All @@ -151,7 +160,7 @@ protected override void Configure(IObjectTypeDescriptor<QueryI> descriptor)
.Field("objectArgC")
.Argument("input", arg => arg.Type<TestInputType>())
.Type<StringType>()
.Resolver(ctx => "hello");
.Resolve(ctx => "hello");

descriptor
.Field("listArgB")
Expand All @@ -163,13 +172,13 @@ protected override void Configure(IObjectTypeDescriptor<QueryI> descriptor)
.Field("listArgC")
.Argument("items", arg => arg.Type<NonNullType<ListType<IntType>>>())
.Type<StringType>()
.Resolver(ctx => "hello");
.Resolve(ctx => "hello");

descriptor
.Field("listOfListArgC")
.Argument("items", arg => arg.Type<NonNullType<ListType<NonNullType<ListType<IntType>>>>>())
.Type<StringType>()
.Resolver(ctx => "hello");
.Resolve(ctx => "hello");
}

public string ScalarArgsBResolver(int a, bool b) => $"{a} | {b}";
Expand All @@ -178,6 +187,10 @@ protected override void Configure(IObjectTypeDescriptor<QueryI> descriptor)
public string ListArgResolver(List<int> items) => string.Join(",", items);
}

public class FooI
{
public string A { get; set; } = "A";
}

public class TestInput
{
Expand Down
3 changes: 0 additions & 3 deletions src/FairyBread/DefaultFairyBreadOptions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,6 @@ public class DefaultFairyBreadOptions : IFairyBreadOptions
/// <inheritdoc/>
public bool OptimizeMiddlewarePlacement { get; set; } = true;

/// <inheritdoc/>
public bool ThrowIfArgumentRuntimeTypeCouldNotBeDeterminedWhileOptimizingMiddlewarePlacement { get; set; } = true;

/// <inheritdoc/>
public Func<ObjectTypeDefinition, ObjectFieldDefinition, ArgumentDefinition, bool> ShouldValidateArgument { get; set; }
= (o, f, a) => true;
Expand Down
10 changes: 0 additions & 10 deletions src/FairyBread/IFairyBreadOptions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -19,16 +19,6 @@ public interface IFairyBreadOptions
/// </summary>
bool OptimizeMiddlewarePlacement { get; }

/// <summary>
/// During middleware placement, if for some reason FairyBread can't determine the argument's
/// runtime type, if this option is true, FairyBread will throw. This is mostly to
/// help find gaps in the optimization process. If you encounter an exception, you can turn
/// this off as FairyBread will optimize where it can still and the problem argument will just
/// always get the validation middleware. Or, less ideal, you can turn off optimization entirely
/// with <see cref="OptimizeMiddlewarePlacement"/>. In either case, please repor the issue in GitHub.
/// </summary>
bool ThrowIfArgumentRuntimeTypeCouldNotBeDeterminedWhileOptimizingMiddlewarePlacement { get; }

/// <summary>
/// A function that evaluates an argument during schema building.
/// If it returns true, validation will occur on this argument at runtime, else it won't.
Expand Down
56 changes: 36 additions & 20 deletions src/FairyBread/ValidationMiddlewareInjector.cs
Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,6 @@ internal class ValidationMiddlewareInjector : TypeInterceptor
{
private FieldMiddleware? _validationFieldMiddleware;

private static readonly string _runtimeTypeCrawlErrorMsg =
"FairyBread could not determine runtime type of field argument to decide whether " +
"or not to add the validation middleware. If you encounter this exception, set the " +
$"{nameof(IFairyBreadOptions.ThrowIfArgumentRuntimeTypeCouldNotBeDeterminedWhileOptimizingMiddlewarePlacement)} " +
$"option to false and report the issue on GitHub. FairyBread will still optimize middleware placement where it can.";

public override void OnBeforeCompleteType(
ITypeCompletionContext completionContext,
DefinitionBase? definition,
Expand Down Expand Up @@ -49,18 +43,13 @@ public override void OnBeforeCompleteType(
continue;
}

// 3. the arg actually has a validator for it's runtime type
// 3. the arg actually has a validator for its runtime type
// (note: if we can't figure out the runtime type, doesn't add)
if (options.OptimizeMiddlewarePlacement)
{
var argRuntimeType = TryGetArgRuntimeType(argDef);
if (argRuntimeType is null)
{
if (options.ThrowIfArgumentRuntimeTypeCouldNotBeDeterminedWhileOptimizingMiddlewarePlacement)
{
throw new Exception(_runtimeTypeCrawlErrorMsg);
}
}
else if (!validatorRegistry.Cache.ContainsKey(argRuntimeType))
var argRuntimeType = TryGetArgRuntimeType(objTypeDef, fieldDef, argDef);
if (argRuntimeType is null ||
!validatorRegistry.Cache.ContainsKey(argRuntimeType))
{
continue;
}
Expand All @@ -83,16 +72,36 @@ public override void OnBeforeCompleteType(
}
}

private static Type? TryGetArgRuntimeType(ArgumentDefinition argDef)
private static Type? TryGetArgRuntimeType(
ObjectTypeDefinition objTypeDef,
ObjectFieldDefinition fieldDef,
ArgumentDefinition argDef)
{
if (argDef.Parameter?.ParameterType is { } argRuntimeType)
{
return argRuntimeType;
}

//if (argDef.Type is SyntaxTypeReference)
//{
// return null;
//}

if (argDef.Type is ExtendedTypeReference extTypeRef)
{
return TryGetRuntimeType(extTypeRef.Type);
try
{
return TryGetRuntimeType(extTypeRef.Type);
}
catch (Exception ex)
{
throw new Exception(
$"Problem getting runtime type for argument '{argDef.Name}' " +
$"in field '{fieldDef.Name}' on object type '{objTypeDef.Name}'. " +
$"Disable {nameof(IFairyBreadOptions.OptimizeMiddlewarePlacement)} in " +
$"options and report the issue on GitHub.",
ex);
}
}

return null;
Expand Down Expand Up @@ -139,11 +148,17 @@ public override void OnBeforeCompleteType(
{
var currBaseType = extType.Type.BaseType;
while (currBaseType is not null &&
currBaseType.BaseType != typeof(InputObjectType))
(!currBaseType.IsGenericType ||
currBaseType.GetGenericTypeDefinition() != typeof(InputObjectType<>)))
{
currBaseType = currBaseType.BaseType;
}

if (currBaseType is null)
{
return null;
}

return currBaseType!.GenericTypeArguments[0];
}

Expand All @@ -152,7 +167,8 @@ public override void OnBeforeCompleteType(
{
var currBaseType = extType.Type.BaseType;
while (currBaseType is not null &&
currBaseType.BaseType != typeof(ScalarType))
(!currBaseType.IsGenericType ||
currBaseType.GetGenericTypeDefinition() != typeof(ScalarType<>)))
{
currBaseType = currBaseType.BaseType;
}
Expand Down

0 comments on commit b455544

Please sign in to comment.