crash on error on non-nullable field

[raphaelfff]

Is your feature request related to a problem? Please describe.

defer and error on non nullable handling

schema:

type Query {
  """
  Error simulator
  """
  errorSimulator: ErrorSimulatorResult
}

type ErrorSimulatorResult {
  field: String
  resolve(delay: Int, err: Boolean!): ErrorSimulatorResult
    @goField(forceResolver: true)
  resolve2(delay: Int, err: Boolean!): ErrorSimulatorResult!
    @goField(forceResolver: true)
}

query:

errorSimulator {
      resolve2(delay: 500, err: false) {
        field
        ... on ErrorSimulatorResult @defer {
          failed: resolve2(delay: 1000, err: true) {
            field
          }
        }
        ... on ErrorSimulatorResult @defer {
          success: resolve2(delay: 100, err: false) {
            field
          }
        }
      }
    }

this causes

Uncaught TypeError: Cannot read properties of undefined (reading 'field')
code: data?.t2?.resolve2.failed.field

stack:

• @apollo/client 3 or 4
• graphql-sse ^2.5.4
• @graphql-codegen/cli: ^5.0.2

Describe the solution you'd like

mark as optional all fields parent of a @defer on non-nullable field (can be an opt-in config?)

Describe alternatives you've considered

No response

Any additional important details?

I think this is partially due to the specs mess that defer is today, should resolve itself, but in the mean time, this bandaid would allow for safe code to be generated until this is all figured out

[eddeee888]

Hi @raphaelfff ,

Are you seeing the error when codegen runs? Or are the generated types wrong?

(I had a look at the Apollo Client issue and it seems to be a runtime issue?)

Could you please provide a minimal reproduction on GitHub, Stackblitz or CodeSandbox?

This will help me understand and debug the issue 🙂

[raphaelfff]

The generated type is wrong, per the comment on apollo-client, per spec, a field marked a @defer becomes nulllable, and thats not reflected in the types generated by this package, causing crash on userland code.

https://codesandbox.io/p/devbox/optimistic-noyce-kdrtx4

Type should be:

-export type UserQuery = { __typename?: 'Query', user: { __typename?: 'User', id: string, username: string, email: string } };
+export type UserQuery = { __typename?: 'Query', user?: { __typename?: 'User', id: string, username: string, email: string } };

[eddeee888]

Hi @raphaelfff ,

I could see your example operation is this:

query user {
    user(id: 1) @defer {
        id
        username
        email
    }
}

@defer only works on fragment spread and inline fragment based on this doc

---

I've tried changing it to the following and I can see the type being generated correctly:

query user {
    user(id: 1) {
        ...UserFragment @defer
    }
}

fragment UserFragment on User {
    id
    username
    email
}

The generated type is:

export type UserQuery = { 
  __typename?: 'Query', 
  user: 
    | { __typename?: 'User' } & ({ __typename?: 'User', id: string, username: string, email: string } 
    | { __typename?: 'User', id?: never, username?: never, email?: never }) 
};

[raphaelfff]

Ah, slightly messed up my codesandbox sorry...

https://codesandbox.io/p/devbox/optimistic-noyce-kdrtx4?file=%2Fdocument.graphql

so now with

query user {
  ... on Query @defer {
    user(id: 1) {
      id
      username
      email
    }
  }
}

i get

export type UserQuery = { __typename?: "Query" } & (
  | {
      __typename?: "Query";
      user: {
        __typename?: "User";
        id: string;
        username: string;
        email: string;
      };
    }
  | { __typename?: "Query"; user?: never }
);

which isnt making user nullable

[eddeee888]

It is not supposed to be nullable. 🙂

If you look at the response section, a deferred field may not be in the original payload (note: this is different from being null). This is typed as user?: never.

So, in your code you would need to do something like this to achieve type-safety:

// you can't access data.user before the if condition, because it'd be `never`

if(data.user) {
  console.log(data.user.id)
}

[raphaelfff]

What you are suggesting would be flagged by a linter as a "useless conditional" and rely on developer to remember that its defered and to add the check

From a userland pov, using the apolloclient, there is a state where loading is false, i have data, but only partial data, and stuff that is per types non nullable actually is null because the data has yet to come

hence my proposal to add (at least as an opt in) the ability to mark defered fields as optional, so that linters downstream force you to do the right amount of checks in the right places

[raphaelfff]

I rereviewed the example i sent, and indeed it does the right thing...
I now copied my example from the first post, and i can repro.... the real crux of the issue if that a defered field failing should make all parent fields nullable too, which is not the case today
t3.resolve2.failed will return an error, causing a "null" to be returned

In my example (and our real product) whats happening is:

TypeError: Cannot read properties of null (reading 'failed')
on data?.t3?.resolve2.failed?.field

https://codesandbox.io/p/devbox/optimistic-noyce-kdrtx4?file=%2Ffoo.ts%3A8%2C46

[eddeee888]

I'm reading that the first resolve2 is not deferred
i.e. the server MUST return a non-nullable value, but may it is not?

t3: errorSimulator {
    resolve2(delay: 500, err: false) { # No defer, so the server MUST return value here
      field
      ... on ErrorSimulatorResult @defer {
        failed: resolve2(delay: 1000, err: true) {
          field
        }
      }
      ... on ErrorSimulatorResult @defer {
        success: resolve2(delay: 100, err: false) {
          field
        }
      }
    }
  }

But it looks like the server is deferring the value here? (Maybe something to do with the nesting?)
So, the client is receiving deferred value but the type doesn't know that. Should the query be like this?

t3: errorSimulator {
    ... on ErrorSimulatorResult @defer { # Defer the first level as well, since it looks like the server is sending deferred data
        resolve2(delay: 500, err: false) {
          field
          ... on ErrorSimulatorResult @defer {
            failed: resolve2(delay: 1000, err: true) {
              field
            }
          }
          ... on ErrorSimulatorResult @defer {
            success: resolve2(delay: 100, err: false) {
              field
            }
          }
      }
    }
  }

Here are some suggestions to debug further:

1. Try avoiding nesting ErrorSimulatorResult?
2. Try delay: 0 on the first resolve2? (assuming delay: 0 returns the data without deferring.)

[raphaelfff]

Please refer to apollographql/apollo-client#12817 (comment)

Its not defered, but one of its child field is, and is non-nullable, when this field returns an error, the server will patch resolve2 with a null, leading to the crash (it will bubble the nullability up)

1. My schema is not nested irl, thats just illustrating the problem of defered non-nullable field
2. My schema doesnt have a delay irl, thats just simulating DB calls

[eddeee888]

when this field returns an error, the server will patch resolve2 with a null, leading to the crash (it will bubble the nullability up)

This is part of the GraphQL error spec, and is not specific to defer. The generated type works for the expected case i.e. when there is no server error.

Since Apollo is an "error handling" client, you should be able to use the error variable e.g.

const { data, error }  = useQuery(YourDoc);

if(error) {
  // return something early
}

// data should have correct nullability from this point

[raphaelfff]

You can have both data & error, thats perfectly valid GQL response coming out of useQuery, in my case i have (partial) data & error

[eddeee888]

Agreed, it's the default GraphQL behaviour. However, if you are using Apollo Client, the default error policy will NOT return partial data, and the generated type targets this specific scenario.

If you'd like to NOT use an error handling client, you can try the nullability config and semanticNonNull directive

[raphaelfff]

So, we are using apollo with errorPolicy: "all", so thats NOT using an error handling client, as far as i understand.

Did not know semanticNonNull existed, great find, but thats a server side directive, not something that can be applied on consumer side, that makes it not a good fit for the defer problem this thread is about. The codegen tool needs to read defer directive on nullable fields and bubble up the nullability in the types.