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

Validation directives on nested Input fields example

Open PhilippS93 opened this issue 5 years ago • 30 comments

Hello,

Thank you for your work on this library. This is very helpful.

I want to use a directive to validate input fields like so:

input UserInput { name: String @Size( min : 2, max : 255) }

There is a callback in SchemaDirectiveWiring:

default GraphQLInputObjectField onInputObjectField(SchemaDirectiveWiringEnvironment<GraphQLInputObjectField> environment) {
        return environment.getElement();
    }

but there is no example on how to use this method to validate the input. There are only examples for onArgument and onField callbacks.

Is it possible to provide examples for this kind of validation?

Thanks

PhilippS93 avatar Feb 12 '20 14:02 PhilippS93

Hi,

I would think implementation of the onInputObjectField callback would be similar to the samples of onArgument and onField. Isn't it possible to deduce it from those samples?

oliemansm avatar Feb 12 '20 15:02 oliemansm

Hi @oliemansm . I have tried to use the onField example but I am getting the error: Caused by: graphql.AssertException: An output field must be in context to call this method on calling

DataFetcher originalFetcher = environment.getFieldDataFetcher();

PhilippS93 avatar Feb 12 '20 22:02 PhilippS93

did you get this resolved anyhow? @PhilippS93

brendamckenne avatar Apr 03 '20 20:04 brendamckenne

Any update?

mohammali avatar Apr 03 '20 23:04 mohammali

@sumitbadaya27 , no the provided examples are unfortunately not working. @oliemansm , any ideas?

PhilippS93 avatar Apr 04 '20 11:04 PhilippS93

Yeah not working for me too. We are using kickstart with java extended validations library.

The library works but then resolvers dont. And if resolvers work then validation wont.

On Sat, 4 Apr, 2020, 5:14 PM Philipp Staudt, [email protected] wrote:

@sumitbadaya27 https://github.com/sumitbadaya27 , no the provided examples are unfortunately not working. @oliemansm https://github.com/oliemansm , any ideas?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/graphql-java-kickstart/graphql-spring-boot/issues/341#issuecomment-609016064, or unsubscribe https://github.com/notifications/unsubscribe-auth/AL37CYNWB2EZLBPYRM6IBLLRK4MRDANCNFSM4KT3JFZA .

brendamckenne avatar Apr 04 '20 11:04 brendamckenne

I’ll try to take a look at it this weekend.

oliemansm avatar Apr 04 '20 12:04 oliemansm

Thanks In case you'd need the link of library for validations, Its here: https://github.com/graphql-java/graphql-java-extended-validation

brendamckenne avatar Apr 04 '20 13:04 brendamckenne

@sumitbadaya27 You're issue seems to be different than what @PhilippS93 raised. You're not getting the validators from that library to work with this project apparently. I don't know how you configured it, but they don't provide a starter but just a library. So you have to expose their SchemaDirectiveWiring into a SchemaDirective bean to be actually configured.

@PhilippS93 graphql-java hasn't documented this either and they refer to https://github.com/graphql-java/graphql-java-extended-validation for example implementation. It uses a FieldDefinition and walks the arguments and input types based on that. I'll see if I can get something like that to work in the sample.

oliemansm avatar Apr 04 '20 15:04 oliemansm

@oliemansm Is there any way through in your library in which we can pass typeDefinitionRegistry and runtimeWiring in generating schema in schemaParser builder? basically this thing: GraphQLSchema graphQLSchema = new SchemaGenerator().makeExecutableSchema(typeDefinitionRegistry, runtimeWiring);

Also, I configured it correctly as I am getting proper validation message but its just that I am not able to add resolvers and my resolvers fail to work if Validation works.

brendamckenne avatar Apr 04 '20 15:04 brendamckenne

@sumitbadaya27 Create a separate issue for that please, this particular issue is not about that library or these questions.

oliemansm avatar Apr 04 '20 15:04 oliemansm

okay

brendamckenne avatar Apr 04 '20 15:04 brendamckenne

The problem is that graphql-java-tools wires the directive on objects and its fields without passing along the constructed GraphQLInputObjectType. That's what is preventing that extended-validation library to fail too. Because it does an instanceof check in case the argument is a GraphQLInputObjectType, while in fact at that point because of the logic in graphql-java-tools, it is a GraphQLTypeReference.

By making some quick changes in graphql-java-tools I was able to pass it along and got it working. Needs a bit more work and clean-up though.

oliemansm avatar Apr 04 '20 19:04 oliemansm

Till the time you cleanup the code, Can you please provide the code that you tweaked ans tried and the location where i need to put that code? So that my work also doesnt stop at this as i am blocked coz of this

On Sun, 5 Apr, 2020, 1:18 AM Michiel Oliemans, [email protected] wrote:

The problem is that graphql-java-tools wires the directive on objects and its fields without passing along the constructed GraphQLInputObjectType. That's what is preventing that extended-validation library to fail too. Because it does an instanceof check https://github.com/graphql-java/graphql-java-extended-validation/blob/bef87f7cbfd6d970aba413eaa51c57852470f77f/src/main/java/graphql/validation/util/DirectivesAndTypeWalker.java#L31 in case the argument is a GraphQLInputObjectType, while in fact at that point because of the logic in graphql-java-tools, it is a GraphQLTypeReference.

By making some quick changes in graphql-java-tools I was able to pass it along and got it working. Needs a bit more work and clean-up though.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/graphql-java-kickstart/graphql-spring-boot/issues/341#issuecomment-609080224, or unsubscribe https://github.com/notifications/unsubscribe-auth/AL37CYLOJJFMOXTJH24FABDRK6FKNANCNFSM4KT3JFZA .

brendamckenne avatar Apr 04 '20 19:04 brendamckenne

I'll push a snapshot release of graphql-java-tools when it's ready to do so and push the updated samples containing the example code then too.

You can do validation on the resolvers themselves in the meantime. That's a valid workaround available to you, so you're actually not blocked by this.

oliemansm avatar Apr 04 '20 19:04 oliemansm

thanks a lot. hoping to have it merged soon

brendamckenne avatar Apr 05 '20 06:04 brendamckenne

Whoa! great. so maven version updated? Or how do I get the updated version?

brendamckenne avatar Apr 05 '20 06:04 brendamckenne

Use snapshot version 7.0.2-SNAPSHOT of graphql-spring-boot-starter. You have to add a snapshot repository to be able to use it, see: https://github.com/graphql-java-kickstart/graphql-spring-boot#snapshots

oliemansm avatar Apr 05 '20 06:04 oliemansm

After using snapshot version of spring boot starter, suddenly my GraphQLResolver.java and such classes are not found. "Cannot resolve symbol 'GraphQLResolver" getting this error. Can you please guide as to how to use this update that you made for validations and input objects?

My POM.XML dependency is as below:


<repositories>
        <repository>
            <id>jfrog-snapshots</id>
            <name>oss-jfrog-artifactory-snapshots</name>
            <url>https://oss.jfrog.org/artifactory/oss-snapshot-local</url>
        </repository>
    </repositories>

        <dependency>
            <groupId>com.graphql-java-kickstart</groupId>
            <artifactId>graphiql-spring-boot-starter</artifactId>
            <version>7.0.1</version>
            <scope>runtime</scope>
        </dependency> 

I guess I have to use this dependency as well?

 <dependency>
            <groupId>com.graphql-java-kickstart</groupId>
            <artifactId>graphql-java-tools</artifactId>
            <version>${graphql.java.tools.version}</version>
        </dependency>

brendamckenne avatar Apr 05 '20 06:04 brendamckenne

In recent versions base packages have been migrated as stated in the release notes. Remove the imports and your IDE should already help you importing the correct versions (graphql.kickstart based packages).

No you don't include graphql-java-tools directly. Rely on graphql-spring-boot-starter to import graphql-java-tools. In the POM you've described above you're importing GraphiQL, not graphql-spring-boot-starter.

oliemansm avatar Apr 05 '20 06:04 oliemansm

Come on, just read.... This is costing me a lot of time that's totally unnecessary. I said to use version 7.0.2-SNAPSHOT of graphql-spring-boot-starter and you're clearly using 7.0.1. Don't pull in graphql-java either, that's done for you by graphql-spring-boot-starter too.

<dependency>
    <groupId>com.graphql-java-kickstart</groupId>
    <artifactId>graphql-spring-boot-starter</artifactId>
    <version>7.0.2-SNAPSHOT</version>
</dependency>

oliemansm avatar Apr 05 '20 07:04 oliemansm

ty

confusion was created by the link you shared ": https://github.com/graphql-java-kickstart/graphql-spring-boot#snapshots" It has 7.0.1 snapshot version so I got confused by it.

brendamckenne avatar Apr 05 '20 07:04 brendamckenne

Then try to add the graphql-java dependency anyway. You should start trying to investigate, debug and figure out why certain errors pop-up instead of asking questions each time you run into something. Programming isn't copy pasting code around until something just magically works. It's about understanding how and why something works and applying that. In the end and also in this case you'll only get that kind of understanding by following the code.

If everything else in this library is working as it should and the problem with that type not being found is limited to your use case and configuration then there's no reason to assume it'll be solved by a next release. Might just be caused by that library you're trying to use.

I'm not using directives on input objects, and I'm not using that extended validator directive library either. And I'm not getting paid for the time I put in this project, so there's no incentive whatsoever for me to put in these hours in my free weekend. I'm going to limit my time now to just fixing this original input validation example, and not to the support of that library. If you keep running into problems with that library icw this library then I suggest you figure out the cause and if it's related to a bug or something in this project create a PR to fix it.

oliemansm avatar Apr 05 '20 07:04 oliemansm

I have figured it out already and fixed it. Was about to post message but was afraid you'd not like it to see another message . thanks for the fixes and support till now.

brendamckenne avatar Apr 05 '20 07:04 brendamckenne

Can you leave the cause and solution here in a comment then so anybody else that runs into the same problem you ran into will know how to solve it?

oliemansm avatar Apr 05 '20 07:04 oliemansm

Thanks @brendamckenne. You might as well delete that graphql-java-tools from your POM entirely btw and just let graphql-spring-boot-starter handle that dependency. That prevents compatibility issues that might otherwise easily occur. We try to update graphql-spring-boot-starter in sync with graphql-java-tools.

oliemansm avatar Apr 05 '20 08:04 oliemansm

@PhilippS93 Back to your original question. I've updated the sample (not the documentation). Take a look at this RangeDirective. It works on both a regular argument and input object. The solution is based on graphql-java-extended-validation from the guys that develop graphql-java, the library that provides the directive implementation in the first place.

Documentation on their end is lacking too for this particular use case. Basically how it works is you don't override the onArgument or onInputObjectField methods, but you override the onField method. Then you iterate over the arguments of that field checking if your directive applies or not. You need to walk up over input object types as well in that case and take lists into account if you need to (this sample doesn't do that btw).

Basically you need to write quite a bit of code to get the result of what you'd want. That's because graphql-java provides an API trying to give you us much flexibility as possible, but in this case making it a bit more difficult to work with.

Perhaps it would be possible to come up with an interface in graphql-spring-boot-starter to simplify it.

oliemansm avatar Apr 05 '20 09:04 oliemansm

Thank you @oliemansm . I have tested the new code but unfortunately it does not work for my case (see below for an example):

directive @Size(min : Int = 0, max : Int = 2147483647) on ARGUMENT_DEFINITION | INPUT_FIELD_DEFINITION

input NameInput {
    forename: String! @Size(min : 3, max : 25)
    surname: String! @Size(min : 3, max : 25)
}

input ChangeUserInput {
    name:NameInput
}

type Mutation {
    changeUser(input: ChangeUserInput): User!
}

It seems that only ChangeUserInput is considered in the onField method, but not NameInput.

PhilippS93 avatar Apr 16 '20 23:04 PhilippS93

My example code doesn't traverse all the way up into nested input objects, so you'd have to add that kind of logic to be able to get that working. As I've indicated above it isn't the most developer friendly solution and some kind of convenience interface for this would come in handy to make it easier to work with.

I'll reopen the issue and rename it for nested input directives.

oliemansm avatar Apr 17 '20 06:04 oliemansm

@oliemansm I tried using the range directive logic for an InputObjectType like this.

type Mutation { addBook(bookInput: BookInput!) : Book! }

input BookInput { id: Int! @range(minimum: 4, maximum:10) name: String! }

As I have used non null('!') for InputObjectType in the mutation, while unwrapping the non-null I am getting a GraphQLTypeReference instead of GraphQLInputObjectType in this LOC https://github.com/graphql-java-kickstart/samples/blob/master/directives/src/main/java/directives/RangeDirective.java#L38. Is there a way to get the GraphQLInputObjectType from GraphQLTypeReference to navigate the fieldDefinitions and look for directive ?

If I use the mutation parameter without non null, the logic is working fine.

addBook(bookInput: BookInput)

Please let me know if there is a way around.

dnagarajan89 avatar Dec 19 '20 07:12 dnagarajan89