fix: Remove renamed reflection objects by identity (#2324)

Author: dcodeIO

index.d.ts

@@ -2666,6 +2666,15 @@ export namespace util {
      */
     function toObject(array: any[]): { [k: string]: any };
 
+    /**
+     * Removes the first matching value from an object.
+     * @param object Object to remove from
+     * @param value Value to remove
+     * @param [key] Optional key for fast path removal
+     * @returns `true` if removed, otherwise `false`
+     */
+    function remove(object: ({ [k: string]: any }|undefined), value: any, key?: string): boolean;
+
     /**
      * Tests whether the specified name is a reserved word in JS.
      * @param name Name to test

src/namespace.js

@@ -321,7 +321,8 @@ Namespace.prototype.remove = function remove(object) {
     if (object.parent !== this)
         throw Error(object + " is not a member of " + this);
 
-    delete this.nested[object.name];
+    if (!util.remove(this.nested, object, object.name))
+        throw Error(object + " is not a member of " + this);
     if (!Object.keys(this.nested).length)
         this.nested = undefined;
 

src/type.js

@@ -425,21 +425,19 @@ Type.prototype.remove = function remove(object) {
         // See Type#add for the reason why extension fields are excluded here.
 
         /* istanbul ignore if */
-        if (!this.fields || this.fields[object.name] !== object)
+        if (!util.remove(this.fields, object, object.name))
             throw Error(object + " is not a member of " + this);
 
-        delete this.fields[object.name];
         object.parent = null;
         object.onRemove(this);
         return clearCache(this);
     }
     if (object instanceof OneOf) {
 
         /* istanbul ignore if */
-        if (!this.oneofs || this.oneofs[object.name] !== object)
+        if (!util.remove(this.oneofs, object, object.name))
             throw Error(object + " is not a member of " + this);
 
-        delete this.oneofs[object.name];
         object.parent = null;
         object.onRemove(this);
         return clearCache(this);

src/util.js

@@ -58,6 +58,28 @@ util.toObject = function toObject(array) {
     return object;
 };
 
+/**
+ * Removes the first matching value from an object.
+ * @param {Object.<string,*>|undefined} object Object to remove from
+ * @param {*} value Value to remove
+ * @param {string} [key] Optional key for fast path removal
+ * @returns {boolean} `true` if removed, otherwise `false`
+ */
+util.remove = function remove(object, value, key) {
+    if (!object)
+        return false;
+    if (key !== undefined && Object.prototype.hasOwnProperty.call(object, key) && object[key] === value) {
+        delete object[key];
+        return true;
+    }
+    for (var names = Object.keys(object), i = 0; i < names.length; ++i)
+        if (object[names[i]] === value) {
+            delete object[names[i]];
+            return true;
+        }
+    return false;
+};
+
 /**
  * Tests whether the specified name is a reserved word in JS.
  * @param {string} name Name to test

tests/api_namespace.js

@@ -131,6 +131,13 @@ message TestMessage {\
         ns.remove(new protobuf.Enum("Enm"));
     }, Error, "should throw when trying to remove non-children");
 
+    var renamed = new protobuf.Type("RenamedBefore");
+    ns.add(renamed);
+    renamed.name = "RenamedAfter";
+    ns.remove(renamed);
+    test.equal(renamed.parent, null, "should remove renamed nested objects");
+    test.equal(ns.get("RenamedBefore"), null, "should remove renamed nested objects from the original key");
+
     test.throws(function() {
         ns.add(new protobuf.Enum("MyEnum", {}));
         ns.define("MyEnum");

tests/api_type.js

@@ -116,6 +116,19 @@ tape.test("reflected types", function(test) {
         type.add(new protobuf.Field("b", 2, "uint32"));
     }, Error, "should throw when trying to add reserved names");
 
+    var renameType = new protobuf.Type("Rename");
+    var field = new protobuf.Field("before", 1, "string");
+    var oneof = new protobuf.OneOf("choice");
+    renameType.add(field);
+    renameType.add(oneof);
+    field.name = "after";
+    oneof.name = "selection";
+    renameType.remove(field);
+    renameType.remove(oneof);
+    test.equal(field.parent, null, "should remove renamed fields");
+    test.equal(oneof.parent, null, "should remove renamed oneofs");
+    test.same(renameType.fields, {}, "should remove renamed fields from the original key");
+    test.same(renameType.oneofs, {}, "should remove renamed oneofs from the original key");
 
     test.end();
 });