vercel / next.js

The React Framework
https://nextjs.org
MIT License
127.11k stars 27.01k forks source link

Next caches `POST` , `PUT` and `DELETE` requests if the request returns a `200 OK` status code even with `Authorization` header present #52405

Open Fredkiss3 opened 1 year ago

Fredkiss3 commented 1 year ago

Verify canary release

Provide environment information

Operating System:
      Platform: darwin
      Arch: arm64
      Version: Darwin Kernel Version 22.5.0: Thu Jun  8 22:22:19 PDT 2023; root:xnu-8796.121.3~7/RELEASE_ARM64_T8103
    Binaries:
      Node: 18.16.0
      npm: 9.5.1
      Yarn: 1.22.19
      pnpm: 8.6.2
    Relevant Packages:
      next: 13.4.10-canary.0
      eslint-config-next: N/A
      react: 18.2.0
      react-dom: 18.2.0
      typescript: 5.1.6
    Next.js Config:
      output: N/A

Which area(s) of Next.js are affected? (leave empty if unsure)

App Router, Data fetching (gS(S)P, getInitialProps), Middleware / Edge (API routes, runtime)

Link to the code that reproduces this issue or a replay of the bug

https://github.com/Fredkiss3/next-fetch-post-cache-edge-bug

To Reproduce

  1. clone and run the server
  2. Go to / in your browser and refresh the browser at least twice
  3. You will see that the data shown in the section POST /api/cache do not change after reloads
  4. You can also see your terminal, the object { cache: ... } will always show the same value
  5. You can also go to the folder .next/cache/fetch-cache/ you will see an entry for the cached data

Describe the Bug

When using edge runtime POST requests are cached if the response returns a 200 OK status code, even if the request contains an Authorization header. This issue do not happen if the request returns a different status code (like 201 for ex).

Expected Behavior

I may understand why this behavior has been introduced (maybe to help people cache graphql queries), but using an Authorization header should opt the fetch out of caching, as it is said in the docs :

Requests are not cached if:

  • Dynamic methods (next/headers, export const POST, or similar) are used and the fetch is a POST request (or uses Authorization or cookie headers)

I also thought that POST requests would not automatically be cached but there is not a clear message in the docs as to wether POST are also cached or are not cached by default. I don't know what the desired behavior is supposed to be, but if you want the user to cache their POST request, they can still manually add a cache field to the fetch i think ?

This issue is particularly annoying as it is difficult to spot if you are using a data-fetching or database library (in my case turso with the HTTP client), most of these librairies use POST requests and return 200 OK as statuses. I think it could cause the same frustration if used with appolo.

Which browser are you using? (if relevant)

No response

How are you deploying your application? (if relevant)

No response

fuma-nama commented 1 year ago

POST requests are cached too, you can check this PR.

The reason why "using Authorization or cookie headers" would cause requests not automatically cached is that it uses the cookies method, which will opt the route into dynamic rendering.

Fredkiss3 commented 1 year ago

this is stupid (sorry for my harsh language). I have this use case : i use turso with drizzle which uses @libsql/client in the hood, each request is a POST with an Authorization header. I don't have the means to change the code under the hood but each request returns a 200 OK response. Now how can i make sure these requests are never cached as they are for my database and i want each request to give me the latest data ? With this i loose control over how i can fix the issue.

If you want a real reproduction link you can see here : https://nextjs-13-contacts.vercel.app/contacts/51 (link to the source code) , in this app if you click on the star button, it will work the first time, but not again, while it is supposed to toggle the data (the favorite column should be toggled from true to false and vice-versa), since it caches the data, only the first click will update, and if the data is already cached, clicking on the button won't do anything. The route is already using dynamic rendering.

But for what i read from the docs, using an Authorization header should also skip the cache, which is an expected behavior as usually you want you don't want a token to be valid for longer that it needs :

Requests are not cached if:

  • [...] the fetch is a POST request (or uses Authorization or cookie headers)
fuma-nama commented 1 year ago

I thought that means "read headers" instead of passing it.

I don't sure how is it originally expected to be (it can be either a internal bug or a docs issue) but Next.js doesn't skip cache even if you passed an authorisation header.

As docs mentioned, you can opt out of fetch caching by configuring options on individual fetch (Which is 100% in control), or using segment-level caching.

Fredkiss3 commented 1 year ago

For me it segment config does not work, because I use parallel routes ?

And I tried to set fetchCache to the root layout, it doesn't work.

You wouldn't read headers to Authenticate to your API server or database, unless you want to implement a proxy server in next the header would need to be passed from the client to your fetch request.

You would use a secret, in a environment variable.

Fredkiss3 commented 1 year ago

Keep in ming that when i run requests that create data it gets cached (it does not create a new entry to the database), i had to add a createdAt field here to bust the cache entry created :

  const [res] = await db
    .insert(contacts)
    .values({
      favorite: false,
      createdAt: new Date(),
    })
    .returning({ insertedId: contacts.id })
    .all();
fuma-nama commented 1 year ago

I have made some researches on the source code, you can see here.

In your case, you need to configure the fetch options. Otherwise, you should use Route Handlers instead, requests in a POST route handler won't be cached.

Fredkiss3 commented 1 year ago

I cannot configure fetch options as i use a wrapper library, and i don't want to use route handlers, (i use server actions).

Reading the comment from the source code, it seems like POST request & other methods are not cached only when a dynamic function is used before (cookies() or headers()) or revalidate is set to 0 which mark the page as dynamic :

link: https://github.com/vercel/next.js/blob/56fcd7ac33e700236e6a37a8dac5ed9c378e0823/packages/next/src/server/lib/patch-fetch.ts#L214C9-L219

// if there are authorized headers or a POST method and
// dynamic data usage was present above the tree we bail
// e.g. if cookies() is used before an authed/POST fetch

I've also tested this, and unless you call cookies or headers before making the request, the POST request is not cached, else it will always cache the request.

this is not good as DELETE and PUT requests should never be cached at all. i may understand POST for graphql but not the other ones.

fuma-nama commented 1 year ago

Segment-level caching doesn't work for server actions, I also experienced the same problem in my project. My approach is to use Route Handler instead (with SWR)

As this behaviour is clearly documented, it might be better to open a Feature request or something else (it is not a bug).

In fact, I've opened a feature request for that months ago but didn't get any attention.

Fredkiss3 commented 1 year ago

right now, i manually call cookies() function everytime before making a DB request, but it shouldn't be.

I'm gonna guess that since it has these variable names hasUnCacheableHeader & isUnCacheableMethod means that POST & other methods (PUT, DELETE, OPTIONS) should not be cached at all and/or the if request has an Authorization headers, it should be the same.

What is also funny is that if it was supposed to cache, it would also cache POST requests returning any status code, but here if the fetch returns any code other than 200, it won't cache it.

fuma-nama commented 1 year ago

right now, i manually call cookies() function everytime before making a DB request, but it shouldn't be.

I'm gonna guess that since it has these variable names hasUnCacheableHeader & isUnCacheableMethod means that POST & other methods (PUT, DELETE, OPTIONS) should not be cached at all and/or the if request has an Authorization headers, it should be the same.

What is also funny is that if it was supposed to cache, it would also cache POST requests returning any status code, but here if the fetch returns any code other than 200, it won't cache it.

Because if it's not 200, it means the fetch has failed. Why do they need to cache failed requests?

Also, this behaviour is expected, please refer to the docs: Post requests are not cached if a dynamic method is used.

This has been answered in the Caching section on the documentation.

Fredkiss3 commented 1 year ago

there are other status codes like 201 & 3xx which still indicates successful requests.

From MDN docs : https://developer.mozilla.org/en-US/docs/Web/HTTP/Status#successful_responses

fuma-nama commented 1 year ago

Great observation! You're right.

At here, it only caches responses with 200 status code which is absolutely a bug.