breaking: rework client-driven refreshes

[elliott-with-the-longest-name-on-github]

Another breaking change to queries.

Prior to this PR, it was possible to write the following on the client:

await myCommand().updates(query1(), query2().withOverride(/* reducer */));

This would tell the server to run the command and refresh query1 and query2 in one round-trip. This is awesome, but it has a crucial flaw: There's no way to design the API so that it's completely safe. The client can request arbitrarily many refreshes of arbitrarily many functions, all boxed up in one nice HTTP request. This kind of pattern is difficult to control for at any level of the security stack.

Our original solution was to remove client-driven refreshes entirely, relying on server-driven refreshes (where, in the server handler, you call query1(/* args */).refresh()). This works much of the time, but it starts to break down when you don't know exactly what args to call query1 with. For example, if you had a call to getPullRequests({ filter: 'author:elliott-with-the-longest-name-on-github }), it would be hard to know to refresh that query inside the createPullRequestcommand, as you wouldn't naturally have any access to the args thatgetPullRequests` was called with on the client.

Thinking through the problem, we realized something important: The server always knows which categories of data it can invalidate, but sometimes only the client knows which specific data to invalidate. This made the API decisions pretty easy. You can continue to write the same code as before, but instead of being a client-driven update, it's now a client-requested update. On the server, you need to accept the requested updates:

import { query, command, requested } from '$app/server';

export const getPullRequests = query(GetPullRequestSchema, async () => {});

export const createPullRequest = command(CreatePullRequestSchema, async (args) => {
  /* do the stuff to create the pull request */

  // if your query uses async validators, you can also `for await` this.
  // it will run validation in parallel and yield results as they're available.
  for (const arg of requested(getPullRequests, 5)) {
    void getPullRequests(arg).refresh();
  }
});

requested accepts a query function (not a query instance) and returns all of the validated args that the client requested for the associated query. Its second argument is required, and is a limit on the number of instances this command is willing to refresh. A couple of notes on its behavior:

• Any requested args that fail validation are excluded. A "this query has failed to refresh" response will be sent back to the client for that query, just as if it had failed to refresh in the first place.
• Any requests beyond the provided limit will also send a failed response back to the client.
• Previously, if query.refresh() failed, the entire command would fail. This is no longer the case. Refresh failures are sent back to the client per-query rather than crashing the entire command.

There's also a small additional trick we've added to command.updates -- you can pass in a query function to request all active instances be refreshed:

await createPullRequest(...).updates(
  // request all active instances be refreshed
  getPullRequests,
  // request just a specific instance be refreshed
  pullRequestsInstance,
  // request just a specific instance with an override
  pullRequestsInstance.withOverride(...)
)

It's perfectly legal to provide getPullRequests and a more-specific pullRequestsInstance.withOverride -- the more-specific request will take precedence. It is not legal to request multiple refreshes of the same query with overrides.

[changeset-bot]

🦋 Changeset detected

Latest commit: 0146ebe

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@sveltejs/kit	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[svelte-docs-bot]

Preview: https://svelte-dev-git-preview-kit-15562-svelte.vercel.app/

[abdelfattahradwan]

I hope I am misunderstanding this, but does this mean we will no longer be able to do something like:

<script lang="ts">
  import { myQuery, myCommand } from "$lib/remote-functions/my-fns.remote";

  const myQueryInstance = $derived(myQuery());
</script>

{myQueryInstance.current ?? "N/A"}

<button
  type="button"
  onclick={() => {
    void myCommand().updates(myQueryInstance);
  }}
>
  Do Something
</button>

If that's true, it would be a massive loss, especially since we cannot refresh queries by, say, keys rather than arguments on the server. We currently heavily utilise client-driven refreshes for that reason.

First, and least importantly: It's just kind of weird to have two different refresh mechanisms for queries.

But didn't they serve different purposes? I personally thought it was intuitive that SvelteKit gave you a choice, letting you pick the solution that worked for you.

Client-driven refreshes pose a security risk, and there's no real way to get around it. They allow an attacker to request arbitrarily-many refreshes of arbitrarily-many queries per request. This kind of pattern is very difficult to defend against from a firewall / application stack perspective.

Isn't it better to simply document the potential security risk rather than removing the feature?

This is an easier argument to make, too, when we already have a robust, composable, and easy-to-use server-driven single-flight-mutation mechanism.

I have to personally disagree with the claim that server-driven refreshes are easy to use. They are only easy to use for very basic use cases, but once you get to more advanced stuff, they get very annoying and are difficult to work with.

As an example, if I have a query which returns an array of objects and takes in a bunch of filtering arguments and a command that mutates a single object, the only way, as far as I can tell, to update the query via server-driven refreshes is to include the filtering arguments alongside the other arguments in the update command. Which, apart from being wasteful (you are sending a bunch of arguments which basically act like a key), gives you less flexibility and sometimes isn't easily doable.

Also, removing client-driven refreshes eliminates the flexibility of, say, passing a bunch of queries as an array to a component that invokes a command that needs to refresh an unknown number of queries.

I can think of many more reasons why removing client-driven refreshes is a bad idea for SvelteKit users. I understand Remote Functions are experimental, and I should expect changes like this, but this one is really bad, in my honest opinion.

[johnathanludwig]

Would this disable a refresh that is outside of a form? In some cases I use an interval to .refresh() for polling situations, and it seems like this wouldn't work anymore. I could use command, but then I lose the cache functionality especially when the polling is conditional.

It doesn't seem like disabling refresh would prevent an attacker as they could still just make the requests themselves. Not sure why they would go through the refresh API.

[Rich-Harris]

Isn't it better to simply document the potential security risk rather than removing the feature?

That's just not how security stuff works unfortunately. Documentation is appropriate when there's some action the developer can take, like 'always sanitize user-generated content you pass to {@html ...}'. If there's no action that can be taken then the documentation would basically amount to 'by the way, if you're using remote functions then you're vulnerable, good luck out there champ'.

There's another reason to make all single-flight mutations server-initiated, and it overlaps with the reasons we don't currently have a tag-based revalidation API: it restricts our future design choices. If the client can specify which things need to be refreshed, either by tag or by the .updates(...) API, then the hidden assumption is that the server is in fact able to refresh those things.

Today, that assumption holds true: all remote functions are handled in the same place. If the server receives a POST request to /_app/remote/abc123/my_command then it will also be able to run the /_app/remote/def456/my_query handler. But in the same way that you can (on supported platforms) split your app up into multiple serverless functions, so that cold start times aren't tanked by a particularly heavy route, we might one day want to be able to handle some remote functions in one serverless function and others in another.

Tag-based revalidation is a naive solution to the problem that makes that impossible (alongside other drawbacks like an inability to e.g. right-click on a remote function and find all the places it's getting called from). Client-driven refreshes are essentially an indirect form of tag-based revalidation.

By contrast, server-driven refreshes solve that problem, because a command that refreshes a query is required to have imported the query function, meaning they are guaranteed to end up in the same server bundle.

(It's quite possible that your response to all that is 'ok but I don't care' which is fine; I'm just explaining why it's potentially important for other people.)

the only way, as far as I can tell, to update the query via server-driven refreshes is to include the filtering arguments alongside the other arguments in the update command

That is one solution, and to be honest I think it's perfectly fine. But there's another, which is to just forgo the single-flight part. You can just refresh() the query immediately after the command. Which leads me to...

Would this disable a refresh that is outside of a form?

No, not at all. refresh() doesn't suffer from the asymmetry that makes updates(...) an issue.

[abdelfattahradwan]

There's another reason to make all single-flight mutations server-initiated, and it overlaps with the reasons we don't currently have a tag-based revalidation API: it restricts our future design choices. If the client can specify which things need to be refreshed, either by tag or by the .updates(...) API, then the hidden assumption is that the server is in fact able to refresh those things.

Today, that assumption holds true: all remote functions are handled in the same place. If the server receives a POST request to /_app/remote/abc123/my_command then it will also be able to run the /_app/remote/def456/my_query handler. But in the same way that you can (on supported platforms) split your app up into multiple serverless functions, so that cold start times aren't tanked by a particularly heavy route, we might one day want to be able to handle some remote functions in one serverless function and others in another.

Tag-based revalidation is a naive solution to the problem that makes that impossible (alongside other drawbacks like an inability to e.g. right-click on a remote function and find all the places it's getting called from). Client-driven refreshes are essentially an indirect form of tag-based revalidation.

By contrast, server-driven refreshes solve that problem, because a command that refreshes a query is required to have imported the query function, meaning they are guaranteed to end up in the same server bundle.

I honestly did not think about that initially, but I see the potential downsides now.

(It's quite possible that your response to all that is 'ok but I don't care' which is fine; I'm just explaining why it's potentially important for other people.)

Haha, no. That would be rude. 😅 I am thankful that you took the time to reply with a detailed explanation.

You can just refresh() the query immediately after the command.

But doesn't this add latency from multiple round-trip requests, rather than everything being done in one go? Unless .refresh calls are batched somehow?

[Rich-Harris]

Not batched, no, otherwise we're back in the same place where someone could trivially create a massive amount of work via a relatively small payload. But the requests would happen concurrently as soon as the command returned.

[abdelfattahradwan]

OK. I guess I have to say goodbye to this feature then. There are workarounds, but it was nice having such a useful, albeit insecure (as I learned today) feature built in.

[Rich-Harris]

What if we exposed the query argument on the instance?

<script lang="ts">
  import { myQuery, myCommand } from "$lib/remote-functions/my-fns.remote";

  const myQueryInstance = $derived(myQuery());
</script>

{myQueryInstance.current ?? "N/A"}

<button
  type="button"
  onclick={() => {
-   void myCommand().updates(myQueryInstance);
+   void myCommand({ updates: myQueryInstance.argument });
  }}
>
  Do Something
</button>

Then on the server, myCommand could, if updates was included in the payload, refresh the query itself (obviously you would control the shape of this API, so you could e.g. pass multiple arguments if needed)

[abdelfattahradwan]

What if we exposed the query argument on the instance?

<script lang="ts">
  import { myQuery, myCommand } from "$lib/remote-functions/my-fns.remote";

  const myQueryInstance = $derived(myQuery());
</script>

{myQueryInstance.current ?? "N/A"}

<button
  type="button"
  onclick={() => {
-   void myCommand().updates(myQueryInstance);
+   void myCommand({ updates: myQueryInstance.argument });
  }}
>
  Do Something
</button>

Then on the server, myCommand could, if updates was included in the payload, refresh the query itself (obviously you would control the shape of this API, so you could e.g. pass multiple arguments if needed)

I was thinking about suggesting something EXACTLY like that, yes! Anything to make server-driven refreshes easier to work with for queries that take many arguments or for unknown sets of queries would be better than nothing. Especially since the only real thing (at least for me) that makes server-driven refreshes inflexible (compared to client-driven ones) is the need to manually pass query arguments to trigger a refresh.

[henrykrinkle01]

That seems ok for commands but how about forms? Arbitrarily shaped arguments can't be easily included in a form submission without creating the inputs. Sounds like a lot of work

[elliott-with-the-longest-name-on-github]

Don't worry everybody, we've got you taken care of 😉

[elliott-with-the-longest-name-on-github]

@abdelfattahradwan just wanted to let you know this has been reworked -- any thoughts on the new API?

[abdelfattahradwan]

@abdelfattahradwan just wanted to let you know this has been reworked -- any thoughts on the new API?

I have been obsessively checking the PR and noticed the changes earlier, hehe.

I personally like them, especially now that I can refresh "all" instances of a particular query. requested is also much better than having to pass an updates property, to be honest.

I wish there were a version of requested that let me refresh all queries passed to updates without having to pass the query as the first argument, but only up to a limit. Basically, something that allows me to say "refresh all queries, whatever they are, but do not refresh more than 5 queries".

For example:

// Refresh up to 5 queries, whatever they may be.
for (const query of requested(5)) {
  query.refresh();
}

But I think the current version is good for 80% of my use cases. Thanks a lot for the awesome work!

[elliott-with-the-longest-name-on-github]

Basically, something that allows me to say "refresh all queries, whatever they are, but do not refresh more than 5 queries".

We considered some different versions of this. In the first place, the reason requested doesn't return query instances is because we want you to be able to interact with the arguments -- it may be you only want to allow runQuery to be refreshed if arg.id === created_id, for example.

While the convenience of the specific API you've outlined is nice, it unfortunately defeats the purpose of the API -- the command must declare which specific queries it is willing to refresh and in what quantities. Reason being, you could have something like this:

export const cheap_query = query(...);

export const create_cheap_thing = command('unchecked', () => {
  // these things are cheap, and we render a bunch of them -- so we want to be able to refresh a bunch of them!
  for (const query of requested(1000)) {
    void query.refresh();
  }
});

// in an unrelated file
export const very_expensive_query = query(...);

export const create_expensive_thing = command('unchecked', () => {
  // these things are expensive -- it is unacceptable to run more than one per command
  for (const query of requested(1)) {
    void query.refresh();
  }
});

All someone would have to do is submit a HTTP request to create_cheap_thing requesting 1000 runs of very_expensive_query, and we'd allow them to do it -- because create_cheap_thing doesn't specify anywhere that it's only supposed to be able to refresh cheap_query.

Relatedly but probably less important, this is also important for code splitting reasons -- requiring you to actually import and use the queries you plan on refreshing means that we can prune unused query dependencies when bundling. If a command can arbitrarily refresh any query without importing it, we have to include the code for all queries with every command.

[abdelfattahradwan]

All someone would have to do is submit a HTTP request to create_cheap_thing requesting 1000 runs of very_expensive_query, and we'd allow them to do it -- because create_cheap_thing doesn't specify anywhere that it's only supposed to be able to refresh cheap_query.

Ah, I see it now, yeah. Makes sense.

If a command can arbitrarily refresh any query without importing it, we have to include the code for all queries with every command.

Whoa, I didn't know that was the case.

But, as I said before, I personally like the new API and think it will work just fine. It may not be as "convenient" as the old one, but its benefits are definitely welcome.

[Rich-Harris]

a few docs notes but otherwise LGTM 👍

[henrykrinkle01]

Can we add a .refresh() method to requested:

requested(query_1, 5).refresh();
requested(query_2, 5).refresh();

which is syntactic sugar to:

for (const arg of requested(query_1, 5)) {
  void query_1(arg).refresh();
}
for (const arg of requested(query_2, 5)) {
  void query_2(arg).refresh();
}

Of course you're free to choose which form to use depending on your needs.

[elliott-with-the-longest-name-on-github]

@henrykrinkle01 good idea, added -- we called it refreshAll just to be a little more clear (and line up with the global refreshAll you can use on the client)