Support of directives on fields and types during introspection #2221

bbakerman · 2021-02-24T06:06:36Z

This allows the Introspection system to get access to the directives on types and fields and other schema elements

Today the graphql spec does not allow you to get this information. You can know the directives in the system but not their usage on schema elements

This optional support class (you have to wire in the code to use it) make the introspection types like this

"An Applied Directive is an instances of a directive as applied to a schema element. This type is NOT specified by the graphql specification presently."
type __AppliedDirective {
  args: [__DirectiveArgument!]!
  name: String
}

"Directive arguments can have names and values.  The values are in graphql SDL syntax printed as a string. This type is NOT specified by the graphql specification presently."
type __DirectiveArgument {
  name: String!
  value: String
}

type __EnumValue {
  appliedDirectives: [__AppliedDirective!]!
  deprecationReason: String
  description: String
  isDeprecated: Boolean!
  name: String!
}

type __Field {
  appliedDirectives: [__AppliedDirective!]!
  args: [__InputValue!]!
  deprecationReason: String
  description: String
  isDeprecated: Boolean!
  name: String!
  type: __Type!
}

type __InputValue {
  appliedDirectives: [__AppliedDirective!]!
  defaultValue: String
  description: String
  name: String!
  type: __Type!
}

"A GraphQL Introspection defines the capabilities of a GraphQL server. It exposes all available types and directives on the server, the entry points for query, mutation, and subscription operations."
type __Schema {
  appliedDirectives: [__AppliedDirective!]!
  description: String
  "'A list of all directives supported by this server."
  directives: [__Directive!]!
  "If this server supports mutation, the type that mutation operations will be rooted at."
  mutationType: __Type
  "The type that query operations will be rooted at."
  queryType: __Type!
  "'If this server support subscription, the type that subscription operations will be rooted at."
  subscriptionType: __Type
  "A list of all types supported by this server."
  types: [__Type!]!
}

type __Type {
  appliedDirectives: [__AppliedDirective!]!
  description: String
  enumValues(includeDeprecated: Boolean = false): [__EnumValue!]
  fields(includeDeprecated: Boolean = false): [__Field!]
  inputFields: [__InputValue!]
  interfaces: [__Type!]
  kind: __TypeKind!
  name: String
  ofType: __Type
  possibleTypes: [__Type!]
  specifiedByUrl: String
}

and hence now you can query for directives applied on schema elements AND the directives arguments values.

The shape of the types was chosen to match the graphql .NET implementation. This means that at least two major graphql implementations have the same shapes.

While not "graphql specification" per se this allows consumers of graphql servers to use introspection to work out what directives are applied to an element.

The spec discussion is here : graphql/graphql-spec#300

bbakerman · 2021-02-25T13:13:41Z

src/main/java/graphql/introspection/Introspection.java

@@ -68,9 +68,10 @@ public static void addCodeForIntrospectionTypes(GraphQLCodeRegistry.Builder code
        codeRegistry.systemDataFetcher(systemCoordinates(TypeNameMetaFieldDef.getName()), TypeNameMetaFieldDefDataFetcher);
        codeRegistry.systemDataFetcher(systemCoordinates(TypeMetaFieldDef.getName()), TypeMetaFieldDefDataFetcher);

-        introspectionDataFetchers.forEach((coordinates, idf) -> codeRegistry.dataFetcher(coordinates, idf));
+        introspectionDataFetchers.forEach(codeRegistry::dataFetcher);


lamda syntax

bbakerman · 2021-02-25T13:14:09Z

src/main/java/graphql/introspection/Introspection.java

@@ -295,7 +296,7 @@ private static String print(Object value, GraphQLInputType type) {
        return null;
    };

-    private static final IntrospectionDataFetcher inputFieldsFetcher = environment -> {
+    private static final IntrospectionDataFetcher<?> inputFieldsFetcher = environment -> {


IntrospectionDataFetcher<?> to stop IDEA warnings

…low __XXX new types

bbakerman · 2021-02-26T01:49:49Z

src/main/java/graphql/introspection/Introspection.java

+                        .name("name")
+                        .type(nonNull(GraphQLString)))
+                .build();
+    }


Moved down from above statics

bbakerman · 2021-02-26T01:51:26Z

src/main/java/graphql/schema/GraphQLSchema.java

+    private static GraphQLDirective[] schemaDirectivesArray(GraphQLSchema existingSchema) {
+        return existingSchema.schemaDirectives.getDirectives().toArray(new GraphQLDirective[0]);
+    }
+


I think this has auto re-arranged the code some how - statics at the top etc..

sungam3r · 2021-02-26T07:27:37Z

Please show the __Directive type definition.

kaqqao · 2021-02-26T15:23:27Z

Ooooh this is lovely 😲 I hope it becomes a thing in the wider GraphQL ecosystem. SDL based tools are terrible, and going back to introspection as the source of truth would be such a relief.

bbakerman · 2021-02-27T04:38:49Z

Please show the __Directive type definition.

https://spec.graphql.org/June2018/#sec-The-__Directive-Type

type __Directive {
  name: String!
  description: String
  locations: [__DirectiveLocation!]!
  args: [__InputValue!]!
}

A query might be

{
            __schema {
                types {
                    name
                     extensions {
                        directives {
                            name
                            args {
                                name
                                extensions {
                                    value
                                }  
                            }                             
                        }
                   }
                   fields(includeDeprecated:true) {
                        name
                        extensions {
                            directives {
                                name
                                args {
                                    name
                                    extensions {
                                        value
                                    }  
                                }                             
                            }
                       }
                    }
                    inputFields {
                        name
                        extensions {
                            directives {
                                name
                                args {
                                    name
                                    extensions {
                                        value
                                    }  
                                }                             
                            }
                       }
                    }
                }
            }
        }
        ```

sungam3r · 2021-02-27T08:31:21Z

So... you use existing __Directive type intended for directive definition in context of applying this directive to schema elements. You did not make a new type that accurately reflects the purpose of the applied directive. Why so? The result is essentially two types glued into one.

Have you considered an option with brand new type that accurately describes the purpose of applied directive? Please see this page for my idea. Only 2 new introspection types and 6 new fields. In your solution I see 4 new introspection types and 4 new fields. Also keep in mind that even __Schema (and even __Directive if you expand the __DirectiveLocation enumeration and modify the parser a little) can have applied directives:

directive @documentation(link: String!) on [many_locations_here]

directive @length(min: Int, max: Int) on INPUT_FIELD | ARGUMENT_DEFINITION @documentation(link: "mysite.com/awesomeschemas/v1/length")

schema MySchema @documentation(link: "mysite.com/awesomeschemas/v1")
{
  query: MyQuery
}

type MyQuery
{
  field(arg: String! @length(10, 50)): String
}

Then you end up with 6 new types and 6 new fields. At the same time, as I understand it, the __Directive type will recursively define itself, because you have not made a separate type for the applied directive.

bbakerman · 2021-02-27T11:25:27Z

@sungam3r - you might be right about the applied directives versus the defined directives.

The reason i did what I did is that I aped what we did in the graphql-java runtime. That is we have a GraphqlDirective class that is built from directive definitions.

And then when directives are applied to schema elements we re-used the same type.

This is a bit painful in one small way - that is the "arguments" of directive have values. No where else in the graphql type system do we have values in a schema context (we except default values - again on arguments)

So we have this in graphql-java

public class GraphQLArgument implements GraphQLNamedSchemaElement, GraphQLInputValueDefinition {
   ...
   
       /**
     * An argument ONLY has a value when its used in a schema definition language (SDL) context as the arguments to SDL directives.  The method
     * should not be called in a query context, but rather the AST / variables map should be used to obtain an arguments value.
     *
     * @return the argument value
     */
    public Object getValue() {
        return value;
    }

This is the design principle we used and this is what we copied.

It has always bugged me in other ways (defined versus applied directives) both in definition and at runtime

I will read your doco and give it some more thought

sungam3r · 2021-02-27T12:24:14Z

OK. I'm glad to see that we understand each other.

This is a bit painful in one small way - that is the "arguments" of directive have values.

Exactly. Indeed directive definition and applied directive (to some element) are different types with different set of fields.

I am very interested in the further development of this PR. Historically, our company has .NET development stack. Recently, due to the expansion, we have received many applications from other teams written in Java.

We are now very close to the GraphQL.NET v4 release. In it, among other improvements, directive introspection will be implemented - documentation page that I post above. If you find the implementation we have made to be acceptable, then that will be great. Nevertheless, we still have time to change something. Waiting for your suggestions. If you choose to go our way with a separate type for the applied directive, then that may move us all closer to standardizing this behavior in the specification because two popular platforms implements introspection directives in the same way.

bbakerman · 2021-02-28T00:57:19Z

Nevertheless, we still have time to change something. Waiting for your suggestions.

So one suggestion I have is to use the extensions/appliedDirectives in your tyes. My reasoning is that then it clear that this is NOT spec and you are very unlikely to get a spec clash when they do decide to do it. If they decide to call it appliedDirectives then you would simply "collapse" then back into the name (and leave the old for backwards compatibility)

bbakerman · 2021-02-28T05:11:13Z

@sungam3r - how about this for a suggestion in order for 2 projects (graphql-java and graphql .net) to be consistent

we will use your __AppliedDirective and __DirectiveArgument types
we will change our naming to use extensions/appliedDirectives on the places directives are allowed (including __Schema which we missed here)
each extensions field is of a type __XXXXExtensions specific to the place where added extensions are - eg __InputValueExtensions or __ArgumentExtensions
you change yours naming to use extensions/appliedDirectives instead of just appliedDirectives

in general @andimarek and myself agree that your specific __AppliedDirective and __DirectiveArgument are more precise. But we are keen to have the extensions field in place to avoid name classes in some future spec and to ape the "extensions" in other places in graphql today

… inside the schema graph correctly

sungam3r · 2021-02-28T09:41:34Z

I considered this option. My thoughts were as follows.

It's not good IMO that you have to create many new __XXXXExtensions types for every existing introspection type (except for enumerations). This "inflates" the schema. That is, if you add 2 more specific types then our choice occurs between 8 new types + 6 new fields vs 2 new types + 6 new fields.
An additional nesting level (extensions field) is created for the property. I see this more as a disadvantage, because such properties will be perceived less naturally, although you can consider this an advantage (it clear that this is NOT spec).
Redundant in meaning between extensions and directives fields. I mean, the directives field already represents some sort of extensions. The applied directive acts as a container for transferring any meta-information (so, extension) to the client.

you are very unlikely to get a spec clash when they do decide to do it.

Well... Honestly, having watched the development of the specification by the working group for a long time (and participated in the review), I generally doubt that in the coming years there will be work in this direction. No offense. Just reality. Better all this time people can work with non-standard parts of the schema in a more natural way. Please note that GraphQL.NET can work in 3 modes. First, experimental features are disabled by default. They need to be turned on. In addition, after turning them on, 2 operating modes are available. I described them on the documentation page that I gave above. All this allows you to maximally maintain backward compatibility with tooling. Also keep in mind that there is also a non-zero chance that the spec might start using the extensions fields (why not?). Therefore, it's a question whose solution will be in conflict with spec.

Next thought. When a new version of the spec is released, it will most likely be tied to the new major version of the application library (at least for GraphQL.NET, for the rest I will not confirm). I agree that code backward incompatibility is completely different from GraphQL request backward incompatibility. However, I thought it was OK if the experimental features were well documented so that people would explicitly turn them on and understand the implications. Thanks for the hint, by the way. I will add a note that in future versions these features may be changed/rolled back/removed in accordance with the evolution of the official specification.

One final note. Whichever option we choose, it still goes "against" the specification, because unknown/new fields and types appear. I put the word against in quotation marks, because I was very surprised that due to the extension of introspection, tooling began to fail 😕 . This shouldn't happen. I find it a significant drawback that the specification does not explicitly say that introspection can be extended (or should not be extended which of course is worse). Tooling would use only that part of introspection (basic) that it can work with.

And one question from me.

type __DirectiveArgument {                 
  name: String!
  value: String      <--- what do you think about this type?
}

I'm still not sure. A directive can have complex argument values. So applied directive can too. For __InputValue.defaultValue spec says http://spec.graphql.org/June2018/#sec-The-__InputValue-Type that

defaultValue may return a String encoding (using the GraphQL language)

I thought the same would apply for the directive argment value. Right?

bbakerman · 2021-02-28T12:02:38Z

Let me think on your words a little more. I take your points about extra types being added. My vision however is BEYOND applied directives, could there be a case for more custom introspection data that a server might do ( for example we run a federated gateway at Atlassian where I work - could we put in more Atlassian specific extensions for our tooling (eg supports ouath or some such??) So the `__XXXExtension` types are there as a well know "holder" place for more extensions beyond what graphql-java might ship.

And one question from me. I'm still not sure. A directive can have complex argument values.

The graphql original team had this problem with default values and they solved it with string encoding. I think there is no other option (except introducing a JSON / Map scalar and I reckon that's just as bad if not worse) Since you CANT know the structure of a directive value any schema for it is going to be convoluted and too meta. The string encoding is a neat (but slightly hacky) way to say "this is graphql values encoded in a string" - tooling would need to decode them This is what I did in our implementation - our `graphql.language.AstValueHelper` is actually a port from graphql-js that is used to make AST node values from objects and then we turn that into a string by printing the AST node via `graphql.language.AstPrinter#printAst(graphql.language.Node)`. So yeah its a string with a special encoding and this idea is already present in standard (graphql-js) graphql

…

On Sun, 28 Feb 2021 at 20:41, Ivan Maximov ***@***.***> wrote: I considered this option. My thoughts were as follows. 1. It's not good IMO that you have to create many new __XXXXExtensions types for every existing introspection type (except for enumerations). This "inflates" the schema. That is, if you add 2 more specific types then our choice occurs between 8 new types + 6 new fields vs 2 new types + 6 new fields. 2. An additional nesting level (extensions field) is created for the property. I see this more as a disadvantage, because such properties will be perceived less naturally, although you can consider this an advantage (it clear that this is NOT spec). 3. Redundant in meaning between extensions and directives fields. I mean, the directives field already represents some sort of *extensions*. The *applied* directive acts as a container for transferring any meta-information (so, extension) to the client. you are very unlikely to get a spec clash when they do decide to do it. Well... Honestly, having watched the development of the specification by the working group for a long time (and participated in the review), I generally doubt that in the coming years there will be work in this direction. No offense. Just reality. Better all this time people can work with non-standard parts of the schema in a more natural way. Please note that GraphQL.NET can work in 3 modes. First, experimental features are disabled by default. They need to be turned on. In addition, after turning them on, 2 operating modes are available. I described them on the documentation page that I gave above. All this allows you to maximally maintain backward compatibility with tooling. Also keep in mind that there is also a non-zero chance that the spec might start using the extensions fields (why not?). Therefore, it's a question whose solution will be in conflict with spec. Next thought. When a new version of the spec is released, it will most likely be tied to the new major version of the application library (at least for GraphQL.NET, for the rest I will not confirm). I agree that code backward incompatibility is completely different from GraphQL request backward incompatibility. However, I thought it was OK if the experimental features were well documented so that people would explicitly turn them on and understand the implications. Thanks for the hint, by the way. I will add a note that in future versions these features may be changed/rolled back/removed in accordance with the evolution of the official specification. One final note. Whichever option we choose, it still goes "against" the specification, because unknown/new fields and types appear. I put the word *against* in quotation marks, because I was very surprised that due to the extension of introspection, tooling began to fail 😕 . This shouldn't happen. I find it a significant drawback that the specification does not explicitly say that introspection can be extended (or should not be extended which of course is worse). Tooling would use only that part of introspection (basic) that it can work with. And one question from me. type __DirectiveArgument { name: String! value: String <--- what do you think about this type? } I'm still not sure. A directive can have complex argument values. So *applied* directive can too. For __InputValue.defaultValue spec says http://spec.graphql.org/June2018/#sec-The-__InputValue-Type that defaultValue may return a String encoding (using the GraphQL language) I thought the same would apply for the directive augment value. Right? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#2221 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAMVWSJW7X7BOK3KIQ7UJPTTBIFVTANCNFSM4YD35AKQ> .

sungam3r · 2021-02-28T14:39:36Z

So yeah its a string with a special encoding

Thanks!

My vision however is BEYOND applied directives, could there be a case for
more custom introspection data that a server might do ( for example we run
a federated gateway at Atlassian where I work - could we put in more
Atlassian specific extensions for our tooling (eg supports ouath or some
such??)

All this fits into the concept of directives. Simplified example:

directive @auth(schema: String!) on FIELD_DEFINITION

type Query {
  field1: String @auth(schema: "Bearer")
  field2: String @auth(schema: "BlaBla")
}

That is, I do not see the point in the existence of such a concept as extensions. Directives are extensions.

bbakerman · 2021-03-19T00:11:20Z

I will be happy to hear your comments about these lines:

/// Note that this class describes only explicitly specified arguments. If the argument in the directive
/// definition has default value and this argument was not specified when applying the directive to schema
/// element, then such an argument with default value will not be returned.

Our implementation will transfer the default value over to the "applied directive" and hence introspection WILL find a value for all defined args (including null for arguments with no default value)

in short every argument for a directive (declared or not) will be presented.

I think default values coming across is what one would generally expect. I acknowledge an argument for "not listing" arguments that are not declared and have no default value. But I dont think we will do it

I is tricky for us to implement (we dont have a separate AppliedDirective class at runtime) and in a simplicity sense I am not sure it's the most simple solution. I think all args being listed is the most simple solution

bbakerman · 2021-03-19T00:12:21Z

I have merged this to our master - this is not to say it cant be revisited before release but we want to get this into early snapshot releases so we can use it at my employer (Atlassian) and also for others to consume in this form

michaelstaib · 2021-03-19T07:19:42Z

The way we implemented it for Hot Chocolate now is that AppliedDirective represents the directive as is in the SDL, so no value coercion has happened. But this shows we should sit down together and hash out an early stage RFC so we all agree on the same thing. I already talke on that during the WG but we are also working on the next release which is why I did not follow up :) sorry for that.

@sungam3r just as an info. We discussed formalizing this issue more into an early stage RFC so that we can work on it and see if this can be adopted for the spec.

I think end of next week I can go back to this and we can hash out a form of collaboration on this.

sungam3r · 2021-03-19T07:26:14Z

OK

bbakerman · 2021-03-26T08:45:59Z

@sungam3r / @michaelstaib

We turned on this PR in a local graphql-java based server inside my company (Atlassian)

The front end team immediately pinged us because all their builds brok?

Why??? Because they used Apollo CLI tools to download schema to generate client side bindings

The Apollo CLI tools use graphql-js (the reference implemementation of graphql) to load up the schema

build	26-Mar-2021 01:29:44	 Error: Command failed: /usr/local/nvm/versions/node/v12.20.0/bin/node /buildeng/bamboo-agent-home/xml-data/build-dir/JF-DEV-CODESTYLESLICE1/scripts/dev/index.js apollo:scheme:download --endpoint=https://api-private.stg.atlassian.com/graphql --schemaPath=/buildeng/bamboo-agent-home/xml-data/build-dir/JF-DEV-CODESTYLESLICE1/graphql-schemas/atlassian-gateway-schema.json
build	26-Mar-2021 01:29:44	Error: Name "__AppliedDirective" must not begin with "__", which is reserved by GraphQL introspection.
build	26-Mar-2021 01:29:44	Name "__DirectiveArgument" must not begin with "__", which is reserved by GraphQL introspection.
build	26-Mar-2021 01:29:44	    at assertValidSchema (/buildeng/bamboo-agent-home/xml-data/build-dir/JF-DEV-CODESTYLESLICE1/node_modules/graphql/type/validate.js:71:11)
build	26-Mar-2021 01:29:44	    at assertValidExecutionArguments (/buildeng/bamboo-agent-home/xml-data/build-dir/JF-DEV-CODESTYLESLICE1/node_modules/graphql/execution/execute.js:136:35)
build	26-Mar-2021 01:29:44	    at executeImpl (/buildeng/bamboo-agent-home/xml-data/build-dir/JF-DEV-CODESTYLESLICE1/node_modules/graphql/execution/execute.js:86:3)
build	26-Mar-2021 01:29:44	    at execute (/buildeng/bamboo-agent-home/xml-data/build-dir/JF-DEV-CODESTYLESLICE1/node_modules/graphql/execution/execute.js:64:63)
build	26-Mar-2021 01:29:44	    at Object.introspectionFromSchema (/buildeng/bamboo-agent-home/xml-data/build-dir/JF-DEV-CODESTYLESLICE1/node_modules/graphql/utilities/introspectionFromSchema.js:31:37)
build	26-Mar-2021 01:29:44	    at Task.task (/buildeng/bamboo-agent-home/xml-data/build-dir/JF-DEV-CODESTYLESLICE1/node_modules/apollo/lib/commands/service/download.js:23:90)
build	26-Mar-2021 01:29:44	
build	26-Mar-2021 01:29:44	    at ChildProcess.exithandler (child_process.js:308:12)
build	26-Mar-2021 01:29:44	    at ChildProcess.emit (events.js:314:20)
build	26-Mar-2021 01:29:44	    at maybeClose (internal/child_process.js:1022:16)
build	26-Mar-2021 01:29:44	    at Process.ChildProcess._handle.onexit (internal/child_process.js:287:5) {

Digging down into the graphql-js code I see this

  // Ensure it is named correctly (excluding introspection types).
    if (!isIntrospectionType(type)) {
      validateName(context, type); //  BB added : for `__` name naming
    }
    
    export const introspectionTypes = Object.freeze([
  __Schema,
  __Directive,
  __DirectiveLocation,
  __Type,
  __Field,
  __InputValue,
  __EnumValue,
  __TypeKind,
]);


export function isIntrospectionType(type: GraphQLNamedType): boolean %checks {
  return introspectionTypes.some(({ name }) => type.name === name);
}

// later


export function isValidNameError(
  name: string,
  node?: ASTNode | void,
): GraphQLError | void {
  devAssert(typeof name === 'string', 'Expected name to be a string.');
  if (name.length > 1 && name[0] === '_' && name[1] === '_') {
    return new GraphQLError(
      `Name "${name}" must not begin with "__", which is reserved by GraphQL introspection.`,
      node,
    );
  }
  if (!NAME_RX.test(name)) {
    return new GraphQLError(
      `Names must match /^[_a-zA-Z][_a-zA-Z0-9]*$/ but "${name}" does not.`,
      node,
    );
  }
}

Sp in other words we have chosen (rightly in my opinion) to use __AppliedDirective as a name but graphql-js, the most common graphql implementation around, including in the front end and back end, has decided that ___Foo names are a closed set and will NOT be allowed

This is a MAJOR blocker to this "common naming scheme" being useful, either in graphql.java or the .Net variants.

I think the graphql-java team we are going to have to re-think this __ naming because of the tooling challenges.

michaelstaib · 2021-03-26T09:04:56Z

That is because of the spec does not allow __ type names, but in this case the validation is wrong since it is introspection :) We have put the feature behind a feature flag and it is disabled by default so most users will not run into this. I will get into touch with Ivan and then we see how we can move forward on this.

michaelstaib · 2021-03-26T09:11:55Z

But interesting enough this means you are using queries on these directive through UI tooling. We mostly are using this in schema stitching and schema federation where our gateway can query directives through introspection..

bbakerman · 2021-03-26T09:13:06Z

We have put the feature behind a feature flag

As did we in a way (we require code optin) but the fact is anyone who runs up a graphql server with this on AND gives it out to front end teams who use client side type generation etc.... that ends up calling graphql-js will fall over on it as the schema being invalid. (Apollo CLI is VERY VERY popular in the front end) So I wanted to call out that "opt in on a server implementation" is not enough because we can control the clients that generate themselves from this opted in schema. Right now my thoughts are that we will drop the `__` bit and call it `AppliedDirectives` so that it can get pass this. This has the drawback that is back into "user type space" and hence could class with user types (as opposed to system types from a system implementation) but it will survive in the wild of the front ends

…

On Fri, 26 Mar 2021 at 20:05, Michael Staib ***@***.***> wrote: That is because of the spec does not allow __ type names but in this case the validation is wrong since it is introspection :) We have put the feature behind a feature flag and it is disabled by default so most users will not run into this. I will get into touch with Ivan and then we see how we can move forward on this. — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#2221 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAMVWSNYNSIPJOGPVFKCZSLTFRE4TANCNFSM4YD35AKQ> .

michaelstaib · 2021-03-26T09:13:06Z

@IvanGoncharov

bbakerman · 2021-03-26T09:15:02Z

But interesting enough this means you are using queries on these

directive through UI tooling. Yeah the front end tooling was trying to * download a schema * generate typescript client bindings to that schema It ends up "running up a schema" via graphql-js code from the downloaded schema.

…

On Fri, 26 Mar 2021 at 20:12, Michael Staib ***@***.***> wrote: But interesting enough this means you are using queries on these directive through UI tooling. We mostly are using this in schema stitching and schema federation where our gateway can query directives through introspection.. — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#2221 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAMVWSLIXXIGBKB33GZ3KYDTFRFWZANCNFSM4YD35AKQ> .

michaelstaib · 2021-03-26T09:15:11Z

Lets wait for input of @IvanGoncharov since there might be an option to get graphql-js on board.

michaelstaib · 2021-03-26T09:16:46Z

This is also why I initially wanted a face to face meeting to discuss this and put it into a proper RFC. We still need to do that otherwise this will be dead before arrival.

sungam3r · 2021-03-26T09:25:15Z

Error: Name "AppliedDirective" must not begin with "", which is reserved by GraphQL introspection.

Yes. Standard introspection type name check as was expected. These errors can be seen in all GraphQL UIs - Playground, GraphiQL, Altair...

We have put the feature behind a feature flag and it is disabled by default so most users will not run into this.

GraphQL.NET has this flag too. Even mode, see https://github.com/graphql-dotnet/graphql-dotnet/blob/master/src/GraphQL/SchemaExtensions.cs#L310 . So UI tooling can still work while server can accept introspection queries with additional fields.

Right now my thoughts are that we will drop the __ bit and call it
AppliedDirectives so that it can get pass this.

IMO it's rather dirty hack.

Also I note that in your company we use our own GraphQL client generator so there are no issues at all for us with additional introspection types/fields.

michaelstaib · 2021-03-26T09:30:51Z

I aggree changing the name would not make this feature more robust, it would just work around the validation. We need to get this properly in or it will not go far.

sungam3r · 2021-03-26T09:33:33Z

IMO the right way would be to "fix" Apollo CLI tools to just ignore/skip unknown types/fields.

sungam3r · 2021-03-26T09:34:39Z

Or make new types known )

IvanGoncharov · 2021-03-26T10:33:58Z

I think we should discuss it online (either WG or a separate call).
As for this particular issue, I suggest just reserve the __Extensions_ (or __EXT_) prefix for introspection extensions.
Otherwise, we will have a name clash when we decide to add something like that into spec.
I also need to think about name clashes of fields.

How about we make a Zoom call on the week after WG?

sungam3r · 2021-03-26T14:35:37Z

I think that applied directive as such should be a first class citizen in introspection. We can talk about extensions but it is additional feature IMO. Applied directives can and should have strongly typed schema. Extensions have always been something like arbitrary map.

bbakerman · 2021-03-26T20:39:01Z

I think we have to two strategies of attack here.

The long term strategy - a WG approved naming schema etc... This is the one that pays of in 6-12 months
The immediate strategy - how would directives on "schema elements" co-exist in todays world with 80% of the client tooling based on graphql-js

The above discussions are rightly about strategy 1 - the long term way for directives to be in schema elements with naming and shapes to be aligned with other Introspection elements.

Regarding strategy 2) we need a way to cope with today.

I notice in the graphql-js this code

const NAME_RX = /^[_a-zA-Z][_a-zA-Z0-9]*$/;

...

if (name.length > 1 && name[0] === '_' && name[1] === '_') {
    return new GraphQLError(
      `Name "${name}" must not begin with "__", which is reserved by GraphQL introspection.`,
      node,
    );
  }
  if (!NAME_RX.test(name)) {
    return new GraphQLError(
      `Names must match /^[_a-zA-Z][_a-zA-Z0-9]*$/ but "${name}" does not.`,
      node,
    );
  }

So it chokes on any name starting with __

but a name like _AppliedDirective will make it pass it today - it smells a bit like an system type

more extreme names could be _x_AppliedDirective or _ext_AppliedDirective or _extension_AppliedDirective to avoid name clashes (I thinking this is probably over kill but I will list it in the interests of idea exploration)

In graphql-java today out opt in to these extensions is via code (not a system flag per se)

You must do this

GraphqlSchema schema = new IntrospectionWithDirectivesSupport().apply(currentSchema)

I am imagining some way to specify a prefix to this like in a strategy 2 work around sense.

GraphqlSchema schema = new IntrospectionWithDirectivesSupport("_xyz" /* prefix */ ).apply(currentSchema)

So to reiterate - I want it to be clear there we (at least the graphql-java team) MUST consider 2 strategies.

I suspect the lead time on WG + graphql-js fix ups is in the order of months - I don't think we can wait that long, because this capability has been needed for a long time and specifically my employer (Atlassian) needs it now.

bbakerman · 2021-03-27T04:57:03Z

BTW:

Playing around with this more I noticed that graphq-java itself wont roundtrip a schema that has __Foo types in it.

graphql.schema.validation.InvalidSchemaException: invalid schema:
"__Foo" must not begin with "__", which is reserved by GraphQL introspection.
	at graphql.schema.GraphQLSchema$Builder.buildImpl(GraphQLSchema.java:647)
	at graphql.schema.GraphQLSchema$Builder.build(GraphQLSchema.java:622)
	at graphql.schema.idl.SchemaGenerator.makeExecutableSchemaImpl(SchemaGenerator.java:106)
	at graphql.schema.idl.SchemaGenerator.makeExecutableSchema(SchemaGenerator.java:79)
	at graphql.schema.idl.SchemaGenerator.makeExecutableSchema(SchemaGenerator.java:52)

So I cant really say that graphql-js is acting badly alone. We obviously copied this (based on text of the error message)

I did try with single _ type names like _Foo and every one is happy with that, that is graphql-js accepts it, apollo cli tooling accepts it and graphql-java accepts it.

So as a work around measure (aka strategy 2) I am keen to go towards a _AppliedDirective work around and then revisit it via a breaking change later if the spec gets updated.

(The breaking change is a loose one as well - that is only people who used fragment type naming would be broken)

kaqqao · 2021-03-27T09:59:24Z

FWIW I've been using the single underscore prefix for SPQR internal directives and their argument types for years with no complaints. So I'm quite confident nothing in the common tool chain is squeamish about such names.

michaelstaib · 2021-03-27T15:36:50Z

@bbakerman So I cant really say that graphql-js is acting badly alone. We obviously copied this (based on text of the error message)

we do the same since the spec is mandating this validation.

http://spec.graphql.org/draft/#sec-Reserved-Names

GraphQL type system authors must not define any types, fields, arguments, or any other type system artifact with two leading underscores.

http://spec.graphql.org/draft/#sec-Schema

All types and directives defined within a schema must not have a name which begins with "__" (two underscores), as this is used exclusively by GraphQL’s introspection system.

http://spec.graphql.org/draft/#sec-Objects

All fields defined within an Object type must not have a name which begins with "__" (two underscores), as this is used exclusively by GraphQL’s introspection system.

and most importantly ... the type validation mandates it:

http://spec.graphql.org/draft/#sec-Objects.Type-Validation

But in this case its the introspection schema which should not be imported by the client anyway. When we export the schema to SDL these new types are not exported since they belong to the introspection.

@IvanGoncharov lets setup a zoom call. I think this is the quickest way to exchange some ideas on this.

michaelstaib · 2021-03-27T15:37:51Z

@kaqqao the single underscore is often used by server implementations for system fields. Like with Apollo Federation.

sungam3r · 2022-09-07T15:25:57Z

By the way regarding #2221 (comment) and broken tools like playground. In our company we have fixed this issue some time ago by patching one JS file from playground scripts. We injected code that adds new types to satisfy validation. Something like that:

<script>
    (() => {
      const freeze = Object.freeze;
      Object.freeze = freezePatch;

      function freezePatch(obj) {
        if (obj && obj.length && obj.length == 8 && obj[0] && obj[0].name && obj[0].name == '__Schema')
        {
          const fieldsValue = {
            name: {
              type: obj[1].getFields().name.type,
            }
          };

          obj.push(new obj[0].constructor({
            name: '__AppliedDirective',
            description: 'Directive applied to a schema element',
            fields: () => fieldsValue,
          }));

          obj.push(new obj[0].constructor({
            name: '__DirectiveArgument',
            description: 'Value of an argument provided to directive',
            fields: () => fieldsValue,
          }));

          Object.freeze = freeze;
        }

        return freeze(obj);
      }
    })();
</script>

bbakerman added 6 commits February 24, 2021 17:06

Initial code for support of directives on field introspection

c88067e

more work on this

3ff18c8

Got it working for schema transform

bfaf367

For andi

8303aa5

Moar for Andi

cc857ee

Seems to be working

be7fcd6

bbakerman commented Feb 25, 2021

View reviewed changes

bbakerman added 2 commits February 26, 2021 01:03

Work on schema helpers

d8aaefb

Now with types per type for extension

f4dce68

bbakerman changed the title ~~WIP - support of directives on fields and type during introspection~~ Support of directives on fields and types during introspection Feb 26, 2021

Put back in the field name validation and fixed up the test to now al…

b9aaf38

…low __XXX new types

bbakerman commented Feb 26, 2021

View reviewed changes

This was referenced Feb 26, 2021

Expose schema directives in introspection #1017

Closed

Expose user-defined meta-information via introspection API in form of directives graphql/graphql-spec#300

Open

andimarek added 2 commits February 28, 2021 19:09

improve schema transformer to deal with strongly connected components…

22b732d

… inside the schema graph correctly

cleanup

76e6b96

cleanup

c961b3f

PR feedback - made things non null

f07eeae

bbakerman merged commit e166c25 into master Mar 19, 2021

andimarek mentioned this pull request Mar 25, 2021

Support schema transformations spring-projects/spring-graphql#40

Closed

skywickenden mentioned this pull request Apr 21, 2021

Feat: Expose user-defined directives via introspection graphql/graphql-js#3047

Draft

andimarek deleted the directives-in-introspection branch May 4, 2021 21:08

michaelstaib mentioned this pull request Feb 28, 2022

Named Lists graphql/graphql-spec#914

Open

Support of directives on fields and types during introspection #2221

Support of directives on fields and types during introspection #2221

Conversation

bbakerman commented Feb 24, 2021 • edited

bbakerman Feb 25, 2021

Choose a reason for hiding this comment

bbakerman Feb 25, 2021

Choose a reason for hiding this comment

bbakerman Feb 26, 2021

Choose a reason for hiding this comment

bbakerman Feb 26, 2021

Choose a reason for hiding this comment

sungam3r commented Feb 26, 2021

kaqqao commented Feb 26, 2021

bbakerman commented Feb 27, 2021

sungam3r commented Feb 27, 2021

bbakerman commented Feb 27, 2021

sungam3r commented Feb 27, 2021

bbakerman commented Feb 28, 2021

bbakerman commented Feb 28, 2021

sungam3r commented Feb 28, 2021 • edited

bbakerman commented Feb 28, 2021 via email

sungam3r commented Feb 28, 2021 • edited

bbakerman commented Mar 19, 2021

bbakerman commented Mar 19, 2021

michaelstaib commented Mar 19, 2021

sungam3r commented Mar 19, 2021

bbakerman commented Mar 26, 2021

michaelstaib commented Mar 26, 2021 • edited

michaelstaib commented Mar 26, 2021

bbakerman commented Mar 26, 2021 via email

michaelstaib commented Mar 26, 2021

bbakerman commented Mar 26, 2021 via email

michaelstaib commented Mar 26, 2021

michaelstaib commented Mar 26, 2021

sungam3r commented Mar 26, 2021

michaelstaib commented Mar 26, 2021

sungam3r commented Mar 26, 2021

sungam3r commented Mar 26, 2021

IvanGoncharov commented Mar 26, 2021

sungam3r commented Mar 26, 2021

bbakerman commented Mar 26, 2021

bbakerman commented Mar 27, 2021

kaqqao commented Mar 27, 2021 • edited

michaelstaib commented Mar 27, 2021

michaelstaib commented Mar 27, 2021

sungam3r commented Sep 7, 2022

bbakerman commented Feb 24, 2021 •

edited

sungam3r commented Feb 28, 2021 •

edited

sungam3r commented Feb 28, 2021 •

edited

michaelstaib commented Mar 26, 2021 •

edited

kaqqao commented Mar 27, 2021 •

edited