[v4] Regression: permission denied on function leads to data:null in response

[EyMaddis]

Hi,
I found a regression when comparing v3 to v4, here is my test setup.

I blocked DELETE for tables, blocking deleteUserById and blocked execute for the function removeUserById.
The following tests are identical, except for the line highlighted with FIXME.

The first test case does break with v3 with AssertionError: expected { removeUserById: null } to deeply equal null, which is the behavior I expected.

it.only('should not allow removal of other users', async () => {
    const response = await authedRequest(REMOVE_USER_BY_ID_MUTATION, { id: OTHER_USER_ID })
    const { body: { data = 'test', errors} } = response

    expect(data).to.eql(null) // FIXME should be { removeUserById: null }
    expect(errors).to.be.an('array').with.lengthOf(1)
    expect(errors[0]).to.have.property('message').to.include('permission denied')

    snapshot(response.body)
})

it('should not allow user deletion via default mutation', async () => {
    const response = await authedRequest(DELETE_USER_BY_ID_MUTATION, { id: OTHER_USER_ID })
    const { body: { data = 'test', errors} } = response

    expect(data).to.eql({ deleteUserById: null })
    expect(errors).to.be.an('array').with.lengthOf(1)
    expect(errors[0]).to.have.property('message').to.include('permission denied')

    snapshot(response.body)
})

[benjie]

Could you provide a minimal pg_dump (schema and data), postgraphql command line and GraphQL query that can be used to reproduce this please?

[benjie]

Here's my attempt to reproduce the issue; but data still contains deletedTypeById as you'd expect: graphile/graphile-engine#59

Are you using the very latest v4?

[EyMaddis]

@benjie I also tried the latest version.

The error only happens with missing grants:

CREATE OR REPLACE FUNCTION public.test() RETURNS boolean LANGUAGE 'sql' AS $func$ SELECT true $func$;
REVOKE EXECUTE ON FUNCTION public.test() FROM PUBLIC;

mutation regression {
  test(input: {}) {
    clientMutationId
    boolean
  }
}

{
  "data": null, // <---
  "errors": [
    {
      "message": "permission denied for function test",
      "locations": [
        {
          "line": 29,
          "column": 3
        }
      ],
      "path": [
        "test"
      ]
    }
  ]
}

[chadfurman]

@EyMaddis in v3, are you saying the data field isn't there?

If we change this at this point, it's a breaking change. Some people might be relying on the combination of errors and data:null to indicate specific failure reasons.

[benjie]

If the behaviour between v3 and v4 differs then the v3 behaviour wins unless there's a good reason to change it. Every version bump of v4 alpha should be seen as a major bump - it's an alpha. Once it's at general availability we will be making schema changes breaking changes but we're not there yet.

I've not decided which behaviour is the correct behaviour in this issue yet, haven't had time to look into it. I think it has to return the mutation payload field otherwise you cannot access the clientMutationId.

[EyMaddis]

@chadfurman in v3 it returns { data: { removeUserById: null }} in v4 that is: { data: null }.

@benjie I would say that the v3 way is the correct one, as I could also query other information within one request. This either breaks with this behavior (did not verify this) or you have to use always check if data exists...

[benjie]

I suspect what has happened here is that I've marked it as "non null" and thus GraphQL has cascaded the null upstream to the root, which is definitely not what we want.

However; what I'd expect is to be able to get { data: { removeUserById: { clientMutationId: "...", result: null } } } so I'm not sure either v3 or v4 are correct. I'm going to have to read the Relay mutations specification again but I don't have time this week.

[benjie]

So I read the mutations spec and it doesn't really account for errors. I'm not sure what clientMutationId is for, nor how it's handled if an error occurs and hence it ends up missing. I think it might be an artifact of how Relay classic worked anyway, so I'm not sure how important it is.

I've changed mutation payloads back to nullable which should solve this.