Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Validation directives on nested Input fields example #385

Open
PhilippS93 opened this issue Feb 12, 2020 · 30 comments · Fixed by #384
Open

Validation directives on nested Input fields example #385

PhilippS93 opened this issue Feb 12, 2020 · 30 comments · Fixed by #384

Comments

@PhilippS93
Copy link

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

@oliemansm
Copy link
Member

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?

@PhilippS93
Copy link
Author

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();

@brendamckenne
Copy link

did you get this resolved anyhow? @PhilippS93

@mohammali
Copy link

Any update?

@PhilippS93
Copy link
Author

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

@brendamckenne
Copy link

brendamckenne commented Apr 4, 2020 via email

@oliemansm
Copy link
Member

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

@brendamckenne
Copy link

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

@oliemansm
Copy link
Member

@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.

@brendamckenne
Copy link

@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.

@oliemansm
Copy link
Member

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

@brendamckenne
Copy link

okay

@oliemansm
Copy link
Member

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.

@brendamckenne
Copy link

brendamckenne commented Apr 4, 2020 via email

@oliemansm
Copy link
Member

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 oliemansm transferred this issue from graphql-java-kickstart/graphql-spring-boot Apr 5, 2020
@oliemansm oliemansm linked a pull request Apr 5, 2020 that will close this issue
@brendamckenne
Copy link

thanks a lot. hoping to have it merged soon

@brendamckenne
Copy link

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

@oliemansm
Copy link
Member

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

@brendamckenne
Copy link

brendamckenne commented Apr 5, 2020

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>

@oliemansm
Copy link
Member

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
Copy link
Member

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>

@brendamckenne
Copy link

brendamckenne commented Apr 5, 2020

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.

@oliemansm
Copy link
Member

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.

@brendamckenne
Copy link

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.

@oliemansm
Copy link
Member

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
Copy link
Member

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
Copy link
Member

@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 oliemansm added this to the 6.0.3 milestone Apr 5, 2020
@PhilippS93
Copy link
Author

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.

@oliemansm
Copy link
Member

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 oliemansm reopened this Apr 17, 2020
@oliemansm oliemansm changed the title Input Validation example Validation directives on nested Input fields example Apr 17, 2020
@KammererTob KammererTob mentioned this issue Jul 4, 2020
2 tasks
@vojtapol vojtapol removed this from the 6.1.0 milestone Dec 8, 2020
@dnagarajan89
Copy link

dnagarajan89 commented Dec 19, 2020

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants