fix: Fixed issue with CJS being imported as ESM (#2168)

.github/workflows/ci-workflow.yml

@@ -94,6 +94,9 @@ jobs:
     if: needs.skip_if_release.outputs.should_skip != 'true'
     runs-on: ubuntu-latest
 
+    env:
+      NODE_NO_WARNINGS: 1
+
     strategy:
       fail-fast: false
       matrix:
@@ -111,11 +114,18 @@ jobs:
       run: npm run services
     - name: Run Integration Tests
       run: npm run integration
+    - name: Run ESM Integration Tests
+      run: npm run integration:esm
     - name: Archive Integration Test Coverage
       uses: actions/upload-artifact@v3
       with:
         name: integration-tests-${{ matrix.node-version }}
         path: ./coverage/integration/lcov.info
+    - name: Archive Integration (ESM) Test Coverage
+      uses: actions/upload-artifact@v3
+      with:
+        name: integration-tests-${{ matrix.node-version }}
+        path: ./coverage/integration-esm/lcov.info
 
   versioned-internal:
     needs: skip_if_release

lib/instrumentation-tracker.js

@@ -217,6 +217,23 @@ class InstrumentationTracker {
       }
     }
   }
+
+  /**
+   * Given a full absolute path to a module, look up the instrumentation
+   * associated with it and return the name for that instrumentation.
+   *
+   * @param {string} modulePath The path to the module being instrumented.
+   *
+   * @returns {string|undefined} The name of the module.
+   */
+  simpleNameFromPath(modulePath) {
+    for (const [key, items] of this.#tracked.entries()) {
+      const instrumentation = items.find((i) => i.instrumentation.absolutePath === modulePath)
+      if (instrumentation) {
+        return key
+      }
+    }
+  }
 }
 
 module.exports = InstrumentationTracker

lib/instrumentation/fastify.js

@@ -95,7 +95,7 @@ module.exports = function initialize(agent, fastify, moduleName, shim) {
    */
   const wrappedExport = shim.wrapExport(fastify, function wrapFastifyModule(shim, fn) {
     return function wrappedFastifyModule() {
-      // call original function get get fastify object (which is singleton-ish)
+      // call original function to get the fastify object (which is singleton-ish)
       const fastifyForWrapping = fn.apply(this, arguments)
 
       setupRouteHandler(shim, fastifyForWrapping)

lib/shim/shim.js

@@ -563,6 +563,13 @@ function wrapClass(nodule, properties, spec, args) {
  * @returns {*} The return value from `spec`.
  */
 function wrapExport(nodule, spec) {
+  if (nodule[symbols.nrEsmProxy] === true) {
+    // A CJS module has been imported as ESM through import-in-the-middle. This
+    // means that `nodule` is set to an instance of our proxy. What we actually
+    // want is the thing to be instrumented. We assume it is the "default"
+    // export.
+    nodule = nodule.default
+  }
   return (this._toExport = this.wrap(nodule, null, spec))
 }
 

lib/shimmer.js

@@ -14,6 +14,7 @@ const shims = require('./shim')
 const { Hook } = require('@newrelic/ritm')
 const IitmHook = require('import-in-the-middle')
 const { nrEsmProxy } = require('./symbols')
+const isAbsolutePath = require('./util/is-absolute-path')
 const InstrumentationDescriptor = require('./instrumentation-descriptor')
 const InstrumentationTracker = require('./instrumentation-tracker')
 let pkgsToHook = []
@@ -463,6 +464,12 @@ const shimmer = (module.exports = {
       moduleName = 'pg'
     }
 
+    if (isAbsolutePath(moduleName) === true) {
+      // moduleName is an absolute path to a module. So we need to look up
+      // the simple name from the registered instrumentations.
+      return this.registeredInstrumentations.simpleNameFromPath(moduleName)
+    }
+
     return moduleName
   },
 
@@ -733,6 +740,16 @@ function _postLoad(agent, nodule, name, resolvedName, esmResolver) {
 
   // Check if this is a known instrumentation and then run it.
   if (hasPostLoadInstrumentation) {
+    if (resolvedName === undefined && isAbsolutePath(name) === true) {
+      // `resolvedName` comes from the `basedir` returned by the `Hook`
+      // function from import-in-the-middle or require-in-the-middle. At least
+      // with IITM, if the path string does not include a `node_modules` then
+      // `basedir` will be `undefined`. But we need it for our instrumentation
+      // to work. We'll only reach this situation if the module being
+      // instrumented has an `absolutePath` defined. So we detect that and
+      // assign appropriately.
+      resolvedName = name
+    }
     shimmer.registeredInstrumentations.setResolvedName(simpleName, resolvedName)
     logger.trace('Instrumenting %s with onRequire (module loaded) hook.', name)
     return instrumentPostLoad(agent, nodule, simpleName, resolvedName, esmResolver)

lib/util/is-absolute-path.js

@@ -0,0 +1,28 @@
+/*
+ * Copyright 2024 New Relic Corporation. All rights reserved.
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+'use strict'
+
+/**
+ * Determines if a given string represents an absolute path to a module.
+ *
+ * @param {string} target Path to a module.
+ *
+ * @returns {boolean} True if it is an absolute path.
+ */
+module.exports = function isAbsolutePath(target) {
+  const leadChar = target.slice(0, 1)
+  if (leadChar !== '.' && leadChar !== '/') {
+    return false
+  }
+
+  const suffix = target.slice(-4)
+  /* eslint-disable-next-line */
+  if (suffix.slice(-3) !== '.js' && suffix !== '.cjs' && suffix !== '.mjs') {
+    return false
+  }
+
+  return true
+}

package.json

@@ -157,6 +157,7 @@
     "docker-env": "./bin/docker-env-vars.sh",
     "docs": "npm ci && jsdoc -c ./jsdoc-conf.json --private -r .",
     "integration": "npm run prepare-test && npm run sub-install && time c8 -o ./coverage/integration tap --test-regex='(\\/|^test\\/integration\\/.*\\.tap\\.js)$' --timeout=600 --no-coverage --reporter classic",
+    "integration:esm": "time c8 -o ./coverage/integration-esm tap --node-arg='--loader=./esm-loader.mjs' --test-regex='(test\\/integration\\/.*\\.tap\\.mjs)$' --timeout=600 --no-coverage --reporter classic",
     "prepare-test": "npm run ssl && npm run docker-env",
     "lint": "eslint ./*.{js,mjs} lib test bin examples",
     "lint:fix": "eslint --fix, ./*.{js,mjs} lib test bin examples",

test/integration/issue-2155/foo.cjs

@@ -0,0 +1,27 @@
+/*
+ * Copyright 2020 New Relic Corporation. All rights reserved.
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+'use strict'
+
+// A fake module that will be imported as ESM. Basically, the issue is that
+// CJS imported as ESM needs its exports proxied and our `shim.wrapExport`
+// needs to recognize the "original" export in order to pass it in to the
+// instrumentation.
+
+function foo() {
+  return Object.create({
+    name() {
+      return 'foo'
+    }
+  })
+}
+
+// This triplet export replicates they way Fastify solves the CJS utilized in
+// ESM issue. It makes it possible to `import foo from './foo.cjs'` or to
+// `import { foo } from './foo.cjs'`. It also allows us to replicate the
+// issue at hand.
+module.exports = foo
+module.exports.default = foo
+module.exports.foo = foo

test/integration/issue-2155/test.tap.mjs

@@ -0,0 +1,61 @@
+/*
+ * Copyright 2024 New Relic Corporation. All rights reserved.
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+import tap from 'tap'
+import crypto from 'crypto'
+import path from 'path'
+import url from 'url'
+import helper from '../../lib/agent_helper.js'
+import shimmer from '../../../lib/shimmer.js'
+import InstrumentationDescriptor from '../../../lib/instrumentation-descriptor.js'
+
+let modPath
+if (import.meta.dirname) {
+  modPath = path.join(import.meta.dirname, 'foo.cjs')
+} else {
+  modPath = path.join(path.dirname(url.fileURLToPath(import.meta.url)), 'foo.cjs')
+}
+
+function instrumentation(shim, resolvedModule) {
+  shim.wrapExport(resolvedModule, function wrapModule(shim, fn) {
+    return function wrappedModule() {
+      // `fn` _should_ be the `foo()` function exported by the module.
+      // If it is anything else, i.e. the proxy object, then we have an error
+      // in our handling of CJS modules as ESM.
+      const foo = fn.apply(this, arguments)
+      const _name = foo.name
+      foo.name = () => {
+        const value = _name.call(foo)
+        return `wrapped: ${value}`
+      }
+      return foo
+    }
+  })
+}
+
+tap.beforeEach(async (t) => {
+  shimmer.registerInstrumentation({
+    type: InstrumentationDescriptor.TYPE_GENERIC,
+    moduleName: 'foo',
+    absolutePath: modPath,
+    onRequire: instrumentation
+  })
+
+  const agent = helper.instrumentMockedAgent()
+  t.context.agent = agent
+
+  const { default: foo } = await import('./foo.cjs?v=' + crypto.randomBytes(16).toString('hex'))
+  t.context.mod = foo
+})
+
+tap.afterEach((t) => {
+  helper.unloadAgent(t.context.agent)
+})
+
+tap.test('CJS imported as ESM gets wrapped correctly', async (t) => {
+  const { mod } = t.context
+  const instance = mod()
+  t.equal(instance.name(), 'wrapped: foo')
+})

test/unit/is-absolute-path.test.js

@@ -0,0 +1,24 @@
+/*
+ * Copyright 2024 New Relic Corporation. All rights reserved.
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+'use strict'
+
+const tap = require('tap')
+const isAbsolutePath = require('../../lib/util/is-absolute-path')
+
+tap.test('verifies paths correctly', async (t) => {
+  const tests = [
+    ['./foo/bar.js', true],
+    ['/foo/bar.cjs', true],
+    ['/foo.mjs', true],
+    ['/foo.smj', false],
+    ['foo', false],
+    ['foo.js', false]
+  ]
+
+  for (const [input, expected] of tests) {
+    t.equal(isAbsolutePath(input), expected)
+  }
+})

test/versioned/esm-package/parse-json-instrumentation.mjs

@@ -3,8 +3,6 @@
  * SPDX-License-Identifier: Apache-2.0
  */
 
-'use-strict'
-
 export default function initialize(shim, parseJson) {
   shim.wrap(parseJson, 'default', function wrappedParseJsonLib(_shim, orig) {
     return function wrappedParseJsonFunc() {

test/versioned/winston-esm/winston.tap.mjs

@@ -6,13 +6,26 @@
 import tap from 'tap'
 import { randomUUID } from 'node:crypto'
 import fs from 'node:fs/promises'
+import path from 'node:path'
+import url from 'node:url'
 import semver from 'semver'
 import helper from '../../lib/agent_helper.js'
 import names from '../../../lib/metrics/names.js'
 import { Sink } from './common.mjs'
 
 const { LOGGING } = names
-const winstonPkg = JSON.parse(await fs.readFile('./node_modules/winston/package.json'))
+let pkgPath
+if (import.meta.dirname) {
+  pkgPath = path.join(import.meta.dirname, 'node_modules', 'winston', 'package.json')
+} else {
+  pkgPath = path.join(
+    path.dirname(url.fileURLToPath(import.meta.url)),
+    'node_modules',
+    'winston',
+    'package.json'
+  )
+}
+const winstonPkg = JSON.parse(await fs.readFile(pkgPath))
 
 tap.skip = true