Validation of nullable input field or argument about apollo-validation-directives HOT 5 CLOSED

we have a documentation inside ValidateDirectiveVisitor about this one:

/**
 * Abstract class to implement value validation in both input and output values
 *
 * This is a general framework inspired by
 * https://www.apollographql.com/docs/apollo-server/schema/creating-directives/#enforcing-value-restrictions
 *
 * However that document uses custom scalars to achieve the validation, using
 * specific `GraphQLScalarType.serialize()`, `GraphQLScalarType.parseValue()`
 * and `GraphQLScalarType.parseLiteral()` to achieve input and output validation.
 *
 * It mostly works, but as seen in
 * https://github.com/apollographql/graphql-tools/issues/789#issuecomment-590143140
 * that will fail on input variables as the variable type and the wrapped target
 * type are different scalars!
 *
 * This class attempts to resolve it in a different way, for output validation
 * the field resolver is wrapped and will call the validation on the resolved
 * value, pretty straightforward and matches
 * [another example](https://www.apollographql.com/docs/apollo-server/schema/creating-directives/#uppercasing-strings)
 * However input objects are trickier since the `SchemaDirectiveVisitor` won't
 * call `visitArgumentDefinition()` unless the argument is annotated with
 * the directive, yet the argument may use an input object type that requires
 * validation (directly or indirectly via nested objects).
 *
 * There is no way to work around such problem and we need to use an external
 * function `addValidationResolversToSchema()` to properly wrap the fields
 * with arguments that requires validation.
 *
 * To match the resolver behavior where exceptions are converted into `null`
 * for nullable fields and the errors are passed along as `errors` array,
 * the failed input object fields will be converted into `null` unless
 * they are `GraphQLNonNull`, and the validation errors are passed along
 * as `validationErrors`, an array of paths and error messages. Use
 * `validateDirectiveSchema` document node to get the input types.
 *
 * If `GraphQLNonNull` is used in the argument type (root), then the resolver
 * will throw and the wrapped resolver won't be called.
 */

from apollo-validation-directives.

ok, since we already had an internal misusage on our own, I think we can add a directive argument policy and default it to fail (ie: throw) and leave the option to let the resolver handle it. Similar to what we have with @hasPermissions().

We can work on this next week, alongside with the move from validationErrors to info, it will lead to us bumping the major version as that one will be a breaking change.

from apollo-validation-directives.

yes, this is designed to make the output and input behave the same. Note you can use the validation on the output as well (ie: to document and make sure your code is respecting it), in such case, the resolver would throw, the field would be made null by Apollo and the errors would contain such record. The input is done in the same fashion, if nullable, the input field or argument is made null and validationErrors get a record (which you can use path to identify).

This gives your resolver the flexibility to handle the situation in a custom way (such as using a default), or throwing that error.

If there is a common pattern to fail even if the field is nullable, maybe we could add a policy argument that fails or replaces with null, like a directive argument that generates a specific error that forces our resolver wrapper to fail.

@alexstrat what do you think?

from apollo-validation-directives.

Yes, I had read this paragraph (and the one in README) and had the idea in mind when submitting the issue.

Specifically, I'm challenging the design choice of matching field resolver behavior.

Indeed, as shown in the example, in graphql-js, if an argument does not match the expected type (coercion failed), the field resolver is actually not called — even if the argument is nullable.

Similarly, by extension of argument value coercion, I would have expected that, when given an invalid argument, a field resolver is not called.

Moreover, I think you could justify this difference of behavior with the spec by interpretation of the difference between resolver value coercion and argument value coercion:

In Executing Fields > Value Completion:

Return the result of “coercing” result, ensuring it is a legal value of fieldType, otherwise null.

In Executing Fields > Coercing Field Arguments

If value cannot be coerced according to the input coercion rules of variableType, throw a field error.

So, I would have expected that the design choice for argument validation follows the principle of argument coercion rather than field coercion.

User has indeed the flexibility to change this behavior, but given the arguments above, I think the default behavior should lean towards reject an invalid argument, nullable or not.

Moreover, I personally think this behavior is generally more common / natural.

from apollo-validation-directives.

fixed on 2.0.0 (available on NPM right now)

from apollo-validation-directives.