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

Input Validation #6

Open
AllirionX opened this issue Oct 9, 2019 · 5 comments
Open

Input Validation #6

AllirionX opened this issue Oct 9, 2019 · 5 comments

Comments

@AllirionX
Copy link

Hello,

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

I am using graphql-java-kickstart with a very basic schema, and noticed that input used as arguments of a mutation would not be validated unless in an array with its own rule.

Example:

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

mutation {
  #does not trigger validation
  createUser(userInput: UserInput): User 
  #does not trigger validation
  createUser(userInputs: [UserInput]): [User]
  #trigger validation on both arguments and inputs
  createUser(userInputs: [UserInput] @Size( min : 0, max : 1)): User
}

Am I missing something? Is it the intended behavior or a restriction of graphql-java-tools (if so, it should be documented)?

@AllirionX
Copy link
Author

AllirionX commented Oct 27, 2019

After further code reading/debugging, it looks like the current implementation is buggy when nested inputs are used.
A TargetValidationRules only holds the rule at the first level, and does not validate any other directive. Therefore, if the first level input does not have a directive or have a different directive from the nested input's directive, the nested input's directive is not checked.
In case of several rules being checked on an argument, the validity of the nested input is checked for each rule validation (instead of being tested only once).

I'll do a pull request during the fix those issues.

AllirionX added a commit to AllirionX/graphql-java-extended-validation that referenced this issue Oct 29, 2019
@brendamckenne
Copy link

any luck on this? I am having same issue and my Input type fields/objects doesn't get validated.

@haneev
Copy link

haneev commented Apr 30, 2020

I have found sort of a workaround, such that I can at least have input validation :) maybe this can help some people. It misuses the fact that you can create your own scalar types.

class ValidateFieldException(message: String) : RuntimeException(message)

interface ValidateFieldHandler {
    val name: String

    @Throws(ValidateFieldException::class)
    fun validate(value: Any)
}

class ValidateCoercing<I, O>(
    private val original: Coercing<I, O>,
    private val validateHandler: ValidateFieldHandler
) : Coercing<I, O> by original {
    override fun parseValue(input: Any): I {
        validate(input)
        return original.parseValue(input)
    }

    override fun parseLiteral(input: Any): I {
        validate(input)
        return original.parseLiteral(input)
    }

    private fun validate(input: Any) {
        try {
            validateHandler.validate(input)
        } catch (ex: ValidateFieldException) {
            throw CoercingParseValueException.newCoercingParseValueException()
                .message(ex.message)
                .errorClassification(ErrorType.ValidationError)
                .cause(ex)
                .build()
        }
    }
}

class ValidateDirective(
    private val validateFieldHandlerCreator: ((String) -> GraphQLArgument?) -> ValidateFieldHandler
) : SchemaDirectiveWiring {
    override fun onInputObjectField(environment: SchemaDirectiveWiringEnvironment<GraphQLInputObjectField>): GraphQLInputObjectField =
        environment.element.wrapWithValidateFieldHandler(environment, validateFieldHandlerCreator(environment.directive::getArgument))

    /**
     * Create a new GraphQLType to add validation on a specific type. Like verifying it is a String we add validation rules too, like min-length, max-length.
     */
    private fun GraphQLInputObjectField.wrapWithValidateFieldHandler(
        environment: SchemaDirectiveWiringEnvironment<GraphQLInputObjectField>,
        handler: ValidateFieldHandler
    ): GraphQLInputObjectField = transform { builder ->
        // Generate unique ID for the custom type
        val uniqueFieldId = environment.nodeParentTree.path.reversed().joinToString(separator = "")

        // Handle GraphQLNonNull wrapped object
        val currentType = type
        val unwrappedType = if (currentType is GraphQLNonNull) currentType.wrappedType else currentType

        // Create a new type that has the ValidateCoercing handler and wrap it around the current one
        val newType = when (unwrappedType) {
            is GraphQLScalarType -> {
                GraphQLScalarType
                    .newScalar(unwrappedType)
                    .name("$uniqueFieldId${handler.name}${unwrappedType.name}Validated")
                    .coercing(ValidateCoercing(unwrappedType.coercing, handler))
                    .build()
            }
            else -> currentType
        }

        // Set the new type to the current type
        builder.type(if (currentType is GraphQLNonNull) GraphQLNonNull.nonNull(newType) else newType)
    }
}

class AlphaNumericFieldValidator : ValidateFieldHandler {
    object Creator {
        fun create(arg: (String) -> GraphQLArgument?): ValidateFieldHandler = AlphaNumericFieldValidator()
    }

    private val regex = "^[A-Za-z0-9]*$".toRegex()

    override val name: String = "AlphaNumeric"

    override fun validate(value: Any) {
        if (value is String && !value.matches(regex)) {
            throw ValidateFieldException("Only alpha-numeric allowed, got: `$value`")
        }
    }
}

And on my TypeWiring

directive("AlphaNumeric", ValidateDirective(AlphaNumericFieldValidator.Creator::create))

@briankrug
Copy link

Two stock Instrumentations (MaxQueryDepthInstrumentation and MaxQueryComplexityInstrumentation) call graphql.analysis.QueryTraverser$Builder.build() which throws graphql.schema.CoercingParseValueException if the nested input does not conform to the definition. This causes an unexpected error and results in a 400 http response which is undesirable. These instrumentations should probably catch those errors and add them to the list. There should also be validation of nested input fields in variables before graphql.execution.instrumentation.SimpleInstrumentation.beginValidation() is called.

Stack trace:
graphql.schema.CoercingParseValueException: Variable 'methods' has an invalid value : Invalid input for Enum 'HttpMethod'. No value found for name 'aaaa' at graphql.schema.CoercingParseValueException$Builder.build(CoercingParseValueException.java:46) at graphql.execution.ValuesResolver.coerceValue(ValuesResolver.java:183) at graphql.execution.ValuesResolver.coerceValue(ValuesResolver.java:143) at graphql.execution.ValuesResolver.coerceValueForList(ValuesResolver.java:228) at graphql.execution.ValuesResolver.coerceValue(ValuesResolver.java:159) at graphql.execution.ValuesResolver.coerceValue(ValuesResolver.java:143) at graphql.execution.ValuesResolver.coerceValueForInputObjectType(ValuesResolver.java:200) at graphql.execution.ValuesResolver.coerceValue(ValuesResolver.java:162) at graphql.execution.ValuesResolver.coerceValue(ValuesResolver.java:143) at graphql.execution.ValuesResolver.coerceVariableValues(ValuesResolver.java:71) at graphql.analysis.QueryTraverser.coerceVariables(QueryTraverser.java:64) at graphql.analysis.QueryTraverser.<init>(QueryTraverser.java:60) at graphql.analysis.QueryTraverser.<init>(QueryTraverser.java:40) at graphql.analysis.QueryTraverser$Builder.build(QueryTraverser.java:297) at graphql.analysis.MaxQueryDepthInstrumentation.newQueryTraverser(MaxQueryDepthInstrumentation.java:92) at graphql.analysis.MaxQueryDepthInstrumentation.lambda$beginValidation$2(MaxQueryDepthInstrumentation.java:57) at graphql.execution.instrumentation.SimpleInstrumentationContext.onCompleted(SimpleInstrumentationContext.java:51) at graphql.execution.instrumentation.ChainedInstrumentation$ChainedInstrumentationContext.lambda$onCompleted$1(ChainedInstrumentation.java:238) at graphql.com.google.common.collect.ImmutableList.forEach(ImmutableList.java:405) at graphql.execution.instrumentation.ChainedInstrumentation$ChainedInstrumentationContext.onCompleted(ChainedInstrumentation.java:238) at graphql.GraphQL.validate(GraphQL.java:544) at graphql.GraphQL.parseAndValidate(GraphQL.java:506) at graphql.GraphQL.lambda$parseValidateAndExecute$10(GraphQL.java:475) at graphql.execution.preparsed.NoOpPreparsedDocumentProvider.getDocument(NoOpPreparsedDocumentProvider.java:15) at graphql.GraphQL.parseValidateAndExecute(GraphQL.java:477) at graphql.GraphQL.executeAsync(GraphQL.java:446)

@coarsehorse
Copy link

Any updates on this?

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

No branches or pull requests

5 participants