fix: stop unified-mailbox from mutating client-returned email objects
fetchUnifiedEmails, fanOutUnifiedQuery and the cross-account fanOutCrossQuery stamped accountId/accountLabel/source* directly onto each email object returned by the per-account client. Those objects are shared references; mutating them in place could surprise any caller that retained them (and corrupt an account-state snapshot). Decorate shallow copies instead, at all three fan-out sites. The original fix/unified-mailbox-no-mutation branch predated the cross-account "All accounts" feature and only covered two sites; this re-applies the fix to main's current code, including the third (shared/group) fan-out site, and preserves all five stamped fields. Flips the characterisation test to assert the client's object is left untouched.
This commit is contained in:
committed by
Linus Rath
parent
de56229ef2
commit
70aaf0aac1
@@ -120,16 +120,17 @@ describe('fetchUnifiedEmails', () => {
|
|||||||
expect(result).toEqual({ emails: [], total: 0, hasMore: false, errors: new Map() });
|
expect(result).toEqual({ emails: [], total: 0, hasMore: false, errors: new Map() });
|
||||||
});
|
});
|
||||||
|
|
||||||
it('CHARACTERISATION: mutates the source email objects in place (shared reference)', async () => {
|
it('does NOT mutate the source email objects (decorates copies)', async () => {
|
||||||
const original = makeEmail('m1', '2026-01-01T00:00:00Z');
|
const original = makeEmail('m1', '2026-01-01T00:00:00Z');
|
||||||
const acc = makeAccount(
|
const acc = makeAccount(
|
||||||
{ accountId: 'A', accountLabel: 'Label A', mailboxes: [makeMailbox({ role: 'inbox' })] },
|
{ accountId: 'A', accountLabel: 'Label A', mailboxes: [makeMailbox({ role: 'inbox' })] },
|
||||||
{ getEmails: vi.fn(async (): Promise<FetchResult> => ({ emails: [original], total: 1, hasMore: false })) },
|
{ getEmails: vi.fn(async (): Promise<FetchResult> => ({ emails: [original], total: 1, hasMore: false })) },
|
||||||
);
|
);
|
||||||
await fetchUnifiedEmails([acc], 'inbox', 20, 0);
|
const res = await fetchUnifiedEmails([acc], 'inbox', 20, 0);
|
||||||
// The very object passed back by the client was mutated, not a copy.
|
// The returned email carries the account info, but the client's object is untouched.
|
||||||
expect(original.accountId).toBe('A');
|
expect(res.emails[0]).toMatchObject({ id: 'm1', accountId: 'A', accountLabel: 'Label A' });
|
||||||
expect(original.accountLabel).toBe('Label A');
|
expect('accountId' in original).toBe(false);
|
||||||
|
expect('accountLabel' in original).toBe(false);
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
|
|||||||
+32
-25
@@ -119,16 +119,19 @@ export async function fetchUnifiedEmails(
|
|||||||
|
|
||||||
const { account, result } = outcome.value;
|
const { account, result } = outcome.value;
|
||||||
|
|
||||||
// Decorate each email with the source account info.
|
// Decorate each email with the source account info. The per-account client
|
||||||
for (const email of result.emails) {
|
// returns shared object references; decorate shallow copies instead of
|
||||||
email.accountId = account.accountId;
|
// mutating them in place so retained callers/snapshots aren't corrupted.
|
||||||
email.accountLabel = account.accountLabel;
|
const decorated = result.emails.map((email) => ({
|
||||||
email.sourceClientAccountId = account.clientAccountId;
|
...email,
|
||||||
email.sourceAccountId = account.jmapAccountId;
|
accountId: account.accountId,
|
||||||
email.sourceFolder = resolveSourceFolderName(email, account.mailboxes);
|
accountLabel: account.accountLabel,
|
||||||
}
|
sourceClientAccountId: account.clientAccountId,
|
||||||
|
sourceAccountId: account.jmapAccountId,
|
||||||
|
sourceFolder: resolveSourceFolderName(email, account.mailboxes),
|
||||||
|
}));
|
||||||
|
|
||||||
mergedEmails = mergedEmails.concat(result.emails);
|
mergedEmails = mergedEmails.concat(decorated);
|
||||||
totalSum += result.total;
|
totalSum += result.total;
|
||||||
if (result.hasMore) {
|
if (result.hasMore) {
|
||||||
anyHasMore = true;
|
anyHasMore = true;
|
||||||
@@ -247,14 +250,16 @@ async function fanOutUnifiedQuery(
|
|||||||
for (const outcome of results) {
|
for (const outcome of results) {
|
||||||
if (outcome.status !== 'fulfilled' || outcome.value === null) continue;
|
if (outcome.status !== 'fulfilled' || outcome.value === null) continue;
|
||||||
const { account, result } = outcome.value;
|
const { account, result } = outcome.value;
|
||||||
for (const email of result.emails) {
|
// Decorate shallow copies, not the shared client-returned objects.
|
||||||
email.accountId = account.accountId;
|
const decorated = result.emails.map((email) => ({
|
||||||
email.accountLabel = account.accountLabel;
|
...email,
|
||||||
email.sourceClientAccountId = account.clientAccountId;
|
accountId: account.accountId,
|
||||||
email.sourceAccountId = account.jmapAccountId;
|
accountLabel: account.accountLabel,
|
||||||
email.sourceFolder = resolveSourceFolderName(email, account.mailboxes);
|
sourceClientAccountId: account.clientAccountId,
|
||||||
}
|
sourceAccountId: account.jmapAccountId,
|
||||||
mergedEmails = mergedEmails.concat(result.emails);
|
sourceFolder: resolveSourceFolderName(email, account.mailboxes),
|
||||||
|
}));
|
||||||
|
mergedEmails = mergedEmails.concat(decorated);
|
||||||
totalSum += result.total;
|
totalSum += result.total;
|
||||||
if (result.hasMore) anyHasMore = true;
|
if (result.hasMore) anyHasMore = true;
|
||||||
}
|
}
|
||||||
@@ -383,14 +388,16 @@ async function fanOutCrossQuery(
|
|||||||
for (const outcome of results) {
|
for (const outcome of results) {
|
||||||
if (outcome.status !== 'fulfilled' || outcome.value === null) continue;
|
if (outcome.status !== 'fulfilled' || outcome.value === null) continue;
|
||||||
const { account, result } = outcome.value;
|
const { account, result } = outcome.value;
|
||||||
for (const email of result.emails) {
|
// Decorate shallow copies, not the shared client-returned objects.
|
||||||
email.accountId = account.accountId;
|
const decorated = result.emails.map((email) => ({
|
||||||
email.accountLabel = account.accountLabel;
|
...email,
|
||||||
email.sourceClientAccountId = account.clientAccountId;
|
accountId: account.accountId,
|
||||||
email.sourceAccountId = account.jmapAccountId;
|
accountLabel: account.accountLabel,
|
||||||
email.sourceFolder = resolveSourceFolderName(email, account.mailboxes);
|
sourceClientAccountId: account.clientAccountId,
|
||||||
}
|
sourceAccountId: account.jmapAccountId,
|
||||||
mergedEmails = mergedEmails.concat(result.emails);
|
sourceFolder: resolveSourceFolderName(email, account.mailboxes),
|
||||||
|
}));
|
||||||
|
mergedEmails = mergedEmails.concat(decorated);
|
||||||
totalSum += result.total;
|
totalSum += result.total;
|
||||||
if (result.hasMore) anyHasMore = true;
|
if (result.hasMore) anyHasMore = true;
|
||||||
}
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user