Allow passing fetch config

[cprussin]

Currently, I don't see any way to pass any configs to the fetch call. One very specific use case where this is going to be necessary is the upcoming next.js app router driven data fetch infrastructure, where next will start caching fetch responses and you will need to pass next.revalidate to fetch calls to configure how long the cache lives.

I don't think this is super high priority as it's also possible to export the revalidate const from a route segment. However, that reduces granularity of control -- i.e. I may have a route segment that consists of multiple DB requests, some of which are more permanent than others and thus can have a longer cache. That said, I also think it's a pretty trivial thing to support, and I'd even be happy to open a PR to implement it, if ya'll at planetscale would be open to accepting such a PR.

[dgraham]

We provide the fetch function override for this purpose. This allows fetch to be configured for the environment outside of the database driver without requiring the driver to pass through customized fetch configuration itself.

[cprussin]

Ah, thanks @dgraham, and sorry for the oversight! I didn't think to look in the Connection and figured it would be configured per-execute call.

Would there be a use case for re-using a connection with different e.g. cache settings? In other words is there appetite for adding a fetch override to the execute function instead of just on the connect?

[mattrobenolt]

If that's something you think you might need, architecturally I'd just suggest initializing multiple Connection or Client objects that have the settings you need. Like a cachedClient and uncachedClient.

But I'd strongly suggest not trying to utilize any HTTP caching for these requests. They are very much intended to not be cacheable and they are POST verb only. If you want you cache the results of something, that should be done by caching your response data explicitly as a common pattern. Or caching the component, whatever is at your disposal, but please do not try to cache the HTTP requests. They contain authentication and session/transaction state, perform mutations. I can't predict what bad would happen from this.

[cprussin]

@mattrobenolt To be clear, this caching is built into the new next.js version and is not something I'm controlling directly (or opting into expliictly). You can disable it of course but it's not the default. It would make sense that POSTs shouldn't be cached, but from my testing it seems that next is caching the data coming from my planetscale DB via this lib, so I'm assuming that it's caching POSTs too.

I'm guessing this is something that a lot of folks will run into, assuming a big chunk of ya'lls users are next.js users, given that it's all implicit in the upcoming next stuff. If the advice is to always disable the cache, you might want to consider either 1) making that the default in this library itself, if you can somehow detect the user is using next.js, or 2) documenting that advice somewhere in the README. However, I also think that always disabling the cache and forcing users to manually maintain a cache seems like a pretty heavy hammer for what appears to be a really nice optimization in the upcoming next platform, so maybe that's something to think about as well.

As I mentioned before, I'm happy to contribute PRs, but would need guidance on where you folks would want to go with this.

[mattrobenolt]

Yeah, for sure. I'm more saying I don't anticipate this being a per-execute call config. You'd just want to disable their behavior explicitly once globally for our Client. Whether we want or need to expand documentation or better default support to prevent the foot gun I think is valid and would require some more understanding from our side.

[cprussin]

FWIW, as a user, I do think disabling the cache entirely as the only solution would be a pretty negative outcome here TBH. Might be worth opening a ticket to investigate changes in the library to make caching safe instead.

[mattrobenolt]

However, I also think that always disabling the cache and forcing users to manually maintain a cache seems like a pretty heavy hammer for what appears to be a really nice optimization in the upcoming next platform, so maybe that's something to think about as well.

Also on this point explicitly, from the HTTP level, or per-query level, it's impossible for us to know if it's safe to cache or not. This interface is simple "run a query". And this query doesn't have to be a SELECT obviously, and this query might purely mutate state. Think of a query to set a session variable, or the "begin" query for a transaction. Queries within a transaction alone aren't necessarily safe to cache until after the transaction is committed, transaction isolation, etc.

From this layer, there's just no safe way to cache the data. If you were using a native traditional mysql binary driver, this wouldn't even be a question since that's not a feature there.

Basically, from a driver's POV and our API, we don't know if it's safe or not since that requires a bit more of your application knowledge.

Arguably you could say "yes, I know it's safe", due to our API explicitly being POST-only, I cannot agree that any caching would yield any behavior we can support. If we expand to an API that supports GET, we can likely limit the scope of the API to have less functionality that we can better guarantee safety. But that'll be difficult.

[mattrobenolt]

Might be worth opening a ticket to investigate changes in the library to make caching safe instead.

If we want to tease this idea, I think we'd need to fully explore an actual cacheable API from the server. This isn't particularly a library feature. This is very much equivalent of asking for adding caching to the mysql binary protocol. Or adding caching to TCP itself.

We just happen to be using HTTP as a transport.

[cprussin]

Yeah, that's all fair and totally makes sense. Would be interesting to chat more with the next.js folks and better understand the intent there. Is it intended to only be used to cache requests to REST-ish upstream services? If so, it seems pretty misleading (and dangerous) that their docs constantly mention the database explicitly, which makes it seem like this functionality is explicitly intended to cache requests to database drivers.

I can put in a ticket over on the next.js repo to clarify docs or something, but I don't have high hopes that it will be prioritized, given the flood of tickets / PRs they receive. So ya'll should probably anticipate some amount of fallout here one way or another. Don't shoot the messenger.

If you all happen to have contacts over at vercel, it might be wise to chat through it with them.

[mattrobenolt]

Yup, I absolutely agree with that as well. That's why I'd like to understand their implementation better before we make any changes or doc updates.

My issue with this is the web and HTTP has a very well understood caching behavior with existing mechanisms such as Cache-Control header, If-Modified-Since, etc. if their caching adopted this anticipated behavior, there wouldn't be confusion. If we wanted or expected our responses to be cached, I'd have implemented it to follow the standards for proxy caching.

I don't think you're doing anything wrong by asking, I just have a strong opinion that we can't do this without intentional design and if Next is doing this like you're suggesting, that's really bad.

So I wanna believe there's some misunderstanding in behavior here since generically caching all POST requests from their POV would be extremely dangerous.

[cprussin]

To be clear, this is early-beta functionality right now. So maybe that's not the intent / is buggy / is incomplete. I also hadn't really bothered to dig deeper before this thread as I just didn't put much thought into the implications of caching. But it's hard to see what the intent is as the docs aren't well fleshed out yet and it's hard to get access to folks in the know.

[mattrobenolt]

Ok, this definitely is intentional behavior from Next.js side of things: vercel/next.js#46856

I have no idea how they came to this conclusion, but this seems... genuinely pretty bad. They're choosing to both ignore Authorization header and POST requests, with their caching now, which is inevitably going to cause a security issue.

I'm going to do what I can to surface this on their side, and see what can be done to reverse this default behavior on their end, or at least try to understand the merits for this change.

In the interim, there's a {cache: 'no-store'} that isn't Next.js specific that I think we can just simply add to our call by default. https://developer.mozilla.org/en-US/docs/Web/API/fetch#cache

I didn't want to add anything Next.js specific, or against the fetch() spec, but this cache: no-store is in the spec, and is overly explicit, but seems safe to save some DX here. Even the fetch spec defines this behavior as to what to expect based on this cache value, and what default implies: https://developer.mozilla.org/en-US/docs/Web/HTTP/Caching

Our usage immediately trips two caching red flags: being a POST, and using an Authorization header. Either one of these, regardless of anything else, should trigger a request to be considered "unsafe" for caching.