update graphql-java to version 15 #120

claymccoy · 2020-08-29T01:38:56Z

This is an old version of a core dependency used in most graphql related projects in java. If this dependency isn't updated then you have conflicts with incompatible transitive versions of graphql-java.
Unfortunately, it looks like a committer to graphql-java likes to make backwards a lot of uneccessarily incompatible changes to the library. So I had to make a lot of changes for this to compile and for the tests to pass. I had to do some archeology in the history of graphql-java to understand some of the changes.

GraphQLType.name no longer exists. simplePrint(GraphQLType) is used instead
GraphQLFieldDefinition is no longer a GraphQLType, but it looks like GraphQLFieldDefinition.originalType will get the type we are looking for.
ScalarInfo.STANDARD_SCALAR_DEFINITIONS was replaced with ScalarInfo.GRAPHQL_SPECIFICATION_SCALARS_DEFINITIONS
The value generic for the return type of GraphQLSchema.typeMap is now more specific so BuildingEnv's constructor had to change.
ShemaPrinter lost some include methods. One was previously mispelled.
DefaultSchemaPrinterComparatorRegistry became DefaultGraphqlTypeComparatorRegistry.

I had to make some changes to the tests to make them pass, which makes me quite uncomfortable with this. Please check these over to see if I should have made more changes. I'm going ahead with this because the overall functionality still seems legit. I have no idea why these were needed.

In translator-tests1 'render values' I had to move the clauses of the where clause around. It mateches the order of the cypher params now, so it kind of makes sense.
The one line GraphQL query no longer works. It had to be broken up to multilines in several tests.
relationship-tests is a weird one. I changed the generated Cypher, but it makes more sense now. The relationship is player through memberships to team. So playerMembershipsTeam: Team makes a lot more sense than the original playerMembershipsPlayer: Player. It actually looks like it was incorrect originally.

This is an old version of a core dependency used in most graphql related projects in java. If this dependency isn't updated then you have conflicts with incompatible transitive versions of graphql-java. Unfortunately, it looks like a committer to graphql-java likes to make backwards a lot of uneccessarily incompatible changes to the library. So I had to make a lot of changes for this to compile and for the tests to pass. I had to do some archeology in the history of graphql-java to understand some of the changes. * GraphQLType.name no longer exists. simplePrint(GraphQLType) is used instead * GraphQLFieldDefinition is no longer a GraphQLType, but it looks like GraphQLFieldDefinition.originalType will get the type we are looking for. * ScalarInfo.STANDARD_SCALAR_DEFINITIONS was replaced with ScalarInfo.GRAPHQL_SPECIFICATION_SCALARS_DEFINITIONS * The value generic for the return type of GraphQLSchema.typeMap is now more specific so BuildingEnv's constructor had to change. * ShemaPrinter lost some include methods. One was previously mispelled. * DefaultSchemaPrinterComparatorRegistry became DefaultGraphqlTypeComparatorRegistry. I had to make some changes to the tests to make them pass, which makes me quite uncomfortable with this. Please check these over to see if I should have made more changes. I'm going ahead with this because the overall functionality still seems legit. I have no idea why these were needed. * In translator-tests1 'render values' I had to move the clauses of the where clause around. It mateches the order of the cypher params now, so it kind of makes sense. * The one line GraphQL query no longer works. It had to be broken up to multilines in several tests. * relationship-tests is a weird one. I changed the generated Cypher, but it makes more sense now. The relationship is player through memberships to team. So `playerMembershipsTeam: Team` makes a lot more sense than the original `playerMembershipsPlayer: Player`. It actually looks like it was incorrect originally.

jexp · 2020-08-30T22:36:13Z

@Andy2003 please help with the review and rebase due to the test changes.

# Conflicts: # src/test/kotlin/org/neo4j/graphql/TranslatorExceptionTest.kt # src/test/kotlin/org/neo4j/graphql/TranslatorExceptionTests.kt # src/test/kotlin/org/neo4j/graphql/utils/GraphQLSchemaTestSuite.kt

Andy2003 · 2020-08-31T11:11:47Z

@claymccoy Thank you for your contribution

I walked through the changes and adjusted the branch in (claymccoy#1)

I replaced the massive use of simplePrint by a fallback name() method

DefaultSchemaPrinterComparatorRegistry became DefaultGraphqlTypeComparatorRegistry.

This was only used to sort the fields of the type for better comparison / diffing. This is no longer possible so I removed the whole block

The one line GraphQL query no longer works. It had to be broken up to multilines in several tests.

There were nbsp characters in the tests. replacing them by space make them run again

In translator-tests1 'render values' I had to move the clauses of the where clause around. It mateches the order of the cypher params now, so it kind of makes sense.

I got the same issue with this test in the past. The test is OK so far.

relationship-tests is a weird one. I changed the generated Cypher, but it makes more sense now. The relationship is player through memberships to team. So playerMembershipsTeam: Team makes a lot more sense than the original playerMembershipsPlayer: Player. It actually looks like it was incorrect originally.

This was indeed a bug

claymccoy · 2020-08-31T14:37:55Z

@Andy2003 I pulled your changes in. Thanks!
Is it possible to make a release once these changes are in? I'd really like to use it in a project.

Andy2003 · 2020-08-31T15:06:43Z

pom.xml

@@ -88,7 +88,7 @@
        <dependency>
            <groupId>com.graphql-java</groupId>
            <artifactId>graphql-java</artifactId>
-            <version>12.0</version>
+            <version>15.0</version>


@jexp Do you think we need to add a compatibility layer to support older versions of graphql-java. Since they are not very compatible code my break. E.g. I'm using graphql-spqr-spring-boot-starter with this library, which has not yet been updated to use version 15.0 of graphql-java. There is an open issue for this leangen/graphql-spqr#356 . So also other users my have this compatibility issues.

That was similar to the position I was in and why I tried to help update the library here. I wanted to use a library similar to spqr (graphql-kotlin) which uses a later graphql-java. Rather than a compatibility layer, I opted to help move this project forward with the thinking that a rising tide lifts all boats. I could help update other projects too. This is a common problem with libraries that use common dependencies. It is often an issue with Guava or something similar. The two strategies I've seen most often used successfully are to severely limit dependencies used in a library and/or to be aggressive about getting everyone to update the common deps.

My suggestion is:

we release a version of 1.0.1 with the nested sorting PR merged and graphql-java 12 (and create a branch/tag for that)

then we release 1.1.0 with the upgrade of graphql-java to 15 (master)

Andy2003 · 2020-08-31T15:07:41Z

src/main/kotlin/org/neo4j/graphql/BuildingEnv.kt

@@ -23,7 +25,7 @@ class BuildingEnv(val types: MutableMap<String, GraphQLType>) {
            type = GraphQLNonNull(type)
        }
        return GraphQLFieldDefinition.newFieldDefinition()
-            .name("$prefix${resultType.name}")
+            .name("$prefix${simplePrint(resultType)}")


use resultType.name()

Andy2003 · 2020-08-31T15:08:38Z

src/main/kotlin/org/neo4j/graphql/BuildingEnv.kt

@@ -71,7 +73,7 @@ class BuildingEnv(val types: MutableMap<String, GraphQLType>) {
        }
        val existingFilterType = types[filterName]
        if (existingFilterType != null) {
-            return (existingFilterType as? GraphQLInputType)?.name
+            return simplePrint(existingFilterType as? GraphQLInputType)


use (existingFilterType as? GraphQLInputType)?.name()

Andy2003 · 2020-08-31T15:14:32Z

Is it possible to make a release once these changes are in? I'd really like to use it in a project.

@jexp is the master of releases ;-)

jexp · 2020-08-31T16:21:51Z

Let me review and merge this in and then the few open PRs from @Andy2003 and then I promise to do the release this week.

Oh btw. @claymccoy can you please sign our CLA first? https://neo4j.com/developer/cla

claymccoy · 2020-08-31T18:13:59Z

Oh btw. @claymccoy can you please sign our CLA first? https://neo4j.com/developer/cla

email sent for cla

Andy2003 · 2020-09-03T08:14:56Z

@claymccoy please merge claymccoy#2 so we can merge this PR afterwards

claymccoy mentioned this pull request Aug 29, 2020

Update graphql-java to 15.0 #118

Closed

jexp requested a review from Andy2003 August 30, 2020 22:36

Merge branch 'master' into updateGraphqlJava

6873361

# Conflicts: # src/test/kotlin/org/neo4j/graphql/TranslatorExceptionTest.kt # src/test/kotlin/org/neo4j/graphql/TranslatorExceptionTests.kt # src/test/kotlin/org/neo4j/graphql/utils/GraphQLSchemaTestSuite.kt

Andy2003 mentioned this pull request Aug 31, 2020

Update graphql java claymccoy/neo4j-graphql-java#1

Merged

adjustments for graphql-java update

b5ff228

Andy2003 requested changes Aug 31, 2020

View reviewed changes

Merge branch 'master' into updateGraphqlJava

230e522

jexp merged commit e579cd2 into neo4j-graphql:master Sep 3, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

update graphql-java to version 15 #120

update graphql-java to version 15 #120

Uh oh!

claymccoy commented Aug 29, 2020

Uh oh!

jexp commented Aug 30, 2020

Uh oh!

Andy2003 commented Aug 31, 2020 •

edited

Loading

Uh oh!

claymccoy commented Aug 31, 2020

Uh oh!

Andy2003 Aug 31, 2020

Uh oh!

claymccoy Aug 31, 2020

Uh oh!

jexp Sep 2, 2020

Uh oh!

Andy2003 Sep 3, 2020

Uh oh!

Andy2003 Aug 31, 2020

Uh oh!

Andy2003 Aug 31, 2020

Uh oh!

Andy2003 commented Aug 31, 2020

Uh oh!

jexp commented Aug 31, 2020

Uh oh!

claymccoy commented Aug 31, 2020

Uh oh!

Andy2003 commented Sep 3, 2020

Uh oh!

Uh oh!

update graphql-java to version 15 #120

update graphql-java to version 15 #120

Uh oh!

Conversation

claymccoy commented Aug 29, 2020

Uh oh!

jexp commented Aug 30, 2020

Uh oh!

Andy2003 commented Aug 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

claymccoy commented Aug 31, 2020

Uh oh!

Andy2003 Aug 31, 2020

Choose a reason for hiding this comment

Uh oh!

claymccoy Aug 31, 2020

Choose a reason for hiding this comment

Uh oh!

jexp Sep 2, 2020

Choose a reason for hiding this comment

Uh oh!

Andy2003 Sep 3, 2020

Choose a reason for hiding this comment

Uh oh!

Andy2003 Aug 31, 2020

Choose a reason for hiding this comment

Uh oh!

Andy2003 Aug 31, 2020

Choose a reason for hiding this comment

Uh oh!

Andy2003 commented Aug 31, 2020

Uh oh!

jexp commented Aug 31, 2020

Uh oh!

claymccoy commented Aug 31, 2020

Uh oh!

Andy2003 commented Sep 3, 2020

Uh oh!

Uh oh!

Andy2003 commented Aug 31, 2020 •

edited

Loading