Wrapping OPTIONAL fields in proto3.

[chhsia0]

In proto3 we no longer have a has_field() function if field is of a primitive type, so we may want to wrap them. Here is a list of OPTIONAL fields that have this problem:

• MountVolume::fs_type: If an empty string is allowed, then we should wrap it. Otherwise we can explicitly specify that it must not be empty when set.
• ListVolumeRequest::starting_token and ListVolumeResponse::Result::next_token: Similarly, is an empty string a valid token?
• ListVolumeRequest::max_entries: Can we issue a request to return 0 entries?

Here is a list of OPTIONAL fields that may not need to be wrapped:

• GetPluginInfoResponse::manifest: map<string, string>
• CreateVolumeRequest::parameters: map<string, string>
• VolumeMetadata::values: map<string, string>
• ValidateVolumeCapabilitiesResponse::Result::message: string
• All OPTIONAL repeated fields

NOTE, the OPTIONAL field VolumeInfo::capacity_bytes requires no wrapping because we specify in the spec that it must be non-zero when set.

[jdef]

• It's probably safe to interpret MountVolume::fs_type == "" as an unset field and NOT a valid file system type.
• starting_token, next_token: empty string, to me, indicates an unset value and not a valid token. If this makes sense to others, we can codify it in the spec.
• max_entries of 0 implies "unlimited" to me

[akutz]

Hi @jdef,

I was about to file a similar issue. To me an empty value is not always safe to assume "unset". That's why for JSON data structures I often use *int32 to indicate the value can be nil and not just 0. This is a specification, and as such nothing should be left to interpretation IMHO.

That said, for the max_entries and token values I agree that empty can mean unset. However, I was about to file an issue stating that this should be called out explicitly, because at the moment it is not:

message ListVolumesRequest {
  // The API version assumed by the CO. This field is REQUIRED.
  Version version = 1;

  // If specified, the Plugin MUST NOT return more entries than this
  // number in the response. If the actual number of entries is more
  // than this number, the Plugin MUST set `next_token` in the response
  // which can be used to get the next page of entries in the subsequent
  // `ListVolumes` call. This field is OPTIONAL. If not specified, it
  // means there is no restriction on the number of entries that can be
  // returned.
  uint32 max_entries = 2;

  // A token to specify where to start paginating. Set this field to 
  // `next_token` returned by a previous `ListVolumes` call to get the
  // next page of entries.
  string starting_token = 3; 
}

My suggested changes are below:

message ListVolumesRequest {
  // The API version assumed by the CO. This field is REQUIRED.
  Version version = 1;

  // If specified, the Plugin MUST NOT return more entries than this
  // number in the response. If the actual number of entries is more
  // than this number, the Plugin MUST set `next_token` in the response
  // which can be used to get the next page of entries in the subsequent
  // `ListVolumes` call. This field is OPTIONAL. If not specified, it
  // means there is no restriction on the number of entries that can be
  // returned. A zero value means no restriction has been set.
  uint32 max_entries = 2;

  // A token to specify where to start paginating. Set this field to 
  // `next_token` returned by a previous `ListVolumes` call to get the
  // next page of entries. A zero-length string means no token has
  // been set.
  string starting_token = 3; 
}

The issue of a type's empty value being interpreted as unset is obviously much more dangerous when it comes to updating an object based on those values -- especially non-nillable values that always have some meaningful value for their empty value -- zero for numbers, empty string for strings, etc. Having reviewed the spec a few times now I don't believe the lack of true, unset values presents too much of an issue, but even so I believe that those cases should be called out explicitly.

[jdef]

there's actually a well defined "wrapper type" library that we could leverage: https://github.com/google/protobuf/blob/master/src/google/protobuf/wrappers.proto