From b5a6d3978fdb80398fc04310efe15b3b048d7ee9 Mon Sep 17 00:00:00 2001 From: Michael Barrett Date: Wed, 22 Jan 2025 17:59:18 +0000 Subject: [PATCH] Updated `following` dispatcher to read data from the new `follows` table refs [AP-663](https://linear.app/ghost/issue/AP-663/update-following-dispatcher-to-read-data-from-the-new-follows-table) Updated `following` dispatcher to read data from the new `follows` table --- .../account.service.integration.test.ts | 347 ++++++++++++------ src/account/account.service.ts | 65 +++- src/app.ts | 8 +- src/dispatchers.ts | 66 ++++ 4 files changed, 376 insertions(+), 110 deletions(-) diff --git a/src/account/account.service.integration.test.ts b/src/account/account.service.integration.test.ts index 8eb09235..98dd4548 100644 --- a/src/account/account.service.integration.test.ts +++ b/src/account/account.service.integration.test.ts @@ -44,36 +44,95 @@ describe('AccountService', () => { service = new AccountService(db); }); - it( - 'should create an internal account', - async () => { - const username = 'foobarbaz'; - - const expectedAccount = { - name: ACTOR_DEFAULT_NAME, - username, - bio: ACTOR_DEFAULT_SUMMARY, - avatar_url: ACTOR_DEFAULT_ICON, - url: `https://${site.host}`, - custom_fields: null, - ap_id: `https://${site.host}${AP_BASE_PATH}/users/${username}`, - ap_inbox_url: `https://${site.host}${AP_BASE_PATH}/inbox/${username}`, - ap_outbox_url: `https://${site.host}${AP_BASE_PATH}/outbox/${username}`, - ap_following_url: `https://${site.host}${AP_BASE_PATH}/following/${username}`, - ap_followers_url: `https://${site.host}${AP_BASE_PATH}/followers/${username}`, - ap_liked_url: `https://${site.host}${AP_BASE_PATH}/liked/${username}`, + describe('createInternalAccount', () => { + it( + 'should create an internal account', + async () => { + const username = 'foobarbaz'; + + const expectedAccount = { + name: ACTOR_DEFAULT_NAME, + username, + bio: ACTOR_DEFAULT_SUMMARY, + avatar_url: ACTOR_DEFAULT_ICON, + url: `https://${site.host}`, + custom_fields: null, + ap_id: `https://${site.host}${AP_BASE_PATH}/users/${username}`, + ap_inbox_url: `https://${site.host}${AP_BASE_PATH}/inbox/${username}`, + ap_outbox_url: `https://${site.host}${AP_BASE_PATH}/outbox/${username}`, + ap_following_url: `https://${site.host}${AP_BASE_PATH}/following/${username}`, + ap_followers_url: `https://${site.host}${AP_BASE_PATH}/followers/${username}`, + ap_liked_url: `https://${site.host}${AP_BASE_PATH}/liked/${username}`, + ap_shared_inbox_url: null, + }; + + const account = await service.createInternalAccount( + site, + username, + ); + + // Assert the created account was returned + expect(account).toMatchObject(expectedAccount); + expect(account.id).toBeGreaterThan(0); + expect(account.ap_public_key).toBeDefined(); + expect(account.ap_public_key).toContain('key_ops'); + expect(account.ap_private_key).toBeDefined(); + expect(account.ap_private_key).toContain('key_ops'); + + // Assert the account was inserted into the database + const accounts = await db(TABLE_ACCOUNTS).select('*'); + + expect(accounts).toHaveLength(1); + + const dbAccount = accounts[0]; + + expect(dbAccount).toMatchObject(expectedAccount); + + // Assert the user was inserted into the database + const users = await db(TABLE_USERS).select('*'); + + expect(users).toHaveLength(1); + + const dbUser = users[0]; + + expect(dbUser.account_id).toBe(account.id); + expect(dbUser.site_id).toBe(site.id); + }, + 1000 * 10, // Increase timeout to 10 seconds as 5 seconds seems to be too short on CI + ); + }); + + describe('createExternalAccount', () => { + it('should create an external account', async () => { + const accountData: ExternalAccountData = { + username: 'external-account', + name: 'External Account', + bio: 'External Account Bio', + avatar_url: 'https://example.com/avatars/external-account.png', + banner_image_url: + 'https://example.com/banners/external-account.png', + url: 'https://example.com/users/external-account', + custom_fields: {}, + ap_id: 'https://example.com/activitypub/users/external-account', + ap_inbox_url: + 'https://example.com/activitypub/inbox/external-account', + ap_outbox_url: + 'https://example.com/activitypub/outbox/external-account', + ap_following_url: + 'https://example.com/activitypub/following/external-account', + ap_followers_url: + 'https://example.com/activitypub/followers/external-account', + ap_liked_url: + 'https://example.com/activitypub/liked/external-account', ap_shared_inbox_url: null, + ap_public_key: '', }; - const account = await service.createInternalAccount(site, username); + const account = await service.createExternalAccount(accountData); // Assert the created account was returned - expect(account).toMatchObject(expectedAccount); + expect(account).toMatchObject(accountData); expect(account.id).toBeGreaterThan(0); - expect(account.ap_public_key).toBeDefined(); - expect(account.ap_public_key).toContain('key_ops'); - expect(account.ap_private_key).toBeDefined(); - expect(account.ap_private_key).toContain('key_ops'); // Assert the account was inserted into the database const accounts = await db(TABLE_ACCOUNTS).select('*'); @@ -82,108 +141,188 @@ describe('AccountService', () => { const dbAccount = accounts[0]; - expect(dbAccount).toMatchObject(expectedAccount); - - // Assert the user was inserted into the database - const users = await db(TABLE_USERS).select('*'); - - expect(users).toHaveLength(1); - - const dbUser = users[0]; - - expect(dbUser.account_id).toBe(account.id); - expect(dbUser.site_id).toBe(site.id); - }, - 1000 * 10, // Increase timeout to 10 seconds as 5 seconds seems to be too short on CI - ); - - it('should create an external account', async () => { - const accountData: ExternalAccountData = { - username: 'external-account', - name: 'External Account', - bio: 'External Account Bio', - avatar_url: 'https://example.com/avatars/external-account.png', - banner_image_url: - 'https://example.com/banners/external-account.png', - url: 'https://example.com/users/external-account', - custom_fields: {}, - ap_id: 'https://example.com/activitypub/users/external-account', - ap_inbox_url: - 'https://example.com/activitypub/inbox/external-account', - ap_outbox_url: - 'https://example.com/activitypub/outbox/external-account', - ap_following_url: - 'https://example.com/activitypub/following/external-account', - ap_followers_url: - 'https://example.com/activitypub/followers/external-account', - ap_liked_url: - 'https://example.com/activitypub/liked/external-account', - ap_shared_inbox_url: null, - ap_public_key: '', - }; + expect(dbAccount).toMatchObject(accountData); + }); + }); - const account = await service.createExternalAccount(accountData); + describe('recordAccountFollow', () => { + it('should record an account being followed', async () => { + const account = await service.createInternalAccount( + site, + 'account', + ); + const follower = await service.createInternalAccount( + site, + 'follower', + ); - // Assert the created account was returned - expect(account).toMatchObject(accountData); - expect(account.id).toBeGreaterThan(0); + await service.recordAccountFollow(account, follower); - // Assert the account was inserted into the database - const accounts = await db(TABLE_ACCOUNTS).select('*'); + // Assert the follow was inserted into the database + const follows = await db(TABLE_FOLLOWS).select('*'); - expect(accounts).toHaveLength(1); + expect(follows).toHaveLength(1); - const dbAccount = accounts[0]; + const follow = follows[0]; - expect(dbAccount).toMatchObject(accountData); - }); + expect(follow.following_id).toBe(account.id); + expect(follow.follower_id).toBe(follower.id); + }); - it('should record an account being followed', async () => { - const account = await service.createInternalAccount(site, 'account'); - const follower = await service.createInternalAccount(site, 'follower'); + it('should not record duplicate follows', async () => { + const account = await service.createInternalAccount( + site, + 'account', + ); + const follower = await service.createInternalAccount( + site, + 'follower', + ); - await service.recordAccountFollow(account, follower); + await service.recordAccountFollow(account, follower); - // Assert the follow was inserted into the database - const follows = await db(TABLE_FOLLOWS).select('*'); + const firstFollow = await db(TABLE_FOLLOWS) + .where({ id: 1 }) + .first(); - expect(follows).toHaveLength(1); + await service.recordAccountFollow(account, follower); - const follow = follows[0]; + // Assert the follow was inserted into the database only once + const follows = await db(TABLE_FOLLOWS).select('*'); - expect(follow.following_id).toBe(account.id); - expect(follow.follower_id).toBe(follower.id); - }); + expect(follows).toHaveLength(1); - it('should not record duplicate follows', async () => { - const account = await service.createInternalAccount(site, 'account'); - const follower = await service.createInternalAccount(site, 'follower'); + // Assert the data was not changed + const follow = follows[0]; - await service.recordAccountFollow(account, follower); + expect(follow.following_id).toBe(firstFollow.following_id); + expect(follow.follower_id).toBe(firstFollow.follower_id); + expect(follow.created_at).toStrictEqual(firstFollow.created_at); + expect(follow.updated_at).toStrictEqual(firstFollow.updated_at); + }); + }); - const firstFollow = await db(TABLE_FOLLOWS).where({ id: 1 }).first(); + describe('getAccountByApId', () => { + it('should retrieve an account by its ActivityPub ID', async () => { + const account = await service.createInternalAccount( + site, + 'account', + ); - await service.recordAccountFollow(account, follower); + const retrievedAccount = await service.getAccountByApId( + account.ap_id, + ); - // Assert the follow was inserted into the database only once - const follows = await db(TABLE_FOLLOWS).select('*'); + // Assert the retrieved account matches the created account + expect(retrievedAccount).toMatchObject(account); + }); + }); - expect(follows).toHaveLength(1); + describe('getDefaultAccountForSite', () => { + it('should retrieve the default account for a site', async () => { + const account = await service.createInternalAccount( + site, + 'account', + ); - // Assert the data was not changed - const follow = follows[0]; + const defaultAccount = await service.getDefaultAccountForSite(site); - expect(follow.following_id).toBe(firstFollow.following_id); - expect(follow.follower_id).toBe(firstFollow.follower_id); - expect(follow.created_at).toStrictEqual(firstFollow.created_at); - expect(follow.updated_at).toStrictEqual(firstFollow.updated_at); - }); + expect(defaultAccount).toMatchObject(account); + }); - it('should retrieve an account by its ActivityPub ID', async () => { - const account = await service.createInternalAccount(site, 'account'); + it('should throw an error if multiple users are found for a site', async () => { + await service.createInternalAccount(site, 'account1'); + await service.createInternalAccount(site, 'account2'); - const retrievedAccount = await service.getAccountByApId(account.ap_id); + await expect( + service.getDefaultAccountForSite(site), + ).rejects.toThrow(`Multiple users found for site: ${site.id}`); + }); + + it('should throw an error if no users are found for a site', async () => { + await expect( + service.getDefaultAccountForSite(site), + ).rejects.toThrow(`No user found for site: ${site.id}`); + }); + }); - expect(retrievedAccount).toMatchObject(account); + describe('getFollowedAccounts', () => { + it( + 'should retrieve the accounts that an account follows', + async () => { + const account = await service.createInternalAccount( + site, + 'account', + ); + const follower1 = await service.createInternalAccount( + site, + 'follower1', + ); + const follower2 = await service.createInternalAccount( + site, + 'follower2', + ); + const follower3 = await service.createInternalAccount( + site, + 'follower3', + ); + + await service.recordAccountFollow(account, follower1); + await service.recordAccountFollow(account, follower2); + await service.recordAccountFollow(account, follower3); + + // Get a page of followed accounts and assert the requested fields are returned + const followedAccounts = await service.getFollowedAccounts( + account, + { + limit: 2, + offset: 0, + fields: ['id', 'username', 'ap_inbox_url'], + }, + ); + + expect(followedAccounts).toHaveLength(2); + expect(followedAccounts[0]).toMatchObject({ + id: follower3.id, + username: follower3.username, + }); + expect(followedAccounts[0].ap_inbox_url).toBeDefined(); + + expect(followedAccounts[1]).toMatchObject({ + id: follower2.id, + username: follower2.username, + }); + expect(followedAccounts[1].ap_inbox_url).toBeDefined(); + + // Get the next page of followed accounts and assert the requested fields are returned + const nextFollowedAccounts = await service.getFollowedAccounts( + account, + { + limit: 2, + offset: 2, + fields: ['id', 'username', 'ap_inbox_url'], + }, + ); + + expect(nextFollowedAccounts).toHaveLength(1); + expect(nextFollowedAccounts[0]).toMatchObject({ + id: follower1.id, + username: follower1.username, + }); + expect(nextFollowedAccounts[0].ap_inbox_url).toBeDefined(); + + // Get another page that will return no results and assert the + // results are empty + const nextFollowedAccountsEmpty = + await service.getFollowedAccounts(account, { + limit: 2, + offset: 3, + fields: ['id', 'username', 'ap_inbox_url'], + }); + + expect(nextFollowedAccountsEmpty).toHaveLength(0); + }, + 1000 * 10, // Increase timeout to 10 seconds as 5 seconds seems to be too short on CI + ); }); }); diff --git a/src/account/account.service.ts b/src/account/account.service.ts index badb4fc4..58626422 100644 --- a/src/account/account.service.ts +++ b/src/account/account.service.ts @@ -12,6 +12,12 @@ import { } from '../constants'; import type { Account, ExternalAccountData, Site } from './types'; +interface GetFollowedAccountsOptions { + limit: number; + offset: number; + fields: (keyof Account)[]; +} + export class AccountService { /** * @param db Database client @@ -114,8 +120,61 @@ export class AccountService { return null; } - return ( - (await this.db(TABLE_ACCOUNTS).where('ap_id', apId).first()) ?? null - ); + return await this.db(TABLE_ACCOUNTS).where('ap_id', apId).first(); + } + + /** + * Get the default account for a site + * + * @param site Site + */ + async getDefaultAccountForSite(site: Site): Promise { + const users = await this.db(TABLE_USERS).where('site_id', site.id); + + if (users.length === 0) { + throw new Error(`No user found for site: ${site.id}`); + } + + if (users.length > 1) { + throw new Error(`Multiple users found for site: ${site.id}`); + } + + const user = users[0]; + + // We can safely assume that there is an account for the user due to + // the foreign key constraint on the users table + return await this.db(TABLE_ACCOUNTS) + .where('id', user.account_id) + .first(); + } + + /** + * Get the accounts that the provided account is following + * + * The results are ordered in reverse chronological order + * + * @param account Account + * @param options Options for the query + */ + async getFollowedAccounts( + account: Account, + options: GetFollowedAccountsOptions, // @TODO: Make this optional + ): Promise { + return await this.db(TABLE_FOLLOWS) + .select(options.fields.map((field) => `${TABLE_ACCOUNTS}.${field}`)) + .where('following_id', account.id) + .innerJoin( + TABLE_ACCOUNTS, + `${TABLE_ACCOUNTS}.id`, + `${TABLE_FOLLOWS}.follower_id`, + ) + .limit(options.limit) + .offset(options.offset) + // order by the date created at in descending order and then by the + // account id in descending order to ensure the most recent follows + // are returned first (i.e in case multiple follows were created at + // the same time) + .orderBy(`${TABLE_FOLLOWS}.created_at`, 'desc') + .orderBy(`${TABLE_ACCOUNTS}.id`, 'desc'); } } diff --git a/src/app.ts b/src/app.ts index 5c08fe7f..a7477918 100644 --- a/src/app.ts +++ b/src/app.ts @@ -44,12 +44,13 @@ import { articleDispatcher, createDispatcher, createFollowHandler, + createFollowingDispatcher, followDispatcher, followersCounter, followersDispatcher, followersFirstCursor, followingCounter, - followingDispatcher, + // followingDispatcher, followingFirstCursor, handleAccept, handleAnnounce, @@ -211,6 +212,7 @@ export type FedifyRequestContext = RequestContext; export const db = await KnexKvStore.create(client, 'key_value'); const accountService = new AccountService(client); +const siteService = new SiteService(client, accountService); /** Fedify */ @@ -281,7 +283,8 @@ fedify fedify .setFollowingDispatcher( '/.ghost/activitypub/following/{handle}', - spanWrapper(followingDispatcher), + // spanWrapper(followingDispatcher), + spanWrapper(createFollowingDispatcher(siteService, accountService)), ) .setCounter(followingCounter) .setFirstCursor(followingFirstCursor); @@ -617,7 +620,6 @@ app.use(async (ctx, next) => { await next(); }); -const siteService = new SiteService(client, accountService); // This needs to go before the middleware which loads the site // Because the site doesn't always exist - this is how it's created app.get('/.ghost/activitypub/site', getSiteDataHandler(siteService)); diff --git a/src/dispatchers.ts b/src/dispatchers.ts index e3fdd813..9ae08223 100644 --- a/src/dispatchers.ts +++ b/src/dispatchers.ts @@ -21,6 +21,7 @@ import { verifyObject, } from '@fedify/fedify'; import * as Sentry from '@sentry/node'; +import type { SiteService } from 'site/site.service'; import { v4 as uuidv4 } from 'uuid'; import type { AccountService } from './account/account.service'; import { mapActorToExternalAccountData } from './account/utils'; @@ -648,6 +649,71 @@ export async function followingDispatcher( }; } +/** + * This logic duplicates the logic in followingDispatcher, the + * main difference being that is uses the account service to retrieve the + * following.followingDispatcher will eventually be removed in favour of this + */ +export function createFollowingDispatcher( + siteService: SiteService, + accountService: AccountService, +) { + return async function dispatchFollowing( + ctx: RequestContext, + handle: string, + cursor: string | null, + ) { + ctx.data.logger.info('Following Dispatcher'); + + const pageSize = Number.parseInt( + process.env.ACTIVITYPUB_COLLECTION_PAGE_SIZE || '', + ); + + if (Number.isNaN(pageSize)) { + throw new Error(`Page size: ${pageSize} is not valid`); + } + + const offset = Number.parseInt(cursor ?? '0'); + let nextCursor: string | null = null; + + const site = await siteService.getSiteByHost( + ctx.request.headers.get('host')!, + ); + + if (!site) { + throw new Error('Site not found'); + } + + const siteDefaultAccount = + await accountService.getDefaultAccountForSite(site); + + if (!siteDefaultAccount) { + throw new Error('Default account for site not found'); + } + + const results = await accountService.getFollowedAccounts( + siteDefaultAccount, + { + fields: ['ap_id'], + limit: pageSize, + offset, + }, + ); + + nextCursor = + results.length > offset + pageSize + ? (offset + pageSize).toString() + : null; + + ctx.data.logger.info('Following results', { results }); + + return { + items: results.map((result) => new URL(result.ap_id)), + nextCursor, + }; + }; +} + export async function followingCounter( ctx: RequestContext, handle: string,