graphql-java-tools icon indicating copy to clipboard operation
graphql-java-tools copied to clipboard

Exception in thread "main" graphql.AssertException: Internal error: should never happen: Directive values of type 'EnumValue' are not supported yet

Open dspenceb opened this issue 4 years ago • 9 comments

This is an exception that is thrown by graphql-java when called from graphql-java-tools. The problem, however, is due to the way graphql-java-tools is using the graphql-java library internal API.

This is evidenced in this issue in graphql-java for a PR created to attempt to fix this issue: https://github.com/graphql-java/graphql-java/pull/1706

Tools does not pass a directiveDefinition -- which is never mapped at any point in the SchemaParser https://github.com/graphql-java-kickstart/graphql-java-tools/blob/master/src/main/kotlin/graphql/kickstart/tools/SchemaParser.kt#L163 And this causes graphql-java to use a code path that, according to them, is dead code and actually is removed in v15.

I created a test application to demonstrate this issue here: https://github.com/dspenceb/graphqltest

For quick reference, the schema:

type Query {
    books: [Book!]
}

type Book {
    id: Int!
    name: String! @allowed(
        allowed: [ALLOWED]
    )
    author: Author!
}
...
directive @allowed(
    allowed: [AllowedState!]
) on FIELD_DEFINITION | OBJECT

enum AllowedState {
    ALLOWED
    DISALLOWED
}

wiring:

GraphQLSchema schema = SchemaParser.newParser()
                .file("schema.graphqls")

                .resolvers(new QueryResolver(new BookRepository()), new BookResolver(new AuthorRepository()))
                .directive("allowed", new AllowedDirective())
                .build()
                .makeExecutableSchema();

Stack:

Exception in thread "main" graphql.AssertException: Internal error: should never happen: Directive values of type 'EnumValue' are not supported yet
	at graphql.Assert.assertShouldNeverHappen(Assert.java:51)
	at graphql.schema.idl.SchemaGeneratorHelper.buildDirectiveInputType(SchemaGeneratorHelper.java:203)
	at graphql.schema.idl.SchemaGeneratorHelper.buildDirectiveInputType(SchemaGeneratorHelper.java:201)
	at graphql.schema.idl.SchemaGeneratorHelper.buildDirectiveArgument(SchemaGeneratorHelper.java:272)
	at graphql.schema.idl.SchemaGeneratorHelper.lambda$buildDirective$6(SchemaGeneratorHelper.java:251)
	at java.util.stream.ReferencePipeline$3$1.accept(ReferencePipeline.java:193)
	at java.util.ArrayList$ArrayListSpliterator.forEachRemaining(ArrayList.java:1382)
	at java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:482)
	at java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:472)
	at java.util.stream.ReduceOps$ReduceOp.evaluateSequential(ReduceOps.java:708)
	at java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234)
	at java.util.stream.ReferencePipeline.collect(ReferencePipeline.java:499)
	at graphql.schema.idl.SchemaGeneratorHelper.buildDirective(SchemaGeneratorHelper.java:252)
	at graphql.kickstart.tools.SchemaParser.buildDirectives(SchemaParser.kt:150)
	at graphql.kickstart.tools.SchemaParser.createField(SchemaParser.kt:278)
	at graphql.kickstart.tools.SchemaParser.access$createField(SchemaParser.kt:18)
	at graphql.kickstart.tools.SchemaParser$createObject$$inlined$forEach$lambda$1.apply(SchemaParser.kt:124)
	at graphql.kickstart.tools.SchemaParser$createObject$$inlined$forEach$lambda$1.apply(SchemaParser.kt:18)
	at graphql.schema.GraphQLObjectType$Builder.field(GraphQLObjectType.java:308)
	at graphql.kickstart.tools.SchemaParser.createObject(SchemaParser.kt:123)
	at graphql.kickstart.tools.SchemaParser.parseSchemaObjects(SchemaParser.kt:68)
	at graphql.kickstart.tools.SchemaParser.makeExecutableSchema(SchemaParser.kt:98)
	at dspenceb.graphqltest.Main.main(Main.java:17)

dspenceb avatar Jun 23 '20 13:06 dspenceb

Thanks for reporting this. This will be likely fixed in the next version when we support GraphQL Java 15.

vojtapol avatar Jun 29 '20 12:06 vojtapol

hi @vojtapol thanks for the response! Is there any kind of timeline or ETA on when that may be?

Thanks!

dspenceb avatar Jun 30 '20 16:06 dspenceb

@vojtapol do you know which version this would be fixed in? we're trying to plan around the fix.

jamesmartinpp avatar Jul 02 '20 19:07 jamesmartinpp

@jamesmartinpp We may need to do a significant rewrite to fix all the issues around directives. In general, we need to defer more work to graphql-java so that it's easier to stay up to date with their releases. I have no ETA on when the work is going to be finished.

vojtapol avatar Jul 07 '20 15:07 vojtapol

I saw in the latest 6.2.0 release that there is support for graphql java 15.0. Does that mean this issue is fixed? I haven't had time to investigate this on my own.

dspenceb avatar Dec 11 '20 19:12 dspenceb

FYI: I tried with the latest 11.0.0 and still having this issue now

williamwjs avatar Mar 13 '21 05:03 williamwjs

@vojtapol Any plans to supports this in the near future?

anthochristen avatar Jan 19 '22 14:01 anthochristen

Anybody has a workaround for this?

wwang107 avatar Jan 24 '22 15:01 wwang107

Any news about this issue? Thanks

sbarbs95 avatar Mar 08 '22 14:03 sbarbs95

Is this still an issue? We have updated to graphql-java v20 and graphql-java-tools v13 but are still seeing the exception. We have been doing a custom patch of graphql-java but we would prefer not to have to do that anymore, assuming that custom patch even works anymore.

jamesmartinpp avatar Dec 08 '23 20:12 jamesmartinpp

@oryan-block is this issue fixed by #763? If it is fixed is it only fixed in the latest version of 13.1.1?

jamesmartinpp avatar Dec 11 '23 20:12 jamesmartinpp

@jamesmartinpp if the problem was with a directive containing an enum parameter it should be fix by https://github.com/graphql-java-kickstart/graphql-java-tools/pull/764

I'm not maintaining older versions so yes, only in 13.1.1.

oryan-block avatar Dec 11 '23 20:12 oryan-block

Got it, Thanks! @oryan-block

jamesmartinpp avatar Dec 11 '23 20:12 jamesmartinpp

@oryan-block Is there no way to produce a 13.0 patch release with this change? Do you think that's not reasonably doable? How about if someone submits a PR?

crankydillo avatar Dec 19 '23 10:12 crankydillo

@crankydillo I don't see what the purpose of a patch would be. The big difference between 13.0 and 13.1 was exactly fixing these issues with directive processing.

oryan-block avatar Dec 21 '23 16:12 oryan-block

@oryan-block the problem is the requirement to upgrade to graphql-java v21 and consequently java 11. We wanted this fix on a version that would work with java 8.

jamesmartinpp avatar Dec 21 '23 16:12 jamesmartinpp

@crankydillo Okay I see what you mean. If you want to open a PR for this I can try releasing a "hotfix"

oryan-block avatar Dec 21 '23 18:12 oryan-block

Hi @oryan-block It doesn't look like this requires much. I just downgraded pom versions and everything built (code). Unfortunately, I'm not able to test in some of our main regressions, but I assume this code will work for v20, like it will for v21.

I will submit a PR to a branch if you create a branch, but feel free to just toggle those versions:)

crankydillo avatar Dec 23 '23 13:12 crankydillo

Hi @oryan-block . Wanted to touch base with you on this. I created this commit off your master. It built and passed all our graphql-java related tests.

I don't mind submitting a PR. I'm going to redo this work on top of the v13.0.4 branch and submit a PR to that. Let me know if that's not what you're thinking.

crankydillo avatar Jan 05 '24 15:01 crankydillo

Ah, it looks like you might already be doing this!

crankydillo avatar Jan 05 '24 15:01 crankydillo

@crankydillo Trying to. The "pipeline" is not built for this.

oryan-block avatar Jan 05 '24 16:01 oryan-block

Hi @oryan-block . Sorry to pester and thanks for doing this. We just want to make sure you plan to do all the tagging and stuff like that you typically do for releases for the 13.0.4 Maven artifact I'm assuming you deployed:) That's going to happen, right?

Basically, we want to make sure that it's OK to use that 13.0.4 Maven artifact, and, if we do and have to fix something, we can submit a patch PR to your repo.

Thanks again!

crankydillo avatar Jan 09 '24 17:01 crankydillo

@crankydillo right so there's a bug in the deploy script that I don't have the time to look into. However, since the artifact was pushed I guess I could create a release for it and it should technically be fine. Let me know if that's what you want.

oryan-block avatar Jan 09 '24 18:01 oryan-block

Yes. That's what we're hoping for, along with a tag for the v13.0.4. I think you currently have that as a branch.

crankydillo avatar Jan 09 '24 18:01 crankydillo

@crankydillo Alright I hope this works for you: https://github.com/graphql-java-kickstart/graphql-java-tools/releases/tag/v13.0.4

oryan-block avatar Jan 09 '24 21:01 oryan-block

Thanks, @oryan-block

jamesmartinpp avatar Jan 10 '24 13:01 jamesmartinpp

Closing for now

oryan-block avatar Jan 18 '24 14:01 oryan-block