Fixes for graphql-java:14 compatibility #46

paulbakker · 2020-02-06T21:28:04Z

Some minor required fixes to be compatible with graphql-java:14.

They fixed a typo in the includeSchemaDefinition option of the SchemaPrinter
GraphQLType::getName does no longer work, need a Named* type now.

For a generated sdl it would now include the directives plus descriptions, I modified the TestUtils to account for this.

apollo-cla · 2020-02-06T21:28:06Z

@paulbakker: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Apollo Contributor License Agreement here: https://contribute.apollographql.com/

glasser

Thanks for looking into this!

glasser · 2020-02-07T00:02:28Z

...l-java-support/src/main/java/com/apollographql/federation/graphqljava/SchemaTransformer.java

                .includeScalarTypes(true)
                .includeExtendedScalarTypes(true)
-                .includeSchemaDefintion(true)
+                .includeSchemaDefinition(true)


Do these changes mean that this version would not work with GraphQL-Java 13 at all? (Probably OK to require upgrades but worth thinking about.)

Yes, that's correct unfortunately.

glasser · 2020-02-07T00:03:06Z

graphql-java-support/src/test/java/com/apollographql/federation/graphqljava/SchemaUtils.java

        assertNotNull(_service);
        final String sdl = (String) _service.get("sdl");
-        assertEquals(expected.trim(), sdl.trim());
+        assertEquals(expected.replaceAll("\n\n", "").trim(), sdl.replace(directivesExclude, "").replaceAll("\n\n", "").trim());


Why not use removeDirectives?

glasser · 2020-02-07T00:03:22Z

graphql-java-support/src/test/java/com/apollographql/federation/graphqljava/SchemaUtils.java

+        assertEquals(expected.replaceAll("\n\n", "").trim(), sdl.replace(directivesExclude, "").replaceAll("\n\n", "").trim());
+    }
+
+    static String removeDirectives(String sdl) {


This should probably be called something like removeSpecDirectives or removeBuiltInDirectives or something (and the fixture file changed too).

glasser · 2020-02-07T00:06:43Z

graphql-java-support/src/test/java/com/apollographql/federation/graphqljava/FederationTest.java

                "type _Service {\n" +
                "  sdl: String!\n" +
-                "}\n", SchemaUtils.printSchema(federated));
+                "}", SchemaUtils.removeDirectives(SchemaUtils.printSchema(federated)).trim());


Do you understand what change in GraphQL-Java is necessitating this? Is this definitely just a testing issue? Note that part of federation is making the _Service.sdl field which uses the GraphQL-Java schema printer to print some SDL and return it to users (see SchemaTransformer.sdl()), so if this upgrade affects the output of that field, that might be an issue.

I don't. I just started looking at upgrading to 14, and it's a big breaking release. The incompatibility with the federation lib is currently blocking to do any testing with federated services, so it's also hard to say what else might be broken. If we can do some sort of snapshot release that would give a much better idea what else needs to happen.

The change appears to be on purpose.

The code change is here: PR.
graphql-java # 1587 describes the rationale.

Standard directives are not printed when the includeDirectives flag is set. This seems like a design choice ...
However this causes issues when we have to input the SDL to other GraphQL services that are not running graphql-java that don't default to having those declared. Can we add these to the SDL?

This should be a safe change. Note that the directives that are seen in the output are standard GraphQL directives that are part of the spec (include, skip and deprecated).

paulbakker · 2020-02-12T20:23:38Z

Update: I've published this internally (Netflix) and did some rollouts. Looking good so far. It's a bit of a weak guarantee, but at least it didn't completely go up in flames.

glasser · 2020-02-12T20:55:21Z

See also #41 (these PRs should be looked at in parallel)

mcohen75 · 2020-02-18T16:43:59Z

See also #41 (these PRs should be looked at in parallel)

The changes in this MR look to be a more appropriate way forward. I left some comments on #41, I'll summarize here.

#41 allows consumers of the SchemaTransformer to specify a predicate for filtering directives out of the generated SDL (used by the _sdl query operation). I don't see a valid use case for this and it opens the door for someone unintentionally filtering out important federation related directives. For these reasons I'd suggest that this MR should be merged over #41.

mcohen75 · 2020-02-20T13:39:33Z

@glasser is this something that can be merged? I believe I've added sufficient information to alleviate any concerns here.

sachindshinde · 2020-02-21T17:31:47Z

@mcohen75
Apologies for the delays here! We'll have some time to look at this in the coming weeks, and will get back to you by March 9th.

smyrick · 2020-02-22T20:27:01Z

Following this issue as we would love to migrate to use this library in graphql-kotlin over our own implementation here:

https://github.com/ExpediaGroup/graphql-kotlin/blob/7eb5d850a4d65a85934f5c8f3921be9e2760efe5/graphql-kotlin-federation/src/main/kotlin/com/expediagroup/graphql/federation/FederatedSchemaGeneratorHooks.kt#L73

However we have upgraded to graphql-java 14 so we are also blocked

mcohen75 · 2020-02-26T13:32:42Z

@sachindshinde March 9th is a long ways off. Is there any way this could be handled sooner? It looks like this MR is in good shape and can be merged.

sachindshinde · 2020-03-16T21:53:24Z

@paulbakker @smyrick @mcohen75

I did some investigating with the Apollo Gateway team, and it turns out that including directive definitions for standard directives (@deprecated, @include, and @skip) in the SDL breaks Apollo Gateway's graph composition.

There's a quick fix by using includeDirectives() to filter out standard directives. This conveniently removes their definitions and usages from the SDL with the exception of directive usages of @deprecated, which is precisely the behavior we want.

I've went ahead and made a PR for that at #51 .

sachindshinde · 2020-03-27T03:40:01Z

Compatibility for graphql-java v14 has been released in federation-jvm v0.4.0, so I'll close out this PR. Feel free to file any follow-ups in separate issues/PRs.

Fixes for graphql-java:14 compatibility

ac98819

glasser requested changes Feb 7, 2020

View reviewed changes

Review comments

cbcfd05

paulbakker force-pushed the graphql-java-14 branch from de025e0 to cbcfd05 Compare February 10, 2020 20:20

paulbakker mentioned this pull request Feb 12, 2020

Support for graphql-java 14 #45

Closed

glasser mentioned this pull request Feb 12, 2020

Bump graphql-java to v14 #41

Closed

sachindshinde mentioned this pull request Mar 16, 2020

graphql-java-support: Bump graphql-java to v14 #51

Merged

sachindshinde closed this Mar 27, 2020

Fixes for graphql-java:14 compatibility #46

Fixes for graphql-java:14 compatibility #46

Uh oh!

Conversation

paulbakker commented Feb 6, 2020

Uh oh!

apollo-cla commented Feb 6, 2020

Uh oh!

glasser left a comment

Choose a reason for hiding this comment

Uh oh!

glasser Feb 7, 2020

Choose a reason for hiding this comment

Uh oh!

paulbakker Feb 7, 2020

Choose a reason for hiding this comment

Uh oh!

glasser Feb 7, 2020

Choose a reason for hiding this comment

Uh oh!

glasser Feb 7, 2020

Choose a reason for hiding this comment

Uh oh!

glasser Feb 7, 2020

Choose a reason for hiding this comment

Uh oh!

paulbakker Feb 7, 2020

Choose a reason for hiding this comment

Uh oh!

mcohen75 Feb 18, 2020

Choose a reason for hiding this comment

Uh oh!

paulbakker commented Feb 12, 2020

Uh oh!

glasser commented Feb 12, 2020

Uh oh!

mcohen75 commented Feb 18, 2020

Uh oh!

mcohen75 commented Feb 20, 2020

Uh oh!

sachindshinde commented Feb 21, 2020

Uh oh!

smyrick commented Feb 22, 2020

Uh oh!

mcohen75 commented Feb 26, 2020

Uh oh!

sachindshinde commented Mar 16, 2020

Uh oh!

sachindshinde commented Mar 27, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants