Do not register unused graph types #1887

Shane32 · 2020-10-08T06:09:18Z

This PR prevents GraphTypesLookup from registering custom .Net scalar graph types (e.g. DateTimeOffsetGraphType) that are not used by the schema. See #1885

Also allows custom Schema implementations necessary hooks to edit the list of built-in scalar graph types during schema initialization. Not actually necessary to solve #1885 since the UriGraphType would never have been registered if it was not used, so there would be no conflict.

This currently targets version 3.1. Should it target 4.0? The binary is backwards compatible, but generated schemas will not include unused types, so introspection results will differ (even though the missing types are unused by the schema).

randyridge · 2020-10-08T06:29:15Z

To be explicit, what I hit was that going from 2.x to 3.x -- I had my own UriType, the base library added it's own UriType (both having name Uri/UriType, I can't recall). Scanning finds both with the same name and fires exception. In this case UriType was 'used', but it's shape differed. I'll try to file a repro of this tomorrow as well.

sungam3r · 2020-10-08T10:47:40Z

Relates to #1423 .

These changes should be carefully reviewed prior to merging.

sungam3r · 2020-10-08T10:49:41Z

I skimmed through the edits. Yet it seems to me that this issue should proceed differently. I'll be back with an answer later.

Shane32 · 2020-10-08T13:28:27Z

Also see graphql/graphql-spec#222 and graphql/graphql-spec#648

Shane32 · 2020-10-08T13:39:17Z

Faced with a similar problem in my graphql client generator, I came to the conclusion that all custom scalars should be connected externally. The base library should only provide 5 built-in scalars.

Originally posted by @sungam3r in https://github.com/graphql-dotnet/graphql-dotnet/issue_comments/705484550

That's EXACTLY what this PR does. Only the 5 basic scalars are registered with a simple schema. And just like any graph type, additional scalars referenced by the graph are automatically added to the schema. The only difference here is that we have special code to prevent the extra provided scalars (like UriGraphType) from having to be registered with the service provider.

There might be another way to go about it, but I think this is the right answer. We should not include these extra scalars in the schema by default. Nor should we remove them from the library. And there's no reason to force users to register them with their DI provider. Finally, it's not a breaking change -- in the sense that all existing code will work perfectly with this update.

sungam3r · 2020-10-08T13:52:12Z

We should not include these extra scalars in the schema by default. Nor should we remove them from the library.

Well, actually, I think that moving all custom scalars into a separate package is the right way.

And there's no reason to force users to register them with their DI provider.

The reason is to make the library free of implementation assumptions and not create conflicts with custom scalars. Custom scalars are a good example of when you can move a piece of code into a separate package without negative consequences.

It will be breaking change for clients, yes. I think this is a long-awaited technical debt. The emergence of individual packages is a normal evolutionary process.

Shane32 · 2020-10-08T13:55:46Z

I'm going to simplify this PR a bit, and get rid of the other changes...

Shane32 · 2020-10-08T14:07:21Z

Let's assume for the moment that in 4.0 we have custom scalars in a separate nuget package. Is there any reason not to include this change in 3.1? I think we should.

I have removed the API changes, so please re-review.

sungam3r · 2020-10-08T14:11:32Z

Give me 1-2 days.

sungam3r · 2020-10-10T16:00:02Z

It will take me longer. I had to deal with other bugfixes.

sungam3r · 2020-10-13T21:04:02Z

Nice workaround. This is an understandable little change. However, I would like to simplify a few things. I'll be back with another PR.

joemcbride · 2020-10-13T23:16:27Z

src/GraphQL/Types/GraphTypesLookup.cs

@@ -184,6 +189,27 @@ public IEnumerable<IGraphType> All()
            }
        }

+        private void AddType(Type builtInGraphType)


I would suggest perhaps a more distinct name like AddBuiltInType? Too closely named to the other AddType methods I think.

I like this direction. 👍🏼

Good idea! - done. 👍

Let me know if you have any other comments. It's a pretty simple enhancement, and will, I believe, work great all the way 'round.

I already intervened in this PR and changed a lot. This method will no longer exist. I will update the PR and describe the changes in the next hour.

Resolving conflicts 😞

Shane32 · 2020-10-14T04:13:12Z

Nice workaround. This is an understandable little change. However, I would like to simplify a few things. I'll be back with another PR.

I copied from the AddType<T>() and AddType<T>(context) methods to create AddBuiltInType(). If you think I should eliminate duplicated code between the functions, I can add a CreateTypeCollectionContext() method.

Since these new methods and fields are all private, we can refactor at will later also.

sungam3r · 2020-10-14T08:28:05Z

Will update PR soon.

sungam3r · 2020-10-14T08:36:17Z

src/GraphQL/Types/GraphType.cs

@@ -99,7 +99,7 @@ public override bool Equals(object obj)
    }

    /// <summary>
-    /// This sucks, find a better way
+    /// TODO: This sucks, find a better way


I understood this remark, but I don’t know how best to proceed. Maybe later.

sungam3r · 2020-10-14T08:41:50Z

What was done:

Move introspection result into json file. I'm terribly tired of editing it in the source without the ability to highlight syntax and collapse blocks.
Further removal of unused types, even built-in ones. See discussions from Remove unused scalars from SDL #1423 and Clarify note about unused custom scalars graphql/graphql-spec#648
Rework of GraphTypesLookup:
a) Move built-in graph types and introspetion types into static.
b) Eliminate allocations of graph types in Activator.CreateInstance().
c) Eliminate allocations of TypeCollectionContext.

I think that #1423 is no longer needed.

sungam3r · 2020-10-14T08:43:33Z

One consideration against static dictionaries. Types can contain metadata. Then a conflict can arise if the user adds different metadata into the built-in types.

sungam3r · 2020-10-14T08:54:51Z

Switched to instance fields. This will return some allocations, but still there will be fewer of them than before this PR.

sungam3r · 2020-10-14T10:37:01Z

src/GraphQL/Types/GraphTypesLookup.cs

    public class GraphTypesLookup
    {
-        private readonly IDictionary<string, IGraphType> _types = new Dictionary<string, IGraphType>();
+        // Introspection types http://spec.graphql.org/June2018/#sec-Schema-Introspection
+        private readonly Dictionary<Type, IGraphType> _introspectionTypes = new IGraphType[]


All these types are always added to the schema. Each type is created only once within schema instance. Before these changes types were created several times. It is also true for built-in scalars.

sungam3r · 2020-10-14T10:39:51Z

src/GraphQL/Types/GraphTypesLookup.cs

-
-            // Add introspection types.  Note that introspection types rely on the
-            //   CamelCaseNameConverter, as some fields are defined in pascal case - e.g. Field(x => x.Name)
+            _context = new TypeCollectionContext(


TypeCollectionContext is just like a proxy to resolve/add methods pair. It can be shared across all introspection types whilst AddType calls.

sungam3r · 2020-10-14T10:46:31Z

src/GraphQL/Types/GraphTypesLookup.cs

                }
-                if (namedType.IsGenericType)
+                else if (namedType.IsGenericType && (namedType.ImplementsGenericType(typeof(EdgeType<>)) || namedType.ImplementsGenericType(typeof(ConnectionType<,>))))


Perhaps these types should be taken out of here. I've been thinking about this for a long time. Not in thin PR though.

It would be better if upon finding a generic type (e.g. EdgeType<MyModel>), it uses reflection to retrieve the generic version of the type (e.g. EdgeType<>) and then checks _builtInCustomScalars for the generic type. If matched, it will use Activator.CreateInstance to create an instance. Fine as-is though.

Better to make a new issue and link it to #1039

sungam3r · 2020-10-14T10:49:06Z

src/GraphQL/Types/GraphTypesLookup.cs

+                {
+                    type = _builtInScalars.Values.FirstOrDefault(t => t.Name == reference.TypeName);
+                    if (type != null)
+                        this[type.Name] = type;


Example - discovering ID built-in type during schema first for

type Post { id: ID! title: String! } type Query { post(id: ID!): Post }

I don't use schema-first so I'll trust you here :-)

Wouldn't this also need to check the _builtInCustomScalars? Seems like that would be the case everywhere, it would need to check both wouldn't it?

You are right. Fixed.

Shane32 · 2020-10-14T15:18:56Z

Switched to instance fields. This will return some allocations, but still there will be fewer of them than before this PR.

Good. We may want to allow customization of this behavior in a future PR. (Not now.)

Shane32 · 2020-10-14T15:20:14Z

This PR looks good to me.

Shane32 · 2020-10-14T15:20:47Z

(I can't actually approve the PR since I created it)

sungam3r · 2020-10-14T19:20:48Z

src/GraphQL.Tests/Utilities/SchemaBuilderExecutionTests.cs

@@ -430,7 +430,7 @@ public void can_execute_complex_schema()
                }
                type Blog {
                    title: String!
-                    post(id: ID!): Post
+                    post(id: ID!, unused: Long!): Post


One of custom built-in scalars.

sungam3r

In fact, I approve my own changes 😄 .

sungam3r · 2020-10-14T19:49:21Z

This is the last issue in 3.1 milestone. I am very glad that this work has been done. I've wanted to get rid of unused scalars for a very long time, but I've been putting it off all the time. Thanks for the review. Merging.

Shane32 added this to the 3.1 milestone Oct 8, 2020

Shane32 requested a review from sungam3r October 8, 2020 06:09

Shane32 self-assigned this Oct 8, 2020

sungam3r added the needs review label Oct 8, 2020

sungam3r linked an issue Oct 10, 2020 that may be closed by this pull request

Allow to override built-in scalars #1885

Closed

Shane32 mentioned this pull request Oct 10, 2020

Prepare to the next release #1894

Closed

sungam3r mentioned this pull request Oct 10, 2020

Shrink 3.1 milestone #1895

Closed

joemcbride reviewed Oct 13, 2020

View reviewed changes

Shane32 requested a review from joemcbride October 14, 2020 04:00

sungam3r force-pushed the open_graphtypeslookup branch from 6c49d33 to b63ad3d Compare October 14, 2020 08:30

sungam3r reviewed Oct 14, 2020

View reviewed changes

sungam3r mentioned this pull request Oct 14, 2020

Remove unused scalars from SDL #1423

Merged

sungam3r reviewed Oct 14, 2020

View reviewed changes

Shane32 assigned sungam3r Oct 14, 2020

Shane32 and others added 8 commits October 14, 2020 22:16

Do not register unused graph types

5fe617a

Change IReadOnlyCollection to IEnumerable

58c0f4d

Remove old comment

be4dd6a

Revert additional changes

e3085a4

Revert additional change

71c4e4b

Change type on private variable

0c29a5e

rework GraphTypesLookup

005c7bf

fix lookup

de54005

sungam3r force-pushed the open_graphtypeslookup branch from 417582f to de54005 Compare October 14, 2020 19:19

sungam3r reviewed Oct 14, 2020

View reviewed changes

sungam3r approved these changes Oct 14, 2020

View reviewed changes

sungam3r merged commit 37a4c57 into master Oct 14, 2020

sungam3r deleted the open_graphtypeslookup branch October 14, 2020 19:52

Shane32 mentioned this pull request Oct 14, 2020

Pick and mark issues for 3.2 milestone #1917

Closed

Shane32 mentioned this pull request Oct 27, 2020

[Allow to replace built-in scalars] Different behavior in data conversion and serialization between versions 2.4 and 3.1 #1952

Closed

sungam3r mentioned this pull request Oct 27, 2020

Expose Schema.CreateTypesLookup and related members as protected #1953

Closed

Shane32 mentioned this pull request Oct 28, 2020

Make types in GraphTypesLookup non static #1954

Merged

Do not register unused graph types #1887

Do not register unused graph types #1887

Conversation

Shane32 commented Oct 8, 2020 • edited

randyridge commented Oct 8, 2020 • edited

sungam3r commented Oct 8, 2020

sungam3r commented Oct 8, 2020

Shane32 commented Oct 8, 2020

Shane32 commented Oct 8, 2020

sungam3r commented Oct 8, 2020

Shane32 commented Oct 8, 2020

Shane32 commented Oct 8, 2020

sungam3r commented Oct 8, 2020

sungam3r commented Oct 10, 2020

sungam3r commented Oct 13, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Shane32 commented Oct 14, 2020

sungam3r commented Oct 14, 2020

Choose a reason for hiding this comment

sungam3r commented Oct 14, 2020

sungam3r commented Oct 14, 2020

sungam3r commented Oct 14, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sungam3r Oct 14, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Shane32 commented Oct 14, 2020

Shane32 commented Oct 14, 2020

Shane32 commented Oct 14, 2020

Choose a reason for hiding this comment

sungam3r left a comment

Choose a reason for hiding this comment

sungam3r commented Oct 14, 2020

Shane32 commented Oct 8, 2020 •

edited

randyridge commented Oct 8, 2020 •

edited

sungam3r Oct 14, 2020 •

edited