-
Notifications
You must be signed in to change notification settings - Fork 4
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
Comments
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. |
@sordina In order to prevent any automatic coercions, we'd have to ban some fairly common TypeScript type patterns. This is because types like Not allowing |
I think it's probably ok to have no distinction of semantics between |
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 That leaves the user only being able to use |
I feel like the optional prop is the clearest expression of omission so I personally don't mind it. Would that be considered unidiomatic? |
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. |
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? |
I think you can coerce intelligently, depending on the type they've defined. |
That could work. |
Related use case from a user on v2 |
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. |
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 |
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.
In GraphQL, there is a semantic difference between providing a null value to a nullable input object property and omitting it:
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:
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:
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. 🙂
The text was updated successfully, but these errors were encountered: