Input Validations are not working on non-nullable input object

[shayanths]

Hi extended-validation team,

I'm facing an issue where nested input objects are not being validated by the validation jar.

I've done the following

1. Defined the directive in the SDL

directive @Size(min : Int = 0, max : Int = 2147483647, message : String = "graphql.validation.Size.message") on ARGUMENT_DEFINITION | INPUT_FIELD_DEFINITION

2. Defined the directive annotation against the input field

input NameRequest {
	# The title associated to the name
   	title: String @Size(min : 1, max : 1)
	# The given name
	givenName: String! @Size(min : 1, max : 1)
	# Middle Name
   	middleName: String
   	# Last Name
   	surName: String!
}

3. Add the SchemaDirectiveWiring

@Bean
  public SchemaDirectiveWiring initializeValidators() {
    /*
     * Can add new rules, but on default it will apply the standard set found in {@link
     * DirectiveConstraints}
     **/
    ValidationRules validationRules = ValidationRules.newValidationRules()
        .onValidationErrorStrategy(OnValidationErrorStrategy.RETURN_NULL).build();
    return new ValidationSchemaWiring(validationRules);
  }

I am using graphql-kickstart-springboot, graphql-java 14.1 and graphql-java-tools from graphql-kickstart.

What is successful is argument based validation. The following kicks off the validation

createProgram(
		#Your partner id
		partnerId: String! @Size(min: 1, max:1),
		# The program request
		programRequest: ProgramRequest!) : Program!

I saw an earlier post that nested validations were solved, but I am not seeing ValidationSchemaWiring.onField being called for Input Types.

[shayanths]

@bbakerman are there any plans to fix this issue? Or do you recommend an alternative approach?

[MFoster]

I believe the issue isn't due to the object being nested but that the argument is non-nullable.

Not that it would make sense to have a nullable ProgramRequest object when issuing the createProgram mutation but try removing the declaration in the schema and see if the validation is then executed correctly.

[setchy]

Correct.

I have done some debugging this morning and the culprit seems to be in the graphql.validation.util.DirectivesAndTypeWalker walkInputType method

When declaring the inputType without the !, the unwrappedInputType object is an instanceof GraphQLInputObjectType, and thus is suitable for constraint validations to be applied to it

When declaring the inputType with a !, the unwrappedInputType is of type GraphQLTypeRefrence, and is skipped.

[setchy]

I'm not exactly sure the best way to solution this. @bbakerman - any suggestions

[bbakerman]

In general we need to handle two edge cases in our type code

• type references need to be dereferenced and that type checked
• wrapped types (and especially nulls) need to be handled.

I haven't look at the code on this as yet but in general these tewo cases always need to be handled in all of the places.

Code like unwrapInputType are mean to handle the latter case this but perhaps we didnt consider TypeReferences and hence need to add support for that

[bbakerman]

I made a failing PR as a starting point on this

[bbakerman]

Actually once I get the setup of this right, it is actually working as expected.

Have a look at test and see if it represents the case outlined above. (I think it does).

The types get unwrapped correctly and the errors are as expected.

I only tested on master but I think that and the 14 branch are fairly equivalent

[setchy]

Hi @bbakerman - just checked out the tests you added to #27.

I agree that the additional test issue 17 - type references handled correctly represents the use-case mentioned above AND is behaving as expected. The NameRequest! GraphQLNonNull object is correctly unwrapped to a GraphQLInputObjectType object

I then went and re-created the same Query and NameRequest! over in a standalone graphql service built using com.graphql-java-kickstart:graphql-spring-boot-starter:7.1.0 which uses the latest graphql-java-extended-validation library.

While debugging in this configuration, the NameRequest! GraphQLNonNull object is unwrapped to only a GraphQLTypeReference and hence no validation directives are applied.

Just a guess at this stage - but perhaps it is because the ValidationSchemaWiring is executing too early before the GraphQLTypeReference objects are replaced with actual types.

[setchy]

Just put together a sample project to demonstrate this behavior.
https://github.com/setchy/sample-springboot-kickstarter-graphql-service

There are two queries (see readme); one with a nullable input and the other with a non-nullable input. The former is working as expected. The later not so much

[bbakerman]

I have debugged this some more. It's more a problem (challenge) in the graphql-java library. The reason is this

In graphql-java, the type references are not filled out until the schema is built. That is UNTIL we know all possible types. Only then can we replace them.

However the SchemaGenerator makes types in a tree like fashion. It takes SDL, and starts building out types as it encounters them. It keeps a stack as it goes and if "stack" already contains the type being built (self reference in some way) then it puts in a type reference. This will later being fixed.

However as each type is built, it calls out to the "SchemaDirectiveWiring" which allows them to "transform" a given type in some way. But its doing this BEFORE the type has been truly built, each before type references are replaced.

This "early" call to SchemaDirectiveWiring is there to allow "mutation of a schema type" as its being built. However I dont think we considered type references here and how it might affect things.

I think the solution is in graphql-java and allowing the whole schema to be built AND then walk the tree of all types and allow the SchemaDirectiveWiring to be called and possible mutate a type.

This is a possible fix but not a small one.

[asaunin]

@bbakerman have you created an issue in graphql-java related with the current one?

[pelletier197]

I am sadly blocked by the exact same issue... would you happen to have any "dirty fix" that can do the job until this is fixed? This make the library pretty unusable in my case, since I have global input types used across many graphqls files

With this issue only half of the query inputs get validated, and it's only a matter of "luck" if the field is validated.

[asaunin]

I am sadly blocked by the exact same issue... would you happen to have any "dirty fix" that can do the job until this is fixed? This make the library pretty unusable in my case, since I have global input types used across many graphqls files

With this issue only half of the query inputs get validated, and it's only a matter of "luck" if the field is validated.

While the issue is not fixed, there is no option to use library, cause it does not guarantee proper validation in general 😕

[bbakerman]

This is not quite fixed with the PR - that is now if you have self referencing types there is a StackOverflow

input NameRequest {
	         # The title associated to the name
            title: String @Size(min : 1, max : 1)
	        # The given name
	        givenName: String! @Size(min : 1, max : 1)
	        # Middle Name
   	        middleName: String
   	        # Last Name
   	        surName: String!
   	        
   	        # recursion
   	        inner : NameRequest
          }

eg this one. DirectivesAndTypeWalker will stack overflow with such a type because it walks naively

[bbakerman]

See https://github.com/graphql-java/graphql-java-extended-validation/pull/34/files for a fix to the above

[pelletier197]

See https://github.com/graphql-java/graphql-java-extended-validation/pull/34/files for a fix to the above

Good job 👍 indeed I did not find a way to reproduce the reference type bug in the tests, and I did not think of recursive fields.

[bbakerman]

This should now be released on 15.0.3 and 14.0.3 - I am going to close this issue.

I will open another for reverting back to "schema build time" rule suitability checking when its fixed upstream in graphql-java