Roslyn analyzers

[gao-artur]

This is an aggregating issue for Roslyn analyzers, code refactorings, and source generators. The list will be updated with additional suggestions.

Analyzers+CodeFixes

• Rewrite the obsolete Field().Name("name") with Field("name") - Add GraphQL.Analyzers #3697
• Rewrite the obsolete Field(name, description, ...) with Field(name).Description(description)... - Add GraphQL.Analyzers #3697
• Forbid using resolvers on the non-output graph types - Add GraphQL.Analyzers #3697
• Ensure an input field can be mapped to the source type field - InputGraphTypeAnalyzer #3720
• Rewrite the obsolete Connection().Name("name") with Connection("name") - Rewrite the obsolete Name method on ConnectionBuilder #3747
• Rewrite the obsolete Argument<TArgumentGraphType, TArgumentType> with Argument<TArgumentGraphType> - FieldArgumentAnalyzer #3755
• Detect awaitable resolvers when used in sync Resolve* methods and rewrite with Resolve*Async - Analyzers/awaitable resolver analyzer #3763
• Ensure all fields defined by the interface are implemented by the graph type

CodeRefactoring

• Suggest replacing Arguments(new QueryArgument(), ...) method with a chain of .Argument() calls (and back?)

Source generation

• Rewrite ObjectExtensions.ToObject, which uses reflection with a source generator - Rewrite ObjectExtensions.ToObject with source generator #3757

[gao-artur]

@Shane32 I checked all the Obsolete attributes in the code to see if there are any other cases where it's worth creating an automatic code fix. It seems that most of the obsolete methods are simple fixes to make manually, except maybe GraphQLMetadataAttribute.InputType and GraphQLMetadataAttribute.OutputType. Do you think they are widely used and people have many of them in their code?

[Shane32]

No I don’t think so. It was added in #2283 and deprecated a year later in #2879 and has been deprecated for a year and a half. It is probably only used by “type-first” code which isn’t very common anyway.

[Shane32]

And it should be pretty uncommon for type-first code. Like I’m sure I use OutputType a few times in my code, but that would be it. Not worth an analyzer.

[gao-artur]

Great then! I'll look now into code generation for ObjectExtensions.ToObject. Unless you have something else in mind you would want me to prepare for 7.7.

[Shane32]

That sounds great!

[Shane32]

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.)

[Shane32]

I don't really have any other goals for 7.7. So whenever you finish the analyzers or anything else that you think should be included in 7.7, lmk and we can release it. For v8, I'd like to be sure to review, polish and release the federation-related PRs that exist, plus these features:

• Allow document validation rules to easily inspect literal and variable values. This code is half done already.
• Extend InputObjectGraphType field definitions to allow "input resolvers" which can validate and/or change values that are read from literals or variables. This is currently possible via ParseDictionary, but allowing this on a field level will allow attributes to modify validation. There are a number of implementation details we should consider.
• Change all files to use file-scoped namespaces (todo for v8 when v7 is fully complete, to minimize merge issues)

Another feature for v8 we had been considering was alterations so that the field builders were distinct for input and output types, so intellisense would not display field builder methods which are only applicable to output types, and so on.

And then there's also the connections / connection builders. Connections don't work in the AOT sample right now. I'd really like to fix that ... somehow... See #3550 (which accurately identifies that the AOT sample does not work, but in so doing breaks the AOT tests).

[gao-artur]

I don't really have any other goals for 7.7. So whenever you finish the analyzers or anything else that you think should be included in 7.7, lmk and we can release it.

Nothing specifically for 7.7, but I want to add the Detect awaitable resolvers when used in sync Resolve* methods and rewrite with Resolve*Async analyzer before the v8.0. And also, maybe create an analyzer for #3451. Both these cases were broken between v5 and v7. So, it can be helpful for people who haven't upgraded to v7 yet to avoid runtime errors.

Extend InputObjectGraphType field definitions to allow "input resolvers"

Will it also allow mapping names? If so, the InputGraphTypeAnalyzer will need to be changed too.

As for the AOT, I need to learn about it first, but code generation can probably solve all the problems (yeah, my golden hammer from now 😆)

[Shane32]

Another analyzer idea: prevent invalid uses of IGraphType-derived classes, such as: (I did this today)

//valid
GraphQLClrInputTypeReference<string>
//invalid
GraphQLClrInputTypeReference<StringGraphType>

Ditto for GraphQLClrOutputTypeReference<T> and IDataLoaderResult<T>. Unfortunately there's no type constraint that says 'is not IGraphType'. This idea could be expanded to other use cases like return types of FieldBuilder and .Returns<T>().

We do have type constraints for NonNullGraphType<T> and ListGraphType<T> so no analyzer is needed there.

[gao-artur]

Good idea. will do.

[gao-artur]

@Shane32 I can't see analyzer pages on the docs site. Did I miss some configuration?

[gao-artur]

Oh, I see. Need to add an entry to sitemap.yml.

[gao-artur]

@Shane32 I'm reviewing the GQL007 and GQL010. Should we allow internal setters and contractors if the type is also internal. If the source type is internal, it can be used by the input type (which also must be internal) only if they are defined in the same assembly. Then, the generated code won't have any issue accessing such a setter or constructor.

[Shane32]

Well, v7 does not allow internal properties, and for reference System.Text.Json does not normally deserialize to internal properties (see here). v7 did allow internal/private constructors, but I don't think it should have done so.

I would just leave it alone.