Rewrite ObjectExtensions.ToObject with source generator

[gao-artur]

Just FYI, here's a few comments that may be helpful:

When InputObjectGraphType<T> is in use, ObjectExtensions.ToObject is called from InputObjectGraphType<T>.ParseDictionary (unless overridden). When GetArgument is later called, the object isn't an IDictionary and so it does not call ObjectExtensions.ToObject again. The advantage here is that deserialization errors are caught during variable parsing. This scenario is probably the case for most code-first and type-first code.

When InputObjectGraphType (untyped) is in use, the dictionary itself is the stored for later use by GetArgument. The field resolver's call to GetArgument triggers ObjectExtensions.ToObject to convert it to the specified type. This is scenario is probably the case for schema-first code. (not sure though)

In the past, I had been thinking about replacing the ToObject code with dynamically-compiled code. This doesn't help for AOT compilation scenarios, however. I think I may have even tried it, but the current design of ToObject is such that it may call a different constructor if an optional field was not provided. (Going from memory here, so I could be wrong.) In the end, the existing code worked fine, so I spent my dev time elsewhere.

AutoRegisteringInputObjectGraphType only adds fields with settable properties. Some people have requested that it add all properties, assuming that get-only properties can be set through the constructor. I would say that it should inspect the constructors and include get-only properties as applicable, but I have not spent any time on it. Either way, the design of AutoRegisteringInputObjectGraphType (and probably InputObjectGraphType<T>) is such that an applicable constructor could be selected and code generated (dynamically compiled or source generated) that always behaves in a predefined way for that type. It may represent a change in behavior, in that all fields are always set on the deserialized object; optional fields would be set to null if not provided and no default field value exists. I would favor this, as it should simplify the code that executes during ParseDictionary. But it should be made for v8 if it changes existing behavior (even if only in rare circumstances.)

Originally posted by @Shane32 in #3732 (comment)

[gao-artur]

The current idea (draft and oversimplified). For this source code

public class MyInput
{
    public MyInput(string field1)
    {
        Field1 = field1;
    }

    public string Field1 { get; }
    public string? Field2 { get; init; }
    public string? Field3 { get; set; } = "default for Field3";
    public string? Field4 { get; set; }
}

public class MyInputObjectGraphType : InputObjectGraphType<MyInput>
{
    public MyInputObjectGraphType()
    {
        Field<NonNullGraphType<StringGraphType>>("Field1");
        Field<StringGraphType>("Field2").DefaultValue("default for Field2");
        Field<StringGraphType>("Field3");
        Field<StringGraphType>("Field4");
    }
}

Generate this parsing code

public MyInput Parse(IDictionary<string, object?> source)
{
    if (source == null)
    {
        throw new Exception();
    }

    if (!source.TryGetValue("Field1", out object? field1))
    {
        throw new Exception();
    }

    return new MyInput((string)field1.GetPropertyValue(typeof(string))!)
    {
        Field2 = GetValueOrDefault(source, "Field2", "default for Field2"), // the default is from the DefaultValue method
        Field3 = GetValueOrDefault(source, "Field3", "default for Field3"), // the default is from the field initializer
        Field4 = GetValueOrDefault(source, "Field4", default(string)),
    };
}

private static T GetValueOrDefault<T>(IDictionary<string, object?> source, string fieldName, T defaultValue) =>
    source.TryGetValue(fieldName, out object? field)
        ? (T)field.GetPropertyValue(typeof(T))!
        : defaultValue;

This is just an example of the direction. The GetPropertyValue probably needs to be rewritten too. It also doesn't handle the cases when the default value for the property is set in the constructor.
Ideally, there should be an if (field exists in source) then set the property block for each property, but it won't work for init-only properties.

[gao-artur]

It looks like init properties are a general problem in the source-generated serializers. We will probably need to fall back to reflection to set them or raise an error. Without considering init properties. The example above may look like this

public MyInput Parse(IDictionary<string, object?> source)
{
    if (source == null)
    {
        throw new ArgumentNullException(nameof(source));
    }

    if (!source.TryGetValue("Field1", out object? field1))
    {
        throw new Exception();
    }

    var result = new MyInput(field1.GetPropertyValue<string>());

    if (source.TryGetValue("Field2", out object? field2))
    {
        result.Field2 = field2.GetPropertyValue<string>();
    }
    else
    {
        result.Field2 = "default for Field2"; // the default is from the DefaultValue method
    }

    if (source.TryGetValue("Field3", out object? field3))
    {
        result.Field3 = field2.GetPropertyValue<string>();
    }

    if (source.TryGetValue("Field4", out object? field4))
    {
        result.Field4 = field4.GetPropertyValue<string>();
    }

    return result;
}

required properties should be verified similarly to constructor parameters. If the required parameter is missing, an exception should be thrown. Here, we will need an analyzer to ensure all required properties are configured as required/have default values on the graph type.

[Shane32]

Why wouldn’t your prior sample work for init properties ?

[gao-artur]

Because init props must be set durring the object creation. It means, first I have to check every init prop if it has a field in the source dictionary, then decide which of them I need to set durring the object creation.
In the first example I always set all field and can accidentally override a value which was set by the constructor, or backing field, or whatever weired logic was applied there.

[Shane32]

Maybe that’s okay. Is the source generator opt in or always on by default?

[gao-artur]

It can be opt-in with msbuild properties. For example, the new configuration binder source generator uses

<EnableConfigurationBindingGenerator>true</EnableConfigurationBindingGenerator>

But I think it should be more granular than just enable/disable. For example, I'd prefer to avoid side effects and be notified when init is used:

• disabled - the current reflection-based implementation
• enabled - no side effects allowed, static analyzer error + runtime exception when init is used (required init is ok btw)
• unsafe - possible side effects when init is used

[gao-artur]

enabled - no side effects allowed, static analyzer error + runtime exception when init is used (required init is ok btw)

There is no need to create an analyzer or throw an exception. The code won't compile for init properties.

[Shane32]

Perhaps we should start by simplifying the reflection-based design for v8, then making the source-generated version of it. I could probably do the former part, and we could discuss. Then the functionality of the reflection-based code and source-generated code would match.

[gao-artur]

Also, to not miss any field, we need the Strong typed API for building fields done as a prerequisite. Otherwise, it will be possible to generate code only for fields defined in the InputObjectGraphType constructor. Because outside of the constructor, we can't guarantee the field builder is for the input object.

[Shane32]

I've been messing around with dynamically compiled code, reviewing the existing mechanics, and thinking about the options here. Here's my thoughts so far:

1. For source generation, init and required members must be set during initialization. However, this limitation does not apply to dynamically-compiled code.
2. The default value of a property or field may be able to be copied from the class source for source generation (??), but is not available via reflection. It also would not be possible if the class is defined in an external library.
3. Record class types, which require values passed within the constructor, should be supported.
4. Looking at the System.Text.Json serializer should provide a good template from which to build the supported features
5. For dynamic compilation, implementing changes only to ToObject would not work; the dynamically-compiled code would be based on the IGraphType instance passed in, which for a scoped schema will change for every schema instance. So a cache implemented within ToObject would function as a memory leak. And if the instance compiled its own ParseDictionary handler, it would recompile for each initialized schema (that called the method). Source generation would be much preferred, while dynamic compilation is feasible for singleton schemas only.
6. Assuming we add source generation (or dynamic compilation), it should probably be opt-in due to the differences of the implementation compared to previously, and/or simply because the source generator needs to successfully run in order to compile the project -- ???
7. We should consider various Microsoft source generation implementations for how to trigger the source generator. I suggest reviewing the logging implementation, where the user will be required to add an attribute to the partial class and/or partial method to trigger source generation. https://learn.microsoft.com/en-us/dotnet/core/extensions/logger-message-generator
8. Any default values defined within the graph type will be visible in the returned dictionary, so the .DefaultValue() extension method need not be used.

I suggest we define these new semantics for behavior:

Behavior	Notes	Previous behavior, if different
Use only public constructors
Use the public default constructor if available	Same as STJ	Uses the constructor with most arguments
Otherwise, selects best constructor at compile-time based on field list		May pick different constructors
Uses only public properties/fields	Same as STJ	Supports private also
Coerces undefined fields to default for constructor arguments	Same as STJ	Looks for another constructor
Coerces undefined fields to default for required/init-only props	STJ source-generation doesn't support required props	Skips field
Other properties are set only when needed	Same as STJ

A simplified sample is below:

public record TestClass2(string Prop1)
{
    public int Prop2 { get; init; }
    public int Prop3 { get; set; }
}

// source-generation and/or dynamic compilation equivalent
public object? ParseDictionary(IDictionary<string, object?> source)
{
    var result = new Class1(source.TryGetValue("prop1", out object? value) ? (string)value.GetPropertyValue(typeof(string), Fields.Find("prop1")!.ResolvedType) : default(string))
    {
        Prop2 = source.TryGetValue("prop2", out object? value2) ? (int)value2.GetPropertyValue(typeof(int), Fields.Find("prop2")!.ResolvedType) : default(int)
    };

    if (source.TryGetValue("prop3", out object? value3))
    {
        result.Prop3 = (int)value3.GetPropertyValue(typeof(int), Fields.Find("prop3")!.ResolvedType);
    }

    return result;
}

Notice in the above sample that there are still dictionary accesses to the Fields list to make the code work properly when mixed with input object types that inherit from InputObjectGraphType (as without a <T> the ParseDictionary code cannot return a concrete type). Dynamic compilation would compile these references directly into the code, so no dictionary lookup would be necessary. But of course source generation is preferred for AOT compilation and scoped schemas, not to mention that debugging runtime issues is vastly easier.

@gao-artur What do you think?

[gao-artur]

1. For source generation, init and required members must be set during initialization. However, this limitation does not apply to dynamically-compiled code.
2. The default value of a property or field may be able to be copied from the class source for source generation (??), but is not available via reflection. It also would not be possible if the class is defined in an external library.

Should we just forbid the usage of init properties when enabling the code generator? It's not a problem for the non-init properties because we will only set it if the field is sent by the client or the default value provided by the schema, so there is no risk of overriding property default values.

5. For dynamic compilation, implementing changes only to ToObject would not work; the dynamically-compiled code would be based on the IGraphType instance passed in, which for a scoped schema will change for every schema instance. So a cache implemented within ToObject would function as a memory leak. And if the instance compiled its own ParseDictionary handler, it would recompile for each initialized schema (that called the method). Source generation would be much preferred, while dynamic compilation is feasible for singleton schemas only.

I think the compiled handler should implement the same "set only if provided" logic as a generated code. The IGraphType should be a handler argument, and caching should be done based on IGraphType implementation type rather than an instance.

6. Assuming we add source generation (or dynamic compilation), it should probably be opt-in due to the differences of the implementation compared to previously, and/or simply because the source generator needs to successfully run in order to compile the project -- ???

I think the dynamic compilation should replace the reflection with all required breaking changes in behavior. And the source generation should be opt-in for the users who accept its limitations for better cold start times and AOT support.

7. We should consider various Microsoft source generation implementations for how to trigger the source generator. I suggest reviewing the logging implementation, where the user will be required to add an attribute to the partial class and/or partial method to trigger source generation. https://learn.microsoft.com/en-us/dotnet/core/extensions/logger-message-generator

We already have all the required markers to discover the types that need the source generation: InputObjectGraphType and InputTypeAttribute. We can require types to be partial or generate the code in totally different place, then register the generated parser in ValueConverter. Just need somehow to ensure the registration runs before any user's registrations.

See my comments in an additional column.

Behavior	Notes	Previous behavior, if different	Additional comments
Use only public constructors
Use the public default constructor if available	Same as STJ	Uses the constructor with most arguments
Otherwise, selects best constructor at compile-time based on field list		May pick different constructors	Should generate code/compiled expression for all the available public contractors (unless default constructor is available) and pick the best match at run-time
Uses only public properties/fields	Same as STJ	Supports private also	I think it currently supports public properties/fields only
Coerces undefined fields to default for constructor arguments	Same as STJ	Looks for another constructor	Can you explain?
Coerces undefined fields to default for required/init-only props	STJ source-generation doesn't support required props	Skips field	required or required init properties should be treated the same as constructor parameters - if a required field is not provided, the parsing should fail. If a property is marked as required, the static analyzer must ensure the input field is also required
Other properties are set only when needed	Same as STJ		Agree. And this is the biggest problem with init properties (but not required init)

As an additional note, it's not always possible to discover all the input type fields during the build. For example, if the field is defined in an external assembly. This means at the end of the generated code, we must check if there are not yet mapped fields in the dictionary and either throw an exception or fall back to the compiled expressions solution (for these fields only). This fallback is obviously not possible in AOT. So when compiled to AOT, we can run a schema visitor to ensure the generated code covers all the fields available in run-time (the generator can create some kind of metadata with a list of covered fields. The visitor will use this list for validation).

[Shane32]

Please explain: Coerces undefined fields to default for constructor arguments

Consider:

class Class1
{
    public string Prop1 { get; }

    public Class1(string prop1)
    {
        this.Prop1 = prop1;
    }
}

// or the following, which does not have a default constructor:
//   record class Class1(string Prop1);

var c = System.Text.Json.JsonSerializer.Deserialize<Class1>("{}")!;
// works fine, passing null to the prop1 argument

In this example, there is only a single constructor, and that constructor the only constructor requires Prop1, and STJ supplies null even though it was actually undefined as there is no property in the JSON that matched.

[Shane32]

required or required init properties should be treated the same as constructor parameters - if a required field is not provided, the parsing should fail. If a property is marked as required, the static analyzer must ensure the input field is also required

I disagree a little here. Consider the following:

record class Widget(string Name, string? Description);

// or:

public class Widget
{
    public required string Name { get; init; }
    public required string? Description { get; init; }
}

In this case the proper way to use the class (via .NET) is the following, assuming the description is null:

var widget = new Widget("Test", null);

// or:

var widget = new Widget { Name = "Test", Description = null };

So far so good. Now how would this be represented in GraphQL.NET ?

public class WidgetType : InputObjectGraphType<Widget>
{
    public WidgetType()
    {
        Field(x => x.Name);
        Field(x => x.Description, true);
    }
}

Now from the view of the GraphQL client, both of these requests should be identical:

query sample1 {
    example(arg: { name: "Test" })
}

query sample2 {
    example(arg: { name: "Test" description: null })
}

So from the point of view of the GraphQL client, both queries should work (as description is a nullable field). Why would we reject the sample1 query and accept the sample2 query? So long as we meet whatever internal requirements we have, both queries should function equally.

Then, from the point of view of the Widget class as represented in .NET, the class definition (in either form) is logical and functional. I don't see a reason not to add support for required nullable properties, whether it is via the required modifier or if it is a required constructor parameter.

Finally, given the above, I would apply the same logic to init-only properties. Why remove support because they are required to be set during construction? This is simply a requirement of the language. Think of GraphQL.NET as a translation layer between two languages. In this case .NET has no built-in understanding of undefined; all properties/fields are defined when compiled. GraphQL.NET should perform translation as best it can, given the context of the two languages.

But let's think a little further: when would it matter if undefined were not coerced to null? Consider the below:

public class Widget
{
    public string Name { get; init; } = "DefaultName";
}

public class WidgetType : InputObjectGraphType<Widget>
{
    public WidgetType()
    {
        Field(x => x.Name).DefaultValue("DefaultName");
    }
}

{
    sample(widget: {}) # name will be DefaultName
}

Here in this example, the Name field is required and has a default value. The client did not pass a value for name. How does this affect any of the above? None; the dictionary passed to ParseDictionary will include entries for any fields that have a default value, as defined by the input object. (Note: this behavior is explicitly defined by the GraphQL spec.)

The only situation where there is a true difference in behavior is when the .NET class defines a default value, but the GraphQL type does not.

public class Widget
{
    public string? Name { get; init; } = "UnspecifiedName";
}

public class WidgetType : InputObjectGraphType<Widget>
{
    public WidgetType()
    {
        Field(x => x.Name, true); // nullable
    }
}

query sample1 {
    sample(widget: {}) # name is UnspecifiedName
}
query sample2 {
    sample(widget: { name: null })  # name is null
}

In this example, the two sample queries "should" behave differently. (This could also be debated.) I feel this is a rare situation, and could easily be accommodated by specifying that in these situations, you must use set; rather than init; or required. Obviously this scenario is impossible when the property is provided through the constructor.

In conclusion, if we were to simply set all constructor parameters and required/init-only properties defined in the input type, we would only have an issue in the scenario where all of these statements are true:

1. The GraphQL field and .NET property is nullable (if non-null, the dictionary will always have a value)
2. The GraphQL field does not define a default value (if it did, the dictionary will always have a value)
3. The .NET property defines a default value (if it didn't, coercion to null/default wouldn't matter)
4. The .NET property is declared init; (within .NET, required fields MUST be set during object initialization anyway, making any .NET default value pointless)

Note that dynamic/AOT compilation can read default values for optional constructor parameters from the signature.

With this limited scope that we need to resolve, we have a few options:

1. Use dynamic compilation and set the init-only properties only when set by the request; AOT will fail to build
2. Ditto, but AOT will coerce to null
3. Always set init-only properties (just like required properties)

What are the workarounds available to the developer for option 3?

• Change init; to set;, or
• Define the default value within the GraphQL schema, or
• Override ParseDictionary with custom logic

Personally, I vote for option 3 above, where we always set init-only properties just as with required properties. I would expect the use case here to be so dramatically miniscule that the following benefits outweigh the alternative choices:

• Behavior is always the same whether it is dynamically-compiled or AOT-compiled
• init; is fully supported in the vast majority of use cases
• Simplified code design within GraphQL.NET
• Very simple (and logical) workarounds available to the developer

For all the same reasons, during dynamic/AOT compilation, I would not provide code paths for every constructor available. The use case is just too remote; even if there was a scenario that met the above unique requirements, now on top of the prior scenario requirements, it would have to be a class with multiple constructors. That seems to be extremely remote, and adds a considerable layer of complexity.

[gao-artur]

var c = System.Text.Json.JsonSerializer.Deserialize<Class1>("{}")!;
// works fine, passing null to the prop1 argument

Interesting. Honestly, I don't understand why.

I have always seen required as a replacement for constructors. But making this property required will fail the deserialize

JSON deserialization for type 'Class1' was missing required properties, including the following: Prop1

So, with STJ, the contractor is a "recommendation", but required is a must...
More than that, the Prop1 is not even nullable. So why STJ decides to fill it with null?

[gao-artur]

Then, from the point of view of the Widget class as represented in .NET, the class definition (in either form) is logical and functional. I don't see a reason not to add support for required nullable properties, whether it is via the required modifier or if it is a required constructor parameter.

Ok then. In other words, we can say that nullable required == optional.

The only situation where there is a true difference in behavior is when the .NET class defines a default value, but the GraphQL type does not.
...
In this example, the two sample queries "should" behave differently. (This could also be debated.) I feel this is a rare situation, and could easily be accommodated by specifying that in these situations, you must use set; rather than init; or required.

I think "behave differently" is a bad choice. It most likely will be unexpected and will lead to bugs. A similar problem was recently fixed in protobuf-net with a static analyzer that aligns defaults.

With this limited scope that we need to resolve, we have a few options:
...
3. Always set init-only properties (just like required properties)

What are the workarounds available to the developer for option 3?

• Change init; to set;, or
• Define the default value within the GraphQL schema, or
• Override ParseDictionary with custom logic

I think the best choice here is Define the default value within the GraphQL schema regardless of the init or set. We should align defaults to avoid unexpected bugs. The user will have to change the schema or c# model to get consistent behavior.

For all the same reasons, during dynamic/AOT compilation, I would not provide code paths for every constructor available. The use case is just too remote; even if there was a scenario that met the above unique requirements, now on top of the prior scenario requirements, it would have to be a class with multiple constructors. That seems to be extremely remote, and adds a considerable layer of complexity.

I tested the following with STJ

class Class1
{
    public string Prop1 { get; }
    public string Prop2 { get; }

    public Class1(string prop1)
        : this(prop1, "Prop2Default")
    {
        Prop1 = prop1;
    }

    public Class1(string prop1, string prop2)
    {
        Prop1 = prop1;
        Prop2 = prop2;
    }
}

var c = System.Text.Json.JsonSerializer.Deserialize<Class1>("""{ "Prop1": "p1" }""")!;

And got an exception:

Deserialization of types without a parameterless constructor, a singular parameterized constructor, or a parameterized constructor annotated with 'JsonConstructorAttribute' is not supported.

Should we have the same limitation with an exception? Silently choosing a constructor with the most parameters will lead to unexpected bugs. In the previous example, the Prop2 will be null when the user may expect Prop2Default.

[Shane32]

Silently choosing a constructor with the most parameters will lead to unexpected bugs.

I like that answer

[gao-artur]

What about the nullable input field and not nullable required property?

public class Widget
{
    public required string Name { get; init; }
}

public class WidgetType : InputObjectGraphType<Widget>
{
    public WidgetType()
    {
        Field(x => x.Name, true);
    }
}

Sounds like a conflict. I believe it's still possible to set it with dynamic compilation but not with a generated code. And even with dynamic compilation user will get NRE where it's not expected to happen.

[Shane32]

Generated code can use Name = …!. But generated code ignores compiler warnings, so it’s unlikely it would be a problem at all. Dynamic compilation knows nothing of NRT attributes, as a general rule of thumb.

For value types, the value would have to be coerced to a non-null type. I would just coerce to default. In general this the designer’s problem. This behavior is occasionally desirable when you want to coerce a nullable int to 0 when null, anyway.

[Shane32]

Note that this is generally what happens when you use GetArgument currently: default if null.

[gao-artur]

5. For dynamic compilation, implementing changes only to ToObject would not work; the dynamically-compiled code would be based on the IGraphType instance passed in, which for a scoped schema will change for every schema instance. So a cache implemented within ToObject would function as a memory leak. And if the instance compiled its own ParseDictionary handler, it would recompile for each initialized schema (that called the method)

You can't have different graph type implementations with the same name, right? So the cache can be based on the type name.

[Shane32]

Yes, but we support multiple concurrent schemas on a single server.

But typically ToObject is called only by ParseDictionary, and as such we can just generate the code within the input object graph type class (or store the compiled function there), right? Saves a dictionary lookup anyway.

[gao-artur]

Yeah, but as you said, ParseDictionary is an instance method, so where exactly the cache will be stored?
Also, for code generation, if the input type is defined like this, the implementation can be in partial class

public class HumanInputType : InputObjectGraphType<Human>
{
    public HumanInputType()
    {
        Name = "HumanInput";
        Field<NonNullGraphType<StringGraphType>>("name");
        Field<StringGraphType>("homePlanet");
    }
}

But in this form, there is no partial class

var input = new InputObjectGraphType<Human> { Name = "HumanInput", };
input.Field<NonNullGraphType<StringGraphType>>("name");
input.Field<StringGraphType>("homePlanet");

[gao-artur]

we support multiple concurrent schemas on a single server.

Can we use the schema type/name/instance as part of the cache key?

[Shane32]

ParseDictionary is an instance method, so where exactly the cache will be stored?

I was thinking something like this within InputObjectGraphType<T>:

private Func<IDictionary<string, object?>, object> _parseDictionaryCompiled;
public virtual object ParseDictionary(IDictionary<string, object?> obj)
{
    return (_parseDictionaryCompiled ??= ObjectExtensions.CompileToObject(this))(obj);
}

But in this form, there is no partial class

True... Hmm... Is it feasable that source generation could work at all in that scenario? It would have to scan "all" of the code to find what fields were defined with what names, and so on.

Another idea: if we had the above example, perhaps CompileToObject could function like this:

1. Is source generated function for requested implementation type? If yes, return reference to it. (best for AOT scenarios or scoped schemas)
2. Is dynamic compilation enabled for ToObject? If yes, compile and return it. (best for singleton schemas)
3. Return reference to reflection-based ToObject. (necessary for untyped InputObjectGraphType when source generation is not in use)

[Shane32]

we support multiple concurrent schemas on a single server.

Can we use the schema type/name/instance as part of the cache key?

Presently, ScalarGraphType.ParseValue and .ParseLiteral and InputObjectGraphType.ParseDictionary are not passed any context (unlike field resolvers). Neither do any graph types hold a reference to the schema. So, the answer is "not at present", although it's worth discussing.

[gao-artur]

I was thinking something like this within InputObjectGraphType:

You mentioned the memory leak in the context of the scoped registration. In the scoped registration, this cache will be useless.

True... Hmm... Is it feasable that source generation could work at all in that scenario? It would have to scan "all" of the code to find what fields were defined with what names, and so on.

Theoretically - maybe. Practically - need to use dataflow analysis. I don't see myself doing this in the near future 😄

If ValueConverter could be made non-static with per schema registration, probably we could use it.

[Shane32]

You mentioned the memory leak in the context of the scoped registration. In the scoped registration, this cache will be useless.

Oh, I forgot, this is where source generation should really shine. (Updated notes above). Regardless of where the source generation takes place, it should make a huge impact over dynamic compilation in scoped schemas, which is extremely slow for first use, and still be better than reflection. Sure the cache is useless, but so is nearly all instance data for a scoped schema beyond the first execution. Scoped schemas really shouldn't be 'supported', but are too commonplace to remove.

Dynamic compilation's benefits are that it works with any schema configuration, such as dynamic schemas, and can always produce an ideal code path for future executions with no lookups for fastest execution speed.

You know what would be a REALLY cool analyzer - remove scoped dependencies from a graph type and put them into the field resolver. Something to help people transition from a scoped schema to a singleton schema.

[Shane32]

Oh, I forgot, this is where source generation should really shine

What I meant here, was that if source generation was in the InputObjectGraphType<T>, we wouldn't need to bother with the cache at all, and there's no issues whether it's a scoped or singleton schema or anything. It just generates the code to replace the ParseDictionary method. AOT would also benefit in that it would be able to identify all of the uses of T's constructors and properties.

[Shane32]

I'd suggest we proceed along these lines:

1. Rewrite/refactor reflection-based ToObject along lines previously discussed
2. Add CompileToObject (or similar name)
3. Add global switch to enable/disable CompileToObject, default to true (favoring singleton schemas)
4. Change base ParseDictionary method to use CompileToObject if not running in AOT mode and cache it
5. Add source code generator, triggered by the following (or similar):

    [GenerateParseDictionary]
    public partial class abc : InputObjectGraphType<Class1>
    {
    }

//or

    public partial class abc : InputObjectGraphType<Class1>
    {
        [GenerateParseDictionary]
        private partial object BaseParseDictionary(IDictionary<string, object?> dictionary);

        public override object ParseDictionary(IDictionary<string, object?> dictionary)
        {
            var obj = BaseParseDictionary(dictionary);
            // do custom stuff
            return obj;
        }
    }

This should cover all these scenarios:

• Singleton schema uses dynamic compilation; should work for all singleton schemas with no changes to existing user code with 100% compatible guarantee compared to reflection-based code
• Scoped schema runs much slower by default; for best speed it requires (a) turn off dynamic compilation, or (b) use source-generation
• AOT runs same by default, for best speed it requires source-generation
• Source-generation can be used for as many or as few graph types as the developer desires

Perhaps we also add a flag to scan all types for source-generation-eligible types and automatically add ParseDictionary overrides.

[gao-artur]

You know what would be a REALLY cool analyzer - remove scoped dependencies from a graph type and put them into the field resolver. Something to help people transition from a scoped schema to a singleton schema.

Very high risk for false-positive. Think multiple services in the same solution register the same dependency. Finding the registration used by the service exposing GraphQL schema is not trivial.

    [GenerateParseDictionary]
    public partial class abc : InputObjectGraphType<Class1>
    {
    }

You don't really need this attribute. You can use InputObjectGraphType as a marker to detect types that require generation.

    public partial class abc : InputObjectGraphType<Class1>
    {
        [GenerateParseDictionary]
        private partial object BaseParseDictionary(IDictionary<string, object?> dictionary);

        public override object ParseDictionary(IDictionary<string, object?> dictionary)
        {
            var obj = BaseParseDictionary(dictionary);
            // do custom stuff
            return obj;
        }
    }

This will require the user to modify every single input type manually. It's not something that can be just recompiled if the source generator doesn't work in his scenario.

Instead, we can add something like this to the InputObjectGraphType

private Func<IDictionary<string, object?>, object> _parser;

It will be initialized with the default implementation, then the code generator can override it with the generated implementation.

[Shane32]

You know what would be a REALLY cool analyzer - remove scoped dependencies from a graph type and put them into the field resolver. Something to help people transition from a scoped schema to a singleton schema.

Very high risk for false-positive. Think multiple services in the same solution register the same dependency. Finding the registration used by the service exposing GraphQL schema is not trivial.

Yeah. Just thinking that we don't need anything perfect, just helper tools to assist, if possible, with the conversion. No warnings or errors. Consider the following example:

public MyGraphType : ObjectGraphType<Person>
{
    public MyGraphType(IMyService service)
    {
        Field(...).Resolve(ctx => service.Stuff());
    }
}

Perhaps a helper on the service (hold mouse over IMyService service) and it converts "best-effort" as follows:

public MyGraphType : ObjectGraphType<Person>
{
    public MyGraphType()
    {
        Field(...).Resolve(ctx =>
        {
            var service = ctx.RequestServices!.GetRequiredService<IMyService>();
            return service.Stuff();
        });
    }
}

If even it did a basic job like that, it could be extremely useful. The developer then would use the helper for each service in their constructor, and then make any final adjustments manually. So it's a tool-assisted conversion, rather than 100% manual or 100% automatic.

[Shane32]

You don't really need this attribute. You can use InputObjectGraphType as a marker to detect types that require generation.

Well, as of yet, I don't think we should automatically enable source generation. There is going to be issues we have not foreseen with source generation. For example, in the constructor, Field("Test") has a field name of "Test". But during runtime, this gets translated to "test" and will exist in the dictionary as "test". The source generator could assume camel case, but cannot know if the name converter was disabled for the schema prior to its initialization.

A global flag to enable it seems fine.

[gao-artur]

I'd say it should be enabled with msbuild property in csproj file.

[gao-artur]

@Shane32 I started working on this feature. I have a first POC, but much work is yet to be done, and much stuff to learn, but a little free time. I suggest continuing the v8 with the original plan and focusing the v9 solely on AOT support. I expect requests for .NET8.0 TFM very soon. Postponing it until all the Roslyn features are ready can take too long.

[Shane32]

Sounds good. I'm also going to clean up the reflection-based implementation and/or write a compiled-at-runtime implementation.