fix: request cache mode is not the same as request mode

[tsibley]

Noticed while reading thru the code.

This relates to...

Rationale

Changes

Features

Bug Fixes

Breaking Changes and Deprecations

Status

• I have read and agreed to the Developer's Certificate of Origin
• Tested
• Benchmarked (optional)
• Documented
• Review ready
• In review
• Merge ready

[mcollina]

Thanks for opening a PR! Can you please add a unit test?

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.16%. Comparing base (9458ffd) to head (1b5cf8a).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3151      +/-   ##
==========================================
- Coverage   94.17%   94.16%   -0.01%     
==========================================
  Files          90       90              
  Lines       24300    24300              
==========================================
- Hits        22884    22882       -2     
- Misses       1416     1418       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[tsibley]

@mcollina Ah, I'm not going to familiarize myself with this project's test suite and add a unit test just for this PR. I'm not even a user of the project. I just wanted to flag this typo/bug to folks.

[mcollina]

ping @KhafraDev

[mcollina]

I'm not going to land a change without a test. @KhafraDev is this something you could do? I don't understand this change.

[Uzlopak]

Unfortunately it is not possible to push into the repo of the PR provider.

I investigated and it is basically not possible to test it, as httpCache is not implemented yet. So httpCache is always null, and thus the cache mode being set to no-store.

undici/lib/web/fetch/index.js

Line 1535 in 8dd32ef

httpRequest.cache = 'no-store'

The following patch would set the value for webidl for RequestInit field cache to default as defined in the spec, add the attribute cache to the RequestInit typescript interface and add a unit test, which fails because cache is not implemented.

From af1ec1a823d4a95f5633b60f6a29b5d4b598cfe0 Mon Sep 17 00:00:00 2001
From: uzlopak <aras.abbasi@googlemail.com>
Date: Wed, 24 Apr 2024 12:19:46 +0200
Subject: [PATCH] add test

---
 lib/web/fetch/request.js |  3 ++-
 test/fetch/redirect.js   | 28 ++++++++++++++++++++++++++++
 types/fetch.d.ts         |  1 +
 3 files changed, 31 insertions(+), 1 deletion(-)

diff --git a/lib/web/fetch/request.js b/lib/web/fetch/request.js
index 2cb8b66d..02f5ad83 100644
--- a/lib/web/fetch/request.js
+++ b/lib/web/fetch/request.js
@@ -961,7 +961,8 @@ webidl.converters.RequestInit = webidl.dictionaryConverter([
     key: 'cache',
     converter: webidl.converters.DOMString,
     // https://fetch.spec.whatwg.org/#requestcache
-    allowedValues: requestCache
+    allowedValues: requestCache,
+    defaultValue: 'default'
   },
   {
     key: 'redirect',
diff --git a/test/fetch/redirect.js b/test/fetch/redirect.js
index a9533bcc..17c20c7a 100644
--- a/test/fetch/redirect.js
+++ b/test/fetch/redirect.js
@@ -76,3 +76,31 @@ test('Redirecting with a body does not fail to write body - #2543', async (t) =>
   assert.strictEqual(await resp.text(), 'ok')
   assert.ok(resp.redirected)
 })
+
+test('Redirecting with cache mode set to only-if-cached should throw a Network Error - #3151', async (t) => {
+  const server = createServer((req, res) => {
+    if (req.url === '/redirect') {
+      res.statusCode = 301
+      res.setHeader('location', '/redirect/')
+      res.write('<a href="/redirect/">Moved Permanently</a>')
+      setTimeout(() => res.end(), 500)
+      return
+    }
+
+    res.write(req.url)
+    res.end()
+  }).listen(0)
+
+  t.after(closeServerAsPromise(server))
+  await once(server, 'listening')
+
+  try {
+    await fetch(`http://localhost:${server.address().port}/redirect`, {
+      mode: 'same-origin',
+      cache: 'only-if-cached'
+    })
+    assert.fail('Expected an error')
+  } catch (err) {
+    assert.strictEqual(err.name, 'NetworkError')
+  }
+})
diff --git a/types/fetch.d.ts b/types/fetch.d.ts
index 18eadb6a..d1ecf7f3 100644
--- a/types/fetch.d.ts
+++ b/types/fetch.d.ts
@@ -127,6 +127,7 @@ export interface RequestInit {
   integrity?: string
   signal?: AbortSignal | null
   credentials?: RequestCredentials
+  cache?: RequestCache,
   mode?: RequestMode
   referrer?: string
   referrerPolicy?: ReferrerPolicy
-- 
2.34.1

[mcollina]

@Uzlopak so, what should we do?

[Uzlopak]

@mcollina

Well, my assessment is, that this PR is valid but it is not testable because http caching according to RFC 9111 is not implemented. The modified branches are noop's and placeholders for to be implemented features mentioned in the fetch RFC. So modifying them has currently no effect .

I recommend to merge this PR as is.

[Uzlopak]

LGTM

[mcollina]

lgtm