From 836ab1b5aaab6b430ceecf7cf2e27d20ea6bd285 Mon Sep 17 00:00:00 2001
From: Trent Mick <trent.mick@elastic.co>
Date: Thu, 2 Sep 2021 14:48:18 -0700
Subject: [PATCH] fix: apm.removePatch(name, somethingNotRegistered) would
 remove last handler (#2315)

There was a bug in the Instrumentation class's data structure for added
patches where removePatch(name, somethingBogus) would "find" that
handler at index -1 and remove the last handler.
---
 CHANGELOG.asciidoc                 |  4 ++++
 lib/instrumentation/named-array.js |  8 +++++---
 test/agent.test.js                 | 32 ++++++++++++++++++++++++++++++
 3 files changed, 41 insertions(+), 3 deletions(-)

diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc
index c42484379c4..ad1f497b889 100644
--- a/CHANGELOG.asciidoc
+++ b/CHANGELOG.asciidoc
@@ -44,6 +44,10 @@ Notes:
 [float]
 ===== Bug fixes
 
+* Fix a bug in `apm.removePatch(module, aHandler)` that would remove the
+  last registered handler if `aHandler` did not match any currently
+  registered handlers. ({pull}2315[#2315])
+
 * Fix a crash in instrumentation of the old Elasticsearch client
   (`elasticsearch`) for some rarer cases of client options -- for example
   passing multiple hosts. ({pull}2312[#2312])
diff --git a/lib/instrumentation/named-array.js b/lib/instrumentation/named-array.js
index d8ca31bc4f6..3fe2342f7fb 100644
--- a/lib/instrumentation/named-array.js
+++ b/lib/instrumentation/named-array.js
@@ -34,9 +34,11 @@ class NamedArray {
     const array = this.get(key)
     if (array) {
       const index = array.indexOf(value)
-      array.splice(index, 1)
-      if (!array.length) {
-        this.clear(key)
+      if (index !== -1) {
+        array.splice(index, 1)
+        if (!array.length) {
+          this.clear(key)
+        }
       }
     }
   }
diff --git a/test/agent.test.js b/test/agent.test.js
index d3d07660fcf..9b6e28dc9f0 100644
--- a/test/agent.test.js
+++ b/test/agent.test.js
@@ -1680,6 +1680,38 @@ test('patches', function (t) {
     agent.destroy()
     t.end()
   })
+
+  t.test('#removePatch(name, oops) does not remove patches', function (t) {
+    const agent = new Agent().start(agentOptsNoopTransport)
+
+    const moduleName = 'removePatch-test-module'
+    t.notOk(agent._instrumentation._patches.has(moduleName))
+
+    const handler1 = function (exports) { return exports }
+    const handler2 = function (exports) { return exports }
+    agent.addPatch(moduleName, handler1)
+    agent.addPatch(moduleName, handler2)
+    const modulePatches = agent._instrumentation._patches.get(moduleName)
+    t.ok(modulePatches.length === 2 &&
+      modulePatches[0] === handler1 &&
+      modulePatches[1] === handler2, 'module patches are as expected')
+
+    agent.removePatch(moduleName)
+    t.equal(agent._instrumentation._patches.get(moduleName).length, 2,
+      'still have 2 patches after removePatch(name)')
+    agent.removePatch(moduleName, 'this is not one of the registered handlers')
+    t.equal(agent._instrumentation._patches.get(moduleName).length, 2,
+      'still have 2 patches after removePatch(name, oops)')
+    agent.removePatch(moduleName, function oops () {})
+    t.equal(agent._instrumentation._patches.get(moduleName).length, 2,
+      'still have 2 patches after removePatch(name, function oops () {})')
+
+    agent.removePatch(moduleName, handler2)
+    agent.removePatch(moduleName, handler1)
+    agent.destroy()
+
+    t.end()
+  })
 })
 
 test('#registerMetric(name, labels, callback)', function (t) {