Omittable input fields

[Desuuuu]

This PR adds the Omittable type to the graphql package. Its purpose is distinguishing between omitted and null input fields and it has the following API:

type Omittable[T any] struct {
	// ...
}

func (o Omittable[T]) Value() T {
	if !o.set {
		var zero T
		return zero
	}
	return o.value
}

func (o Omittable[T]) ValueOK() (T, bool) {
	if !o.set {
		var zero T
		return zero, false
	}
	return o.value, true
}

func (o Omittable[T]) IsSet() bool {
	return o.set
}

The type is meant to be bound with nullable input fields.

Model generation is able to generate omittable fields by:

• Using the @goField(omittable: true) directive on a nullable input field.
• Setting the nullable_input_omittable configuration option to true. This causes nullable input field to be omittable by default.

The goField directive always has priority over the nullable_input_omittable option meaning you can have nullable input fields default to being omittable, and override it with @goField(omittable: false).

Relevant issue: #1416

I have:

• Added tests covering the bug / feature (see testing)
• Updated any relevant documentation (see docs)

[coveralls]

Coverage: 75.442% (-3.3%) from 78.753% when pulling cbf3ac2 on Desuuuu:optional-input into 2ad08ff on 99designs:master.

[StevenACoffman]

Cool! I would love to get some other people to look it over and take it for a spin.

[Desuuuu]

For anyone interested, here's a barebones repository I put together to play around with this: https://github.com/Desuuuu/gqlgen-optional-test

[carldunham]

This is great! Would love to see some tests using input fields in addition to String. There may be some more complexity there around using omitempty in the json tags or something. Didn't debug, but was playing around and saw some issues: https://github.com/carldunham/gqlgen-optional-test/tree/cd/gqlgen-2585/testing

[Desuuuu]

You're calling the Optional's Stringer, which currently returns an empty string if the value is undefined, right there: https://github.com/carldunham/gqlgen-optional-test/blob/bf0d7f4e468044de25fc8f21002ac707ebafbf3d/graph/model/extensions.go#L6

I included it mostly for convenience/debugging but maybe printing something like <undefined> in this case would me more useful.

JSON marshalling (with the standard library at least) wouldn't be great since omitempty doesn't omit zero-value structs. If this is really a concern, it could work better if we made Optional's API something like this:

type Optional[T any] struct {
	value T
}

func OptionalOf[T any](value T) *Optional[T] {
	return &Optional[T]{
		value: value,
	}
}

func (o *Optional[T]) IsDefined() bool {
	return o != nil
}

func (o *Optional[T]) IsUndefined() bool {
	return o == nil
}

func (o *Optional[T]) Value() (T, bool) {
	if o == nil {
		var zero T
		return zero, false
	}
	return o.value, true
}

func (o *Optional[T]) ValueOr(def T) T {
	if o == nil {
		return def
	}
	return o.value
}

func (o *Optional[T]) String() string {
	if o == nil {
		return "<undefined>"
	}
	return fmt.Sprintf("%v", o.value)
}

func (o Optional[T]) MarshalJSON() ([]byte, error) {
	return json.Marshal(o.value)
}

func (o *Optional[T]) UnmarshalJSON(b []byte) error {
	return json.Unmarshal(b, &o.value)
}

In this case, every field would have to be an Optional pointer since a nil *Optional would be used to represent an undefined value.

[carldunham]

Ah, got it. Would still like to see those tests :)

[Desuuuu]

I added tests with different types and changed the Stringer's output for undefined values.

Any thoughts regarding Optional vs *Optional?

[StevenACoffman]

I generally prefer *Optional over Optional because I prefer pointers for every struct type, but would be open to hearing arguments the other way.

[carldunham]

For small structs like this, meant to be embedded, it can be nice to avoid the indirection. But I would prefer nil meaning “not set” over having a bool field for that. 

[StevenACoffman]

There are two reasons to use a pointer receiver:

1. The first is so that the method can modify the value that its receiver points to. (Optional will be immutable after it is created, correct?)
2. The second is to avoid copying the value on each method call. This can be more efficient if the receiver is a large struct, for example. (Optional is pretty small if Value is a primitive, but big if Value is a big struct)

In general, all methods on a given type should have either value or pointer receivers, but not a mixture of both.

Am I correct in understanding:

1. People who prefer pointers to structs will probably not opt to use this feature as nil already (mostly) means "not set".
2. People who like to avoid pointers will use this feature and embed this Optional struct but will prefer Optional to be a value rather than a pointer.

[Desuuuu]

But I would prefer nil meaning “not set” over having a bool field for that.

It's not really the value that's nil in the *Optional case though, it's the wrapper itself since you can have a value that's nil and defined.

I don't think it there's much of a difference for the copy, because T will almost always be a pointer type. It could be a non-pointer type in non-generated models, but if you use that for some reason, I think you're likely aware of the implication.

As for mutability, I don't think it really adds anything for the user, so I'd prefer having methods to retrieve the value (something like Value() T, ValueOK() (T, bool), IsDefined() bool for example).

The downsides of *Optional to me are:

• Its API is pretty unclear: an undefined value is represented as the wrapper being nil. You can have methods to make that clearer, but the user can still compare the wrapper to nil.
• If you use it directly (without a pointer), it stops making sense.

While the upside is that marshaling the input to JSON with the standard library is more correct (undefined values are not marshaled instead of being null). I have a hard time seeing the use-case for marshaling input structs to JSON though.

For these reasons Optional (with unexported fields) feels like a better choice to me.

[oiime]

@Desuuuu It seems like a perfectly reasonable approach to me, it requires dealing with different types down the line so more work adapting existing code, but overall cleaner than the other approaches before we had generics, if you want minor feedback:

• MarshalJSON might be a bit pointless here as the only use case for this is with input values, maybe it'll be best to panic there as a way to let the developer know this is not the intended use-case for the type? Misunderstood the use
• Returning "<undefined>" in the Stringer implementation is a bit of an odd arbitrary behavior imo, Can't think of a nicer way to distinguish between undefined and nil there though.
• Optional is a bit of a misleading name, String is already optional, and that is the word used within the graphql terminology to refer to an optional value, maybe Delineated or Defined?
• does @goField(optional: true) serve a purpose in the example schema if nullable_input_optional=true?

Edit: Also, definitely support Optional over *Optional, it conveys the use much better

[Desuuuu]

MarshalJSON might be a bit pointless here as the only use case for this is with input values, maybe it'll be best to panic there as a way to let the developer know this is not the intended use-case for the type?

I thought it might be useful for some people, but in the context of this PR it does not serve any purpose indeed. Same goes for UnmarshalJSON as well!

Returning "" in the Stringer implementation is a bit of an odd arbitrary behavior imo, Can't think of a nicer way to distinguish between undefined and nil there though.

It's kinda similar to the standard library printing nil pointers as <nil> but I'm thinking it might be better to not include a Stringer.

Optional is a bit of a misleading name, String is already optional, and that is the word used within the graphql terminology to refer to an optional value, maybe Delineated or Defined?

The name is not in any way linked to GraphQL, it's simply a Go type that fields can be bound to. It will not appear anywhere in the schema. Maybe we can find a better name for a value that can be undefined, null or set though?

does @gofield(optional: true) serve a purpose in the example schema if nullable_input_optional=true?

No, it's redundant. But you can do @goField(optional: false) to override nullable_input_optional=true!

I'm thinking the type could also be:

type Optional[T any] struct {
	value   *T
	defined bool
}

Which would mean the value can always be nil.

[oiime]

@Desuuuu

The name is not in any way linked to GraphQL, it's simply a Go type that fields can be bound to. It will not appear anywhere in the schema. Maybe we can find a better name for a value that can be undefined, null or set though?

Yeah, that's what I mean, it can get conflated with something else, that is, people don't read docs in depth and might confuse this as a way to have "optional" input fields, maybe Undefinable or Omittable might work better, just my initial gut feeling looking at the name.

I'm thinking the type could also be:

type Optional[T any] struct {
	Value   *T
	Defined bool
}

might get a bit messy with mixed pointer and non-pointer T no? imo your current type implementation is less opinionated about the underlying type it handles which is good

[Desuuuu]

It's definitely more opinionated, since it forces you to handle nil values.

I'm not sure but I think modelgen always generates pointer types for nullable input fields, so it would not affect that but it would indeed prevent you from treating null values as zero values with custom input models.

[oiime]

@Desuuuu It does always generate pointer types, but that assumes people only use modelgen models, in a lot of cases (mine included) this is not how it is used, types are created somewhere else and often represent null following sql.NullString style rather than *string.

[Desuuuu]

I'm not too familiar with SQL in Go, but if the intent is to use the type directly, is there a way to make the driver work correctly with Optional[sql.NullString] in any case? By that I mean making it skip fields that are undefined. I might be wrong but it doesn't seem possible, at least with sql.Valuer.

If that was possible and a use-case we wanted to support, there would be pretty much no reason to use sql.NullString instead of string since Optional could be made to skip undefined fields and to serialize nil as NULL.

Overall I think trying to make Optional work correctly with third-party code like this might be outside the scope of this PR (same goes for JSON).

We could either add that later or even allow the user to use his own generic types somehow.

Edit: Besides that, you can already differentiate between null and undefined for custom types that have an unmarshaler!

[oiime]

@Desuuuu

I'm not too familiar with SQL in Go, but if the intent is to use the type directly, is there a way to make the driver work correctly with Optional[sql.NullString] in any case? By that I mean making it skip fields that are undefined. I might be wrong but it doesn't seem possible, at least with sql.Valuer.

This is not really about SQL, NullType is a common way to represent null instead of *Type, I don't use it for SQL in my case but rather interacting with another legacy system that uses this approach. The purpose is not to pass onward Optional[NullString] but rather to be able to extract all types the same way, meaning both Optional[*string] and Optional[NullString] can be converted respectively into *string and NullString without caring for how the underlying mechanism works

If that was possible and a use-case we wanted to support, there would be pretty much no reason to use sql.NullString instead of string since Optional could be made to skip undefined fields and to serialize nil as NULL.

Imo there are just too many assumptions there about the behavior and limitations down the line, I'd much rather fit gqlgen to the rest of the system, not vice versa. Up to you if you consider it a use-case you want to support.

Besides that, you can already differentiate between null and undefined for custom types that have an unmarshaler!

How can that be done without using this Optional implementation? an umarshaler would simply not be called if it is not defined, meaning we'd need to create a custom optional wrapper for every type separately eg:

type OptionalNullString struct {
	Value 	NullString
	Defined bool
}

func UnmarshalOptionalNullString(v interface{}) (OptionalNullString, error) {
	if v == nil {
		return OptionalNullString{Defined: true}
	}
	return OptionalNullString{Defined: true, NullString{Valid: true, String: "parse v"}}
}

[Desuuuu]

Sorry, I misunderstood your comment. I thought it was about making the type interact with third-party packages.

The purpose is not to pass onward Optional[NullString] but rather to be able to extract all types the same way, meaning both Optional[*string] and Optional[NullString] can be converted respectively into *string and NullString without caring for how the underlying mechanism works

That's a really good point against forcing pointers which I wasn't thinking about!

I also agree with you that Optional might be a confusing name. I like Omittable better.

[carldunham]

But I would prefer nil meaning “not set” over having a bool field for that.

It's not really the value that's nil in the *Optional case though, it's the wrapper itself since you can have a value that's nil and defined.

Right, so the nil *Optional can mean not provided, and a nil *T would mean a null value.

I'm not arguing in favor, necessarily, but it seems to clearly reflect the semantics of a "nullable" input field (or argument?) that can be provided or not, and where that is meaningful.

FWIW, I prefer avoiding terms like "optional" and "undefined", as they don't clearly reflect GraphQL semantics. Those are terms most often used to map gql concepts into language-specific ones, as I've seen them.

Which is why I'm a bit hesitant about this discussion at all, but I'm aware that in the real world there are implementations that distinguish between "set" and "null" for things like partial object updates. I've provided PRs for that use case myself. 🙂

Either option chosen here would be cleaner than those.

[Ned-el-ch]

this solves something i've been scratching my head at so would love to see it merged 😅

[voslartomas]

I guess it is not possible to just leverage optional vs mandatory from graphql, with String vs String! right? This is great solution, but little bit verbose.

[Desuuuu]

Right, so the nil *Optional can mean not provided, and a nil *T would mean a null value.
I'm not arguing in favor, necessarily, but it seems to clearly reflect the semantics of a "nullable" input field (or argument?) that can be provided or not, and where that is meaningful.

I think exposing that kind of API to the user would be confusing/prone to mistakes.

FWIW, I prefer avoiding terms like "optional" and "undefined", as they don't clearly reflect GraphQL semantics. Those are terms most often used to map gql concepts into language-specific ones, as I've seen them.

Would you prefer Omittable? I think it's a lot clearer about the intent of this PR.

IMO we shouldn't be trying to map to GraphQL's nullability concept since this is specific to input fields and nullability itself is already handled farily well (and not necessarily with pointers as discussed earlier).

I guess it is not possible to just leverage optional vs mandatory from graphql, with String vs String! right? This is great solution, but little bit verbose.

For generated models, there's an option to wrap all nullable input fields by default. I think that's the best we can do without breaking compatibility for existing users.

[oiime]

@voslartomas

I guess it is not possible to just leverage optional vs mandatory from graphql, with String vs String! right? This is great solution, but little bit verbose.

Unless you intend to reinvent all the types you can't really do that, that would just get you in the same end result of having an Optiona[T] like implementation but with more extra steps. The issue is representing 3 distinct states, eg:

{value: null}
{value: "foobar"}
{}

This is simply not possible to express with string or *string so you need to add another type that can express those 3 states or convey that information through another source which is what all previous solutions did

[carldunham]

Where would defaults fall here? I assume the executor is dropping them in if values aren't provided, but also smart enough to replace with null if it's explicitly given.

[Desuuuu]

I updated the name to Omittable as suggested earlier, I think it conveys the purpose of this PR better.

I also made its fields unexported, added a few methods to read its content and removed the extra methods which weren't really serving any purpose.

Where would defaults fall here? I assume the executor is dropping them in if values aren't provided, but also smart enough to replace with null if it's explicitly given.

I don't understand which defaults you're referring to, could you clarify?

Just remembered that input fields can have default values. The field's value will always be defined in this case and the value will either be the default value or the actual value. I'm not sure what would be expected in this case.

[carldunham]

Just remembered that input fields can have default values. The field's value will always be defined in this case and the value will either be the default value or the actual value. I'm not sure what would be expected in this case.

Makes sense. I've run into issues in the past over nullable arguments or input fields with defaults with a null explicitly passed, and the default being used instead.

[oiime]

@carldunham I would think that is the intended behavior there, no? I guess supporting optional database/sql style scanner interface makes sense if somebody wants some unique behavior for defaults, there is already a standard library example on how to tackle a similar scenario

eg:

func (v *MyNullableDefaultType) Scan(value any) error {
   if any == nil {
      // do some v stuff representing a different default
   }
}

[carldunham]

Well this is by the GraphQL spec, language independent. CarlOn Apr 1, 2023, at 09:10, Itamar Maltz ***@***.***> wrote: @carldunham I would think that is the intended behavior there, no? I guess supporting optional database/sql style scanner interface makes sense if somebody wants some unique behavior for defaults, there is already a standard library example on how to tackle a similar scenario eg: func (v *MyNullableDefaultType) Scan(value any) error { if any == nil { // do some v stuff representing a different default } } —Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: ***@***.***>

[Desuuuu]

I updated the PR to reflect the changes that have been made, squashed and rebased.