prismake / typegql

Create GraphQL schema with TypeScript classes.
https://prismake.github.io/typegql/
MIT License
424 stars 21 forks source link

make InputField nullable by default #47

Open capaj opened 6 years ago

capaj commented 6 years ago

Field is nullable by default, whereas InputField is not. This is inconsistent and also it doesn't make much sense- at least in the API I am building. We have quite a few GQL calls like this:

 mutation {
                        user(id: ${id}) {
                          patch(input: {organisation_role: "role"}) {
                            organisation_role
                          }
                        }
                      }

now if I wanted to edit another field on my user, for example email:

 mutation {
                        user(id: ${id}) {
                          patch(input: {email: "some@email.com"}) {
                            email
                          }
                        }
                      }

so what I end up doing is marking all of my input fields on User entity as isNullable anyway. I've got the alias I've made, but it would make it easier if typegql had input fields as nullable by default.

What do you think @pie6k ?

pie6k commented 6 years ago

I'll think about it, but since I'm thinking now about unifing input and output fields into one universal one anyway, it's loosing it's point a bit so I'll defer decision here.

ps. consider using graphql variables instead of putting your values like this into query string. It's perfect opportunity for having sql injection in your api server.

mutation MyMutatiion($userId: ID!) {
  user(id: $userId) {
    patch(input: { organisation_role: "role" }) {
      organisation_role
    }
  }
}
capaj commented 6 years ago

Sounds good :+1: I'll keep this open because if you decide not to unify, it would be good to at least do this one.

Regarding the mutation variables-I am not sure what that has to do with sql injection. You get SQL injection when you pass raw strings to sql query. I never do that in my apps. Also from the server side these should be equivalent. The graphql spec says:

A GraphQL query can be parameterized with variables, maximizing query reuse, and avoiding costly string building in clients at runtime.

I have it defined as ID in the schema, so using a parametrized query doesn't help me at all. It only makes usage of that query easier on the client.

Even if you passed an SQL command there, it would be escaped, because on the BE I call http://vincit.github.io/objection.js/#findbyid