Skip to content

Commit 4088c3f

Browse files
mkruskal-googlemkruskal-google
committed
Cleanup: remove protoFeatures, move editions out of options, handle unresolved options for fromJSON, fix aggregate feature handling
1 parent 9c5a178 commit 4088c3f

File tree

11 files changed

+463
-72
lines changed

11 files changed

+463
-72
lines changed

‎src/object.js‎

Lines changed: 16 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -67,17 +67,6 @@ function ReflectionObject(name, options) {
6767
*/
6868
this._features = {};
6969

70-
/**
71-
* Whether or not features have been resolved.
72-
* @type {boolean}
73-
*/
74-
this._featuresResolved = false;
75-
76-
/**
77-
* Unresolved Features.
78-
*/
79-
this._protoFeatures = null;
80-
8170
/**
8271
* Parent namespace.
8372
* @type {Namespace|null}
@@ -205,17 +194,14 @@ ReflectionObject.prototype._resolveFeaturesRecursive = function _resolveFeatures
205194
* @returns {undefined}
206195
*/
207196
ReflectionObject.prototype._resolveFeatures = function _resolveFeatures(edition) {
208-
if (this._featuresResolved) {
209-
return;
210-
}
211-
212197
var defaults = {};
213198

214199
if (!edition) {
215200
throw new Error("Unknown edition for " + this.fullName);
216201
}
217202

218-
var protoFeatures = Object.assign(Object.assign({}, this._protoFeatures), this._inferLegacyProtoFeatures(edition));
203+
var protoFeatures = Object.assign(this.options ? Object.assign({}, this.options.features) : {},
204+
this._inferLegacyProtoFeatures(edition));
219205

220206
if (this._edition) {
221207
// For a namespace marked with a specific edition, reset defaults.
@@ -248,9 +234,7 @@ ReflectionObject.prototype._resolveFeatures = function _resolveFeatures(edition)
248234
if (this.extensionField) {
249235
// Sister fields should have the same features as their extensions.
250236
this.extensionField._features = this._features;
251-
this.extensionField._featuresResolved = true;
252237
}
253-
this._featuresResolved = true;
254238
};
255239

256240
/**
@@ -278,14 +262,25 @@ ReflectionObject.prototype.getOption = function getOption(name) {
278262
* Sets an option.
279263
* @param {string} name Option name
280264
* @param {*} value Option value
281-
* @param {boolean} [ifNotSet] Sets the option only if it isn't currently set
265+
* @param {boolean|undefined} [ifNotSet] Sets the option only if it isn't currently set
282266
* @returns {ReflectionObject} `this`
283267
*/
284268
ReflectionObject.prototype.setOption = function setOption(name, value, ifNotSet) {
285-
if (!ifNotSet || !this.options || this.options[name] === undefined) {
269+
if (!this.options)
270+
this.options = {};
271+
if (name === "features") {
272+
if (ifNotSet) {
273+
this.options.features = Object.assign(Object.assign({}, value), this.options.features || {});
274+
} else {
275+
this.options.features = Object.assign(this.options.features || {}, value);
276+
}
277+
} else if (/^features\./.test(name)) {
278+
util.setProperty(this.options, name, value, ifNotSet);
279+
} else if (!ifNotSet || this.options[name] === undefined) {
286280
if (this.getOption(name) !== value) this.resolved = false;
287-
(this.options || (this.options = {}))[name] = value;
281+
this.options[name] = value;
288282
}
283+
289284
return this;
290285
};
291286

@@ -326,12 +321,6 @@ ReflectionObject.prototype.setParsedOption = function setParsedOption(name, valu
326321
parsedOptions.push(newOpt);
327322
}
328323

329-
330-
if (isFeature) {
331-
var features = parsedOptions.find(x => {return Object.prototype.hasOwnProperty.call(x, "features");});
332-
this._protoFeatures = features.features || {};
333-
}
334-
335324
return this;
336325
};
337326

‎src/parse.js‎

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -88,7 +88,6 @@ function parse(source, root, options) {
8888

8989
var topLevelObjects = [];
9090
var topLevelOptions = {};
91-
var topLevelParsedOptions = {};
9291

9392
var applyCase = options.keepCase ? function(name) { return name; } : util.camelCase;
9493

@@ -98,9 +97,6 @@ function parse(source, root, options) {
9897
Object.keys(topLevelOptions).forEach(opt => {
9998
if (obj.getOption(opt) !== undefined) return;
10099
obj.setOption(opt, topLevelOptions[opt]);
101-
if (topLevelParsedOptions[opt.substring("features.".length)]) {
102-
obj.setParsedOption("features", topLevelParsedOptions[opt.substring("features.".length)], opt.substring("features.".length));
103-
}
104100
});
105101
});
106102
}
@@ -769,10 +765,6 @@ function parse(source, root, options) {
769765
}
770766

771767
function setParsedOption(parent, name, value, propName) {
772-
if (ptr === parent && /^features$/.test(name)) {
773-
topLevelParsedOptions[propName] = value;
774-
return;
775-
}
776768
if (parent.setParsedOption)
777769
parent.setParsedOption(name, value, propName);
778770
}

‎src/root.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,7 @@ Root.fromJSON = function fromJSON(json, root) {
5151
root = new Root();
5252
if (json.options)
5353
root.setOptions(json.options);
54-
return root.addJSON(json.nested);
54+
return root.addJSON(json.nested).resolveAll();
5555
};
5656

5757
/**

‎src/util.js‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -171,10 +171,10 @@ util.decorateEnum = function decorateEnum(object) {
171171
* @param {Object.<string,*>} dst Destination object
172172
* @param {string} path dot '.' delimited path of the property to set
173173
* @param {Object} value the value to set
174-
* @param {boolean} overWrite whether or not to concatenate the values into an array or overwrite; defaults to false.
174+
* @param {boolean|undefined} [ifNotSet] Sets the option only if it isn't currently set
175175
* @returns {Object.<string,*>} Destination object
176176
*/
177-
util.setProperty = function setProperty(dst, path, value) {
177+
util.setProperty = function setProperty(dst, path, value, ifNotSet) {
178178
function setProp(dst, path, value) {
179179
var part = path.shift();
180180
if (part === "__proto__" || part === "prototype") {
@@ -184,6 +184,8 @@ util.setProperty = function setProperty(dst, path, value) {
184184
dst[part] = setProp(dst[part] || {}, path, value);
185185
} else {
186186
var prevValue = dst[part];
187+
if (prevValue && ifNotSet)
188+
return dst;
187189
if (prevValue)
188190
value = [].concat(prevValue).concat(value);
189191
dst[part] = value;

‎tests/api_enum.js‎

Lines changed: 98 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -89,3 +89,101 @@ tape.test("reflected enums", function(test) {
8989

9090
test.end();
9191
});
92+
93+
tape.test("feature resolution legacy proto3", function(test) {
94+
var json = {
95+
values: {
96+
a: 0, b: 1
97+
}
98+
};
99+
var messageJson = {
100+
fields: {},
101+
nested: { Enum: { values: {
102+
a: 0, b:1
103+
} } }
104+
};
105+
var root = new protobuf.Root();
106+
var Enum = protobuf.Enum.fromJSON("Enum", json);
107+
var Message = protobuf.Type.fromJSON("Message", messageJson)
108+
var Nested = Message.nested.Enum;
109+
root.add(Enum).add(Message).resolveAll();
110+
111+
test.same(Enum.toJSON(), json, "JSON should roundtrip");
112+
test.same(Message.toJSON(), messageJson, "container JSON should roundtrip");
113+
test.same(Nested.toJSON(), messageJson.nested.Enum, "nested JSON should roundtrip");
114+
115+
test.equal(Enum._edition, "proto3", "should infer proto3 syntax");
116+
test.equal(Enum._features.enum_type, "OPEN", "should be open by default");
117+
118+
test.equal(Nested._edition, null, "should not set edition");
119+
test.equal(Nested._features.enum_type, "OPEN", "should be open by default");
120+
121+
test.end();
122+
});
123+
124+
tape.test("feature resolution proto2", function(test) {
125+
var json = {
126+
edition: "proto2",
127+
values: {
128+
a: 0, b: 1
129+
}
130+
};
131+
var messageJson = {
132+
edition: "proto2",
133+
fields: {},
134+
nested: { Enum: { values: {
135+
a: 0, b: 1
136+
} } }
137+
};
138+
var root = new protobuf.Root();
139+
var Enum = protobuf.Enum.fromJSON("Enum", json);
140+
var Message = protobuf.Type.fromJSON("Message", messageJson)
141+
var Nested = Message.nested.Enum;
142+
root.add(Enum).add(Message).resolveAll();
143+
144+
test.same(Enum.toJSON(), json, "JSON should roundtrip");
145+
test.same(Message.toJSON(), messageJson, "container JSON should roundtrip");
146+
test.same(Nested.toJSON(), messageJson.nested.Enum, "nested JSON should roundtrip");
147+
148+
test.equal(Enum._edition, "proto2", "should set edition");
149+
test.equal(Enum._features.enum_type, "CLOSED", "should be closed by default");
150+
151+
test.equal(Nested._edition, null, "should not set edition");
152+
test.equal(Nested._features.enum_type, "CLOSED", "should be closed by default");
153+
154+
test.end();
155+
});
156+
157+
tape.test("feature resolution legacy proto3", function(test) {
158+
var json = {
159+
edition: "2023",
160+
values: {
161+
a: 0, b: 1
162+
}
163+
};
164+
var messageJson = {
165+
edition: "2023",
166+
options: { features: { enum_type: "CLOSED" } },
167+
fields: {},
168+
nested: { Enum: { values: {
169+
a: 0, b: 1
170+
} } }
171+
};
172+
var root = new protobuf.Root();
173+
var Enum = protobuf.Enum.fromJSON("Enum", json);
174+
var Message = protobuf.Type.fromJSON("Message", messageJson)
175+
var Nested = Message.nested.Enum;
176+
root.add(Enum).add(Message).resolveAll();
177+
178+
test.same(Enum.toJSON(), json, "JSON should roundtrip");
179+
test.same(Message.toJSON(), messageJson, "container JSON should roundtrip");
180+
test.same(Nested.toJSON(), messageJson.nested.Enum, "nested JSON should roundtrip");
181+
182+
test.equal(Enum._edition, "2023", "should set edition");
183+
test.equal(Enum._features.enum_type, "OPEN", "should be open by default");
184+
185+
test.equal(Nested._edition, null, "should not set edition");
186+
test.equal(Nested._features.enum_type, "CLOSED", "should inherit override");
187+
188+
test.end();
189+
});

‎tests/api_field.js‎

Lines changed: 100 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -82,4 +82,103 @@ tape.test("reflected fields", function(test) {
8282
test.equal(field.resolved, 2, "should not resolve again if already resolved");
8383

8484
test.end();
85-
});
85+
});
86+
87+
tape.test("feature resolution legacy proto3", function(test) {
88+
var json = {
89+
type: "string", id: 1000, extend: "Message"
90+
};
91+
var messageJson = {
92+
fields: {
93+
a: { type: "string", id: 1}
94+
},
95+
extensions: [[1, 100000]],
96+
};
97+
var root = new protobuf.Root();
98+
var ext = protobuf.Field.fromJSON("ext", json);
99+
var Message = protobuf.Type.fromJSON("Message", messageJson)
100+
var field = Message.fields.a;
101+
root.add(ext).add(Message).resolveAll();
102+
103+
test.same(ext.toJSON(), json, "JSON should roundtrip");
104+
test.same(Message.toJSON(), messageJson, "container JSON should roundtrip");
105+
test.same(field.toJSON(), messageJson.fields.a, "nested JSON should roundtrip");
106+
107+
test.equal(ext._edition, "proto3", "should set edition");
108+
test.equal(ext._features.utf8_validation, "VERIFY", "should verify by default");
109+
test.ok(ext.hasPresence, "should have explicit presence");
110+
111+
test.equal(field._edition, null, "should not set edition");
112+
test.equal(ext._features.utf8_validation, "VERIFY", "should verify by default");
113+
test.notOk(field.hasPresence, "should be implicit by default");
114+
115+
test.end();
116+
});
117+
118+
tape.test("feature resolution proto2", function(test) {
119+
var json = {
120+
edition: "proto2",
121+
type: "string", id: 1000, extend: "Message"
122+
};
123+
var messageJson = {
124+
edition: "proto2",
125+
fields: {
126+
a: { type: "string", id: 1}
127+
},
128+
extensions: [[1, 100000]],
129+
};
130+
var root = new protobuf.Root();
131+
var ext = protobuf.Field.fromJSON("ext", json);
132+
var Message = protobuf.Type.fromJSON("Message", messageJson)
133+
var field = Message.fields.a;
134+
root.add(ext).add(Message).resolveAll();
135+
136+
test.same(ext.toJSON(), json, "JSON should roundtrip");
137+
test.same(Message.toJSON(), messageJson, "container JSON should roundtrip");
138+
test.same(field.toJSON(), messageJson.fields.a, "nested JSON should roundtrip");
139+
140+
test.equal(ext._edition, "proto2", "should set edition");
141+
test.ok(ext.hasPresence, "should have explicit presence");
142+
test.equal(ext._features.utf8_validation, "NONE", "should not verify by default");
143+
144+
test.equal(field._edition, null, "should not set edition");
145+
test.ok(field.hasPresence, "should be explicit by default");
146+
test.equal(ext._features.utf8_validation, "NONE", "should not verify by default");
147+
148+
test.end();
149+
});
150+
151+
tape.test("feature resolution editions", function(test) {
152+
var json = {
153+
edition: "2023",
154+
options: { features: { utf8_validation: "NONE" } },
155+
type: "string", id: 1000, extend: "Message"
156+
};
157+
var messageJson = {
158+
edition: "2023",
159+
options: { features: { field_presence: "IMPLICIT" } },
160+
fields: {
161+
a: { type: "string", id: 1}
162+
},
163+
extensions: [[1, 100000]],
164+
};
165+
var root = new protobuf.Root();
166+
var ext = protobuf.Field.fromJSON("ext", json);
167+
var Message = protobuf.Type.fromJSON("Message", messageJson)
168+
var field = Message.fields.a;
169+
root.add(ext).add(Message).resolveAll();
170+
171+
test.same(ext.toJSON(), json, "JSON should roundtrip");
172+
test.same(Message.toJSON(), messageJson, "container JSON should roundtrip");
173+
test.same(field.toJSON(), messageJson.fields.a, "nested JSON should roundtrip");
174+
175+
test.equal(ext._edition, "2023", "should set edition");
176+
test.ok(ext.hasPresence, "should be explicit by default");
177+
test.equal(ext._features.utf8_validation, "NONE", "should get file overrides");
178+
179+
test.equal(field._edition, null, "should not set edition");
180+
test.notOk(field.hasPresence, "should get message overrides");
181+
test.equal(field._features.utf8_validation, "VERIFY", "should verify by default");
182+
183+
test.end();
184+
});

0 commit comments

Comments
 (0)