Skip to content

Commit

Permalink
Fix asJSON defaults option, make it work for repeated fields.
Browse files Browse the repository at this point in the history
Fixing two issues in here:

1. When the `defaults` option was enabled, we were using `Object.keys(this)`
   which ends up returning an array that includes things like `$root` and
   asJSON. Instead we can use the `fields` we already have available.

2. For repeated values we were not setting the empty list default because the
   length check would break earlier. We're now passing through when the
   defaults option is set as well.

Added a test on this stuff as well.
  • Loading branch information
conorbranagan committed Dec 14, 2016
1 parent c33835c commit 5f9bede
Show file tree
Hide file tree
Showing 3 changed files with 36 additions and 5 deletions.
8 changes: 3 additions & 5 deletions src/message.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ module.exports = Message;

/**
* Constructs a new message instance.
*
*
* This method should be called from your custom constructors, i.e. `Message.call(this, properties)`.
* @classdesc Abstract runtime message.
* @extends {Object}
Expand Down Expand Up @@ -46,17 +46,15 @@ MessagePrototype.asJSON = function asJSON(options) {
json = {};
var keys;
if (options.defaults) {
keys = [];
for (var k in this) // eslint-disable-line guard-for-in
keys.push(k);
keys = Object.keys(fields);
} else
keys = Object.keys(this);
for (var i = 0, key; i < keys.length; ++i) {
var field = fields[key = keys[i]],
value = this[key];
if (field) {
if (field.repeated) {
if (value && value.length) {
if (value && (value.length || options.defaults)) {
var array = new Array(value.length);
for (var j = 0, l = value.length; j < l; ++j)
array[j] = field.jsonConvert(value[j], options);
Expand Down
8 changes: 8 additions & 0 deletions tests/data/message.proto
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
syntax = "proto3";

package test;

message TestRepeatedDefaults {
required string aString = 1;
repeated string repeatString = 2;
}
25 changes: 25 additions & 0 deletions tests/message.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
var tape = require("tape");
var protobuf = require("..");

tape.test("test asJSON repeated defaults", function(test) {

protobuf.load("tests/data/message.proto", function(err, root) {
if (err)
return test.fail(err.message);
test.ok(true, "should parse without errors");

var TestMessage = root.lookup("test.TestRepeatedDefaults");
var msg1 = TestMessage.create({ aString: 'foo' });

var defaultsOn = msg1.asJSON({ defaults: true });
test.equal(defaultsOn.aString, 'foo', "should set aString value");
test.same(defaultsOn.repeatString, [], "should set repeated default");

var defaultsOff = msg1.asJSON({ defaults: false });
test.equal(defaultsOff.aString, 'foo', "should set aString value");
test.same(defaultsOff.repeatString, undefined, "should not set repeated default");

test.end();
});

});

0 comments on commit 5f9bede

Please sign in to comment.