Skip to content
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

make InputField nullable by default #47

Open
capaj opened this issue Jun 5, 2018 · 2 comments
Open

make InputField nullable by default #47

capaj opened this issue Jun 5, 2018 · 2 comments

Comments

@capaj
Copy link
Contributor

capaj commented Jun 5, 2018

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
Copy link
Collaborator

pie6k commented Jun 5, 2018

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
Copy link
Contributor Author

capaj commented Jun 5, 2018

Sounds good 👍 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants