No input field null validation on non nullable fields

Issues GraphQL
, ,
1

Report a Dgraph Bug

What version of Dgraph are you using?

Dgraph Version 
$ dgraph version
 
Using dgraph cloud (v20.11.2-rc1-23-gaf5030a5)

Have you tried reproducing the issue with the latest release?

Nope

What is the hardware spec (RAM, OS)?

Dgraph Cloud

Steps to reproduce the issue (command/config used to run Dgraph).

I have a simple schema

type Test  {
	id: ID! 
	name: String! 
}

Dgraph creates the addTest mutation but the following query is valid even though it shoundn’t be as name is non nullable

mutation MyMutation {
  addTest(input: {}) {
    numUids
  }
}

Expected behaviour and actual result.

Dgraph should throw error

1 Like
2

This is a GraphQL issue, moving it to GraphQL topic. :rocket:

3

input: {} is not null. It’s an empty object.

4

Correct, the issue is that the name field should he required or else throw a GraphQL error

5 
mutation MyMutation {
  addTest(input: {name:null}) {
    numUids
  }
}

Fails as expected. An empty object is not prefilled with nulls per JavaScript… it would appear that our definition follows Go’s definition of an empty object: things filled with default values.

Should we change this?

Debate (10 marks)

2 Likes
6

@chewxy if I write a query like

mutation MyMutation {
  addTest(input: {}) {
    numUids
    test {
      name
    }
  }
}

I get a graphql error from dgraph saying name is null, which means name is passed as null by default.

2 Likes
7

Aha, then it is a bug. @graphql FYI.

Accepting as a bug

1 Like
9

I just experienced this:

Ran the following mutation:

mutation{
  addTranscript(input: {name: "test"}){
    numUids
  }
}

Transcript is created.

However, the schema specifies that primaryLanguage is non-nullable, so my expectation is that the mutation should fail because there is no value specified for primaryLanguage:

image

If I removed primaryLanguage from requested return result, then I get the following result:

{
  "data": {
    "queryTranscript": [
      {
        "id": "0x4e21",
        "name": "test",
        "isPublished": null,
        "shareDescription": null,
        "shareTitle": null,
        "slug": null
      },
      {
        "id": "0x4e22",
        "name": "test",
        "isPublished": null,
        "shareDescription": null,
        "shareTitle": null,
        "slug": null
      }
    ]
  },
  "extensions": {
    "touched_uids": 12,
    "tracing": {
      "version": 1,
      "startTime": "2021-05-25T08:20:37.1343371Z",
      "endTime": "2021-05-25T08:20:37.1421826Z",
      "duration": 7845400,
      "execution": {
        "resolvers": [
          {
            "path": [
              "queryTranscript"
            ],
            "parentType": "Query",
            "fieldName": "queryTranscript",
            "returnType": "[Transcript]",
            "startOffset": 317600,
            "duration": 7473600,
            "dgraph": [
              {
                "label": "query",
                "startOffset": 425600,
                "duration": 7355200
              }
            ]
          }
        ]
      }
    }
  }
}

However, if I include it, I get the following:

{
  "errors": [
    {
      "message": "Non-nullable field 'primaryLanguage' (type String!) was not present in result from Dgraph.  GraphQL error propagation triggered.",
      "locations": [
        {
          "line": 12,
          "column": 5
        }
      ],
      "path": [
        "queryTranscript",
        0,
        "primaryLanguage"
      ]
    },
    {
      "message": "Non-nullable field 'primaryLanguage' (type String!) was not present in result from Dgraph.  GraphQL error propagation triggered.",
      "locations": [
        {
          "line": 12,
          "column": 5
        }
      ],
      "path": [
        "queryTranscript",
        1,
        "primaryLanguage"
      ]
    }
  ],
  "data": {
    "queryTranscript": [
      null,
      null
    ]
  },
  "extensions": {
    "touched_uids": 14,
    "tracing": {
      "version": 1,
      "startTime": "2021-05-25T08:22:22.1628428Z",
      "endTime": "2021-05-25T08:22:22.1710665Z",
      "duration": 8223000,
      "execution": {
        "resolvers": [
          {
            "path": [
              "queryTranscript"
            ],
            "parentType": "Query",
            "fieldName": "queryTranscript",
            "returnType": "[Transcript]",
            "startOffset": 278900,
            "duration": 7893000,
            "dgraph": [
              {
                "label": "query",
                "startOffset": 485300,
                "duration": 7677700
              }
            ]
          }
        ]
      }
    }
  }
}

Dgraph version   : v21.03.0
Dgraph codename  : rocket
Dgraph SHA-256   : b4e4c77011e2938e9da197395dbce91d0c6ebb83d383b190f5b70201836a773f
Commit SHA-1     : a77bbe8ae
Commit timestamp : 2021-04-07 21:36:38 +0530
Branch           : HEAD
Go version       : go1.16.2
jemalloc enabled : true

Using Dgraph via Docker locally.

1 Like
10

I’m not sure if there is validation in mutation inputs. I thought it was param-related only. Need to check.

I did a small search about this, no luck even in the GraphQL spec. We need confirmation. If there is validation for mutation objects via input.

Also, you may try to use “strict mode” for mutation. It is a global flag that forces any mutations(DQL, not sure if GQL bypasses it) to follow the DQL schema. It throws an error if the mutation doesn’t reflect the schema.

11

This looks like a solution?

1 Like
12

@MichelDiz

I think the problem here is that putting an ! at the end of a field in a schema seems to have no consequences. It should throw an error if that field does not exist. Quoting the GraphQL Specs:

Here, we’re using a String type and marking it as Non-Null by adding an exclamation mark, ! after the type name. This means that our server always expects to return a non-null value for this field, and if it ends up getting a null value that will actually trigger a GraphQL execution error, letting the client know that something has gone wrong.

The Non-Null type modifier can also be used when defining arguments for a field, which will cause the GraphQL server to return a validation error if a null value is passed as that argument, whether in the GraphQL string or in the variables

This is 100% a bug and needs to be fixed.

(Side not, the docs need to be updated as there is no mention of the effects of ! in dgraph graphql)

I don’t think we need another package to fix an internal problem. However, we could technically also solve the problem by using @auth rules:

type User @auth(
    add: { rule:  "queryPosts(filter: { has: [name, description] }) { id }",
    update: { rule: "won't work here..."
) {

However, as I stated in the below post, this will only work on add and not update, as they will easily be editable until we can create update rules based on the update after value as well as the update before value.

J

2 Likes
13

Think with me, if there is a package doing it, it is because that is a known issue that the GraphQL Specs doesn’t cover. Maybe I’m wrong but we need a strong argument.

But nothing stops us to make it throw an error tho. I’m just reporting possible evidence.

Cheers.

14

So I re-read the specs. I am trying to wrap my brain around why this package would exist.

  • According to graphql specs, a query should trigger an error if a null value exists on a field with !. We know dgraph does not do this, as we use @cascade to get around it. This is a separate issue, but needs to be looked at.
  • We know it does, however, throw an error when passing an argument to a variable with !, which works as expected.

So it seems the package exists to throw an error specifically when using add / update mutations, which is what we want. So, you are right. :shushing_face:

Like I said before, this still could be fixed with the right @auth directive (once you guys look at the field validation needs you can solve with very little coding by adding update-after).

We need to be able to implement the @nonNull directive in Dgraph Cloud, which would be on the Dgraph Team to do.

I am pro doing this.

J

1 Like
15

Pay attention that a Query and a Mutation are totally different things. So both have their own specs. In Dgraph we have this distinction too, only Upsert Block you can do some tricks outside of it. But in a mutation, you can only use RDF or JSON, never a DQL(like cascade directive) functions.

In my view, that’s true via parameters/arguments in a query(or query inputs). I never saw a validation at the mutation level. Maybe someone could get an Apollo Server up and running to check if it does. As the docs aren’t helping much.

I’ll ping some one about this. Does it has a ticket? I think you have some open tickets right?

So, if GraphQL spec doesn’t cover it. We could potentially transpose the code(from graphql-non-null-directive) or do something similar in go and introduce it into the Dgraph core. So the directive would be native.

ping @mrjn @dmai @hardik

16

Correct. Because, the specs do not talk about mutations, just queries (when I said you were right).

No one ever responds to my feature requests one way or the other, so I have no idea if there is an open ticket. Should there be? @amaster507 mentioned this update-after problem a long time ago. I do feel like this is more of an overlooked problem then a feature request:

As I stated in my post, the code (in go) to validate both frontend and backend is already written. It just needs to be connected to a new method: update-after. This opens up a world of backend validation possibilities that frontend developers will love.

Should I create a new feature request thread for that?

Thanks,

J

17

We will, as soon as we have the bandwidth to cover all new features requests and so on. We have focused on urgent bugs and the cloud.

Maybe, if we find out that it is not a bug. So we can open a feature request.

1 Like
18

I am going to say again that this is a bug and here is spec to support this claim of a bug:

http://spec.graphql.org/June2018/#sec-Null-Value

2.9.5 deals exclusively with inputs. Null values can either be specifically provided as null or understood as null if not given.

GraphQL has two semantically different ways to represent the lack of a value:

  • Explicitly providing the literal value: null.
  • Implicitly not providing a value at all.

For example, these two field calls are similar, but are not identical:

{
 field(arg: null)
 field
}

The first has explictly provided null to the argument “arg”, while the second has implicitly not provided a value to the argument “arg”. These two forms may be interpreted differently. For example, a mutation representing deleting a field vs not altering a field, respectively. Neither form may be used for an input expecting a Non‐Null type.

(Added emphasis on the last line)

Go to this live mutation from GraphQL, https://graphql.org/learn/schema/#input-types

And change the variables to [removing stars]:

{
  "ep": "JEDI",
  "review": {
    "commentary": "This is a great movie!"
  }
}

And it will trigger the error

Object of type “ReviewInput” is missing required field “stars”

3 Likes
19

qq, besides the UI(like GraphiQL, Insomnia, and so on) the usual clients throw errors based on the schema? Does it(the validation) work via cURL? is this validation a job for the client or the server? For me looks like a job for the client(Which Dgraph has none for GraphQL) that reads(introspects it) the schema. Right? How Dgraph would control the validation? There is a way to “deactivate” or “forgot to activate” the validation? Feels like something built-in the GraphQL itself, not the implementation code. See?

Something in my guts says that this is really odd. If this is part of the spec, and mandatory. Why Dgraph would be able to unnable it by mistake in the code? I can be wrong and the code has some flaws. But isn’t something missing somewhere? Are you guys using the Client the right way?

I will check this(this week) with different clients and ways to hit queries with other GraphQL solutions and compare the behavior.

20

So, the original spec I quoted above specifically says:

so, in the query context, the value should be a validation error.

However, according to Validation Specs – maybe in the case where something may not be used, we have this:

GraphQL does not just verify if a request is syntactically correct, but also ensures that it is unambiguous and mistake‐free in the context of a given GraphQL schema.

An invalid request is still technically executable, and will always produce a stable result as defined by the algorithms in the Execution section, however that result may be ambiguous, surprising, or unexpected relative to a request containing validation errors, so execution should only occur for valid requests.

Typically validation is performed in the context of a request immediately before execution, however a GraphQL service may execute a request without explicitly validating it if that exact same request is known to have been validated before. For example: the request may be validated during development, provided it does not later change, or a service may validate a request once and memoize the result to avoid validating the same request again in the future. Any client‐side or development‐time tool should report validation errors and not allow the formulation or execution of requests known to be invalid at that given point in time.

So a validation error is not technically what it should do, but it should not execute at all.

J

1 Like
21

Haha, yes we are using the GraphQL clients correctly. I use Apollo, but also test with insomnia and Postman. The errors should be returned no matter what client is used.

It is server side responsibility to return errors not clients.

Maybe you will believe me by looking at Github’s GraphQL API…

https://api.github.com/graphql

Use any GraphQL client with your own auth token to send the following mutation:

mutation { 
  createDiscussion(input: { body: "foobar" }) {
    clientMutationId
  } 
}

And you will get the errors (from the server)

{
  "errors": [
    {
      "path": [
        "mutation",
        "createDiscussion",
        "input",
        "repositoryId"
      ],
      "extensions": {
        "code": "missingRequiredInputObjectAttribute",
        "argumentName": "repositoryId",
        "argumentType": "ID!",
        "inputObjectType": "CreateDiscussionInput"
      },
      "locations": [
        {
          "line": 1,
          "column": 36
        }
      ],
      "message": "Argument 'repositoryId' on InputObject 'CreateDiscussionInput' is required. Expected type ID!"
    },
    {
      "path": [
        "mutation",
        "createDiscussion",
        "input",
        "title"
      ],
      "extensions": {
        "code": "missingRequiredInputObjectAttribute",
        "argumentName": "title",
        "argumentType": "String!",
        "inputObjectType": "CreateDiscussionInput"
      },
      "locations": [
        {
          "line": 1,
          "column": 36
        }
      ],
      "message": "Argument 'title' on InputObject 'CreateDiscussionInput' is required. Expected type String!"
    },
    {
      "path": [
        "mutation",
        "createDiscussion",
        "input",
        "categoryId"
      ],
      "extensions": {
        "code": "missingRequiredInputObjectAttribute",
        "argumentName": "categoryId",
        "argumentType": "ID!",
        "inputObjectType": "CreateDiscussionInput"
      },
      "locations": [
        {
          "line": 1,
          "column": 36
        }
      ],
      "message": "Argument 'categoryId' on InputObject 'CreateDiscussionInput' is required. Expected type ID!"
    }
  ]
}

This thread is becoming laughable. I am starting to really miss Pawan, Chewy, and Michael Compton :frowning:

The presence of a bug is as visible as if the sun was shining at night yet here we are still arguing about whether it is really a bug or not. :man_facepalming: