From b19aacb925e44ccce1d99a713ad12bdf7b4a008f Mon Sep 17 00:00:00 2001 From: Gar Date: Tue, 7 May 2024 13:39:47 -0700 Subject: [PATCH] fix: isolate full and corgi packuments in packumentCache (#369) --- lib/registry.js | 13 +++--- test/registry.js | 114 +++++++++++++++++++++++++++++------------------ 2 files changed, 78 insertions(+), 49 deletions(-) diff --git a/lib/registry.js b/lib/registry.js index f2b5a8a..56887e3 100644 --- a/lib/registry.js +++ b/lib/registry.js @@ -20,6 +20,7 @@ const fullDoc = 'application/json' const MISSING_TIME_CUTOFF = '2015-01-01T00:00:00.000Z' class RegistryFetcher extends Fetcher { + #cacheKey constructor (spec, opts) { super(spec, opts) @@ -32,8 +33,8 @@ class RegistryFetcher extends Fetcher { this.packumentCache = this.opts.packumentCache || null this.registry = fetch.pickRegistry(spec, opts) - this.packumentUrl = removeTrailingSlashes(this.registry) + '/' + - this.spec.escapedName + this.packumentUrl = `${removeTrailingSlashes(this.registry)}/${this.spec.escapedName}` + this.#cacheKey = `${this.fullMetadata ? 'full' : 'corgi'}:${this.packumentUrl}` const parsed = new URL(this.registry) const regKey = `//${parsed.host}${parsed.pathname}` @@ -78,8 +79,8 @@ class RegistryFetcher extends Fetcher { // note this might be either an in-flight promise for a request, // or the actual packument, but we never want to make more than // one request at a time for the same thing regardless. - if (this.packumentCache?.has(this.packumentUrl)) { - return this.packumentCache.get(this.packumentUrl) + if (this.packumentCache?.has(this.#cacheKey)) { + return this.packumentCache.get(this.#cacheKey) } // npm-registry-fetch the packument @@ -99,10 +100,10 @@ class RegistryFetcher extends Fetcher { if (contentLength) { packument._contentLength = Number(contentLength) } - this.packumentCache?.set(this.packumentUrl, packument) + this.packumentCache?.set(this.#cacheKey, packument) return packument } catch (err) { - this.packumentCache?.delete(this.packumentUrl) + this.packumentCache?.delete(this.#cacheKey) if (err.code !== 'E404' || this.fullMetadata) { throw err } diff --git a/test/registry.js b/test/registry.js index 99c1218..fe1f582 100644 --- a/test/registry.js +++ b/test/registry.js @@ -22,57 +22,62 @@ const MockedRegistryFetcher = t.mock('../lib/registry.js', { const port = 18000 + (+process.env.TAP_CHILD_ID || 0) const me = t.testdir() +const registry = `http://localhost:${port}/` +const cache = me + '/cache' -t.test('start mock registry', { bail: true }, t => { - mr({ - port, - mocks: { - get: { - '/no-integrity/-/no-integrity-1.2.3.tgz': [ - 200, - `${__dirname}/fixtures/abbrev-1.1.1.tgz`, - ], +let mockServer +t.before(() => { + return new Promise((resolve, reject) => { + mr({ + port, + mocks: { + get: { + '/no-integrity/-/no-integrity-1.2.3.tgz': [ + 200, + `${__dirname}/fixtures/abbrev-1.1.1.tgz`, + ], + }, }, - }, - plugin (server) { - server.get('/thing-is-not-here').many().reply(404, { error: 'not found' }) - server.get('/no-tarball').many().reply(200, { - name: 'no-tarball', - 'dist-tags': { latest: '1.2.3' }, - versions: { - '1.2.3': { - name: 'no-tarball', - version: '1.2.3', + plugin (server) { + server.get('/thing-is-not-here').many().reply(404, { error: 'not found' }) + server.get('/no-tarball').many().reply(200, { + name: 'no-tarball', + 'dist-tags': { latest: '1.2.3' }, + versions: { + '1.2.3': { + name: 'no-tarball', + version: '1.2.3', + }, }, - }, - }) - server.get('/no-integrity').many().reply(200, { - name: 'no-integrity', - 'dist-tags': { latest: '1.2.3' }, - versions: { - '1.2.3': { - name: 'no-integrity', - version: '1.2.3', - dist: { - tarball: `${registry}no-integrity/-/no-integrity-1.2.3.tgz`, + }) + server.get('/no-integrity').many().reply(200, { + name: 'no-integrity', + 'dist-tags': { latest: '1.2.3' }, + versions: { + '1.2.3': { + name: 'no-integrity', + version: '1.2.3', + dist: { + tarball: `${registry}no-integrity/-/no-integrity-1.2.3.tgz`, + }, }, }, - }, - }) - }, - }, (er, s) => { - if (er) { - throw er - } - - t.parent.teardown(() => s.close()) - t.end() + }) + }, + }, (err, s) => { + if (err) { + return reject(err) + } + mockServer = s + resolve() + }) }) }) -const registry = `http://localhost:${port}/` -const cache = me + '/cache' +t.teardown(() => { + mockServer?.close() +}) t.test('underscore, no tag or version', t => { const f = new RegistryFetcher('underscore', { registry, cache, fullReadJson: true }) @@ -1207,11 +1212,34 @@ t.test('packument that has been cached', async t => { }, }, } - const packumentCache = new Map([[packumentUrl, packument]]) + const packumentCache = new Map([[`corgi:${packumentUrl}`, packument]]) const f = new RegistryFetcher('asdf@1.2', { registry, cache, packumentCache }) t.equal(await f.packument(), packument, 'got cached packument') }) +t.test('corgi packument is not cached as full packument', async t => { + const packumentUrl = `${registry}underscore` + const packument = { + name: 'underscore', + versions: { + '1.5.1': { + cached: true, + name: 'underscore', + version: '1.5.1', + }, + }, + } + const packumentCache = new Map([[`corgi:${packumentUrl}`, packument]]) + const f = new RegistryFetcher('underscore', { + packumentCache, + registry, + cache, + fullMetadata: true, + }) + t.notEqual(await f.packument(), packument, 'did not get cached packument') + t.ok(packumentCache.has(`full:${packumentUrl}`), 'full packument is also now cached') +}) + t.test('packument that falls back to fullMetadata', t => { const http = require('http') const packument = {