From 62065ed30d7c955d51308291dcca008e75a2d61b Mon Sep 17 00:00:00 2001 From: ManyTheFish Date: Tue, 18 Nov 2025 14:30:35 +0100 Subject: [PATCH 1/2] Fix the pagination bug where the last document of the previous page was duplicated as the first document of the current page. This was due to a bug on the custom nth function of the sort ranking rule skipping `n-1` documents instead of `n` --- .../tests/documents/get_documents.rs | 95 +++++++++++++++++++ crates/milli/src/documents/sort.rs | 2 +- 2 files changed, 96 insertions(+), 1 deletion(-) diff --git a/crates/meilisearch/tests/documents/get_documents.rs b/crates/meilisearch/tests/documents/get_documents.rs index b3c68351f..3ceeacbad 100644 --- a/crates/meilisearch/tests/documents/get_documents.rs +++ b/crates/meilisearch/tests/documents/get_documents.rs @@ -1339,3 +1339,98 @@ async fn get_document_with_vectors() { } "###); } + +#[actix_rt::test] +async fn test_fetch_documents_pagination_with_sorting() { + let server = Server::new_shared(); + let index = server.unique_index(); + let (task, _code) = index.create(None).await; + server.wait_task(task.uid()).await.succeeded(); + + // Add documents as described in the bug report + let documents = json!([ + {"id": 1, "name": "a"}, + {"id": 2, "name": "b"}, + {"id": 3, "name": "c"}, + {"id": 4, "name": "d"}, + {"id": 5, "name": "e"}, + {"id": 6, "name": "f"} + ]); + let (task, code) = index.add_documents(documents, None).await; + assert_eq!(code, 202); + server.wait_task(task.uid()).await.succeeded(); + + // Set name as sortable attribute + let (task, code) = index.update_settings_sortable_attributes(json!(["name"])).await; + assert_eq!(code, 202); + server.wait_task(task.uid()).await.succeeded(); + + // Request 1 (first page): offset 0, limit 2 + let (response, code) = index + .fetch_documents(json!({ + "offset": 0, + "limit": 2, + "sort": ["name:asc"] + })) + .await; + assert_eq!(code, 200); + let results = response["results"].as_array().unwrap(); + snapshot!(json_string!(results), @r#" + [ + { + "id": 1, + "name": "a" + }, + { + "id": 2, + "name": "b" + } + ] + "#); + + // Request 2 (second page): offset 2, limit 2 + let (response, code) = index + .fetch_documents(json!({ + "offset": 2, + "limit": 2, + "sort": ["name:asc"] + })) + .await; + assert_eq!(code, 200); + let results = response["results"].as_array().unwrap(); + snapshot!(json_string!(results), @r###" + [ + { + "id": 3, + "name": "c" + }, + { + "id": 4, + "name": "d" + } + ] + "###); + + // Request 3 (third page): offset 4, limit 2 + let (response, code) = index + .fetch_documents(json!({ + "offset": 4, + "limit": 2, + "sort": ["name:asc"] + })) + .await; + assert_eq!(code, 200); + let results = response["results"].as_array().unwrap(); + snapshot!(json_string!(results), @r#" + [ + { + "id": 5, + "name": "e" + }, + { + "id": 6, + "name": "f" + } + ] + "#); +} diff --git a/crates/milli/src/documents/sort.rs b/crates/milli/src/documents/sort.rs index f76081847..b26a850fc 100644 --- a/crates/milli/src/documents/sort.rs +++ b/crates/milli/src/documents/sort.rs @@ -87,7 +87,7 @@ impl Iterator for SortedDocumentsIterator<'_> { }; // Otherwise don't directly iterate over children, skip them if we know we will go further - let mut to_skip = n - 1; + let mut to_skip = n; while to_skip > 0 { if let Err(e) = SortedDocumentsIterator::update_current( current_child, From 925bce5fbdf8b9f57ea0967d67ba20b470f4dccf Mon Sep 17 00:00:00 2001 From: ManyTheFish Date: Tue, 18 Nov 2025 16:31:05 +0100 Subject: [PATCH 2/2] Modify the test to test all the sort branches and fix the untested branch --- .../tests/documents/get_documents.rs | 75 ++++++++++++------- crates/milli/src/documents/sort.rs | 2 +- 2 files changed, 48 insertions(+), 29 deletions(-) diff --git a/crates/meilisearch/tests/documents/get_documents.rs b/crates/meilisearch/tests/documents/get_documents.rs index 3ceeacbad..63716687f 100644 --- a/crates/meilisearch/tests/documents/get_documents.rs +++ b/crates/meilisearch/tests/documents/get_documents.rs @@ -1347,21 +1347,17 @@ async fn test_fetch_documents_pagination_with_sorting() { let (task, _code) = index.create(None).await; server.wait_task(task.uid()).await.succeeded(); - // Add documents as described in the bug report - let documents = json!([ - {"id": 1, "name": "a"}, - {"id": 2, "name": "b"}, - {"id": 3, "name": "c"}, - {"id": 4, "name": "d"}, - {"id": 5, "name": "e"}, - {"id": 6, "name": "f"} - ]); - let (task, code) = index.add_documents(documents, None).await; + // Set name as sortable attribute + let (task, code) = index.update_settings_sortable_attributes(json!(["name"])).await; assert_eq!(code, 202); server.wait_task(task.uid()).await.succeeded(); - // Set name as sortable attribute - let (task, code) = index.update_settings_sortable_attributes(json!(["name"])).await; + let documents = json!((0..50) + .map(|i| json!({"id": i, "name": format!("doc_{:05}", std::cmp::min(i, 5))})) + .collect::>()); + + // Add documents as described in the bug report + let (task, code) = index.add_documents(documents, None).await; assert_eq!(code, 202); server.wait_task(task.uid()).await.succeeded(); @@ -1375,18 +1371,18 @@ async fn test_fetch_documents_pagination_with_sorting() { .await; assert_eq!(code, 200); let results = response["results"].as_array().unwrap(); - snapshot!(json_string!(results), @r#" + snapshot!(json_string!(results), @r###" [ { - "id": 1, - "name": "a" + "id": 0, + "name": "doc_00000" }, { - "id": 2, - "name": "b" + "id": 1, + "name": "doc_00001" } ] - "#); + "###); // Request 2 (second page): offset 2, limit 2 let (response, code) = index @@ -1401,12 +1397,12 @@ async fn test_fetch_documents_pagination_with_sorting() { snapshot!(json_string!(results), @r###" [ { - "id": 3, - "name": "c" + "id": 2, + "name": "doc_00002" }, { - "id": 4, - "name": "d" + "id": 3, + "name": "doc_00003" } ] "###); @@ -1421,16 +1417,39 @@ async fn test_fetch_documents_pagination_with_sorting() { .await; assert_eq!(code, 200); let results = response["results"].as_array().unwrap(); - snapshot!(json_string!(results), @r#" + snapshot!(json_string!(results), @r###" [ { - "id": 5, - "name": "e" + "id": 4, + "name": "doc_00004" }, { - "id": 6, - "name": "f" + "id": 5, + "name": "doc_00005" } ] - "#); + "###); + + // Request 4 (fourth page): offset 6, limit 2 + let (response, code) = index + .fetch_documents(json!({ + "offset": 6, + "limit": 2, + "sort": ["name:asc"] + })) + .await; + assert_eq!(code, 200); + let results = response["results"].as_array().unwrap(); + snapshot!(json_string!(results), @r###" + [ + { + "id": 6, + "name": "doc_00005" + }, + { + "id": 7, + "name": "doc_00005" + } + ] + "###); } diff --git a/crates/milli/src/documents/sort.rs b/crates/milli/src/documents/sort.rs index b26a850fc..db6e54078 100644 --- a/crates/milli/src/documents/sort.rs +++ b/crates/milli/src/documents/sort.rs @@ -108,7 +108,7 @@ impl Iterator for SortedDocumentsIterator<'_> { continue; } else { // The current iterator is large enough, so we can forward the call to it. - return inner.nth(to_skip + 1); + return inner.nth(to_skip); } }