ChilliCream / graphql-platform

Welcome to the home of the Hot Chocolate GraphQL server for .NET, the Strawberry Shake GraphQL client for .NET and Banana Cake Pop the awesome Monaco based GraphQL IDE.
https://chillicream.com
MIT License
5.03k stars 724 forks source link

Code organization #196

Closed Dolfik1 closed 5 years ago

Dolfik1 commented 5 years ago

How I should organize my code with this library? If my project have many queries and mutations, how to correctly split it? For example, ASP.NET uses MVC architecture and we can put code in different controllers and areas. I tried to create multiple QueryType, but seems this is not working. Only the last QueryType was created, and others were just skipped.

michaelstaib commented 5 years ago

I think we could allow something like this. I mean registering multiple types during configuration phase and the merge the ones that have matching names. I will talk this over with some of my colleagues just to get a feeling what they would think.

At this moment we do not allow to split them.

Dolfik1 commented 5 years ago

I have another idea for code organization. We can use MVC pattern like in ASP.NET:

public class UsersMutations : Mutations
{
    [Field, Description("Register user")]
    [ReturnType(typeof(UserType))]
    public User RegisterUser(
        [NotNull, StringType] string name,
        [NotNull, StringType] string email,
        [NotNull, StringType] string password)
    {
        // ...            
    }
}

I think this code is not clean, and difficult to implement in the current architecture, but maybe this somehow help.

michaelstaib commented 5 years ago

This almost would work already :)

We will add attribute support with Version 0.6.0. With the final release of Version 0.4.0 we will add a slew of new attributes. But with Version 0.6.0 you will have full attribute support.

Moreover, if you are using attributes you won't have to use any inheritance.

Our vision for this looks more like the following:


[GraphQLName("Mutation")]
public class UsersMutations
{
    [GraphQLDescription("Register user")]
    public User RegisterUser(
        [NotNullType] string name,
        [NotNullType] string email,
        [NotNullType] string password)
    {
        // ...            
    }
}

Today you could already do the following:


[GraphQLName("Mutation")]
public class UsersMutations
{
    [GraphQLDescription("Register user")]
    public User RegisterUser(
        string name,
        string email,
        string password)
    {
        // ...            
    }
}

But in the current version you cannot tell the engine that your parameters are not null types.

michaelstaib commented 5 years ago

The types do not have to be annotated since we can infer those.

michaelstaib commented 5 years ago

But you would still need to register your types with the schema. But your approach just got me thinking to enable a complete new way to write a GraphQL Service API. We could enable a workflow like the current asp.net core where we would infer the schema from these controller types and you would never have to bother about configuring the schema unless you explicitly want to.

michaelstaib commented 5 years ago

202

Dolfik1 commented 5 years ago

I think this approach would be preferable for people already familiar with ASP.NET. Also, this would simplify migration from REST to GraphQL.

michaelstaib commented 5 years ago

Yes, I think this is a really good idea. We, will first focus on closing the last spec gaps with Version 0.4.0 and 0.5.0. After that we will focus on the configuration API.

I originally had three workflows in mind:

  1. Schema-First

    Create your schema with the GraphQL syntax and than bind .net types to it.

  2. Code-First

    Define your schema with csharp.

  3. Mixed-Mode

    Merge schema-first and code-first with parts defined in the graphql syntax and other parts defined in csharp

But know I think we will allow for a 4th workflow where we flavour it like ASP.net. In this workflow you do not have to bother about the schema. You just create your controller like types and we will create from those a schema.

I will talk this through with my colleagues

michaelstaib commented 5 years ago

I will splitt this issue in two issues:

202 ASP.net core like workflow

203 Mergeable types

tdekoekkoekod commented 5 years ago

Personally I favor the schema-first approach. I think we should be thinking in terms of solving GraphQL problems where .NET is just the implementation. Schema-first also allows easier migration should we later decide to use NodeJS or something else. But more to the point I think it helps think in terms of GraphQL first and not force-fitting GraphQL on top of ASP.NET

michaelstaib commented 5 years ago

@tdekoekkoekod we will support multiple styles.

I personally started working on the parser and initially planned only an GraphQL SDL approach. We now support Schema-First and Code-First. Both variants are equally valid. I think it depends from where you come from.

We have layered the api so the asp.net specific implementations are on top and do not interfere with the core engine.

Moreover, the different variants can be mixed.

ronnyek commented 5 years ago

I'd think for ease of providing conventions at least in the non-schema first method, keeping things as simple as possible and actually close to how you do things in MVC seem like they would be ideal (for simplicity and familiarity)

Without being aware of this issue, I'd been thinking about this very thing for being able to easily compose API's, and likewise build out methods in ways that to some degree guard a developer from doing anything they might regret later.

IMO there could be two different methodologies for this to work, and really I don't see why both couldn't be supported. In my opinion, I think where it makes sense, and applicable, re-use System.ComponentModel.Annotations in order to not necessarily pollute POCO's with redundant metadata attributes.

Explicit mapping with strongly typed Query entry point (Similar to what's there today) Generally we always have a single root element (query), and so it seems like it'd be easy enough to take a query POCO object that would contain minimal extra information. What shows up here, is exactly and only what you specify.

Seems like this method could actually be as simple as automatically inferring types of the response objects themselves (and potentially generate ObjectTypes on the fly) e.g.

        public class Query
    {
        public PointOfSale PointOfSale => new PointOfSale();
        public BookLibrary Library => new BookLibrary();
    }

    [Display(Name="PointOfSale", Description = "Point of Sale functionality")]
    public class PointOfSale
    {
        [Display(Description = "Transactions for customer")]
        public IReadOnlyList<Transaction> TransactionsForCustomerId(int id)
        {
            return new List<Transaction>();
        }

        [Display(Description = "Transaction by id")]
        public Transaction Transaction(int id)
        {
            return new Transaction();
        }
    }

    [Display(Name="BookLibrary", Description = "Book library related functionality")]
    public class BookLibrary
    {
        [Display(Description = "Search Books with querystring")]
        public IReadOnlyList<Book> SearchBooks(string search)
        {
            return new List<Book>();
        }
        [Display(Description = "Books belonging to specified genre")]
        public IReadOnlyList<Book> BooksForGenre(string genre)
        {
            return new List<Book>();
        }

        [Display(Description = "Book for corresponding id")]
        public Book Book(int id)
        {
            return new Book();
        }

        [Display(Description = "Book for corresponding ISBN")]
        public Book BookByISBN(int isbn)
        {
            return new Book();
        }
    }

    [Display(Description = "Commerce Transaction")]
    public class Transaction
    {
        // if you dont want to expose custom name or description, need no additional markup
        public int CustomerId { get; set; }
        public string Description { get; set; }
        public decimal TotalAmount { get; set; }
    }

    [Display(Description = "Book information")]
    public class Book
    {

    }

which would produce something like

                             ┌───────┐
                                           │ query │
                                           └───┬───┘
                          ┌────────────────────┴────────────────────┐
                   ┌──────┴──────┐                             ┌────┴────┐
                   │ PointOfSale │                             │ Library │
                   └──────┬──────┘                             └────┬────┘
              ┌───────────┴───────────┐                ┌────────────┼────────────┐
┌─────────────┴─────────────┐  ┌──────┴──────┐  ┌──────┴──────┐  ┌──┴───┐  ┌─────┴──────┐
│ TransactionsForCustomerId │  │ Transaction │  │ SearchBooks │  │ Book │  │ BookByISBN │
└───────────────────────────┘  └─────────────┘  └─────────────┘  └──────┘  └────────────┘

Attribute based "Route" composition I think the idea here would be more in line with what you might see with asp.net mvc. Basically assuming schema/query objects would be composed via a tree generated by using routes.

        [GraphQLRoute("PointOfSale")]
    [Display(Name="PointOfSale", Description = "Point of Sale functionality")]
    public class PointOfSale
    {
        [Display(Description = "Transactions for customer")]
        public IReadOnlyList<Transaction> TransactionsForCustomerId(int id)
        {
            return new List<Transaction>();
        }

        [Display(Description = "Transaction by id")]
        public Transaction Transaction(int id)
        {
            return new Transaction();
        }
    }

    [GraphQLRoute("BookLibrary")]
    [Display(Name="BookLibrary", Description = "Book library related functionality")]
    public class BookLibrary
    {
        [Display(Description = "Search Books with querystring")]
        public IReadOnlyList<Book> SearchBooks(string search)
        {
            return new List<Book>();
        }
        [Display(Description = "Books belonging to specified genre")]
        public IReadOnlyList<Book> BooksForGenre(string genre)
        {
            return new List<Book>();
        }

        [Display(Description = "Book for corresponding id")]
        public Book Book(int id)
        {
            return new Book();
        }

        [Display(Description = "Book for corresponding ISBN")]
        public Book BookByISBN(int isbn)
        {
            return new Book();
        }
    }

Just some thoughts... I personally tend to lean towards the composition pattern, because it seems like it'll be a bit easier to maintain, and slightly more dynamic.

I think both models (regardless of specific conventions with out of box annotations etc, would provide enough information to bootstrap a functional graphql endpoint

michaelstaib commented 5 years ago

Very well written. The one thing I would get rid of is the GraphQLRoute since in GraphQL we infer the route from the graph structure. I like your approach using custom attributes only when we have to. This makes it a very clean solution.

ronnyek commented 5 years ago

I concur... if you use the explicit mapping type scenario (the first one) you wouldn't really need an idea of the graphqlroute, it'd all be explicit... and it'd be a lot simpler to reflect (since you have a finite and specific entry point you can recurse down.

I was digging through the code the other day, and it seemed like there already existed a type register that appears to have had some notion of caching typeinfos... so I've just got a couple followups

I saw an old issue talking about type inference for objects (presumably to generate the equivalent of ObjectType<Someobj> on the fly without needing to explicitly add those things manually), and then in some of the milestone work, a bug about that mapping. Is that work already in progress? (so we could check the registration of graphql entities that represent the entity types).

If not, is there a way to do something like this:

desc.Field(<derived group name>).Type(<set type to some registered/derived type>).Resolver(() => new object());

Seems like we'll need some mechanism that will basically take specified "query" object and reflect down its properties and dynamically create inferred types to be registered with the engine... and ultimately when programatically constructing the ObjectType, referencing said type.

(As a side note, I saw your note about the .Include() in the configuration of an object type, maybe thats a different / better way to go about that).

It'd be nice to at least have the option of not REQUIRING objecttypes to be built for entity types, but it'd be nice to be able to take advantage of the custom configuration. Any thoughts on the idea of maybe providing a public interface like IGraphQLConfigurable, and then when reflecting we could see if class was implementation of IGraphQLConfigurable, and likewise use methods defined in that interface to configure the objecttype, overriding the native / automatic generation? (thought here is a simple interface to implement for POCO's without having to inherit from a specific type)

michaelstaib commented 5 years ago

I saw an old issue talking about type inference for objects

Yes, I have implemented this, but there is a bug which prevents the type inference from walking the type graph.

I will have to look up what the issue was. It was a simple bug. I will look that up.

The idea with this is that you only provide your type (clr-type) like in the following example and the schema builder process will try to infer the schema.

Schema.Create(c => c.RegisterQueryType<Foo>());

The above example just registers Foo as query type and the schema builder will try to infer everything from this type by walking all its edges.

There are some limitations at the moment. Interfaces are not correctly detected since the GraphQL interfaces currently are not associated with the .net interfaces -> this is something that is a todo.

Union types are not existing in .net. So, it is possible to declare union types with the UnionType object. I talked to Mads Torgersen about union types and they do have that on their list for c#. We could come up with something like tagged unions like a OneOf<> type.

var x = OneOf<A,B,C>(new A());  

Moreover, schema can mix and match. So you could provide the schema just with the Foo type but also provide GraphQLTypeDefinitions where the auto-detection has limitations.

Schema.Create(c => 
{
  c.RegisterQueryType<Foo>();
  c.RegisterType<SpecialUnionType>();
});
michaelstaib commented 5 years ago

I think we have quite a view things to do here:

michaelstaib commented 5 years ago

I also was thinking breaking up how schemas are created. Currently we have the schema create-method of the schema. When we scan for types something like a ISchemaBuilder might be better equipped.

michaelstaib commented 5 years ago

Just looked at it and I think this could be a quick win fixing the type inference.

I will write some tests for this which expose the errors and than we could have a look at how we want to go forward with this.

After we have this running we basically need something that discovers all relevant types like with ASP.net the controller discoverer.

michaelstaib commented 5 years ago

So, since we now have the ability to infer a schema from .net types we still need some more hints for the discovery.

With GrpahQL every type can be a nonnull-type with lists this can be even more complicated since we can have a nonnull-list that only allows nonnull-elements.

Should we hint this with en extra attribute?

Variant 1:

[Display(Description = "Search Books with querystring")]
[GraphQLType(typeof(NonNullType<NonNullList<NonNullType<Book>>>))]
public IReadOnlyList<Book> SearchBooks(string search)
{
    return new List<Book>();
}

Variant 2:

[Display(Description = "Search Books with querystring")]
[GraphQLNonNullType]
[GraphQLNonNullElementType]
public IReadOnlyList<Book> SearchBooks(string search)
{
    return new List<Book>();
}

Variant 3:

[Display(Description = "Search Books with querystring")]
[GraphQLNonNullType(NonNullList = true, NonNullElement = true)]
public IReadOnlyList<Book> SearchBooks(string search)
{
    return new List<Book>();
}

Do you have any suggestions/ideas to solve this?

ronnyek commented 5 years ago

I don't have specific input on those either way... I'd just say if there is likely one configuration that would be more common than other, make attributes be required for the other. Might be able to keep entities clean if there is a more typical configuration, by making that the default and overriding with attributes...

eg, if people typically would work with these things and expecte dthem to be nullable etc... I'd argue that by default having them auto map over as nullable unless they provided attribute [NotNull], or if it was the other way, default to not null unless [Nullable()]

namnm commented 5 years ago

Hi guys! I came across to this and found GraphQLDescription but it's not working any more? Any suggestion for something similar so I can easily add description to the field?

michaelstaib commented 5 years ago

@namnm I will have a look at that.

michaelstaib commented 5 years ago

I will close this issue now since with version 0.6.5 we now added a lot of additional features in this area.

We have now the following attributes:

Especially GraphQLResolverAttribute and GraphQLResolverOfAttribute let you organize types much better since with those attributes you can merge types basically.

public class Query 
{
    public string FieldA { get; set; }
}

[GraphQLResolverOf(typeof(Query))]
public class SomeResolvers 
{
    public string FieldB([Parent]Query query)
   {
       return null;
   }
}

would result in the following schema:

type Query {
  fieldA: String
  fieldB: String
}

There are still some gaps like you cannot add directives with attributes at the moment but for that I will open some separate issues.

michaelstaib commented 5 years ago

Also everything that you can do with the attributes now is available through the type api. you can also mix and match with these.