Skip to content

Commit a84409b

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 a84409b

13 files changed

+497
-90
lines changed

‎cli/targets/proto.js

+3-3
Original file line numberDiff line numberDiff line change
@@ -102,7 +102,7 @@ function buildRoot(root) {
102102
if (pkg.length)
103103
out.push("", "package " + pkg.join(".") + ";");
104104

105-
buildOptions(ptr, ["edition"]);
105+
buildOptions(ptr);
106106
ptr.nestedArray.forEach(build);
107107
}
108108

@@ -235,7 +235,7 @@ function buildFieldOptions(field) {
235235
}
236236
var sb = [];
237237
keys.forEach(function(key) {
238-
if (key === "proto3_optional" || key === "packed" || key.startsWith("features.")) return;
238+
if (key === "proto3_optional" || key === "packed" || key === "features") return;
239239

240240
var val = field.options[key];
241241
switch (key) {
@@ -330,7 +330,7 @@ function buildOptions(object, ignore = []) {
330330
return;
331331
first = true;
332332
Object.keys(object.options).forEach(function(key) {
333-
if (ignore.includes(key) || key.startsWith("features.") || key === "edition") return;
333+
if (ignore.includes(key) || key === "features") return;
334334
if (first) {
335335
first = false;
336336
push("");

‎index.d.ts

+31-15
Original file line numberDiff line numberDiff line change
@@ -188,12 +188,6 @@ export class Enum extends ReflectionObject {
188188
/** Reserved ranges, if any. */
189189
public reserved: (number[]|string)[];
190190

191-
/**
192-
* Resolves value features
193-
* @returns `this`
194-
*/
195-
public resolve(): Enum;
196-
197191
/**
198192
* Constructs an enum from an enum descriptor.
199193
* @param name Enum name
@@ -906,11 +900,17 @@ export abstract class ReflectionObject {
906900
/** Unique name within its namespace. */
907901
public name: string;
908902

909-
/** Resolved Features. */
910-
public _features: any;
903+
/** The edition specified for this object. Only relevant for top-level objects. */
904+
public _edition: string;
911905

912-
/** Unresolved Features. */
913-
public _protoFeatures: any;
906+
/**
907+
* The default edition to use for this object if none is specified. For legacy reasons,
908+
* this is proto2 except in the JSON parsing case where it was proto3.
909+
*/
910+
public _defaultEdition: string;
911+
912+
/** Resolved Features. */
913+
public _features: object;
914914

915915
/** Parent namespace. */
916916
public parent: (Namespace|null);
@@ -954,8 +954,18 @@ export abstract class ReflectionObject {
954954
*/
955955
public resolve(): ReflectionObject;
956956

957-
/** Resolves child features from parent features */
958-
public _resolveFeatures(): void;
957+
/**
958+
* Resolves this objects editions features.
959+
* @param edition The edition we're currently resolving for.
960+
* @returns `this`
961+
*/
962+
public _resolveFeaturesRecursive(edition: string): ReflectionObject;
963+
964+
/**
965+
* Resolves child features from parent features
966+
* @param edition The edition we're currently resolving for.
967+
*/
968+
public _resolveFeatures(edition: string): void;
959969

960970
/**
961971
* Infers features from legacy syntax that may have been specified differently.
@@ -979,7 +989,7 @@ export abstract class ReflectionObject {
979989
* @param [ifNotSet] Sets the option only if it isn't currently set
980990
* @returns `this`
981991
*/
982-
public setOption(name: string, value: any, ifNotSet?: boolean): ReflectionObject;
992+
public setOption(name: string, value: any, ifNotSet?: (boolean|undefined)): ReflectionObject;
983993

984994
/**
985995
* Sets a parsed option.
@@ -1003,6 +1013,12 @@ export abstract class ReflectionObject {
10031013
* @returns Class name[, space, full name]
10041014
*/
10051015
public toString(): string;
1016+
1017+
/**
1018+
* Converts the edition this object is pinned to for JSON format.
1019+
* @returns The edition string for JSON representation
1020+
*/
1021+
public _editionToJSON(): (string|undefined);
10061022
}
10071023

10081024
/** Reflected oneof. */
@@ -2244,10 +2260,10 @@ export namespace util {
22442260
* @param dst Destination object
22452261
* @param path dot '.' delimited path of the property to set
22462262
* @param value the value to set
2247-
* @param overWrite whether or not to concatenate the values into an array or overwrite; defaults to false.
2263+
* @param [ifNotSet] Sets the option only if it isn't currently set
22482264
* @returns Destination object
22492265
*/
2250-
function setProperty(dst: { [k: string]: any }, path: string, value: object, overWrite: boolean): { [k: string]: any };
2266+
function setProperty(dst: { [k: string]: any }, path: string, value: object, ifNotSet?: (boolean|undefined)): { [k: string]: any };
22512267

22522268
/** Decorator root (TypeScript). */
22532269
let decorateRoot: Root;

‎src/object.js

+16-27
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

-8
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

+1-1
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

+4-2
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

+98
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+
});

0 commit comments

Comments
 (0)