fix(backend): チャンネルフォロー一覧のsinceId/untilIdによる絞り込みが上手く動いていないのを修正

[samunohito]

What

チャンネルのフォロー一覧を返すAPIにて、sinceId/untilIdの比較条件として与えているIDに間違いがあるのを直します（与えられているIDはchannel.idだが、比較先のIDはchannel_following.id）
これにより、チャンネルフォロー一覧から結果が抜け落ちる現象が改善されます。

Why

fix #12175

Additional info (optional)

実際にチャンネルを大量に作成＆お気に入り登録し、歯抜けにならないことを確認。

Checklist

• Read the contribution guide
• Test working in a local environment
• (If needed) Add story of storybook
• (If needed) Update CHANGELOG.md
• (If possible) Add tests

[codecov]

Codecov Report

Attention: Patch coverage is 3.12500% with 31 lines in your changes missing coverage. Please review.

Project coverage is 64.90%. Comparing base (e0cf5b2) to head (cd94b17).
Report is 78 commits behind head on develop.

Files	Patch %	Lines
packages/backend/src/core/QueryService.ts	4.34%	22 Missing ⚠️
...kend/src/server/api/endpoints/channels/followed.ts	0.00%	9 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop   #13698      +/-   ##
===========================================
- Coverage    64.91%   64.90%   -0.01%     
===========================================
  Files          989      989              
  Lines       113003   113018      +15     
  Branches      5777     5776       -1     
===========================================
  Hits         73352    73352              
- Misses       38209    38224      +15     
  Partials      1442     1442              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[github-actions]

このPRによるapi.jsonの差分

差分はこちら

Get diff files from Workflow Page

[samunohito]

SELECT
    "MiChannelFollowing"."id" AS "MiChannelFollowing_id",
    "MiChannelFollowing"."followeeId" AS "MiChannelFollowing_followeeId",
    "MiChannelFollowing"."followerId" AS "MiChannelFollowing_followerId"
FROM
    "channel_following" "MiChannelFollowing"
WHERE
    "MiChannelFollowing"."followeeId" < $ 1
    AND "MiChannelFollowing"."followerId" = $ 2
ORDER BY
    "MiChannelFollowing"."followeeId" DESC
LIMIT
    30

↑のような感じで、外から任意のIDカラムを条件として使用できるようにしました。
省略時は従来通りidを使います

[syuilo]

channel.idをIDとして与えている箇所って具体的にどこかしら？

[syuilo]

見た感じバックエンドは現行の実装で何も問題なさそうだった

[samunohito]

（いまコメントに気が付いた）

[samunohito]

①フロントエンド側のMkPaginationはidというプロパティを期待しており、これをページネーションのsinceId/untilIdに使用している

misskey/packages/frontend/src/components/MkPagination.vue

Lines 83 to 85 in c1514ce

	function arrayToEntries(entities: MisskeyEntity[]): [string, MisskeyEntity][] {
	return entities.map(en => [en.id, en]);
	}

misskey/packages/frontend/src/components/MkPagination.vue

Lines 239 to 328 in c1514ce

	const fetchMore = async (): Promise<void> => {
	if (!more.value || fetching.value || moreFetching.value || items.value.size === 0) return;
	moreFetching.value = true;
	const params = props.pagination.params ? isRef(props.pagination.params) ? props.pagination.params.value : props.pagination.params : {};
	await misskeyApi<MisskeyEntity[]>(props.pagination.endpoint, {
	...params,
	limit: SECOND_FETCH_LIMIT,
	...(props.pagination.offsetMode ? {
	offset: offset.value,
	} : {
	untilId: Array.from(items.value.keys()).at(-1),
	}),
	}).then(res => {
	for (let i = 0; i < res.length; i++) {
	const item = res[i];
	if (i === 10) item._shouldInsertAd_ = true;
	}
	const reverseConcat = _res => {
	const oldHeight = scrollableElement.value ? scrollableElement.value.scrollHeight : getBodyScrollHeight();
	const oldScroll = scrollableElement.value ? scrollableElement.value.scrollTop : window.scrollY;
	items.value = concatMapWithArray(items.value, _res);
	return nextTick(() => {
	if (scrollableElement.value) {
	scroll(scrollableElement.value, { top: oldScroll + (scrollableElement.value.scrollHeight - oldHeight), behavior: 'instant' });
	} else {
	window.scroll({ top: oldScroll + (getBodyScrollHeight() - oldHeight), behavior: 'instant' });
	}
	return nextTick();
	});
	};
	if (res.length === 0) {
	if (props.pagination.reversed) {
	reverseConcat(res).then(() => {
	more.value = false;
	moreFetching.value = false;
	});
	} else {
	items.value = concatMapWithArray(items.value, res);
	more.value = false;
	moreFetching.value = false;
	}
	} else {
	if (props.pagination.reversed) {
	reverseConcat(res).then(() => {
	more.value = true;
	moreFetching.value = false;
	});
	} else {
	items.value = concatMapWithArray(items.value, res);
	more.value = true;
	moreFetching.value = false;
	}
	}
	offset.value += res.length;
	}, err => {
	moreFetching.value = false;
	});
	};
	const fetchMoreAhead = async (): Promise<void> => {
	if (!more.value || fetching.value || moreFetching.value || items.value.size === 0) return;
	moreFetching.value = true;
	const params = props.pagination.params ? isRef(props.pagination.params) ? props.pagination.params.value : props.pagination.params : {};
	await misskeyApi<MisskeyEntity[]>(props.pagination.endpoint, {
	...params,
	limit: SECOND_FETCH_LIMIT,
	...(props.pagination.offsetMode ? {
	offset: offset.value,
	} : {
	sinceId: Array.from(items.value.keys()).at(-1),
	}),
	}).then(res => {
	if (res.length === 0) {
	items.value = concatMapWithArray(items.value, res);
	more.value = false;
	} else {
	items.value = concatMapWithArray(items.value, res);
	more.value = true;
	}
	offset.value += res.length;
	moreFetching.value = false;
	}, err => {
	moreFetching.value = false;
	});
	};

---

②バックエンド側のQueryService.makePaginationQuery()はidというプロパティを期待しており、これをページネーションのsinceId/untilIdに使用している

misskey/packages/backend/src/server/api/endpoints/channels/followed.ts

Lines 51 to 52 in c1514ce

	const query = this.queryService.makePaginationQuery(this.channelFollowingsRepository.createQueryBuilder(), ps.sinceId, ps.untilId)
	.andWhere({ followerId: me.id });

misskey/packages/backend/src/core/QueryService.ts

Lines 43 to 68 in c1514ce

	public makePaginationQuery<T extends ObjectLiteral>(q: SelectQueryBuilder<T>, sinceId?: string | null, untilId?: string | null, sinceDate?: number | null, untilDate?: number | null): SelectQueryBuilder<T> {
	if (sinceId && untilId) {
	q.andWhere(`${q.alias}.id > :sinceId`, { sinceId: sinceId });
	q.andWhere(`${q.alias}.id < :untilId`, { untilId: untilId });
	q.orderBy(`${q.alias}.id`, 'DESC');
	} else if (sinceId) {
	q.andWhere(`${q.alias}.id > :sinceId`, { sinceId: sinceId });
	q.orderBy(`${q.alias}.id`, 'ASC');
	} else if (untilId) {
	q.andWhere(`${q.alias}.id < :untilId`, { untilId: untilId });
	q.orderBy(`${q.alias}.id`, 'DESC');
	} else if (sinceDate && untilDate) {
	q.andWhere(`${q.alias}.id > :sinceId`, { sinceId: this.idService.gen(sinceDate) });
	q.andWhere(`${q.alias}.id < :untilId`, { untilId: this.idService.gen(untilDate) });
	q.orderBy(`${q.alias}.id`, 'DESC');
	} else if (sinceDate) {
	q.andWhere(`${q.alias}.id > :sinceId`, { sinceId: this.idService.gen(sinceDate) });
	q.orderBy(`${q.alias}.id`, 'ASC');
	} else if (untilDate) {
	q.andWhere(`${q.alias}.id < :untilId`, { untilId: this.idService.gen(untilDate) });
	q.orderBy(`${q.alias}.id`, 'DESC');
	} else {
	q.orderBy(`${q.alias}.id`, 'DESC');
	}
	return q;
	}

ここで渡しているのはchannelFollowingsRepositoryのクエリビルダーなので、channel_followings.idがsinceId/untilIdとして渡されることが想定されている

---

③バックエンドから返される時にpackされているが、この時のIDはchannelのものになる

misskey/packages/backend/src/server/api/endpoints/channels/followed.ts

Line 58 in c1514ce

return await Promise.all(followings.map(x => this.channelEntityService.pack(x.followeeId, me)));

misskey/packages/backend/src/core/entities/ChannelEntityService.ts

Line 75 in c1514ce

id: channel.id,

---

①＋②＋③

フロントエンドにはchannel.idをキーとした一覧が返却され、MkPaginationによるページネーションもこのIDが使用される。channels/followedのsinceId/untilIdにはchannel.idが渡ってくることになるが、channels/followedそのものはchannel_followings.idを使って絞り込みをしており、不正確な条件で絞り込みをしてしまう（IDの種類が違うので）

[samunohito]

@syuilo
↑こんな感じかなと

[syuilo]

ふぉろーIDを想定しているAPIにチャンネルIDを渡しているのを修正しないとダメそう

[samunohito]

@syuilo
この流れ、1回やった気がするのでもう一度 #12175 を確認いただけますと… 🙏
そのうえでこのブランチの対応がよく無さそうであれば、別の方法を考えます

[syuilo]

互換性を維持したまま直すとしたら新しいエンドポイント作るしかなさそうね

[syuilo]

フォロー中ユーザー一覧ってどんなレスポンスだったっけ

[samunohito]

misskey/packages/backend/src/core/entities/FollowingEntityService.ts

Lines 90 to 101 in 514a65e

	return await awaitAll({
	id: following.id,
	createdAt: this.idService.parse(following.id).date.toISOString(),
	followeeId: following.followeeId,
	followerId: following.followerId,
	followee: opts.populateFollowee ? hint?.packedFollowee ?? this.userEntityService.pack(following.followee ?? following.followeeId, me, {
	schema: 'UserDetailedNotMe',
	}) : undefined,
	follower: opts.populateFollower ? hint?.packedFollower ?? this.userEntityService.pack(following.follower ?? following.followerId, me, {
	schema: 'UserDetailedNotMe',
	}) : undefined,
	});

followingのIDでした

[syuilo]

それと同じ形式で返すエンドポイントを作るしかないわね

[samunohito]

うーむ。

[samunohito]

方針がよくなさそうなので、このPRは白紙にしますか。

[Sayamame-beans]

個人的な考えとして…

• 本来返すべきもの(ChannelFollowing)を返せていない(Channelを返している)問題
  • v2なエンドポイントの作成が必要で、v2が生え次第Misskeyのフロントエンドもこっちを使うように変えるべき
  • v1(現行)とv2で互換性は無い
• 現状返しているものを前提とした上での挙動が正しくない(現行Misskeyのフロントエンドと、v1を使っているサードパーティクライアントが正常に動いていない)問題
  • v1(現行)のエンドポイントを使っているサードパーティクライアント(更新未追従)が壊れないまま修正でき、互換性も失われない(このPR)

という2つの問題があると捉えていて、前者と後者は方向性が違う(ので両方やって良さそう)と思っているのですが、syuiloさんとしては両方やるのは冗長…という感覚でしょうか?

• 前者のみの対応だと、サードパーティクライアントは追従するまで壊れた挙動のままになる
• 後者の対応を行ったとしても、本来返すべきものを返すようにv2を作成する理由は依然ある(v2が生えればMisskeyのフロントエンドもv2に切り替える。v1は互換性のために残す)

[syuilo]

v1(現行)のエンドポイントを使っているサードパーティクライアント(更新未追従)が壊れないまま修正でき、互換性も失われない(このPR)

自分の理解が間違ってるかもしれないけど、このPRで修正されるとは思えない(違う種類のデータ同士を比較しているから、期待したデータが返って来たとしても偶然と思われる)

[Sayamame-beans]

v1(現行)のエンドポイントを使っているサードパーティクライアント(更新未追従)が壊れないまま修正でき、互換性も失われない(このPR)

自分の理解が間違ってるかもしれないけど、このPRで修正されるとは思えない(違う種類のデータ同士を比較しているから、期待したデータが返って来たとしても偶然と思われる)

sinceId/untilIdをフォローした時間ベースのものであると捉えると正にそうなのですが、しかし「"v1(現行)は返ってきているものがChannelであり、ChannelFollowingではない"ので、v1(現行)におけるsinceId/untilIdがチャンネルが作成された時間ベースのものであってもおかしくはない」と思うのですが、いかがでしょうか…?
(もちろん、v2が生えればそれが返すものはChannelFollowingになるので、v2のsinceId/untilIdはフォローした時間ベースになるべき)

[syuilo]

あー

[syuilo]

それならおかしくないわね

[syuilo]

とはいえこのエンドポイントのためだけに makePaginationQuery を拡張したりするのは混乱が生まれそう

[samunohito]

makePaginationQueryを少し弄ったコピペ版を作成し（ただし、安易に複製しないでほしい旨と経緯、このissue/prをコメントで添える）、それを使ってページング処理をするようにすれば、すべてクリアになりますかね…？

[poppingmoon]

ページネーションが壊れているのを直すという目的を考えると、offsetを追加してsinceId/untilIdは非推奨にするのがシンプルなのではないかと思います

変更に追従しないサードパーティは壊れたままになりますがレスポンスが異なる別のエンドポイントが増える場合よりも追従は容易になります

[Sayamame-beans]

確かに、offset式に変えるとページネーションは直せるかもしれません…? しかし、正しいものが返ってきていないということが発覚した時点で、"このエンドポイントに手を付けるならv2を作る"ということ自体はほぼ確定しているのでは、と思っています。
そのため、v1に対して"変更に追従しないと壊れたまま"な変更を加えるぐらいなら、直さずにv2作るだけで良いじゃんとなりそうな気がします?
(このPRは、とりあえず"変更に追従されなくても挙動が直る"ようにすることを目的にしていると思うので)

[poppingmoon]

Channelの代わりにChannelFollowingを返すようにすることにどのような意味があるのでしょうか（フロントエンドでフォローした日時を表示する？）
返すものを変えるためにほとんど同じ機能の別のエンドポイントを作るぐらいなら元のエンドポイントに（互換性を保ったまま）変更を入れるだけでいいのではと思いました

[syuilo]

Channelの代わりにChannelFollowingを返すようにすることにどのような意味があるのでしょうか（フロントエンドでフォローした日時を表示する？）

一般的にフォロー中の何かを一覧で返す場合は、フォローした順に一覧したいことが多そう

[poppingmoon]

現在の実装でもフォローした順に返されているのではないかと思います

[Sayamame-beans]

現在の実装でもフォローした順に返されているのではないかと思います

そうですが、代わりにページネーションで間が抜けます

[poppingmoon]

offsetにすれば抜けないのでは？

[Sayamame-beans]

offsetにすれば抜けないのでは？

サードパーティクライアントが(追従するまで)壊れたままになります

[poppingmoon]

サードパーティクライアントで壊れないようにするにはv1も修正する必要があると思います
v1を変更してv2も追加するということですか？

[Sayamame-beans]

offsetを使うか、v2を生やすかのどちらが良いかは諸説あるかもしれませんが、sinceId/untilIdをした時に間が抜けなくなる実装はその話とは別な気もします。
(#13698 (comment) の前者と後者の関係に近いので)

サードパーティクライアントで壊れないようにするにはv1も修正する必要があると思います v1を変更してv2も追加するということですか？

私はそれを意図していますね。