From fe6f4fd43b660ec238cb6395085c0d21074d72d1 Mon Sep 17 00:00:00 2001
From: Zoe Roux <zoe.roux@zoriya.dev>
Date: Fri, 10 Jan 2025 12:15:31 +0100
Subject: [PATCH] Fix null sorting

---
 api/src/models/utils/keyset-paginate.ts       | 18 +++++++++++++---
 api/src/models/utils/page.ts                  |  3 +++
 .../movies/get-all-movies-with-null.test.ts   | 21 ++++++++++---------
 3 files changed, 29 insertions(+), 13 deletions(-)

diff --git a/api/src/models/utils/keyset-paginate.ts b/api/src/models/utils/keyset-paginate.ts
index a5a7dc43..6bcba223 100644
--- a/api/src/models/utils/keyset-paginate.ts
+++ b/api/src/models/utils/keyset-paginate.ts
@@ -1,5 +1,5 @@
 import type { NonEmptyArray, Sort } from "./sort";
-import { eq, or, type Column, and, gt, lt } from "drizzle-orm";
+import { eq, or, type Column, and, gt, lt, isNull } from "drizzle-orm";
 
 type Table<Name extends string> = Record<Name, Column>;
 
@@ -41,8 +41,20 @@ export const keysetPaginate = <
 	let previous = undefined;
 	for (const [i, by] of [...sort, pkSort].entries()) {
 		const cmp = by.desc ? lt : gt;
-		where = or(where, and(previous, cmp(table[by.key], cursor[i])));
-		previous = and(previous, eq(table[by.key], cursor[i]));
+		where = or(
+			where,
+			and(
+				previous,
+				or(
+					cmp(table[by.key], cursor[i]),
+					!table[by.key].notNull ? isNull(table[by.key]) : undefined,
+				),
+			),
+		);
+		previous = and(
+			previous,
+			cursor[i] === null ? isNull(table[by.key]) : eq(table[by.key], cursor[i]),
+		);
 	}
 
 	return where;
diff --git a/api/src/models/utils/page.ts b/api/src/models/utils/page.ts
index a533dcfa..daad081e 100644
--- a/api/src/models/utils/page.ts
+++ b/api/src/models/utils/page.ts
@@ -19,6 +19,9 @@ export const createPage = <T>(
 ) => {
 	let next: string | null = null;
 
+	// we can't know for sure if there's a next page when the current page is full.
+	// maybe the next page is empty, this is a bit weird but it allows us to handle pages
+	// without making a new request to the db so it's fine.
 	if (items.length === limit && limit > 0) {
 		const uri = new URL(url);
 		uri.searchParams.set("after", generateAfter(items[items.length - 1], sort));
diff --git a/api/tests/movies/get-all-movies-with-null.test.ts b/api/tests/movies/get-all-movies-with-null.test.ts
index 1504a0bc..a6cc6477 100644
--- a/api/tests/movies/get-all-movies-with-null.test.ts
+++ b/api/tests/movies/get-all-movies-with-null.test.ts
@@ -52,14 +52,6 @@ describe("with a null value", () => {
 	});
 
 	it("sort by dates desc with a null value", async () => {
-		console.log(
-			(
-				await getMovies({
-					sort: "-airDate",
-					langs: "en",
-				})
-			)[1].items,
-		);
 		let [resp, body] = await getMovies({
 			limit: 2,
 			sort: "-airDate",
@@ -67,6 +59,11 @@ describe("with a null value", () => {
 		});
 		expectStatus(resp, body).toBe(200);
 
+		expect(body.items.map((x: any) => x.slug)).toMatchObject([
+			bubble.slug,
+			dune.slug,
+		]);
+
 		// we copy this due to https://github.com/oven-sh/bun/issues/3521
 		const next = body.next;
 		expect(body).toMatchObject({
@@ -84,6 +81,10 @@ describe("with a null value", () => {
 		body = await resp.json();
 
 		expectStatus(resp, body).toBe(200);
+		expect(body.items.map((x: any) => x.slug)).toMatchObject([
+			dune1984.slug,
+			"no-air-date",
+		]);
 		expect(body).toMatchObject({
 			items: [
 				expect.objectContaining({
@@ -96,7 +97,7 @@ describe("with a null value", () => {
 				}),
 			],
 			this: next,
-			next: null,
+			next: expect.anything(),
 		});
 	});
 	it("sort by dates asc with a null value", async () => {
@@ -139,7 +140,7 @@ describe("with a null value", () => {
 				}),
 			],
 			this: next,
-			next: null,
+			next: expect.anything(),
 		});
 	});
 });