Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

services/horizon/internal/db2/history: Fix claimable_balance_claimants subquery in GetClaimableBalances() #5207

Merged
merged 1 commit into from
Feb 14, 2024
Merged

services/horizon/internal/db2/history: Fix claimable_balance_claimants subquery in GetClaimableBalances() #5207

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 21 additions & 11 deletions services/horizon/internal/db2/history/claimable_balances.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@

sq "github.com/Masterminds/squirrel"
"github.com/guregu/null"

"github.com/stellar/go/services/horizon/internal/db2"
"github.com/stellar/go/support/errors"
"github.com/stellar/go/xdr"
Expand Down Expand Up @@ -57,7 +58,7 @@
// ApplyCursor applies cursor to the given sql. For performance reason the limit
// is not applied here. This allows us to hint the planner later to use the right
// indexes.
func applyClaimableBalancesQueriesCursor(sql sq.SelectBuilder, lCursor int64, rCursor string, order string) (sq.SelectBuilder, error) {
func applyClaimableBalancesQueriesCursor(sql sq.SelectBuilder, tableName string, lCursor int64, rCursor string, order string) (sq.SelectBuilder, error) {

Check failure on line 61 in services/horizon/internal/db2/history/claimable_balances.go

View workflow job for this annotation

GitHub Actions / golangci 

line is 153 characters (lll)
hasPagedLimit := false
if lCursor > 0 && rCursor != "" {
hasPagedLimit = true
Expand All @@ -67,17 +68,26 @@
case db2.OrderAscending:
if hasPagedLimit {
sql = sql.
Where(sq.Expr("(cb.last_modified_ledger, cb.id) > (?, ?)", lCursor, rCursor))

Where(
sq.Expr(
fmt.Sprintf("(%s.last_modified_ledger, %s.id) > (?, ?)", tableName, tableName),
Copy link
Contributor

@sreuland sreuland Feb 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

embedded sql text fragment assemblies become so tedious, difficult to recognize the overall sql statement, not suggesting anything here, just mentioning go-jet again, to see if type-safe sql bindings re-resonate with anyone. the concept was actually pitched in a future ideas worksheet a ways back.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, that's a good idea. I think it's worth reviving go-jet

lCursor, rCursor,
),
)
}
sql = sql.OrderBy("cb.last_modified_ledger asc, cb.id asc")
sql = sql.OrderBy(fmt.Sprintf("%s.last_modified_ledger asc, %s.id asc", tableName, tableName))
case db2.OrderDescending:
if hasPagedLimit {
sql = sql.
Where(sq.Expr("(cb.last_modified_ledger, cb.id) < (?, ?)", lCursor, rCursor))
Where(
sq.Expr(
fmt.Sprintf("(%s.last_modified_ledger, %s.id) < (?, ?)", tableName, tableName),
lCursor,
rCursor,
),
)
}

sql = sql.OrderBy("cb.last_modified_ledger desc, cb.id desc")
sql = sql.OrderBy(fmt.Sprintf("%s.last_modified_ledger desc, %s.id desc", tableName, tableName))
default:
return sql, errors.Errorf("invalid order: %s", order)
}
Expand Down Expand Up @@ -216,7 +226,7 @@
return nil, errors.Wrap(err, "error getting cursor")
}

sql, err := applyClaimableBalancesQueriesCursor(selectClaimableBalances, l, r, query.PageQuery.Order)
sql, err := applyClaimableBalancesQueriesCursor(selectClaimableBalances, "cb", l, r, query.PageQuery.Order)
if err != nil {
return nil, errors.Wrap(err, "could not apply query to page")
}
Expand All @@ -242,10 +252,10 @@
// does not perform efficiently. Instead, use a subquery (with LIMIT) to retrieve claimable balances based on
// the claimant's address.

var selectClaimableBalanceClaimants = sq.Select("id").From("claimable_balance_claimants").
Where("destination = ?", query.Claimant.Address()).Limit(query.PageQuery.Limit)
var selectClaimableBalanceClaimants = sq.Select("claimable_balance_claimants.id").From("claimable_balance_claimants").
Where("claimable_balance_claimants.destination = ?", query.Claimant.Address()).Limit(query.PageQuery.Limit)

subSql, err := applyClaimableBalancesQueriesCursor(selectClaimableBalanceClaimants, l, r, query.PageQuery.Order)
subSql, err := applyClaimableBalancesQueriesCursor(selectClaimableBalanceClaimants, "claimable_balance_claimants", l, r, query.PageQuery.Order)

Check failure on line 258 in services/horizon/internal/db2/history/claimable_balances.go

View workflow job for this annotation

GitHub Actions / golangci 

line is 145 characters (lll)
if err != nil {
return nil, errors.Wrap(err, "could not apply subquery to page")
}
Expand Down
5 changes: 3 additions & 2 deletions services/horizon/internal/db2/history/claimable_balances_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"testing"

"github.com/guregu/null"

"github.com/stellar/go/services/horizon/internal/db2"
"github.com/stellar/go/services/horizon/internal/test"
"github.com/stellar/go/xdr"
Expand Down Expand Up @@ -203,7 +204,7 @@ func TestFindClaimableBalancesByDestination(t *testing.T) {
tt.Assert.Equal(dest2, cbs[0].Claimants[1].Destination)

// this validates the cb query with claimant and cb.id/ledger cursor parameters
query.PageQuery = db2.MustPageQuery(fmt.Sprintf("%v-%s", 150, cbs[0].BalanceID), false, "", 10)
query.PageQuery = db2.MustPageQuery(fmt.Sprintf("%v-%s", 150, cbs[0].BalanceID), false, "asc", 10)
query.Claimant = xdr.MustAddressPtr(dest1)
cbs, err = q.GetClaimableBalances(tt.Ctx, query)
tt.Assert.NoError(err)
Expand All @@ -212,7 +213,7 @@ func TestFindClaimableBalancesByDestination(t *testing.T) {

// this validates the cb query with no claimant parameter,
// should still produce working sql, as it triggers different LIMIT position in sql.
query.PageQuery = db2.MustPageQuery("", false, "", 1)
query.PageQuery = db2.MustPageQuery("", false, "desc", 1)
query.Claimant = nil
cbs, err = q.GetClaimableBalances(tt.Ctx, query)
tt.Assert.NoError(err)
Expand Down
Loading