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

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

Closed
tamird opened this Issue Mar 25, 2017 · 11 comments

Comments

Projects
None yet
3 participants
@tamird

tamird commented Mar 25, 2017

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

This comment has been minimized.

Owner

dcodeIO commented Mar 25, 2017

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

This comment has been minimized.

tamird commented Mar 25, 2017

@dcodeIO

This comment has been minimized.

Owner

dcodeIO commented Mar 25, 2017

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

This comment has been minimized.

tamird commented Mar 25, 2017

@dcodeIO

This comment has been minimized.

Owner

dcodeIO commented Mar 25, 2017

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.

@dcodeIO dcodeIO closed this Mar 25, 2017

@dcodeIO dcodeIO reopened this Mar 25, 2017

@tamird

This comment has been minimized.

tamird commented Mar 25, 2017

@dcodeIO

This comment has been minimized.

Owner

dcodeIO commented Mar 25, 2017

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

This comment has been minimized.

tamird commented Mar 25, 2017

@dcodeIO

This comment has been minimized.

Owner

dcodeIO commented Mar 25, 2017

#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 added a commit that referenced this issue Mar 25, 2017

@dcodeIO dcodeIO added the invalid label Mar 29, 2017

@dcodeIO

This comment has been minimized.

Owner

dcodeIO commented Apr 11, 2017

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

@dcodeIO dcodeIO closed this Apr 11, 2017

@vjpr

This comment has been minimized.

vjpr commented Dec 18, 2017

I had some tough time with this.

Assume the protobuf def message is something like:

{
  a: {
    foo: Number,
    bar: Number,
  }
}
const message = MyMessage.decodeDelimited(bytes)
const messageJson =  MyMessage.toObject(message)

// {foo: 1}
console.log(message.a)

// {foo: 1}
console.log(messageJson.a)

// 0
console.log(message.a.bar)

// undefined
console.log(messageJson.a.bar)

If you forget to use toObject you get a very different result.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment