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

Features requests #126

Open
gao-artur opened this issue Jan 28, 2023 · 13 comments
Open

Features requests #126

gao-artur opened this issue Jan 28, 2023 · 13 comments

Comments

@gao-artur
Copy link
Contributor

gao-artur commented Jan 28, 2023

Hey. We need a few features that require refactoring of the generated code. I would be happy to provide PR's for them, but first want to coordinate the changes with you.

  1. We need the ability to override the serialization of the JObject fields (custom scalar). Currently, all types handling is hardcoded in BuildArgumentValue method. Instead, I propose creating the TypeRegestrar that will return the builder/serializer delegate. The user can provide his overrides in the BuilderSettings argument passed into GraphQlQueryBuilder.Build method. The same BuilderSettings object may be used later for additional configuration like requested in [Feature Request] NullValueHandling when building request #115. This can be implemented without introducing breaking changes by providing an overload for GraphQlQueryBuilder.Build method instead of replacing the existing one.
  2. Union handling. Currently, the union generates one big class with all fields of all types in the union. While this is a simple way to deserialize any json, it's not very user friendly. Instead, I propose creating a marker interface (an empty interface) to represent the union type. All dotnet types generated from graph types in the union must implement this interface. The deserialization process is the same as for regular graphql interfaces. The user can use pattern-matching to cast the interface to the implementing class. The breaking change may be avoided by providing a configuration flag for union deserialization. But I think the new implementation should be the default.
  3. graphql-dotnet allows retrieving server-side directives via introspection. We use this feature to indicate the real dotnet type behind the ID scalar type. Currently, we use a local copy of your generator with modifications to retrieve the dotnet type from the applied directive during the generation. This is the most difficult feature to make it generic enough to include in the library I need more time to come with a good design. In any case, it most probably will require a breaking change to IScalarFieldTypeMappingProvider interface.

These changes require refactoring that can't be safely done without integration tests: testing the code created by the generator.
Please let me know your thoughts and if you need more concrete examples.

@Husqvik
Copy link
Owner

Husqvik commented Jan 29, 2023

We need the ability to override the serialization of the JObject fields (custom scalar). Currently, all types handling is hardcoded in BuildArgumentValue method. Instead, I propose creating the TypeRegestrar that will return the builder/serializer delegate. The user can provide his overrides in the BuilderSettings argument passed into GraphQlQueryBuilder.Build method. The same BuilderSettings object may be used later for additional configuration like requested in #115. This can be implemented without introducing breaking changes by providing an overload for GraphQlQueryBuilder.Build method instead of replacing the existing one.

Could you specify what kind of customizations you have in mind? I plan to add an interface property that would expose an extension point for overriding the hardcoded .NET -> GraphQL parameter string representation in newly introduced GraphQlBuilderOptions

@gao-artur
Copy link
Contributor Author

gao-artur commented Jan 29, 2023

We need to serialize JObject as an escaped string. For example:

if (value is JObject jObject)
{
    var str = jObject.ToString(formatting == Formatting.Indented ? Newtonsoft.Json.Formatting.Indented : Newtonsoft.Json.Formatting.None);
    var escapedStr = Newtonsoft.Json.JsonConvert.ToString(str);
    return escapedStr;
}

@Husqvik
Copy link
Owner

Husqvik commented Jan 29, 2023

builder.Build(new GraphQlBuilderOptions { ArgumentBuilder = <IGraphQlArgumentBuilder implementation> })

you can customize literally any .NET type passed as parameter

@Husqvik
Copy link
Owner

Husqvik commented Jan 29, 2023

graphql-dotnet allows retrieving server-side directives via introspection. We use this feature to indicate the real dotnet type behind the ID scalar type. Currently, we use a local copy of your generator with modifications to retrieve the dotnet type from the applied directive during the generation. This is the most difficult feature to make it generic enough to include in the library I need more time to come with a good design. In any case, it most probably will require a breaking change to IScalarFieldTypeMappingProvider interface.

Assuming your API is not public so I cannot access its metadata to play around with the server side directives and how to incorporate that option into the generator

@gao-artur
Copy link
Contributor Author

builder.Build(new GraphQlBuilderOptions { ArgumentBuilder = <IGraphQlArgumentBuilder implementation> })

you can customize literally any .NET type passed as parameter

Oh! That's great! Thanks!

@gao-artur
Copy link
Contributor Author

graphql-dotnet allows retrieving server-side directives via introspection. We use this feature to indicate the real dotnet type behind the ID scalar type. Currently, we use a local copy of your generator with modifications to retrieve the dotnet type from the applied directive during the generation. This is the most difficult feature to make it generic enough to include in the library I need more time to come with a good design. In any case, it most probably will require a breaking change to IScalarFieldTypeMappingProvider interface.

Assuming your API is not public so I cannot access its metadata to play around with the server side directives and how to incorporate that option into the generator

I'll create a simple server to demonstrate the usage.

@gao-artur
Copy link
Contributor Author

gao-artur commented Jan 29, 2023

Maybe posting the code here will be simpler

CustomDirectives.cs
using GraphQL.Types;
using GraphQLParser.AST;

namespace CustomDirectives;

public class ClrTypeDirective : Directive
{
    public const string DirectiveName = "clrType";
    public const string ArgumentTypeName = "type";

    public ClrTypeDirective()
        : base(
            DirectiveName,
            DirectiveLocation.ArgumentDefinition,
            DirectiveLocation.FieldDefinition,
            DirectiveLocation.InputFieldDefinition)
    {
        Description = "Clr type of ID graph type";
        Arguments = new QueryArguments(new QueryArgument<NonNullGraphType<StringGraphType>>
        {
            Name = "type",
            Description = "The clr type"
        });
    }

    public override bool? Introspectable => true;
}
FieldBuilderExtensions.cs
using System;
using System.Collections.Generic;

using GraphQL;
using GraphQL.Builders;
using GraphQL.Types;

namespace CustomDirectives;

public static class FieldBuilderExtensions
{
    private static readonly IReadOnlyDictionary<Type, string> _typesMap = new Dictionary<Type, string>
    {
        [typeof(int)] = "int?",
        [typeof(int?)] = "int?",
        [typeof(long)] = "long?",
        [typeof(long?)] = "long?",
        [typeof(string)] = "string"
    };

    public static FieldBuilder<TSourceType, TReturnType> ApplyClrDirective<TSourceType, TReturnType>(
        this FieldBuilder<TSourceType, TReturnType> fieldBuilder)
    {
        var clrTypeDirective = fieldBuilder.FieldType.FindAppliedDirective(ClrTypeDirective.DirectiveName);

        if (clrTypeDirective != null)
        {
            throw new InvalidOperationException(
                $"Field '{fieldBuilder.FieldType.Name}': {nameof(ApplyClrDirective)} should be applied only once.");
        }

        if (!IsAllowedGraphType(fieldBuilder.FieldType.Type))
        {
            throw new InvalidOperationException(
                $"Field '{fieldBuilder.FieldType.Name}': {nameof(ApplyClrDirective)} should be applied only on fields of type '{nameof(IdGraphType)}' (including non-nullable and list of {nameof(IdGraphType)}).");
        }

        if (typeof(TReturnType) == typeof(object))
        {
            throw new InvalidOperationException(
                $"Field '{fieldBuilder.FieldType.Name}': {nameof(ApplyClrDirective)} should be used only on fields with return type defined. Use 'Field<TSourceType,TReturnType>' overload to define return type.");
        }

        if (!TryGetClrType<TReturnType>(out var clrType))
        {
            throw new InvalidOperationException(
                $"Field '{fieldBuilder.FieldType.Name}': {nameof(ApplyClrDirective)} can't be used on field with return type '{typeof(TReturnType).Name}'. Only int(?), long(?) and string are allowed.");
        }

        return fieldBuilder.Directive(
            name: ClrTypeDirective.DirectiveName,
            argumentName: ClrTypeDirective.ArgumentTypeName,
            argumentValue: clrType);
    }

    public static QueryArgument ApplyClrDirective<TClrType>(this QueryArgument arg)
    {
        var clrTypeDirective = arg.FindAppliedDirective(ClrTypeDirective.DirectiveName);

        if (clrTypeDirective != null)
        {
            throw new InvalidOperationException(
                $"Argument '{arg.Name}': {nameof(ApplyClrDirective)} should be applied only once.");
        }

        if (!IsAllowedGraphType(arg.Type))
        {
            throw new InvalidOperationException(
                $"Argument '{arg.Name}': {nameof(ApplyClrDirective)} should be applied only on arguments of type '{nameof(IdGraphType)}' (including non-nullable and list of {nameof(IdGraphType)}).");
        }

        if (!TryGetClrType<TClrType>(out var clrType))
        {
            throw new InvalidOperationException(
                $"Argument '{arg.Name}': {nameof(ApplyClrDirective)} can't be used with TClrType '{typeof(TClrType).Name}'. Only int(?), long(?) and string are allowed.");
        }

        arg.ApplyDirective(
            name: ClrTypeDirective.DirectiveName,
            argumentName: ClrTypeDirective.ArgumentTypeName,
            argumentValue: clrType);

        return arg;
    }

    private static bool IsAllowedGraphType(Type type)
    {
        while (type.IsGenericType)
        {
            type = type.GetGenericArguments()[0];
        }

        return type.IsAssignableTo(typeof(IdGraphType));
    }

    private static bool TryGetClrType<TClrType>(out string clrType)
    {
        var type = typeof(TClrType);

        while (!IsNullable(type) && type.IsGenericType)
        {
            type = type.GetGenericArguments()[0];
        }

        return _typesMap.TryGetValue(type, out clrType);
    }

    private static bool IsNullable(Type type)
    {
        return Nullable.GetUnderlyingType(type) != null;
    }
}
Usage sample
using System.Collections.Generic;

using CustomDirectives;
using SampleServer.Models;

using GraphQL.Types;

namespace SampleServer;

public class ClrTypeDirectiveGraphType : ObjectGraphType<ClrDirectiveExampleStub>
{
    public ClrTypeDirectiveGraphType()
    {
        Field<IdGraphType, int?>("id")
            .ApplyClrDirective()
            .Resolve(_ => 1);

        Field<NonNullGraphType<IdGraphType>, int>("nonNullableId")
            .ApplyClrDirective()
            .Resolve(_ => 2);

        Field<ListGraphType<IdGraphType>, IEnumerable<int>>("listOfIds")
            .ApplyClrDirective()
            .Resolve(_ => new[] { 3 });

        Field<ListGraphType<NonNullGraphType<IdGraphType>>, IEnumerable<long>>("listOfNonNullableIds")
            .ApplyClrDirective()
            .Resolve(_ => new[] { 4L });

        Field<NonNullGraphType<ListGraphType<NonNullGraphType<IdGraphType>>>, IEnumerable<long>>("nonNullableListOfNonNullableIds")
            .ApplyClrDirective()
            .Resolve(_ => new[] { 5L });

        Field<StringGraphType>("ArgsFluent")
            .Argument<IdGraphType>("id", arg => arg.ApplyClrDirective<int?>())
            .Resolve(_ => "all allowed args");

        Field<StringGraphType>("ArgNonFluent", arguments: new QueryArguments
        {
            new QueryArgument<IdGraphType> { Name = "a" }.ApplyClrDirective<int>()
        });
    }
}

@gao-artur
Copy link
Contributor Author

That's how we are currently doing it
https://github.com/Husqvik/GraphQlClientGenerator/pull/128/files

@Husqvik
Copy link
Owner

Husqvik commented Jan 29, 2023

Could you get the schema metadata json? I want something to experiment on. I want to establish some extension points so the integration can be more componentized.

@gao-artur
Copy link
Contributor Author

gao-artur commented Jan 30, 2023

Hey @Husqvik, here are the instrospection.json and the sample server so you can play with it and adjust as you want. Feel free to take ownership on the PR above.

There are 2 profiles in VS: Playground and Console. Make sure to start the server with Console profile to enable applied directives introspection.

SampleService.zip

@Husqvik
Copy link
Owner

Husqvik commented Feb 4, 2023

I don't want to hardcode single dotnet specific server implementation into the generator core. Extended the schema types with JsonExtensionData which makes applied directive or any other field accessible from the GetCustomScalarFieldType method. The concrete implementation can then use that information for resolving the target type.

if (valueType.Extensions.TryGetValue("appliedDirectives", out var appliedDirectives))
{
        //  use appliedDirectives information to resolve the target .NET type
}

@gao-artur
Copy link
Contributor Author

I wouldn't say it's a single server implementation. At least Graphql.NET, HotChocolate and Java have this implementation. And it's probably going to be included in the spec. But I understand your decision. I'll check if your addition can be helpful, but I'm sceptic about it.

@Husqvik
Copy link
Owner

Husqvik commented Feb 4, 2023

Once it becomes standard I gladly include it in the metadata. You have to pass the schema file to the generator, of course, the retrieve schema function cannot be used as the metadata query has to be extended.

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

No branches or pull requests

2 participants