Update BaseDataService to accommodate mutations, not just queries

[mcmire]

Explanation

Currently, BaseDataService has a fetchQuery method which is best used for making read-only requests ("queries" in TanStack Query parlance), but does not work as well for requests that change state on the server side ("mutations"). For instance, it makes sense for queries to be cached, but not so much for mutations.

This commit adds a separate method, executeMutation, which accommodates mutations better. Its implementation is different from fetchQuery as it uses the mutation cache instead of the query cache.

References

https://consensyssoftware.atlassian.net/browse/WPC-1118

Checklist

• I've updated the test suite for new or updated code as appropriate
• I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate
• I've communicated my changes to consumers by updating changelogs for packages I've changed
• I've introduced breaking changes in this PR and have prepared draft pull requests for clients and consumer packages to resolve them

---

Note

Medium Risk
Introduces a new path for server writes; incorrect use (e.g. still using fetchQuery for mutations or assuming cache events) could cause subtle integration bugs, though existing query behavior is unchanged.

Overview
Adds a protected executeMutation API on BaseDataService so subclasses can run TanStack Query mutations (state-changing HTTP calls) instead of overloading fetchQuery, which is aimed at cached reads.

executeMutation builds from the mutation cache, requires mutationKey and mutationFn, omits per-mutation retry / retryDelay (same pattern as queries—retries go through the service policy), and wraps mutationFn with #policy.execute. Mutations are not treated like query cache entries; tests assert addFollower succeeds and that :cacheUpdated messenger events are not published for mutations.

The test ExampleDataService gains addFollower (PUT to the social API) as a reference implementation, plus nock mocks and changelog entry.

^{Reviewed by Cursor Bugbot for commit e88e14a. Bugbot is set up for automated code reviews on this repo. Configure here.}

[mcmire]

QueryClient doesn't have a executeMutation method itself. I looked inside of the class, however, and saw that there was a getMutationCache method and followed where that led. This is not how useMutation works, which uses MutationObserver, but I don't know if that really matters. I figured it made more sense to mimic how fetchQuery works. But I'm not very familiar with TanStack Query, so if this is not the way we should be doing things, I'm happy to change this.

[mcmire]

Adapted from SocialService:

core/packages/social-controllers/src/SocialService.ts

Line 515 in 552b51e

async follow(options: FollowOptions): Promise<FollowResponse> {

[cursor]

Cursor Bugbot has reviewed your changes and found 2 potential issues.

^{❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.}

^{Reviewed by Cursor Bugbot for commit 603d673. Configure here.}

[cursor]

Mutation variables never forwarded

Medium Severity

executeMutation always calls mutation.execute() with no arguments, so TanStack mutationFn never receives TVariables. Callers that pass inputs via variables (the usual mutation pattern) instead of closing over them will run with missing or undefined data.

 

^{Reviewed by Cursor Bugbot for commit 603d673. Configure here.}

[mcmire]

We are using an older version of TanStack Query where Mutation.execute doesn't take any arguments. Variables are passed to the Mutation constructor.

[FrederikBolding]

Hmm, are we safe to apply the policy as-is to mutations?

Just wondering if there could be a problem with accidentally doing double the mutation with retries 🤔

[mcmire]

Hmm, good point. We're still making an API request, so I feel like we would still want to have some kind of retry logic. But maybe it needs to be a little different than the logic used for GET requests.

I wonder if the default retry policy for createServicePolicy is too liberal. Right now, if the function you pass to the policy throws any error, then the function will get run again.

In RpcService we only retry connection errors, JSON parse errors, HTTP server errors (5xx), timeout errors, and "connection reset" errors. I wonder if BaseDataService should configure createServicePolicy such that it does the same thing?

Then I would be less worried here, because at that point, either we never make the request, or we do make the request but the server returns a 5xx. And in that case I feel like we ought to assume that the server is well-behaved, i.e. won't attempt to write to a database if it runs into an error. (If the server is not well-behaved, it shouldn't be our fault — the engineer writing the data service should know that and account for it.)

What do you think about that idea?

[FrederikBolding]

Improving the defaults to be more "targeted" to BaseDataService makes sense to me. Though I still would be a bit worried that developers configure the service policy mainly for GET requests and don't realize how it may impact a PUT 🤔

[mcmire]

I wouldn't expect that most developers would configure the service policy, I would expect them to go with the defaults. But that's a good point. Should we have two kinds of service policies, one for queries and another for mutations? Then if a developer does configure a service policy, it should be more obvious what it's used for, and maybe it will cause them to think about it more critically. Or maybe this is solvable via documentation: we can add an "advanced" section to the tutorial/guide on data services that talks about the service policy, and we can remind the reader to consider non-GET requests when configuring it.