From da1eb6f6acae94f9e0ba83addb9e829262284cd0 Mon Sep 17 00:00:00 2001
From: Sam Rose <hello@samwho.dev>
Date: Mon, 24 Jun 2024 17:09:27 +0100
Subject: [PATCH 1/2] Fix primary key appearing twice in the SQL ORDER BY
 clause.

---
 packages/backend-core/src/sql/sql.ts          |  8 ++-
 .../src/api/routes/tests/search.spec.ts       | 51 ++++++++++++++++++-
 2 files changed, 55 insertions(+), 4 deletions(-)

diff --git a/packages/backend-core/src/sql/sql.ts b/packages/backend-core/src/sql/sql.ts
index 72ff8d4578..94926feb70 100644
--- a/packages/backend-core/src/sql/sql.ts
+++ b/packages/backend-core/src/sql/sql.ts
@@ -449,8 +449,12 @@ class InternalBuilder {
         query = query.orderBy(`${aliased}.${key}`, direction, nulls)
       }
     }
-    // always add sorting by the primary key - make sure result is deterministic
-    query = query.orderBy(`${aliased}.${primaryKey[0]}`)
+
+    // add sorting by the primary key if the result isn't isn't already sorted
+    // by it, to make sure result is deterministic
+    if (!sort || sort[primaryKey[0]] === undefined) {
+      query = query.orderBy(`${aliased}.${primaryKey[0]}`)
+    }
     return query
   }
 
diff --git a/packages/server/src/api/routes/tests/search.spec.ts b/packages/server/src/api/routes/tests/search.spec.ts
index 23c2ca819c..589f129f31 100644
--- a/packages/server/src/api/routes/tests/search.spec.ts
+++ b/packages/server/src/api/routes/tests/search.spec.ts
@@ -1,5 +1,9 @@
 import { tableForDatasource } from "../../../tests/utilities/structures"
-import { DatabaseName, getDatasource } from "../../../integrations/tests/utils"
+import {
+  DatabaseName,
+  getDatasource,
+  knexClient,
+} from "../../../integrations/tests/utils"
 import { db as dbCore, utils } from "@budibase/backend-core"
 
 import * as setup from "./utilities"
@@ -24,6 +28,8 @@ import _ from "lodash"
 import tk from "timekeeper"
 import { encodeJSBinding } from "@budibase/string-templates"
 import { dataFilters } from "@budibase/shared-core"
+import { Knex } from "knex"
+import { structures } from "@budibase/backend-core/tests"
 
 describe.each([
   ["in-memory", undefined],
@@ -42,6 +48,7 @@ describe.each([
 
   let envCleanup: (() => void) | undefined
   let datasource: Datasource | undefined
+  let client: Knex | undefined
   let table: Table
   let rows: Row[]
 
@@ -63,8 +70,10 @@ describe.each([
     }
 
     if (dsProvider) {
+      const rawDatasource = await dsProvider
+      client = await knexClient(rawDatasource)
       datasource = await config.createDatasource({
-        datasource: await dsProvider,
+        datasource: rawDatasource,
       })
     }
   })
@@ -909,6 +918,44 @@ describe.each([
           }).toMatchExactly([{ name: "foo" }, { name: "bar" }])
         })
       })
+
+      !isInternal &&
+        !isInMemory &&
+        // This test was added because we automatically add in a sort by the
+        // primary key, and we used to do this unconditionally which caused
+        // problems because it was possible for the primary key to appear twice
+        // in the resulting SQL ORDER BY clause, resulting in an SQL error.
+        // We now check first to make sure that the primary key isn't already
+        // in the sort before adding it.
+        describe("sort on primary key", () => {
+          beforeAll(async () => {
+            const tableName = structures.uuid().substring(0, 10)
+            await client!.schema.createTable(tableName, t => {
+              t.string("name").primary()
+            })
+            const resp = await config.api.datasource.fetchSchema({
+              datasourceId: datasource!._id!,
+            })
+
+            table = resp.datasource.entities![tableName]
+
+            await createRows([{ name: "foo" }, { name: "bar" }])
+          })
+
+          it("should be able to sort by a primary key column ascending", async () =>
+            expectSearch({
+              query: {},
+              sort: "name",
+              sortOrder: SortOrder.ASCENDING,
+            }).toMatchExactly([{ name: "bar" }, { name: "foo" }]))
+
+          it("should be able to sort by a primary key column descending", async () =>
+            expectSearch({
+              query: {},
+              sort: "name",
+              sortOrder: SortOrder.DESCENDING,
+            }).toMatchExactly([{ name: "foo" }, { name: "bar" }]))
+        })
     })
   })
 

From 58d8f2bb642b72d3e55db974cfdaa64f688fec42 Mon Sep 17 00:00:00 2001
From: Sam Rose <hello@samwho.dev>
Date: Mon, 24 Jun 2024 17:30:10 +0100
Subject: [PATCH 2/2] Respond to PR feedback.

---
 packages/backend-core/src/sql/sql.ts | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/packages/backend-core/src/sql/sql.ts b/packages/backend-core/src/sql/sql.ts
index 94926feb70..964b121eee 100644
--- a/packages/backend-core/src/sql/sql.ts
+++ b/packages/backend-core/src/sql/sql.ts
@@ -450,8 +450,8 @@ class InternalBuilder {
       }
     }
 
-    // add sorting by the primary key if the result isn't isn't already sorted
-    // by it, to make sure result is deterministic
+    // add sorting by the primary key if the result isn't already sorted by it,
+    // to make sure result is deterministic
     if (!sort || sort[primaryKey[0]] === undefined) {
       query = query.orderBy(`${aliased}.${primaryKey[0]}`)
     }