Problem with using pagination and sorting by field with common values

[svntime]

Link to reproduction

https://github.com/svntime/payload-sorting-problem

To Reproduce

1. run Payload using yarn dev _community (default credentials: dev@payloadcms.com:test)
2. check if data was populated correctly (there should be 10 posts with equal publicationDate value)
3. visit http://localhost:3000/api/posts?limit=2&page=1&sort=-publicationDate
4. visit http://localhost:3000/api/posts?limit=2&page=2&sort=-publicationDate

Note: you can also use below query on http://localhost:3000/api/graphql-playground

query Posts {
  Posts(limit: 2, page: 1, sort: "-publicationDate") {
    docs {
      title
      id
    }
  }
}

Describe the Bug

Hi there! I've just encountered a problem with using pagination and sort by field which is not unique. In my case, there is a field called publicationDate which can be the same for multiple articles. When I try to query different pages, I get duplicate items in the response. Here is the example:

As you can see, post1 is present on both pages. It's probably related to https://stackoverflow.com/questions/44692075/mongo-db-duplication-issue-while-using-sorting-with-limit-and-skip-in-aggregatio but I didn't investigate it. If so, I don't think we can sort by multiple fields right now 😢 Maybe there is a simpler way?

Thanks a lot!

Payload Version

1.6.26

[jmikrut]

Hey @svntime — hmm, interesting. It looks like that Stackoverflow post suggests to just put the second sort on something like id, which I guess we could probably do by default directly within Payload... that is, until we can define multiple sorts through the API itself.

Either way, I think for now what you should do is try and set your publicationDate to being unique. Is that possible?

And then I will convert this to a discussion, tag it as a feature request, and slate it into our roadmap 😎

[svntime]

It would be great if you could put second sort on id by default until the feature request will be merged into the core. Project I'm working on is already deployed to production and I'd like to avoid explaining to the client that they should be careful when setting a publication dates on articles 😄

Of course, if this change breaks anything then I have no choice, just let me know please @jmikrut.
By the way, I've just found the same feature request on Discord: #2089

[svntime]

I just checked to see if this is a solution to the problem at all and it looks like it is.

Here is the change I made (only in single file, probably should be included wherever buildSortParam is used) src/collections/operations/find.ts:

Current results (page=1 and page=2):

[foopis23]

I just ran into this issue as well.

I have 2 thoughts about this solution. I believe it would be more robust if this could be handled in the buildSortParam function. That way it will work for anything that uses a sort param.

Also, I tried to look into mongoose and mquerys source code to see how order would be handled when using an object as the sort parameter. As far as I can tell, when you get all the way down, it just inserts the sort parameters using Object.keys() and looping over the object. I think to guarantee the user's passed parameter is always evaluated first, I think its better to pass an array or string? I don't know how real of an issue this is because I couldn't really find any documentation that said explicitly or not how mongoose and mquery handle this and if you can count on a specific order with an object. I know mongodb says its sort function is evaluated left to right, but that is the raw query and not a javascript object.

I know this is just a patch file of the built version, but I think a fix for this could look something like this

diff --git a/node_modules/payload/dist/collections/operations/find.js b/node_modules/payload/dist/collections/operations/find.js
index 49e9b5e..07df73f 100644
--- a/node_modules/payload/dist/collections/operations/find.js
+++ b/node_modules/payload/dist/collections/operations/find.js
@@ -73,7 +73,7 @@ async function find(incomingArgs) {
     // /////////////////////////////////////
     // Find
     // /////////////////////////////////////
-    const [sortProperty, sortOrder] = (0, buildSortParam_1.buildSortParam)({
+    const sortQuery = (0, buildSortParam_1.buildSortParam)({
         sort: (_a = args.sort) !== null && _a !== void 0 ? _a : collectionConfig.defaultSort,
         config: payload.config,
         fields: collectionConfig.fields,
@@ -85,9 +85,7 @@ async function find(incomingArgs) {
     let result;
     const paginationOptions = {
         page: page || 1,
-        sort: {
-            [sortProperty]: sortOrder,
-        },
+        sort: sortQuery,
         limit: limitToUse,
         lean: true,
         leanWithId: true,
diff --git a/node_modules/payload/dist/collections/operations/findVersions.js b/node_modules/payload/dist/collections/operations/findVersions.js
index f3b8577..63724c5 100644
--- a/node_modules/payload/dist/collections/operations/findVersions.js
+++ b/node_modules/payload/dist/collections/operations/findVersions.js
@@ -52,7 +52,7 @@ async function findVersions(args) {
     // /////////////////////////////////////
     // Find
     // /////////////////////////////////////
-    const [sortProperty, sortOrder] = (0, buildSortParam_1.buildSortParam)({
+    const sortQuery = (0, buildSortParam_1.buildSortParam)({
         sort: args.sort || '-updatedAt',
         fields: (0, buildCollectionFields_1.buildVersionCollectionFields)(collectionConfig),
         timestamps: true,
@@ -62,9 +62,7 @@ async function findVersions(args) {
     const paginatedDocs = await VersionsModel.paginate(query, {
         page: page || 1,
         limit: limit !== null && limit !== void 0 ? limit : 10,
-        sort: {
-            [sortProperty]: sortOrder,
-        },
+        sort: sortQuery,
         lean: true,
         leanWithId: true,
         useEstimatedCount,
diff --git a/node_modules/payload/dist/globals/operations/findVersions.js b/node_modules/payload/dist/globals/operations/findVersions.js
index 4794535..6ad4d3f 100644
--- a/node_modules/payload/dist/globals/operations/findVersions.js
+++ b/node_modules/payload/dist/globals/operations/findVersions.js
@@ -52,7 +52,7 @@ async function findVersions(args) {
     // /////////////////////////////////////
     // Find
     // /////////////////////////////////////
-    const [sortProperty, sortOrder] = (0, buildSortParam_1.buildSortParam)({
+    const sortQuery = (0, buildSortParam_1.buildSortParam)({
         sort: args.sort || '-updatedAt',
         fields: (0, buildGlobalFields_1.buildVersionGlobalFields)(globalConfig),
         timestamps: true,
@@ -62,9 +62,7 @@ async function findVersions(args) {
     const paginatedDocs = await VersionsModel.paginate(query, {
         page: page || 1,
         limit: limit !== null && limit !== void 0 ? limit : 10,
-        sort: {
-            [sortProperty]: sortOrder,
-        },
+        sort: sortQuery,
         lean: true,
         leanWithId: true,
         useEstimatedCount,
diff --git a/node_modules/payload/dist/mongoose/buildSortParam.js b/node_modules/payload/dist/mongoose/buildSortParam.js
index c596fa9..52afee1 100644
--- a/node_modules/payload/dist/mongoose/buildSortParam.js
+++ b/node_modules/payload/dist/mongoose/buildSortParam.js
@@ -31,6 +31,11 @@ const buildSortParam = ({ sort, config, fields, timestamps, locale }) => {
             locale,
         });
     }
+
+    if (sortProperty !== '_id') {
+        return [[sortProperty, sortOrder], ['_id', 'asc']];
+    }
+
     return [sortProperty, sortOrder];
 };
 exports.buildSortParam = buildSortParam;

(For anyone who saw it before, I edited the patch because I found that it was broken when testing more.)