Optional-types API

[divan]

Hi,

I just switched to 1-beta.3, and all Go types that I used for optional fields started to return error in queries about implementing OptionalMarshaller, so had to look into those and have couple of thoughts.

It would be handy for Optional- types to have an easy access to the "zero value". I.e. empty string for OptionalStr. It's an idiomatic Go pattern to have useful zero value, and some other optional/null implementations leverage this and (at least to me) it worked pretty well in practice.

Most of them have form of:

type NullString struct {
    Value string
    Valid bool
}

which is simple and concise, and allows using zero value for cases where default value is a good replacement for missing value (like empty string).

2. From the developer point of view, I regularly want to be able to be able to change field between 'required' and 'optional'. For example, when adding new field I might postpone deciding if it's optional or not – what I care at the beginning is an actual type.

But with current API this change is not easy:

Required definition:

Field  string

Optional definition:

Field  edgedb.OptionalStr

Required read (to be used in other struct, with default zero value):

return Response{
 field: v.Field,
}

Optional read (to be used in other struct, with default zero value):

ret, _ := v.Field.Get()
return Response{
 field: ret,
}

// or oneliner if there are multiple optional fields:

return Response{
 field: func() string { ret, _ := v.Field.Get(); return ret }(),
}

Required set:

...
v := MyType{
    Field : str,
}

Optional set:

    var field edgedb.OptionalStr
    field.Set(str)
    v := MyType{
      Field : field,
    }

// or oneliner:

    var field edgedb.OptionalStr
    field.Set(str)
    v := MyType{
      Field : func() edgedb.OptionalStr { var f edgedb.OptionalStr; f.Set(str); return f; }(),
    }

And while I like the explicitness and verbosity of that approach, I end up writing own wrappers for all those optional types.

With that "standard" approach, the following code would be:

...
  v := MyType{
    Field : str.Value,
  }

and

    v := MyType{
      Field: NewOptionalStr(str),
    }

accordingly.

What do you think?
I know it'll be hard to change API, but maybe at least add getters/setters for default value for basic types? (i.e. String()/NewOptionalStr())

Or I'm missing something behind the current Optional- types design?

[divan]

I see such pattern already in options.go - NewOptionalBool().

But what would be the downsides of replacing:

// OptionalInt64 is an optional int64. Optional types must be used for out
// parameters when a shape field is not required.
type OptionalInt64 struct {
    val   int64
    isSet bool
}

// Get returns the value and a boolean indicating if the value is present.
func (o *OptionalInt64) Get() (int64, bool) { return o.val, o.isSet }

// Set sets the value.
func (o *OptionalInt64) Set(val int64) *OptionalInt64 {
    o.val = val
    o.isSet = true
    return o
}

// Unset marks the value as missing.
func (o *OptionalInt64) Unset() *OptionalInt64 {
    o.val = 0
    o.isSet = false
    return o
}

with

// OptionalInt64 is an optional int64. Optional types must be used for out
// parameters when a shape field is not required.
type OptionalInt64 struct {
    Val   int64
    IsSet bool
}

I initially thought that API is heavily based around OptionalMarshaller interface, but it looks like most Optional- types don't implement it. (And I assume they eventually must)
So maybe even change to this:

// OptionalInt64 is an optional int64. Optional types must be used for out
// parameters when a shape field is not required.
type OptionalInt64 struct {
    Optional
    Val   int64
}

In this way, all Optional- types would have IsMissing() bool method and satisfy OptionalMarshaller interface and will have greater flexibility in accessing/setting value and it's missing status.

[fmoor]

Thanks for the feed back @divan!

The primary concern with this API is correctness. If the fields in an Optional- type are public then it is much easier to forget to check IsSet. It also becomes possible to change the Val field without changing IsSet which could lead to inconsistent state. I agree that this choice does lead to a more verbose API, but the hope is that the trade off is worth the effort.

There should be a convenience function for all scalar types similar to NewOptionalBool(). It looks like I forgot to add all of them 😕. Good catch! I'll fix that.

The OptionalUnmarshaler interface is intended for user defined struct types that correspond to optional links etc. in queries. I'm not opposed to adding a Missing() bool method to all Optional- types. It would be helpful to understand when it would be better than _, ok := optionalStr.Get() though.

[1st1]

return error in queries about implementing OptionalMarshaller

@fmoor I think we need to improve our error messages to instead suggest the user to use the correct OptionalScalar type or use the Optional struct if the error is about a link. The OptionalMarshaller, IMO, should only be mentioned in the docs.

[divan]

@fmoor agree on correctness priority.

It also becomes possible to change the Val field without changing IsSet which could lead to inconsistent state.

From my experience with API described above, I virtually never had to set IsSet/Valid manually. Practical usage was reduced to nulls.NewString(s)/str.Valid/str.Value.
But I agree that such approach incentivizes using "zero" values instead of "no value" and/or don't care much about it (I routinely had "0001-01-01" in time/date fields as a result - which is kinda ok until it's not).

Yet, I'm trying to think about better tradeoff between being explicit and concise, and keep returning to the idiomatic Go patterns, including ones found in stdlib.

One example is os.Getenv and os.LookupEnv which resembles situation with Optional- types. First method returns env var or empty string, second – provides extra bool for distinguishing missing value from empty value. Granted, second method was added later, but still – I use mostly os.Getenv, because it's not very often that I expect the different behavior in "empty value" vs "missing value". Less correct, but works 99.999% and always has a 'better' version.

Or using pointers for optional types, like:

type Entity struct {
    First string
    Second *string // this is optional
}

thus having nil as a clear indication of missing values. It's super easy to use, but I don't really like this approach as pointer vs value choice also carries other implications for type, including GC behavior, so that may confuse more than help.

Another thoughts are revolving around using tags. I.e.

type Entity struct {
    First string `edgedb:"first"`
    Second string `edgedb:"second,some_smart_tag_name_to_show_that_default_value_means_missing"`
}

Can't come up with a good tag name at the moment, sorry :) But in line with time.Time.IsZero() from stdlib, we can add similar methods for zero value checks for other complex optional types (like edgedb.LocalDate) and use with this approach.
It's looks a bit hackish and I'm not 100% happy with it (also haven't seen db/orms libraries that use tags for that), but it's quite idiomatic to Go and allows explicitly mark if you care about explicity check for missing value or you're okay with 'zero values`.

So, no clear suggestions for now, just throwing some random thoughts, maybe they will be useful.

[1st1]

@fmoor Frederick, can we close this one now?

[1st1]

So, no clear suggestions for now, just throwing some random thoughts, maybe they will be useful.

I've re-read the discussion and I agree, the current API is a bit too verbose. That said, I want us to run with it for a month or two before iterating further. I'm gonna close this issue, but @divan please feel free to open new one (or reopen this one) if you have any new ideas or feature proposals!

[divan]

Just leaving here hilarious discussion about similar problem in protobuf v3. protocolbuffers/protobuf#1606

And actual docs page on field presense: https://github.com/protocolbuffers/protobuf/blob/master/docs/field_presence.md

As for today, official Go proto generator plugins generates struct fields as scalar types (int32, string, etc), and uses default "zero value" in case of missing field. But, if optional flag is specified (it's still experimental), then struct fields are generated as pointer (*int32, *string, etc).

PS. I'm using EdgeDB with protobuf3, so have to sync the structs definitions and will be able to compare on-hands experience with both approaches :)