Skip to content

ConstraintString type appeared in graphql schema on the client #2

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

Closed
dbuduev opened this issue Jun 8, 2018 · 34 comments
Closed

ConstraintString type appeared in graphql schema on the client #2

dbuduev opened this issue Jun 8, 2018 · 34 comments
Labels
bug Something isn't working

Comments

@dbuduev
Copy link

dbuduev commented Jun 8, 2018

ConstraintString leaked through introspection. Is it by design?
This makes GraphiQL unhappy.

Error: Invalid or incomplete schema, unknown type: ConstraintString. Ensure that a full introspection query is used in order to build a client schema.

ConstraintString can be revealed by the following query:

query Introspection { 
  __schema {
    types {
      name
      inputFields {
        type {
          name
        }
      }
    }
  }
}
@confuser
Copy link
Owner

confuser commented Jun 8, 2018

No, certainly not by design. Possibly a bug within graphql-tools.

@yvele
Copy link

yvele commented Jun 13, 2018

I have the same problem, and I filled an issue within graphql-tools ardatan/graphql-tools#842

Edit: I'm having this issue when trying to make schema stitching on underlying schema having directive constraints

@koresar
Copy link
Contributor

koresar commented Jun 25, 2018

UPD: ignore the below. The directive stopped working. :)
UPD2: If you'd remove the two scalars below it will make the directive work, but will break client side retrospection. So, choose your destiny. :)


I found a workaround! Omg, took a while.

Add this to your .graphql schema file (if you have one though):

# Current implementation on graphql-import does not support directives declared outside of the scehma (in node.js code).
# We have to redeclare each directive and type for now.
directive @constraint(
    # String constraints
    minLength: Int
    maxLength: Int
    startsWith: String
    endsWith: String
    notContains: String
    pattern: String
    format: String

    # Number constraints
    min: Int
    max: Int
    exclusiveMin: Int
    exclusiveMax: Int
    multipleOf: Int
) on INPUT_FIELD_DEFINITION

scalar ConstraintString

scalar ConstraintNumber

@koresar
Copy link
Contributor

koresar commented Jun 25, 2018

@confuser hey James,

Quick question. In this article
https://dev-blog.apollodata.com/reusable-graphql-schema-directives-131fb3a177d1
they suggest to override field.resolver. See AuthDirective example.

Any reason you went by creating the new ConstraintString type class?

@confuser
Copy link
Owner

@koresar The recommendation seemed to be using custom scalar types to achieve this, rather than overriding the resolver function https://www.apollographql.com/docs/graphql-tools/schema-directives.html#Enforcing-value-restrictions I'm certainly open to switching to a resolver function instead if it eases pains people are facing.

@koresar
Copy link
Contributor

koresar commented Jun 25, 2018

Frankly, I tried this:

class MyConstraintDirective extends SchemaDirectiveVisitor {
    static getDirectiveDeclaration(directiveName) {
        return new GraphQLDirective({
            name: directiveName,
            locations: [DirectiveLocation.INPUT_FIELD_DEFINITION]
        });
    }

    visitInputFieldDefinition(field) {
        field.resolve = async function() {
            throw new Error("not valid");
        };
    }
}

But for some reason the resolve is never hit. The visitInputFieldDefinition is executed for every field though.

So, not sure what to do frankly.

@koresar
Copy link
Contributor

koresar commented Jun 25, 2018

I debugged a little. TL;DR: The makeExecutableSchema does not replace the directive object in schema.

  • The graphql-import people tell you to "declare the @constraint scalar in .graphql file". It effectively creates a directive object in schema. Which is right thing to do IMO. My IDE stops complaining about "undeclared directive!".
  • The makeExecutableSchema accepts the schemaDirectives and applies the schema visitor to them. But it does not replace the previously declared directive generated by graphql-import.

Here is a screenshot. I made it in node.js debugger one step before exiting the makeExecutableSchema. Basically, that schema variable is what I get.
image

@confuser where should I rant about it next? In apollographql/graphql-tools ? :)

@koresar
Copy link
Contributor

koresar commented Jul 4, 2018

I solved the problem. The @constaint is fully working. What I did:

  • ditched the graphql-yoga and the graphql-import.

Turns out Prisma and Apollo packages are not fully compatible.

Now I'm using Apollo Stack v2 (currently RC). The documentation does not match the actual API yet, but I figured all out.

@confuser
Copy link
Owner

confuser commented Jul 4, 2018

@koresar Interesting, so the new apollo-server v2 solves the issue with schema stitching without any changes here? I presume it doesn't resolve the problem you mentioned earlier around overwriting the resolver function instead of using custom scalars?

@koresar
Copy link
Contributor

koresar commented Jul 5, 2018

Sorry, should have mentioned. There were actually a lot of changes.

I had to rewrite schema stitching.

  • Removed all the Prisma's #import statements, and
  • used Apollo's mergeSchema() JS function.

It did solve all the problems I had.

The high level solution: do not use Prisma and Apollo packages in the same project. They are often incompatible.

@koresar
Copy link
Contributor

koresar commented Jul 6, 2018

Ok. I was wrong. Again. No, I did not fix all the problems I have. Even Apollo Stack v2 didn't help.

Currently:

  • Either directive works, but introspection doesn't.
  • Or directive does not work, but introspection works.

If I follow the README.md instructions then I get this error in the GraphQL Playground console: Invalid or incomplete schema, unknown type: ConstraintString.

If I add scalar ConstraintString to my schema then the directive stops working.

I believe that official Apollo docs are wrong: https://www.apollographql.com/docs/apollo-server/v2/features/creating-directives.html#Enforcing-value-restrictions
This way of implementing it does not work.

@koresar
Copy link
Contributor

koresar commented Jul 6, 2018

I've reported the issue here: apollographql/apollo-server#1303

@confuser It is so frustrating. Do you know any other ways the directive can be implemented? I am ready to do it myself. Just need an idea.

@confuser
Copy link
Owner

confuser commented Jul 9, 2018

@koresar Were you not able to overwrite the resolver function at all? If not, there is one other way, but it was deprecated in favour of the current approach, so I'd be hesitant of using it... directiveResolvers

@koresar
Copy link
Contributor

koresar commented Jul 10, 2018

I tried directiveResolvers. Exactly the same issue - the directive resolver function is never executed.
apollographql/apollo-server#1303 (comment)

I tried all kind of approaches (including reimplementing the directive myself). Nothing works.

Basically, the directive is in a non-working state atm.

@confuser
Copy link
Owner

Strange... I have a project that uses directiveResolvers as a form of ACL and it works. I'll find some time and investigate what's happening.

@koresar
Copy link
Contributor

koresar commented Jul 10, 2018

Which graphql-tools version it uses?
Also, is your directive for input type(s)?

@FrankSandqvist
Copy link

FrankSandqvist commented Jul 11, 2018

I'm currently having exactly the same issue. Super frustrating.

Haven't had any success yet. Switched to Apollo server 2 from Yoga

I had an idea of perhaps doing MakeExecutableSchema twice and then combining them, getting the generated validated scalars from the first one into the other one

@FrankSandqvist
Copy link

Woot! Found a workaround.

First make an executable schema for the scalar schema, and then merge that with the rest.

It's hacky and I don't like it, but at least I can move on now.

const scalars = makeExecutableSchema({
  typeDefs: readFileSync(path/to/scalars).toString()
});

const schema = makeExecutableSchema({
  resolvers,
  typeDefs,
  schemaDirectives: {
    identifier: IdentifierDirective,
    access: AccessDirective,
    validate: ValidateDirective
  }
});

const finalSchema = mergeSchemas({ schemas: [scalars, schema] });

@koresar
Copy link
Contributor

koresar commented Jul 12, 2018

Hey @FrankSandqvist
The ApolloServer constructor accepts only one schema.

How did you attach these two schemas to Playground and to the API?

@FrankSandqvist
Copy link

FrankSandqvist commented Jul 12, 2018

I pass in the merged schema finalSchema, also re-pass the resolvers.

It's hacky, again, but it works for now.

const scalars = makeExecutableSchema({
  typeDefs: readFileSync(path/to/scalars).toString()
});

const schema = makeExecutableSchema({
  resolvers,
  typeDefs,
  schemaDirectives: {
    identifier: IdentifierDirective,
    access: AccessDirective,
    validate: ValidateDirective
  }
});

const finalSchema = mergeSchemas({ schemas: [scalars, schema] });

const server = new ApolloServer({
  schema: finalSchema,
  resolvers,
  context: async ({ req }) => ({
    db: new Prisma({
      endpoint: 'http://localhost:4466', // the endpoint of the Prisma API
      debug: true, // log all GraphQL queries & mutations sent to the Prisma API
      secret: process.env.PRISMA_SECRET, // only needed if specified in `database/prisma.yml`,
      // fragmentReplacements: replacements
    }),
    requestUser: await verifyUserEmailFromToken(req.headers.authorization)
  })
});

@FrankSandqvist
Copy link

Of course this still doesn't solve the problem that the scalars are public.

For example StringWithMaxLength50WithMinLength5ThatConformsToEMAIL

@koresar
Copy link
Contributor

koresar commented Jul 12, 2018

Dear @FrankSandqvist

People like you deserve monuments erected.

image

(That's my way of saying "thank you, it works").

@FrankSandqvist
Copy link

Hahah, that's awesome! I've always wanted a thinking Shakespeare ape monument in my name 😄

@pathikdevani
Copy link

Any solution or update on this?

@confuser
Copy link
Owner

Unfortunately not, dependent on apollo

@arvi
Copy link

arvi commented Sep 11, 2018

I am also waiting for a solution to this 😄

@danwetherald
Copy link

danwetherald commented Oct 2, 2018

Still having this issue as well. Is there a plan to fix this?

shfshanyue added a commit to shfshanyue/graphql-sequelize-starter that referenced this issue Nov 2, 2018

Unverified

This user has not yet uploaded their public signing key.
@gerwinbrunner
Copy link

@FrankSandqvist Could you share how your Scalar files that you read in look like?

@padenj
Copy link

padenj commented Apr 30, 2019

I ran across this issue trying to solve a similar issue in a custom directive I created. My situation was slightly different, but I came up with a solution that could work for this issue as well. It would require an update to the directive and I'm not sure how/if it would affect non-Apollo users.

The issue is that a new type is being created in the directive schema visitor, but the type is not being added to the schema type map. For example, https://github.com/confuser/graphql-constraint-directive/blob/master/index.js#L54. If the type is not added to the schema type map, then the above mentioned error "Incomplete or Invalid schema..." will occur when Playground introspects the schema.

The following could be a possible solution. This example could be simplified since the new type is known to be either ConstraintStringType or ConstraintNumberType, but I left it a bit more generic for anyone else who might run across this issue. Note, isNamedType and isWrappingType is imported from 'graphql'.

A possible solution could be to modify the wrapType function:

wrapType (field) {
    const fieldName = field.astNode.name.value
    if (field.type instanceof GraphQLNonNull && field.type.ofType === GraphQLString) {
      field.type = new GraphQLNonNull(new ConstraintStringType(fieldName, field.type.ofType, this.args))
    } else if (field.type === GraphQLString) {
      field.type = new ConstraintStringType(fieldName, field.type, this.args)
    } else if (field.type instanceof GraphQLNonNull && (field.type.ofType === GraphQLFloat || field.type.ofType === GraphQLInt)) {
      field.type = new GraphQLNonNull(new ConstraintNumberType(fieldName, field.type.ofType, this.args))
    } else if (field.type === GraphQLFloat || field.type === GraphQLInt) {
      field.type = new ConstraintNumberType(fieldName, field.type, this.args)
    } else {
      throw new Error(`Not a scalar type: ${field.type}`)
    }

   const typeMap = this.schema.getTypeMap();
   let type = field.type;

    if (isWrappingType(type)) {
           type = type.ofType;
    }

    if (isNamedType(type) && !typeMap[type.name]) {
          typeMap[type.name] = type;
    }
  }

L-L-Cool-J referenced this issue in L-L-Cool-J/graphql-constraint-directive Aug 20, 2019
@mdecurtins
Copy link

Just checking in here; has anybody seen any movement from Apollo on a fix? Should I just consider implementing @FrankSandqvist 's workaround? Thanks!

@koresar
Copy link
Contributor

koresar commented Jan 23, 2020

Hi there. Here is a fresh out of the press new (forked) constraint directive. It's using the idea suggested by @padenj
https://github.com/alexanderVu/apollo-server-constraint-directive

@julianomqs
Copy link

@padenj code does not work if you have the constraintdirective in more than one field of the input, like this:

input CreateUserInput {
  login: String! @constraint(minLength: 1)
  password: String! @constraint(minLength: 1)
}

because of this if:

if (isNamedType(type) && !typeMap[type.name]) {
  typeMap[type.name] = type;
}

Steps of directive resolution:

  1. When the login field is processed by the directive visitor a new ConstraintString scalar is created with fieldName value being login and set in the field type.

  2. In the if above the type created for login field is assigned to the key ConstraintString in the typeMap, in another words becoming the global resolver for ConstraintString scalar type.

  3. When the password field is processed by the directive visitor a new ConstraintString scalar is created with fieldName value being password and set in the field type.

  4. The if above is not executed.

The login and password field will not use the types created for them to resolve their code, but they will use the first type put in the map (login type) instead.

Apparently scalar types in Apollo (and/or GraphQL) can have only one resolver for a scalar type. Theorically it makes sense, because any field of the same type must be serialized or parsed in the same way.

If you don't add @padenj code, the directives will work but not the introspection, and we return to the main problem of this issue.

@RedHotMan
Copy link

@julianomqs creating a specific scalar type using the fieldName per example would be a solution.

class StringConstraintType extends GraphQLScalarType {
  constructor(fieldName, type, args) {
    super({
      name: `StringConstraint${fieldName.toUpperCase()}`,
      serialize(value) {
        value = type.serialize(value);
        validate(fieldName, value, args);
        return value;
      },
      parseValue(value) {
        value = type.parseValue(value);
        validate(fieldName, value, args);
        return value;
      },
      parseLiteral(ast) {
        const value = type.parseLiteral(ast);
        validate(fieldName, value, args);
        return value;
      }
    });
  }
}

@cmartin81
Copy link

cmartin81 commented May 12, 2020

Woot! Found a workaround.

First make an executable schema for the scalar schema, and then merge that with the rest.

It's hacky and I don't like it, but at least I can move on now.

const scalars = makeExecutableSchema({
  typeDefs: readFileSync(path/to/scalars).toString()
});

const schema = makeExecutableSchema({
  resolvers,
  typeDefs,
  schemaDirectives: {
    identifier: IdentifierDirective,
    access: AccessDirective,
    validate: ValidateDirective
  }
});

const finalSchema = mergeSchemas({ schemas: [scalars, schema] });

This works for now! Thanks.
I did almost the same use this

const scalars = makeExecutableSchema({
  typeDefs: `
  scalar ValidateString
  scalar ValidateNumber
`,
});

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests