Graphql-dotnet: Reduce the Public API

[Grauenwolf]

I'm seeing a lot of utility methods that probably shouldn't be part of the public API. I'm especially concerned about StringExtensions. Most people have a string extensions library already, so putting this in the GraphQL namespace is going to cause them problems.

Also, it's just makes maintenance harder because people start relying on stuff that they really shouldn't.

I recommend having someone do a pass through the code, changing as much as possible to internal instead of public. (You'll also need to add InternalsVisibleTo in order to not break the unit tests.)

[drobertson123]

I would second this and possibly go a bit beyond.

Version 2.0 would be a good time to define what is and what isn't part of GraphQL and also set some ground rules for extensions, API's, etc. A basic philosophy document that makes it clear for contributors how things should be done and why they are done that way.

I really appreciate GraphQL. It is a great project, but it is developed in a confusing mix of C# and javascript conventions. That isn't surprising considering where it came from, but I think it leads to confusion and makes it challenging for people to contribute to the project.

A basic developers bible with clear rules for how GraphQL is built would go a long way towards getting more help for this project. Other more traditional projects may get away without doing this, but graphql-dotnet is a challenge to dive into.

I don't think many of us came from the javascript/node world looking for a C# implementation of the GraphQL javascript example project. I know my motivation was that I wasn't strong in javascript/node but I wanted to find a way to implement a GraphQL server in the C# and ASP.NET core world I was familiar with.

This has made understanding GraphQL-dotnet much harder than other projects. There is an impedance mismatch between what I expect to see and the way this follows some javascript idioms. Over time I have learned but it was a steep curve in places.

GraphQL-dotnet is hitting it's 2.0 phase now. Establishing a clear vision of how the library works and is developed will make it much easier for others to help and encourage adoption.

Just some thoughts.

[Grauenwolf]

I'm torn here. I want to wait until this is done so I only have to document the public API. But I think documenting the public API will make this effort more successful.

[joemcbride]

@drobertson123 Thanks for you thoughts and I agree that lack of documentation has been an issue.

confusing mix of C# and javascript conventions

Can you point out some specifics? Admittedly I did follow the JS and Java projects quite a bit, though I'm not sure what aspects don't "feel like C#". Living in both communities and being the person that wrote it I need specifics to be able to make this better.

[Grauenwolf]

This looks promising: https://marketplace.visualstudio.com/items?itemName=VisualStudioPlatformTeam.MicrosoftCodeAnalysis2017

For .NET Framework projects I always use FXCop (a.k.a. VS code analysis) to ensure that my APIs actually look like .NET APIs.

[Grauenwolf]

Scratch that, this its probably a better choice. https://www.nuget.org/packages/Microsoft.CodeAnalysis.FxCopAnalyzers/

I'll do a pass when I have some free time. A lot of the things it flags are stuff you probably wouldn't catch like exposing a public field instead of a public property. (Does that distinction even exist in JavaScript?)

[Grauenwolf]

I did a trial pass with FxCopAnalyzers and I think this will definitely move us in the right direction.

However, we definitely need to fix the warnings from FxCopAnalyzers one at a time and suppress the rest. Otherwise we're just going to get completely overwhelmed.

[pekkah]

So it's this https://github.com/dotnet/roslyn-analyzers/blob/master/README.md

[Grauenwolf]

Yep

[Grauenwolf]

First pass: https://github.com/graphql-dotnet/graphql-dotnet/pull/553

[joemcbride]

Just as a forewarning, there is about a 99% chance that I won't be pulling in any PRs that add FX Cop or anything similar. I'll admit that some of the APIs could be better, though most of the difficulty I've seen people have is with learning GraphQL itself. I would prefer to get direct feedback from users of the library vs. a code analyzer. There have been a few threads like this that distinctly lack feedback on specific APIs and what was confusing other than "make it better". There's not much I can do with that.

[jquense]

Yes please, let's keep api churn down unless it's making a practical improvement based on specific feedback :)

[Grauenwolf]

That's why I turned off most rules in the sample PR and focused just on internal stuff like reducing unnecessary memory allocations. There are a lot of CA rules that offer incremental improvements without changing the public API.

[drobertson123]

@joemcbride Sorry for the slow response. It has been a hell week.

I think one of the main differences is how graphql-dotnet feels like layer after layer of object resolvers. Each piece of the system requires multiple calls to look up an object type, or converter or handler. In fact, it feels like the whole system is an intricate IoC mapping scheme.

There is absolutely nothing wrong with this and it is one of the reasons why it can be so powerful, but this approach is mind bending when you are coming from a more traditional hierarchical object-oriented world. I know that made me feel like I was heading down Alice's rabbit hole when I first started.

It is a bit strange to think that I am not making a call to a specific class, but instead making a call to one class implementation out of many possible ones that will be there when I need it and is based on the resolvers finding the right one based on the type of data. You can do this in C#, but it is far from what you normally expect in .NET code.

The next one I actually like a lot and have started using, but it is an uncommon way of doing things in C#. Using generics as a way to pass a type as a variable to be used in a method. Example;

Field<ObjectType>();

Normally variables would be passed as parameters of the method and the Generic Type would define some object that acts on those variables. In this case, the Generic Type is acting as the variable itself. It is specifically what you want to pass in.

Field<ObjectType>() is also an example of something that was confusing for a different reason. This is probably because I have programmed in one way for such a long time that changes hurt my brain. For the longest time, I couldn't figure out what you were doing with all those Nouns in the constructor.

I had always been taught that you used Nouns for properties and Verbs for methods. It was such a simple thing, but I couldn't get my head around the fact that Field<ObjectType>()was actually adding the object type to the collection. In my mind Field is something and AddField does something.

Now that I work in Javascript more I see these things everywhere, but they tripped me up a lot as I was trying to figure out what is going on.

One thing that would help is an introduction to the flow of how the whole system works. Including the concept of the resolvers and how everything works together. When you are first dealing with graphql-dotnet it is hard to get your head around some of those things. The nearest concept is a DI/IoC container and not everyone is familiar with using those.

Obviously, these are my perspectives. Other people will have different views.

Over time I have come to really like graphql-dotnet and we are using it at the core of a major project. Hopefully smoothing out some of these things and adding documentation that clarifies the basic concepts will help more people use it and contribute to the project.

[johnrutherford]

Like @drobertson123 it took me a bit to wrap my head around how to build one's graph types. In particular, I was confused about the different approaches. The schema + class and attributes versus creating a class that derived from ObjectGraphType<T>. I didn't think having my schema as a string was going to be maintainable for a real application, so I couldn't use that. And it was a little confusing at first figuring out the different options for creating fields using Field().

I think something more convention-based with a more typical class structure would be more intuitive. Something like this:

[GraphQLType("User")] // Optionally used to override type name
public class UserType : ObjectGraphType<User>
{
    // All public properties and methods are exposed by default
    // Field name and graph types are inferred
    public string FirstName => Source.FirstName;

    [Description("The User's first name")]
    public string LastName => Source.LastName; // Source is a property of type T

    [GraphQLField(NonNull = true)] // Attribute is only necessary to override inferred parameters
    public string UserName => Source.UserName;

    [GraphQLField(DeprecationReason = "Use the UserName field instead")]
    public string Email => Source.Email;

    [GraphQLField("Orders")] // Override field name
    public Task<IEnumerable<Order>> GetOrders()
    {
        ...
    }
}

So this has the advantage of using composition to explicitly expose the fields that you want, but is more declarative and feels more like a normal class.

[kamsar]

When I picked up graphql-dotnet I found most of my issues were related to needing to learn GraphQL more than the library conventions. GraphQL itself springs from JS and does things in a more JS-ey fashion overall, so there is some adaptation necessary for folks familiar with C# idioms. Resolvers and how dynamically typed a lot of things are comes to mind. I don't think that's a problem, but it is an educational opportunity.

In my implementation, I'm building a schema dynamically based on a database so having the ability to define types directly from ObjectGraphType<T> has been amazing (especially being able to make a schema out of object instances - IoC isn't always the way) - whereas a convention-based approach like this would have made my schema generation impossible. Admittedly mine is probably a less common use-case, but there are places for both a convention-based and more raw API.

[AndrewRissing]

@joemcbride

Thanks for the library, but I'd like to point out this change is more than an enhancement as well. Since you're leveraging DI throughout the code, all of the public properties/fields you have exposed are fair game for a DI to go in and populate.

I just spent a decent amount of time tracking down a bug that was attributed to such behavior, because the ObjectGraphType had a public property that the DI happily recognized and replaced.

If you don't expect someone to monkey with something outside of your internal code, I would highly stress that you shield it from the outside world.

Thanks. :)

[joemcbride]

I just spent a decent amount of time tracking down a bug that was attributed to such behavior, because the ObjectGraphType had a public property that the DI happily recognized and replaced.

Can you please provide what happened and what your expectation was? From your comment I have no idea if that is expected behavior or not. Need the specifics to be of any help or to fix it.

[AndrewRissing]

@joemcbride Sorry, I'll be more specific. The ObjectGraphType.IsTypeOf was being matched by my DI and replaced with a different function. Why my DI has that is another story...

Either way, if you were to hide away all of the public properties/fields used only internally, it might prevent others from having to dig through things like I had to.

[joemcbride]

@AndrewRissing That one is meant to be public and should stay public, so unfortunately no help there. ☹️

https://graphql-dotnet.github.io/getting-started/#istypeof - if you haven't seen it.

[AndrewRissing]

@joemcbride Ah, I see. Np. Thanks for the link. I've got a clean enough workaround for now.

[sungam3r]

This issue was published almost 2 years ago and applied to version 2 of the package. Now in a state of preview for a long time is version 3. I suspect that much has changed since then. I see that this issue actually consists of several parts:

1. Reduce the public API surface.
2. Use some code rules and style across all repo (analyzers may help).
3. Lack of documentation: schema first, graph type first approaches, etc.
4. The difficulty of quickly entering GraphQL patterns and technology.

The first task is too general. I can’t imagine that it’s so easy to walk around the entire repository and, based on some conclusions, remove some part of the public APIs. If this is still relevant for someone, then let's focus on specific APIs and discuss solutions.

The second. In #553 @joemcbride made it clear that he was not interested in such a solution. It should be noted that I myself actively use code analyzers, including FxCopAnalyzers. It seems to me (I no longer remember) that we also discussed the advisability of adding these analyzers. Ultimately, we agreed that it would be better to add editorconfig file - see #1361 and #1438 . Let's focus on this solution. There is still something to discuss.

The third. In fact, there is documentation and examples. Perhaps two years ago this really was nothing. The readme in this repository contains many useful links. However, I agree that there are many documentation related issues. PRs are welcome!

The last. What can I say? Any technology requires study - somewhere more, somewhere less. I myself had to go a long way from misunderstanding and rejection to comprehension and love for GraphQL. I do not think that this problem has a “solution” at all. You yourself need to learn to think in new terms (GraphQL specific, not C# specific). Otherwise, documentation can help (see point 3).

@Grauenwolf , @drobertson123, @jquense , @johnrutherford , @kamsar, @AndrewRissing Thus, I think we need to close this issue in favor of more focused ones.

[jasonkolodziej]

Like @drobertson123 it took me a bit to wrap my head around how to build one's graph types. In particular, I was confused about the different approaches. The schema + class and attributes versus creating a class that derived from ObjectGraphType<T>. I didn't think having my schema as a string was going to be maintainable for a real application, so I couldn't use that. And it was a little confusing at first figuring out the different options for creating fields using Field().

I think something more convention-based with a more typical class structure would be more intuitive. Something like this:

[GraphQLType("User")] // Optionally used to override type name
public class UserType : ObjectGraphType<User>
{
    // All public properties and methods are exposed by default
    // Field name and graph types are inferred
    public string FirstName => Source.FirstName;

    [Description("The User's first name")]
    public string LastName => Source.LastName; // Source is a property of type T

    [GraphQLField(NonNull = true)] // Attribute is only necessary to override inferred parameters
    public string UserName => Source.UserName;

    [GraphQLField(DeprecationReason = "Use the UserName field instead")]
    public string Email => Source.Email;

    [GraphQLField("Orders")] // Override field name
    public Task<IEnumerable<Order>> GetOrders()
    {
        ...
    }
}

So this has the advantage of using composition to explicitly expose the fields that you want, but is more declarative and feels more like a normal class.

So I am just going to assume that this was not implemented?? Can I please get a confirming statement?

It does seem quite counter intuitive that I cannot just define my Fields and what "GraphType" my C# object is.

Even @kevin-carroll has something similar to what is stated... https://github.com/graphql-aspnet/graphql-aspnet

Going to see if I make more of a ruckus with all my reflective ObjectGraphTypes and normal classes.

To my best extent so far is having to basically make turn my object as its own namespace.

namespace XTools.Connection
{
sealed public partial class Connection
{
 /// defined C# attributes, private methods, etc.
 /// ...
}
}
/// Type.cs
namespace XTools.Connection
{
sealed public partial class Connection : ObjectGraphType<Connection>
{
 public Connection()
{
   // Fields declared
}
}

Very time intensive but... couldn't think of any better way without writing a brand new graphql library.

[sungam3r]

So I am just going to assume that this was not implemented??

What do you mean _this_ exactly?

[jasonkolodziej]

So I am just going to assume that this was not implemented??

What do you mean _this_ exactly?

Using Attributes as a means to declaring what a C# classes attributes would mean to the graphql library.
Similar to what you would do if you are using Newtonsoft.Json to define C# class attributes as a property in a JSON object on serialization.

[sungam3r]

1. https://graphql-dotnet.github.io/docs/getting-started/introduction#schema-first-approach for attributes.
2. https://graphql-dotnet.github.io/docs/getting-started/introduction#graphtype-first-approach for graph types.

Also you may find usefull conventions project.

[sungam3r]

If you are missing some feature, then now (New Year holidays) is the good time to describe it well and ask to help.
Or you can do it yourself of course. PRs are welcome.