No input field null validation on non nullable fields

Issues GraphQL
, ,
22

Okay, fine. Still makes no sense it to be Dgraph the source of the bug. As GraphQL is a a separated thing.

So I never did a GraphQL Server by hand, so I didn’t know that was possible to remove validation from something that was required by design.

Pinging @Naman

23

This is not the Dgraph way as GraphQL is in the Dgraph core as a set of resolvers/rewriters. So yes it is a problem with the GraphQL API endpoint that does not exist on the DQL endpoint, but it is very much still in the hands of Dgraph as I do not host my own GraphQL API on top of DQL like every other GraphQL API in existence. This is why I love Dgraph! But if I have to fight this hard to get simple bugs even recognized then who knows… all my eggs are in one basket here and I am trusting this basket to Dgraph engineers to fix bugs I cannot (wish I knew golang). Please don’t make me look for a new basket as there is nothing even close in comparison.

1 Like
24

We are in the same team/page here. I’m just asking/arguing for myself. No one asked me to do so. It is my personal interest to understand if there aren’t any other simple mistake somewhere before increasing the priority(I mean, push this with the devs). As I’m not a Go Lang dev and there are users interested in this, I came to help kind of “filtering” possible unseen problem in the context.

It is like science right? We add hypotheses and test them with what we can. All docs, procedures, and ideas should be evaluated.

Sidetopic: There is a book from Bertrand Russell(if I’m not mistaking, but the book exists) that has an entire volume just to prove that 1 + 1 = 2. It is funny, but it is a serious thing.

Thanks for helping speeding up the process. I’ll advocate for this.

Cheers.

3 Likes
25

Thanks Michel. Definitely expecting Dgraph’s graphql API to return the error—looking forward to seeing this issue fixed, bugs are less likely to occur when the API is throwing useful errors.

2 Likes
27

We have acknowledged it internally and will be working towards it.

5 Likes
28

Hi @minhaj we are also looking at this issue to be fixed. In the absence of errors thrown during add/update mutations for non-nullable fields marked with (!), it can very quickly lead to inconsistent data while the queries throw an error. We are trying to be vigilant around this issue during data creation/updates and would like to know a timeline around this getting shipped?

29

CC: @aman-bansal can you take a look at this?

30

This should be fixed via chore: adding order to the validation rules by aman-bansal · Pull Request #17 · dgraph-io/gqlparser · GitHub</t and fix: add validation of null values with correct order of graphql rule validation by aman-bansal · Pull Request #8.

It will be available in the next public release.

4 Likes
31

Sorry for resurrecting this old thread, but this bug seems to be present yet, on dgraph/dgraph:latest on docker.

Let’s have the following schema:

type Person {
  id: ID!
  name: String!
  firstname: String!
}

This mutation gives expected result:

mutation {
	addPerson(input: {
		name: "Test"
		firstname: null
	}) {
		person {
			id
			name
			firstname
		}
	}
}

{
	"errors": [
		{
			"message": "Expected type String!, found null.",
			"locations": [
				{
					"line": 4,
					"column": 14
				}
			]
		}
	]
}

But this mutation shows that input doesn’t get null validation:

mutation {
	addPerson(input: {
		name: "Test"
	}) {
		person {
			id
		}
	}
}

{
	"data": {
		"addPerson": {
			"person": [
				{
					"id": "0x4"
				}
			]
		}
	}
}
32

Why is this working on cloud but not when self-hosted ?

On Cloud I have the error : couldn't rewrite mutation addUser because failed to rewrite mutation payload because type User requires a value for field name, but no value present

Same scenario on a fresh dgraph/standalone:v21.03.2 and dgraph/standalone:latest, the mutation is allowed.

I tried to set alpha flag --limit mutations=strict but this does not seem to change anything.

33

Edit : OK I think this time I narrowed down and understood the issue : if I take again the example above, this mutation gives the problem with no input validation :

mutation {
	addPerson(input: {
		name: "Test"
	}) {
		person {
			id
		}
	}
}

But the correct syntax, which gets validated as expected, is as follows (please note the [ ], as the input should be a list) :

mutation {
	addPerson(input: [{
		name: "Test"
	}]) {
		person {
			id
		}
	}
}

IMHO, this should at least be documented to avoid others wasting hours on this issue !

Thanks again for all the hard work !

1 Like