test: share the three-install request-log cases between pnpm v11 and v12 (#17063)

`pnpr/.fixtures/request-logs/metadata-three-installs.json` lists the
three-install request-log cases: settings, arguments, and the metadata
requests of each install. The v12 CLI suite and a new v11 e2e test both
assert it, so a difference in what the two stacks request fails one of
them. The v11 test records requests through a proxy in front of the mock
registry. Tarballs bypass that proxy, so the table leaves them out and
the v12 test asserts them on its own.

The table found one difference: v12's `to_http_date` read only RFC 3339
timestamps and HTTP dates, so a packument whose `modified` is a bare date
revalidated with `If-None-Match` alone. pnpm v11 converts the same value
with `new Date(...).toUTCString()` and sends both validators. v12 now
reads a bare date as midnight UTC too.

`httpdate::fmt_http_date` panics outside the years 1970 to 9999, and a
registry's `modified` reached it unchecked. `to_http_date` now drops
such a value.

Part of pnpm/tasks#146 and pnpm/tasks#141.

.changeset/if-modified-since-bare-date.md

@@ -0,0 +1,5 @@
+---
+"pacquet": patch
+---
+
+When pnpm revalidates cached registry metadata, it now also sends `If-Modified-Since` if the registry's modification time is a date without a time.

.changeset/if-modified-since-before-1970.md

@@ -0,0 +1,5 @@
+---
+"pacquet": patch
+---
+
+pnpm no longer crashes when it revalidates cached registry metadata whose modification time is before 1970.

pnpm/crates/cli/tests/suite/metadata_request_log.rs

@@ -1,6 +1,7 @@
 //! The registry requests of three installs that resolve the same project
-//! against an unchanged registry, sharing one metadata cache. Only the first
-//! install may download a body.
+//! against an unchanged registry, sharing one metadata cache. The cases and
+//! their metadata request logs are shared with pnpm v11, see the table's
+//! `description`.
 
 use assert_cmd::prelude::*;
 use pnpm_testing_utils::{
@@ -8,15 +9,54 @@ use pnpm_testing_utils::{
     registry::RecordedRequest,
 };
 use pretty_assertions::assert_eq;
+use serde_json::Value;
 use std::{collections::HashMap, fs, io};
 
-const PKG: &str = "/@pnpm.e2e%2Fpkg-with-1-dep";
-const DEP: &str = "/@pnpm.e2e%2Fdep-of-pkg-with-1-dep";
-const PKG_TARBALL: &str = "/@pnpm.e2e/pkg-with-1-dep/-/pkg-with-1-dep-100.1.0.tgz";
-const DEP_TARBALL: &str = "/@pnpm.e2e/dep-of-pkg-with-1-dep/-/dep-of-pkg-with-1-dep-100.1.0.tgz";
+const CASE_TABLE: &str =
+    include_str!("../../../../../pnpr/.fixtures/request-logs/metadata-three-installs.json");
 
-/// Each install's requests, sorted: concurrent fetches finish in any order.
-fn three_installs(settings: &str, args: &[&str]) -> [Vec<String>; 3] {
+const PKG_TARBALL: &str = "GET /@pnpm.e2e/pkg-with-1-dep/-/pkg-with-1-dep-100.1.0.tgz 200";
+const DEP_TARBALL: &str =
+    "GET /@pnpm.e2e/dep-of-pkg-with-1-dep/-/dep-of-pkg-with-1-dep-100.1.0.tgz 200";
+
+/// Install 1 also downloads both tarballs, which the store keeps for installs
+/// 2 and 3. The table leaves tarballs out: in pnpm v11's test they bypass the
+/// recording proxy.
+#[test]
+fn every_case_of_the_shared_table() {
+    let table: Value = serde_json::from_str(CASE_TABLE).expect("parse the case table");
+    for case in table["cases"].as_array().expect("cases") {
+        let name = case["name"].as_str().expect("case name");
+        let settings: Vec<String> = case["settings"]
+            .as_object()
+            .expect("case settings")
+            .iter()
+            .map(|(key, value)| format!("{key}: {value}"))
+            .collect();
+        let args: Vec<&str> = case["args"]
+            .as_array()
+            .expect("case args")
+            .iter()
+            .map(|arg| arg.as_str().expect("case arg"))
+            .collect();
+        let expected: Vec<Vec<String>> =
+            serde_json::from_value(case["installs"].clone()).expect("case installs");
+
+        let (metadata, tarballs) = three_installs(&table["dependencies"], &settings, &args);
+        assert_eq!(metadata, expected, "{name}");
+        assert_eq!(tarballs, [vec![DEP_TARBALL, PKG_TARBALL], vec![], vec![]], "{name}");
+    }
+}
+
+type InstallLogs = Vec<Vec<String>>;
+
+/// Each install's metadata requests as the table spells them, and its
+/// tarball paths, both sorted: concurrent fetches finish in any order.
+fn three_installs(
+    dependencies: &Value,
+    settings: &[String],
+    args: &[&str],
+) -> (InstallLogs, InstallLogs) {
     let CommandTempCwd {
         mut pacquet,
         root,
@@ -25,40 +65,57 @@ fn three_installs(settings: &str, args: &[&str]) -> [Vec<String>; 3] {
         ..
     } = CommandTempCwd::init().add_recording_mocked_registry();
     let AddMockedRegistry { mock_instance, .. } = npmrc_info;
-    let manifest =
-        serde_json::json!({ "dependencies": { "@pnpm.e2e/pkg-with-1-dep": "^100.0.0" } });
+    let manifest = serde_json::json!({ "dependencies": dependencies });
     fs::write(workspace.join("package.json"), manifest.to_string()).expect("write package.json");
     let workspace_yaml = workspace.join("pnpm-workspace.yaml");
     let yaml = fs::read_to_string(&workspace_yaml).expect("read pnpm-workspace.yaml");
-    fs::write(&workspace_yaml, format!("{yaml}{settings}")).expect("write pnpm-workspace.yaml");
+    fs::write(&workspace_yaml, format!("{yaml}{}\n", settings.join("\n")))
+        .expect("write pnpm-workspace.yaml");
     pacquet.arg("install").args(args);
 
-    let installs = [(); 3].map(|()| {
-        // Remove what the last install wrote, so this one resolves again.
-        remove_if_present(fs::remove_dir_all(workspace.join("node_modules")));
-        remove_if_present(fs::remove_file(workspace.join("pnpm-lock.yaml")));
-        pacquet.assert().success();
-        mock_instance.take_requests()
-    });
+    let installs: Vec<Vec<RecordedRequest>> = (0..3)
+        .map(|_| {
+            // Remove what the last install wrote, so this one resolves again.
+            remove_if_present(fs::remove_dir_all(workspace.join("node_modules")));
+            remove_if_present(fs::remove_file(workspace.join("pnpm-lock.yaml")));
+            pacquet.assert().success();
+            mock_instance.take_requests()
+        })
+        .collect();
     drop(root);
 
-    let etags: HashMap<(String, &str), String> = installs
+    let etags: HashMap<(&str, &str), &str> = installs
         .iter()
         .flatten()
         .filter(|request| request.status == 200)
         .filter_map(|request| {
-            let etag = request.etag.clone()?;
-            Some(((request.path.clone(), document(request)), etag))
+            Some(((request.path.as_str(), document(request)), request.etag.as_deref()?))
         })
         .collect();
-    installs.map(|requests| {
+    let log_of = |requests: &Vec<RecordedRequest>, tarballs: bool| -> Vec<String> {
         let mut log: Vec<String> = requests
             .iter()
-            .map(|request| describe(request, &etags))
+            .filter(|request| is_tarball(request) == tarballs)
+            .map(|request| {
+                if tarballs {
+                    format!("{} {} {}", request.method, request.path, request.status)
+                } else {
+                    describe(request, &etags)
+                }
+            })
             .collect();
         log.sort();
         log
-    })
+    };
+    let metadata = installs
+        .iter()
+        .map(|requests| log_of(requests, false))
+        .collect();
+    let tarballs = installs
+        .iter()
+        .map(|requests| log_of(requests, true))
+        .collect();
+    (metadata, tarballs)
 }
 
 fn remove_if_present(result: io::Result<()>) {
@@ -67,6 +124,10 @@ fn remove_if_present(result: io::Result<()>) {
     }
 }
 
+fn is_tarball(request: &RecordedRequest) -> bool {
+    request.path.ends_with(".tgz")
+}
+
 fn document(request: &RecordedRequest) -> &'static str {
     match request.accept.as_deref() {
         Some(accept) if accept.starts_with("application/vnd.npm.install-v1+json") => "abbreviated",
@@ -74,10 +135,9 @@ fn document(request: &RecordedRequest) -> &'static str {
     }
 }
 
-/// A metadata request reads `etag` when it sends the `ETag` this registry
-/// gave the same representation, `other-etag` for any other value, and
-/// `since` when it sends `If-Modified-Since`.
-fn describe(request: &RecordedRequest, etags: &HashMap<(String, &str), String>) -> String {
+/// A request sending some other `ETag` reads `other-etag`, which no case
+/// expects.
+fn describe(request: &RecordedRequest, etags: &HashMap<(&str, &str), &str>) -> String {
     let RecordedRequest {
         method,
         path,
@@ -86,13 +146,10 @@ fn describe(request: &RecordedRequest, etags: &HashMap<(String, &str), String>)
         status,
         ..
     } = request;
-    if path.ends_with(".tgz") {
-        return format!("{method} {path} {status}");
-    }
     let document = document(request);
     let mut validators = String::new();
     if let Some(sent) = if_none_match {
-        let own = etags.get(&(path.clone(), document)) == Some(sent);
+        let own = etags.get(&(path.as_str(), document)) == Some(&sent.as_str());
         validators.push_str(if own { " etag" } else { " other-etag" });
     }
     if if_modified_since.is_some() {
@@ -100,55 +157,3 @@ fn describe(request: &RecordedRequest, etags: &HashMap<(String, &str), String>)
     }
     format!("{method} {path} {document}{validators} {status}")
 }
-
-fn first_install(document: &str) -> Vec<String> {
-    let mut log = vec![
-        format!("GET {DEP} {document} 200"),
-        format!("GET {DEP_TARBALL} 200"),
-        format!("GET {PKG} {document} 200"),
-        format!("GET {PKG_TARBALL} 200"),
-    ];
-    log.sort();
-    log
-}
-
-/// `request` is the document kind and the validators, as [`describe`]
-/// spells them.
-fn revalidation(request: &str) -> Vec<String> {
-    vec![format!("GET {DEP} {request} 304"), format!("GET {PKG} {request} 304")]
-}
-
-#[test]
-fn abbreviated_metadata_is_revalidated_with_its_etag() {
-    let [first, second, third] = three_installs("minimumReleaseAge: 0\n", &[]);
-    assert_eq!(first, first_install("abbreviated"));
-    assert_eq!(second, revalidation("abbreviated etag"));
-    assert_eq!(third, revalidation("abbreviated etag"));
-}
-
-#[test]
-fn full_metadata_is_revalidated_with_its_etag() {
-    let [first, second, third] =
-        three_installs("minimumReleaseAge: 0\ntrustPolicy: no-downgrade\n", &[]);
-    assert_eq!(first, first_install("full"));
-    assert_eq!(second, revalidation("full etag since"));
-    assert_eq!(third, revalidation("full etag since"));
-}
-
-/// A mirror written after the `minimumReleaseAge` cutoff cannot be missing a
-/// mature version, so it answers without a request.
-#[test]
-fn a_mirror_written_after_the_release_age_cutoff_answers_without_a_request() {
-    let [first, second, third] = three_installs("minimumReleaseAge: 1440\n", &[]);
-    assert_eq!(first, first_install("abbreviated"));
-    assert_eq!(second, Vec::<String>::new());
-    assert_eq!(third, Vec::<String>::new());
-}
-
-#[test]
-fn prefer_offline_answers_from_the_mirror_without_a_request() {
-    let [first, second, third] = three_installs("minimumReleaseAge: 0\n", &["--prefer-offline"]);
-    assert_eq!(first, first_install("abbreviated"));
-    assert_eq!(second, Vec::<String>::new());
-    assert_eq!(third, Vec::<String>::new());
-}

pnpm/crates/resolving-npm-resolver/src/fetch_full_metadata.rs

@@ -276,14 +276,23 @@ async fn send_once<'a>(
 /// Convert a stored `modified` value — the packument's ISO-8601
 /// `time.modified` — to the HTTP-date form `If-Modified-Since` requires
 /// (RFC 9110 §8.8.3), the same conversion the TypeScript CLI applies with
-/// `toUTCString()`. A value already in HTTP-date form is kept. `None` when
-/// the value parses as neither, so it is dropped instead of sent — a
-/// recipient must ignore a malformed `If-Modified-Since` anyway.
+/// `toUTCString()`. A date without a time is midnight UTC, as JavaScript's
+/// `Date` reads it. A value already in HTTP-date form is kept. `None` when
+/// the value parses as none of these, or names a time outside the years
+/// 1970 to 9999 that an HTTP date can carry, so it is dropped instead of
+/// sent — a recipient must ignore a malformed `If-Modified-Since` anyway.
 fn to_http_date(value: &str) -> Option<String> {
-    if let Ok(datetime) = chrono::DateTime::parse_from_rfc3339(value) {
-        return Some(httpdate::fmt_http_date(datetime.into()));
-    }
-    httpdate::parse_http_date(value).ok().map(httpdate::fmt_http_date)
+    let time: chrono::DateTime<chrono::Utc> =
+        if let Ok(datetime) = chrono::DateTime::parse_from_rfc3339(value) {
+            datetime.into()
+        } else if let Ok(date) = chrono::NaiveDate::parse_from_str(value, "%Y-%m-%d") {
+            date.and_time(chrono::NaiveTime::MIN).and_utc()
+        } else {
+            return httpdate::parse_http_date(value).ok().map(httpdate::fmt_http_date);
+        };
+    // `fmt_http_date` panics outside that range.
+    let in_range = time.timestamp() >= 0 && chrono::Datelike::year(&time) <= 9999;
+    in_range.then(|| httpdate::fmt_http_date(time.into()))
 }
 
 /// Fetch the registry metadata document for `pkg_name`. The

pnpm/crates/resolving-npm-resolver/src/fetch_full_metadata/tests.rs

@@ -520,6 +520,17 @@ fn to_http_date_converts_iso_8601() {
     );
 }
 
+#[test]
+fn to_http_date_reads_a_date_without_a_time_as_midnight_utc() {
+    assert_eq!(super::to_http_date("2022-01-01").as_deref(), Some("Sat, 01 Jan 2022 00:00:00 GMT"));
+}
+
+#[test]
+fn to_http_date_drops_a_time_before_1970() {
+    assert_eq!(super::to_http_date("1969-12-31"), None);
+    assert_eq!(super::to_http_date("1969-12-31T23:59:59Z"), None);
+}
+
 #[test]
 fn to_http_date_keeps_http_dates() {
     assert_eq!(

pnpm11/pnpm/test/install/metadataRequestLog.ts

@@ -0,0 +1,137 @@
+import fs from 'node:fs'
+import http from 'node:http'
+import type { AddressInfo } from 'node:net'
+import path from 'node:path'
+
+import { afterEach, expect, test } from '@jest/globals'
+import { prepare } from '@pnpm/prepare'
+import { REGISTRY_MOCK_PORT } from '@pnpm/testing.registry-mock'
+import { writeYamlFileSync } from 'write-yaml-file'
+
+import { execPnpm } from '../utils/index.js'
+
+// The cases and their metadata request logs are shared with pnpm v12, see the
+// table's `description`.
+const CASE_TABLE = path.join(import.meta.dirname, '../../../../pnpr/.fixtures/request-logs/metadata-three-installs.json')
+
+interface CaseTable {
+  dependencies: Record<string, string>
+  cases: Array<{
+    name: string
+    settings: Record<string, string | number>
+    args: string[]
+    installs: string[][]
+  }>
+}
+
+interface RecordedRequest {
+  method: string
+  path: string
+  accept?: string
+  ifNoneMatch?: string
+  ifModifiedSince?: string
+  status: number
+  etag?: string
+}
+
+interface RecordingRegistry {
+  url: string
+  takeRequests: () => RecordedRequest[]
+  close: () => Promise<void>
+}
+
+const table: CaseTable = JSON.parse(fs.readFileSync(CASE_TABLE, 'utf8'))
+
+let registry: RecordingRegistry | undefined
+
+afterEach(async () => {
+  await registry?.close()
+  registry = undefined
+})
+
+test.each(table.cases)('$name', async ({ settings, args, installs: expected }) => {
+  registry = await startRecordingRegistry()
+  prepare({ dependencies: table.dependencies })
+  writeYamlFileSync('pnpm-workspace.yaml', { registry: registry.url })
+  // The test environment sets `minimumReleaseAge` as an environment
+  // variable, which outranks pnpm-workspace.yaml.
+  const env = Object.fromEntries(Object.entries(settings).map(([key, value]) => [
+    `pnpm_config_${key.replace(/[A-Z]/g, (letter) => `_${letter.toLowerCase()}`)}`,
+    String(value),
+  ]))
+  const installs: RecordedRequest[][] = []
+  for (let install = 0; install < 3; install++) {
+    // Remove what the last install wrote, so this one resolves again.
+    fs.rmSync('node_modules', { recursive: true, force: true })
+    fs.rmSync('pnpm-lock.yaml', { force: true })
+    // eslint-disable-next-line no-await-in-loop -- the installs share one cache and must run in order
+    await execPnpm(['install', ...args], { env })
+    installs.push(registry.takeRequests())
+  }
+
+  const etags = new Map(installs.flat()
+    .filter((request) => request.status === 200 && request.etag != null)
+    .map((request) => [`${request.path} ${documentOf(request)}`, request.etag]))
+  expect(installs.map((requests) => requests.map((request) => describe(request, etags)).sort())).toStrictEqual(expected)
+})
+
+function documentOf (request: RecordedRequest): string {
+  return request.accept?.startsWith('application/vnd.npm.install-v1+json') ? 'abbreviated' : 'full'
+}
+
+// A request sending some other ETag reads `other-etag`, which no case expects.
+function describe (request: RecordedRequest, etags: Map<string, string | undefined>): string {
+  const document = documentOf(request)
+  let validators = ''
+  if (request.ifNoneMatch != null) {
+    validators += etags.get(`${request.path} ${document}`) === request.ifNoneMatch ? ' etag' : ' other-etag'
+  }
+  if (request.ifModifiedSince != null) {
+    validators += ' since'
+  }
+  return `${request.method} ${request.path} ${document}${validators} ${request.status}`
+}
+
+// Forwards to the mock registry. Tarball URLs in the packuments name the mock
+// registry itself, so only metadata requests pass through here.
+async function startRecordingRegistry (): Promise<RecordingRegistry> {
+  let requests: RecordedRequest[] = []
+  const server = http.createServer((req, res) => {
+    const upstream = http.request({
+      host: 'localhost',
+      port: REGISTRY_MOCK_PORT,
+      method: req.method,
+      path: req.url,
+      headers: req.headers,
+    }, (upstreamRes) => {
+      requests.push({
+        method: req.method!,
+        path: req.url!,
+        accept: req.headers.accept,
+        ifNoneMatch: req.headers['if-none-match'],
+        ifModifiedSince: req.headers['if-modified-since'],
+        status: upstreamRes.statusCode!,
+        etag: upstreamRes.headers.etag,
+      })
+      res.writeHead(upstreamRes.statusCode!, upstreamRes.headers)
+      upstreamRes.pipe(res)
+    })
+    req.pipe(upstream)
+  })
+  await new Promise<void>((resolve) => {
+    server.listen(0, '127.0.0.1', resolve)
+  })
+  return {
+    url: `http://127.0.0.1:${(server.address() as AddressInfo).port}/`,
+    takeRequests: () => {
+      const taken = requests
+      requests = []
+      return taken
+    },
+    close: async () => new Promise<void>((resolve) => {
+      server.close(() => {
+        resolve()
+      })
+    }),
+  }
+}

pnpr/.fixtures/request-logs/metadata-three-installs.json

@@ -0,0 +1,83 @@
+{
+  "description": "Three installs that resolve the same project against an unchanged fixture registry, sharing one metadata cache. Each install's metadata requests, sorted, as `METHOD path document [etag] [since] status`: `etag` when the request sends the ETag the registry gave the same document, `since` when it sends If-Modified-Since. Asserted by pnpm/crates/cli/tests/suite/metadata_request_log.rs and pnpm11/pnpm/test/install/metadataRequestLog.ts.",
+  "dependencies": {
+    "@pnpm.e2e/pkg-with-1-dep": "^100.0.0"
+  },
+  "cases": [
+    {
+      "name": "abbreviated metadata is revalidated with its ETag",
+      "settings": {
+        "minimumReleaseAge": 0
+      },
+      "args": [],
+      "installs": [
+        [
+          "GET /@pnpm.e2e%2Fdep-of-pkg-with-1-dep abbreviated 200",
+          "GET /@pnpm.e2e%2Fpkg-with-1-dep abbreviated 200"
+        ],
+        [
+          "GET /@pnpm.e2e%2Fdep-of-pkg-with-1-dep abbreviated etag since 304",
+          "GET /@pnpm.e2e%2Fpkg-with-1-dep abbreviated etag since 304"
+        ],
+        [
+          "GET /@pnpm.e2e%2Fdep-of-pkg-with-1-dep abbreviated etag since 304",
+          "GET /@pnpm.e2e%2Fpkg-with-1-dep abbreviated etag since 304"
+        ]
+      ]
+    },
+    {
+      "name": "full metadata is revalidated with its ETag",
+      "settings": {
+        "minimumReleaseAge": 0,
+        "trustPolicy": "no-downgrade"
+      },
+      "args": [],
+      "installs": [
+        [
+          "GET /@pnpm.e2e%2Fdep-of-pkg-with-1-dep full 200",
+          "GET /@pnpm.e2e%2Fpkg-with-1-dep full 200"
+        ],
+        [
+          "GET /@pnpm.e2e%2Fdep-of-pkg-with-1-dep full etag since 304",
+          "GET /@pnpm.e2e%2Fpkg-with-1-dep full etag since 304"
+        ],
+        [
+          "GET /@pnpm.e2e%2Fdep-of-pkg-with-1-dep full etag since 304",
+          "GET /@pnpm.e2e%2Fpkg-with-1-dep full etag since 304"
+        ]
+      ]
+    },
+    {
+      "name": "a mirror written after the release age cutoff answers without a request",
+      "settings": {
+        "minimumReleaseAge": 1440
+      },
+      "args": [],
+      "installs": [
+        [
+          "GET /@pnpm.e2e%2Fdep-of-pkg-with-1-dep abbreviated 200",
+          "GET /@pnpm.e2e%2Fpkg-with-1-dep abbreviated 200"
+        ],
+        [],
+        []
+      ]
+    },
+    {
+      "name": "prefer-offline answers from the mirror without a request",
+      "settings": {
+        "minimumReleaseAge": 0
+      },
+      "args": [
+        "--prefer-offline"
+      ],
+      "installs": [
+        [
+          "GET /@pnpm.e2e%2Fdep-of-pkg-with-1-dep abbreviated 200",
+          "GET /@pnpm.e2e%2Fpkg-with-1-dep abbreviated 200"
+        ],
+        [],
+        []
+      ]
+    }
+  ]
+}