variables should default to undefined

[stephenh]

We've switched to fastify+graphql-jit and had mostly a really easy/seamless transition. Thanks for the project!

One bump we hit was that it seems like when graphql-jit does variable interpolation for clients, it starts out with null default values, i.e. for a client mutation that looks like:

mutation SaveTaskStatus(
  $complete: Boolean	
  $durationInDays: Int	
  $internalUserId: ID	
  $internalNote: String	
  $baselineMode: Boolean!	
) {	
  saveTask(	
    input: {	
      id: $taskId	
      complete: $complete	
      durationInDays: $durationInDays	
      internalUserId: $internalUserId	
      internalNote: $internalNote	
      baselineMode: $baselineMode	
    }	
  ) {

Here is a chunk of our graphql-jit-generated code:

  let MutationsaveTaskResolverValidArgs = true;
  const MutationsaveTaskResolverArgs = {"input":{"id":null,"durationInDays":null,"complete":null,"internalNote":null,"internalUserId":null,"baselineMode":null}};
  if (Object.prototype.hasOwnProperty.call(__context.variables, "taskId")) {
    MutationsaveTaskResolverArgs["input"]["id"] = __context.variables['taskId'];
    }if (Object.prototype.hasOwnProperty.call(__context.variables, "durationInDays")) {
    MutationsaveTaskResolverArgs["input"]["durationInDays"] = __context.variables['durationInDays'];

Where it starts out with the input.id/durationInDays/etc. fields as null and then sets them to the __context.variables value only if the variable is present.

Which means that with graphql-jit, if a client did not set the (say) internalNote variable, it shows up in our resolver as args.input.internalNote === null, which for us means "unset / clear".

For us, this was a behavior change from apollo-server, where if a client did not set the internalNote variable, it would show up in our resolver as args.input.internalNote === undefined, which for us is "do not change".

So this caused a fairly bad bug where our clients were "wiping" these fields like internalNote, internalUserId, etc., b/c they didn't set the variables. (...I believe I also tried to have the clients set the variables as undefined, but that meant apollo-client didn't send them over the wire, just as if we had not set them at all.)

So, again, disclaimer I don't technically if apollo-server or graphql-jit is "more correct", and so I'm kind of lazily filing the issue with the guess that apollo-sever is correct here, but I'm not 100% sure.

[laurisvan]

I have an issue which I believe has this same root cause. We have several schemas that default their input values based on schema. Now that JIT compiler interprets unset values as nulls this mechanism fails.

eg. with our schema (that is trimmed here for the sake of readability)

input CompensationTypeInput { currency: Currency = EUR }

if we leave currency unset in the input payload, GraphQL fails to replace the unset value with EUR (enumeration). I believe this is because of interpreting the value as null. However, if I make the value mandatory (e.g. currency on Currency! = EUR) the results are the same.

[ruiaraujo]

GraphQL has no concept of undefined so this problem so I will try to understand why graphql-js includes undefined.

I think supporting undefined leaks the implementation of the server since a JVM or Go based one would have no difference.

[ruiaraujo]

@stephenh Take a look if https://github.com/zalando-incubator/graphql-jit/pull/145 solves your issue.

[Garon92]

Hello graphql-jit maintainers and contributors,

Firstly, I'd like to express my appreciation for the work on this project. It has been invaluable for many, including myself.

I've been closely following the discussion in this thread and have encountered a similar issue in my projects. The differentiation between null and undefined has certain implications in our application logic and error handling. While I understand and respect the perspective that GraphQL itself doesn't inherently support the concept of undefined, we've noticed that some other GraphQL servers (e.g., Apollo) do make this distinction, leading to different behaviors.

Given this context, I've been considering working on a patch to introduce an option in graphql-jit that allows developers to choose the default behavior for uninitialized variables (null vs. undefined). Before diving deep into this, I wanted to get a sense of the maintainers' and community's thoughts on this. Would such a patch be considered for merging if it aligns with the project's standards and guidelines? My goal is to ensure that this potential enhancement would be in line with the project's philosophy and long-term vision.

Looking forward to your feedback, and once again, thank you for your dedication to this project.

Best regards, David Muzik

[laurisvan]

If I interpreted the GraphQL Spec correctly, undefined is at least allowed to be treated as undefined (no entry is added to the coerced unordered map):

If no value is provided for a defined input object field and that field definition provides a default value, the default value should be used. If no default value is provided and the input object field’s type is non-null, an error should be raised. Otherwise, if the field is not required, then no entry is added to the coerced unordered map.

https://github.com/graphql/graphql-spec/issues/1044#issuecomment-1714787343

Though, the reference implementation in graphql-js seems to have a similar issue? https://github.com/graphql/graphql-js/issues/2533