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

Cannot return null for non-nullable field when throwing error #5550

Closed
Cretezy opened this issue Jul 27, 2021 · 11 comments
Closed

Cannot return null for non-nullable field when throwing error #5550

Cretezy opened this issue Jul 27, 2021 · 11 comments

Comments

@Cretezy
Copy link

Cretezy commented Jul 27, 2021

Apollo Gateway: 0.33.0

Apollo Federation: 0.26.0

Apollo Server: 3.0.1


I have a federated graph, with a service with the following schema:

# Service A
type User {
  id: ID!
}

type Query {
  me: User!
}

# Service B
type Query {
  permissions: [String!]!
}

In my resolver, it's defined roughly like:

// Service A
me: async (_, __, ctx) => {
  // from `context`
  if(!ctx.user) {
    throw new AuthenticationError("MISSING_AUTHENTICATION")
  }

  return getUser(ctx.user);
}

// Service B
permissions: async (_, __, ctx) => {
  // from `context`
  if(!ctx.user) {
    throw new AuthenticationError("MISSING_AUTHENTICATION")
  }

  return getUserPermissions(ctx.user);
}

With the following query:

{
  me {
    id
  }
  permissions
}

Actual

When querying me without ctx.user set, it returns 2 errors instead of 1.

Errors:

Error: MISSING_AUTHENTICATION

Cannot return null for non-nullable field Query.permissions.

Expected

I believe it should only return a different error.

Errors:

Error: MISSING_AUTHENTICATION

Error: MISSING_AUTHENTICATION
@cactusblew
Copy link

type User {
id: ID
}

make Id nullable. that should return just the error that you are expecting

@cactusblew
Copy link

type User {
id: ID
}

make Id nullable. that should return just the error that you are expecting

Sorry, did not notice the issue was on Query.permissions. Are you sure that permissions results in a valid response. Looks like it yields in a null and hence is complaining about the schema being not optional

@Cretezy
Copy link
Author

Cretezy commented Aug 2, 2021

@cactusblew That seems to be correct, but in this case, it's returning null because the other part of the query is erroring. It seems weird that errors aren't propagated correctly here, as the error is causing another error (when it should only be returning the original error)

@glasser
Copy link
Member

glasser commented Aug 3, 2021

me and permissions fields are siblings of each other so GraphQL evaluates them in parallel and they can have errors separately.

If you expect permissions to only work if auth is set, then its own resolver should check the condition itself, or perhaps you could move permissions to be a field on User that you fetch via me.

After all, if you ran the query { permissions } without me and permissions didn't do its own MISSING_AUTHENTICATION check, where would the appropriate error come from?

@glasser glasser closed this as completed Aug 3, 2021
@Cretezy
Copy link
Author

Cretezy commented Aug 3, 2021

@glasser This is not the behavior that's being described here.

me and permissions throw without authentication. In this case, both are throwing errors, however one is throwing that the authentication is missing (which is correct), and the other is saying it's non-nullable (which is incorrect, since it threw the same error).

I've updated the OP to clarify this. This still seems to be a bug.

@glasser
Copy link
Member

glasser commented Aug 3, 2021

Well, I didn't see your code, so I didn't know that. If you'd like us to look more into it, we need to see the whole code: something we can git clone or on codesandbox.io, say.

@Cretezy
Copy link
Author

Cretezy commented Aug 3, 2021

@glasser I will provide an example tomorrow!

@Cretezy
Copy link
Author

Cretezy commented Aug 15, 2021

Been trying to replicate for a while with no luck. Somehow no longer getting the error in the application (had found a workaround I think) too. If I can find a way to reproduce it again, I should be able to translate it to my Codesandbox. Thanks!

@glasser
Copy link
Member

glasser commented Aug 18, 2021

I think the issue you're talking about is the one I describe at apollographql/federation#159 (comment)

@Cretezy
Copy link
Author

Cretezy commented Aug 18, 2021

@glasser Yes that seems similar to what was my issue!

@paulpdaniels
Copy link
Contributor

paulpdaniels commented Aug 24, 2021

@glasser This broke our frontend as well, we now have two errors which are getting shown. One is meaningful but the second is always "cannot return null for a non-nullable field". Is a new issue needed? I can attempt a fix

abernix added a commit to apollographql/federation that referenced this issue Aug 26, 2021
…xecute (#159)"

This reverts commit 9045f55, which was from
#159.

See the comment in #981
which explains the justification as well, but briefly:

While that PR was an improvement in principle, it:

- has a bug that was first captured in
  #159 (comment)
To quote that here:

  > I think there's a bug here. Let's say we have a query `{ x }` where `x`
is a non-nullable field, and the federated execution has `x` throw an error.
This turns all of `data` into `null` (not `{ x: null }`). But then this
re-execution adds another error saying that the non-null field is null.
  >
  > Take a look at
https://codesandbox.io/s/angry-raman-uuzuf?file=/src/index.js for an example
of what's going on here.

- Sometimes — unexpectedly to current users, at least — breaks client
  expectations.

As of now, the pain points seem to be outweighing the gains.  We'll need to
revert this and revisit this when time allows with a slightly different
approach and #981 tracks
the need to do so.

Ref: #159
Ref: apollographql/apollo-server#5550
Ref: #974
Ref: apollographql/apollo-server#4523
abernix added a commit to apollographql/federation that referenced this issue Aug 26, 2021
…xecute (#159)" (#982)

This reverts commit 9045f55, which was from
#159.

See the comment in #981
which explains the justification as well, but briefly:

While that PR was an improvement in principle, it:

- has a bug that was first captured in
  #159 (comment)
To quote that here:

  > I think there's a bug here. Let's say we have a query `{ x }` where `x`
is a non-nullable field, and the federated execution has `x` throw an error.
This turns all of `data` into `null` (not `{ x: null }`). But then this
re-execution adds another error saying that the non-null field is null.
  >
  > Take a look at
https://codesandbox.io/s/angry-raman-uuzuf?file=/src/index.js for an example
of what's going on here.

- Sometimes — unexpectedly to current users, at least — breaks client
  expectations.

As of now, the pain points seem to be outweighing the gains.  We'll need to
revert this and revisit this when time allows with a slightly different
approach and #981 tracks
the need to do so.

Ref: #159
Ref: apollographql/apollo-server#5550
Ref: #974
Ref: apollographql/apollo-server#4523
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants