Introduce optional IGraphTypeFactory

[PSanetra]

This PR implements two features:

1. Add a CaseInsensitiveEnumerationGraphType which enables developers to allow passing case-insensitive enum variable values
2. Allow custom GraphType implementation in schema-first approach

Fixes #3105

[Shane32]

My initial thought is that this is a breaking change, since non-generic graph types are not typically registered within DI. So any existing users that don't use the GraphQL builder methods would have a broken build. We could fix that by using (FieldType?)ServiceProvider.GetService(typeof(FieldType)) ?? new FieldType() to minimize the likelyhood of an issue, although technically it's still breaking as seen in the below paragraph.

Second, we have tests to ensure that graph types are only pulled from DI once when the schema is built. This allows use of AddSingleton for graph types, which is very common. The code only works if the graph type is registered as a transient, as singleton / scoped would cause it to re-use the same graph type instance repeatedly. This would seriously break a schema, and probably would not be apparent what the cause of the problem is. In other words, it would be required that users only register via AddTransient, and the only use case is to provide a different base implementation such as using a case-insensitive enumeration graph type, and only for schema-first. (On the other hand, if you use AddSingleton for a scoped schema, that's a major problem also.) We would probably need additional documentation about this in the DI documentation page.

Third, tests don't yet pass. However, it looks like the ApiTests need to run and a single test is failing due to the added registrations in GraphQLBuilderBase. Should be an easy fix.

I really don't think we should include the "case-insensitive" implementation. First, case-insensitivity is specifically against the GraphQL spec, and we should not be 'encouraging' such design. I'd rather include it as a sample in the Q&A. Further, the implementation here, although named "case-insensitive" really only allows for constant-case and upper-case. These are multiple ways of spelling, not a difference in case. So "JohnDoe" becomes either "JOHNDOE" or "JOHN_DOE". You can't use "johndoe" or "johnDoe" or "john_doe".

I'd like input from other of the maintainers here. If there are other solutions to this issue, I'd like to discuss/review them as well.

[Shane32]

You can't use "johndoe" or "johnDoe" or "john_doe".

My mistake. I see you convert the input to uppercase. I'd suggest you just use a case-insensitive comparer in the dictionary constructors.

[PSanetra]

@Shane32 I have removed the public CaseInsensitiveEnumerationGraphType, and have simplified the approach with the introduction of an optional IGraphTypeFactory interface. Please have another look.

[codecov-commenter]

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (49f24b5) 84.22% compared to head (c550b98) 84.21%.

Files	Patch %	Lines
src/GraphQL/Utilities/SchemaBuilder.cs	92.10%	0 Missing and 3 partials ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3913      +/-   ##
==========================================
- Coverage   84.22%   84.21%   -0.01%     
==========================================
  Files         421      422       +1     
  Lines       19532    19542      +10     
  Branches     3047     3051       +4     
==========================================
+ Hits        16451    16458       +7     
  Misses       2366     2366              
- Partials      715      718       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[Shane32]

@Shane32 I have removed the public CaseInsensitiveEnumerationGraphType, and have simplified the approach with the introduction of an optional IGraphTypeFactory interface. Please have another look.

I like this a lot better. The name Factory indicates that it is used to create new instances, similar to IServiceScopeFactory.

But how about this for the interface:

public interface IGraphTypeFactory
{
    // add xml comments here stating that the supported types are ObjectGraphType, EnumerationGraphType, FieldType, etc
    object CreateInstance(Type type);
}

public static GraphTypeFactoryExtensions
{
    public static T CreateInstance<T>(this IGraphTypeFactory factory)
        where T : class
        => (T)factory.CreateInstance(typeof(T));
}

This design makes it so there is only one instance of GraphTypeFactory installed in the DI engine.

[Shane32]

Also with IGraphTypeFactory, it's fully backwards compatible; it's not a breaking change. 👍

[PSanetra]

@Shane32 I like it better to have one Factory Service per GraphType. This way it is possible to customize only specific GraphType implementations. Otherwise I would need a single factory to support all GraphTypes.

[Shane32]

Well you can use inheritance:

public MyFactory : DefaultGraphTypeFactory
{
    public object CreateInstance(Type type)
    {
        if (type == typeof(EnumerationGraphType))
            return new MyEnumGraphType();
        return base.CreateInstance(type);
    }
}

although honestly this isn't much different:

public MyFactory : IGraphTypeFactory
{
    public object CreateInstance(Type type)
        => type == typeof(EnumerationGraphType) ? new MyEnumGraphType() : Activator.CreateInstance(type);
}

[PSanetra]

Yes it is not impossible to find ways, but if possible I would like to avoid coupling between different Factory implementations.

[Shane32]

@gao-artur any comments here?

[gao-artur]

Looks good 👌

[Shane32]

@gao-artur any thoughts on IGraphTypeFactory<T>.Create vs what's basically IGraphTypeFactory.Create<T> ?

[gao-artur]

In general, a single factory makes more sense for me, but I understand @PSanetra's logic.

[Shane32]

I just feel that this is not a good design pattern. Are there other examples of this design pattern in practice? As a single IGraphTypeFactory it's much like IServiceProvider, so it feels like we are reinventing the wheel here to do it differently. And you can always decouple it on the user side if you wanted to with an implementation of IGraphTypeFactory that pulled the requested service from the underlying service provider.

[PSanetra]

@Shane32 I think a good example of this pattern is the IAzureClientFactory<TClient> which is the generic factory interface to use to create client instances in all Azure SDKs.

See https://learn.microsoft.com/en-us/dotnet/api/microsoft.extensions.azure.iazureclientfactory-1?view=azure-dotnet

[gao-artur]

IMO, DefaultGraphTypeFactory is a better name.

[Shane32]

Agree. It also matches DefaultServiceProvider and DefaultNameConverter and stuff

[gao-artur]

I think I'll take @PSanetra's side. Although a generic factory interface is a less common approach than the generic method, it looks "cleaner". It also allows replacing the default behavior only for types you are interested in (EnumerationGraphType in this case), leaving other types untouched. Whoever wants to provide a common logic for all types can do it with inheritance or composition

services.AddSingleton(typeof(IGraphTypeFactory<>), typeof(MyFactory<>))()

public class MyFactory<TGraphType> : BaseFactory, IGraphTypeFactory<TGraphType>
    where TGraphType : IGraphType
{
    public TGraphType Create() => Create<TGraphType>();
}

public class BaseFactory
{
    // Put here whatever logic you want to share
    protected TGraphType Create<TGraphType>()
        where TGraphType : IGraphType
        => Activator.CreateInstance<TGraphType>();
}

[Shane32]

Sounds good. I’ll review again tonight

[Shane32]

Let's make the changes @gao-artur suggested and then merge.

[PSanetra]

@gao-artur @Shane32 Alright I have applied the suggested changes and squashed the commits.

[PSanetra]

@Shane32 @gao-artur Thank you for merging! Do you have an ETA when this change might be released? Is it possible to get a pre-release containing this change?

[Shane32]

When reasonable, I usually issue a release when asked; I can do it now.

You can get prerelease versions from the GitHub Package Registry. Unfortunately it requires authentication so it's a bit of a pain to use. See https://github.com/graphql-dotnet/graphql-dotnet/pkgs/nuget/GraphQL . Version 7.6.2-preview-980 would be the latest 7.x version (newer than 7.7).