ChilliCream/graphql-platform

Don't require ToString to return a valid enum value name in IEnumTypeDescriptor.Value

cmeeren opened this issue · 2 comments

Product

Hot Chocolate

Is your feature request related to a problem?

When configuring an enum type using a type deriving from EnumType<_>, we call IEnumTypeDescriptor.Value(...) in the Configure override to add values. The Value method calls INamingConventions.GetEnumValueName, which in the default naming conventions calls ToString() on the enum value.

Now consider a non-enum type that we want to surface as an enum in GraphQL. My use-case is surfacing F# union types as enums in HotChocolate.FSharp, but the principle is the same for any non-enum type, so for your convenience I'll give a C# example:

public class Component
{
    public static readonly Component SystemA = new Component("System.A");
    public static readonly Component SubsystemA = new Component("Subsystem.A");
    
    private string Value { get; }

    private Component(string value)
    {
        Value = value;
    }

    public override string ToString()
    {
        return Value;
    }
}

public class ComponentDescriptor : EnumType<Component>
{
    protected override void Configure(IEnumTypeDescriptor<Component> descriptor)
    {
        descriptor.Value(Component.SystemA).Name("SYSTEM_A");
        descriptor.Value(Component.SubsystemA).Name("SUBSYSTEM_A");
    }
}

What happens above is that when I call e.g. Value(Component.SystemA), the naming conventions end up getting the value "System.A" from the ToString call it does. Since this contains a dot, it's not a valid GraphQL enum name, and HotChocolate immediately throws at this point (in HotChocolate.Utilities.NameUtils.EnsureGraphQLName, via the setter of DefinitionBase.Name), not reaching the Name call which specifies a proper GraphQL enum name.

The solution you'd like

I'm open to suggestions, but one simple, low-maintenance and backwards compatible solution would be to add an overload of IEnumTypeDescriptor<_>.Value that accepts the name as the second parameter, and just set DefinitionBase.Name to this name. I would then call:

descriptor.Value(Component.SystemA, "SYSTEM_A");

I have considered adding a custom naming convention in FSharp.HotChocolate, but that is not something I'd like to do, because either FSharp.HotChocolate would override the user's custom naming conventions if set, or the user's would override the library's, negating the fix.

This has wider implications. The API needs to be consistent across all types. I do not want to introduce a new overload here. A better solution is to not set these things until the type is created. But this is quite a large refactoring with implications across a lot of extensions. We will have a look at it but this is nothing that will be done in the V14 or V15 cycle. We have a larger work at the moment focused on schema primitives to get a new abstraction in that will make schema building more flexible with source generators. Until this is done we will not attempt any larger changes to the core.

A better solution is to not set these things until the type is created.

I wholeheartedly agree.

We have a larger work at the moment ... Until this is done we will not attempt any larger changes to the core.

I understand. It is of course unfortunate, since there is no workaround apart from changing a type's ToString implementation, which may not always be possible, but in my specific case I can live with it.