graphql-dotnet / graphql-dotnet

GraphQL for .NET
https://graphql-dotnet.github.io
MIT License
5.83k stars 924 forks source link

making Query and Mutations clean and more maintainable #1249

Closed aammfe closed 3 years ago

aammfe commented 5 years ago

Hello!! We are doing 2 things in StarWarsQuery. for example

Field<HumanType>( "human", arguments: new QueryArguments( new QueryArgument<NonNullGraphType<StringGraphType>> { Name = "id", Description = "id of the human" } ), resolve: context => data.GetHumanByIdAsync(context.GetArgument<string>("id")) );

1- declaring what is query. 2- resolving it

I think StarWarsQuery should do one thing. Declaring it self.

Question is what about resolver ? resolver could be an abstract function in auto generated class. and its arguments would be query arguments.

Motivation 1- StarWars Query is just a declaration. less code , more readability 2- its resolver is some where else 3- in resolver we don't have to do context.GetArgument<string>("id")

joemcbride commented 5 years ago

I agree, and this can currently be done with the "Schema First" approach as shown in the docs:

https://graphql-dotnet.github.io/docs/getting-started/introduction#schema-first-nested-types

The question is, if you prefer the "Graph Type First" approach, how do you wire up the resolver to the field?

All of the pieces are here to support this, it's just a matter of how we want to go about doing it.

aammfe commented 5 years ago

Are you talking about aggregated properties like count ? or full name ?

public class Person
    {
        public string FirstName { get; set; }
        public string LastName { get; set; }
    }

    public class PersonDTO
    {
        public string FirstName { get; set; }
        public string LastName { get; set; }
        public string FullName { get; set; }  //aggregated property   
    }

If yes then

Person query could be in PersonQueryResolver class. If FullNameis non nullable then it there would be auto generated abstract function named GetFullName

otherwise it would be virtual function that bydefault return null

joemcbride commented 5 years ago

Are you talking about aggregated properties like count ? or full name ?

No, I’m talking about every resolver for a GraphQL Type. Resolvers are both properties and methods. How would the user of this framework wire up the C# class that represents the resolvers to the GraphQL definition?

You’re suggesting to have two different classes (one for the definition, one for resolvers), though you haven’t described how you would actually pair them together. That’s the hard part. The resolver classes can’t be auto generated because that is the code the user of the framework needs to write.

aammfe commented 5 years ago

Ok now I understand. good Question !!

using DI , replacing PersonAutoGeneratedResolverwith PersonResolver

or some simpler DI solution.

aammfe commented 5 years ago

and bcs we are generating personAutoGeneratedReaolver we know what is the query for it..

aammfe commented 5 years ago

an other thing.

we can even generate extended classes as well.

even partial class which has a static method to register it in DI.

while generating just check if base class has been extended or not. if not then generate extended one.

but user has to tell in which assembly it should generate.
base and extended class could have different assemblies.

aammfe commented 5 years ago

Are you talking about aggregated properties like count ? or full name ?

No, I’m talking about every resolver for a GraphQL Type. Resolvers are both properties and methods. How would the user of this framework wire up the C# class that represents the resolvers to the GraphQL definition?

You’re suggesting to have two different classes (one for the definition, one for resolvers), though you haven’t described how you would actually pair them together. That’s the hard part. The resolver classes can’t be auto generated because that is the code the user of the framework needs to write.

I think this method is lot cleaner and less code for user , don't need to register it afterwards. and we could have attributes like MVC on it


       interface IResolver
        { }

        class BasePersonResolverAutoGenerated<DrivedType> : IResolver
           where DrivedType : IResolver
        {
            static void RegisterInDI(IDIService dI)
            {
                dI.Register<BasePersonResolverAutoGenerated<DrivedType>, DrivedType>();
            }
        }

        [NotActive]
        class PersonResolver : BasePersonResolverAutoGenerated<PersonResolver>
        {
        }
sungam3r commented 3 years ago

I think question was answered here https://github.com/graphql-dotnet/graphql-dotnet/issues/1249#issuecomment-522173683

@aammfe Also look into #1376 . I am closing this issue. If you still have a need for some missing features, then feel free to open a new task.