Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
cmeeren opened this issue Nov 22, 2024 · 2 comments

Comments

@cmeeren
Copy link
Contributor

cmeeren commented Nov 22, 2024

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.

@michaelstaib
Copy link
Member

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.

@cmeeren
Copy link
Contributor Author

cmeeren commented Nov 25, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants