Added special handling for single-case discriminated unions

[Stock44]

Single case discriminated unions should not be treated as regular DUs, instead they are usually used to model constrained types within a domain. This PR implements automatic unwrapping of these values into their field types in the GraphQL schema. This allows the direct mapping of wrapped DUs coming from a source such as Marten (that use IQueryable) to be directly exposed as part of the GraphQL schema.

The unwrapping support is only enabled for output object types, as generally input types should be validated before being wrapped.

[cmeeren]

Thanks!

I have a couple of issues:

• I am not willing to add ModifyOptions(fun options -> options.RemoveUnreachableTypes <- true). This is somewhat related to #16: If I had been willing to use RemoveUnreachableTypes, that issue could have been solved by my just adding every type from every F# assembly, but it is not the place of AddFSharpSupport to decide to call RemoveUnreachableTypes. This must be a choice by the user, so we can't depend on it. Therefore, I expect that support for single-case unions must be opt-in, similar to DU-based enums and unions. (If you somehow find a solution to single-case unions that is not opt-in and does not require the current approach, tests must be added to ensure the user can override the behavior for individual types.)
• If we make this opt-in, then we can remove SkipFSharpSingleCaseUnionUnwrappingAttribute.
• SingleCaseUnionConverter seems to do "too much" by mapping/wrapping, whereas this feature is only about output, not input.
• There are some added <None> items in the test .fsproj that can be removed.
• There is some commented-out code in the test .fs that should be removed

But most importantly, you touch on an important point when you say this:

The unwrapping support is only enabled for output object types, as generally input types should be validated before being wrapped.

I absolutely agree, and I would take it even further: It's not always you want the inner value returned directly as-is. I have single-case DUs consisting of multiple fields, e.g. for composite IDs. I also have single-case DUs that outwardly in the API is represented by a string, even though internally they wrap e.g. byte array (for some access tokens), or they internally wrap an int, but externally are represented by (possibly opaque) strings.

Opt-in support for all of the above, strongly typed and without using reflection, can be implemented more simply than in this PR. In my code, I solve discriminated unions for both input and output using this extension method on IRequestExecutorBuilder:

type IRequestExecutorBuilder with

    member this.AddPrimitiveConverter<'domain, 'serialized, 'schemaType
        when 'schemaType :> INamedType and 'schemaType: (new: unit -> 'schemaType)>
        (parse, getValue)
        =
        let parse x =
            match parse x with
            | Ok x -> x
            | Error(msg: string) -> raise (SerializationException(msg, new 'schemaType ()))

        this
            .AddTypeConverter<'serialized, 'domain>(parse)
            .AddTypeConverter<'domain, 'serialized>(getValue)
            .BindRuntimeType<'domain, 'schemaType>()

It's called like this:

.AddPrimitiveConverter<_, _, StringType>(StatusMessageText.create, _.value)

(To be accurate, all my DUs have create and value members like this, so I use some SRTP in the extension member and just call .AddPrimitiveConverter<StatusMessageText, string, StringType>(), but the principle is the same.)

This provides opt-in and trivial support for custom domain primitives for both input and output (and works whether the primitives are DU-based or not). If we do this opt-in, I think that is the way to go about this.

Similarly, if I want to actually have a separate type for this in the schema (and not just "erase" to String or similar), I have the following base type descriptors:

type PrimitiveType<'domain, 'schema when 'schema :> IValueNode>(name, description, parse, getNode) =
    inherit ScalarType<'domain, 'schema>(name)
    do base.Description <- description

    override this.ParseLiteral(x: 'schema) : 'domain =
        parse x
        |> Result.defaultWith (fun (msg: string) -> raise (SerializationException(msg, this)))

    override this.ParseValue(x: 'domain) : 'schema = getNode x

    override this.ParseResult(resultValue) = this.ParseValue(resultValue)

type StringPrimitiveType<'domain>(name, description, parse, getValue) =
    inherit PrimitiveType<'domain, StringValueNode>(name, description, _.Value >> parse, getValue >> StringValueNode)

This allows me to define schema scalars like this:

type CountryCode2Descriptor() =
    inherit
        StringPrimitiveType<CountryCode2>(
            "CountryCode",
            "A String-based ISO 3166-1 alpha-2 two-letter country code.",
            CountryCode2.create,
            _.value
        )

    override this.Configure(descriptor) =
        base.Configure(descriptor)

        descriptor.Directive(SpecifiedByDirective("https://en.wikipedia.org/wiki/ISO_3166-1_alpha-2"))
        |> ignore

Again, I think this is a better way to go about opt-in support for primitives (whether represented by single-case DUs or something else).

Let me know what you think. I'm not sure what of this I'm comfortable adding, because it's adapted to my usage (e.g. PrimitiveType accepting an explicit name and description), but at the very least, some of this could be documented as a good way of doing it (and maybe something could be added also, possibly with adjustments).

[Stock44]

Regarding the usage of RemoveUnreachableTypes <- true, I added that because without it, the GraphQL schema is left with an object representing the DU, even though all of its usages were replaced by the respective inner type. When the DU is marked private, this errors out Schema generation, as the DU schema object has no members. I could not find a way to actually delete this type myself, as it appears that all schema discovery methods are internal in the TypeInterceptor interface.

I do prefer how your approach works internally, although the one thing I don't like is that it requires manual registration of the type descriptors. This is something I wanted to avoid, and instead make HotChocolate automatically handle the DUs. This is mostly useful when working in an implementation-first style.

I think we could maybe have both approaches, where types that require conversion can be registered manually, while simpler types can just be automatically unwrapped by the GraphQL schema. Perhaps we could even have a convention for DU methods, such as your create and value (my DUs tend to have them in the form of TryCreate and Value), so that they are automatically used for parsing and unwrapping the DU values. This would have to be done via reflection, though.

To actually enable any of this, I need to find a way to remove the leftover types after conversion. I'll also remove the unused capabilities of the SingeCaseUnionConverter

As an aside, I believe the best way to do this would be to replicate how Option support was originally added to HC. Option types are actually removed when inspecting the types, which is similar to how it should work with single case DUs. The option type converter is then responsible for wrapping and unwrapping these types. When dealing with DUs, we could use reflection to find methods that match the create and value conventions, and use them in the conversion.

All of this is done inside the TypeInfo class, which is implemented in the main HC project. Sadly, I don't see a way of replicating this behavior without an extension point there. Perhaps the new APIs intended to replace TypeInterceptor could be used here, once they are ready.

[cmeeren]

Please let me know if I've failed to address all of your points below.

Handling DUs by default, without registration

Despite my earlier remarks, I agree that if you can find a way to make HotChocolate support single-case DUs by unwrapping them by default, then that is acceptable (and even preferrable, given that I can see no other sensible default representation of single-case DUs). This aligns with #16, which is about making HotChocolate automatically use FSharp.HotChocolate's type descriptors for all relevant DU types returned in GraphQL.

Heck, I'd even accept by default constructing single-case DUs on input, as long as the constructor is public.

The requirements for adding this to FSharp.HotChocolate is:

• It must be easily overridable per type, for example using the two approaches I described with type converters or type descriptors. (This also goes for DUs as unions or DUs as enums, as mentioned in #16.)
• It must take effect only for types returned through GraphQL; unreachable types must not be added to the schema.
• It must not enable RemoveUnreachableTypes. I'd prefer if it doesn't require it at all, though I may be open to an opt-in method that enables this functionality and requires RemoveUnreachableTypes to work. That could also be added for DUs as unions and DUs as enums, as described in #16. But, again, I'd prefer something that works without RemoveUnreachableTypes.

Manual registration of type descriptors

I do prefer how your approach works internally, although the one thing I don't like is that it requires manual registration of the type descriptors.

If you specifically talk about type descriptors (not including type converters, the other approach I mentioned for "erased" primitives), then I actually don't register type descriptors manually; I use the following IRequestExecutorBuilder extension:

type IRequestExecutorBuilder with

    [<RequiresExplicitTypeArguments>]
    member this.AddTypesInAssembly<'a>() =
        Assembly.GetAssembly(typeof<'a>).GetTypes()
        |> Seq.filter (fun schemaType ->
            not schemaType.IsGenericType
            && tryGetBaseType
                (fun t ->
                    t.IsGenericType
                    && (let td = t.GetGenericTypeDefinition()

                        td = typedefof<ScalarType<_, _>>
                        || td = typedefof<EnumType<_>>
                        || td = typedefof<ObjectType<_>>
                        || td = typedefof<InputObjectType<_>>
                        || td = typedefof<UnionType<_>>)
                )
                schemaType
               |> Option.isSome
        )
        |> Seq.iter (fun schemaType -> this.AddType(schemaType) |> ignore<IRequestExecutorBuilder>)

        this

(I may have missed some base types in the || chain, but those are all I use at the moment.)

I call this as:

.AddTypesInAssembly<Query>()

That registers all type descriptors in the assembly containing the specified type. My thinking is that if I define a (non-generic) type descriptor, I would always want it added, and this helper does that once for the whole assembly.

This extension method is not related to F# types in any way, so it has no place in FSharp.HotChocolate. I think something like this would make a nice addition to HotChocolate itself, but it's not something I feel strongly enough that I'm willing to do something about.

If you meant to include my explicit registration of type converters in your remark, then I don't see how you can automatically implement the parse/value-based type converter generally. As I mentioned, many of my DUs have multiple representations, and I want control over which representation is used in the API. This also flows into the point below.

Convention for DU create/value methods

Perhaps we could even have a convention for DU methods, such as your create and value (my DUs tend to have them in the form of TryCreate and Value), so that they are automatically used for parsing and unwrapping the DU values. This would have to be done via reflection, though.

Since there is no standard for this in the F# community, I don't think this is a good idea to build into the library. Issues:

• The methods have no standard name
• The methods may not be methods at all, but functions in a separate module
• The methods may return Result, Option, or simply throw for invalid input
• If they return Result, there is no knowing what the error value is. It may be a string as in my case, or it may be an error DU, or something else. It may be a single error or a list of errors.

Also, the AddType call is as simple as this:

.AddPrimitiveConverter<_, _, StringType>(StatusMessageText.create, _.value)

With SRTP constraints suited to my methods, it's even simpler:

.AddPrimitiveConverter<StatusMessageText, string, StringType>()

Both are strongly typed and neither uses reflection, and both give me full control over how the type is represented and parsed in the API. Given all of that, this is not something I see a big need for, even with quite a few of these types.

By the way, the AddPrimitiveConverter methods are also examples of something that really isn't F#-related, but would be a nice addition to HotChocolate itself. Again, I don't feel strongly enough about this to do something about it, since the extension methods are fairly small and simple.

[cmeeren]

Closing due to lack of activity. Feel free to continue the discussion, if desired.