From bf14079bde966b03b3ad00953cd34a2269182ee1 Mon Sep 17 00:00:00 2001 From: sunlei Date: Fri, 24 Jul 2026 07:25:49 +0800 Subject: [PATCH] =?UTF-8?q?fix:=20=E4=BF=9D=E8=AF=81=E6=B6=88=E6=81=AF?= =?UTF-8?q?=E5=8F=91=E5=B8=83=E7=BB=91=E5=AE=9A=E4=BE=9D=E8=B5=96=E4=B8=80?= =?UTF-8?q?=E8=87=B4=E6=80=A7?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../qqbot-account-message-push.service.ts | 16 +- .../qqbot-message-subscription.service.ts | 20 ++ .../qqbot-message-target-options.service.ts | 19 +- .../qqbot-message-template.service.ts | 28 ++- ...qqbot-account-message-push.service.spec.ts | 193 ++++++++++++++++++ ...qqbot-message-subscription.service.spec.ts | 69 ++++++- ...bot-message-target-options.service.spec.ts | 51 +++++ .../qqbot-message-template.service.spec.ts | 47 ++++- 8 files changed, 431 insertions(+), 12 deletions(-) diff --git a/src/modules/qqbot/core/application/message-push/qqbot-account-message-push.service.ts b/src/modules/qqbot/core/application/message-push/qqbot-account-message-push.service.ts index 3a7c69b..63d642e 100644 --- a/src/modules/qqbot/core/application/message-push/qqbot-account-message-push.service.ts +++ b/src/modules/qqbot/core/application/message-push/qqbot-account-message-push.service.ts @@ -103,7 +103,13 @@ export class QqbotAccountMessagePushService { if (historicalCandidate) { const historical = await bindings.findOne({ lock: { mode: 'pessimistic_write' }, - where: { id: historicalCandidate.id, isDeleted: true }, + where: { + accountId: String(account.id), + id: historicalCandidate.id, + isDeleted: true, + selfId: String(account.selfId), + subscriptionId: String(input.subscriptionId), + }, }); if (historical) { Object.assign( @@ -364,9 +370,11 @@ export class QqbotAccountMessagePushService { ): Promise { const [subscription, template, targets] = await Promise.all([ this.subscriptionRepository.findOne({ - where: { id: binding.subscriptionId }, + where: { id: binding.subscriptionId, isDeleted: false }, + }), + this.templateRepository.findOne({ + where: { id: binding.templateId, isDeleted: false }, }), - this.templateRepository.findOne({ where: { id: binding.templateId } }), this.targetRepository.find({ order: { createTime: 'ASC', id: 'ASC' }, where: { bindingId: String(binding.id), isDeleted: false }, @@ -573,7 +581,7 @@ export class QqbotAccountMessagePushService { /** Signals that a target binding is missing, deleted, or owned by another account. */ private throwBindingUnavailable(): never { - throw new SystemMessageContractError('binding_invalid'); + throw new SystemMessageContractError('binding_disabled'); } /** Recognizes only MySQL's duplicate-key conflict used as final concurrency authority. */ diff --git a/src/modules/qqbot/core/application/message-push/qqbot-message-subscription.service.ts b/src/modules/qqbot/core/application/message-push/qqbot-message-subscription.service.ts index b8c7b79..7cb15a6 100644 --- a/src/modules/qqbot/core/application/message-push/qqbot-message-subscription.service.ts +++ b/src/modules/qqbot/core/application/message-push/qqbot-message-subscription.service.ts @@ -15,6 +15,7 @@ import { type MessageSubscriptionView, } from '../../contract/message-push/qqbot-message-push.types'; import { QqbotMessageSubscription } from '../../infrastructure/persistence/message-push/qqbot-message-subscription.entity'; +import { QqbotMessagePublishBinding } from '../../infrastructure/persistence/message-push/qqbot-message-publish-binding.entity'; import { SystemMessageSourceRegistry } from './system-message-source.registry'; const DEFAULT_PAGE_NO = 1; @@ -154,6 +155,9 @@ export class QqbotMessageSubscriptionService { async (manager) => { const repository = manager.getRepository(QqbotMessageSubscription); const current = await this.findActiveForWrite(repository, id); + if (current.sourceKey !== normalized.sourceKey) { + await this.assertNoLiveBindings(manager, id); + } const conflict = await repository.findOne({ where: { activeKey: normalized.activeKey, isDeleted: false }, }); @@ -215,6 +219,7 @@ export class QqbotMessageSubscriptionService { async (manager) => { const repository = manager.getRepository(QqbotMessageSubscription); const current = await this.findActiveForWrite(repository, id); + await this.assertNoLiveBindings(manager, id); current.activeKey = null; current.enabled = false; current.isDeleted = true; @@ -325,6 +330,21 @@ export class QqbotMessageSubscriptionService { return current; } + /** Rejects a dependency lifecycle mutation while any enabled or disabled live binding references it. */ + private async assertNoLiveBindings( + manager: EntityManager, + subscriptionId: string, + ): Promise { + const count = await manager + .getRepository(QqbotMessagePublishBinding) + .count({ + where: { isDeleted: false, subscriptionId }, + }); + if (count > 0) { + throw new SystemMessageContractError('invalid_source_config'); + } + } + /** * Maps duplicate natural keys to the existing Vben-compatible HTTP conflict response. * @returns Never returns because it always throws an HTTP 409 exception. diff --git a/src/modules/qqbot/core/application/message-push/qqbot-message-target-options.service.ts b/src/modules/qqbot/core/application/message-push/qqbot-message-target-options.service.ts index ac256c5..eb38892 100644 --- a/src/modules/qqbot/core/application/message-push/qqbot-message-target-options.service.ts +++ b/src/modules/qqbot/core/application/message-push/qqbot-message-target-options.service.ts @@ -35,7 +35,8 @@ export class QqbotMessageTargetOptionsService { ]; const unique = new Map(); options.forEach((option) => { - unique.set(`${option.targetType}:${option.targetId}`, option); + const key = `${option.targetType}:${option.targetId}`; + unique.set(key, this.preferCandidate(unique.get(key), option)); }); return { available: true, @@ -98,6 +99,22 @@ export class QqbotMessageTargetOptionsService { return typeof value === 'string' && value.trim() ? value.trim() : null; } + /** Selects a stable duplicate candidate, retaining known names before lexically ordering conflicts. */ + private preferCandidate( + current: QqbotMessagePushTargetOption | undefined, + candidate: QqbotMessagePushTargetOption, + ): QqbotMessagePushTargetOption { + if (!current) return candidate; + const currentKnown = current.label !== current.targetId; + const candidateKnown = candidate.label !== candidate.targetId; + if (currentKnown !== candidateKnown) { + return candidateKnown ? candidate : current; + } + return candidate.label.localeCompare(current.label) < 0 + ? candidate + : current; + } + /** Produces the stable HTTP-safe empty candidate response. */ private unavailable( reasonCode: string, 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 e01ade9..389d40b 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 @@ -103,10 +103,32 @@ export class QqbotMessageTemplateService { id: string, input: MessageTemplateInput, ): Promise { - const current = await this.findActive(id); this.validateInput(input); - Object.assign(current, this.toPersistenceInput(input)); - return this.toView(await this.templateRepository.save(current)); + const saved = await this.templateRepository.manager.transaction( + /** Holds the dependency row through reference counting and its source-safe save. */ + 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'); + if (current.sourceKey !== input.sourceKey) { + const referenceCount = await bindingRepository.count({ + where: { isDeleted: false, templateId: id }, + }); + if (referenceCount > 0) { + throw new SystemMessageContractError('template_invalid'); + } + } + Object.assign(current, this.toPersistenceInput(input)); + return templateRepository.save(current); + }, + ); + return this.toView(saved); } /** diff --git a/test/modules/qqbot/message-push/qqbot-account-message-push.service.spec.ts b/test/modules/qqbot/message-push/qqbot-account-message-push.service.spec.ts index 6cf859b..15af841 100644 --- a/test/modules/qqbot/message-push/qqbot-account-message-push.service.spec.ts +++ b/test/modules/qqbot/message-push/qqbot-account-message-push.service.spec.ts @@ -160,4 +160,197 @@ describe('QqbotAccountMessagePushService', () => { }); expect(events).toEqual(['subscription', 'template', 'binding', 'targets']); }); + + it('falls back to a new binding when the selected historical row changes before its write lock', async () => { + const historical = { + accountId: 'account-1', + activeKey: null, + id: 'historical-1', + isDeleted: true, + selfId: '10001', + subscriptionId: 'subscription-1', + templateId: 'template-1', + updateTime: '2026-07-24', + }; + const saved: any[] = []; + const subscription = { + enabled: true, + id: 'subscription-1', + isDeleted: false, + name: '订阅', + sourceConfig: {}, + sourceKey: 'network.stun.mapping-port-changed', + }; + const template = { + content: '正文', + enabled: true, + id: 'template-1', + isDeleted: false, + name: '模板', + sourceKey: subscription.sourceKey, + }; + const targets: any[] = []; + const bindingStore = { + create: jest.fn((value) => ({ + ...value, + createTime: '2026-07-24', + id: 'new-binding-1', + updateTime: '2026-07-24', + })), + findOne: jest.fn(async (options) => { + const where = options.where as Record; + if ('activeKey' in where) return null; + if ('subscriptionId' in where && !options.lock) return historical; + if (options.lock?.mode === 'pessimistic_write') { + Object.assign(historical, { + accountId: 'account-2', + selfId: '10002', + subscriptionId: 'subscription-2', + }); + return null; + } + throw new Error('unexpected binding lookup'); + }), + save: jest.fn(async (value) => { + saved.push(value); + return value; + }), + }; + const targetStore = { + create: jest.fn((value) => ({ ...value, id: 'target-1' })), + find: jest.fn(async () => []), + save: jest.fn(async (values) => { + targets.splice(0, targets.length, ...values); + return values; + }), + }; + const manager = { + getRepository: (entity: unknown) => { + if (entity === QqbotMessagePublishBinding) return bindingStore; + if (entity === QqbotMessagePublishTarget) return targetStore; + throw new Error('unexpected repository'); + }, + }; + const service = new QqbotAccountMessagePushService( + { + manager: { transaction: async (callback: any) => callback(manager) }, + } as never, + { find: async () => targets } as never, + { findOne: async () => subscription } as never, + { findOne: async () => template } as never, + { + findBySelfId: async () => ({ id: 'account-1', selfId: '10001' }), + } as never, + { requireAvailableForBinding: async () => subscription } as never, + { requireAvailableForBinding: async () => template } as never, + { + get: () => ({ + definition: { displayName: 'STUN 映射', variables: [] }, + inspectSubscription: async () => ({ valid: true }), + }), + } as never, + { validate: jest.fn() } as never, + ); + + await expect( + service.createBinding('10001', { + enabled: true, + subscriptionId: 'subscription-1', + templateId: 'template-1', + targets: [{ targetId: '20001', targetType: 'group' }], + }), + ).resolves.toEqual(expect.objectContaining({ id: 'new-binding-1' })); + expect(bindingStore.findOne).toHaveBeenLastCalledWith({ + lock: { mode: 'pessimistic_write' }, + where: { + accountId: 'account-1', + id: 'historical-1', + isDeleted: true, + selfId: '10001', + subscriptionId: 'subscription-1', + }, + }); + expect(saved).toEqual([ + expect.objectContaining({ id: 'new-binding-1', isDeleted: false }), + ]); + expect(saved[0].id).not.toBe(historical.id); + }); + + it('uses binding_disabled for missing and stale binding mutation snapshots', async () => { + const unavailable = () => + new QqbotAccountMessagePushService( + { + manager: { + transaction: async (callback: any) => + callback({ + getRepository: () => ({ findOne: async () => null }), + }), + }, + } as never, + {} as never, + {} as never, + {} as never, + { + findBySelfId: async () => ({ id: 'account-1', selfId: '10001' }), + } as never, + {} as never, + {} as never, + {} as never, + {} as never, + ); + const input = { + enabled: false, + subscriptionId: 'subscription-1', + targets: [], + templateId: 'template-1', + }; + + await expect( + unavailable().updateBinding('10001', 'missing', input), + ).rejects.toMatchObject({ code: 'binding_disabled' }); + await expect( + unavailable().setBindingEnabled('10001', 'missing', true), + ).rejects.toMatchObject({ code: 'binding_disabled' }); + await expect( + unavailable().removeBinding('10001', 'missing'), + ).rejects.toMatchObject({ code: 'binding_disabled' }); + }); + + it('redacts deleted legacy dependency metadata from unavailable binding views', async () => { + const binding = { + accountId: 'account-1', + createTime: '2026-07-24', + enabled: false, + id: 'binding-1', + isDeleted: false, + selfId: '10001', + subscriptionId: 'subscription-1', + templateId: 'template-1', + updateTime: '2026-07-24', + }; + const service = new QqbotAccountMessagePushService( + { find: async () => [binding] } as never, + { find: async () => [] } as never, + { findOne: jest.fn(async () => null) } as never, + { findOne: jest.fn(async () => null) } as never, + { + findBySelfId: async () => ({ id: 'account-1', selfId: '10001' }), + } as never, + {} as never, + {} as never, + {} as never, + {} as never, + ); + + await expect(service.listBindings('10001')).resolves.toEqual([ + expect.objectContaining({ + available: false, + invalidReasonCode: 'invalid_source_config', + sourceKey: '', + sourceName: '', + subscriptionName: '', + templateName: '', + }), + ]); + }); }); diff --git a/test/modules/qqbot/message-push/qqbot-message-subscription.service.spec.ts b/test/modules/qqbot/message-push/qqbot-message-subscription.service.spec.ts index b1eb474..b3497ec 100644 --- a/test/modules/qqbot/message-push/qqbot-message-subscription.service.spec.ts +++ b/test/modules/qqbot/message-push/qqbot-message-subscription.service.spec.ts @@ -12,6 +12,7 @@ import { NetworkStunMessageSourceAdapter } from '../../../../src/modules/admin/p import { NetworkDdnsRecord } from '../../../../src/modules/admin/platform-config/network-management/network-ddns.entity'; import { NetworkPortForward } from '../../../../src/modules/admin/platform-config/network-management/network-management.entity'; import { QqbotMessageSubscription } from '../../../../src/modules/qqbot/core/infrastructure/persistence/message-push/qqbot-message-subscription.entity'; +import { QqbotMessagePublishBinding } from '../../../../src/modules/qqbot/core/infrastructure/persistence/message-push/qqbot-message-publish-binding.entity'; const SOURCE_KEY = 'network.stun.mapping-port-changed'; const NOW = new KtDateTime('2026-07-24 08:09:10'); @@ -122,6 +123,8 @@ function registry( function setup( items: QqbotMessageSubscription[] = [], source: SystemMessageSourceAdapter = adapter(), + bindings: Array<{ isDeleted: boolean; subscriptionId: string }> = [], + additionalSources: SystemMessageSourceAdapter[] = [], ) { let createSequence = 1000; const subscriptionRepository = { @@ -194,20 +197,41 @@ function setup( return item; }), } as unknown as jest.Mocked>; + const bindingRepository = { + count: jest.fn( + async ({ where: { isDeleted, subscriptionId } }) => + bindings.filter( + (binding) => + binding.isDeleted === isDeleted && + binding.subscriptionId === subscriptionId, + ).length, + ), + } as unknown as jest.Mocked>; const manager = { getRepository: jest.fn((entity) => { if (entity === QqbotMessageSubscription) return subscriptionRepository; + if (entity === QqbotMessagePublishBinding) return bindingRepository; throw new Error('unexpected repository'); }), } as unknown as jest.Mocked; Object.assign(subscriptionRepository, { manager: { transaction: jest.fn((callback) => callback(manager)) }, }); + const sourceRegistry = registry(source); + additionalSources.forEach((item) => sourceRegistry.register(item)); const service = new QqbotMessageSubscriptionService( subscriptionRepository, - registry(source), + sourceRegistry, ); - return { items, manager, service, source, subscriptionRepository }; + return { + bindingRepository, + items, + manager, + service, + source, + sourceRegistry, + subscriptionRepository, + }; } /** @@ -878,6 +902,44 @@ describe('QqbotMessageSubscriptionService', () => { }); }); + it('rejects deletion and source changes while enabled or disabled live bindings reference the subscription', async () => { + const other = adapter(); + Object.assign(other.definition, { sourceKey: 'other.source' }); + const { bindingRepository, service, subscriptionRepository } = setup( + [subscription()], + adapter(), + [{ isDeleted: false, subscriptionId: '100' }], + [other], + ); + + await expect(service.remove('100')).rejects.toMatchObject({ + code: 'invalid_source_config', + }); + await expect( + service.update('100', { + enabled: false, + name: '改源', + sourceConfig: CONFIG, + sourceKey: 'other.source', + }), + ).rejects.toMatchObject({ code: 'invalid_source_config' }); + await expect( + service.update('100', { + enabled: false, + name: '同源编辑', + sourceConfig: CONFIG, + sourceKey: SOURCE_KEY, + }), + ).resolves.toEqual(expect.objectContaining({ name: '同源编辑' })); + + expect(bindingRepository.count).toHaveBeenCalledWith({ + where: { isDeleted: false, subscriptionId: '100' }, + }); + expect(subscriptionRepository.save).not.toHaveBeenCalledWith( + expect.objectContaining({ isDeleted: true }), + ); + }); + it('makes a binding transaction observe deletion after the shared subscription row lock commits', async () => { const current = subscription(); const deleteHasLock = deferred(); @@ -910,6 +972,9 @@ describe('QqbotMessageSubscriptionService', () => { const manager = { getRepository: jest.fn((entity) => { if (entity === QqbotMessageSubscription) return repository; + if (entity === QqbotMessagePublishBinding) { + return { count: jest.fn(async () => 0) }; + } throw new Error('unexpected repository'); }), } as unknown as EntityManager; diff --git a/test/modules/qqbot/message-push/qqbot-message-target-options.service.spec.ts b/test/modules/qqbot/message-push/qqbot-message-target-options.service.spec.ts index a02687b..2b78987 100644 --- a/test/modules/qqbot/message-push/qqbot-message-target-options.service.spec.ts +++ b/test/modules/qqbot/message-push/qqbot-message-target-options.service.spec.ts @@ -98,4 +98,55 @@ describe('QqbotMessageTargetOptionsService', () => { reasonCode: 'onebot_unavailable', }); }); + + it('deduplicates equivalent OneBot candidates deterministically without discarding known names', async () => { + const responses = [ + { + data: [ + { group_id: '20000000000000001' }, + { group_id: '20000000000000001', group_name: 'Beta' }, + { group_id: '20000000000000001', group_name: 'Alpha' }, + ], + status: 'ok', + }, + { data: [], status: 'ok' }, + ]; + const accountService = { + findBySelfId: jest.fn().mockResolvedValue({ id: '1' }), + }; + const reverseWsService = { + sendAction: jest.fn(async () => responses.shift()), + }; + const service = new QqbotMessageTargetOptionsService( + accountService as never, + reverseWsService as never, + ); + + const first = await service.listTargetOptions('10001'); + responses.push( + { + data: [ + { group_id: '20000000000000001', group_name: 'Alpha' }, + { group_id: '20000000000000001', group_name: 'Beta' }, + { group_id: '20000000000000001' }, + ], + status: 'ok', + }, + { data: [], status: 'ok' }, + ); + const second = await service.listTargetOptions('10001'); + + expect(first).toEqual({ + available: true, + options: [ + { + label: 'Alpha (20000000000000001)', + targetId: '20000000000000001', + targetType: 'group', + }, + ], + reasonCode: null, + }); + expect(second).toEqual(first); + }); }); 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 a388d23..b2cbd70 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 @@ -169,13 +169,20 @@ function setup( transaction: jest.fn((callback) => callback(manager)), }, }); + const sourceRegistry = registry(); const service = new QqbotMessageTemplateService( templateRepository, bindingRepository, - registry(), + sourceRegistry, new SystemMessageTemplateRendererService(), ); - return { bindingRepository, manager, service, templateRepository }; + return { + bindingRepository, + manager, + service, + sourceRegistry, + templateRepository, + }; } describe('QqbotMessageTemplateService', () => { @@ -297,6 +304,42 @@ describe('QqbotMessageTemplateService', () => { ); }); + it('locks template source changes through reference counting but permits same-source edits', async () => { + const { bindingRepository, service, sourceRegistry, templateRepository } = + setup([template()], [binding({ enabled: false })]); + sourceRegistry.register({ + ...registry().get(SOURCE_KEY), + definition: { + ...registry().get(SOURCE_KEY).definition, + sourceKey: 'other.source', + }, + }); + const input = { + content: '端口 ${{port}}，就绪：${{ready}}', + enabled: false, + name: '改源', + sourceKey: 'other.source', + }; + + await expect(service.update('100', input)).rejects.toMatchObject({ + code: 'template_invalid', + }); + await expect( + service.update('100', { + ...input, + name: '同源编辑', + sourceKey: SOURCE_KEY, + }), + ).resolves.toEqual(expect.objectContaining({ name: '同源编辑' })); + expect(templateRepository.findOne).toHaveBeenCalledWith({ + lock: { mode: 'pessimistic_write' }, + where: { id: '100', isDeleted: false }, + }); + expect(bindingRepository.count).toHaveBeenCalledWith({ + where: { isDeleted: false, templateId: '100' }, + }); + }); + it('blocks soft deletion when any non-deleted binding references the template, including disabled bindings', async () => { const { bindingRepository, manager, service, templateRepository } = setup(); bindingRepository.count.mockResolvedValueOnce(1).mockResolvedValueOnce(0);