Skip to content
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.

Sign up for GitHub

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

Move default values to prototypes and ditch nulls #380

Closed
wants to merge 1 commit into from
Closed

Move default values to prototypes and ditch nulls #380

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 6 additions & 5 deletions src/ProtoBuf/Builder/Message.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,10 +23,11 @@ var Message = function(values, var_args) {
// Create fields and set default values
for (i=0, k=fields.length; i<k; ++i) {
var field = fields[i];
this[field.name] =
field.repeated ? [] :
(field.map ? new ProtoBuf.Map(field) : null);
if ((field.required || T.syntax === 'proto3') &&
if (field.repeated)
this[field.name] = [];
else if (field.map)
this[field.name] = new ProtoBuf.Map(field);
else if (T.syntax === 'proto3' &&
field.defaultValue !== null)
this[field.name] = field.defaultValue;
}
Expand Down Expand Up @@ -128,7 +129,7 @@ MessagePrototype.set = function(keyOrObj, value, noAssert) {
var currentField = this[field.oneof.name]; // Virtual field references currently set field
if (value !== null) {
if (currentField !== null && currentField !== field.name)
this[currentField] = null; // Clear currently set field
delete this[currentField]; // Clear currently set field
this[field.oneof.name] = field.name; // Point virtual field at this field
} else if (/* value === null && */currentField === keyOrObj)
this[field.oneof.name] = null; // Clear virtual field (current field explicitly cleared)
Expand Down
16 changes: 11 additions & 5 deletions src/ProtoBuf/Reflect/Message.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,13 @@ MessagePrototype.build = function(rebuild) {
throw Error("Illegal reflect child of "+this.toString(true)+": "+this.children[i].toString(true));
}

// Default values
for (i=0, k=this._fields.length; i<k; i++) {
var field = this._fields[i];
if (!field.repeated && !field.map && T.syntax !== 'proto3')
clazz.prototype[field.name] = field.defaultValue;
}

return this.clazz = clazz;
};

Expand All @@ -130,7 +137,7 @@ MessagePrototype.encode = function(message, buffer, noVerify) {
if (field.required && val === null) {
if (fieldMissing === null)
fieldMissing = field;
} else
} else if (field.required || message.hasOwnProperty(field.name))
field.encode(noVerify ? val : field.verifyValue(val), buffer, message);
}
if (fieldMissing !== null) {
Expand Down Expand Up @@ -258,7 +265,7 @@ MessagePrototype.decode = function(buffer, length, expectedGroupEndId) {
if (field.oneof) { // Field is part of an OneOf (not a virtual OneOf field)
var currentField = msg[field.oneof.name]; // Virtual field references currently set field
if (currentField !== null && currentField !== field.name)
msg[currentField] = null; // Clear currently set field
delete msg[currentField]; // Clear currently set field
msg[field.oneof.name] = field.name; // Point virtual field at this field
}
}
Expand All @@ -267,15 +274,14 @@ MessagePrototype.decode = function(buffer, length, expectedGroupEndId) {
// Check if all required fields are present and set default values for optional fields that are not
for (var i=0, k=this._fields.length; i<k; ++i) {
field = this._fields[i];
if (msg[field.name] === null) {
if (!msg.hasOwnProperty(field.name)) {
if (this.syntax === "proto3") { // Proto3 sets default values by specification
msg[field.name] = field.defaultValue;
} else if (field.required) {
var err = Error("Missing at least one required field for " + this.toString(true) + ": " + field.name);
err["decoded"] = msg; // Still expose what we got
throw(err);
} else if (ProtoBuf.populateDefaults && field.defaultValue !== null)
msg[field.name] = field.defaultValue;
}
}
}
return msg;
Expand Down
10 changes: 3 additions & 7 deletions src/ProtoBuf/Reflect/Message/Field.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,14 +145,10 @@ FieldPrototype.build = function() {
if (this.map)
this.keyElement = new Element(this.keyType, undefined, true, this.syntax);

// In proto3, fields do not have field presence, and every field is set to
// its type's default value ("", 0, 0.0, or false).
if (this.syntax === 'proto3' && !this.repeated && !this.map)
this.defaultValue = Element.defaultFieldValue(this.type);

// Otherwise, default values are present when explicitly specified
else if (typeof this.options['default'] !== 'undefined')
if (typeof this.options['default'] !== 'undefined')
this.defaultValue = this.verifyValue(this.options['default']);
else if (!this.repeated && !this.map)
this.defaultValue = Element.defaultFieldValue(this.type);
};

/**
Expand Down
8 changes: 0 additions & 8 deletions src/protobuf.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -248,14 +248,6 @@ ProtoBuf.convertFieldsToCamelCase = false;
*/
ProtoBuf.populateAccessors = true;

/**
* By default, messages are populated with default values if a field is not present on the wire. To disable
* this behavior, set this setting to `false`.
* @type {boolean}
* @expose
*/
ProtoBuf.populateDefaults = true;

//? include("ProtoBuf/Util.js");

//? include("ProtoBuf/Lang.js");
Expand Down
31 changes: 16 additions & 15 deletions tests/suite.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -432,7 +432,7 @@
var builder = ProtoBuf.loadProto("message Test { optional bool ok = 1 [ default = false ]; }"),
Test = builder.build("Test"),
t = new Test();
test.strictEqual(t.ok, null); // Not set as it is optional
test.strictEqual(t.ok, false); // = default when missing
t.setOk(true);
test.strictEqual(t.ok, true);
test.strictEqual(Test.decode(t.encode()).ok, true);
Expand Down Expand Up @@ -561,6 +561,7 @@
var Test = builder.build("Test");
var t = new Test(), bb = new ByteBuffer(2);
t.setA(1);
t.b = null;
try {
bb = t.encode(bb).flip();
test.ok(false);
Expand All @@ -577,7 +578,7 @@
// But still be able to access the rest
var t3 = e.decoded;
test.strictEqual(t3.a, 1);
test.strictEqual(t3.b, null);
test.strictEqual(t3.b, 0);
}
test.strictEqual(t2, undefined);
} catch (e) {
Expand Down Expand Up @@ -931,8 +932,8 @@
ProtoBuf.loadProto(y, builder);
var Test = builder.build('x.Test');
var inst = new Test();
test.strictEqual(inst[".x.first_val"], null);
test.strictEqual(inst[".y.second_val"], null);
test.strictEqual(inst[".x.first_val"], 0);
test.strictEqual(inst[".y.second_val"], 0);
test.done();
},

Expand Down Expand Up @@ -1148,13 +1149,13 @@
test.strictEqual(myOneOf.my_oneof, "id");
myOneOf.set("name", "me");
test.strictEqual(myOneOf.my_oneof, "name");
test.strictEqual(myOneOf.id, null);
test.strictEqual(myOneOf.id, 0);
var bb = myOneOf.encode().compact();
test.strictEqual(bb.toString("debug"), "<12 02 6D 65>"); // id 2, wt 2, len 2
myOneOf = MyOneOf.decode(bb);
test.strictEqual(myOneOf.my_oneof, "name");
test.strictEqual(myOneOf.name, "me");
test.strictEqual(myOneOf.id, null);
test.strictEqual(myOneOf.id, 0);
} catch (e) {
fail(e);
}
Expand Down Expand Up @@ -1299,12 +1300,12 @@
var builder = ProtoBuf.loadProtoFile(__dirname+"/optional.proto");
var Test1 = builder.build("Test1");
var test1 = new Test1();
test.strictEqual(test1.a, null);
test.deepEqual(Object.keys(test1), ['a','b']);
test.strictEqual(test1.a, 0);
test.deepEqual(Object.keys(test1), []);
var bb = test1.encode();
test1 = Test1.decode(bb);
test.strictEqual(test1.a, null);
test.deepEqual(Object.keys(test1), ['a','b']);
test.strictEqual(test1.a, 0);
test.deepEqual(Object.keys(test1), []);
} catch (e) {
fail(e);
}
Expand All @@ -1320,20 +1321,20 @@
var msg = new Test();

// Reverted collision on 1st
test.strictEqual(msg.some_field, null);
test.strictEqual(msg.someField, null);
test.strictEqual(msg.some_field, 0);
test.strictEqual(msg.someField, 0);
test.equal(TTest.getChild("some_field").id, 1);
test.equal(TTest.getChild("someField").id, 2);


// Reverted collision on 2nd
test.strictEqual(msg.aField, null);
test.strictEqual(msg.a_field, null);
test.strictEqual(msg.aField, 0);
test.strictEqual(msg.a_field, 0);
test.equal(TTest.getChild("aField").id, 3);
test.equal(TTest.getChild("a_field").id, 4);

// No collision
test.strictEqual(msg.itsAField, null);
test.strictEqual(msg.itsAField, 0);
test.equal(TTest.getChild("itsAField").id, 5);

test.ok(typeof msg.set_its_a_field === "function");
Expand Down