diff --git a/src/providers/lemmyv0/compat.ts b/src/providers/lemmyv0/compat.ts index a441ca9..afb250d 100644 --- a/src/providers/lemmyv0/compat.ts +++ b/src/providers/lemmyv0/compat.ts @@ -505,8 +505,28 @@ export function toModlogView( } } +/** + * Canonical pagination for the page-number softwares (Lemmy v0, PieFed). + * + * `next_page` has to mean "there is more" — handing one out unconditionally + * makes a consumer's `while (next_page)` loop spin forever. The server's own + * cursor wins when the endpoint returns one; otherwise a page at least as + * long as the limit implies there may be another, and a shorter one is the + * end. + * + * "At least" matters: endpoints that merge several requests (notifications, + * person content, all-type search, modlog) legitimately overshoot, because + * these servers apply the limit per bucket. A merged page shorter than the + * limit means every bucket was short — genuinely exhausted. + */ export function toPageResponse( params: types.PageParams, + page?: { + /** How many items this page returned */ + items: number; + /** The server's own cursor, for endpoints that return one */ + next_page?: null | string; + }, ): types.PagableResponse { const page_cursor = params.page_cursor; @@ -515,9 +535,25 @@ export function toPageResponse( "lemmyv0 does not support string page_cursor", ); - return { - next_page: (page_cursor ?? 1) + 1, - }; + if (page?.next_page !== undefined) { + const serverCursor = + page.next_page === null ? undefined : Number(page.next_page); + + // A cursor we can't use as a page number is no cursor at all — better + // to report end-of-feed than to send the server garbage + if (serverCursor === undefined || !Number.isFinite(serverCursor)) + return { next_page: undefined }; + + return { next_page: serverCursor }; + } + + const nextPage = (page_cursor ?? 1) + 1; + + // Without a limit there's nothing to compare a short page against, so + // keep assuming there's more + if (!page || params.limit === undefined) return { next_page: nextPage }; + + return { next_page: page.items >= params.limit ? nextPage : undefined }; } export function toPerson( diff --git a/src/providers/lemmyv0/index.ts b/src/providers/lemmyv0/index.ts index 20a92bb..fbc8f87 100644 --- a/src/providers/lemmyv0/index.ts +++ b/src/providers/lemmyv0/index.ts @@ -256,9 +256,11 @@ export class UnsafeLemmyV0Client implements BaseClient { options, ); + const data = response.comments.map(compat.toCommentView); + return { - ...compat.toPageResponse(payload), - data: response.comments.map(compat.toCommentView), + ...compat.toPageResponse(payload, { items: data.length }), + data, }; } @@ -308,13 +310,15 @@ export class UnsafeLemmyV0Client implements BaseClient { options, ); + const data = Object.values(response) + .flat() + .map(compat.toModlogView) + .filter((m) => !!m) + .sort((a, b) => Date.parse(getLogDate(b)) - Date.parse(getLogDate(a))); + return { - ...compat.toPageResponse(payload), - data: Object.values(response) - .flat() - .map(compat.toModlogView) - .filter((m) => !!m) - .sort((a, b) => Date.parse(getLogDate(b)) - Date.parse(getLogDate(a))), + ...compat.toPageResponse(payload, { items: data.length }), + data, }; } @@ -376,7 +380,7 @@ export class UnsafeLemmyV0Client implements BaseClient { } return { - ...compat.toPageResponse(payload), + ...compat.toPageResponse(payload, { items: data.length }), data, }; } @@ -539,9 +543,11 @@ export class UnsafeLemmyV0Client implements BaseClient { options, ); + const data = response.comment_reports.map(compat.toCommentReportView); + return { - ...compat.toPageResponse(payload), - data: response.comment_reports.map(compat.toCommentReportView), + ...compat.toPageResponse(payload, { items: data.length }), + data, }; } @@ -564,9 +570,11 @@ export class UnsafeLemmyV0Client implements BaseClient { options, ); + const data = response.communities.map(compat.toCommunityView); + return { - ...compat.toPageResponse(payload), - data: response.communities.map(compat.toCommunityView), + ...compat.toPageResponse(payload, { items: data.length }), + data, }; } @@ -604,7 +612,7 @@ export class UnsafeLemmyV0Client implements BaseClient { })(); return { - ...compat.toPageResponse(payload), + ...compat.toPageResponse(payload, { items: data.length }), data, }; } @@ -625,12 +633,14 @@ export class UnsafeLemmyV0Client implements BaseClient { this.#client.getComments(v0Payload, options), ]); + const data = [ + ...comments.map(compat.toCommentView), + ...posts.map(compat.toPostView), + ].sort(sortPostCommentByPublished); + return { - data: [ - ...comments.map(compat.toCommentView), - ...posts.map(compat.toPostView), - ].sort(sortPostCommentByPublished), - ...compat.toPageResponse(payload), + ...compat.toPageResponse(payload, { items: data.length }), + data, }; } @@ -657,9 +667,11 @@ export class UnsafeLemmyV0Client implements BaseClient { options, ); + const data = response.post_reports.map(compat.toPostReportView); + return { - ...compat.toPageResponse(payload), - data: response.post_reports.map(compat.toPostReportView), + ...compat.toPageResponse(payload, { items: data.length }), + data, }; } @@ -674,15 +686,17 @@ export class UnsafeLemmyV0Client implements BaseClient { this.#client.listPostReports(params, options), ]); + const data = [ + ...comment_reports.map(compat.toCommentReportView), + ...post_reports.map(compat.toPostReportView), + ].sort( + (a, b) => + getPostCommentItemCreatedDate(b) - getPostCommentItemCreatedDate(a), + ); + return { - ...compat.toPageResponse(payload), - data: [ - ...comment_reports.map(compat.toCommentReportView), - ...post_reports.map(compat.toPostReportView), - ].sort( - (a, b) => - getPostCommentItemCreatedDate(b) - getPostCommentItemCreatedDate(a), - ), + ...compat.toPageResponse(payload, { items: data.length }), + data, }; } @@ -856,14 +870,16 @@ export class UnsafeLemmyV0Client implements BaseClient { options, ); + const data = [ + ...response.comments.map(compat.toCommentView), + ...response.posts.map(compat.toPostView), + ...response.communities.map(compat.toCommunityView), + ...response.users.map(compat.toPersonView), + ]; + return { - ...compat.toPageResponse(payload), - data: [ - ...response.comments.map(compat.toCommentView), - ...response.posts.map(compat.toPostView), - ...response.communities.map(compat.toCommunityView), - ...response.users.map(compat.toPersonView), - ], + ...compat.toPageResponse(payload, { items: data.length }), + data, }; } diff --git a/src/providers/piefed/index.ts b/src/providers/piefed/index.ts index a2702a6..062144c 100644 --- a/src/providers/piefed/index.ts +++ b/src/providers/piefed/index.ts @@ -76,6 +76,19 @@ const piefedMiddleware: Middleware = { }, }; +/** Canonical search types PieFed can actually serve (its enum has no "All") */ +type SearchableType = Exclude; + +const PIEFED_SEARCH_TYPE = { + comments: "Comments", + communities: "Communities", + posts: "Posts", + users: "Users", +} as const satisfies Record< + SearchableType, + NonNullable["type_"] +>; + export class UnsafePiefedClient implements BaseClient { static mode = "piefed" as const; @@ -394,9 +407,14 @@ export class UnsafePiefedClient implements BaseClient { params: { query }, }); + const data = response.data!.comments.map(compat.toCommentView); + return { - ...compat.toPageResponse(payload), - data: response.data!.comments.map(compat.toCommentView), + ...compat.toPageResponse(payload, { + items: data.length, + next_page: response.data!.next_page, + }), + data, }; } @@ -498,7 +516,7 @@ export class UnsafePiefedClient implements BaseClient { } return { - ...compat.toPageResponse(payload), + ...compat.toPageResponse(payload, { items: data.length }), data, }; } @@ -556,9 +574,14 @@ export class UnsafePiefedClient implements BaseClient { params: { query }, }); + const data = response.data!.posts.map(compat.toPostView); + return { - ...compat.toPageResponse(payload), - data: response.data!.posts.map(compat.toPostView), + ...compat.toPageResponse(payload, { + items: data.length, + next_page: response.data!.next_page, + }), + data, }; } @@ -687,9 +710,14 @@ export class UnsafePiefedClient implements BaseClient { }, }); + const data = response.data!.communities.map(compat.toCommunityView); + return { - ...compat.toPageResponse(payload), - data: response.data!.communities.map(compat.toCommunityView), + ...compat.toPageResponse(payload, { + items: data.length, + next_page: response.data!.next_page, + }), + data, }; } @@ -700,22 +728,23 @@ export class UnsafePiefedClient implements BaseClient { switch (payload.type) { case "all": case undefined: { - const response = await Promise.all([ + const [posts, comments] = await Promise.all([ this.#listPersonPosts(payload, options), - this.#listPersonComments(payload, options), - ]).then(([posts, comments]) => - [...posts.data, ...comments.data].sort( - (a, b) => - getPostCommentItemCreatedDate(b) - - getPostCommentItemCreatedDate(a), - ), + ]); + + const data = [...posts.data, ...comments.data].sort( + (a, b) => + getPostCommentItemCreatedDate(b) - getPostCommentItemCreatedDate(a), ); + // Both halves already resolved the server's own cursor; either one + // having more means this merged feed does too. More reliable than + // inferring from the merged length, since piefed silently clamps + // large limits. return { - ...compat.toPageResponse(payload), - - data: response, + data, + next_page: posts.next_page ?? comments.next_page, }; } @@ -764,7 +793,7 @@ export class UnsafePiefedClient implements BaseClient { })(); return { - ...compat.toPageResponse(payload), + ...compat.toPageResponse(payload, { items: data.length }), data, }; } @@ -968,27 +997,47 @@ export class UnsafePiefedClient implements BaseClient { ): ReturnType { const { listing_type, search_term, type_, ...rest } = compat.fromPageParams(payload); - const response = await this.#client.GET("/api/alpha/search", { - ...options, - params: { - query: { - ...rest, - listing_type: compat.fromListingType(listing_type), - q: search_term, - // @ts-expect-error piefed's SearchType is narrower than ours; we pass through whatever the caller sent - type_: compat.fromSearchType(type_), + + // PieFed requires a concrete `type_` — its enum has no "All" — so an + // all-type search fans out and merges, matching what the canonical + // `type_: "all"` (or an unspecified type) means everywhere else. + const searchTypes: SearchableType[] = + !type_ || type_ === "all" + ? ["communities", "posts", "users", "comments"] + : [type_]; + + const searchOne = async (searchType: SearchableType) => { + const response = await this.#client.GET("/api/alpha/search", { + ...options, + params: { + query: { + ...rest, + listing_type: compat.fromListingType(listing_type), + q: search_term, + type_: PIEFED_SEARCH_TYPE[searchType], + }, }, - }, - }); + }); + + // Each response carries every bucket, but only the requested one is + // populated — take just that bucket so a fan-out can't double-count + switch (searchType) { + case "comments": + return response.data!.comments.map(compat.toCommentView); + case "communities": + return response.data!.communities.map(compat.toCommunityView); + case "posts": + return response.data!.posts.map(compat.toPostView); + case "users": + return response.data!.users.map(compat.toPersonView); + } + }; + + const data = (await Promise.all(searchTypes.map(searchOne))).flat(); return { - ...compat.toPageResponse(payload), - data: [ - ...response.data!.communities.map(compat.toCommunityView), - ...response.data!.posts.map(compat.toPostView), - ...response.data!.users.map(compat.toPersonView), - ...response.data!.comments.map(compat.toCommentView), - ], + ...compat.toPageResponse(payload, { items: data.length }), + data, }; } @@ -1034,9 +1083,14 @@ export class UnsafePiefedClient implements BaseClient { params: { query: { sort: "New", ...compat.fromPageParams(payload) } }, }); + const data = response.data!.comments.map(compat.toCommentView); + return { - ...compat.toPageResponse(payload), - data: response.data!.comments.map(compat.toCommentView), + ...compat.toPageResponse(payload, { + items: data.length, + next_page: response.data!.next_page, + }), + data, }; } @@ -1050,9 +1104,14 @@ export class UnsafePiefedClient implements BaseClient { params: { query: { sort: "New", ...compat.fromPageParams(payload) } }, }); + const data = response.data!.posts.map(compat.toPostView); + return { - ...compat.toPageResponse(payload), - data: response.data!.posts.map(compat.toPostView), + ...compat.toPageResponse(payload, { + items: data.length, + next_page: response.data!.next_page, + }), + data, }; } } diff --git a/test/live-smoke.test.ts b/test/live-smoke.test.ts index 985cf8f..6b49b35 100644 --- a/test/live-smoke.test.ts +++ b/test/live-smoke.test.ts @@ -58,6 +58,14 @@ describe.runIf(process.env.LIVE_SMOKE)("live smoke", () => { expect(data.length).toBeGreaterThan(0); }); + it("all-type search passes canonical validation", OPTIONS, async () => { + // PieFed has no all-type search endpoint — the adapter fans out and + // merges, so an unspecified type_ must work everywhere + const { data } = await client.search({ limit: 3, search_term: "news" }); + + expect(data.length).toBeGreaterThan(0); + }); + it("search passes canonical validation", OPTIONS, async () => { const { data } = await client.search({ limit: 3, diff --git a/test/testing-seed-matrix.test.ts b/test/testing-seed-matrix.test.ts index e668824..eeedf90 100644 --- a/test/testing-seed-matrix.test.ts +++ b/test/testing-seed-matrix.test.ts @@ -267,10 +267,8 @@ describe.each([ } as Parameters[0]); expect(third.data.map((view) => view.post.name)).toEqual(["Post 5"]); - // End-of-feed differs by provider: Lemmy stops handing out cursors, - // while the piefed adapter always computes the next page number — so - // consumers there stop on a short page instead. - if (mode === "lemmyv1") expect(third.next_page).toBeUndefined(); + // End of feed means the same thing on every provider: no cursor + expect(third.next_page).toBeUndefined(); }); it("serves a trailing empty page when the last page was full", async () => { @@ -379,6 +377,35 @@ describe.each([ expect(none.data).toHaveLength(0); }); + it("searches every type at once", async () => { + const { client, fake } = setup(); + + fake.seed.community({ name: "cats_only", title: "Cats Only" }); + fake.seed.person({ name: "catlover" }); + fake.seed.comment({ content: "cats are great" }); + + // PieFed's API has no all-type search, so the adapter fans out and + // merges — the canonical result matches Lemmy's single request + const { data } = await client.search({ search_term: "cat" }); + + const kinds = data.map((item) => { + switch (true) { + case "comment" in item: + return "comment"; + case "post" in item: + return "post"; + case "community" in item: + return "community"; + default: + return "person"; + } + }); + + expect(new Set(kinds)).toEqual( + new Set(["comment", "community", "person", "post"]), + ); + }); + it("seed.clear() empties the derived feed", async () => { const { client, fake } = setup(); diff --git a/test/toPageResponse.test.ts b/test/toPageResponse.test.ts new file mode 100644 index 0000000..a7b02f0 --- /dev/null +++ b/test/toPageResponse.test.ts @@ -0,0 +1,71 @@ +// The page-number providers (Lemmy v0, PieFed) don't all report whether +// more pages exist, so `toPageResponse` derives it. Getting this wrong is +// invisible in the fakes — neither models lemmyv0 — but breaks real feeds, +// so the rules are pinned here directly. + +import { describe, expect, it } from "vitest"; + +import { InvalidPayloadError } from "../src/errors"; +import { toPageResponse } from "../src/providers/lemmyv0/compat"; + +describe("toPageResponse", () => { + it("prefers the server's own cursor", () => { + expect( + toPageResponse({ limit: 10 }, { items: 10, next_page: "3" }), + ).toEqual({ next_page: 3 }); + }); + + it("treats a null server cursor as end of feed", () => { + expect( + toPageResponse({ limit: 10 }, { items: 10, next_page: null }), + ).toEqual({ next_page: undefined }); + }); + + it("treats an unusable server cursor as end of feed", () => { + // Sending a NaN page number back would be worse than stopping + expect( + toPageResponse({ limit: 10 }, { items: 10, next_page: "abc" }), + ).toEqual({ next_page: undefined }); + }); + + it("infers another page from a full one", () => { + expect(toPageResponse({ limit: 10 }, { items: 10 })).toEqual({ + next_page: 2, + }); + expect( + toPageResponse({ limit: 10, page_cursor: 4 }, { items: 10 }), + ).toEqual({ next_page: 5 }); + }); + + it("infers end of feed from a short page", () => { + expect(toPageResponse({ limit: 10 }, { items: 9 })).toEqual({ + next_page: undefined, + }); + expect(toPageResponse({ limit: 10 }, { items: 0 })).toEqual({ + next_page: undefined, + }); + }); + + it("keeps paging when a merged page overshoots the limit", () => { + // Endpoints that merge several requests (notifications, person content, + // all-type search, modlog) get `limit` items *per bucket*, so page one + // legitimately returns more than the limit — it is not the last page + expect(toPageResponse({ limit: 5 }, { items: 70 })).toEqual({ + next_page: 2, + }); + expect(toPageResponse({ limit: 5 }, { items: 20 })).toEqual({ + next_page: 2, + }); + }); + + it("assumes more when there's no limit to compare against", () => { + expect(toPageResponse({}, { items: 3 })).toEqual({ next_page: 2 }); + expect(toPageResponse({})).toEqual({ next_page: 2 }); + }); + + it("rejects string cursors, which these providers can't page with", () => { + expect(() => toPageResponse({ page_cursor: "abc" }, { items: 1 })).toThrow( + InvalidPayloadError, + ); + }); +});