From 5c8c0cc273d3e6fba557cbb8b92f75d30e9f0f84 Mon Sep 17 00:00:00 2001
From: Urvi <urvi.savla@stellar.org>
Date: Fri, 6 Sep 2024 01:44:25 -0700
Subject: [PATCH] addressing review comments

---
 services/horizon/internal/actions/helpers.go  | 10 +++++-----
 .../horizon/internal/actions/helpers_test.go  | 20 +++++++++++++------
 services/horizon/internal/actions/trade.go    |  5 +----
 3 files changed, 20 insertions(+), 15 deletions(-)

diff --git a/services/horizon/internal/actions/helpers.go b/services/horizon/internal/actions/helpers.go
index 8db4674991..99071bfba7 100644
--- a/services/horizon/internal/actions/helpers.go
+++ b/services/horizon/internal/actions/helpers.go
@@ -546,10 +546,9 @@ func validateAssetParams(aType, code, issuer, prefix string) error {
 // For ascending queries, we adjust the cursor to ensure it starts at
 // the oldest available ledger.
 func validateAndAdjustCursor(ledgerState *ledger.State, pq *db2.PageQuery) error {
+	err := validateCursorWithinHistory(ledgerState, *pq)
 
-	if pq.Order == db2.OrderDescending {
-		return validateCursorWithinHistory(ledgerState, *pq)
-	} else if pq.Order == db2.OrderAscending {
+	if pq.Order == db2.OrderAscending {
 		// an ascending query should never return a gone response:  An ascending query
 		// prior to known history should return results at the beginning of history,
 		// and an ascending query beyond the end of history should not error out but
@@ -559,13 +558,14 @@ func validateAndAdjustCursor(ledgerState *ledger.State, pq *db2.PageQuery) error
 		// set/modify the cursor for ascending queries to start at the oldest available ledger if it
 		// precedes the oldest ledger. This avoids inefficient queries caused by index bloat from deleted rows
 		// that are removed as part of reaping to maintain the retention window.
-		if pq.Cursor == "" || errors.Is(validateCursorWithinHistory(ledgerState, *pq), &hProblem.BeforeHistory) {
+		if pq.Cursor == "" || errors.Is(err, &hProblem.BeforeHistory) {
 			pq.Cursor = toid.AfterLedger(
 				ordered.Max(0, ledgerState.CurrentStatus().HistoryElder-1),
 			).String()
+			return nil
 		}
 	}
-	return nil
+	return err
 }
 
 // validateCursorWithinHistory checks if the cursor is within the known history range.
diff --git a/services/horizon/internal/actions/helpers_test.go b/services/horizon/internal/actions/helpers_test.go
index 1c7cd6c1f7..e393cc357e 100644
--- a/services/horizon/internal/actions/helpers_test.go
+++ b/services/horizon/internal/actions/helpers_test.go
@@ -401,15 +401,15 @@ func TestGetPageQueryWithoutCursor(t *testing.T) {
 	validateCursor(2, "desc", "")
 }
 
-func TestGetPageQueryWithCursor(t *testing.T) {
+func TestValidateAndAdjustCursor(t *testing.T) {
 	ledgerState := &ledger.State{}
 
 	validateCursor := func(cursor string, limit uint64, order string, expectedCursor string, expectedError error) {
-		req := makeTestActionRequest(fmt.Sprintf("/foo-bar/blah?cursor=%s&limit=%d&order=%s", cursor, limit, order), testURLParams())
-		pq, err := GetPageQuery(ledgerState, req)
-		assert.NoError(t, err)
-		assert.Equal(t, cursor, pq.Cursor)
-		err = validateAndAdjustCursor(ledgerState, &pq)
+		pq := db2.PageQuery{Cursor: cursor,
+			Limit: limit,
+			Order: order,
+		}
+		err := validateAndAdjustCursor(ledgerState, &pq)
 		if expectedError != nil {
 			assert.EqualError(t, expectedError, err.Error())
 		} else {
@@ -428,6 +428,10 @@ func TestGetPageQueryWithCursor(t *testing.T) {
 		ExpHistoryLatest:      7000,
 	})
 
+	// invalid cursor
+	validateCursor("blah", 2, "asc", "blah", problem.BadRequest)
+	validateCursor("blah", 2, "desc", "blah", problem.BadRequest)
+
 	validateCursor(toid.AfterLedger(0).String(), 2, "asc", toid.AfterLedger(0).String(), nil)
 	validateCursor(toid.AfterLedger(200).String(), 2, "asc", toid.AfterLedger(200).String(), nil)
 	validateCursor(toid.AfterLedger(7001).String(), 2, "asc", toid.AfterLedger(7001).String(), nil)
@@ -444,6 +448,10 @@ func TestGetPageQueryWithCursor(t *testing.T) {
 		ExpHistoryLatest:      7000,
 	})
 
+	// invalid cursor
+	validateCursor("blah", 2, "asc", "blah", problem.BadRequest)
+	validateCursor("blah", 2, "desc", "blah", problem.BadRequest)
+
 	// asc order
 	validateCursor(toid.AfterLedger(0).String(), 2, "asc", toid.AfterLedger(299).String(), nil)
 	validateCursor(toid.AfterLedger(200).String(), 2, "asc", toid.AfterLedger(299).String(), nil)
diff --git a/services/horizon/internal/actions/trade.go b/services/horizon/internal/actions/trade.go
index 1e81dbb7a9..57d97593cf 100644
--- a/services/horizon/internal/actions/trade.go
+++ b/services/horizon/internal/actions/trade.go
@@ -287,10 +287,7 @@ func (handler GetTradeAggregationsHandler) GetResource(w HeaderWriter, r *http.R
 	if err != nil {
 		return nil, err
 	}
-	err = validateAndAdjustCursor(handler.LedgerState, &pq)
-	if err != nil {
-		return nil, err
-	}
+
 	qp := TradeAggregationsQuery{}
 	if err = getParams(&qp, r); err != nil {
 		return nil, err