Refuse to replace already replaced values

When values where replaced multiple times, `restore` didn't restore the
original value. This change causes `sandbox.replace` to throw if the
given object / property combination was already used.

Fixes #1779

docs/release-source/release/sandbox.md

@@ -130,7 +130,7 @@ A convenience reference for [`sinon.assert`](./assertions)
 
 #### `sandbox.replace(object, property, replacement);`
 
-Replaces `property` on `object` with `replacement` argument.
+Replaces `property` on `object` with `replacement` argument. Attempts to replace an already replaced value cause an exception.
 
 `replacement` can be any value, including `spies`, `stubs` and `fakes`.
 
@@ -154,7 +154,7 @@ console.log(myObject.myMethod());
 
 #### `sandbox.replaceGetter();`
 
-Replaces getter for `property` on `object` with `replacement` argument.
+Replaces getter for `property` on `object` with `replacement` argument. Attempts to replace an already replaced getter cause an exception.
 
 `replacement` must be a `Function`, and can be instances of `spies`, `stubs` and `fakes`.
 
@@ -175,7 +175,7 @@ console.log(myObject.myProperty);
 
 #### `sandbox.replaceSetter();`
 
-Replaces setter for `property` on `object` with `replacement` argument.
+Replaces setter for `property` on `object` with `replacement` argument. Attempts to replace an already replaced setter cause an exception.
 
 `replacement` must be a `Function`, and can be instances of `spies`, `stubs` and `fakes`.
 

lib/sinon/sandbox.js

@@ -161,9 +161,20 @@ function Sandbox() {
     function getFakeRestorer(object, property) {
         var descriptor = getPropertyDescriptor(object, property);
 
-        return function restorer() {
+        function restorer() {
             Object.defineProperty(object, property, descriptor);
-        };
+        }
+        restorer.object = object;
+        restorer.property = property;
+        return restorer;
+    }
+
+    function verifyNotReplaced(object, property) {
+        fakeRestorers.forEach(function (fakeRestorer) {
+            if (fakeRestorer.object === object && fakeRestorer.property === property) {
+                throw new TypeError("Attempted to replace " + property + " which is already replaced");
+            }
+        });
     }
 
     sandbox.replace = function replace(object, property, replacement) {
@@ -189,6 +200,8 @@ function Sandbox() {
             throw new TypeError("Cannot replace " + typeof object[property] + " with " + typeof replacement);
         }
 
+        verifyNotReplaced(object, property);
+
         // store a function for restoring the replaced property
         push.call(fakeRestorers, getFakeRestorer(object, property));
 
@@ -212,6 +225,8 @@ function Sandbox() {
             throw new Error("`object.property` is not a getter");
         }
 
+        verifyNotReplaced(object, property);
+
         // store a function for restoring the replaced property
         push.call(fakeRestorers, getFakeRestorer(object, property));
 
@@ -238,6 +253,8 @@ function Sandbox() {
             throw new Error("`object.property` is not a setter");
         }
 
+        verifyNotReplaced(object, property);
+
         // store a function for restoring the replaced property
         push.call(fakeRestorers, getFakeRestorer(object, property));
 

lib/sinon/stub.js

@@ -104,8 +104,8 @@ var proto = {
             return getCurrentBehavior(fnStub).invoke(this, arguments);
         };
 
-        functionStub.id = "stub#" + uuid++;
         var orig = functionStub;
+        functionStub.id = "stub#" + uuid++;
         functionStub = spy.create(functionStub, stubLength);
         functionStub.func = orig;
 

test/sandbox-test.js

@@ -506,6 +506,34 @@ describe("Sandbox", function () {
             }, {message: "Cannot replace function with string"});
         });
 
+        it("should refuse to replace a fake twice", function () {
+            var sandbox = this.sandbox;
+            var object = {
+                property: function () {
+                    return "apple pie";
+                }
+            };
+
+            sandbox.replace(object, "property", fake());
+
+            assert.exception(function () {
+                sandbox.replace(object, "property", fake());
+            }, {message: "Attempted to replace property which is already replaced"});
+        });
+
+        it("should refuse to replace a string twice", function () {
+            var sandbox = this.sandbox;
+            var object = {
+                property: "original"
+            };
+
+            sandbox.replace(object, "property", "first");
+
+            assert.exception(function () {
+                sandbox.replace(object, "property", "second");
+            }, {message: "Attempted to replace property which is already replaced"});
+        });
+
         it("should return the replacement argument", function () {
             var replacement = "replacement";
             var existing = "existing";
@@ -630,6 +658,21 @@ describe("Sandbox", function () {
 
             assert.equals(object.foo, "bar");
         });
+
+        it("should refuse to replace a getter twice", function () {
+            var sandbox = this.sandbox;
+            var object = {
+                get foo() {
+                    return "bar";
+                }
+            };
+
+            sandbox.replaceGetter(object, "foo", fake.returns("one"));
+
+            assert.exception(function () {
+                sandbox.replaceGetter(object, "foo", fake.returns("two"));
+            }, {message: "Attempted to replace foo which is already replaced"});
+        });
     });
 
     describe(".replaceSetter", function () {
@@ -728,6 +771,20 @@ describe("Sandbox", function () {
 
             assert.equals(object.prop, "bla");
         });
+
+        it("should refuse to replace a setter twice", function () {
+            var sandbox = this.sandbox;
+            // eslint-disable-next-line accessor-pairs
+            var object = {
+                set foo(value) {}
+            };
+
+            sandbox.replaceSetter(object, "foo", fake());
+
+            assert.exception(function () {
+                sandbox.replaceSetter(object, "foo", fake.returns("two"));
+            }, {message: "Attempted to replace foo which is already replaced"});
+        });
     });
 
     describe(".reset", function () {