Skip to content

Update BaseDataService to accommodate mutations, not just queries #9324

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
mcmire wants to merge 2 commits into
base: main
Choose a base branch
from
+118 −0
Open

Update BaseDataService to accommodate mutations, not just queries #9324

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
603d673
Update BaseDataService to accommodate mutations, not just queries
mcmire Jun 30, 2026
e88e14a
Make test more accurate
mcmire Jul 1, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions packages/base-data-service/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

## [Unreleased]

### Added

- Add `executeMutation` to `BaseDataService` to allow for making state-mutating requests ([#9324](https://github.com/MetaMask/core/pull/9324))

### Changed

- Bump `@metamask/utils` from `^11.9.0` to `^11.11.0` ([#9074](https://github.com/MetaMask/core/pull/9074))
Expand Down
28 changes: 28 additions & 0 deletions packages/base-data-service/src/BaseDataService.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import { cleanAll } from 'nock';

import { ExampleDataService, serviceName } from '../tests/ExampleDataService';
import {
mockAddFollowerRequest,
mockAssets,
mockTransactionsPage1,
mockTransactionsPage2,
Expand All @@ -23,6 +24,7 @@ const MOCK_ASSETS = [

describe('BaseDataService', () => {
beforeEach(() => {
mockAddFollowerRequest();
mockAssets();
mockTransactionsPage1();
mockTransactionsPage2();
Expand Down Expand Up @@ -161,6 +163,22 @@ describe('BaseDataService', () => {
);
});

it('handles mutations', async () => {
const messenger = new Messenger({ namespace: serviceName });
const service = new ExampleDataService(messenger);

expect(await service.addFollower('1')).toStrictEqual({
followed: [
{
profileId: '550e8400-e29b-41d4-a716-446655440000',
address: '0x1234567890abcdef1234567890abcdef12345678',
name: 'TraderAlice',
imageUrl: 'https://example.com/avatar.png',
},
],
});
});

it('emits `:cacheUpdated` events when cache entry is removed', async () => {
const messenger = new Messenger({ namespace: serviceName });
const service = new ExampleDataService(messenger);
Expand All @@ -186,6 +204,16 @@ describe('BaseDataService', () => {
);
});

it('does not emit `:cacheUpdated` when a mutation is executed', async () => {
const messenger = new Messenger({ namespace: serviceName });
const service = new ExampleDataService(messenger);
const publishSpy = jest.spyOn(messenger, 'publish');

await service.addFollower('1');

expect(publishSpy).not.toHaveBeenCalled();
});

it('does not emit events after being destroyed', async () => {
const messenger = new Messenger({ namespace: serviceName });
const service = new ExampleDataService(messenger);
Expand Down
32 changes: 32 additions & 0 deletions packages/base-data-service/src/BaseDataService.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import {
InfiniteData,
InvalidateOptions,
InvalidateQueryFilters,
MutationOptions,
OmitKeyof,
QueryClient,
QueryClientConfig,
Expand Down Expand Up @@ -250,6 +251,37 @@ export class BaseDataService<
return result.pages[pageIndex];
}

/**
* Execute a mutation (a request that is expected to change the state of a server).
* Unlike `fetchQuery`, the request will not be cached.
*
* @param options - The options defining the mutation. Keep in mind that `mutationKey` and `mutationFn` are required when using data services.
* Additionally `retry` and `retryDelay` are not available, retries can be customized using the `servicePolicyOptions`.
* @returns The mutation results.
*/
protected async executeMutation<
TData extends Json,
TError = unknown,
TVariables = void,
TContext = unknown,
>(
options: WithRequired<
OmitKeyof<
MutationOptions<TData, TError, TVariables, TContext>,
'retry' | 'retryDelay'
>,
'mutationKey' | 'mutationFn'
>,
): Promise<TData> {
const mutationCache = this.#queryClient.getMutationCache();

@mcmire mcmire Jun 30, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

const mutation = mutationCache.build(this.#queryClient, {
...options,
mutationFn: (context) =>
this.#policy.execute(() => options.mutationFn(context)),

@FrederikBolding FrederikBolding Jul 1, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 mcmire Jul 1, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 FrederikBolding Jul 2, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 mcmire Jul 2, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

});
return await mutation.execute();

@cursor cursor Bot Jun 30, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 603d673. Configure here.

@mcmire mcmire Jul 1, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

/**
* Invalidate queries serviced by this data service.
*
Expand Down
34 changes: 34 additions & 0 deletions packages/base-data-service/tests/ExampleDataService.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,15 @@ export type GetActivityResponse = {
};
};

export type AddFollowerResponse = {
followed: {
profileId: string;
address: string;
name: string;
imageUrl?: string | null;
}[];
};

export type PageParam =
| {
before: string;
Expand All @@ -60,6 +69,8 @@ export class ExampleDataService extends BaseDataService<

readonly #tokensBaseUrl = 'https://tokens.api.cx.metamask.io';

readonly #socialBaseUrl = 'https://social.api.cx.metamask.io';

constructor(messenger: ExampleMessenger) {
super({
name: serviceName,
Expand Down Expand Up @@ -139,6 +150,29 @@ export class ExampleDataService extends BaseDataService<
);
}

async addFollower(followerId: string): Promise<AddFollowerResponse> {

@mcmire mcmire Jun 30, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adapted from SocialService:

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

Line 515 in 552b51e

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

return this.executeMutation<AddFollowerResponse>({
mutationKey: [`${this.name}:addFollower`, followerId],
mutationFn: async () => {
const url = new URL(`${this.#socialBaseUrl}/api/v1/users/me/follows`);

const response = await fetch(url, {
method: 'PUT',
headers: { 'Content-Type': 'application/json' },
body: JSON.stringify({ followerId }),
});

if (!response.ok) {
throw new Error(
`Mutation failed with status code: ${response.status}.`,
);
}

return response.json();
},
});
}

destroy(): void {
super.destroy();
}
Expand Down
20 changes: 20 additions & 0 deletions packages/base-data-service/tests/mocks.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,26 @@ type MockReply = {
body?: nock.Body;
};

export function mockAddFollowerRequest(mockReply?: MockReply): nock.Scope {
const reply = mockReply ?? {
status: 200,
body: {
followed: [
{
profileId: '550e8400-e29b-41d4-a716-446655440000',
address: '0x1234567890abcdef1234567890abcdef12345678',
name: 'TraderAlice',
imageUrl: 'https://example.com/avatar.png',
},
],
Comment thread
cursor[bot] marked this conversation as resolved.
},
};

return nock('https://social.api.cx.metamask.io:443')
.put('/api/v1/users/me/follows', { followerId: '1' })
.reply(reply.status, reply.body);
}

export function mockAssets(mockReply?: MockReply): nock.Scope {
const reply = mockReply ?? {
status: 200,
Expand Down
Loading