From 5973e616cc077e51613be21275723bf489c2a8fe Mon Sep 17 00:00:00 2001 From: Urvi Date: Thu, 29 Aug 2024 00:46:34 -0700 Subject: [PATCH 1/6] services/horizon: Limit sql queries for history endpoints to retention history --- services/horizon/internal/actions/helpers.go | 20 +++++++------ .../internal/db2/history/account_signers.go | 2 +- .../horizon/internal/db2/history/accounts.go | 8 +++--- services/horizon/internal/db2/main.go | 7 +++-- services/horizon/internal/db2/page_query.go | 28 +++++++++---------- 5 files changed, 35 insertions(+), 30 deletions(-) diff --git a/services/horizon/internal/actions/helpers.go b/services/horizon/internal/actions/helpers.go index 2575b13251..bd9608d97a 100644 --- a/services/horizon/internal/actions/helpers.go +++ b/services/horizon/internal/actions/helpers.go @@ -3,6 +3,7 @@ package actions import ( "context" "encoding/hex" + "errors" "fmt" "net/http" "net/url" @@ -20,7 +21,6 @@ import ( "github.com/stellar/go/services/horizon/internal/db2" "github.com/stellar/go/services/horizon/internal/ledger" hProblem "github.com/stellar/go/services/horizon/internal/render/problem" - "github.com/stellar/go/support/errors" "github.com/stellar/go/support/ordered" "github.com/stellar/go/support/render/problem" "github.com/stellar/go/toid" @@ -171,7 +171,7 @@ func getLimit(r *http.Request, name string, def uint64, max uint64) (uint64, err if asI64 <= 0 { err = errors.New("invalid limit: non-positive value provided") } else if asI64 > int64(max) { - err = errors.Errorf("invalid limit: value provided that is over limit max of %d", max) + err = fmt.Errorf("invalid limit: value provided that is over limit max of %d", max) } if err != nil { @@ -221,12 +221,16 @@ func GetPageQuery(ledgerState *ledger.State, r *http.Request, opts ...Opt) (db2. return db2.PageQuery{}, err } - if cursor == "" && defaultTOID { - if pageQuery.Order == db2.OrderAscending { - pageQuery.Cursor = toid.AfterLedger( - ordered.Max(0, ledgerState.CurrentStatus().HistoryElder-1), - ).String() + elderCursor := toid.AfterLedger( + ordered.Max(0, ledgerState.CurrentStatus().HistoryElder-1), + ).String() + + if defaultTOID && pageQuery.Order == db2.OrderAscending { + if cursor == "" || errors.Is(validateCursorWithinHistory(ledgerState, pageQuery), &hProblem.BeforeHistory) { + pageQuery.Cursor = elderCursor } + } else if defaultTOID && pageQuery.Order == db2.OrderDescending { + pageQuery.ElderCursor = elderCursor } return pageQuery, nil @@ -596,7 +600,7 @@ func countNonEmpty(params ...interface{}) (int, error) { for _, param := range params { switch param := param.(type) { default: - return 0, errors.Errorf("unexpected type %T", param) + return 0, fmt.Errorf("unexpected type %T", param) case int32: if param != 0 { count++ diff --git a/services/horizon/internal/db2/history/account_signers.go b/services/horizon/internal/db2/history/account_signers.go index 78afcdf06c..8014379a4b 100644 --- a/services/horizon/internal/db2/history/account_signers.go +++ b/services/horizon/internal/db2/history/account_signers.go @@ -37,7 +37,7 @@ func (q *Q) SignersForAccounts(ctx context.Context, accounts []string) ([]Accoun // AccountsForSigner returns a list of `AccountSigner` rows for a given signer func (q *Q) AccountsForSigner(ctx context.Context, signer string, page db2.PageQuery) ([]AccountSigner, error) { sql := selectAccountSigners.Where("accounts_signers.signer = ?", signer) - sql, err := page.ApplyToUsingCursor(sql, "accounts_signers.account_id", page.Cursor) + sql, err := page.ApplyRawTo(sql, "accounts_signers.account_id") if err != nil { return nil, errors.Wrap(err, "could not apply query to page") } diff --git a/services/horizon/internal/db2/history/accounts.go b/services/horizon/internal/db2/history/accounts.go index 2af197da4f..8f3b79a4fe 100644 --- a/services/horizon/internal/db2/history/accounts.go +++ b/services/horizon/internal/db2/history/accounts.go @@ -144,7 +144,7 @@ func (q *Q) AccountsForAsset(ctx context.Context, asset xdr.Asset, page db2.Page "trust_lines.asset_code": code, }) - sql, err := page.ApplyToUsingCursor(sql, "trust_lines.account_id", page.Cursor) + sql, err := page.ApplyRawTo(sql, "trust_lines.account_id") if err != nil { return nil, errors.Wrap(err, "could not apply query to page") } @@ -168,7 +168,7 @@ func (q *Q) AccountsForLiquidityPool(ctx context.Context, poolID string, page db "trust_lines.liquidity_pool_id": poolID, }) - sql, err := page.ApplyToUsingCursor(sql, "trust_lines.account_id", page.Cursor) + sql, err := page.ApplyRawTo(sql, "trust_lines.account_id") if err != nil { return nil, errors.Wrap(err, "could not apply query to page") } @@ -189,7 +189,7 @@ func selectBySponsor(table, sponsor string, page db2.PageQuery) (sq.SelectBuilde "sponsor": sponsor, }) - sql, err := page.ApplyToUsingCursor(sql, "account_id", page.Cursor) + sql, err := page.ApplyRawTo(sql, "account_id") if err != nil { return sql, errors.Wrap(err, "could not apply query to page") } @@ -255,7 +255,7 @@ func (q *Q) AccountEntriesForSigner(ctx context.Context, signer string, page db2 "accounts_signers.signer": signer, }) - sql, err := page.ApplyToUsingCursor(sql, "accounts_signers.account_id", page.Cursor) + sql, err := page.ApplyRawTo(sql, "accounts_signers.account_id") if err != nil { return nil, errors.Wrap(err, "could not apply query to page") } diff --git a/services/horizon/internal/db2/main.go b/services/horizon/internal/db2/main.go index 5ec81eb0f3..c497817061 100644 --- a/services/horizon/internal/db2/main.go +++ b/services/horizon/internal/db2/main.go @@ -5,7 +5,8 @@ package db2 // PageQuery represents a portion of a Query struct concerned with paging // through a large dataset. type PageQuery struct { - Cursor string - Order string - Limit uint64 + Cursor string + Order string + Limit uint64 + ElderCursor string } diff --git a/services/horizon/internal/db2/page_query.go b/services/horizon/internal/db2/page_query.go index 3c458af855..988b294366 100644 --- a/services/horizon/internal/db2/page_query.go +++ b/services/horizon/internal/db2/page_query.go @@ -59,7 +59,7 @@ func (p PageQuery) ApplyTo( return sql, err } - return p.ApplyToUsingCursor(sql, col, cursor) + return p.applyToUsingCursor(sql, col, cursor) } // ApplyRawTo applies the raw PageQuery.Cursor cursor to the builder @@ -67,12 +67,12 @@ func (p PageQuery) ApplyRawTo( sql sq.SelectBuilder, col string, ) (sq.SelectBuilder, error) { - return p.ApplyToUsingCursor(sql, col, p.Cursor) + return p.applyToUsingCursor(sql, col, p.Cursor) } // ApplyToUsingCursor returns a new SelectBuilder after applying the paging effects of // `p` to `sql`. This method allows any type of cursor by a single column -func (p PageQuery) ApplyToUsingCursor( +func (p PageQuery) applyToUsingCursor( sql sq.SelectBuilder, col string, cursor interface{}, @@ -81,23 +81,23 @@ func (p PageQuery) ApplyToUsingCursor( switch p.Order { case "asc": - if cursor == "" { + if cursor != "" { sql = sql. - OrderBy(fmt.Sprintf("%s asc", col)) - } else { - sql = sql. - Where(fmt.Sprintf("%s > ?", col), cursor). - OrderBy(fmt.Sprintf("%s asc", col)) + Where(fmt.Sprintf("%s > ?", col), cursor) } + sql = sql. + OrderBy(fmt.Sprintf("%s asc", col)) case "desc": - if cursor == "" { + if cursor != "" { sql = sql. - OrderBy(fmt.Sprintf("%s desc", col)) - } else { + Where(fmt.Sprintf("%s < ?", col), cursor) + } + if p.ElderCursor != "" { sql = sql. - Where(fmt.Sprintf("%s < ?", col), cursor). - OrderBy(fmt.Sprintf("%s desc", col)) + Where(fmt.Sprintf("%s > ?", col), p.ElderCursor) } + sql = sql. + OrderBy(fmt.Sprintf("%s desc", col)) default: return sql, errors.Errorf("invalid order: %s", p.Order) } From 8a9f62478014c1b8daed8c87f2b0e8e8a4500981 Mon Sep 17 00:00:00 2001 From: Urvi Date: Thu, 29 Aug 2024 12:21:18 -0700 Subject: [PATCH 2/6] Reduce the scope of change to asc order --- services/horizon/internal/actions/helpers.go | 10 ++----- .../internal/db2/history/account_signers.go | 2 +- .../horizon/internal/db2/history/accounts.go | 8 +++--- services/horizon/internal/db2/main.go | 7 ++--- services/horizon/internal/db2/page_query.go | 28 +++++++++---------- 5 files changed, 25 insertions(+), 30 deletions(-) diff --git a/services/horizon/internal/actions/helpers.go b/services/horizon/internal/actions/helpers.go index bd9608d97a..f4ddeb5b89 100644 --- a/services/horizon/internal/actions/helpers.go +++ b/services/horizon/internal/actions/helpers.go @@ -221,18 +221,14 @@ func GetPageQuery(ledgerState *ledger.State, r *http.Request, opts ...Opt) (db2. return db2.PageQuery{}, err } - elderCursor := toid.AfterLedger( - ordered.Max(0, ledgerState.CurrentStatus().HistoryElder-1), - ).String() if defaultTOID && pageQuery.Order == db2.OrderAscending { if cursor == "" || errors.Is(validateCursorWithinHistory(ledgerState, pageQuery), &hProblem.BeforeHistory) { - pageQuery.Cursor = elderCursor + pageQuery.Cursor = toid.AfterLedger( + ordered.Max(0, ledgerState.CurrentStatus().HistoryElder-1), + ).String() } - } else if defaultTOID && pageQuery.Order == db2.OrderDescending { - pageQuery.ElderCursor = elderCursor } - return pageQuery, nil } diff --git a/services/horizon/internal/db2/history/account_signers.go b/services/horizon/internal/db2/history/account_signers.go index 8014379a4b..78afcdf06c 100644 --- a/services/horizon/internal/db2/history/account_signers.go +++ b/services/horizon/internal/db2/history/account_signers.go @@ -37,7 +37,7 @@ func (q *Q) SignersForAccounts(ctx context.Context, accounts []string) ([]Accoun // AccountsForSigner returns a list of `AccountSigner` rows for a given signer func (q *Q) AccountsForSigner(ctx context.Context, signer string, page db2.PageQuery) ([]AccountSigner, error) { sql := selectAccountSigners.Where("accounts_signers.signer = ?", signer) - sql, err := page.ApplyRawTo(sql, "accounts_signers.account_id") + sql, err := page.ApplyToUsingCursor(sql, "accounts_signers.account_id", page.Cursor) if err != nil { return nil, errors.Wrap(err, "could not apply query to page") } diff --git a/services/horizon/internal/db2/history/accounts.go b/services/horizon/internal/db2/history/accounts.go index 8f3b79a4fe..2af197da4f 100644 --- a/services/horizon/internal/db2/history/accounts.go +++ b/services/horizon/internal/db2/history/accounts.go @@ -144,7 +144,7 @@ func (q *Q) AccountsForAsset(ctx context.Context, asset xdr.Asset, page db2.Page "trust_lines.asset_code": code, }) - sql, err := page.ApplyRawTo(sql, "trust_lines.account_id") + sql, err := page.ApplyToUsingCursor(sql, "trust_lines.account_id", page.Cursor) if err != nil { return nil, errors.Wrap(err, "could not apply query to page") } @@ -168,7 +168,7 @@ func (q *Q) AccountsForLiquidityPool(ctx context.Context, poolID string, page db "trust_lines.liquidity_pool_id": poolID, }) - sql, err := page.ApplyRawTo(sql, "trust_lines.account_id") + sql, err := page.ApplyToUsingCursor(sql, "trust_lines.account_id", page.Cursor) if err != nil { return nil, errors.Wrap(err, "could not apply query to page") } @@ -189,7 +189,7 @@ func selectBySponsor(table, sponsor string, page db2.PageQuery) (sq.SelectBuilde "sponsor": sponsor, }) - sql, err := page.ApplyRawTo(sql, "account_id") + sql, err := page.ApplyToUsingCursor(sql, "account_id", page.Cursor) if err != nil { return sql, errors.Wrap(err, "could not apply query to page") } @@ -255,7 +255,7 @@ func (q *Q) AccountEntriesForSigner(ctx context.Context, signer string, page db2 "accounts_signers.signer": signer, }) - sql, err := page.ApplyRawTo(sql, "accounts_signers.account_id") + sql, err := page.ApplyToUsingCursor(sql, "accounts_signers.account_id", page.Cursor) if err != nil { return nil, errors.Wrap(err, "could not apply query to page") } diff --git a/services/horizon/internal/db2/main.go b/services/horizon/internal/db2/main.go index c497817061..5ec81eb0f3 100644 --- a/services/horizon/internal/db2/main.go +++ b/services/horizon/internal/db2/main.go @@ -5,8 +5,7 @@ package db2 // PageQuery represents a portion of a Query struct concerned with paging // through a large dataset. type PageQuery struct { - Cursor string - Order string - Limit uint64 - ElderCursor string + Cursor string + Order string + Limit uint64 } diff --git a/services/horizon/internal/db2/page_query.go b/services/horizon/internal/db2/page_query.go index 988b294366..3c458af855 100644 --- a/services/horizon/internal/db2/page_query.go +++ b/services/horizon/internal/db2/page_query.go @@ -59,7 +59,7 @@ func (p PageQuery) ApplyTo( return sql, err } - return p.applyToUsingCursor(sql, col, cursor) + return p.ApplyToUsingCursor(sql, col, cursor) } // ApplyRawTo applies the raw PageQuery.Cursor cursor to the builder @@ -67,12 +67,12 @@ func (p PageQuery) ApplyRawTo( sql sq.SelectBuilder, col string, ) (sq.SelectBuilder, error) { - return p.applyToUsingCursor(sql, col, p.Cursor) + return p.ApplyToUsingCursor(sql, col, p.Cursor) } // ApplyToUsingCursor returns a new SelectBuilder after applying the paging effects of // `p` to `sql`. This method allows any type of cursor by a single column -func (p PageQuery) applyToUsingCursor( +func (p PageQuery) ApplyToUsingCursor( sql sq.SelectBuilder, col string, cursor interface{}, @@ -81,23 +81,23 @@ func (p PageQuery) applyToUsingCursor( switch p.Order { case "asc": - if cursor != "" { + if cursor == "" { sql = sql. - Where(fmt.Sprintf("%s > ?", col), cursor) + OrderBy(fmt.Sprintf("%s asc", col)) + } else { + sql = sql. + Where(fmt.Sprintf("%s > ?", col), cursor). + OrderBy(fmt.Sprintf("%s asc", col)) } - sql = sql. - OrderBy(fmt.Sprintf("%s asc", col)) case "desc": - if cursor != "" { + if cursor == "" { sql = sql. - Where(fmt.Sprintf("%s < ?", col), cursor) - } - if p.ElderCursor != "" { + OrderBy(fmt.Sprintf("%s desc", col)) + } else { sql = sql. - Where(fmt.Sprintf("%s > ?", col), p.ElderCursor) + Where(fmt.Sprintf("%s < ?", col), cursor). + OrderBy(fmt.Sprintf("%s desc", col)) } - sql = sql. - OrderBy(fmt.Sprintf("%s desc", col)) default: return sql, errors.Errorf("invalid order: %s", p.Order) } From e0b3d98f1c9ea285c8ef6a3af7a71ebaba13ae2a Mon Sep 17 00:00:00 2001 From: Urvi Date: Thu, 29 Aug 2024 14:15:19 -0700 Subject: [PATCH 3/6] Add unit tests --- services/horizon/internal/actions/helpers.go | 6 +- .../horizon/internal/actions/helpers_test.go | 110 ++++++++++++++++++ 2 files changed, 115 insertions(+), 1 deletion(-) diff --git a/services/horizon/internal/actions/helpers.go b/services/horizon/internal/actions/helpers.go index f4ddeb5b89..a2ce9675eb 100644 --- a/services/horizon/internal/actions/helpers.go +++ b/services/horizon/internal/actions/helpers.go @@ -223,7 +223,7 @@ func GetPageQuery(ledgerState *ledger.State, r *http.Request, opts ...Opt) (db2. } if defaultTOID && pageQuery.Order == db2.OrderAscending { - if cursor == "" || errors.Is(validateCursorWithinHistory(ledgerState, pageQuery), &hProblem.BeforeHistory) { + if cursor == "" || errors.Is(validateCursor(ledgerState, pageQuery), &hProblem.BeforeHistory) { pageQuery.Cursor = toid.AfterLedger( ordered.Max(0, ledgerState.CurrentStatus().HistoryElder-1), ).String() @@ -566,6 +566,10 @@ func validateCursorWithinHistory(ledgerState *ledger.State, pq db2.PageQuery) er return nil } + return validateCursor(ledgerState, pq) +} + +func validateCursor(ledgerState *ledger.State, pq db2.PageQuery) error { var cursor int64 var err error diff --git a/services/horizon/internal/actions/helpers_test.go b/services/horizon/internal/actions/helpers_test.go index 05e840577e..b18a6b4b1f 100644 --- a/services/horizon/internal/actions/helpers_test.go +++ b/services/horizon/internal/actions/helpers_test.go @@ -345,7 +345,117 @@ func TestGetPageQueryCursorDefaultTOID(t *testing.T) { assert.Equal(t, "", pq.Cursor) assert.Equal(t, uint64(2), pq.Limit) assert.Equal(t, "desc", pq.Order) +} + +func TestPageQueryCursorFullHistory(t *testing.T) { + ledgerState := &ledger.State{} + ledgerState.SetHorizonStatus(ledger.HorizonStatus{ + HistoryLatest: 7000, + HistoryLatestClosedAt: time.Now(), + HistoryElder: 0, + ExpHistoryLatest: 7000, + }) + + ascReq := makeTestActionRequest(fmt.Sprintf("/foo-bar/blah?cursor=%s&limit=2", toid.AfterLedger(200).String()), testURLParams()) + pq, err := GetPageQuery(ledgerState, ascReq, DefaultTOID) + assert.NoError(t, err) + assert.Equal(t, toid.AfterLedger(200).String(), pq.Cursor) + assert.Equal(t, uint64(2), pq.Limit) + assert.Equal(t, "asc", pq.Order) + + ascReq = makeTestActionRequest(fmt.Sprintf("/foo-bar/blah?cursor=%s&limit=2", toid.AfterLedger(200).String()), testURLParams()) + pq, err = GetPageQuery(ledgerState, ascReq) + assert.NoError(t, err) + assert.Equal(t, toid.AfterLedger(200).String(), pq.Cursor) + assert.Equal(t, uint64(2), pq.Limit) + assert.Equal(t, "asc", pq.Order) + + descReq := makeTestActionRequest(fmt.Sprintf("/foo-bar/blah?cursor=%s&limit=2&order=desc", toid.AfterLedger(200).String()), testURLParams()) + pq, err = GetPageQuery(ledgerState, descReq, DefaultTOID) + assert.NoError(t, err) + assert.Equal(t, toid.AfterLedger(200).String(), pq.Cursor) + assert.Equal(t, uint64(2), pq.Limit) + assert.Equal(t, "desc", pq.Order) + + descReq = makeTestActionRequest(fmt.Sprintf("/foo-bar/blah?cursor=%s&limit=2&order=desc", toid.AfterLedger(200).String()), testURLParams()) + pq, err = GetPageQuery(ledgerState, descReq) + assert.NoError(t, err) + assert.Equal(t, toid.AfterLedger(200).String(), pq.Cursor) + assert.Equal(t, uint64(2), pq.Limit) + assert.Equal(t, "desc", pq.Order) +} + +func TestPageQueryCursorLimitedHistory(t *testing.T) { + ledgerState := &ledger.State{} + ledgerState.SetHorizonStatus(ledger.HorizonStatus{ + HistoryLatest: 7000, + HistoryLatestClosedAt: time.Now(), + HistoryElder: 300, + ExpHistoryLatest: 7000, + }) + + ascReq := makeTestActionRequest(fmt.Sprintf("/foo-bar/blah?cursor=%s&limit=2", toid.AfterLedger(200).String()), testURLParams()) + pq, err := GetPageQuery(ledgerState, ascReq) + assert.NoError(t, err) + assert.Equal(t, toid.AfterLedger(200).String(), pq.Cursor) + assert.Equal(t, uint64(2), pq.Limit) + assert.Equal(t, "asc", pq.Order) + ascReq = makeTestActionRequest(fmt.Sprintf("/foo-bar/blah?cursor=%s&limit=2", toid.AfterLedger(200).String()), testURLParams()) + pq, err = GetPageQuery(ledgerState, ascReq, DefaultTOID) + assert.NoError(t, err) + assert.Equal(t, toid.AfterLedger(299).String(), pq.Cursor) + assert.Equal(t, uint64(2), pq.Limit) + assert.Equal(t, "asc", pq.Order) + + ascReq = makeTestActionRequest(fmt.Sprintf("/foo-bar/blah?cursor=%s&limit=2", toid.AfterLedger(298).String()), testURLParams()) + pq, err = GetPageQuery(ledgerState, ascReq, DefaultTOID) + assert.NoError(t, err) + assert.Equal(t, toid.AfterLedger(299).String(), pq.Cursor) + assert.Equal(t, uint64(2), pq.Limit) + assert.Equal(t, "asc", pq.Order) + + ascReq = makeTestActionRequest(fmt.Sprintf("/foo-bar/blah?cursor=%s&limit=2", toid.AfterLedger(299).String()), testURLParams()) + pq, err = GetPageQuery(ledgerState, ascReq, DefaultTOID) + assert.NoError(t, err) + assert.Equal(t, toid.AfterLedger(299).String(), pq.Cursor) + assert.Equal(t, uint64(2), pq.Limit) + assert.Equal(t, "asc", pq.Order) + + ascReq = makeTestActionRequest(fmt.Sprintf("/foo-bar/blah?cursor=%s&limit=2", toid.AfterLedger(300).String()), testURLParams()) + pq, err = GetPageQuery(ledgerState, ascReq, DefaultTOID) + assert.NoError(t, err) + assert.Equal(t, toid.AfterLedger(300).String(), pq.Cursor) + assert.Equal(t, uint64(2), pq.Limit) + assert.Equal(t, "asc", pq.Order) + + ascReq = makeTestActionRequest(fmt.Sprintf("/foo-bar/blah?cursor=%s&limit=2", toid.AfterLedger(301).String()), testURLParams()) + pq, err = GetPageQuery(ledgerState, ascReq, DefaultTOID) + assert.NoError(t, err) + assert.Equal(t, toid.AfterLedger(301).String(), pq.Cursor) + assert.Equal(t, uint64(2), pq.Limit) + assert.Equal(t, "asc", pq.Order) + + descReq := makeTestActionRequest(fmt.Sprintf("/foo-bar/blah?cursor=%s&limit=2&order=desc", toid.AfterLedger(298).String()), testURLParams()) + pq, err = GetPageQuery(ledgerState, descReq, DefaultTOID) + assert.NoError(t, err) + assert.Equal(t, toid.AfterLedger(298).String(), pq.Cursor) + assert.Equal(t, uint64(2), pq.Limit) + assert.Equal(t, "desc", pq.Order) + + descReq = makeTestActionRequest(fmt.Sprintf("/foo-bar/blah?cursor=%s&limit=2&order=desc", toid.AfterLedger(320).String()), testURLParams()) + pq, err = GetPageQuery(ledgerState, descReq, DefaultTOID) + assert.NoError(t, err) + assert.Equal(t, toid.AfterLedger(320).String(), pq.Cursor) + assert.Equal(t, uint64(2), pq.Limit) + assert.Equal(t, "desc", pq.Order) + + descReq = makeTestActionRequest(fmt.Sprintf("/foo-bar/blah?cursor=%s&limit=2&order=desc", toid.AfterLedger(298).String()), testURLParams()) + pq, err = GetPageQuery(ledgerState, descReq) + assert.NoError(t, err) + assert.Equal(t, toid.AfterLedger(298).String(), pq.Cursor) + assert.Equal(t, uint64(2), pq.Limit) + assert.Equal(t, "desc", pq.Order) } func TestGetString(t *testing.T) { From b27c09e8db3a6b32802224d3522b7a16d2bd8383 Mon Sep 17 00:00:00 2001 From: Urvi Date: Wed, 4 Sep 2024 10:47:27 -0700 Subject: [PATCH 4/6] minor refactor --- services/horizon/internal/actions/effects.go | 4 +- services/horizon/internal/actions/helpers.go | 51 ++--- .../horizon/internal/actions/helpers_test.go | 210 +++++++++--------- services/horizon/internal/actions/ledger.go | 4 +- .../horizon/internal/actions/operation.go | 4 +- services/horizon/internal/actions/trade.go | 6 +- .../horizon/internal/actions/transaction.go | 4 +- 7 files changed, 137 insertions(+), 146 deletions(-) diff --git a/services/horizon/internal/actions/effects.go b/services/horizon/internal/actions/effects.go index e04997aca5..d8620fc11f 100644 --- a/services/horizon/internal/actions/effects.go +++ b/services/horizon/internal/actions/effects.go @@ -51,12 +51,12 @@ type GetEffectsHandler struct { } func (handler GetEffectsHandler) GetResourcePage(w HeaderWriter, r *http.Request) ([]hal.Pageable, error) { - pq, err := GetPageQuery(handler.LedgerState, r, DefaultTOID) + pq, err := GetPageQuery(handler.LedgerState, r) if err != nil { return nil, err } - err = validateCursorWithinHistory(handler.LedgerState, pq) + err = validateAndAdjustCursor(handler.LedgerState, &pq) if err != nil { return nil, err } diff --git a/services/horizon/internal/actions/helpers.go b/services/horizon/internal/actions/helpers.go index a2ce9675eb..8db4674991 100644 --- a/services/horizon/internal/actions/helpers.go +++ b/services/horizon/internal/actions/helpers.go @@ -45,8 +45,6 @@ type Opt int const ( // DisableCursorValidation disables cursor validation in GetPageQuery DisableCursorValidation Opt = iota - // DefaultTOID sets a default cursor value in GetPageQuery based on the ledger state - DefaultTOID Opt = iota ) // HeaderWriter is an interface for setting HTTP response headers @@ -185,14 +183,10 @@ func getLimit(r *http.Request, name string, def uint64, max uint64) (uint64, err // using the results from a call to GetPagingParams() func GetPageQuery(ledgerState *ledger.State, r *http.Request, opts ...Opt) (db2.PageQuery, error) { disableCursorValidation := false - defaultTOID := false for _, opt := range opts { if opt == DisableCursorValidation { disableCursorValidation = true } - if opt == DefaultTOID { - defaultTOID = true - } } cursor, err := getCursor(ledgerState, r, ParamCursor) @@ -222,13 +216,6 @@ func GetPageQuery(ledgerState *ledger.State, r *http.Request, opts ...Opt) (db2. return db2.PageQuery{}, err } - if defaultTOID && pageQuery.Order == db2.OrderAscending { - if cursor == "" || errors.Is(validateCursor(ledgerState, pageQuery), &hProblem.BeforeHistory) { - pageQuery.Cursor = toid.AfterLedger( - ordered.Max(0, ledgerState.CurrentStatus().HistoryElder-1), - ).String() - } - } return pageQuery, nil } @@ -553,23 +540,37 @@ func validateAssetParams(aType, code, issuer, prefix string) error { return nil } -// validateCursorWithinHistory compares the requested page of data against the +// validateAndAdjustCursor compares the requested page of data against the // ledger state of the history database. In the event that the cursor is // guaranteed to return no results, we return a 410 GONE http response. -func validateCursorWithinHistory(ledgerState *ledger.State, pq db2.PageQuery) error { - // 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 - // rather return an empty page (allowing code that tracks the procession of - // some resource more easily). - if pq.Order != "desc" { - return nil +// 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 { + + if pq.Order == db2.OrderDescending { + return validateCursorWithinHistory(ledgerState, *pq) + } else 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 + // rather return an empty page (allowing code that tracks the procession of + // some resource more easily). + + // 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) { + pq.Cursor = toid.AfterLedger( + ordered.Max(0, ledgerState.CurrentStatus().HistoryElder-1), + ).String() + } } - - return validateCursor(ledgerState, pq) + return nil } -func validateCursor(ledgerState *ledger.State, pq db2.PageQuery) error { +// validateCursorWithinHistory checks if the cursor is within the known history range. +// If the cursor is before the oldest available ledger, it returns BeforeHistory error. +func validateCursorWithinHistory(ledgerState *ledger.State, pq db2.PageQuery) error { var cursor int64 var err error diff --git a/services/horizon/internal/actions/helpers_test.go b/services/horizon/internal/actions/helpers_test.go index b18a6b4b1f..97a5d92c8d 100644 --- a/services/horizon/internal/actions/helpers_test.go +++ b/services/horizon/internal/actions/helpers_test.go @@ -16,6 +16,7 @@ import ( horizonContext "github.com/stellar/go/services/horizon/internal/context" "github.com/stellar/go/services/horizon/internal/db2" "github.com/stellar/go/services/horizon/internal/ledger" + hProblem "github.com/stellar/go/services/horizon/internal/render/problem" "github.com/stellar/go/services/horizon/internal/test" "github.com/stellar/go/support/db" "github.com/stellar/go/support/errors" @@ -126,11 +127,21 @@ func TestValidateCursorWithinHistory(t *testing.T) { { cursor: "0", order: "asc", - valid: true, + valid: false, }, { cursor: "0-1234", order: "asc", + valid: false, + }, + { + cursor: "1", + order: "asc", + valid: true, + }, + { + cursor: "1-1234", + order: "asc", valid: true, }, } @@ -291,11 +302,10 @@ func TestGetPageQuery(t *testing.T) { tt.Assert.Error(err) } -func TestGetPageQueryCursorDefaultTOID(t *testing.T) { - ascReq := makeTestActionRequest("/foo-bar/blah?limit=2", testURLParams()) - descReq := makeTestActionRequest("/foo-bar/blah?limit=2&order=desc", testURLParams()) - +func TestPageQueryCursorDefaultOrder(t *testing.T) { ledgerState := &ledger.State{} + + // truncated history ledgerState.SetHorizonStatus(ledger.HorizonStatus{ HistoryLatest: 7000, HistoryLatestClosedAt: time.Now(), @@ -303,30 +313,30 @@ func TestGetPageQueryCursorDefaultTOID(t *testing.T) { ExpHistoryLatest: 7000, }) - pq, err := GetPageQuery(ledgerState, ascReq, DefaultTOID) + req := makeTestActionRequest("/foo-bar/blah?limit=2", testURLParams()) + + // default asc, w/o cursor + pq, err := GetPageQuery(ledgerState, req) assert.NoError(t, err) + assert.Empty(t, pq.Cursor) + assert.Error(t, &hProblem.BeforeHistory, validateAndAdjustCursor(ledgerState, &pq)) assert.Equal(t, toid.AfterLedger(299).String(), pq.Cursor) assert.Equal(t, uint64(2), pq.Limit) assert.Equal(t, "asc", pq.Order) - pq, err = GetPageQuery(ledgerState, descReq, DefaultTOID) - assert.NoError(t, err) - assert.Equal(t, "", pq.Cursor) - assert.Equal(t, uint64(2), pq.Limit) - assert.Equal(t, "desc", pq.Order) + cursor := toid.AfterLedger(200).String() + reqWithCursor := makeTestActionRequest(fmt.Sprintf("/foo-bar/blah?cursor=%s&limit=2", cursor), testURLParams()) - pq, err = GetPageQuery(ledgerState, ascReq) + // default asc, w/ cursor + pq, err = GetPageQuery(ledgerState, reqWithCursor) assert.NoError(t, err) - assert.Empty(t, pq.Cursor) + assert.Equal(t, cursor, pq.Cursor) + assert.Error(t, &hProblem.BeforeHistory, validateAndAdjustCursor(ledgerState, &pq)) + assert.Equal(t, toid.AfterLedger(299).String(), pq.Cursor) assert.Equal(t, uint64(2), pq.Limit) assert.Equal(t, "asc", pq.Order) - pq, err = GetPageQuery(ledgerState, descReq) - assert.NoError(t, err) - assert.Empty(t, pq.Cursor) - assert.Equal(t, "", pq.Cursor) - assert.Equal(t, "desc", pq.Order) - + // full history ledgerState.SetHorizonStatus(ledger.HorizonStatus{ HistoryLatest: 7000, HistoryLatestClosedAt: time.Now(), @@ -334,128 +344,108 @@ func TestGetPageQueryCursorDefaultTOID(t *testing.T) { ExpHistoryLatest: 7000, }) - pq, err = GetPageQuery(ledgerState, ascReq, DefaultTOID) + // default asc, w/o cursor + pq, err = GetPageQuery(ledgerState, req) assert.NoError(t, err) + assert.Empty(t, pq.Cursor) + assert.Error(t, &hProblem.BeforeHistory, validateAndAdjustCursor(ledgerState, &pq)) assert.Equal(t, toid.AfterLedger(0).String(), pq.Cursor) assert.Equal(t, uint64(2), pq.Limit) assert.Equal(t, "asc", pq.Order) - pq, err = GetPageQuery(ledgerState, descReq, DefaultTOID) + // default asc, w/ cursor + pq, err = GetPageQuery(ledgerState, reqWithCursor) assert.NoError(t, err) - assert.Equal(t, "", pq.Cursor) + assert.Equal(t, cursor, pq.Cursor) + assert.Error(t, &hProblem.BeforeHistory, validateAndAdjustCursor(ledgerState, &pq)) + assert.Equal(t, cursor, pq.Cursor) assert.Equal(t, uint64(2), pq.Limit) - assert.Equal(t, "desc", pq.Order) + assert.Equal(t, "asc", pq.Order) + } -func TestPageQueryCursorFullHistory(t *testing.T) { +func TestGetPageQueryWithoutCursor(t *testing.T) { ledgerState := &ledger.State{} + + validateCursor := func(limit uint64, order string, expectedCursor string) { + req := makeTestActionRequest(fmt.Sprintf("/foo-bar/blah?limit=%d&order=%s", limit, order), testURLParams()) + pq, err := GetPageQuery(ledgerState, req) + assert.NoError(t, err) + assert.Empty(t, pq.Cursor) + assert.Error(t, &hProblem.BeforeHistory, validateAndAdjustCursor(ledgerState, &pq)) + assert.Equal(t, expectedCursor, pq.Cursor) + assert.Equal(t, limit, pq.Limit) + assert.Equal(t, order, pq.Order) + } + + // truncated history ledgerState.SetHorizonStatus(ledger.HorizonStatus{ HistoryLatest: 7000, HistoryLatestClosedAt: time.Now(), - HistoryElder: 0, + HistoryElder: 300, ExpHistoryLatest: 7000, }) - ascReq := makeTestActionRequest(fmt.Sprintf("/foo-bar/blah?cursor=%s&limit=2", toid.AfterLedger(200).String()), testURLParams()) - pq, err := GetPageQuery(ledgerState, ascReq, DefaultTOID) - assert.NoError(t, err) - assert.Equal(t, toid.AfterLedger(200).String(), pq.Cursor) - assert.Equal(t, uint64(2), pq.Limit) - assert.Equal(t, "asc", pq.Order) - - ascReq = makeTestActionRequest(fmt.Sprintf("/foo-bar/blah?cursor=%s&limit=2", toid.AfterLedger(200).String()), testURLParams()) - pq, err = GetPageQuery(ledgerState, ascReq) - assert.NoError(t, err) - assert.Equal(t, toid.AfterLedger(200).String(), pq.Cursor) - assert.Equal(t, uint64(2), pq.Limit) - assert.Equal(t, "asc", pq.Order) - - descReq := makeTestActionRequest(fmt.Sprintf("/foo-bar/blah?cursor=%s&limit=2&order=desc", toid.AfterLedger(200).String()), testURLParams()) - pq, err = GetPageQuery(ledgerState, descReq, DefaultTOID) - assert.NoError(t, err) - assert.Equal(t, toid.AfterLedger(200).String(), pq.Cursor) - assert.Equal(t, uint64(2), pq.Limit) - assert.Equal(t, "desc", pq.Order) + validateCursor(2, "asc", toid.AfterLedger(299).String()) + validateCursor(2, "desc", "") - descReq = makeTestActionRequest(fmt.Sprintf("/foo-bar/blah?cursor=%s&limit=2&order=desc", toid.AfterLedger(200).String()), testURLParams()) - pq, err = GetPageQuery(ledgerState, descReq) - assert.NoError(t, err) - assert.Equal(t, toid.AfterLedger(200).String(), pq.Cursor) - assert.Equal(t, uint64(2), pq.Limit) - assert.Equal(t, "desc", pq.Order) -} - -func TestPageQueryCursorLimitedHistory(t *testing.T) { - ledgerState := &ledger.State{} + // full history ledgerState.SetHorizonStatus(ledger.HorizonStatus{ HistoryLatest: 7000, HistoryLatestClosedAt: time.Now(), - HistoryElder: 300, + HistoryElder: 0, ExpHistoryLatest: 7000, }) - ascReq := makeTestActionRequest(fmt.Sprintf("/foo-bar/blah?cursor=%s&limit=2", toid.AfterLedger(200).String()), testURLParams()) - pq, err := GetPageQuery(ledgerState, ascReq) - assert.NoError(t, err) - assert.Equal(t, toid.AfterLedger(200).String(), pq.Cursor) - assert.Equal(t, uint64(2), pq.Limit) - assert.Equal(t, "asc", pq.Order) + validateCursor(2, "asc", toid.AfterLedger(0).String()) + validateCursor(2, "desc", "") +} - ascReq = makeTestActionRequest(fmt.Sprintf("/foo-bar/blah?cursor=%s&limit=2", toid.AfterLedger(200).String()), testURLParams()) - pq, err = GetPageQuery(ledgerState, ascReq, DefaultTOID) - assert.NoError(t, err) - assert.Equal(t, toid.AfterLedger(299).String(), pq.Cursor) - assert.Equal(t, uint64(2), pq.Limit) - assert.Equal(t, "asc", pq.Order) +func TestPageQueryWithCursor(t *testing.T) { + ledgerState := &ledger.State{} - ascReq = makeTestActionRequest(fmt.Sprintf("/foo-bar/blah?cursor=%s&limit=2", toid.AfterLedger(298).String()), testURLParams()) - pq, err = GetPageQuery(ledgerState, ascReq, DefaultTOID) - assert.NoError(t, err) - assert.Equal(t, toid.AfterLedger(299).String(), pq.Cursor) - assert.Equal(t, uint64(2), pq.Limit) - assert.Equal(t, "asc", pq.Order) + validateCursor := func(cursor string, limit uint64, order string, expectedCursor string) { + 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) + assert.Error(t, &hProblem.BeforeHistory, validateAndAdjustCursor(ledgerState, &pq)) + assert.Equal(t, expectedCursor, pq.Cursor) + assert.Equal(t, limit, pq.Limit) + assert.Equal(t, order, pq.Order) + } - ascReq = makeTestActionRequest(fmt.Sprintf("/foo-bar/blah?cursor=%s&limit=2", toid.AfterLedger(299).String()), testURLParams()) - pq, err = GetPageQuery(ledgerState, ascReq, DefaultTOID) - assert.NoError(t, err) - assert.Equal(t, toid.AfterLedger(299).String(), pq.Cursor) - assert.Equal(t, uint64(2), pq.Limit) - assert.Equal(t, "asc", pq.Order) + // full history + ledgerState.SetHorizonStatus(ledger.HorizonStatus{ + HistoryLatest: 7000, + HistoryLatestClosedAt: time.Now(), + HistoryElder: 0, + ExpHistoryLatest: 7000, + }) - ascReq = makeTestActionRequest(fmt.Sprintf("/foo-bar/blah?cursor=%s&limit=2", toid.AfterLedger(300).String()), testURLParams()) - pq, err = GetPageQuery(ledgerState, ascReq, DefaultTOID) - assert.NoError(t, err) - assert.Equal(t, toid.AfterLedger(300).String(), pq.Cursor) - assert.Equal(t, uint64(2), pq.Limit) - assert.Equal(t, "asc", pq.Order) + validateCursor(toid.AfterLedger(200).String(), 2, "asc", toid.AfterLedger(200).String()) + validateCursor(toid.AfterLedger(200).String(), 2, "desc", toid.AfterLedger(200).String()) - ascReq = makeTestActionRequest(fmt.Sprintf("/foo-bar/blah?cursor=%s&limit=2", toid.AfterLedger(301).String()), testURLParams()) - pq, err = GetPageQuery(ledgerState, ascReq, DefaultTOID) - assert.NoError(t, err) - assert.Equal(t, toid.AfterLedger(301).String(), pq.Cursor) - assert.Equal(t, uint64(2), pq.Limit) - assert.Equal(t, "asc", pq.Order) + // truncated history + ledgerState.SetHorizonStatus(ledger.HorizonStatus{ + HistoryLatest: 7000, + HistoryLatestClosedAt: time.Now(), + HistoryElder: 300, + ExpHistoryLatest: 7000, + }) - descReq := makeTestActionRequest(fmt.Sprintf("/foo-bar/blah?cursor=%s&limit=2&order=desc", toid.AfterLedger(298).String()), testURLParams()) - pq, err = GetPageQuery(ledgerState, descReq, DefaultTOID) - assert.NoError(t, err) - assert.Equal(t, toid.AfterLedger(298).String(), pq.Cursor) - assert.Equal(t, uint64(2), pq.Limit) - assert.Equal(t, "desc", pq.Order) + // asc order + validateCursor(toid.AfterLedger(200).String(), 2, "asc", toid.AfterLedger(299).String()) + validateCursor(toid.AfterLedger(298).String(), 2, "asc", toid.AfterLedger(299).String()) + validateCursor(toid.AfterLedger(299).String(), 2, "asc", toid.AfterLedger(299).String()) + validateCursor(toid.AfterLedger(300).String(), 2, "asc", toid.AfterLedger(300).String()) + validateCursor(toid.AfterLedger(301).String(), 2, "asc", toid.AfterLedger(301).String()) - descReq = makeTestActionRequest(fmt.Sprintf("/foo-bar/blah?cursor=%s&limit=2&order=desc", toid.AfterLedger(320).String()), testURLParams()) - pq, err = GetPageQuery(ledgerState, descReq, DefaultTOID) - assert.NoError(t, err) - assert.Equal(t, toid.AfterLedger(320).String(), pq.Cursor) - assert.Equal(t, uint64(2), pq.Limit) - assert.Equal(t, "desc", pq.Order) + // desc order + validateCursor(toid.AfterLedger(298).String(), 2, "desc", toid.AfterLedger(298).String()) + validateCursor(toid.AfterLedger(300).String(), 2, "desc", toid.AfterLedger(300).String()) + validateCursor(toid.AfterLedger(320).String(), 2, "desc", toid.AfterLedger(320).String()) - descReq = makeTestActionRequest(fmt.Sprintf("/foo-bar/blah?cursor=%s&limit=2&order=desc", toid.AfterLedger(298).String()), testURLParams()) - pq, err = GetPageQuery(ledgerState, descReq) - assert.NoError(t, err) - assert.Equal(t, toid.AfterLedger(298).String(), pq.Cursor) - assert.Equal(t, uint64(2), pq.Limit) - assert.Equal(t, "desc", pq.Order) } func TestGetString(t *testing.T) { diff --git a/services/horizon/internal/actions/ledger.go b/services/horizon/internal/actions/ledger.go index 0b3d51b8e1..0836ba9b10 100644 --- a/services/horizon/internal/actions/ledger.go +++ b/services/horizon/internal/actions/ledger.go @@ -17,12 +17,12 @@ type GetLedgersHandler struct { } func (handler GetLedgersHandler) GetResourcePage(w HeaderWriter, r *http.Request) ([]hal.Pageable, error) { - pq, err := GetPageQuery(handler.LedgerState, r, DefaultTOID) + pq, err := GetPageQuery(handler.LedgerState, r) if err != nil { return nil, err } - err = validateCursorWithinHistory(handler.LedgerState, pq) + err = validateAndAdjustCursor(handler.LedgerState, &pq) if err != nil { return nil, err } diff --git a/services/horizon/internal/actions/operation.go b/services/horizon/internal/actions/operation.go index 9ccadb272e..6b200cba29 100644 --- a/services/horizon/internal/actions/operation.go +++ b/services/horizon/internal/actions/operation.go @@ -72,12 +72,12 @@ type GetOperationsHandler struct { func (handler GetOperationsHandler) GetResourcePage(w HeaderWriter, r *http.Request) ([]hal.Pageable, error) { ctx := r.Context() - pq, err := GetPageQuery(handler.LedgerState, r, DefaultTOID) + pq, err := GetPageQuery(handler.LedgerState, r) if err != nil { return nil, err } - err = validateCursorWithinHistory(handler.LedgerState, pq) + err = validateAndAdjustCursor(handler.LedgerState, &pq) if err != nil { return nil, err } diff --git a/services/horizon/internal/actions/trade.go b/services/horizon/internal/actions/trade.go index b29ad64715..1e81dbb7a9 100644 --- a/services/horizon/internal/actions/trade.go +++ b/services/horizon/internal/actions/trade.go @@ -159,12 +159,12 @@ type GetTradesHandler struct { func (handler GetTradesHandler) GetResourcePage(w HeaderWriter, r *http.Request) ([]hal.Pageable, error) { ctx := r.Context() - pq, err := GetPageQuery(handler.LedgerState, r, DefaultTOID) + pq, err := GetPageQuery(handler.LedgerState, r) if err != nil { return nil, err } - err = validateCursorWithinHistory(handler.LedgerState, pq) + err = validateAndAdjustCursor(handler.LedgerState, &pq) if err != nil { return nil, err } @@ -287,7 +287,7 @@ func (handler GetTradeAggregationsHandler) GetResource(w HeaderWriter, r *http.R if err != nil { return nil, err } - err = validateCursorWithinHistory(handler.LedgerState, pq) + err = validateAndAdjustCursor(handler.LedgerState, &pq) if err != nil { return nil, err } diff --git a/services/horizon/internal/actions/transaction.go b/services/horizon/internal/actions/transaction.go index 6a2fd1c6e2..ed579f9d6f 100644 --- a/services/horizon/internal/actions/transaction.go +++ b/services/horizon/internal/actions/transaction.go @@ -98,12 +98,12 @@ type GetTransactionsHandler struct { func (handler GetTransactionsHandler) GetResourcePage(w HeaderWriter, r *http.Request) ([]hal.Pageable, error) { ctx := r.Context() - pq, err := GetPageQuery(handler.LedgerState, r, DefaultTOID) + pq, err := GetPageQuery(handler.LedgerState, r) if err != nil { return nil, err } - err = validateCursorWithinHistory(handler.LedgerState, pq) + err = validateAndAdjustCursor(handler.LedgerState, &pq) if err != nil { return nil, err } From a492f617fd73de2d3c1e9ec1f0eadc9acda0950f Mon Sep 17 00:00:00 2001 From: Urvi Date: Wed, 4 Sep 2024 13:50:51 -0700 Subject: [PATCH 5/6] fix unit tests --- .../horizon/internal/actions/helpers_test.go | 52 ++++++++++++------- 1 file changed, 33 insertions(+), 19 deletions(-) diff --git a/services/horizon/internal/actions/helpers_test.go b/services/horizon/internal/actions/helpers_test.go index 97a5d92c8d..1c7cd6c1f7 100644 --- a/services/horizon/internal/actions/helpers_test.go +++ b/services/horizon/internal/actions/helpers_test.go @@ -319,7 +319,7 @@ func TestPageQueryCursorDefaultOrder(t *testing.T) { pq, err := GetPageQuery(ledgerState, req) assert.NoError(t, err) assert.Empty(t, pq.Cursor) - assert.Error(t, &hProblem.BeforeHistory, validateAndAdjustCursor(ledgerState, &pq)) + assert.NoError(t, validateAndAdjustCursor(ledgerState, &pq)) assert.Equal(t, toid.AfterLedger(299).String(), pq.Cursor) assert.Equal(t, uint64(2), pq.Limit) assert.Equal(t, "asc", pq.Order) @@ -331,7 +331,7 @@ func TestPageQueryCursorDefaultOrder(t *testing.T) { pq, err = GetPageQuery(ledgerState, reqWithCursor) assert.NoError(t, err) assert.Equal(t, cursor, pq.Cursor) - assert.Error(t, &hProblem.BeforeHistory, validateAndAdjustCursor(ledgerState, &pq)) + assert.NoError(t, validateAndAdjustCursor(ledgerState, &pq)) assert.Equal(t, toid.AfterLedger(299).String(), pq.Cursor) assert.Equal(t, uint64(2), pq.Limit) assert.Equal(t, "asc", pq.Order) @@ -348,7 +348,7 @@ func TestPageQueryCursorDefaultOrder(t *testing.T) { pq, err = GetPageQuery(ledgerState, req) assert.NoError(t, err) assert.Empty(t, pq.Cursor) - assert.Error(t, &hProblem.BeforeHistory, validateAndAdjustCursor(ledgerState, &pq)) + assert.NoError(t, validateAndAdjustCursor(ledgerState, &pq)) assert.Equal(t, toid.AfterLedger(0).String(), pq.Cursor) assert.Equal(t, uint64(2), pq.Limit) assert.Equal(t, "asc", pq.Order) @@ -357,7 +357,7 @@ func TestPageQueryCursorDefaultOrder(t *testing.T) { pq, err = GetPageQuery(ledgerState, reqWithCursor) assert.NoError(t, err) assert.Equal(t, cursor, pq.Cursor) - assert.Error(t, &hProblem.BeforeHistory, validateAndAdjustCursor(ledgerState, &pq)) + assert.NoError(t, validateAndAdjustCursor(ledgerState, &pq)) assert.Equal(t, cursor, pq.Cursor) assert.Equal(t, uint64(2), pq.Limit) assert.Equal(t, "asc", pq.Order) @@ -372,7 +372,7 @@ func TestGetPageQueryWithoutCursor(t *testing.T) { pq, err := GetPageQuery(ledgerState, req) assert.NoError(t, err) assert.Empty(t, pq.Cursor) - assert.Error(t, &hProblem.BeforeHistory, validateAndAdjustCursor(ledgerState, &pq)) + assert.NoError(t, validateAndAdjustCursor(ledgerState, &pq)) assert.Equal(t, expectedCursor, pq.Cursor) assert.Equal(t, limit, pq.Limit) assert.Equal(t, order, pq.Order) @@ -401,15 +401,20 @@ func TestGetPageQueryWithoutCursor(t *testing.T) { validateCursor(2, "desc", "") } -func TestPageQueryWithCursor(t *testing.T) { +func TestGetPageQueryWithCursor(t *testing.T) { ledgerState := &ledger.State{} - validateCursor := func(cursor string, limit uint64, order string, expectedCursor string) { + 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) - assert.Error(t, &hProblem.BeforeHistory, validateAndAdjustCursor(ledgerState, &pq)) + err = validateAndAdjustCursor(ledgerState, &pq) + if expectedError != nil { + assert.EqualError(t, expectedError, err.Error()) + } else { + assert.NoError(t, err) + } assert.Equal(t, expectedCursor, pq.Cursor) assert.Equal(t, limit, pq.Limit) assert.Equal(t, order, pq.Order) @@ -423,8 +428,13 @@ func TestPageQueryWithCursor(t *testing.T) { ExpHistoryLatest: 7000, }) - validateCursor(toid.AfterLedger(200).String(), 2, "asc", toid.AfterLedger(200).String()) - validateCursor(toid.AfterLedger(200).String(), 2, "desc", toid.AfterLedger(200).String()) + 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) + + validateCursor(toid.AfterLedger(0).String(), 2, "desc", toid.AfterLedger(0).String(), nil) + validateCursor(toid.AfterLedger(200).String(), 2, "desc", toid.AfterLedger(200).String(), nil) + validateCursor(toid.AfterLedger(7001).String(), 2, "desc", toid.AfterLedger(7001).String(), nil) // truncated history ledgerState.SetHorizonStatus(ledger.HorizonStatus{ @@ -435,17 +445,21 @@ func TestPageQueryWithCursor(t *testing.T) { }) // asc order - validateCursor(toid.AfterLedger(200).String(), 2, "asc", toid.AfterLedger(299).String()) - validateCursor(toid.AfterLedger(298).String(), 2, "asc", toid.AfterLedger(299).String()) - validateCursor(toid.AfterLedger(299).String(), 2, "asc", toid.AfterLedger(299).String()) - validateCursor(toid.AfterLedger(300).String(), 2, "asc", toid.AfterLedger(300).String()) - validateCursor(toid.AfterLedger(301).String(), 2, "asc", toid.AfterLedger(301).String()) + validateCursor(toid.AfterLedger(0).String(), 2, "asc", toid.AfterLedger(299).String(), nil) + validateCursor(toid.AfterLedger(200).String(), 2, "asc", toid.AfterLedger(299).String(), nil) + validateCursor(toid.AfterLedger(298).String(), 2, "asc", toid.AfterLedger(299).String(), nil) + validateCursor(toid.AfterLedger(299).String(), 2, "asc", toid.AfterLedger(299).String(), nil) + validateCursor(toid.AfterLedger(300).String(), 2, "asc", toid.AfterLedger(300).String(), nil) + validateCursor(toid.AfterLedger(301).String(), 2, "asc", toid.AfterLedger(301).String(), nil) + validateCursor(toid.AfterLedger(7001).String(), 2, "asc", toid.AfterLedger(7001).String(), nil) // desc order - validateCursor(toid.AfterLedger(298).String(), 2, "desc", toid.AfterLedger(298).String()) - validateCursor(toid.AfterLedger(300).String(), 2, "desc", toid.AfterLedger(300).String()) - validateCursor(toid.AfterLedger(320).String(), 2, "desc", toid.AfterLedger(320).String()) - + validateCursor(toid.AfterLedger(0).String(), 2, "desc", toid.AfterLedger(0).String(), hProblem.BeforeHistory) + validateCursor(toid.AfterLedger(200).String(), 2, "desc", toid.AfterLedger(200).String(), hProblem.BeforeHistory) + validateCursor(toid.AfterLedger(299).String(), 2, "desc", toid.AfterLedger(299).String(), hProblem.BeforeHistory) + validateCursor(toid.AfterLedger(300).String(), 2, "desc", toid.AfterLedger(300).String(), nil) + validateCursor(toid.AfterLedger(320).String(), 2, "desc", toid.AfterLedger(320).String(), nil) + validateCursor(toid.AfterLedger(7001).String(), 2, "desc", toid.AfterLedger(7001).String(), nil) } func TestGetString(t *testing.T) { From 5c8c0cc273d3e6fba557cbb8b92f75d30e9f0f84 Mon Sep 17 00:00:00 2001 From: Urvi Date: Fri, 6 Sep 2024 01:44:25 -0700 Subject: [PATCH 6/6] 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