ChilliCream / graphql-platform

Welcome to the home of the Hot Chocolate GraphQL server for .NET, the Strawberry Shake GraphQL client for .NET and Banana Cake Pop the awesome Monaco based GraphQL IDE.
https://chillicream.com
MIT License
4.96k stars 722 forks source link

Add filtering convention runtime type binding support for open generic types #7082

Open Lippur opened 2 weeks ago

Lippur commented 2 weeks ago

Product

Hot Chocolate

Is your feature request related to a problem?

The BindRuntimeType method on the IFilterConventionDescriptor does not currently work for open generic types. This means that every generic type definition has to be registered separately. Since the filter convention is configured earlier than all schema types are discovered, automatic registration is also not possible via interceptors or other means.

In practical terms, this means that using custom scalars for generic types in combination with filtering for these custom scalars is not possible and leads to incorrect filter inputs being generated, which also cannot be removed or ignored automatically.

Example

builder.Services.AddGraphQLServer()
// ...
  .AddFiltering(f =>
    {
      f.BindRuntimeType(typeof(Id<>), typeof(IdFilterType<>)); // IdFilterType<T> implements ComparableOperationFilterInputType<T>
    }
  )

This binding is ignored entirely during runtime. Instead of a comparable filter input type, a default object comparison input type is added.

The only way to make it work is to register all types manually:

builder.Services.AddGraphQLServer()
// ...
  .AddFiltering(f =>
    {
      f.BindRuntimeType(typeof(Id<MyEntity>), typeof(IdFilterType<MyEntity>));
      f.BindRuntimeType(typeof(Id<OtherEntity>), typeof(IdFilterType<OtherEntity>));
      // ... and so on
    }
  )

This is extremely non-ergonomic and defeats the entire purpose of generic types in C#.

This problem is made much worse by the fact that filtering completely ignores the bindings of custom scalars. Even if a type uses a custom scalar which is a StringValueNode (ScalarType<Id, StringValueNode> for example), filtering still treats it like an object, not a string. This is documented in the HotChocolate documentation, but that shortcoming makes the issue with generic types even worse.

The solution you'd like

The issue could be solved if any of the following fixes are made to the runtime type binding for filtering:

  1. Allow binding open generic types and/or interfaces which are then correctly resolved at runtime to register the appropriate input objects.
  2. Use the custom scalars already registered in the schema without separately registering them with the filtering convention.
  3. Allow binding custom scalars to filter types directly, rather than having to register the underlying runtime type of the custom scalar, and use the actual schema type of the scalar for filtering, e.g. if the custom scalar is a StringValueNode, treat the value as a string for filtering too.

Ideally, all three should be implemented, but any one of the three would fix this particular issue.

Lippur commented 2 weeks ago

I came up with a very dirty implementation of adding support for open generics in the filter convention:

public override ExtendedTypeReference GetFieldType (MemberInfo member)
{
    if (member is not PropertyInfo { PropertyType.IsGenericType: true } genericProperty)
    {
        // Not a generic property, default to existing logic
        return base.GetFieldType(member);
    }

    // If the property is nullable, get the underlying type from it
    var genericType = Nullable.GetUnderlyingType(genericProperty.PropertyType) is
        { IsGenericType: true } nullableGenericType
        ? nullableGenericType
        : genericProperty.PropertyType;

    if (_bindings?.FirstOrDefault(
            b => b.Key.IsGenericTypeDefinition &&
                 b.Key.GetGenericTypeDefinition() == genericType.GetGenericTypeDefinition()
        ) is not (not null, var filterType) { Value: not null })
    {
        // There is no registered open generic binding for this type, default to existing logic
        return base.GetFieldType(member);
    }

    if (filterType is { IsGenericTypeDefinition: true, ContainsGenericParameters: true } &&
        filterType.GetGenericArguments()
            .All(
                a => a.GetGenericParameterConstraints().All(genericType.IsAssignableTo)
            )
       )
    {
        // The filter input type is generic and expects the field type as a generic type argument, so let's construct the correct filter input type
        filterType = filterType.MakeGenericType(genericType);
    }

    return _typeInspector!.GetTypeRef(
        filterType,
        TypeContext.Input,
        Scope
    );
}

This code is from a class which extends FilterConvention, as seen by the override and calls to base method. Basically, it checks if a generic binding (either open or closed generic) has been made for the given field type, and if so, uses that to return the correct filter input type. Otherwise, it defers to existing logic.

With a few tweaks, this could be incorporated into the built-in FilterConvention, causing no breaking changes, as open generics were just ignored previously.