-
Notifications
You must be signed in to change notification settings - Fork 65
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.
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
Bump graphql-java to v14 #41
Conversation
@@ -85,7 +95,7 @@ public final GraphQLSchema build() throws SchemaProblem { | |||
final Set<String> entityTypeNames = originalSchema.getAllTypesAsList().stream() | |||
.filter(t -> t instanceof GraphQLDirectiveContainer && | |||
((GraphQLDirectiveContainer) t).getDirective(FederationDirectives.keyName) != null) | |||
.map(GraphQLType::getName) | |||
.map(GraphQLNamedType::getName) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.includeSchemaDefintion(true) | ||
.includeDirectives(true); | ||
.includeSchemaDefinition(true) | ||
.includeDirectives(directivePredicate); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Directs the executor to include this field or fragment only when the `if` argument is true" | ||
directive @include( | ||
"Included when true." | ||
if: Boolean! | ||
) on FIELD | FRAGMENT_SPREAD | INLINE_FRAGMENT | ||
|
||
"Directs the executor to skip this field or fragment when the `if`'argument is true." | ||
directive @skip( | ||
"Skipped when true." | ||
if: Boolean! | ||
) on FIELD | FRAGMENT_SPREAD | INLINE_FRAGMENT | ||
|
||
"Marks the field or enum value as deprecated" | ||
directive @deprecated( | ||
"The reason for the deprecation" | ||
reason: String = "No longer supported" | ||
) on FIELD_DEFINITION | ENUM_VALUE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not quite sure why these started showing up in the SDL - graphql-java/graphql-java#1723 maybe?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe it's from graphql-java/graphql-java#1609 , specifically the removal of
// we don't print the standard directives that always ship with graphql-java
List<String> standardDirectives = Arrays.asList(
"skip",
"include",
"defer",
"deprecated");
dc612dc
to
fed66ba
Compare
See also #46 (these PRs should be looked at in parallel) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems to me that including support for the directive predicate when transforming for federation should not be supported.
@@ -45,6 +49,12 @@ public SchemaTransformer fetchEntities(DataFetcher entitiesDataFetcher) { | |||
return this; | |||
} | |||
|
|||
@NotNull | |||
public SchemaTransformer directivePredicate(Predicate<GraphQLDirective> directivePredicate) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the purpose of exposing the directive predicate in this transformer? This predicate is provided to the SchemaPrinter
when generating SDL for the _sdl
query operation. It seems to me that this provides no utility and only opens the door for problems where someone inadvertently removes an important directive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change really deserves to be in a separate PR - but you can find more background here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I agree that it would be helpful for this to be in a separate PR because it's a separate bit of functionality.
If I'm following along correctly here it seems that the desire is to exclude the private directives because they cause the schema to fail to be transformed successfully. The failure is due to the fact that the directives are not included in the schema. Why is it desirable to use directives in a schema but not include the definition of the directives?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is it desirable to use directives in a schema but not include the definition of the directives?
I'm afraid I can't answer that question. I believe it's an implementation detail of graphql-spqr. Ping @kaqqao?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is actually part of the federation spec for the sdl
field: https://www.apollographql.com/docs/apollo-server/federation/federation-spec/#query_service
We have to do the same logic here in graphql-kotlin-federation: https://github.com/ExpediaGroup/graphql-kotlin/blob/8ff7de80e8681b75091f244564a991aef7eea4fd/graphql-kotlin-federation/src/main/kotlin/com/expediagroup/graphql/federation/FederatedSchemaGeneratorHooks.kt#L90-L97
I have created a PR in graphql-java to make this a little easier by adding a new option to the printer: graphql-java/graphql-java#1756
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's been a slight misunderstanding about Apollo Federation, there's more info at #41 (comment)
@@ -23,29 +26,42 @@ | |||
|
|||
class FederationTest { | |||
private final String emptySDL = TestUtils.readResource("schemas/empty.graphql"); | |||
private final String emptySDLOutput = TestUtils.readResource("schemas/empty-output.graphql"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refactoring this to a separate file is a nice change as this is more consistent with the rest of the tests here.
One thing to point out for other reviewers here is that the expected printed schema is different with this version because the include, skip and deprecated directives are now output.
private final String interfacesSDL = TestUtils.readResource("schemas/interfaces.graphql"); | ||
private final String isolatedSDL = TestUtils.readResource("schemas/isolated.graphql"); | ||
private final String productSDL = TestUtils.readResource("schemas/product.graphql"); | ||
private final String productSDLOutput = TestUtils.readResource("schemas/product-output.graphql"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It wasn't clear to me at first why a separate schema is needed here. I realize now that it's because a few new directives started appearing and I suspect an error occurs when loading the schema with the directives expressed.
It's not only that certain definitions should be excluded, but the directive usages (and especially the argument values) that should be excluded too. I don't fully understand how Apollo Federation works, so please excuse me if I say something incorrect. That said, I believe Apollo Federation operates on SDL and not introspection. The printed SDL might easily contain sensitive schema directives for e.g. access control that have no business ever being exposed. SPQR utilizes directives to store meta data about the fields and types. These are defined as schema directives that are never intended for the clients to be aware of (as opposed to client/query directives, that clients can provide as a part of the query and thus must be aware of). What would a client even do with the knowledge of such a directive (except perhaps abuse it)? E.g. type Character {
name: String!
appearsIn: [Episode!]! @hasRoles(roles: "ADMIN")
} The As of graphql-java v14, |
Sorry for the delays in resolving this issue. I talked to the team managing Apollo Gateway, and I have some answers here. Regarding Apollo Federation, an important aspect to understand is that
Apollo Gateway, when composing the graphs of the implementing services, will effectively prevent server-side directives from being made visible to the client. More specifically, during the gateway's graph composition, the gateway will do the following:
So while server-side directives are exposed via |
Thanks for the PR! There's a few changes here that are desirable
I've went ahead and made a PR that does this at #51 . |
|
This can be closed as the version was upgraded in #51 |
No description provided.