neo4j-graphql / neo4j-graphql-js

NOTE: This project is no longer actively maintained. Please consider using the official Neo4j GraphQL Library (linked in README).
Other
609 stars 147 forks source link

Schema Transforms are not applied when using graphql-constraint-directive #562

Open gpaluk opened 3 years ago

gpaluk commented 3 years ago

I am getting back no errors when I apply constraints to my new Neo4J Apollo project. I have created a minimal project to illustrate the point at:

https://github.com/gpaluk/apollo-graphql-mfp

When using the following GraphQL query in the playground, the entity gets created which is undesired:

mutation {
  CreateUser(userRole:"admin", username:"a", email:"a")
  {
    userRole
    username
    email
  }
}

I filed an issue at: https://github.com/confuser/graphql-constraint-directive/issues/52 and they redirected me here.

michaeldgraham commented 3 years ago

Hey @gpaluk, thanks for bringing this here, and for sharing your code! It looks like you're using the most recent version of neo4j-graphql-js, 2.18.0. We updated graphql and graphql-tools to the most recent versions, so something was perhaps missed in assessing the differences. I'll try to use the graphql-constraint-directive and get back to you! We probably need to add testing for schema transforms~

gpaluk commented 3 years ago

@michaeldgraham I wonder if there might be an ETA on this or if I can assist with anything? I'm reviewing the viability of this stack but have a close deadline. It seems like this could possibly a minor issue and don't want to have to rule it out. Thank you.

michaeldgraham commented 3 years ago

Hey @gpaluk, so it looks like the upgrade of graphql-tools needed to be fixed, as it was an upgrade to a version that included support for the schemaTransforms argument. The makeExecutableSchema used internally wasn't exposing the schemaTransforms argument, despite it being available in makeAugmentedSchema from neo4j-graphql-js.

So, we've updated the library today to v2.19.0, which internally uses the most recent versions of graphql and graphql-tools - please let me know if this works for you after upgrading :)

Edit: It looks like there's still a problem :( I'll be working on trying to figure out what's going on with the dependencies, we should have it fixed up shortly

Edit 2: Alrighty! It should work with the current version of neo4j-graphql-js v2.19.1, in which graphql-tools is now updated :)

gpaluk commented 3 years ago

@michaeldgraham Stellar start, it seems close for some fields however when I add the @unique generated annotation to the username, that is still ignored also the email constraint doesn't seem to being applied to my example field so it feels like there is some flakiness still. So many thanks for taking a look at this though, like I say, it seems close.

[Edit] I pushed a small update to the original example.

michaeldgraham commented 3 years ago

Heyy @gpaluk, so to use the @unique directive to set a property uniqueness constraint enforced in Neo4j, we use the APOC procedure apoc.schema.assert(), which would be called in a transaction sent to Neo4j by the assertSchema export of neo4j-graphql-js. It looks like you have assertSchema commented out on this line, so if you uncomment it and import assertSchema from neo4j-graphql-js, then the uniqueness constraint should be set in Neo4j.

It looks like the @constraint directive can be used as it's intended, on object type fields for validating queries and on input type fields for validating argument values. But since the neo4j-graphql-js schema augmentation process does not know about the @constraint directive, it isn't carried over onto mutation argument input schema from its use on object type fields. This seems like it could be a nice integration! I'll take a look into applying @constraint directives when generating arguments and input objects used in the Mutation API~

gpaluk commented 3 years ago

That's correct, I'd commented that line out due to the previous version crashing. I can confirm that both the length and unique annotations being applied to username are now working however validation doesn't seem to be picked up at all by the email field.

using the following GraphQL:

mutation {
  CreateUser(
    userRole:"admin",
    username:"admin3",
    email:"bob")
  {
    username
  }
}

email validates without issue, even though the following annotation is applied:

type User {
  userId: ID! @id

  userRole: String!
  username: String! @constraint(minLength: 6, maxLength: 30) @unique
  email: String! @constraint(minLength: 5, format: "email")
}

As can be seen above, bob is neither longer than the minimum string length nor a valid email pattern. Many thanks again for your attention on this.

gpaluk commented 3 years ago

@michaeldgraham - After re-reading your post, I think that you might be alluding to the functionality that I am referring. Am I correct in assuming that there needs to be a check on the data validity earlier in the entity processing system?

J-Krush commented 3 years ago

Bumping this thread. Would love to be able to use the @constraint directive as you said @michaeldgraham: carrying over the "mutation argument input schema from its use on object type fields."

michaeldgraham commented 3 years ago

https://github.com/neo4j-graphql/neo4j-graphql-js/issues/608