Directive visitArgumentDefinition variable healing

[confuser]

Trying to implement a directive with argument definition support. Receiving an error when variables are used. Not sure if this is to be expected or not?

const typeDefs = `
  type Query {
    books: [Book]
  }
  type Book {
    title: String
  }
  type Mutation {
    createBook(title: String @constraint(minLength: 3)): Book
  }`

Working, custom scalar parseValue method executed:

const data = { query: `mutation createBook {
    createBook(title: "h") {
      title
    }
  }`
}

Following mutation query returns an error

const data = { query: `mutation createBook($title: String) {
    createBook(title: $title) {
      title
    }
  }`,
  variables: { title: 'h' }
}

{ errors:
   [ { message: 'Variable "$title" of type "String" used in position expecting type "ConstraintString".',
       locations: [ { line: 1, column: 21 }, { line: 2, column: 23 } ] } ] }

ConstraintString is the custom scalar field type that is set within visitArgumentDefinition

Possibly caused by the type being swapped and schema healing not taking into account variables?

Code with example tests failing confuser/graphql-constraint-directive#1

[kristianmandrup]

Hi @confuser , perhaps you can identify where in graphql-tools it fails and make a PR to fix it!? :)
I'd love to see this fixed

[FluorescentHallucinogen]

@stubailo @benjamn PTAL.

[nodkz]

- mutation createBook($title: String)
+ mutation createBook($title: ConstraintString)

graphql-constraint-directive replace String type in your schema on ConstraintString via @constraint directive.

You may close this issue.

[confuser]

@nodkz that's incorrect. It should not be required to expose and use the custom type in the schema for an argument definition when it's not needed for an input definition.

[FluorescentHallucinogen]

Just tested, I have a different error:

{
  "errors": [
    {
      "message": "Unexpected error value: [{ message: \"Unknown type \\\"ConstraintNumber\\\".\", locations: [] }, { message: \"Unknown type \\\"ConstraintString\\\".\", locations: [] }]",

P.S. I don't use variables in my query, scalar ConstraintString and scalar ConstraintNumber have added to SDL.

[FluorescentHallucinogen]

If I use variables and use ConstraintString instead of String, I have the same error.

[nodkz]

There are two errors.

The first one was on the client side query (where should be used proper type for variable):

- const data = { query: `mutation createBook($title: ConstraintString) {
+ const data = { query: `mutation createBook($title: String) {

The second one on the server side, where incorrectly works Directives handler, which takes graphql-tool's Directive and wraps/modifies existed types/resolver/field_configs with specific logic.

How to fix the second error, I don't know. Cause I use graphql-tools only for education purposes for newcomers to the graphql world. For real projects with complex schemas where needs to use code generation – I recommend using type-graphql or graphql-compose.

[barbieri]

The issue will happen with all variables that are used with input types following the example at https://www.apollographql.com/docs/apollo-server/schema/creating-directives/#enforcing-value-restrictions.

Once we wrapType() it will replace one scalar with another (ex: LimitedLengthType or ConstraintString and after that the code will fail since allowedVariableUsage() will return false since isTypeSubTypeOf() will return false.

AFAIU there is no way to extend a scalar, say ConstraintString extends String, or LimitedLengthType extends String.

To fix the output side is simple, just follow the @upper example and use the resolver value before your wrapper returns.

However the input is bit trickier, I'll try to see if there is a way to handle that in the resolver as well.

[yaacovCR]

I wonder if this is fixed in graphql-tools-fork, I think healing there is working pretty well, and so you should just be able to set the variable as ConstraintString, type should not be wrapped by directive, should be replaced...

[barbieri]

Hi, it took me some time to rewrite everything we had in a way that variables would work, but it's done and public at https://www.npmjs.com/package/@profusion/apollo-validation-directives

There was no way to solve it other than patching the schema so any field with validated arguments would be wrapped and validation would be done before calling the actual resolver. This is done by ValidateDirectiveVisitor.addValidationResolversToSchema().

While the idea is simple, the implementation wasn't. The SchemaDirectiveVisitor just visit what is directly annotated, so this works:

type T {
  f(arg: String @stringLength(max: 10)): String
}

But this doesn't:

input InputType {
  name: String @stringLength(max: 10)
}
type T {
  f(arg: InputType): String
}

As InputType.name would be visited, but neither of T.f or T.f.arg would... then no resolver would be wrapped with validation.

Then most of my work was to create an extension that annotates GraphQL types with .validation or .mustValidateInput, then we walk the whole schema and check each field if it must be wrapped or not, looking in every argument for any type that must be validated.

Other than that, there was a need to cope with the non-null rules the same way a resolver would do: if an input field fails, it's converted to null if possible, storing the errors at validationErrors, a list that is injected as argument to the resolver. Non-null fields will throw and bubble the null to the next nullable field, eventually failing the resolver if the argument itself is non-null.

This package I published contains the validation directives we've been using internally at our company, they should be general enough or extensible (such as @auth and @hasPermissions() use context-provided functions).

[yaacovCR]

I guess I don't understand why in the latter example, healSchema is not repointing the argument to the new input type with a field with a validating scalar type. That is the point of healing...

I think there were a few errors with healing that I fixed in fork, so if I have time, I will put together an example that tests that.

[yaacovCR]

@barbieri, please see code sandbox demonstrating this working without need for walking entire schema:

Note that the code sandbox differs from the example you referenced above somewhat. The example only validates on output because it only wraps the serialize method; to parse inputs, you must also wrap the parseLiteral and parseValue properties.

An aside: from what I recall, parseValue works on variables, and parseLiteral on the AST, but I think parseLiteral is now optional.

The code sandbox shows this working with the version of apollo-tools/SchemaDirectiveVisitor internal to the latest version of ApolloServer. I also tested with the latest separately bundled versions of both graphql-tools and graphql-tools-fork, and all seemed to be working. I do believe I fixed some things having to do with healing in the fork that were not quite right in upstream graphql-tools, but this does not seem to be one of them.

Let me know if I above is not quite on the mark. Is there a different scenario in which SchemaDirectiveVisitor -- and the schema healing it relies on -- is not working?

[yaacovCR]

Please see earlier discussion as to whether there is indeed a bug at all, also see confuser/graphql-constraint-directive#1 (comment)

[barbieri]

I guess I don't understand why in the latter example, healSchema is not repointing the argument to the new input type with a field with a validating scalar type. That is the point of healing...

I think there were a few errors with healing that I fixed in fork, so if I have time, I will put together an example that tests that.

It was, but then it was failing the input variables.

Recap:

• SchemaDirectiveVisitor doesn't visit T.f.arg or T.f in my example (as expected, as they are not annotated with the directive)
• InputType.name IS changed, the original type (GraphQLString) is replaced with a new GraphQLScalar({...}) created by the visitInputFieldDefinition((
• using query Q($var: String) { f(arg: { name: $var }) } would fail, since the $var: String is of one type (GraphQLString), while the field is of another (new GraphQLScalar({...})).

[barbieri]

Just tested your codebox with the following operation and it fails:

mutation M($title:String){
  createBook(book:{
    title: $title
  }) {
    title
  }
}

{
  "error": {
    "errors": [
      {
        "message": "Variable \"$title\" of type \"String\" used in position expecting type \"LengthAtMost10!\".",
        "locations": [
          {
            "line": 1,
            "column": 12
          },
          {
            "line": 2,
            "column": 28
          }
        ],
        "extensions": {
          "code": "GRAPHQL_VALIDATION_FAILED",
          "exception": {
            "stacktrace": [
              "GraphQLError: Variable \"$title\" of type \"String\" used in position expecting type \"LengthAtMost10!\".",
              "    at Object.leave (/sandbox/node_modules/graphql/validation/rules/VariablesInAllowedPosition.js:59:35)",
              "    at Object.leave (/sandbox/node_modules/graphql/language/visitor.js:345:29)",
              "    at Object.leave (/sandbox/node_modules/graphql/language/visitor.js:395:21)",
              "    at visit (/sandbox/node_modules/graphql/language/visitor.js:242:26)",
              "    at Object.validate (/sandbox/node_modules/graphql/validation/validate.js:73:24)",
              "    at validate (/sandbox/node_modules/apollo-server-core/dist/requestPipeline.js:218:32)",
              "    at Object.<anonymous> (/sandbox/node_modules/apollo-server-core/dist/requestPipeline.js:125:42)",
              "    at Generator.next (<anonymous>)",
              "    at fulfilled (/sandbox/node_modules/apollo-server-core/dist/requestPipeline.js:5:58)",
              "    at process._tickCallback (internal/process/next_tick.js:68:7)"
            ]
          }
        }
      }
    ]
  }
}

[yaacovCR]

Got it, same issue as above commentators, nothing to do with healing. A difference solution would be to use a ValidatedTitle scalar type for the title, and set parse and serialize properties that do what you want. You could also use a -- different -- schema directive that changes the serialize and parse functions declaratively.

Then you can clearly communicate to clients that they should not send a string, but a different scalar type string that satisfies validation. That is the power of graphql, a contract between clients and servers with type safety, not clear to me why you are trying to hide that from clients. Well, I guess clear, as it is simpler, but not sure really harnesses power of graphql.

Having said that, awesome that you wrote your own schema directive to do what you want and double awesome that you made available to all.

If this package was being actively maintained, it would be great to have a list of community schema directives. Maybe there is one somewhere?

Sorry for my initial confusion

[yaacovCR]

A lot of ideas about validation here: graphql/graphql-js#361

Again, apologies for not understanding what you were trying to achieve.

More to help you with modifying schemas, just fyi in the fork, you have multiple options for wrapping resolvers that are exported, including more generic visitSchema and forEachField. Not sure if that is better than your custom function, but just relaying some options.

[yaacovCR]

A possible improvement to the schema directives API might be to set up multiple rounds of directive application ( an array of arrays of SchemaDirectives ? ) and then for the walking the schema bit, that could be set up as a second schema directive on the schema type itself.

If you would find that beneficial, I could add that to the fork, and they you could explicitly call makeExecutableSchema from the fork and send that to ApolloServer. If you or others would find that helpful, that is quite straightforward.

[barbieri]

@yaacovCR the directives are documented in the README.md, I also extended SchemaDirectiveVisitor to provide some things I found useful, they are implemented as EasyDirectiveVisitor (see tests), one of the things is that it will generate the type defs to be used in the schema, including the used scalars, enums and input types. It also implements a sane getDirectiveDeclaration() based on a class-defined (static) config, making it sure the declared locations and arguments exist, patching if it doesn't.

As for the visitor, the SchemaDirectiveVisitor is correct for its purpose. I tried to use one generic SchemaVisitor (same base class used by SchemaDirectiveVisitor), but it's not of help because of makeExecutableSchema.ts#L88:

  if (schemaDirectives) {
    SchemaDirectiveVisitor.visitSchemaDirectives(schema, schemaDirectives);
  }

There is no way I could use my own visitSchemaDirectives as internally this does the filtering at https://github.com/apollographql/graphql-tools/blob/master/src/schemaVisitor.ts#L546-L549

      const directiveNodes = type.astNode && type.astNode.directives;
      if (! directiveNodes) {
        return visitors;
      }

[yaacovCR]

See #858

[yaacovCR]

Thinking about this some more, you could accomplish your own goals of resolver wrapping based on argument input object type members solely within a regular SchemaDirectiveVisitor class by first adding a visitSchema method to your class that walks the schema and marks scalars and inputObjectTypes based on presence of directive within the type.astNode or type.extensionAstNode, then the regular visitFieldDefinition method and check to see whether arguments to field have been marked.

Still a two step process, but doable all as a packagable directive. In fact, because of the visitSchema method available within SchemaDirectiveVisitor, I believe even the most complex schema changes are easily packaged without separate functions...

[barbieri]

this is exactly what I do (algorithm), but you can NOT use SchemaDirectiveVisitor as it's only applied to the locations that were marked with @directiveName (as expected).

input InputType {
   validatedField: String @directiveName
}

type T {
  f(arg: InputType): String
}

If you declare schemaDirectives: { directiveName: SchemaVisitorSubclass }, it won't visit T.f neither T.f.arg

[yaacovCR]

Lol, apologies again, I getcha now. :) Thanks for taking me through it one slow step at a time.

[yaacovCR]

Closing, please see above discussion.

[alexstrat]

@barbieri very clever approach! 👏

I think it your ValidateDirectiveVisitor would deserve an independent module (maybe part of graphql-tools) so that other GQL input validation libraries (like graphql-constraint-directive) could rely on it.

[barbieri]

@barbieri very clever approach! 👏

I think it your ValidateDirectiveVisitor would deserve an independent module (maybe part of graphql-tools) so that other GQL input validation libraries (like graphql-constraint-directive) could rely on it.

As you found, it's already is: https://www.npmjs.com/package/@profusion/apollo-validation-directives

if you think it could go inside graphql-tools, let me know... would be a pleasure to get our code in the project :-)

[alexstrat]

Yes I had found it, I meant a module independent of Apollo, of its applications (auth, stringLength..).
For it to be in graphql-tools I guess it's necessary 🤷‍♂️

[alexstrat]

I went ahead and published graphql-field-arguments-coercion which kind of implements the idea of resolver/validater on Input types (graphql/graphql-js#361, graphql/graphql-js#747).

It's not a ready-to-plug implementation of a directive-based input validation, but a building block that, in combination with graphql-tools, can be used to implement validation, transformation, etc. of input and arguments.
(Of course, it does not transform scalars into custom scalars).

Moreover, it supports asynchronousity and context passing.

Here is the documentation's example for value enforcing written with graphql-field-arguments-coercion:

Source code

import {
  CoercibleGraphQLInputField,
  defaultCoercer,
  CoercibleGraphQLArgument,
  coerceFieldArgumentsValues,
  pathToArray
} from "graphql-field-arguments-coercion";
import { assert } from "chai";
import {
  SchemaDirectiveVisitor,
  makeExecutableSchema,
  visitSchema,
  SchemaVisitor
} from "graphql-tools";
import { GraphQLField, defaultFieldResolver } from "graphql";
import { ApolloServer, ValidationError } from "apollo-server";

const typeDefs = `
directive @length(max: Int!) on FIELD_DEFINITION | INPUT_FIELD_DEFINITION

type Query {
  books: [Book]
}

type Book {
  title: String
}

type Mutation {
  createBook(book: BookInput): Book
}

input BookInput {
  title: String! @length(max: 50)
}`;

// Add the coercer function to every argument definition and input definition
// targeted by the directive.
class LengthDirective<TContext> extends SchemaDirectiveVisitor<{ max: number }, TContext> {
  visitInputFieldDefinition(field: CoercibleGraphQLInputField<string, TContext>) {
    const { coerce } = field;
    field.coerce = async (value, ...args) => {
      // call existing coercers if any
      if (coerce) value = await coerce(value, ...args);

      const { path } = args[1]; // inputCoerceInfo
      const { max } = this.args;

      assert.isAtMost(value.length, max, `${pathToArray(path).join('.')} length`);

      return value;
    }
  }

  visitArgumentDefinition(argument: CoercibleGraphQLArgument<string, TContext>) {
    const { coerce = defaultCoercer } = argument;
    argument.coerce = async (value, ...args) => {
      // call other coercers
      if (coerce) value = await coerce(value, ...args);
      assert.isAtMost(value.length, this.args.max);
      return value;
    }
  }

  visitFieldDefinition(field: GraphQLField<any, any>) {
    const { resolve = defaultFieldResolver } = field;
    field.resolve = async (...args) => {
      const value = await resolve(...args);
      value && assert.isAtMost(value.length, this.args.max);
      return value;
    }
  }
}


const schema = makeExecutableSchema({
  typeDefs,
  resolvers: {
    Mutation: {
      createBook: (_, { book }) => book,
    }
  },
  schemaDirectives: {
    // @ts-ignore
    length: LengthDirective
  }
});

// wrap all fields' resolvers with a use of `coerceFieldArgumentsValues` so that
// we make sure the arguments are valid before calling the resolver — or, we
// throw the appropriate error otherwise
class FieldResoverWrapperVisitor<TContext> extends SchemaVisitor {
  visitFieldDefinition(field: GraphQLField<any, TContext>) {
    const { resolve = defaultFieldResolver } = field;
    field.resolve = async (parent, argumentValues, context, info) => {
      const coercionErrors: Error[] = [];
      const coercedArgumentValues = await coerceFieldArgumentsValues(
        field,
        argumentValues,
        context,
        info,
        e => coercionErrors.push(e)
      );

      if (coercionErrors.length > 0) {
        throw new ValidationError(`Arguments are incorrect: ${coercionErrors.join(',')}`);
      }
      return resolve(parent, coercedArgumentValues, context, info);
    }
  }
}

visitSchema(schema, new FieldResoverWrapperVisitor());