fix(backend): DBフォールバック有効時、複数のFTTソースから取得するタイムラインで取得漏れが起きる現象の修正 (再) (#114)
* fix: DBフォールバック有効時、複数のFTTソースから取得するタイムラインで取得漏れが起きる現象の修正 https://github.com/nadesskey/nadesskey/pull/35 の修正 * fix: revert unnecessary changes * refactor(test): FTTEndpointのテストを改善 * fix: ids自体が空配列の時を考慮 * Fix: typerror * fix: 昇順のページネーションにおけるfttThresholdIdの計算を修正し、関連するテストを追加しました。 * Fix: Type Error * fix: `useDbFallback` が false の場合に `fttThresholdId` によるタイムラインノートの意図しないフィルタリングを防止
This commit is contained in:
taichan
tai-cha
parent
1a9e205f0f
commit
4a2970fdb0
|
|
@ -83,12 +83,18 @@ export class FanoutTimelineEndpointService {
|
|||
(ps.preventEmptyTimelineDbFallback !== true && redisResult.length > 1 && redisResult.some(ids => ids.length === 0));
|
||||
|
||||
// 取得したresultの中で最古のIDのうち、最も新しいものを取得
|
||||
const fttThresholdId = redisResult.map(ids => ids[0]).sort()[0];
|
||||
// ids自体が空配列の場合、ids[ids.length - 1]はundefinedになるため、filterでnullを除外する
|
||||
// eslint-disable-next-line @typescript-eslint/no-unnecessary-condition
|
||||
const fttThresholdId = redisResult.map(ids => ascending ? ids[0] : ids[ids.length - 1]).filter(id => id != null).sort().pop();
|
||||
|
||||
// TODO: いい感じにgetMulti内でソート済だからuniqするときにredisResultが全てソート済なのを利用して再ソートを避けたい
|
||||
const redisResultIds = shouldFallbackToDb ? [] : Array.from(new Set(redisResult.flat(1))).sort(idCompare);
|
||||
let redisResultIds = shouldFallbackToDb ? [] : Array.from(new Set(redisResult.flat(1)));
|
||||
if (ps.useDbFallback && fttThresholdId != null) {
|
||||
redisResultIds = redisResultIds.filter(id => id >= fttThresholdId);
|
||||
}
|
||||
redisResultIds.sort(idCompare);
|
||||
|
||||
let noteIds = redisResultIds.filter(id => id >= fttThresholdId).slice(0, ps.limit);
|
||||
let noteIds = redisResultIds.slice(0, ps.limit);
|
||||
|
||||
const oldestNoteId = ascending ? redisResultIds[0] : redisResultIds[redisResultIds.length - 1];
|
||||
shouldFallbackToDb ||= ps.useDbFallback && (noteIds.length === 0 || ps.sinceId != null && ps.sinceId < oldestNoteId);
|
||||
|
|
|
|||
|
|
@ -0,0 +1,245 @@
|
|||
/*
|
||||
* SPDX-FileCopyrightText: syuilo and misskey-project
|
||||
* SPDX-License-Identifier: AGPL-3.0-only
|
||||
*/
|
||||
|
||||
import { describe, jest, test, expect, beforeEach, afterEach, beforeAll, afterAll } from '@jest/globals';
|
||||
import { Test, TestingModule } from '@nestjs/testing';
|
||||
import { GlobalModule } from '@/GlobalModule.js';
|
||||
import { CoreModule } from '@/core/CoreModule.js';
|
||||
import { FanoutTimelineEndpointService } from '@/core/FanoutTimelineEndpointService.js';
|
||||
import { FanoutTimelineService, FanoutTimelineName } from '@/core/FanoutTimelineService.js';
|
||||
import { IdService } from '@/core/IdService.js';
|
||||
import { NotesRepository, UsersRepository, UserProfilesRepository, MiUser, MiNote } from '@/models/_.js';
|
||||
import { DI } from '@/di-symbols.js';
|
||||
|
||||
describe('FanoutTimelineEndpointService', () => {
|
||||
let app: TestingModule;
|
||||
let service: FanoutTimelineEndpointService;
|
||||
let fanoutTimelineService: jest.Mocked<FanoutTimelineService>;
|
||||
let notesRepository: NotesRepository;
|
||||
let usersRepository: UsersRepository;
|
||||
let userProfilesRepository: UserProfilesRepository;
|
||||
let idService: IdService;
|
||||
|
||||
let alice: MiUser;
|
||||
|
||||
async function createUser(data: Partial<MiUser> = {}) {
|
||||
const user = await usersRepository
|
||||
.insert({
|
||||
id: idService.gen(),
|
||||
username: 'username',
|
||||
usernameLower: 'username',
|
||||
...data,
|
||||
})
|
||||
.then(x => usersRepository.findOneByOrFail(x.identifiers[0]));
|
||||
|
||||
await userProfilesRepository.insert({
|
||||
userId: user.id,
|
||||
});
|
||||
|
||||
return user;
|
||||
}
|
||||
|
||||
async function createNote(data: Partial<MiNote> = {}) {
|
||||
return await notesRepository
|
||||
.insert({
|
||||
id: idService.gen(),
|
||||
userId: alice.id,
|
||||
text: 'test',
|
||||
visibility: 'public',
|
||||
localOnly: false,
|
||||
...data,
|
||||
})
|
||||
.then(x => notesRepository.findOneByOrFail(x.identifiers[0]));
|
||||
}
|
||||
|
||||
beforeAll(async () => {
|
||||
app = await Test.createTestingModule({
|
||||
imports: [
|
||||
GlobalModule,
|
||||
CoreModule,
|
||||
],
|
||||
providers: [
|
||||
FanoutTimelineEndpointService,
|
||||
],
|
||||
})
|
||||
.overrideProvider(FanoutTimelineService)
|
||||
.useValue({
|
||||
getMulti: jest.fn(),
|
||||
})
|
||||
.compile();
|
||||
|
||||
app.enableShutdownHooks();
|
||||
|
||||
service = app.get<FanoutTimelineEndpointService>(FanoutTimelineEndpointService);
|
||||
fanoutTimelineService = app.get(FanoutTimelineService) as jest.Mocked<FanoutTimelineService>;
|
||||
notesRepository = app.get<NotesRepository>(DI.notesRepository);
|
||||
usersRepository = app.get<UsersRepository>(DI.usersRepository);
|
||||
userProfilesRepository = app.get<UserProfilesRepository>(DI.userProfilesRepository);
|
||||
idService = app.get<IdService>(IdService);
|
||||
});
|
||||
|
||||
afterAll(async () => {
|
||||
await app.close();
|
||||
});
|
||||
|
||||
beforeEach(async () => {
|
||||
alice = await createUser({ username: 'alice', usernameLower: 'alice' });
|
||||
});
|
||||
|
||||
afterEach(async () => {
|
||||
jest.clearAllMocks();
|
||||
await notesRepository.deleteAll();
|
||||
await userProfilesRepository.deleteAll();
|
||||
await usersRepository.deleteAll();
|
||||
});
|
||||
|
||||
test('should use correctly calculated threshold (Max of Oldest) when merging disjoint timelines', async () => {
|
||||
const now = Date.now();
|
||||
// HTL: Recent (T-2m to T-4m)
|
||||
const htlNote1 = await createNote({ id: idService.gen(now - 1000 * 60 * 2) });
|
||||
const htlNote2 = await createNote({ id: idService.gen(now - 1000 * 60 * 3) });
|
||||
const htlNote3 = await createNote({ id: idService.gen(now - 1000 * 60 * 4) }); // End of HTL (T-4m)
|
||||
|
||||
const htlIds = [htlNote1.id, htlNote2.id, htlNote3.id];
|
||||
|
||||
// LTL: Old (T-60m to T-62m)
|
||||
const ltlNote1 = await createNote({ id: idService.gen(now - 1000 * 60 * 60) });
|
||||
const ltlNote2 = await createNote({ id: idService.gen(now - 1000 * 60 * 61) });
|
||||
const ltlNote3 = await createNote({ id: idService.gen(now - 1000 * 60 * 62) });
|
||||
|
||||
const ltlIds = [ltlNote1.id, ltlNote2.id, ltlNote3.id];
|
||||
|
||||
// Mock FanoutTimelineService to return these IDs
|
||||
fanoutTimelineService.getMulti.mockResolvedValue([htlIds, ltlIds]);
|
||||
|
||||
// dbFallback spy
|
||||
// eslint-disable-next-line @typescript-eslint/no-unused-vars
|
||||
const dbFallback = jest.fn((_untilId: string | null, _sinceId: string | null, _limit: number) => Promise.resolve([] as MiNote[]));
|
||||
|
||||
const ps = {
|
||||
redisTimelines: ['homeTimeline', 'localTimeline'] as FanoutTimelineName[],
|
||||
useDbFallback: true,
|
||||
limit: 10,
|
||||
allowPartial: false,
|
||||
excludePureRenotes: false,
|
||||
dbFallback,
|
||||
noteFilter: () => false, // Simulate strict filtering (force fallback)
|
||||
untilId: null,
|
||||
sinceId: null,
|
||||
};
|
||||
|
||||
// See comments in original file for logic explanation.
|
||||
// Essentially, we expect the fallback to start from the end of the most recent reliable timeline (HTL).
|
||||
|
||||
await service.getMiNotes(ps);
|
||||
|
||||
expect(dbFallback).toHaveBeenCalled();
|
||||
const callArgs = dbFallback.mock.calls[0];
|
||||
const untilId = callArgs[0] as string;
|
||||
|
||||
// We expect untilId to be the HTL oldest (htlNote3.id), NOT the LTL newest (ltlNote1.id).
|
||||
expect(untilId).toBe(htlNote3.id);
|
||||
expect(untilId > ltlNote1.id).toBe(true);
|
||||
});
|
||||
|
||||
test('should maintain correct pagination cursor when using sinceId (ascending)', async () => {
|
||||
const now = Date.now();
|
||||
// Ascending: Oldest to Newest.
|
||||
const note1 = await createNote({ id: idService.gen(now - 3000) });
|
||||
const note2 = await createNote({ id: idService.gen(now - 2000) });
|
||||
const note3 = await createNote({ id: idService.gen(now - 1000) });
|
||||
|
||||
const ids = [note1.id, note2.id, note3.id];
|
||||
|
||||
fanoutTimelineService.getMulti.mockResolvedValue([ids]);
|
||||
|
||||
// Mock dbFallback to return empty array
|
||||
// eslint-disable-next-line @typescript-eslint/no-unused-vars
|
||||
const dbFallback = jest.fn((untilId: string | null, sinceId: string | null, limit: number) => Promise.resolve([] as MiNote[]));
|
||||
|
||||
const ps = {
|
||||
redisTimelines: [`homeTimeline:${alice.id}`] as FanoutTimelineName[],
|
||||
useDbFallback: false, // Disable fallback to check Redis filtering logic directly
|
||||
limit: 2,
|
||||
allowPartial: true,
|
||||
excludePureRenotes: false,
|
||||
dbFallback,
|
||||
untilId: null,
|
||||
sinceId: idService.gen(now - 4000),
|
||||
};
|
||||
|
||||
const result = await service.getMiNotes(ps);
|
||||
|
||||
// With the fix, we should get note1 and note2.
|
||||
// Without the fix, we would get only note3 (or empty if limit blocked it).
|
||||
expect(result).toHaveLength(2);
|
||||
expect(result[0].id).toBe(note1.id);
|
||||
expect(result[1].id).toBe(note2.id);
|
||||
});
|
||||
|
||||
test('should not fallback to DB when useDbFallback is false even if insufficient notes', async () => {
|
||||
const now = Date.now();
|
||||
const note1 = await createNote({ id: idService.gen(now) });
|
||||
const ids = [note1.id];
|
||||
|
||||
fanoutTimelineService.getMulti.mockResolvedValue([ids]);
|
||||
|
||||
const dbFallback = jest.fn((_untilId: string | null, _sinceId: string | null, _limit: number) => Promise.resolve([] as MiNote[]));
|
||||
|
||||
const ps = {
|
||||
redisTimelines: [`homeTimeline:${alice.id}`] as FanoutTimelineName[],
|
||||
useDbFallback: false,
|
||||
limit: 10,
|
||||
allowPartial: false,
|
||||
excludePureRenotes: false,
|
||||
dbFallback,
|
||||
noteFilter: () => false, // Filter out everything
|
||||
untilId: null,
|
||||
sinceId: null,
|
||||
};
|
||||
|
||||
const result = await service.getMiNotes(ps);
|
||||
|
||||
expect(dbFallback).not.toHaveBeenCalled();
|
||||
expect(result).toEqual([]);
|
||||
});
|
||||
|
||||
test('should merge disjoint timelines correctly when useDbFallback is false', async () => {
|
||||
const now = Date.now();
|
||||
// TL1: Recent
|
||||
const note1 = await createNote({ id: idService.gen(now - 1000) });
|
||||
const note2 = await createNote({ id: idService.gen(now - 2000) });
|
||||
// TL2: Old
|
||||
const note3 = await createNote({ id: idService.gen(now - 5000) });
|
||||
const note4 = await createNote({ id: idService.gen(now - 6000) });
|
||||
|
||||
const ids1 = [note1.id, note2.id];
|
||||
const ids2 = [note3.id, note4.id];
|
||||
|
||||
fanoutTimelineService.getMulti.mockResolvedValue([ids1, ids2]);
|
||||
|
||||
// eslint-disable-next-line @typescript-eslint/no-unused-vars
|
||||
const dbFallback = jest.fn((untilId: string | null, sinceId: string | null, limit: number) => Promise.resolve([] as MiNote[]));
|
||||
|
||||
const ps = {
|
||||
redisTimelines: ['homeTimeline', 'localTimeline'] as FanoutTimelineName[],
|
||||
useDbFallback: false,
|
||||
limit: 10,
|
||||
allowPartial: true,
|
||||
excludePureRenotes: false,
|
||||
dbFallback,
|
||||
noteFilter: () => true, // Accept all
|
||||
untilId: null,
|
||||
sinceId: null,
|
||||
};
|
||||
|
||||
const result = await service.getMiNotes(ps);
|
||||
|
||||
// With the previous logic, note3 and note4 would be filtered out because they are older than the "threshold" (end of TL1).
|
||||
// With the fixed logic (skipping filter when !useDbFallback), all notes should be present.
|
||||
expect(result).toHaveLength(4);
|
||||
expect(result.map(n => n.id)).toEqual([note1.id, note2.id, note3.id, note4.id]);
|
||||
});
|
||||
});
|
||||
Loading…
Reference in New Issue