Skip to content

Conversation

@AnkBurov
Copy link

#1500
Currently enum types in directives are not supported for some reason. A PR to add support for them.

@andimarek andimarek added this to the 14.0 milestone Jan 9, 2020
@bbakerman bbakerman self-assigned this Jan 13, 2020
@andimarek andimarek added the needs to be backported a bugfix that still needs to be backported label Jan 13, 2020
@bbakerman
Copy link
Member

We are going to decline this directive. The code path changed is in fact invalid in and of itself.

That is in the absence of a directive definition, this code base tries to guess a value. But this is wrong. Which is why enum and object type are not supported. We have no way of guessing.

The real fix is that we enforce "having to declare schema directives". That is if you declare @foothen some where else you have declared directive @foo on ARGUMENT_DEFINITION first

@bbakerman bbakerman closed this Jan 13, 2020
@AnkBurov
Copy link
Author

AnkBurov commented Jan 13, 2020

@bbakerman thanks for the answer. Could you clarify will graphql-java project support enums as arguments in directives? E.g. this:

directive @directiveExample (
    enumArguments: [SomeEnum!] = []
) on FIELD_DEFINITION

enum SomeEnum {
    VALUE_1
    VALUE_2
}

@andimarek andimarek removed this from the 14.0 milestone Jan 14, 2020
@andimarek
Copy link
Member

@AnkBurov yes this is how we will support enums (and everything else): you have to declare the directive.

@AnkBurov
Copy link
Author

AnkBurov commented Jan 15, 2020

@andimarek Any specific plans about supporting enums in directive arguments? Because the following schema

directive @directiveExample (
    enumArguments: [SomeEnum!] = []
) on FIELD_DEFINITION

enum SomeEnum {
    VALUE_1
    VALUE_2
}

currently is unsupported in graphql-java (see #1500) unless the pull request fix is applied.

@andimarek
Copy link
Member

@AnkBurov this example you mentioned is working: See this comment #1500 (comment)

@AnkBurov
Copy link
Author

Yes, I can confirm that this is working. The problem was that the project graphql-java-tools (from kickstart guys, not you) under the hood of schema parsing invokes graphql-java class SchemaGeneratorHelper.buildDirective(..) method that eventually invokes SchemaGeneratorHelper.buildDirectiveInputType(..) that throws the error about not supporting enums.

SchemaGeneratorHelper is marked as internal API so I guess that's the fault of graphql-java-tools for using your project's internal API. Thanks.

@etheran
Copy link

etheran commented Jun 18, 2020

@AnkBurov how did you work around this issue? I'm currently facing the same problem and I'm not seeing a way to overcome this without ditching graphql-java-tools at this point, which really isn't possible in my project.
Thanks!

@AnkBurov
Copy link
Author

@etheran I'm using a fork of the project with this merged pull request AnkBurov@d5429d5
which I synchronize with an upstream once in a while.

Other solutions are related with rewriting graphql-java-tools to use public API of graphql-java instead of an internal one or throw the graphql-java-tools out of my projects completely. Neither of options are realistic or applicable to me.

If you face the same issue (which you do) I suggest you to make a fork with similar changes and use it everywhere where you need a combination of graphql-java-tools from kickstart guys and graphql-java.

Or you can try to persuade graphql-java maintainers to make such changes in the internal part of the API 😄 but it can be a little tricky since they take position it's internal API, don't want to hear anything wada wada wada.

@andimarek
Copy link
Member

@AnkBurov @etheran the original reported issue is fixed: GraphQL Java supports Enums as directive arguments. The original exception also doesn't exist anymore (the whole code sections is changed).

If there is another Issue or something similar please open a new Issue and describe what the problem is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs to be backported a bugfix that still needs to be backported

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants