From 45172ac8ae8ec27859b59c066251524bfe08e6b7 Mon Sep 17 00:00:00 2001
From: Rishabh Rathod <rishabh.rathod@appsmith.com>
Date: Mon, 29 Apr 2024 11:23:34 +0530
Subject: [PATCH] fix: firestore pagination issue (#32912)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

## Description

As per the documentation the where clause must always come before the
order by clause, and the startAfter clause must come after the order by
clause i.e the recommended order : `where clause --> order by --> start
after`. However, the way code was written it applies the where clause
after the startAfter i.e as per the code : `order by --> start after -->
where clause` .

In this PR, we change the logic to make sure we follow the order `where
clause -> order by and startAfter / endBefore` when generating query.

Fixes https://github.com/appsmithorg/appsmith/issues/32604

documentation link. -
https://firebase.google.com/docs/firestore/query-data/query-cursors

Changes were taken from the PR created by @sumitsum here
https://github.com/appsmithorg/appsmith-ee/pull/4001




## Automation

/ok-to-test tags="@tag.Datasource"

### :mag: Cypress test results
<!-- This is an auto-generated comment: Cypress test results  -->
> [!TIP]
> 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
> Workflow run:
<https://github.com/appsmithorg/appsmith/actions/runs/8814737910>
> Commit: 4c7ed97cc1808a537d40682fc0e930cd0e30be70
> Cypress dashboard url: <a
href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=8814737910&attempt=1"
target="_blank">Click here!</a>

<!-- end of auto-generated comment: Cypress test results  -->




## Communication
Should the DevRel and Marketing teams inform users about this change?
- [ ] Yes
- [x] No


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->

## Summary by CodeRabbit

- **Refactor**
- Improved the order of operations in Firestore queries to enhance query
performance and reliability.

<!-- end of auto-generated comment: release notes by coderabbit.ai -->
---
 .../com/external/plugins/FirestorePlugin.java | 51 ++++++++++---------
 1 file changed, 26 insertions(+), 25 deletions(-)

diff --git a/app/server/appsmith-plugins/firestorePlugin/src/main/java/com/external/plugins/FirestorePlugin.java b/app/server/appsmith-plugins/firestorePlugin/src/main/java/com/external/plugins/FirestorePlugin.java
index a16844262f..fe20eb0139 100644
--- a/app/server/appsmith-plugins/firestorePlugin/src/main/java/com/external/plugins/FirestorePlugin.java
+++ b/app/server/appsmith-plugins/firestorePlugin/src/main/java/com/external/plugins/FirestorePlugin.java
@@ -683,32 +683,9 @@ public class FirestorePlugin extends BasePlugin {
             }
 
             return Mono.just(query)
-                    // Apply ordering, if provided.
-                    .map(query1 -> {
-                        Query q = query1;
-                        final List<Object> startAfterValues = new ArrayList<>();
-                        final List<Object> endBeforeValues = new ArrayList<>();
-                        for (final String field : orderings) {
-                            q = q.orderBy(
-                                    field.replaceAll("^-", ""),
-                                    field.startsWith("-") ? Query.Direction.DESCENDING : Query.Direction.ASCENDING);
-                            if (startAfter != null) {
-                                startAfterValues.add(startAfter.get(field));
-                            }
-                            if (endBefore != null) {
-                                endBeforeValues.add(endBefore.get(field));
-                            }
-                        }
-                        if (PaginationField.NEXT.equals(paginationField) && !CollectionUtils.isEmpty(startAfter)) {
-                            q = q.startAfter(startAfterValues.toArray());
-                        } else if (PaginationField.PREV.equals(paginationField)
-                                && !CollectionUtils.isEmpty(endBefore)) {
-                            q = q.endBefore(endBeforeValues.toArray());
-                        }
-                        return q;
-                    })
                     // Apply where condition, if provided.
-                    .flatMap(query1 -> {
+                    .flatMap(q -> {
+                        Query query1 = q;
                         if (!isWhereMethodUsed(formData)) {
                             return Mono.just(query1);
                         }
@@ -742,6 +719,30 @@ public class FirestorePlugin extends BasePlugin {
 
                         return Mono.just(query1);
                     })
+                    // Apply ordering, if provided.
+                    .map(query1 -> {
+                        Query q = query1;
+                        final List<Object> startAfterValues = new ArrayList<>();
+                        final List<Object> endBeforeValues = new ArrayList<>();
+                        for (final String field : orderings) {
+                            q = q.orderBy(
+                                    field.replaceAll("^-", ""),
+                                    field.startsWith("-") ? Query.Direction.DESCENDING : Query.Direction.ASCENDING);
+                            if (startAfter != null) {
+                                startAfterValues.add(startAfter.get(field));
+                            }
+                            if (endBefore != null) {
+                                endBeforeValues.add(endBefore.get(field));
+                            }
+                        }
+                        if (PaginationField.NEXT.equals(paginationField) && !CollectionUtils.isEmpty(startAfter)) {
+                            q = q.startAfter(startAfterValues.toArray());
+                        } else if (PaginationField.PREV.equals(paginationField)
+                                && !CollectionUtils.isEmpty(endBefore)) {
+                            q = q.endBefore(endBeforeValues.toArray());
+                        }
+                        return q;
+                    })
                     // Apply limit, always provided, since without it, we can inadvertently end up processing too much
                     // data.
                     .map(query1 -> {