Parameterized introspection queries #2993

MayCXC · 2022-10-24T20:08:29Z

graphql-js has a number of parameters for generating introspection queries that include deprecated input fields, __Schema.description, __Directive.isRepeatable, __Type.specifiedByUrl, etc.. This branch adds support for these parameters, plus an arbitrary TypeRef depth, without changing the result of IntrospectionQuery.INTROSPECTION_QUERY. Using the AST rather than string formatting to generate the introspection query makes the class about twice as verbose, but offers much more control.

Two things to consider during review:

__Type.specifiedByUrl is resolved by __Type.specifiedByURL in my graphql-java projects, there were test cases and introspection datafetchers using specifiedByUrl, that I updated to match the current spec's capitalization.
__Directive.args(includeDeprecated: Boolean) causes a validation error in those projects when I pass includeDeprecated: true, even though the argument shows up in introspection.

bbakerman · 2022-10-27T03:57:09Z

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

                    .type(typeRef("__Type")))
            .field(newFieldDefinition()
-                    .name("specifiedByUrl")
+                    .name("specifiedByURL")


This is wrong in both cases

https://spec.graphql.org/draft/#sec--specifiedBy

directive @specifiedBy(url: String!) on SCALAR

It probably should be specifiedBy as a field name

Even if we go with this new name - it a breaking change - anyone using it today would be broken

We need to add a new field and deprecate the old if we changed.

What does graphql-js do here?

This is wrong in both cases

https://spec.graphql.org/draft/#sec--specifiedBy

directive @specifiedBy(url: String!) on SCALAR

It probably should be specifiedBy as a field name

unless I am mistaken, the field is the one here: https://spec.graphql.org/draft/#sec-Schema-Introspection.Schema-Introspection-Schema

Even if we go with this new name - it a breaking change - anyone using it today would be broken

We need to add a new field and deprecate the old if we changed.

What does graphql-js do here?

they currently only support the specifiedByURL field: https://github.com/graphql/graphql-js/blob/main/src/type/introspection.ts#L256 but expressed the same concern here: graphql/graphql-js#3156 . I see no issue with keeping specifiedByUrl and possibly deprecating it, similar to how you extended default introspection behavior with IntrospectionWithDirectivesSupport. But I see no similar approach in graphql-js.

Let's put both fields in, deprecate the old and laugh about it on 20 years

bbakerman

We need a a new class here to do this Introspection building and it needs to use a Options pattern

bbakerman · 2022-10-27T03:58:13Z

src/test/groovy/graphql/GraphQLTest.groovy


        then:
-        result == [__type: [name: "MyScalar", specifiedByUrl: "myUrl"]]
+        result == [__type: [name: "MyScalar", specifiedByURL: "myUrl"]]


I am not sure how this relates to the change you have proposed

this was just to keep the test passing. if you would like to keep support for the old field, it can be reverted.

bbakerman · 2022-10-27T03:59:13Z

src/main/java/graphql/introspection/IntrospectionQuery.java

 @PublicApi
-public interface IntrospectionQuery {
+public interface IntrospectionQuery { // todo iwds support
+    static <T> List<T> filter(T... args) {


private static - is that a thing in interfaces - we would not want this helper exposed

it is not, but adding a builder class will fix this.

graphql-js chose this naming

export interface IntrospectionOptions { /** * Whether to include descriptions in the introspection result. * Default: true */ descriptions?: boolean; /** * Whether to include `specifiedByURL` in the introspection result. * Default: false */ specifiedByUrl?: boolean; /** * Whether to include `isRepeatable` flag on directives. * Default: false */ directiveIsRepeatable?: boolean; export function getIntrospectionQuery(options?: IntrospectionOptions): string {

we could ape that

I've added an options builder with the fields on this interface: adafa6a#diff-77fac6499b3cfc7d87aebc6bc02e3ad3439fae0f5e549d5b5d39eaf09a052781R35

bbakerman · 2022-10-27T04:08:08Z

src/main/java/graphql/introspection/IntrospectionQuery.java

+        boolean schemaDescription,
+        boolean inputValueDeprecation,
+        int typeRefFragmentDepth
+    ) {


A 6 parameter method is a terrible interface for generating introspection queries

getIntrospectionQuery(true, false, true, false, true, 7);

Looking at that code its near impossible to get signal on what you are getting - and Java doesnt have named parameters which makes it worse

I think this needs a builder object (aka options). See graphql.schema.idl.SchemaPrinter.Options for inspiration

I also think this should go into a class (not the interface)

Hence this might be

String INTROSPECTION_QUERY = IntrospectionQueryBuilder.build(IntrospectionQueryBuilder.defaultOptions())

a great example of how IntelliJ has spoiled me, I can surely improve this with a builder.

bbakerman · 2022-10-27T04:10:10Z

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

                    .type(typeRef("__Type")))
            .field(newFieldDefinition()
-                    .name("specifiedByUrl")
+                    .name("specifiedByURL")


Even if we go with this new name - it a breaking change - anyone using it today would be broken

We need to add a new field and deprecate the old if we changed.

What does graphql-js do here?

bbakerman · 2022-10-27T05:17:50Z

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

            .name("__TypeKind")
            .description("An enum describing what kind of type a given __Type is")
-            .value("SCALAR", TypeKind.SCALAR, "Indicates this type is a scalar. 'specifiedByUrl' is a valid field")
+            .value("SCALAR", TypeKind.SCALAR, "Indicates this type is a scalar. 'specifiedByURL' is a valid field")


export interface IntrospectionScalarType { readonly kind: 'SCALAR'; readonly name: string; readonly description?: Maybe<string>; readonly specifiedByURL?: Maybe<string>; }

So it seems that graphql-js chose the name specifiedByURL and we chose specifiedByUrl

(Which is ironical because @andimarek drove this change in spec terms)

I think we should align to the reference implementation in this case - since it drives tooling behavior

But for backwards compat - we should "add" a new field and not change its name

will adafa6a#diff-eba216571550152c40136483beff9a41311df7b7681264e65d8d0e7c866cff7eR403 suffice? I could revert or add new tests for it as well.

bbakerman · 2022-10-27T05:18:24Z

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

                    .type(typeRef("__Type")))
            .field(newFieldDefinition()
-                    .name("specifiedByUrl")
+                    .name("specifiedByURL")


As above - add a new field and deprecate the old

…ecifiedByUrl

bbakerman · 2022-10-27T06:10:55Z

src/test/groovy/graphql/introspection/IntrospectionTest.groovy

+        then:
+            parseExecutionResult(allTrueExecutionResult).every()
+            allTrueExecutionResult.data["__schema"]["types"].find{it["name"] == "Query"}["fields"].find{it["name"] == "tenDimensionalList"}["type"]["ofType"]["ofType"]["ofType"]["ofType"]["ofType"]["ofType"]["ofType"]["ofType"] == null // typeRefFragmentDepth is 7
+    }


To be sure that IntrospectionQuery. INTROSPECTION_QUERY actualyl equals what it used to eqal can we hard code a test here

It would copy the OLD string value here and then check that the new generated constant is the same

There are differences/improvements in the spacing and formatting of the string produced by the AST printer. Perhaps we can trim all whitespace and compare, or parse the old query with SchemaParser and reprint it?

I've verified that the queries are equivalent via diff and added a test that verifies they are equal ignoring whitespace.

…tring

bbakerman · 2022-10-29T03:38:06Z

src/test/groovy/graphql/introspection/IntrospectionTest.groovy

+        then:
+            oldIntrospectionQuery.replaceAll("\\s+","").equals(
+                newIntrospectionQuery.replaceAll("\\s+","")
+            )


* use ast builders and stub test * remove main and add test * restore jvm check * revert test spacing * fix test * revers build.gradle * use spec specifiedByURL capitalization * update another specifiedByURL test * refactor introspection query builder, restore and deprecate __Type.specifiedByUrl * test that AST printed introspection query is equivalent to original string

aaron_paterson added 4 commits October 21, 2022 17:37

use ast builders and stub test

1b0d164

remove main and add test

966309e

restore jvm check

f64f1e6

revert test spacing

438ce16

MayCXC marked this pull request as draft October 24, 2022 20:16

aaron_paterson added 2 commits October 24, 2022 14:21

fix test

f0ee674

revers build.gradle

887a9c7

MayCXC marked this pull request as ready for review October 24, 2022 20:21

use spec specifiedByURL capitalization

2b1b786

MayCXC mentioned this pull request Oct 25, 2022

[Feature request] parameters for introspection queries #2995

Closed

update another specifiedByURL test

e0adefe

bbakerman reviewed Oct 27, 2022

View reviewed changes

bbakerman requested changes Oct 27, 2022

View reviewed changes

bbakerman reviewed Oct 27, 2022

View reviewed changes

refactor introspection query builder, restore and deprecate __Type.sp…

adafa6a

…ecifiedByUrl

bbakerman reviewed Oct 27, 2022

View reviewed changes

test that AST printed introspection query is equivalent to original s…

3cef2a5

…tring

MayCXC requested a review from bbakerman October 28, 2022 14:53

bbakerman reviewed Oct 29, 2022

View reviewed changes

bbakerman approved these changes Oct 29, 2022

View reviewed changes

bbakerman added this to the 20.0 milestone Oct 29, 2022

bbakerman merged commit bb4a3b2 into graphql-java:master Oct 29, 2022

Parameterized introspection queries #2993

Parameterized introspection queries #2993

Uh oh!

Conversation

MayCXC commented Oct 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bbakerman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bbakerman Oct 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MayCXC Oct 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

MayCXC commented Oct 24, 2022 •

edited

Loading

bbakerman Oct 27, 2022 •

edited

Loading

MayCXC Oct 27, 2022 •

edited

Loading