From b4aac345b0a7cdec4d713c5be4daea37330b2b26 Mon Sep 17 00:00:00 2001 From: Gar Date: Tue, 14 Sep 2021 07:27:36 -0700 Subject: [PATCH] fix(config): user-agent properly shows ci The way we were flattening user-agent back into itself meant that any values that were dependent on other config items would never be seen, since we have to re-flatten the item for each one it depends on. We also weren't re-flattening the user-agent when setting workspaces or workspace, which were things that affected the final result. This does change the main config value of `user-agent` but not the flattened one. We are not using the main config value anywhere (which is correct). PR-URL: https://github.com/npm/cli/pull/3754 Credit: @wraithgar Close: #3754 Reviewed-by: @nlf --- lib/utils/config/definitions.js | 12 ++++++- smoke-tests/index.js | 40 ++++++++++++++++++++++- smoke-tests/server.js | 8 ++++- tap-snapshots/test/lib/config.js.test.cjs | 4 +-- test/lib/utils/config/definitions.js | 30 ++++++++++++++--- 5 files changed, 85 insertions(+), 9 deletions(-) diff --git a/lib/utils/config/definitions.js b/lib/utils/config/definitions.js index 092e0fc435cb4..009f60a7bce61 100644 --- a/lib/utils/config/definitions.js +++ b/lib/utils/config/definitions.js @@ -2053,10 +2053,14 @@ define('user-agent', { .replace(/\{workspaces\}/gi, inWorkspaces) .replace(/\{ci\}/gi, ciName ? `ci/${ciName}` : '') .trim() + + // We can't clobber the original or else subsequent flattening will fail + // (i.e. when we change the underlying config values) + // obj[key] = flatOptions.userAgent + // user-agent is a unique kind of config item that gets set from a template // and ends up translated. Because of this, the normal "should we set this // to process.env also doesn't work - obj[key] = flatOptions.userAgent process.env.npm_config_user_agent = flatOptions.userAgent }, }) @@ -2140,6 +2144,9 @@ define('workspace', { a workspace which does not yet exist, to create the folder and set it up as a brand new workspace within the project. `, + flatten: (key, obj, flatOptions) => { + definitions['user-agent'].flatten('user-agent', obj, flatOptions) + }, }) define('workspaces', { @@ -2151,6 +2158,9 @@ define('workspaces', { Enable running a command in the context of **all** the configured workspaces. `, + flatten: (key, obj, flatOptions) => { + definitions['user-agent'].flatten('user-agent', obj, flatOptions) + }, }) define('yes', { diff --git a/smoke-tests/index.js b/smoke-tests/index.js index 9235c8960a26a..5e2d5e071ae12 100644 --- a/smoke-tests/index.js +++ b/smoke-tests/index.js @@ -1,8 +1,9 @@ const fs = require('fs') const { promisify } = require('util') const execAsync = promisify(require('child_process').exec) -const { resolve } = require('path') +const { join, resolve } = require('path') const t = require('tap') +const rimraf = promisify(require('rimraf')) const normalizePath = path => path.replace(/[A-Z]:/, '').replace(/\\/g, '/') const cwd = normalizePath(process.cwd()) @@ -47,6 +48,43 @@ const exec = async cmd => { const readFile = filename => String(fs.readFileSync(resolve(localPrefix, filename))) +// this test must come first, its package.json will be destroyed and the one +// created in the next test (npm init) will create a new one that must be +// present for later tests +t.test('npm install sends correct user-agent', async t => { + const pkgPath = join(localPrefix, 'package.json') + const pkgContent = JSON.stringify({ + name: 'smoke-test-workspaces', + workspaces: ['packages/*'], + }) + fs.writeFileSync(pkgPath, pkgContent, { encoding: 'utf8' }) + + const wsRoot = join(localPrefix, 'packages') + fs.mkdirSync(wsRoot) + + const wsPath = join(wsRoot, 'foo') + fs.mkdirSync(wsPath) + + const wsPkgPath = join(wsPath, 'package.json') + const wsContent = JSON.stringify({ + name: 'foo', + }) + fs.writeFileSync(wsPkgPath, wsContent, { encoding: 'utf8' }) + t.teardown(async () => { + await rimraf(`${localPrefix}/*`) + }) + + const cmd = `${npmBin} install fail_reflect_user_agent` + await t.rejects(exec(cmd), { + stderr: /workspaces\/false/, + }, 'workspaces/false is present in output') + + const wsCmd = `${npmBin} install fail_reflect_user_agent --workspaces` + await t.rejects(exec(wsCmd), { + stderr: /workspaces\/true/, + }, 'workspaces/true is present in output') +}) + t.test('npm init', async t => { const cmd = `${npmBin} init -y` const cmdRes = await exec(cmd) diff --git a/smoke-tests/server.js b/smoke-tests/server.js index b864114af64a8..e0ac0c94eb5ab 100644 --- a/smoke-tests/server.js +++ b/smoke-tests/server.js @@ -1,5 +1,5 @@ /* istanbul ignore file */ -const {join, dirname} = require('path') +const {join, dirname, basename} = require('path') const {existsSync, readFileSync, writeFileSync} = require('fs') const PORT = 12345 + (+process.env.TAP_CHILD_ID || 0) const http = require('http') @@ -150,6 +150,12 @@ const startServer = () => new Promise((res, rej) => { } const f = join(__dirname, 'content', join('/', req.url.replace(/@/, '').replace(/%2f/i, '/'))) + // a magic package that causes us to return an error that will be logged + if (basename(f) === 'fail_reflect_user_agent') { + res.setHeader('npm-notice', req.headers['user-agent']) + res.writeHead(404) + return res.end() + } const isCorgi = req.headers.accept.includes('application/vnd.npm.install-v1+json') const file = f + ( isCorgi && existsSync(`${f}.min.json`) ? '.min.json' diff --git a/tap-snapshots/test/lib/config.js.test.cjs b/tap-snapshots/test/lib/config.js.test.cjs index 817f3c173f19e..b21b75cd25e11 100644 --- a/tap-snapshots/test/lib/config.js.test.cjs +++ b/tap-snapshots/test/lib/config.js.test.cjs @@ -146,7 +146,7 @@ exports[`test/lib/config.js TAP config list --json > output matches snapshot 1`] "unicode": false, "update-notifier": true, "usage": false, - "user-agent": "npm/{NPM-VERSION} node/{NODE-VERSION} {PLATFORM} {ARCH} workspaces/false", + "user-agent": "npm/{npm-version} node/{node-version} {platform} {arch} workspaces/{workspaces} {ci}", "version": false, "versions": false, "viewer": "{VIEWER}", @@ -296,7 +296,7 @@ umask = 0 unicode = false update-notifier = true usage = false -user-agent = "npm/{NPM-VERSION} node/{NODE-VERSION} {PLATFORM} {ARCH} workspaces/false" +user-agent = "npm/{npm-version} node/{node-version} {platform} {arch} workspaces/{workspaces} {ci}" ; userconfig = "{HOME}/.npmrc" ; overridden by cli version = false versions = false diff --git a/test/lib/utils/config/definitions.js b/test/lib/utils/config/definitions.js index 65193020d050c..88993303b539c 100644 --- a/test/lib/utils/config/definitions.js +++ b/test/lib/utils/config/definitions.js @@ -747,7 +747,7 @@ t.test('user-agent', t => { definitions['user-agent'].flatten('user-agent', obj, flat) t.equal(flat.userAgent, expectNoCI) t.equal(process.env.npm_config_user_agent, flat.userAgent, 'npm_user_config environment is set') - t.equal(obj['user-agent'], flat.userAgent, 'config user-agent template is translated') + t.not(obj['user-agent'], flat.userAgent, 'config user-agent template is not translated') obj['ci-name'] = 'foo' obj['user-agent'] = definitions['user-agent'].default @@ -755,7 +755,7 @@ t.test('user-agent', t => { definitions['user-agent'].flatten('user-agent', obj, flat) t.equal(flat.userAgent, expectCI) t.equal(process.env.npm_config_user_agent, flat.userAgent, 'npm_user_config environment is set') - t.equal(obj['user-agent'], flat.userAgent, 'config user-agent template is translated') + t.not(obj['user-agent'], flat.userAgent, 'config user-agent template is not translated') delete obj['ci-name'] obj.workspaces = true @@ -764,7 +764,7 @@ t.test('user-agent', t => { definitions['user-agent'].flatten('user-agent', obj, flat) t.equal(flat.userAgent, expectWorkspaces) t.equal(process.env.npm_config_user_agent, flat.userAgent, 'npm_user_config environment is set') - t.equal(obj['user-agent'], flat.userAgent, 'config user-agent template is translated') + t.not(obj['user-agent'], flat.userAgent, 'config user-agent template is not translated') delete obj.workspaces obj.workspace = ['foo'] @@ -772,7 +772,7 @@ t.test('user-agent', t => { definitions['user-agent'].flatten('user-agent', obj, flat) t.equal(flat.userAgent, expectWorkspaces) t.equal(process.env.npm_config_user_agent, flat.userAgent, 'npm_user_config environment is set') - t.equal(obj['user-agent'], flat.userAgent, 'config user-agent template is translated') + t.not(obj['user-agent'], flat.userAgent, 'config user-agent template is not translated') t.end() }) @@ -853,3 +853,25 @@ t.test('package-lock-only', t => { t.strictSame(flat, { packageLock: false, packageLockOnly: false }) t.end() }) + +t.test('workspaces', t => { + const obj = { + workspaces: true, + 'user-agent': definitions['user-agent'].default, + } + const flat = {} + definitions.workspaces.flatten('workspaces', obj, flat) + t.match(flat.userAgent, /workspaces\/true/) + t.end() +}) + +t.test('workspace', t => { + const obj = { + workspace: ['workspace-a'], + 'user-agent': definitions['user-agent'].default, + } + const flat = {} + definitions.workspace.flatten('workspaces', obj, flat) + t.match(flat.userAgent, /workspaces\/true/) + t.end() +})