Skip to content

Parameterized introspection queries #2993

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
bbakerman merged 10 commits into from
Oct 29, 2022
Merged

Parameterized introspection queries #2993

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
1b0d164
use ast builders and stub test
Oct 21, 2022
966309e
remove main and add test
Oct 24, 2022
f64f1e6
restore jvm check
Oct 24, 2022
438ce16
revert test spacing
Oct 24, 2022
f0ee674
fix test
Oct 24, 2022
887a9c7
revers build.gradle
Oct 24, 2022
2b1b786
use spec specifiedByURL capitalization
Oct 24, 2022
e0adefe
update another specifiedByURL test
Oct 25, 2022
adafa6a
refactor introspection query builder, restore and deprecate __Type.sp…
Oct 27, 2022
3cef2a5
test that AST printed introspection query is equivalent to original s…
Oct 27, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 9 additions & 3 deletions src/main/java/graphql/introspection/Introspection.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ public enum TypeKind {
public static final GraphQLEnumType __TypeKind = GraphQLEnumType.newEnum()
.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")
Copy link
Member

@bbakerman bbakerman Oct 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

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

Copy link
Contributor Author

@MayCXC MayCXC Oct 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

.value("OBJECT", TypeKind.OBJECT, "Indicates this type is an object. `fields` and `interfaces` are valid fields.")
.value("INTERFACE", TypeKind.INTERFACE, "Indicates this type is an interface. `fields` and `possibleTypes` are valid fields.")
.value("UNION", TypeKind.UNION, "Indicates this type is a union. `possibleTypes` is a valid field.")
Expand Down Expand Up @@ -398,8 +398,13 @@ private static String printDefaultValue(InputValueWithState inputValueWithState,
.name("ofType")
.type(typeRef("__Type")))
.field(newFieldDefinition()
.name("specifiedByUrl")
.name("specifiedByURL")
Copy link
Member

@bbakerman bbakerman Oct 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@bbakerman bbakerman Oct 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@MayCXC MayCXC Oct 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@bbakerman bbakerman Oct 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@bbakerman bbakerman Oct 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As above - add a new field and deprecate the old

.type(GraphQLString))
.field(newFieldDefinition()
.name("specifiedByUrl")
.type(GraphQLString)
.deprecate("see `specifiedByURL`")
)
.build();

static {
Expand All @@ -412,7 +417,8 @@ private static String printDefaultValue(InputValueWithState inputValueWithState,
register(__Type, "ofType", OfTypeFetcher);
register(__Type, "name", nameDataFetcher);
register(__Type, "description", descriptionDataFetcher);
register(__Type, "specifiedByUrl", specifiedByUrlDataFetcher);
register(__Type, "specifiedByURL", specifiedByUrlDataFetcher);
register(__Type, "specifiedByUrl", specifiedByUrlDataFetcher); // note that this field is deprecated
}


Expand Down
103 changes: 1 addition & 102 deletions src/main/java/graphql/introspection/IntrospectionQuery.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,106 +4,5 @@

@PublicApi
public interface IntrospectionQuery {

String INTROSPECTION_QUERY = "\n" +
" query IntrospectionQuery {\n" +
" __schema {\n" +
" queryType { name }\n" +
" mutationType { name }\n" +
" subscriptionType { name }\n" +
" types {\n" +
" ...FullType\n" +
" }\n" +
" directives {\n" +
" name\n" +
" description\n" +
" locations\n" +
" args(includeDeprecated: true) {\n" +
" ...InputValue\n" +
" }\n" +
" isRepeatable\n" +
" }\n" +
" }\n" +
" }\n" +
"\n" +
" fragment FullType on __Type {\n" +
" kind\n" +
" name\n" +
" description\n" +
" fields(includeDeprecated: true) {\n" +
" name\n" +
" description\n" +
" args(includeDeprecated: true) {\n" +
" ...InputValue\n" +
" }\n" +
" type {\n" +
" ...TypeRef\n" +
" }\n" +
" isDeprecated\n" +
" deprecationReason\n" +
" }\n" +
" inputFields(includeDeprecated: true) {\n" +
" ...InputValue\n" +
" }\n" +
" interfaces {\n" +
" ...TypeRef\n" +
" }\n" +
" enumValues(includeDeprecated: true) {\n" +
" name\n" +
" description\n" +
" isDeprecated\n" +
" deprecationReason\n" +
" }\n" +
" possibleTypes {\n" +
" ...TypeRef\n" +
" }\n" +
" }\n" +
"\n" +
" fragment InputValue on __InputValue {\n" +
" name\n" +
" description\n" +
" type { ...TypeRef }\n" +
" defaultValue\n" +
" isDeprecated\n" +
" deprecationReason\n" +
" }\n" +
"\n" +
//
// The depth of the types is actually an arbitrary decision. It could be any depth in fact. This depth
// was taken from GraphIQL https://github.com/graphql/graphiql/blob/master/src/utility/introspectionQueries.js
// which uses 7 levels and hence could represent a type like say [[[[[Float!]]]]]
//
"fragment TypeRef on __Type {\n" +
" kind\n" +
" name\n" +
" ofType {\n" +
" kind\n" +
" name\n" +
" ofType {\n" +
" kind\n" +
" name\n" +
" ofType {\n" +
" kind\n" +
" name\n" +
" ofType {\n" +
" kind\n" +
" name\n" +
" ofType {\n" +
" kind\n" +
" name\n" +
" ofType {\n" +
" kind\n" +
" name\n" +
" ofType {\n" +
" kind\n" +
" name\n" +
" }\n" +
" }\n" +
" }\n" +
" }\n" +
" }\n" +
" }\n" +
" }\n" +
" }\n" +
"\n";
String INTROSPECTION_QUERY = IntrospectionQueryBuilder.build();
}
Loading