From ae641fd24db4cc2434304479d82430e50fcf6bbe Mon Sep 17 00:00:00 2001 From: bndw Date: Tue, 2 May 2023 07:42:27 -0700 Subject: [PATCH] refactor(postgres): make SQL generation testable Decouples the postgresql sql generation from the query execution. This allows the logic for building sql to be unit tested without access to a database. This work was motivated when a client was not receiving events as expected. In debugging I found that if a tag's value was an empty array, then no query would be executed - and to my surprised no error is raised either. I wanted to get a better sense of the current constraints on when queries are and are not executed, but I had a hard time keeping the code in my head. This led me to extracting the sql generation into its own function and writing the unit tests that document its current behavior. This refactor makes no changes to the current logic. I have added some REVIEW comments in the test cases where I thought some error handling could be introduced but I wanted to first see if you were receptive to this refactor before proposing any functional changes. --- go.mod | 5 +- go.sum | 6 +- storage/postgresql/query.go | 80 +++++++------ storage/postgresql/query_test.go | 190 +++++++++++++++++++++++++++++++ 4 files changed, 243 insertions(+), 38 deletions(-) create mode 100644 storage/postgresql/query_test.go diff --git a/go.mod b/go.mod index 7277259..90de5b2 100644 --- a/go.mod +++ b/go.mod @@ -18,7 +18,9 @@ require ( github.com/mmcdole/gofeed v1.1.3 github.com/nbd-wtf/go-nostr v0.17.1 github.com/rif/cache2go v1.0.0 + github.com/rs/cors v1.7.0 github.com/stevelacy/daz v0.1.4 + github.com/stretchr/testify v1.8.0 github.com/tidwall/gjson v1.14.1 golang.org/x/exp v0.0.0-20230224173230-c95f2b4c22f2 ) @@ -79,7 +81,7 @@ require ( github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd // indirect github.com/modern-go/reflect2 v1.0.1 // indirect github.com/pkg/errors v0.9.1 // indirect - github.com/rs/cors v1.7.0 // indirect + github.com/pmezard/go-difflib v1.0.0 // indirect github.com/tidwall/match v1.1.1 // indirect github.com/tidwall/pretty v1.2.0 // indirect golang.org/x/crypto v0.0.0-20210921155107-089bfa567519 // indirect @@ -87,4 +89,5 @@ require ( golang.org/x/sys v0.1.0 // indirect golang.org/x/term v0.0.0-20201126162022-7de9c90e9dd1 // indirect golang.org/x/text v0.3.7 // indirect + gopkg.in/yaml.v3 v3.0.1 // indirect ) diff --git a/go.sum b/go.sum index 86cbde5..7c343ad 100644 --- a/go.sum +++ b/go.sum @@ -394,7 +394,8 @@ github.com/spf13/viper v1.3.2/go.mod h1:ZiWeW+zYFKm7srdB9IoDzzZXaJaI5eL9QjNiN/DM github.com/stevelacy/daz v0.1.4 h1:ugmff/D7D764wZjXSgSryEINE/bi+Xddllw3JQQGbWk= github.com/stevelacy/daz v0.1.4/go.mod h1:AbK6DzjiIL15r4bQtcFvOBAvDGMXoh+uIG26NRUugt0= github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME= -github.com/stretchr/objx v0.2.0 h1:Hbg2NidpLE8veEBkEZTL3CvlkUIVzuU9jDplZO54c48= +github.com/stretchr/objx v0.4.0 h1:M2gUjqZET1qApGOWNSnZ49BAIMX4F/1plDv3+l31EJ4= +github.com/stretchr/objx v0.4.0/go.mod h1:YvHI0jy2hoMjB+UWwv71VJQ9isScKT/TqJzVSSt89Yw= github.com/stretchr/testify v1.2.2/go.mod h1:a8OnRcib4nhh0OaRAV+Yts87kKdq0PP7pXfy6kDkUVs= github.com/stretchr/testify v1.3.0/go.mod h1:M5WIy9Dh21IEIfnGCwXGc5bZfKNJtfHm1UVUgZn+9EI= github.com/stretchr/testify v1.4.0/go.mod h1:j7eGeouHqKxXV5pUuKE4zz7dFj8WfuZ+81PSLYec5m4= @@ -402,6 +403,7 @@ github.com/stretchr/testify v1.5.1/go.mod h1:5W2xD1RspED5o8YsWQXVCued0rvSQ+mT+I5 github.com/stretchr/testify v1.7.0/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg= github.com/stretchr/testify v1.7.1/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg= github.com/stretchr/testify v1.8.0 h1:pSgiaMZlXftHpm5L7V1+rVB+AZJydKsMxsQBIJw4PKk= +github.com/stretchr/testify v1.8.0/go.mod h1:yNjHg4UonilssWZ8iaSj1OCr/vHnekPRkoO+kdMU+MU= github.com/syndtr/goleveldb v1.0.1-0.20210819022825-2ae1ddf74ef7 h1:epCh84lMvA70Z7CTTCmYQn2CKbY8j86K7/FAIr141uY= github.com/syndtr/goleveldb v1.0.1-0.20210819022825-2ae1ddf74ef7/go.mod h1:q4W45IWZaF22tdD+VEXcAWRA037jwmWEB5VWYORlTpc= github.com/tidwall/gjson v1.14.1 h1:iymTbGkQBhveq21bEvAQ81I0LEBork8BFe1CUZXdyuo= @@ -609,6 +611,7 @@ gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8 gopkg.in/check.v1 v1.0.0-20180628173108-788fd7840127/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= gopkg.in/check.v1 v1.0.0-20190902080502-41f04d3bba15/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= gopkg.in/check.v1 v1.0.0-20200227125254-8fa46927fb4f/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= +gopkg.in/check.v1 v1.0.0-20201130134442-10cb98267c6c h1:Hei/4ADfdWqJk1ZMxUNpqntNwaWcugrBjAiHlqqRiVk= gopkg.in/fsnotify.v1 v1.4.7/go.mod h1:Tz8NjZHkW78fSQdbUxIjBTcgA1z1m8ZHf0WmKUhAMys= gopkg.in/go-playground/assert.v1 v1.2.1/go.mod h1:9RXL0bg/zibRAgZUYszZSwO/z8Y/a8bDuhia5mkpMnE= gopkg.in/go-playground/validator.v8 v8.18.2/go.mod h1:RX2a/7Ha8BgOhfk7j780h4/u/RRjR0eouCJSH80/M2Y= @@ -622,6 +625,7 @@ gopkg.in/yaml.v2 v2.3.0/go.mod h1:hI93XBmqTisBFMUTm0b8Fm+jr3Dg1NNxqwp+5A1VGuI= gopkg.in/yaml.v2 v2.4.0 h1:D8xgwECY7CYvx+Y2n4sBz93Jn9JRvxdiyyo8CTfuKaY= gopkg.in/yaml.v3 v3.0.0-20200313102051-9f266ea9e77c/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA= +gopkg.in/yaml.v3 v3.0.1/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= honnef.co/go/tools v0.0.0-20180728063816-88497007e858/go.mod h1:rf3lG4BRIbNafJWhAfAdb/ePZxsR/4RtNHQocxwk9r4= honnef.co/go/tools v0.0.0-20190102054323-c2f93a96b099/go.mod h1:rf3lG4BRIbNafJWhAfAdb/ePZxsR/4RtNHQocxwk9r4= honnef.co/go/tools v0.0.0-20190523083050-ea95bdfd59fc/go.mod h1:rf3lG4BRIbNafJWhAfAdb/ePZxsR/4RtNHQocxwk9r4= diff --git a/storage/postgresql/query.go b/storage/postgresql/query.go index 54226e9..ff3e508 100644 --- a/storage/postgresql/query.go +++ b/storage/postgresql/query.go @@ -4,29 +4,58 @@ import ( "context" "database/sql" "encoding/hex" - "errors" "fmt" "strconv" "strings" + "github.com/jmoiron/sqlx" "github.com/nbd-wtf/go-nostr" ) func (b PostgresBackend) QueryEvents(ctx context.Context, filter *nostr.Filter) (ch chan *nostr.Event, err error) { ch = make(chan *nostr.Event) + query, params, err := queryEventsSql(filter) + if err != nil { + return nil, err + } + + rows, err := b.DB.Query(query, params...) + if err != nil && err != sql.ErrNoRows { + return nil, fmt.Errorf("failed to fetch events using query %q: %w", query, err) + } + + go func() { + defer rows.Close() + defer close(ch) + for rows.Next() { + var evt nostr.Event + var timestamp int64 + err := rows.Scan(&evt.ID, &evt.PubKey, ×tamp, + &evt.Kind, &evt.Tags, &evt.Content, &evt.Sig) + if err != nil { + return + } + evt.CreatedAt = nostr.Timestamp(timestamp) + ch <- &evt + } + }() + + return ch, nil +} + +func queryEventsSql(filter *nostr.Filter) (string, []any, error) { var conditions []string var params []any if filter == nil { - err = errors.New("filter cannot be null") - return + return "", nil, fmt.Errorf("filter cannot be null") } if filter.IDs != nil { if len(filter.IDs) > 500 { // too many ids, fail everything - return + return "", nil, nil } likeids := make([]string, 0, len(filter.IDs)) @@ -41,7 +70,7 @@ func (b PostgresBackend) QueryEvents(ctx context.Context, filter *nostr.Filter) } if len(likeids) == 0 { // ids being [] mean you won't get anything - return + return "", nil, nil } conditions = append(conditions, "("+strings.Join(likeids, " OR ")+")") } @@ -49,7 +78,7 @@ func (b PostgresBackend) QueryEvents(ctx context.Context, filter *nostr.Filter) if filter.Authors != nil { if len(filter.Authors) > 500 { // too many authors, fail everything - return + return "", nil, nil } likekeys := make([]string, 0, len(filter.Authors)) @@ -64,7 +93,7 @@ func (b PostgresBackend) QueryEvents(ctx context.Context, filter *nostr.Filter) } if len(likekeys) == 0 { // authors being [] mean you won't get anything - return + return "", nil, nil } conditions = append(conditions, "("+strings.Join(likekeys, " OR ")+")") } @@ -72,12 +101,12 @@ func (b PostgresBackend) QueryEvents(ctx context.Context, filter *nostr.Filter) if filter.Kinds != nil { if len(filter.Kinds) > 10 { // too many kinds, fail everything - return + return "", nil, nil } if len(filter.Kinds) == 0 { // kinds being [] mean you won't get anything - return + return "", nil, nil } // no sql injection issues since these are ints inkinds := make([]string, len(filter.Kinds)) @@ -91,7 +120,7 @@ func (b PostgresBackend) QueryEvents(ctx context.Context, filter *nostr.Filter) for _, values := range filter.Tags { if len(values) == 0 { // any tag set to [] is wrong - return + return "", nil, nil } // add these tags to the query @@ -99,7 +128,7 @@ func (b PostgresBackend) QueryEvents(ctx context.Context, filter *nostr.Filter) if len(tagQuery) > 10 { // too many tags, fail everything - return + return "", nil, nil } } @@ -136,32 +165,11 @@ func (b PostgresBackend) QueryEvents(ctx context.Context, filter *nostr.Filter) params = append(params, filter.Limit) } - query := b.DB.Rebind(`SELECT + query := sqlx.Rebind(sqlx.BindType("postgres"), `SELECT id, pubkey, created_at, kind, tags, content, sig - FROM event WHERE ` + - strings.Join(conditions, " AND ") + + FROM event WHERE `+ + strings.Join(conditions, " AND ")+ " ORDER BY created_at DESC LIMIT ?") - rows, err := b.DB.Query(query, params...) - if err != nil && err != sql.ErrNoRows { - return nil, fmt.Errorf("failed to fetch events using query %q: %w", query, err) - } - - go func() { - defer rows.Close() - defer close(ch) - for rows.Next() { - var evt nostr.Event - var timestamp int64 - err := rows.Scan(&evt.ID, &evt.PubKey, ×tamp, - &evt.Kind, &evt.Tags, &evt.Content, &evt.Sig) - if err != nil { - return - } - evt.CreatedAt = nostr.Timestamp(timestamp) - ch <- &evt - } - }() - - return ch, nil + return query, params, nil } diff --git a/storage/postgresql/query_test.go b/storage/postgresql/query_test.go new file mode 100644 index 0000000..310374a --- /dev/null +++ b/storage/postgresql/query_test.go @@ -0,0 +1,190 @@ +package postgresql + +import ( + "fmt" + "strconv" + "strings" + "testing" + + "github.com/nbd-wtf/go-nostr" + "github.com/stretchr/testify/assert" +) + +func TestQueryEventsSql(t *testing.T) { + var tests = []struct { + name string + filter *nostr.Filter + query string + params []any + err error + }{ + { + name: "empty filter", + filter: &nostr.Filter{}, + query: "SELECT id, pubkey, created_at, kind, tags, content, sig FROM event WHERE true ORDER BY created_at DESC LIMIT $1", + params: []any{100}, + err: nil, + }, + { + name: "ids filter", + filter: &nostr.Filter{ + IDs: []string{"083ec57f36a7b39ab98a57bedab4f85355b2ee89e4b205bed58d7c3ef9edd294"}, + }, + query: `SELECT id, pubkey, created_at, kind, tags, content, sig + FROM event + WHERE (id LIKE '083ec57f36a7b39ab98a57bedab4f85355b2ee89e4b205bed58d7c3ef9edd294%') + ORDER BY created_at DESC LIMIT $1`, + params: []any{100}, + err: nil, + }, + { + name: "kind filter", + filter: &nostr.Filter{ + Kinds: []int{1, 2, 3}, + }, + query: `SELECT id, pubkey, created_at, kind, tags, content, sig + FROM event + WHERE kind IN(1,2,3) + ORDER BY created_at DESC LIMIT $1`, + params: []any{100}, + err: nil, + }, + { + name: "authors filter", + filter: &nostr.Filter{ + Authors: []string{"7bdef7bdebb8721f77927d0e77c66059360fa62371fdf15f3add93923a613229"}, + }, + query: `SELECT id, pubkey, created_at, kind, tags, content, sig + FROM event + WHERE (pubkey LIKE '7bdef7bdebb8721f77927d0e77c66059360fa62371fdf15f3add93923a613229%') + ORDER BY created_at DESC LIMIT $1`, + params: []any{100}, + err: nil, + }, + // errors + { + name: "nil filter", + filter: nil, + query: "", + params: nil, + err: fmt.Errorf("filter cannot be null"), + }, + { + name: "too many ids", + filter: &nostr.Filter{ + IDs: strSlice(501), + }, + query: "", + params: nil, + // REVIEW: should return error + err: nil, + }, + { + name: "invalid ids", + filter: &nostr.Filter{ + IDs: []string{"stuff"}, + }, + query: "", + params: nil, + // REVIEW: should return error + err: nil, + }, + { + name: "too many authors", + filter: &nostr.Filter{ + Authors: strSlice(501), + }, + query: "", + params: nil, + // REVIEW: should return error + err: nil, + }, + { + name: "invalid authors", + filter: &nostr.Filter{ + Authors: []string{"stuff"}, + }, + query: "", + params: nil, + // REVIEW: should return error + err: nil, + }, + { + name: "too many kinds", + filter: &nostr.Filter{ + Kinds: intSlice(11), + }, + query: "", + params: nil, + // REVIEW: should return error + err: nil, + }, + { + name: "no kinds", + filter: &nostr.Filter{ + Kinds: []int{}, + }, + query: "", + params: nil, + // REVIEW: should return error + err: nil, + }, + { + name: "tags of empty array", + filter: &nostr.Filter{ + Tags: nostr.TagMap{ + "#e": []string{}, + }, + }, + query: "", + params: nil, + // REVIEW: should return error + err: nil, + }, + { + name: "too many tag values", + filter: &nostr.Filter{ + Tags: nostr.TagMap{ + "#e": strSlice(11), + }, + }, + query: "", + params: nil, + // REVIEW: should return error + err: nil, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + query, params, err := queryEventsSql(tt.filter) + assert.Equal(t, tt.err, err) + if err != nil { + return + } + + assert.Equal(t, clean(tt.query), clean(query)) + assert.Equal(t, tt.params, params) + }) + } +} + +func clean(s string) string { + return strings.ReplaceAll(strings.ReplaceAll(strings.ReplaceAll(s, "\t", ""), "\n", ""), " ", "") +} + +func intSlice(n int) []int { + slice := make([]int, 0, n) + for i := 0; i < n; i++ { + slice = append(slice, i) + } + return slice +} + +func strSlice(n int) []string { + slice := make([]string, 0, n) + for i := 0; i < n; i++ { + slice = append(slice, strconv.Itoa(i)) + } + return slice +}