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 https://github.com/apollographql/apollo-client/issues/12817#issuecomment-3139743770

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.

[eddeee888]

I'll reinforce that there are 2 things in this convo here: error handling and defer.

TLDR; server returns fields with runtime errors as null for any use case, not only for defer.

If you are using non-error-handling client, you can apply semanticNonNull on the server and use nullability config to get the desired type.

The codegen tool needs to read defer directive on nullable fields and bubble up the nullability in the types.

As far as I know, defer spec doesn't say anything about nullability. What is happening is purely GraphQL error IMO.

[eddeee888]

Closing this issue as the generated type is working as intended. Please let me know if this is not correct and I'll re-open

[raphaelfff]

Sorry i missed your reply, semanticNonNull is a server config, defer is a client opt-in, if the client decides to defer a field, that config cannot be in the server ! According to the apollo guys, the nullability on error is per spec: https://github.com/apollographql/apollo-client/issues/12817#issuecomment-3139743770

[ns-jimmyw]

@eddeee888 Can we reopen this issue please? As you mentioned in the example above, a deferred field may not be in the field, but when it is, it can also be null when server fails. The generated code is not representative of the actual data.

[eddeee888]

When the server fails, any non-nullable field can be null, not just deferred fields AFAIK. You can see more here:

https://youtu.be/odwQUAkmW44?si=njX5xTw88EQMdyL1

TLDR; there are a few options here:

• use ok/error payload wrapper types to represent errors in payload
• use semanticNonNull on the server, and appropriate nullability codegen option to achieve correct type. Codegen generates the type to match the "error handling client" by default, and you can change it if needed.

[ns-jimmyw]

Correct any non-nullable field can be null, I agree with that, but the nullability bubble up to the "closest nullable parent position".

A deferred field seems to be nullable as it is the highest "data" point in the inner query, and we want null on the deferred field.

use ok/error payload wrapper types to represent errors in payload

We get both partial data and error through errorPolicy: "all". We show partial data even if there are errors. That's why we need this type change, because the deferred value can be present and null while error is thrown.

[raphaelfff]

And i will also emphasize again, your repeated proposal of using semanticNonNull (which is a SERVER-side annotation) is incompatible without our desire of using defer which is a CLIENT-side annotation, they achieve different goals that are unrelated, I'm not gonna be able to ask github to change they API to add semanticNonNull where i want to defer data on my frontend...

[eddeee888]

A deferred field seems to be nullable as it is the highest "data" point in the inner query, and we want null on the deferred field.

From my testing, this is already happening if your deferred field is nullable. Consider this schema:

extend type Query {
  errorSimulator: ErrorSimulatorResult
}
type ErrorSimulatorResult {
  field(err: Boolean): String
  nnfield(err: Boolean): String!
  resolve(wait: Int, err: Boolean): ErrorSimulatorResult
  resolve2(wait: Int, err: Boolean): ErrorSimulatorResult!
}

May I ask which of the scenario below is not working correctly? 🙂 (please let me know I'm missing your scenario)

Scenario 1: nullable field of a nullable object

The field becomes null when an error happens

Scenario 2: non-nullable field of a nullable object

The object becomes null when an error happens

Scenario 3: nullable field of a non-nullable object

Scenario 4: non-nullable field of a non-nullable object

Types generated is also saying nonnullable_field can be undefined e.g. when an error happens:

---

And i will also emphasize again, your repeated proposal of using semanticNonNull (which is a SERVER-side annotation) is incompatible without our desire of using defer which is a CLIENT-side annotation, they achieve different goals that are unrelated, I'm not gonna be able to ask github to change they API to add semanticNonNull where i want to defer data on my frontend...

Ah gotcha, you don't have access to server code. Type generation is based on schema types (server) as the base. 🙂 So if there's a bug in the client type (scenario above), we can fix it

[eddeee888]

Hi @raphaelfff @ns-jimmyw , please let me know if this is still an issue.

[ns-jimmyw]

Hi @eddeee888 yes this is still an issue.

[eddeee888]

Hi @ns-jimmyw ! Could you please let me know which scenario is the problem/incorrect? 🙂

[ns-jimmyw]

@eddeee888 They're all problematic, but only 4 makes it obvious because the error bubbles up to the parent. The generated type assumes the non-nullable field is always defined, which is not the case when @defer fails.

[eddeee888]

They're all problematic, but only 4 makes it obvious because the error bubbles up to the parent.

Error always bubble to the nearest nullable parent (starting with the field), regardless if defer is used or not.

The generated type assumes the non-nullable field is always defined, which is not the case when @defer fails.

This is the correct behaviour as far as I understand the type.

In that example:

• the parent is either an object (success case) or undefined (error case)
• if the parent is an object (success case), then the field is non-nullable.
• if the parent is undefined (error case), then there's no field s

[raphaelfff]

Error always bubble to the nearest nullable parent (starting with the field), regardless if defer is used or not.

Errors bubble to the nearest NULLABLE parent in non-defer Errors bubble to the DIRECT parent in defer Thats the different, the codegen MUST reflect that

This is the correct behaviour as far as I understand the type.

Its not.

[eddeee888]

Hi @raphaelfff , which scenario is wrong?

https://github.com/dotansimha/graphql-code-generator/issues/10389#issuecomment-3356768414

[raphaelfff]

Okay let me ask you a different question: Is 4 month of discussion going round in circle really worth it vs just catering our need motivated by observed behavior from the server of having 1 config option to turn on a codegen behavior to make a defer node optional ?

Your example that you put together dont follow the ones i put together that actually highlight the problem The very first message of this issue highlight the exact issue and observed behavior: a non nullable field resolve2 that is failing will be null if it is defered, the current codegen doesnt mark it as such but it should, thats the issue.

[eddeee888]

I'm more than happy to improve Codegen if I could see the issue.

a non nullable field resolve2 that is failing will be null if it is defered

Are you talking about this line?

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

It is saying resolve2.failed is undefined, not null