optional fields return zero value (empty string) instead of null

[tamird]

Given the definition:

syntax = "proto2";
package cockroach.build;
option go_package = "build";

import "gogoproto/gogo.proto";

// Info describes build information for this CockroachDB binary.
message Info {
  optional string go_version = 1 [(gogoproto.nullable) = false];
  optional string tag = 2 [(gogoproto.nullable) = false];
  optional string time = 3 [(gogoproto.nullable) = false];
  optional string revision = 4 [(gogoproto.nullable) = false];
  optional string cgo_compiler = 5 [(gogoproto.nullable) = false];
  optional string platform = 6 [(gogoproto.nullable) = false];
  optional string distribution = 7 [(gogoproto.nullable) = false];
  optional string type = 8 [(gogoproto.nullable) = false];

  // dependencies exists to allow tests that run against old clusters
  // to unmarshal JSON containing this field. The tag is unimportant,
  // but the field name must remain unchanged.
  //
  // alternatively, we could set jsonpb.Unmarshaler.AllowUnknownFields
  // to true in httputil.doJSONRequest, but that comes at the expense
  // of run-time type checking, which is nice to have.
  optional string dependencies = 10000;
}

The generated message behaves incorrectly:

$ node
> var protos = require('./src/js/protos');
undefined
> r = new protos.cockroach.build.Info()
Info {}
> r.go_version
''

This should return null, not empty string. Note that in protobuf 3 it should return empty string, since primitives are non-nullable in protobuf 3.

The typescript definitions are also incorrect with respect to the nullability of these attributes.

[dcodeIO]

If not set, it returns the default value of the field and the default value of an unset string field is an empty string as of the official spec. More precisely, the value you see there comes from protos.cockroach.build.Info.prototype.go_version

[tamird]

The spec says that you should be able to distinguish between an empty value and an unset value.

…

On Mar 25, 2017 15:17, "Daniel Wirtz" ***@***.***> wrote: If not set, it returns the default value of the field and the default value of an unset string field is an empty string as of the official spec. More precisely, the value you see there comes from protos.cockroach.build.Info.prototype.go_version — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#728 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABdsPESOkTTyzVuWyBGV2BRrmJLCgmI_ks5rpWhHgaJpZM4MpI3d> .

[dcodeIO]

You can by using r != null && r.hasOwnProperty("go_version") for normal fields. For repeated fields the check (now) is r.someField && r.someField.length and for maps it is r.someField && Object.keys(r.someField).length.

[tamird]

Yikes, that's an unfortunate API. Still, the type definitions should reflect that the value is non-null.

…

On Mar 25, 2017 15:42, "Daniel Wirtz" ***@***.***> wrote: You can by using r.hasOwnProperty("go_version") — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#728 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABdsPPaI9Xj4ebSmQN0u-Nzr1Ep0Wqz1ks5rpW4QgaJpZM4MpI3d> .

[dcodeIO]

The type definition just says it is string. You can still manually set fields to null or undefined (which is checked by r != null, not that this only evaluates to true for non-null, non-undefined). The behavior above also is different for $Properties, which don't have values on their prototype (and thus the values are undefined), while runtime messages have.

I know that the API isn't perfect, but I don't see another way in JS (without introducing something not really performant like hidden properties through getters/setters for everything and such).

Edit: Sorry, accidentally closed the issue.

[tamird]

Why can't you make unset properties return null?

…

On Mar 25, 2017 15:47, "Daniel Wirtz" ***@***.***> wrote: Reopened #728 <#728>. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#728 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABdsPIGfnaiDCVkJ0fXPCE6TkLACcYr-ks5rpW82gaJpZM4MpI3d> .

[dcodeIO]

Using null for everything was something frequently discussed in v5. This ultimately lead to the behavior we have today. You might be able to find related issues by searching for null and/or prototype.

[tamird]

Can you link to some of those discussions?

…

On Mar 25, 2017 15:56, "Daniel Wirtz" ***@***.***> wrote: Using null for everything was something frequently discussed in v5. This ultimately lead to the behavior we have today. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#728 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABdsPH3KHBDH2OghbVLdmYrEEhsb08qhks5rpXFPgaJpZM4MpI3d> .

[dcodeIO]

#380 (including related) for example.

What could be done, though, is to wrap all of this in an utility method for convenience. Like:

function isset(obj, prop) {
  var value = obj[prop];
  if (value != null && obj.hasOwnProperty(prop))
    return typeof value !== 'object' || (Array.isArray(value) ? value.length : Object.keys(value).length) > 0;
  return false;
}

[dcodeIO]

Closing this issue for now as it hasn't received any replies recently. Feel free to reopen it if necessary!