Merge pull request #643 from appsignal/fix-global-flags

Fix global flags

Author: unflxw

Diff for: packages/javascript/.changesets/fix-regexes-with-global-flag-on--ignoreerrors-.md

@@ -0,0 +1,6 @@
+---
+bump: patch
+type: fix
+---
+
+Fix an issue when regexes with the `g` global flag are used on `ignoreErrors`. Before this change, after successfully matching on an error to ignore, if the following error would also match the same regular expression, the regular expression would then fail to match it.

Diff for: packages/javascript/jest.config.js

@@ -4,5 +4,6 @@ module.exports = {
   roots: ["<rootDir>/src"],
   transform: {
     "^.+\\.tsx?$": "ts-jest"
-  }
+  },
+  restoreMocks: true,
 }

Diff for: packages/javascript/src/__mocks__/api.ts

@@ -1,7 +1,16 @@
-export const pushMock = jest.fn(span => Promise.resolve(span))
+let mock: jest.Mock
+
+beforeEach(() => {
+  mock = jest.fn(span => Promise.resolve(span))
+})
+
+export const pushMockCall = (index: number = 0) => {
+  expect(mock).toHaveBeenCalledTimes(index + 1)
+  return mock.mock.calls[index][0].serialize()
+}
 
 export const PushApi = jest.fn().mockImplementation(() => {
   return {
-    push: pushMock
+    push: mock
   }
 })

Diff for: packages/javascript/src/__tests__/index.test.ts

@@ -3,8 +3,10 @@ import { VERSION } from "../version"
 import Appsignal from "../index"
 import { Span } from "../span"
 
+import * as api from "../api"
+
 // @ts-ignore
-import { pushMock } from "../api"
+const pushMockCall: (index?: number) => any = api.pushMockCall
 
 jest.mock("../api")
 
@@ -51,7 +53,10 @@ describe("Appsignal", () => {
       ignoreErrors: ignored
     })
 
-    expect(appsignal.ignored).toEqual(ignored)
+    // We clean the given regexes to remove the `g` flag, as it causes
+    // matches to misbehave by remembering the last matched position:
+    // https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/RegExp/test#using_test_on_a_regex_with_the_global_flag
+    expect(appsignal.ignored).toEqual([/Ignore me/m])
   })
 
   describe("send", () => {
@@ -71,10 +76,18 @@ describe("Appsignal", () => {
 
       appsignal.send(new Error(name))
 
+      expect(console.warn).toBeCalledTimes(1)
       expect(console.warn).toBeCalledWith(
         `[APPSIGNAL]: Ignored an error: ${name}`
       )
 
+      // As the regex used has the `g` flag, it would
+      // remember the last matched position and fail to match again:
+      // https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/RegExp/test#using_test_on_a_regex_with_the_global_flag
+      // We remove the `g` flag to make it match again, so it should match.
+      appsignal.send(new Error(name))
+      expect(console.warn).toBeCalledTimes(2)
+
       // reset
       console.warn = original
     })
@@ -83,7 +96,7 @@ describe("Appsignal", () => {
       appsignal = new Appsignal({
         key: "TESTKEY",
         namespace: "test",
-        matchBacktracePaths: [/here(.*)/]
+        matchBacktracePaths: [/here(.*)/g]
       })
 
       const error = new Error("test error")
@@ -94,15 +107,23 @@ describe("Appsignal", () => {
 
       appsignal.send(error)
 
-      expect(pushMock).toHaveBeenCalled()
-      const payload = pushMock.mock.calls[0][0].serialize()
+      const firstPayload = pushMockCall(0)
 
-      expect(payload.error.backtrace).toEqual([
+      expect(firstPayload.error.backtrace).toEqual([
         "Error: test error",
         "    at Foo (/istheapp.js:13:10)"
       ])
 
-      expect(payload.environment.backtrace_paths_matched).toEqual("1")
+      expect(firstPayload.environment.backtrace_paths_matched).toEqual("1")
+
+      // As the regex used has the `g` flag, it would
+      // remember the last matched position and fail to match again:
+      // https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/RegExp/test#using_test_on_a_regex_with_the_global_flag
+      // We remove the `g` flag to make it match again, so it should match.
+      appsignal.send(error)
+
+      const secondPayload = pushMockCall(1)
+      expect(secondPayload.environment.backtrace_paths_matched).toEqual("1")
     })
   })
 

Diff for: packages/javascript/src/__tests__/span.test.ts

@@ -71,7 +71,7 @@ describe("Span", () => {
       ].join("\n")
 
       span.setError(error)
-      span.cleanBacktracePath(new RegExp("/assets/(app/.*)$"))
+      span.cleanBacktracePath([new RegExp("/assets/(app/.*)$")])
 
       const backtrace = span.serialize().error.backtrace
       expect(backtrace).toEqual([
@@ -97,7 +97,7 @@ describe("Span", () => {
       ].join("\n")
 
       span.setError(error)
-      span.cleanBacktracePath(new RegExp("/assets/(app/.*)$"))
+      span.cleanBacktracePath([new RegExp("/assets/(app/.*)$")])
 
       const backtrace = span.serialize().error.backtrace
       expect(backtrace).toEqual([
@@ -123,9 +123,9 @@ describe("Span", () => {
       ].join("\n")
 
       span.setError(error)
-      span.cleanBacktracePath(
+      span.cleanBacktracePath([
         new RegExp(".*/(assets/)(?:[0-9a-f]{16}/)?(app/.*)$")
-      )
+      ])
 
       const backtrace = span.serialize().error.backtrace
       expect(backtrace).toEqual([
@@ -181,7 +181,7 @@ describe("Span", () => {
       // empty string.
       //
       // This should result in the line not being modified.
-      span.cleanBacktracePath(new RegExp(".*"))
+      span.cleanBacktracePath([new RegExp(".*")])
 
       const backtrace = span.serialize().error.backtrace
       expect(backtrace).toEqual([
@@ -203,7 +203,7 @@ describe("Span", () => {
       // empty string.
       //
       // This should result in the line not being modified.
-      span.cleanBacktracePath(new RegExp(".*(z*)$"))
+      span.cleanBacktracePath([new RegExp(".*(z*)$")])
 
       const backtrace = span.serialize().error.backtrace
       expect(backtrace).toEqual([
@@ -220,7 +220,7 @@ describe("Span", () => {
       ].join("\n")
 
       span.setError(error)
-      span.cleanBacktracePath(new RegExp("^pancakes/(.*)$"))
+      span.cleanBacktracePath([new RegExp("^pancakes/(.*)$")])
 
       const backtrace = span.serialize().error.backtrace
       expect(backtrace).toEqual([

Diff for: packages/javascript/src/index.ts

@@ -18,6 +18,7 @@ import { AppsignalOptions } from "./interfaces/options"
 export default class Appsignal implements JSClient {
   public VERSION = VERSION
   public ignored: RegExp[] = []
+  private matchBacktracePaths: RegExp[] = []
 
   private _dispatcher: Dispatcher
   private _options: AppsignalOptions
@@ -40,7 +41,13 @@ export default class Appsignal implements JSClient {
    * @param   {AppsignalOptions}  options        An object of options to configure the AppSignal client
    */
   constructor(options: AppsignalOptions) {
-    const { key = "", uri, revision, ignoreErrors } = options
+    const {
+      key = "",
+      uri,
+      revision,
+      ignoreErrors,
+      matchBacktracePaths
+    } = options
 
     // `revision` should be a `string`, but we attempt to
     // normalise to one anyway
@@ -65,6 +72,20 @@ export default class Appsignal implements JSClient {
     // property of a given `Error`
     if (ignoreErrors && Array.isArray(ignoreErrors)) {
       this.ignored = ignoreErrors
+        .filter(value => value instanceof RegExp)
+        .map(unglobalize)
+    }
+
+    if (matchBacktracePaths) {
+      if (Array.isArray(matchBacktracePaths)) {
+        this.matchBacktracePaths = matchBacktracePaths
+      } else {
+        this.matchBacktracePaths = [matchBacktracePaths]
+      }
+
+      this.matchBacktracePaths = this.matchBacktracePaths
+        .filter(value => value instanceof RegExp)
+        .map(unglobalize)
     }
 
     this._dispatcher = new Dispatcher(this._queue, this._api)
@@ -207,9 +228,7 @@ export default class Appsignal implements JSClient {
       compose(...this._hooks.overrides)(span)
     }
 
-    if (this._options.matchBacktracePaths) {
-      span.cleanBacktracePath(this._options.matchBacktracePaths)
-    }
+    span.cleanBacktracePath(this.matchBacktracePaths)
 
     if (Environment.supportsPromises()) {
       // clear breadcrumbs as they are now loaded into the span,
@@ -447,3 +466,11 @@ export default class Appsignal implements JSClient {
     return event
   }
 }
+
+// Returns a new `RegExp` object with the global flag removed.
+// This fixes issues where using global regexes repeatedly with `test` against
+// different strings will return unexpected results, due to the regex object
+// storing the location of its last match and resuming the search from there.
+function unglobalize(regexp: RegExp): RegExp {
+  return new RegExp(regexp.source, regexp.flags.replace("g", ""))
+}

Diff for: packages/javascript/src/span.ts

@@ -78,12 +78,8 @@ export class Span extends Serializable<JSSpanData> {
   // @private
   // Do not use this function directly. Instead, set the `matchBacktracePaths`
   // configuration option when initializing AppSignal.
-  public cleanBacktracePath(matchBacktracePaths: RegExp | RegExp[]): this {
-    if (matchBacktracePaths instanceof RegExp) {
-      matchBacktracePaths = [matchBacktracePaths]
-    }
-
-    if (!Array.isArray(matchBacktracePaths)) {
+  public cleanBacktracePath(matchBacktracePaths: RegExp[]): this {
+    if (matchBacktracePaths.length === 0) {
       return this
     }
 
@@ -100,10 +96,6 @@ export class Span extends Serializable<JSSpanData> {
       }
 
       for (const matcher of matchBacktracePaths as RegExp[]) {
-        if (!(matcher instanceof RegExp)) {
-          continue
-        }
-
         const match = path.match(matcher)
         if (!match || match.length < 2) {
           continue