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

Omitted vs null arguments and object properties #72

Open
daniel-chambers opened this issue Dec 4, 2023 · 12 comments
Open

Omitted vs null arguments and object properties #72

daniel-chambers opened this issue Dec 4, 2023 · 12 comments

Comments

@daniel-chambers
Copy link
Contributor

daniel-chambers commented Dec 4, 2023

In the NDC spec, one can define arguments that have a nullable type, and also define object properties that have a nullable type.

When these are represented as input types in GraphQL (as field arguments or input object properties), it is possible for the GraphQL API user to omit these values altogether, or specify them explicitly with a null value.

explicitNullArg: my_field_with_arg(other: 123, nullableArg: null)
omittedArg: my_field_with_arg(other: 123)

explicitNullProp: my_field_with_object_arg(object: { other: 123, nullableProp: null })
omittedProp: my_field_with_object_arg(object: { other: 123 })

In GraphQL, there is a semantic difference between providing a null value to a nullable input object property and omitting it:

there is a semantic difference between the explicitly provided value null versus having not provided a value
(source)

Field arguments also seem to capture the difference between an omitted field argument and one provided an explicit null value. In the Coercing Field Arguments spec algorithm, coercedValues will not have an entry for an omitted nullable argument, and will have an entry for an explicit null value argument.

The GraphQL spec does not allow for a non-nullable typed arguments/properties to be omitted.

The NDC spec does not define an expected behaviour for omitted vs null values. Currently the v3-engine carries the GraphQL behaviour through to NDC (ie omitting arguments/properties when they are omitted in GraphQL).

For many connectors the distinction is unimportant and omitted can simply be coerced to null by the connector. However, for connectors that do themselves have a concept of omitted, the distinction becomes important. The case in point is the Deno TypeScript connector, that exposes TypeScript functions as NDC functions/procedures.

In TypeScript, function arguments and object properties can have types that explicitly capture the difference between null and undefined. For example:

function example(
  nullableArg: string | null, 
  undefinedArg: string | undefined, 
  nullOrUndefinedArg: string | null | undefined,
  optionalArg?: string, // The type is actually 'string | undefined'
) {
}

type Example = {
  nullableProp: string | null, 
  undefinedProp: string | undefined, 
  nullOrUndefinedProp: string | null | undefined,
  optionalProp?: string, // The type is actually 'string | undefined'
}

If we choose to say that NDC does not have the concept of omitted arguments/properties and therefore all omissions are coerced into nulls in the v3-engine, then binding to these arguments would result in null being provided in all cases, except in the cases where null is not a valid value, then null would need to be coerced to undefined.

If we choose to say that NDC does have the concept of omitted arguments/properties, then in the cases where an omission is present, undefined would be provided (except in cases where undefined is not valid, in which case null would be provided). In cases where null is explicitly provided, we would provide null in all cases, except where it is not a valid value in which case it would need to be coerced to undefined.

We could make the omission a first-class controllable concept in NDC, by letting object properties and arguments define whether they are allowed to be omitted. However, to match that up with GraphQL, we'd need to say that only nullable-typed props/args could be omittable. Or else we'd need to insist the v3-engine generate nullable GraphQL types for omittable non-nullable NDC prop/args, which seems overly complex.

I'd argue making omission a first-class controllable concept seems mostly unnecessary. So long as we make it clear that nullable arguments/properties can be omitted and will be if they are omitted in GraphQL, we can leave how that is interpreted up to the agent. However, the advantage of making it explicit means that we could shift the value coercion logic into the v3-engine instead of making the connectors deal with it (ie. if a connector doesn't want to deal with omitted values, it could say not-omittable to everything and the engine would coerce these values to null).

Something else to consider is that omittability is only relevant for input values. For output values in GraphQL, a requested field is never omitted. Nulls are always returned explicitly:

If result is null (or another internal value similar to null such as undefined), return null
(source)

This lack of symmetry in omittability between input and output values is somewhat annoying, but is the reality of GraphQL, and therefore probably should be enforced in NDC too.

I'd appreciate everyone's thoughts on this topic. 🙂

@sordina
Copy link
Contributor

sordina commented Dec 4, 2023

Great analysis.

I think the main concern I have with coercing upstream is that a connector author (or TS function author) may be expecting certain semantic usage of their requests, such as: only applying defaults if a param/field is omitted, and not if it is null. We should endeavour to make authorship as easy and intuitive as possible, so if we are not going to support a possible distinction in semantics in the connector, then we should not allow it to be expressed in the schema either.

I'm generally keen to give authors more power in their connectors, and expand that power over time even if it is initially limited.

@daniel-chambers
Copy link
Contributor Author

@sordina In order to prevent any automatic coercions, we'd have to ban some fairly common TypeScript type patterns. This is because types like string | undefined are impossible to define in GraphQL without coercions somewhere. GraphQL will always allow you to pass null, because the GraphQL type for that example would be String (ie nullable).

Not allowing string | undefined-style types means that optional arguments and optional properties would need to be banned. That seems like a worse and more unexpected outcome than just coercing the nulls into undefineds in that case.

@sordina
Copy link
Contributor

sordina commented Dec 4, 2023

I think it's probably ok to have no distinction of semantics between null | undefined expected by authors since as you say, graphql can't express that difference. But omitted fields being coerced to null would be unexpected IMO.

@daniel-chambers
Copy link
Contributor Author

But it is impossible to prevent nullable args/props from being omitted in GraphQL. So either you allow coercion of omissions to null, or you need to start banning types of string | null (because you can't coerce omitted to null), which leaves you with types of string | undefined where you'd still need to coerce an explicit null in GraphQL to undefined, so you'd need to ban that too.

That leaves the user only being able to use string | null | undefined (or optional arg/prop of type string | null (effectively string | null | undefined)), which is pretty horrible.

@sordina
Copy link
Contributor

sordina commented Dec 4, 2023

I feel like the optional prop is the clearest expression of omission so I personally don't mind it. Would that be considered unidiomatic?

@daniel-chambers
Copy link
Contributor Author

Defining as optional prop/arg as also nullable is pretty weird, yes, unless your object/function has a specific reason for caring about null separately to omission (undefined). Most don't, in my experience.

@sordina
Copy link
Contributor

sordina commented Dec 4, 2023

It all comes down to defaulting for me. If you want to be able to have connectors be able to distinguish null and default, then I think you can't coerce omission. But if we decide that this just isn't useful then we can forget about it. What do you think?

@daniel-chambers
Copy link
Contributor Author

I think you can coerce intelligently, depending on the type they've defined.
If they define string | null, then you coerce omissions to null, and explicit nulls are passed through.
If they define string | undefined, omission is passed as undefined, and explicit nulls as coerced to undefined.
If they define string | null | undefined, you don't perform any coercions, omission is passed as undefined and explicit null is passed through.

@sordina
Copy link
Contributor

sordina commented Dec 4, 2023

That could work.

@nullxone
Copy link

nullxone commented Dec 4, 2023

Related use case from a user on v2

hasura/graphql-engine#9627

@paf31
Copy link
Collaborator

paf31 commented Dec 4, 2023

Assuming we keep NDC as-is, and do not distinguish null from undefined at the spec level (which would be my strong preference), then presumably the TS connector can warn the user about these non-recommended types (e.g. string | undefined) when building the schema. Meanwhile, I think it's technically incorrect for the engine to omit any arguments in the request, even if they are omitted in the GraphQL request, and we should fix that, and not exploit the undefined behavior in the TS connector.

@daniel-chambers
Copy link
Contributor Author

I don't think I agree. I think users often do care about the difference between omitted and null (see @nullxone's post above for an immediate example at hand), and given GraphQL does have the concept, we should retain it.

And as for the TypeScript connector, we can expect people to use libraries, existing code and existing programming patterns (such as optional function parameters, or optional object properties) that do use string | undefined-style types, and I think telling people how they should write their code (ie don't use these sorts of types) will just add unnecessary friction to the developer experience of using the connector. Given that it's very possible for us to support this scenario, I don't see why we shouldn't.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants