From 1a19730c18cf732d6e95c7cf4817c9cf6691675f Mon Sep 17 00:00:00 2001 From: sunlei Date: Fri, 24 Jul 2026 06:00:25 +0800 Subject: [PATCH] =?UTF-8?q?fix:=20=E4=BF=9D=E8=AF=81=E6=B6=88=E6=81=AF?= =?UTF-8?q?=E6=A8=A1=E6=9D=BF=E5=BC=95=E7=94=A8=E4=B8=80=E8=87=B4=E6=80=A7?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../qqbot-message-template.service.ts | 53 +++- .../qqbot-message-template.service.spec.ts | 247 ++++++++++++++++-- 2 files changed, 262 insertions(+), 38 deletions(-) diff --git a/src/modules/qqbot/core/application/message-push/qqbot-message-template.service.ts b/src/modules/qqbot/core/application/message-push/qqbot-message-template.service.ts index ff8f08c..e01ade9 100644 --- a/src/modules/qqbot/core/application/message-push/qqbot-message-template.service.ts +++ b/src/modules/qqbot/core/application/message-push/qqbot-message-template.service.ts @@ -1,6 +1,11 @@ import { Injectable } from '@nestjs/common'; import { InjectRepository } from '@nestjs/typeorm'; -import { Like, Repository, type FindOptionsWhere } from 'typeorm'; +import { + Like, + Repository, + type EntityManager, + type FindOptionsWhere, +} from 'typeorm'; import type { MessageTemplateInput, MessageTemplateListQuery, @@ -124,16 +129,31 @@ export class QqbotMessageTemplateService { * @throws {SystemMessageContractError} When any non-deleted binding still references it. */ async remove(id: string): Promise { - const current = await this.findActive(id); - const referenceCount = await this.bindingRepository.count({ - where: { isDeleted: false, templateId: id }, - }); - if (referenceCount > 0) - throw new SystemMessageContractError('template_invalid'); - current.enabled = false; - current.isDeleted = true; - await this.templateRepository.save(current); - return true; + return this.templateRepository.manager.transaction( + /** Holds the template write lock until the reference check and soft deletion commit together. */ + async (manager) => { + const templateRepository = manager.getRepository(QqbotMessageTemplate); + const bindingRepository = manager.getRepository( + QqbotMessagePublishBinding, + ); + const current = await templateRepository.findOne({ + lock: { mode: 'pessimistic_write' }, + where: { id, isDeleted: false }, + }); + if (!current) throw new SystemMessageContractError('template_invalid'); + + const referenceCount = await bindingRepository.count({ + where: { isDeleted: false, templateId: id }, + }); + if (referenceCount > 0) { + throw new SystemMessageContractError('template_invalid'); + } + current.enabled = false; + current.isDeleted = true; + await templateRepository.save(current); + return true; + }, + ); } /** @@ -154,7 +174,12 @@ export class QqbotMessageTemplateService { } /** - * Enforces the reusable template gate before a publish binding is created or enabled. + * Enforces the template lock order before a publish binding is created, updated, or revived. + * + * Callers must invoke this inside their current transaction before saving a non-deleted + * binding, and must retain that transaction through the binding save and commit. Disabled + * bindings also require the lock because they still create a live template reference. + * @param manager - Current binding-write transaction manager; never pass a global manager. * @param templateId - Candidate template's string Snowflake identity. * @param sourceKey - Source subscribed by the binding. * @param bindingEnabled - Whether the binding is being created or enabled for delivery. @@ -162,11 +187,13 @@ export class QqbotMessageTemplateService { * @throws {SystemMessageContractError} With `template_invalid` for every unavailable binding choice. */ async requireAvailableForBinding( + manager: EntityManager, templateId: string, sourceKey: string, bindingEnabled: boolean, ): Promise { - const template = await this.templateRepository.findOne({ + const template = await manager.getRepository(QqbotMessageTemplate).findOne({ + lock: { mode: 'pessimistic_write' }, where: { id: templateId, isDeleted: false }, }); if (!template || template.sourceKey !== sourceKey) { diff --git a/test/modules/qqbot/message-push/qqbot-message-template.service.spec.ts b/test/modules/qqbot/message-push/qqbot-message-template.service.spec.ts index 2a53d79..a388d23 100644 --- a/test/modules/qqbot/message-push/qqbot-message-template.service.spec.ts +++ b/test/modules/qqbot/message-push/qqbot-message-template.service.spec.ts @@ -1,4 +1,4 @@ -import type { Repository } from 'typeorm'; +import type { EntityManager, FindOptionsWhere, Repository } from 'typeorm'; import { KtDateTime } from '../../../../src/common'; import { SystemMessageTemplateRendererService } from '../../../../src/modules/qqbot/core/application/message-push/system-message-template-renderer.service'; import { QqbotMessageTemplateService } from '../../../../src/modules/qqbot/core/application/message-push/qqbot-message-template.service'; @@ -28,6 +28,45 @@ function template( }; } +/** Creates a publish-binding fixture used to prove live-reference filtering. */ +function binding( + overrides: Partial = {}, +): QqbotMessagePublishBinding { + return { + accountId: '200', + activeKey: '200:300', + createId: jest.fn(), + createTime: NOW as never, + enabled: true, + id: '400', + isDeleted: false, + selfId: '123456', + subscriptionId: '300', + templateId: '100', + updateTime: NOW as never, + ...overrides, + }; +} + +/** Reads the value held by TypeORM's private Like find operator for fake filtering. */ +function likeValue(value: unknown): string | undefined { + if (typeof value !== 'object' || value === null) return undefined; + const candidate = value as { _value?: unknown }; + return typeof candidate._value === 'string' ? candidate._value : undefined; +} + +/** Builds a promise gate whose resolution is controlled by the test. */ +function deferred(): { + promise: Promise; + resolve: () => void; +} { + let resolve!: () => void; + const promise = new Promise((next) => { + resolve = next; + }); + return { promise, resolve }; +} + /** Registers the source definition used by template lifecycle tests. */ function registry(): SystemMessageSourceRegistry { const value = new SystemMessageSourceRegistry(); @@ -72,10 +111,37 @@ function registry(): SystemMessageSourceRegistry { } /** Constructs a template service with narrow repository fakes for unit-level lifecycle checks. */ -function setup(items: QqbotMessageTemplate[] = [template()]) { +function setup( + items: QqbotMessageTemplate[] = [template()], + bindings: QqbotMessagePublishBinding[] = [], +) { const templateRepository = { create: jest.fn((input) => template(input)), - findAndCount: jest.fn().mockResolvedValue([items, items.length]), + findAndCount: jest.fn( + async ({ + skip = 0, + take = items.length, + where, + }: { + skip?: number; + take?: number; + where: FindOptionsWhere; + }) => { + const namePattern = likeValue(where.name); + const name = namePattern?.slice(1, -1); + const filtered = items.filter((item) => { + if (item.isDeleted !== where.isDeleted) return false; + if (where.enabled !== undefined && item.enabled !== where.enabled) { + return false; + } + if (where.sourceKey && item.sourceKey !== where.sourceKey) { + return false; + } + return !name || item.name.includes(name); + }); + return [filtered.slice(skip, skip + take), filtered.length]; + }, + ), findOne: jest.fn( async ({ where: { id } }) => items.find((item) => item.id === id && !item.isDeleted) ?? null, @@ -83,30 +149,53 @@ function setup(items: QqbotMessageTemplate[] = [template()]) { save: jest.fn(async (item) => item), } as unknown as jest.Mocked>; const bindingRepository = { - count: jest.fn().mockResolvedValue(0), + count: jest.fn( + async ({ where: { isDeleted, templateId } }) => + bindings.filter( + (item) => + item.isDeleted === isDeleted && item.templateId === templateId, + ).length, + ), } as unknown as jest.Mocked>; + const manager = { + getRepository: jest.fn((entity) => { + if (entity === QqbotMessageTemplate) return templateRepository; + if (entity === QqbotMessagePublishBinding) return bindingRepository; + throw new Error('unexpected repository'); + }), + } as unknown as jest.Mocked; + Object.assign(templateRepository, { + manager: { + transaction: jest.fn((callback) => callback(manager)), + }, + }); const service = new QqbotMessageTemplateService( templateRepository, bindingRepository, registry(), new SystemMessageTemplateRendererService(), ); - return { bindingRepository, service, templateRepository }; + return { bindingRepository, manager, service, templateRepository }; } describe('QqbotMessageTemplateService', () => { it('pages undeleted templates with filters, source names, and every live binding reference', async () => { - const { bindingRepository, service, templateRepository } = setup([ - template({ id: '100' }), - template({ id: '101', name: '另一模板' }), - ]); - bindingRepository.count.mockResolvedValueOnce(2).mockResolvedValueOnce(1); + const { bindingRepository, service, templateRepository } = setup( + [ + template({ id: '100' }), + template({ id: '101', isDeleted: true }), + template({ id: '102', enabled: false }), + template({ id: '103', sourceKey: 'other.source' }), + template({ id: '104', name: '名称不匹配' }), + ], + [binding({ enabled: false }), binding({ id: '401', isDeleted: true })], + ); await expect( service.page({ enabled: true, name: '端口', - pageNo: 2, + pageNo: 1, pageSize: 10, sourceKey: SOURCE_KEY, }), @@ -114,19 +203,23 @@ describe('QqbotMessageTemplateService', () => { items: [ expect.objectContaining({ id: '100', - referenceCount: 2, - sourceName: 'STUN 端口变化', - }), - expect.objectContaining({ - id: '101', referenceCount: 1, sourceName: 'STUN 端口变化', }), ], - total: 2, + total: 1, }); expect(templateRepository.findAndCount).toHaveBeenCalledWith( - expect.objectContaining({ skip: 10, take: 10 }), + expect.objectContaining({ + skip: 0, + take: 10, + where: { + enabled: true, + isDeleted: false, + name: expect.objectContaining({ _type: 'like', _value: '%端口%' }), + sourceKey: SOURCE_KEY, + }, + }), ); expect(bindingRepository.count).toHaveBeenNthCalledWith(1, { where: { isDeleted: false, templateId: '100' }, @@ -205,7 +298,7 @@ describe('QqbotMessageTemplateService', () => { }); it('blocks soft deletion when any non-deleted binding references the template, including disabled bindings', async () => { - const { bindingRepository, service, templateRepository } = setup(); + const { bindingRepository, manager, service, templateRepository } = setup(); bindingRepository.count.mockResolvedValueOnce(1).mockResolvedValueOnce(0); await expect(service.remove('100')).rejects.toMatchObject({ @@ -215,28 +308,132 @@ describe('QqbotMessageTemplateService', () => { expect(templateRepository.save).toHaveBeenLastCalledWith( expect.objectContaining({ enabled: false, isDeleted: true }), ); + expect(templateRepository.manager.transaction).toHaveBeenCalledTimes(2); + expect(manager.getRepository).toHaveBeenCalledWith(QqbotMessageTemplate); + expect(templateRepository.findOne).toHaveBeenLastCalledWith({ + lock: { mode: 'pessimistic_write' }, + where: { id: '100', isDeleted: false }, + }); }); it('gates publish bindings by source, deletion, enabled state, and current template validity', async () => { const current = template(); - const { service } = setup([current]); + const { manager, service, templateRepository } = setup([current]); await expect( - service.requireAvailableForBinding('100', SOURCE_KEY, true), + service.requireAvailableForBinding(manager, '100', SOURCE_KEY, true), ).resolves.toBe(current); current.enabled = false; await expect( - service.requireAvailableForBinding('100', SOURCE_KEY, true), + service.requireAvailableForBinding(manager, '100', SOURCE_KEY, true), ).rejects.toMatchObject({ code: 'template_invalid' }); await expect( - service.requireAvailableForBinding('100', SOURCE_KEY, false), + service.requireAvailableForBinding(manager, '100', SOURCE_KEY, false), ).resolves.toBe(current); await expect( - service.requireAvailableForBinding('100', 'other.source', false), + service.requireAvailableForBinding(manager, '100', 'other.source', false), ).rejects.toMatchObject({ code: 'template_invalid' }); current.isDeleted = true; await expect( - service.requireAvailableForBinding('100', SOURCE_KEY, false), + service.requireAvailableForBinding(manager, '100', SOURCE_KEY, false), ).rejects.toMatchObject({ code: 'template_invalid' }); + expect(templateRepository.findOne).toHaveBeenCalledWith({ + lock: { mode: 'pessimistic_write' }, + where: { id: '100', isDeleted: false }, + }); + }); + + it('holds the binding template lock through commit so deletion re-counts the committed live binding', async () => { + const current = template(); + const bindings: QqbotMessagePublishBinding[] = []; + const bindingHasLock = deferred(); + const allowBindingCommit = deferred(); + let lockTail = Promise.resolve(); + let deleteCountStarted = false; + + /** Creates a transaction manager whose pessimistic lock is held until callback completion. */ + const createTransactionManager = (): EntityManager => { + let releaseTemplateLock: (() => void) | undefined; + const acquireTemplateLock = async (): Promise => { + const previous = lockTail; + const lockReleased = deferred(); + lockTail = lockReleased.promise; + await previous; + releaseTemplateLock = lockReleased.resolve; + }; + const manager = { + getRepository: jest.fn((entity) => { + if (entity === QqbotMessageTemplate) { + return { + findOne: jest.fn(async (options) => { + if (options.lock?.mode === 'pessimistic_write') { + await acquireTemplateLock(); + } + return current.isDeleted ? null : current; + }), + save: jest.fn(async (item) => item), + }; + } + if (entity === QqbotMessagePublishBinding) { + return { + count: jest.fn(async () => { + deleteCountStarted = true; + return bindings.filter( + (item) => !item.isDeleted && item.templateId === current.id, + ).length; + }), + }; + } + throw new Error('unexpected repository'); + }), + } as unknown as EntityManager; + Object.assign(manager, { + transaction: async ( + callback: (currentManager: EntityManager) => Promise, + ) => { + try { + return await callback(manager); + } finally { + releaseTemplateLock?.(); + } + }, + }); + return manager; + }; + const transactionManager = createTransactionManager(); + const templateRepository = { + manager: transactionManager, + } as unknown as Repository; + const service = new QqbotMessageTemplateService( + templateRepository, + {} as Repository, + registry(), + new SystemMessageTemplateRendererService(), + ); + + const bindingTransaction = transactionManager.transaction( + async (manager) => { + await service.requireAvailableForBinding( + manager, + '100', + SOURCE_KEY, + true, + ); + bindingHasLock.resolve(); + await allowBindingCommit.promise; + bindings.push(binding()); + }, + ); + await bindingHasLock.promise; + + const deletion = service.remove('100'); + await Promise.resolve(); + expect(deleteCountStarted).toBe(false); + + allowBindingCommit.resolve(); + await bindingTransaction; + await expect(deletion).rejects.toMatchObject({ code: 'template_invalid' }); + expect(current.isDeleted).toBe(false); + expect(bindings).toHaveLength(1); }); });