Skip to content

Commit 8817595

Browse files
mike182ukmike182uk
authored
Fixed N+1 query in AccountFollowsView domain block checks (#1486)
ref https://linear.app/ghost/issue/BER-3013 Replaced per-account `domainIsBlocked` calls with a single `getBlockedDomains` query that returns all blocked domains upfront as a `Set` for `O(1)` lookups
1 parent 393a72d commit 8817595

File tree

3 files changed

+100
-122
lines changed

3 files changed

+100
-122
lines changed

‎src/http/api/views/account.follows.view.ts‎

Lines changed: 15 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -118,28 +118,25 @@ export class AccountFollowsView {
118118
? (next + FOLLOWS_LIMIT).toString()
119119
: null;
120120

121+
const blockedDomains = await this.moderationService.getBlockedDomains(
122+
siteDefaultAccount.id,
123+
);
124+
121125
const accounts: MinimalAccountDTO[] = [];
122126

123127
for (const result of results) {
124-
const domainBlockedByMe =
125-
await this.moderationService.domainIsBlocked(
126-
siteDefaultAccount.id,
127-
new URL(result.ap_id),
128-
);
128+
const apIdUrl = new URL(result.ap_id);
129129

130130
accounts.push({
131131
id: result.ap_id,
132132
apId: result.ap_id,
133133
name: result.name || '',
134-
handle: getAccountHandle(
135-
new URL(result.ap_id).host,
136-
result.username,
137-
),
134+
handle: getAccountHandle(apIdUrl.host, result.username),
138135
avatarUrl: result.avatar_url || '',
139136
isFollowing: !!result.followed_by_me,
140137
followedByMe: !!result.followed_by_me,
141138
blockedByMe: !!result.blocked_by_me,
142-
domainBlockedByMe,
139+
domainBlockedByMe: blockedDomains.has(apIdUrl.hostname),
143140
});
144141
}
145142

@@ -361,32 +358,32 @@ export class AccountFollowsView {
361358
]),
362359
);
363360

361+
const blockedDomains = await this.moderationService.getBlockedDomains(
362+
siteDefaultAccount.id,
363+
);
364+
364365
for await (const item of followsList) {
365366
try {
366367
const followeeAccount = accountsMap.get(
367368
createHash('sha256').update(item.href).digest('hex'),
368369
);
369370

370371
if (followeeAccount) {
371-
const domainBlockedByMe =
372-
await this.moderationService.domainIsBlocked(
373-
siteDefaultAccount.id,
374-
new URL(followeeAccount.ap_id),
375-
);
372+
const apIdUrl = new URL(followeeAccount.ap_id);
376373

377374
accounts.push({
378375
id: followeeAccount.ap_id,
379376
apId: followeeAccount.ap_id,
380377
name: followeeAccount.name || '',
381378
handle: getAccountHandle(
382-
new URL(followeeAccount.ap_id).host,
379+
apIdUrl.host,
383380
followeeAccount.username,
384381
),
385382
avatarUrl: followeeAccount.avatar_url || '',
386383
isFollowing: !!followeeAccount.followed_by_me,
387384
followedByMe: !!followeeAccount.followed_by_me,
388385
blockedByMe: !!followeeAccount.blocked_by_me,
389-
domainBlockedByMe,
386+
domainBlockedByMe: blockedDomains.has(apIdUrl.hostname),
390387
});
391388
} else {
392389
const followsActorObj = await lookupObject(item.href, {
@@ -405,12 +402,6 @@ export class AccountFollowsView {
405402
continue;
406403
}
407404

408-
const domainBlockedByMe =
409-
await this.moderationService.domainIsBlocked(
410-
siteDefaultAccount.id,
411-
item,
412-
);
413-
414405
accounts.push({
415406
id: followsActor.id,
416407
apId: followsActor.id,
@@ -423,7 +414,7 @@ export class AccountFollowsView {
423414
isFollowing: false,
424415
followedByMe: false,
425416
blockedByMe: false,
426-
domainBlockedByMe,
417+
domainBlockedByMe: blockedDomains.has(item.hostname),
427418
});
428419
}
429420
} catch (_err) {

‎src/moderation/moderation.service.integration.test.ts‎

Lines changed: 80 additions & 87 deletions
Original file line numberDiff line numberDiff line change
@@ -317,134 +317,127 @@ describe('ModerationService', () => {
317317
});
318318
});
319319

320-
describe('domainIsBlocked', () => {
321-
it('should return true when a domain is blocked', async () => {
322-
const [[aliceAccount], [bobAccount]] = await Promise.all([
323-
fixtureManager.createInternalAccount(),
324-
fixtureManager.createInternalAccount(
325-
null,
326-
'blocked-domain.com',
320+
describe('getBlockedDomains', () => {
321+
it('should return blocked domains for an account', async () => {
322+
const [[aliceAccount], bobAccount, charlieAccount] =
323+
await Promise.all([
324+
fixtureManager.createInternalAccount(),
325+
fixtureManager.createExternalAccount(
326+
'https://blocked-domain.com/users/bob',
327+
),
328+
fixtureManager.createExternalAccount(
329+
'https://another-blocked.org/users/charlie',
330+
),
331+
]);
332+
333+
await Promise.all([
334+
fixtureManager.createDomainBlock(aliceAccount, bobAccount.apId),
335+
fixtureManager.createDomainBlock(
336+
aliceAccount,
337+
charlieAccount.apId,
327338
),
328339
]);
329340

330-
// Alice blocks bob's domain
331-
await fixtureManager.createDomainBlock(
332-
aliceAccount,
333-
bobAccount.apId,
341+
const blockedDomains = await moderationService.getBlockedDomains(
342+
aliceAccount.id,
334343
);
335344

336-
const isBlocked = await moderationService.domainIsBlocked(
345+
expect(blockedDomains.size).toBe(2);
346+
expect(blockedDomains.has('blocked-domain.com')).toBe(true);
347+
expect(blockedDomains.has('another-blocked.org')).toBe(true);
348+
});
349+
350+
it('should return empty set when no domains are blocked', async () => {
351+
const [[aliceAccount]] = await Promise.all([
352+
fixtureManager.createInternalAccount(),
353+
]);
354+
355+
const blockedDomains = await moderationService.getBlockedDomains(
337356
aliceAccount.id,
338-
bobAccount.apId,
339357
);
340358

341-
expect(isBlocked).toBe(true);
359+
expect(blockedDomains.size).toBe(0);
342360
});
343361

344-
it('should return false when a domain is not blocked', async () => {
345-
const [[aliceAccount], [bobAccount], [charlieAccount]] =
346-
await Promise.all([
347-
fixtureManager.createInternalAccount(),
348-
fixtureManager.createInternalAccount(
349-
null,
350-
'blocked-domain.com',
351-
),
352-
fixtureManager.createInternalAccount(
353-
null,
354-
'not-blocked-domain.com',
355-
),
356-
]);
362+
it('should return domains in lowercase for case-insensitive matching', async () => {
363+
const [[aliceAccount], bobAccount] = await Promise.all([
364+
fixtureManager.createInternalAccount(),
365+
fixtureManager.createExternalAccount(
366+
'https://example.com/users/bob',
367+
),
368+
]);
357369

358-
// Alice blocks bob's domain
359370
await fixtureManager.createDomainBlock(
360371
aliceAccount,
361372
bobAccount.apId,
362373
);
363374

364-
const notBlockedDomain = await moderationService.domainIsBlocked(
375+
const blockedDomains = await moderationService.getBlockedDomains(
365376
aliceAccount.id,
366-
charlieAccount.apId,
367377
);
368378

369-
expect(notBlockedDomain).toBe(false);
379+
// Domains are lowercased to match URL.hostname (which is always lowercase per URL standard)
380+
expect(blockedDomains.has('example.com')).toBe(true);
381+
382+
// Verify that URL.hostname lookups work correctly (simulating caller behavior)
383+
const ucDomainUrl = new URL('https://EXAMPLE.COM/users/test');
384+
385+
expect(blockedDomains.has(ucDomainUrl.hostname)).toBe(true);
370386
});
371387

372-
it('should handle different subdomains correctly', async () => {
373-
const [[aliceAccount], [bobAccount]] = await Promise.all([
374-
fixtureManager.createInternalAccount(),
375-
fixtureManager.createInternalAccount(null, 'example.com'),
376-
]);
388+
it('should only return domains blocked by the specified account', async () => {
389+
const [[aliceAccount], [bobAccount], charlieAccount] =
390+
await Promise.all([
391+
fixtureManager.createInternalAccount(),
392+
fixtureManager.createInternalAccount(),
393+
fixtureManager.createExternalAccount(
394+
'https://blocked-domain.com/users/charlie',
395+
),
396+
]);
377397

378-
// Alice blocks example.com
398+
// Only alice blocks the domain
379399
await fixtureManager.createDomainBlock(
380400
aliceAccount,
381-
bobAccount.apId,
401+
charlieAccount.apId,
382402
);
383403

384-
// Test with different subdomains
385-
const subdomain1 = new URL('https://sub1.example.com');
386-
const subdomain2 = new URL('https://sub2.example.com');
387-
const differentDomain = new URL('https://different.org');
388-
389-
// Since we're explicitly checking the domain host property,
390-
// subdomains will be treated as different domains
391-
const isDomainBlocked = await moderationService.domainIsBlocked(
392-
aliceAccount.id,
393-
bobAccount.apId,
404+
const aliceBlockedDomains =
405+
await moderationService.getBlockedDomains(aliceAccount.id);
406+
const bobBlockedDomains = await moderationService.getBlockedDomains(
407+
bobAccount.id,
394408
);
395-
const isSubdomain1Blocked = await moderationService.domainIsBlocked(
396-
aliceAccount.id,
397-
subdomain1,
398-
);
399-
const isSubdomain2Blocked = await moderationService.domainIsBlocked(
400-
aliceAccount.id,
401-
subdomain2,
402-
);
403-
const isDifferentDomainBlocked =
404-
await moderationService.domainIsBlocked(
405-
aliceAccount.id,
406-
differentDomain,
407-
);
408409

409-
expect(isDomainBlocked).toBe(true);
410-
expect(isSubdomain1Blocked).toBe(false);
411-
expect(isSubdomain2Blocked).toBe(false);
412-
expect(isDifferentDomainBlocked).toBe(false);
410+
expect(aliceBlockedDomains.size).toBe(1);
411+
expect(bobBlockedDomains.size).toBe(0);
413412
});
414413

415-
it('should handle case insensitivity', async () => {
416-
const [[aliceAccount], [bobAccount]] = await Promise.all([
414+
it('should treat subdomains as distinct from parent domains', async () => {
415+
const [[aliceAccount], bobAccount] = await Promise.all([
417416
fixtureManager.createInternalAccount(),
418-
fixtureManager.createInternalAccount(null, 'Example.COM'),
417+
fixtureManager.createExternalAccount(
418+
'https://example.com/users/bob',
419+
),
419420
]);
420421

421-
// Alice blocks Example.COM
422+
// Alice blocks example.com
422423
await fixtureManager.createDomainBlock(
423424
aliceAccount,
424425
bobAccount.apId,
425426
);
426427

427-
// Test with different case variations
428-
const lowercase = new URL('https://example.com');
429-
const uppercase = new URL('https://EXAMPLE.COM');
430-
const mixedCase = new URL('https://ExAmPlE.cOm');
431-
432-
const isLowercaseBlocked = await moderationService.domainIsBlocked(
433-
aliceAccount.id,
434-
lowercase,
435-
);
436-
const isUppercaseBlocked = await moderationService.domainIsBlocked(
428+
const blockedDomains = await moderationService.getBlockedDomains(
437429
aliceAccount.id,
438-
uppercase,
439-
);
440-
const isMixedCaseBlocked = await moderationService.domainIsBlocked(
441-
aliceAccount.id,
442-
mixedCase,
443430
);
444431

445-
expect(isLowercaseBlocked).toBe(true);
446-
expect(isUppercaseBlocked).toBe(true);
447-
expect(isMixedCaseBlocked).toBe(true);
432+
// The exact domain should be blocked
433+
expect(blockedDomains.has('example.com')).toBe(true);
434+
435+
// Subdomains should NOT be blocked (they are distinct domains)
436+
expect(blockedDomains.has('sub1.example.com')).toBe(false);
437+
expect(blockedDomains.has('sub2.example.com')).toBe(false);
438+
439+
// Other domains should NOT be blocked
440+
expect(blockedDomains.has('different.org')).toBe(false);
448441
});
449442
});
450443
});

‎src/moderation/moderation.service.ts‎

Lines changed: 5 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -124,17 +124,11 @@ export class ModerationService {
124124
return domainBlock === undefined;
125125
}
126126

127-
async domainIsBlocked(
128-
targetAccountId: number,
129-
domain: URL,
130-
): Promise<boolean> {
131-
const block = await this.db('domain_blocks')
132-
.where('blocker_id', targetAccountId)
133-
.whereRaw('domain_hash = UNHEX(SHA2(LOWER(?), 256))', [
134-
domain.hostname,
135-
])
136-
.first();
127+
async getBlockedDomains(accountId: number): Promise<Set<string>> {
128+
const blocks = await this.db('domain_blocks')
129+
.where('blocker_id', accountId)
130+
.select('domain');
137131

138-
return block !== undefined;
132+
return new Set(blocks.map((block) => block.domain.toLowerCase()));
139133
}
140134
}

0 commit comments

Comments
 (0)