From bae616273d455d225d131eb17c56db6c20a0b6b3 Mon Sep 17 00:00:00 2001 From: Chocobozzz Date: Thu, 5 May 2022 10:29:35 +0200 Subject: [PATCH] Convert followers/following in raw SQL queries Prevent weird bug in SQL generation --- server/controllers/api/server/follows.ts | 4 +- server/lib/activitypub/send/send-undo.ts | 2 +- server/models/actor/actor-follow.ts | 146 ++---------------- .../instance-list-followers-query-builder.ts | 69 +++++++++ .../instance-list-following-query-builder.ts | 69 +++++++++ .../shared/actor-follow-table-attributes.ts | 62 ++++++++ .../instance-list-follows-query-builder.ts | 97 ++++++++++++ server/models/shared/abstract-run-query.ts | 4 + server/models/utils.ts | 15 +- .../sql/video/videos-id-list-query-builder.ts | 4 +- .../tests/api/redundancy/manage-redundancy.ts | 11 ++ 11 files changed, 338 insertions(+), 145 deletions(-) create mode 100644 server/models/actor/sql/instance-list-followers-query-builder.ts create mode 100644 server/models/actor/sql/instance-list-following-query-builder.ts create mode 100644 server/models/actor/sql/shared/actor-follow-table-attributes.ts create mode 100644 server/models/actor/sql/shared/instance-list-follows-query-builder.ts diff --git a/server/controllers/api/server/follows.ts b/server/controllers/api/server/follows.ts index c613386b2..9557810b5 100644 --- a/server/controllers/api/server/follows.ts +++ b/server/controllers/api/server/follows.ts @@ -99,7 +99,7 @@ export { async function listFollowing (req: express.Request, res: express.Response) { const serverActor = await getServerActor() const resultList = await ActorFollowModel.listInstanceFollowingForApi({ - id: serverActor.id, + followerId: serverActor.id, start: req.query.start, count: req.query.count, sort: req.query.sort, @@ -159,7 +159,7 @@ async function removeFollowing (req: express.Request, res: express.Response) { const follow = res.locals.follow await sequelizeTypescript.transaction(async t => { - if (follow.state === 'accepted') await sendUndoFollow(follow, t) + if (follow.state === 'accepted') sendUndoFollow(follow, t) // Disable redundancy on unfollowed instances const server = follow.ActorFollowing.Server diff --git a/server/lib/activitypub/send/send-undo.ts b/server/lib/activitypub/send/send-undo.ts index 36d7ef991..442178c42 100644 --- a/server/lib/activitypub/send/send-undo.ts +++ b/server/lib/activitypub/send/send-undo.ts @@ -44,7 +44,7 @@ function sendUndoFollow (actorFollow: MActorFollowActors, t: Transaction) { const followActivity = buildFollowActivity(actorFollow.url, me, following) const undoActivity = undoActivityData(undoUrl, me, followActivity) - return t.afterCommit(() => { + t.afterCommit(() => { return unicastTo({ data: undoActivity, byActor: me, diff --git a/server/models/actor/actor-follow.ts b/server/models/actor/actor-follow.ts index 0f4d3c0a6..af1d85e9f 100644 --- a/server/models/actor/actor-follow.ts +++ b/server/models/actor/actor-follow.ts @@ -1,5 +1,5 @@ import { difference, values } from 'lodash' -import { Includeable, IncludeOptions, literal, Op, QueryTypes, Transaction, WhereOptions } from 'sequelize' +import { Includeable, IncludeOptions, Op, QueryTypes, Transaction } from 'sequelize' import { AfterCreate, AfterDestroy, @@ -30,7 +30,6 @@ import { MActorFollowFormattable, MActorFollowSubscriptions } from '@server/types/models' -import { ActivityPubActorType } from '@shared/models' import { AttributesOnly } from '@shared/typescript-utils' import { FollowState } from '../../../shared/models/actors' import { ActorFollow } from '../../../shared/models/actors/follow.model' @@ -39,9 +38,11 @@ import { ACTOR_FOLLOW_SCORE, CONSTRAINTS_FIELDS, FOLLOW_STATES, SERVER_ACTOR_NAM import { AccountModel } from '../account/account' import { ServerModel } from '../server/server' import { doesExist } from '../shared/query' -import { createSafeIn, getFollowsSort, getSort, searchAttribute, throwIfNotValid } from '../utils' +import { createSafeIn, getSort, searchAttribute, throwIfNotValid } from '../utils' import { VideoChannelModel } from '../video/video-channel' import { ActorModel, unusedActorAttributesForAPI } from './actor' +import { InstanceListFollowersQueryBuilder, ListFollowersOptions } from './sql/instance-list-followers-query-builder' +import { InstanceListFollowingQueryBuilder, ListFollowingOptions } from './sql/instance-list-following-query-builder' @Table({ tableName: 'actorFollow', @@ -348,144 +349,17 @@ export class ActorFollowModel extends Model { - const actorModel = forCount - ? ActorModel.unscoped() - : ActorModel - - return { - distinct: true, - offset: start, - limit: count, - order: getFollowsSort(sort), - where: followWhere, - include: [ - { - model: actorModel, - required: true, - as: 'ActorFollower', - where: { - id - } - }, - { - model: actorModel, - as: 'ActorFollowing', - required: true, - where: followingWhere, - include: [ - { - model: ServerModel, - required: true - } - ] - } - ] - } - } - + static listInstanceFollowingForApi (options: ListFollowingOptions) { return Promise.all([ - ActorFollowModel.count(getQuery(true)), - ActorFollowModel.findAll(getQuery(false)) + new InstanceListFollowingQueryBuilder(this.sequelize, options).countFollowing(), + new InstanceListFollowingQueryBuilder(this.sequelize, options).listFollowing() ]).then(([ total, data ]) => ({ total, data })) } - static listFollowersForApi (options: { - actorIds: number[] - start: number - count: number - sort: string - state?: FollowState - actorType?: ActivityPubActorType - search?: string - }) { - const { actorIds, start, count, sort, search, state, actorType } = options - - const followWhere = state ? { state } : {} - const followerWhere: WhereOptions = {} - - if (search) { - const escapedSearch = ActorFollowModel.sequelize.escape('%' + search + '%') - - Object.assign(followerWhere, { - id: { - [Op.in]: literal( - `(` + - `SELECT "actor".id FROM actor LEFT JOIN server on server.id = actor."serverId" ` + - `WHERE "preferredUsername" ILIKE ${escapedSearch} OR "host" ILIKE ${escapedSearch}` + - `)` - ) - } - }) - } - - if (actorType) { - Object.assign(followerWhere, { type: actorType }) - } - - const getQuery = (forCount: boolean) => { - const actorModel = forCount - ? ActorModel.unscoped() - : ActorModel - - return { - distinct: true, - - offset: start, - limit: count, - order: getFollowsSort(sort), - where: followWhere, - include: [ - { - model: actorModel, - required: true, - as: 'ActorFollower', - where: followerWhere - }, - { - model: actorModel, - as: 'ActorFollowing', - required: true, - where: { - id: { - [Op.in]: actorIds - } - } - } - ] - } - } - + static listFollowersForApi (options: ListFollowersOptions) { return Promise.all([ - ActorFollowModel.count(getQuery(true)), - ActorFollowModel.findAll(getQuery(false)) + new InstanceListFollowersQueryBuilder(this.sequelize, options).countFollowers(), + new InstanceListFollowersQueryBuilder(this.sequelize, options).listFollowers() ]).then(([ total, data ]) => ({ total, data })) } diff --git a/server/models/actor/sql/instance-list-followers-query-builder.ts b/server/models/actor/sql/instance-list-followers-query-builder.ts new file mode 100644 index 000000000..4a17a8f11 --- /dev/null +++ b/server/models/actor/sql/instance-list-followers-query-builder.ts @@ -0,0 +1,69 @@ +import { Sequelize } from 'sequelize' +import { ModelBuilder } from '@server/models/shared' +import { parseRowCountResult } from '@server/models/utils' +import { MActorFollowActorsDefault } from '@server/types/models' +import { ActivityPubActorType, FollowState } from '@shared/models' +import { InstanceListFollowsQueryBuilder } from './shared/instance-list-follows-query-builder' + +export interface ListFollowersOptions { + actorIds: number[] + start: number + count: number + sort: string + state?: FollowState + actorType?: ActivityPubActorType + search?: string +} + +export class InstanceListFollowersQueryBuilder extends InstanceListFollowsQueryBuilder { + + constructor ( + protected readonly sequelize: Sequelize, + protected readonly options: ListFollowersOptions + ) { + super(sequelize, options) + } + + async listFollowers () { + this.buildListQuery() + + const results = await this.runQuery({ nest: true }) + const modelBuilder = new ModelBuilder(this.sequelize) + + return modelBuilder.createModels(results, 'ActorFollow') + } + + async countFollowers () { + this.buildCountQuery() + + const result = await this.runQuery() + + return parseRowCountResult(result) + } + + protected getWhere () { + let where = 'WHERE "ActorFollowing"."id" IN (:actorIds) ' + this.replacements.actorIds = this.options.actorIds + + if (this.options.state) { + where += 'AND "ActorFollowModel"."state" = :state ' + this.replacements.state = this.options.state + } + + if (this.options.search) { + const escapedLikeSearch = this.sequelize.escape('%' + this.options.search + '%') + + where += `AND (` + + `"ActorFollower->Server"."host" ILIKE ${escapedLikeSearch} ` + + `OR "ActorFollower"."preferredUsername" ILIKE ${escapedLikeSearch} ` + + `)` + } + + if (this.options.actorType) { + where += `AND "ActorFollower"."type" = :actorType ` + this.replacements.actorType = this.options.actorType + } + + return where + } +} diff --git a/server/models/actor/sql/instance-list-following-query-builder.ts b/server/models/actor/sql/instance-list-following-query-builder.ts new file mode 100644 index 000000000..880170b85 --- /dev/null +++ b/server/models/actor/sql/instance-list-following-query-builder.ts @@ -0,0 +1,69 @@ +import { Sequelize } from 'sequelize' +import { ModelBuilder } from '@server/models/shared' +import { parseRowCountResult } from '@server/models/utils' +import { MActorFollowActorsDefault } from '@server/types/models' +import { ActivityPubActorType, FollowState } from '@shared/models' +import { InstanceListFollowsQueryBuilder } from './shared/instance-list-follows-query-builder' + +export interface ListFollowingOptions { + followerId: number + start: number + count: number + sort: string + state?: FollowState + actorType?: ActivityPubActorType + search?: string +} + +export class InstanceListFollowingQueryBuilder extends InstanceListFollowsQueryBuilder { + + constructor ( + protected readonly sequelize: Sequelize, + protected readonly options: ListFollowingOptions + ) { + super(sequelize, options) + } + + async listFollowing () { + this.buildListQuery() + + const results = await this.runQuery({ nest: true }) + const modelBuilder = new ModelBuilder(this.sequelize) + + return modelBuilder.createModels(results, 'ActorFollow') + } + + async countFollowing () { + this.buildCountQuery() + + const result = await this.runQuery() + + return parseRowCountResult(result) + } + + protected getWhere () { + let where = 'WHERE "ActorFollowModel"."actorId" = :followerId ' + this.replacements.followerId = this.options.followerId + + if (this.options.state) { + where += 'AND "ActorFollowModel"."state" = :state ' + this.replacements.state = this.options.state + } + + if (this.options.search) { + const escapedLikeSearch = this.sequelize.escape('%' + this.options.search + '%') + + where += `AND (` + + `"ActorFollowing->Server"."host" ILIKE ${escapedLikeSearch} ` + + `OR "ActorFollowing"."preferredUsername" ILIKE ${escapedLikeSearch} ` + + `)` + } + + if (this.options.actorType) { + where += `AND "ActorFollowing"."type" = :actorType ` + this.replacements.actorType = this.options.actorType + } + + return where + } +} diff --git a/server/models/actor/sql/shared/actor-follow-table-attributes.ts b/server/models/actor/sql/shared/actor-follow-table-attributes.ts new file mode 100644 index 000000000..156b37d44 --- /dev/null +++ b/server/models/actor/sql/shared/actor-follow-table-attributes.ts @@ -0,0 +1,62 @@ +export class ActorFollowTableAttributes { + + getFollowAttributes () { + return [ + '"ActorFollowModel"."id"', + '"ActorFollowModel"."state"', + '"ActorFollowModel"."score"', + '"ActorFollowModel"."url"', + '"ActorFollowModel"."actorId"', + '"ActorFollowModel"."targetActorId"', + '"ActorFollowModel"."createdAt"', + '"ActorFollowModel"."updatedAt"' + ].join(', ') + } + + getActorAttributes (actorTableName: string) { + return [ + `"${actorTableName}"."id" AS "${actorTableName}.id"`, + `"${actorTableName}"."type" AS "${actorTableName}.type"`, + `"${actorTableName}"."preferredUsername" AS "${actorTableName}.preferredUsername"`, + `"${actorTableName}"."url" AS "${actorTableName}.url"`, + `"${actorTableName}"."publicKey" AS "${actorTableName}.publicKey"`, + `"${actorTableName}"."privateKey" AS "${actorTableName}.privateKey"`, + `"${actorTableName}"."followersCount" AS "${actorTableName}.followersCount"`, + `"${actorTableName}"."followingCount" AS "${actorTableName}.followingCount"`, + `"${actorTableName}"."inboxUrl" AS "${actorTableName}.inboxUrl"`, + `"${actorTableName}"."outboxUrl" AS "${actorTableName}.outboxUrl"`, + `"${actorTableName}"."sharedInboxUrl" AS "${actorTableName}.sharedInboxUrl"`, + `"${actorTableName}"."followersUrl" AS "${actorTableName}.followersUrl"`, + `"${actorTableName}"."followingUrl" AS "${actorTableName}.followingUrl"`, + `"${actorTableName}"."remoteCreatedAt" AS "${actorTableName}.remoteCreatedAt"`, + `"${actorTableName}"."serverId" AS "${actorTableName}.serverId"`, + `"${actorTableName}"."createdAt" AS "${actorTableName}.createdAt"`, + `"${actorTableName}"."updatedAt" AS "${actorTableName}.updatedAt"` + ].join(', ') + } + + getServerAttributes (actorTableName: string) { + return [ + `"${actorTableName}->Server"."id" AS "${actorTableName}.Server.id"`, + `"${actorTableName}->Server"."host" AS "${actorTableName}.Server.host"`, + `"${actorTableName}->Server"."redundancyAllowed" AS "${actorTableName}.Server.redundancyAllowed"`, + `"${actorTableName}->Server"."createdAt" AS "${actorTableName}.Server.createdAt"`, + `"${actorTableName}->Server"."updatedAt" AS "${actorTableName}.Server.updatedAt"` + ].join(', ') + } + + getAvatarAttributes (actorTableName: string) { + return [ + `"${actorTableName}->Avatars"."id" AS "${actorTableName}.Avatars.id"`, + `"${actorTableName}->Avatars"."filename" AS "${actorTableName}.Avatars.filename"`, + `"${actorTableName}->Avatars"."height" AS "${actorTableName}.Avatars.height"`, + `"${actorTableName}->Avatars"."width" AS "${actorTableName}.Avatars.width"`, + `"${actorTableName}->Avatars"."fileUrl" AS "${actorTableName}.Avatars.fileUrl"`, + `"${actorTableName}->Avatars"."onDisk" AS "${actorTableName}.Avatars.onDisk"`, + `"${actorTableName}->Avatars"."type" AS "${actorTableName}.Avatars.type"`, + `"${actorTableName}->Avatars"."actorId" AS "${actorTableName}.Avatars.actorId"`, + `"${actorTableName}->Avatars"."createdAt" AS "${actorTableName}.Avatars.createdAt"`, + `"${actorTableName}->Avatars"."updatedAt" AS "${actorTableName}.Avatars.updatedAt"` + ].join(', ') + } +} diff --git a/server/models/actor/sql/shared/instance-list-follows-query-builder.ts b/server/models/actor/sql/shared/instance-list-follows-query-builder.ts new file mode 100644 index 000000000..1d70fbe70 --- /dev/null +++ b/server/models/actor/sql/shared/instance-list-follows-query-builder.ts @@ -0,0 +1,97 @@ +import { Sequelize } from 'sequelize' +import { AbstractRunQuery } from '@server/models/shared' +import { getInstanceFollowsSort } from '@server/models/utils' +import { ActorImageType } from '@shared/models' +import { ActorFollowTableAttributes } from './actor-follow-table-attributes' + +type BaseOptions = { + sort: string + count: number + start: number +} + +export abstract class InstanceListFollowsQueryBuilder extends AbstractRunQuery { + protected readonly tableAttributes = new ActorFollowTableAttributes() + + protected innerQuery: string + + constructor ( + protected readonly sequelize: Sequelize, + protected readonly options: T + ) { + super(sequelize) + } + + protected abstract getWhere (): string + + protected getJoins () { + return 'INNER JOIN "actor" "ActorFollower" ON "ActorFollower"."id" = "ActorFollowModel"."actorId" ' + + 'INNER JOIN "actor" "ActorFollowing" ON "ActorFollowing"."id" = "ActorFollowModel"."targetActorId" ' + } + + protected getServerJoin (actorName: string) { + return `LEFT JOIN "server" "${actorName}->Server" ON "${actorName}"."serverId" = "${actorName}->Server"."id" ` + } + + protected getAvatarsJoin (actorName: string) { + return `LEFT JOIN "actorImage" "${actorName}->Avatars" ON "${actorName}.id" = "${actorName}->Avatars"."actorId" ` + + `AND "${actorName}->Avatars"."type" = ${ActorImageType.AVATAR}` + } + + private buildInnerQuery () { + this.innerQuery = `${this.getInnerSelect()} ` + + `FROM "actorFollow" AS "ActorFollowModel" ` + + `${this.getJoins()} ` + + `${this.getServerJoin('ActorFollowing')} ` + + `${this.getServerJoin('ActorFollower')} ` + + `${this.getWhere()} ` + + `${this.getOrder()} ` + + `LIMIT :limit OFFSET :offset ` + + this.replacements.limit = this.options.count + this.replacements.offset = this.options.start + } + + protected buildListQuery () { + this.buildInnerQuery() + + this.query = `${this.getSelect()} ` + + `FROM (${this.innerQuery}) AS "ActorFollowModel" ` + + `${this.getAvatarsJoin('ActorFollower')} ` + + `${this.getAvatarsJoin('ActorFollowing')} ` + + `${this.getOrder()}` + } + + protected buildCountQuery () { + this.query = `SELECT COUNT(*) AS "total" ` + + `FROM "actorFollow" AS "ActorFollowModel" ` + + `${this.getJoins()} ` + + `${this.getServerJoin('ActorFollowing')} ` + + `${this.getServerJoin('ActorFollower')} ` + + `${this.getWhere()}` + } + + private getInnerSelect () { + return this.buildSelect([ + this.tableAttributes.getFollowAttributes(), + this.tableAttributes.getActorAttributes('ActorFollower'), + this.tableAttributes.getActorAttributes('ActorFollowing'), + this.tableAttributes.getServerAttributes('ActorFollower'), + this.tableAttributes.getServerAttributes('ActorFollowing') + ]) + } + + private getSelect () { + return this.buildSelect([ + '"ActorFollowModel".*', + this.tableAttributes.getAvatarAttributes('ActorFollower'), + this.tableAttributes.getAvatarAttributes('ActorFollowing') + ]) + } + + private getOrder () { + const orders = getInstanceFollowsSort(this.options.sort) + + return 'ORDER BY ' + orders.map(o => `"${o[0]}" ${o[1]}`).join(', ') + } +} diff --git a/server/models/shared/abstract-run-query.ts b/server/models/shared/abstract-run-query.ts index c39b7bcfe..f1182c7be 100644 --- a/server/models/shared/abstract-run-query.ts +++ b/server/models/shared/abstract-run-query.ts @@ -25,4 +25,8 @@ export class AbstractRunQuery { return this.sequelize.query(this.query, queryOptions) } + + protected buildSelect (entities: string[]) { + return `SELECT ${entities.join(', ')} ` + } } diff --git a/server/models/utils.ts b/server/models/utils.ts index 70bfbdb8b..b57290aff 100644 --- a/server/models/utils.ts +++ b/server/models/utils.ts @@ -87,12 +87,12 @@ function getBlacklistSort (model: any, value: string, lastSort: OrderItem = [ 'i return [ firstSort, lastSort ] } -function getFollowsSort (value: string, lastSort: OrderItem = [ 'id', 'ASC' ]): OrderItem[] { +function getInstanceFollowsSort (value: string, lastSort: OrderItem = [ 'id', 'ASC' ]): OrderItem[] { const { direction, field } = buildDirectionAndField(value) if (field === 'redundancyAllowed') { return [ - [ 'ActorFollowing', 'Server', 'redundancyAllowed', direction ], + [ 'ActorFollowing.Server.redundancyAllowed', direction ], lastSort ] } @@ -197,6 +197,12 @@ function parseAggregateResult (result: any) { return total } +function parseRowCountResult (result: any) { + if (result.length !== 0) return result[0].total + + return 0 +} + function createSafeIn (sequelize: Sequelize, stringArr: (string | number)[]) { return stringArr.map(t => { return t === null @@ -263,10 +269,11 @@ export { buildWhereIdOrUUID, isOutdated, parseAggregateResult, - getFollowsSort, + getInstanceFollowsSort, buildDirectionAndField, createSafeIn, - searchAttribute + searchAttribute, + parseRowCountResult } // --------------------------------------------------------------------------- diff --git a/server/models/video/sql/video/videos-id-list-query-builder.ts b/server/models/video/sql/video/videos-id-list-query-builder.ts index 09cb791db..8692a436a 100644 --- a/server/models/video/sql/video/videos-id-list-query-builder.ts +++ b/server/models/video/sql/video/videos-id-list-query-builder.ts @@ -2,7 +2,7 @@ import { Sequelize } from 'sequelize' import validator from 'validator' import { exists } from '@server/helpers/custom-validators/misc' import { WEBSERVER } from '@server/initializers/constants' -import { buildDirectionAndField, createSafeIn } from '@server/models/utils' +import { buildDirectionAndField, createSafeIn, parseRowCountResult } from '@server/models/utils' import { MUserAccountId, MUserId } from '@server/types/models' import { VideoInclude, VideoPrivacy, VideoState } from '@shared/models' import { AbstractRunQuery } from '../../../shared/abstract-run-query' @@ -105,7 +105,7 @@ export class VideosIdListQueryBuilder extends AbstractRunQuery { countVideoIds (countOptions: BuildVideosListQueryOptions): Promise { this.buildIdsListQuery(countOptions) - return this.runQuery().then(rows => rows.length !== 0 ? rows[0].total : 0) + return this.runQuery().then(rows => parseRowCountResult(rows)) } getQuery (options: BuildVideosListQueryOptions) { diff --git a/server/tests/api/redundancy/manage-redundancy.ts b/server/tests/api/redundancy/manage-redundancy.ts index cbf3106bd..d415ac3bf 100644 --- a/server/tests/api/redundancy/manage-redundancy.ts +++ b/server/tests/api/redundancy/manage-redundancy.ts @@ -69,6 +69,7 @@ describe('Test manage videos redundancy', function () { // Server 1 and server 2 follow each other await doubleFollow(servers[0], servers[1]) + await doubleFollow(servers[0], servers[2]) await commands[0].updateRedundancy({ host: servers[1].host, redundancyAllowed: true }) await waitJobs(servers) @@ -83,6 +84,16 @@ describe('Test manage videos redundancy', function () { } }) + it('Should correctly list followings by redundancy', async function () { + const body = await servers[0].follows.getFollowings({ sort: '-redundancyAllowed' }) + + expect(body.total).to.equal(2) + expect(body.data).to.have.lengthOf(2) + + expect(body.data[0].following.host).to.equal(servers[1].host) + expect(body.data[1].following.host).to.equal(servers[2].host) + }) + it('Should not have "remote-videos" redundancies on server 2', async function () { this.timeout(120000) -- 2.41.0