fix: handle non-null in arg parser (#18)

Federated `ArgumentParser` is used to map entity representation to method arguments. When unwrapping nested fields, current logic was not accounting to nullability of the underlying field. If the underlying field returned non-null object, parser would fail trying to find the target property on `NonNull` GraphQL wrapper (which obviously doesn't exist). Updated argument parser object handling logic to unwrap non-null types. Note: this PR only addresses the issue with non-null types. Similar bug is present when handling keys with list representations. Resolves: #6
apollographql · Oct 17, 2023 · 2bf6ab6 · 2bf6ab6
1 parent 1362cb6
commit 2bf6ab6
Show file tree

Hide file tree

Showing 5 changed files with 27 additions and 142 deletions.
diff --git a/compatibility/Types/Product.cs b/compatibility/Types/Product.cs
@@ -43,23 +43,22 @@ public Product(string id, string? sku, string? package, ProductVariation? variat
     public static Product? GetProductById(
         string id,
         Data repository)
-        => repository.Products.FirstOrDefault(t => t.Id.Equals(id));
+        => repository.Products.FirstOrDefault(t => id.Equals(t.Id));
 
     [ReferenceResolver]
     public static Product? GetProductByPackage(
         string sku,
         string package,
         Data repository)
         => repository.Products.FirstOrDefault(
-            t => (t.Sku?.Equals(sku) ?? false) &&
-                (t.Package?.Equals(package) ?? false));
+            t => sku.Equals(t.Sku) && package.Equals(t.Package));
 
     [ReferenceResolver]
     public static Product? GetProductByVariation(
         string sku,
         [Map("variation.id")] string variationId,
         Data repository)
         => repository.Products.FirstOrDefault(
-            t => (t.Sku?.Equals(sku) ?? false) &&
-                (t.Variation?.Id.Equals(variationId) ?? false));
+            t => sku.Equals(t.Sku) && variationId.Equals(t.Variation?.Id)
+        );
 }
diff --git a/compatibility/Types/ProductResearch.cs b/compatibility/Types/ProductResearch.cs
@@ -20,7 +20,6 @@ public ProductResearch(CaseStudy study, string? outcome)
         [Map("study.caseNumber")] string caseNumber,
         Data repository)
     {
-        Console.WriteLine("case number = {0}", caseNumber);
         return repository.ProductResearches.FirstOrDefault(
             r => r.Study.CaseNumber.Equals(caseNumber));
     }

diff --git a/src/Federation/Descriptors/EntityResolverDescriptor.cs b/src/Federation/Descriptors/EntityResolverDescriptor.cs
@@ -65,29 +65,6 @@ private void OnCompleteDefinition(ObjectTypeDefinition definition)
 
     protected override EntityResolverDefinition Definition { get; set; } = new();
 
-    /// <inheritdoc cref="IEntityResolverDescriptor"/>
-    public IObjectTypeDescriptor ResolveReference(
-        FieldResolverDelegate fieldResolver)
-        => ResolveReference(fieldResolver, Array.Empty<string[]>());
-
-    private IObjectTypeDescriptor ResolveReference(
-        FieldResolverDelegate fieldResolver,
-        IReadOnlyList<string[]> required)
-    {
-        if (fieldResolver is null)
-        {
-            throw new ArgumentNullException(nameof(fieldResolver));
-        }
-
-        if (required is null)
-        {
-            throw new ArgumentNullException(nameof(required));
-        }
-
-        Definition.ResolverDefinition = new(fieldResolver, required);
-        return _typeDescriptor;
-    }
-
     /// <inheritdoc cref="IEntityResolverDescriptor{T}"/>
     public IObjectTypeDescriptor ResolveReferenceWith(
         Expression<Func<TEntity, object?>> method)
@@ -114,13 +91,6 @@ public IObjectTypeDescriptor ResolveReferenceWith<TResolver>(
             nameof(member));
     }
 
-    /// <inheritdoc cref="IEntityResolverDescriptor"/>
-    public IObjectTypeDescriptor ResolveReferenceWith<TResolver>()
-        => ResolveReferenceWith(
-            Context.TypeInspector.GetNodeResolverMethod(
-                Definition.EntityType ?? typeof(TResolver),
-                typeof(TResolver))!);
-
     /// <inheritdoc cref="IEntityResolverDescriptor"/>
     public IObjectTypeDescriptor ResolveReferenceWith(MethodInfo method)
     {
@@ -141,10 +111,21 @@ public IObjectTypeDescriptor ResolveReferenceWith(MethodInfo method)
         return ResolveReference(resolver.Resolver!, argumentBuilder.Required);
     }
 
-    /// <inheritdoc cref="IEntityResolverDescriptor"/>
-    public IObjectTypeDescriptor ResolveReferenceWith(Type type)
-        => ResolveReferenceWith(
-            Context.TypeInspector.GetNodeResolverMethod(
-                Definition.EntityType ?? type,
-                type)!);
+    private IObjectTypeDescriptor ResolveReference(
+        FieldResolverDelegate fieldResolver,
+        IReadOnlyList<string[]> required)
+    {
+        if (fieldResolver is null)
+        {
+            throw new ArgumentNullException(nameof(fieldResolver));
+        }
+
+        if (required is null)
+        {
+            throw new ArgumentNullException(nameof(required));
+        }
+
+        Definition.ResolverDefinition = new(fieldResolver, required);
+        return _typeDescriptor;
+    }
 }
diff --git a/src/Federation/Descriptors/IEntityResolverDescriptor.cs b/src/Federation/Descriptors/IEntityResolverDescriptor.cs
@@ -9,44 +9,6 @@ namespace ApolloGraphQL.HotChocolate.Federation.Descriptors;
 /// </summary>
 public interface IEntityResolverDescriptor
 {
-    /// <summary>
-    /// Resolve an entity from its representation.
-    /// </summary>
-    /// <param name="fieldResolver">
-    /// The resolver.
-    /// </param>
-    /// <returns>
-    /// Returns the descriptor for configuration chaining.
-    /// </returns>
-    IObjectTypeDescriptor ResolveReference(
-        FieldResolverDelegate fieldResolver);
-
-    /// <summary>
-    /// Resolve an entity from its representation.
-    /// </summary>
-    /// <param name="method">
-    /// The reference resolver selector.
-    /// </param>
-    /// <typeparam name="TResolver">
-    /// The type on which the reference resolver is located.
-    /// </typeparam>
-    /// <returns>
-    /// Returns the descriptor for configuration chaining.
-    /// </returns>
-    IObjectTypeDescriptor ResolveReferenceWith<TResolver>(
-        Expression<Func<TResolver, object?>> method);
-
-    /// <summary>
-    /// Resolve an entity from its representation.
-    /// </summary>
-    /// <typeparam name="TResolver">
-    /// The type on which the reference resolver is located.
-    /// </typeparam>
-    /// <returns>
-    /// Returns the descriptor for configuration chaining.
-    /// </returns>
-    IObjectTypeDescriptor ResolveReferenceWith<TResolver>();
-
     /// <summary>
     /// Resolve an entity from its representation.
     /// </summary>
@@ -57,36 +19,13 @@ IObjectTypeDescriptor ResolveReferenceWith<TResolver>(
     /// Returns the descriptor for configuration chaining.
     /// </returns>
     IObjectTypeDescriptor ResolveReferenceWith(MethodInfo method);
-
-    /// <summary>
-    /// Resolve an entity from its representation.
-    /// </summary>
-    /// <param name="type">
-    /// The type on which the reference resolver is located.
-    /// </param>
-    /// <returns>
-    /// Returns the descriptor for configuration chaining.
-    /// </returns>
-    IObjectTypeDescriptor ResolveReferenceWith(Type type);
 }
 
 /// <summary>
 /// The entity descriptor allows to specify a reference resolver.
 /// </summary>
 public interface IEntityResolverDescriptor<TEntity>
 {
-    /// <summary>
-    /// Resolve an entity from its representation.
-    /// </summary>
-    /// <param name="fieldResolver">
-    /// The resolver.
-    /// </param>
-    /// <returns>
-    /// Returns the descriptor for configuration chaining.
-    /// </returns>
-    IObjectTypeDescriptor ResolveReference(
-        FieldResolverDelegate fieldResolver);
-
     /// <summary>
     /// Resolve an entity from its representation.
     /// </summary>
@@ -99,32 +38,6 @@ IObjectTypeDescriptor ResolveReference(
     IObjectTypeDescriptor ResolveReferenceWith(
         Expression<Func<TEntity, object?>> method);
 
-    /// <summary>
-    /// Resolve an entity from its representation.
-    /// </summary>
-    /// <param name="method">
-    /// The reference resolver selector.
-    /// </param>
-    /// <typeparam name="TResolver">
-    /// The type on which the reference resolver is located.
-    /// </typeparam>
-    /// <returns>
-    /// Returns the descriptor for configuration chaining.
-    /// </returns>
-    IObjectTypeDescriptor ResolveReferenceWith<TResolver>(
-        Expression<Func<TResolver, object?>> method);
-
-    /// <summary>
-    /// Resolve an entity from its representation.
-    /// </summary>
-    /// <typeparam name="TResolver">
-    /// The type on which the reference resolver is located.
-    /// </typeparam>
-    /// <returns>
-    /// Returns the descriptor for configuration chaining.
-    /// </returns>
-    IObjectTypeDescriptor ResolveReferenceWith<TResolver>();
-
     /// <summary>
     /// Resolve an entity from its representation.
     /// </summary>
@@ -135,15 +48,4 @@ IObjectTypeDescriptor ResolveReferenceWith<TResolver>(
     /// Returns the descriptor for configuration chaining.
     /// </returns>
     IObjectTypeDescriptor ResolveReferenceWith(MethodInfo method);
-
-    /// <summary>
-    /// Resolve an entity from its representation.
-    /// </summary>
-    /// <param name="type">
-    /// The type on which the reference resolver is located.
-    /// </param>
-    /// <returns>
-    /// Returns the descriptor for configuration chaining.
-    /// </returns>
-    IObjectTypeDescriptor ResolveReferenceWith(Type type);
 }
diff --git a/src/Federation/Helpers/ArgumentParser.cs b/src/Federation/Helpers/ArgumentParser.cs
@@ -31,10 +31,15 @@ private static bool TryGetValue<T>(
         int i,
         out T? value)
     {
+        // TODO does not support list
         switch (valueNode.Kind)
         {
             case SyntaxKind.ObjectValue:
                 var current = path[i];
+                if (type.IsNonNullType())
+                {
+                    return TryGetValue(valueNode, type.InnerType(), path, i, out value);
+                }
 
                 if (type is not IComplexOutputType complexType ||
                     !complexType.Fields.TryGetField(current, out var field))
@@ -55,11 +60,9 @@ private static bool TryGetValue<T>(
                     }
                 }
                 break;
-
             case SyntaxKind.NullValue:
                 value = default(T);
                 return true;
-
             case SyntaxKind.StringValue:
             case SyntaxKind.IntValue:
             case SyntaxKind.FloatValue:
@@ -115,6 +118,7 @@ private static bool Matches(IValueNode valueNode, string[] path, int i)
     {
         switch (valueNode.Kind)
         {
+            // TODO does not handle list
             case SyntaxKind.ObjectValue:
                 var current = path[i];