From 8d871d6cce8e8f7d5a783c629152c106d59c8d9b Mon Sep 17 00:00:00 2001 From: Matias <83959431+mativm02@users.noreply.github.com> Date: Thu, 27 Jul 2023 09:52:12 -0300 Subject: [PATCH 01/13] new encoding url function --- .../internal/driver/mongo/life_cycle.go | 110 +++++++++++++++- .../internal/driver/mongo/life_cycle_test.go | 118 +++++++++++++++++- 2 files changed, 219 insertions(+), 9 deletions(-) diff --git a/persistent/internal/driver/mongo/life_cycle.go b/persistent/internal/driver/mongo/life_cycle.go index c10eda3b..cb0c2c94 100644 --- a/persistent/internal/driver/mongo/life_cycle.go +++ b/persistent/internal/driver/mongo/life_cycle.go @@ -3,6 +3,9 @@ package mongo import ( "context" "errors" + "fmt" + "net/url" + "strings" "time" "github.com/TykTechnologies/storage/persistent/internal/helper" @@ -12,7 +15,6 @@ import ( "go.mongodb.org/mongo-driver/mongo" "go.mongodb.org/mongo-driver/mongo/options" "go.mongodb.org/mongo-driver/mongo/readpref" - "go.mongodb.org/mongo-driver/x/mongo/driver/connstring" "github.com/TykTechnologies/storage/persistent/internal/types" ) @@ -31,12 +33,13 @@ func (lc *lifeCycle) Connect(opts *types.ClientOpts) error { var err error var client *mongo.Client - // we check if the connection string is valid before building the connOpts. - cs, err := connstring.ParseAndValidate(opts.ConnectionString) + url, cs, err := parseURL(opts.ConnectionString) if err != nil { - return errors.New("invalid connection string") + return err } + opts.ConnectionString = url + connOpts, err := mongoOptsBuilder(opts) if err != nil { return errors.New(err.Error()) @@ -50,12 +53,109 @@ func (lc *lifeCycle) Connect(opts *types.ClientOpts) error { } lc.connectionString = opts.ConnectionString - lc.database = cs.Database + lc.database = cs.db lc.client = client return lc.client.Ping(context.Background(), nil) } +type urlInfo struct { + addrs []string + user string + pass string + db string + options map[string]string +} + +func isOptSep(c rune) bool { + return c == ';' || c == '&' +} + +func parseURL(s string) (string, *urlInfo, error) { + var info *urlInfo + prefix := "" + if strings.HasPrefix(s, "mongodb://") { + prefix = "mongodb://" + s = strings.TrimPrefix(s, "mongodb://") + } else if strings.HasPrefix(s, "mongodb+srv://") { + prefix = "mongodb+srv://" + s = strings.TrimPrefix(s, "mongodb+srv://") + } else { + return "", info, errors.New("invalid connection string, no prefix found") + } + + info, err := extractURL(s) + if err != nil { + return "", info, err + } + + var connString string + connString += prefix + + if info.user != "" { + info.user = url.QueryEscape(info.user) + connString += info.user + if info.pass != "" { + info.pass = url.QueryEscape(info.pass) + connString += ":" + info.pass + } + connString += "@" + } + + connString += strings.Join(info.addrs, ",") + if info.db != "" { + connString += "/" + info.db + } + + if len(info.options) > 0 { + connString += "?" + for k, v := range info.options { + connString += k + "=" + v + "&" + } + connString = connString[:len(connString)-1] + } + + return connString, info, nil +} + +func extractURL(s string) (*urlInfo, error) { + info := &urlInfo{options: make(map[string]string)} + if c := strings.Index(s, "?"); c != -1 { + for _, pair := range strings.FieldsFunc(s[c+1:], isOptSep) { + l := strings.SplitN(pair, "=", 2) + if len(l) != 2 || l[0] == "" || l[1] == "" { + return nil, errors.New("connection option must be key=value: " + pair) + } + info.options[l[0]] = l[1] + } + s = s[:c] + } + if c := strings.Index(s, "@"); c != -1 { + pair := strings.SplitN(s[:c], ":", 2) + if len(pair) > 2 || pair[0] == "" { + return nil, errors.New("credentials must be provided as user:pass@host") + } + var err error + info.user, err = url.QueryUnescape(pair[0]) + if err != nil { + return nil, fmt.Errorf("cannot unescape username in URL: %q", pair[0]) + } + if len(pair) > 1 { + info.pass, err = url.QueryUnescape(pair[1]) + if err != nil { + return nil, fmt.Errorf("cannot unescape password in URL") + } + } + s = s[c+1:] + } + if c := strings.Index(s, "/"); c != -1 { + info.db = s[c+1:] + s = s[:c] + } + info.addrs = strings.Split(s, ",") + return info, nil +} + // Close finish the session. func (lc *lifeCycle) Close() error { if lc.client != nil { diff --git a/persistent/internal/driver/mongo/life_cycle_test.go b/persistent/internal/driver/mongo/life_cycle_test.go index 465713c1..349d3c06 100644 --- a/persistent/internal/driver/mongo/life_cycle_test.go +++ b/persistent/internal/driver/mongo/life_cycle_test.go @@ -1,6 +1,3 @@ -//go:build mongo -// +build mongo - package mongo import ( @@ -160,7 +157,7 @@ func TestConnect(t *testing.T) { UseSSL: false, Type: "mongodb", }, - want: errors.New("invalid connection string"), + want: errors.New("invalid connection string, no prefix found"), }, { name: "valid connection_string and invalid tls config", @@ -185,6 +182,119 @@ func TestConnect(t *testing.T) { } } +func TestParseURL(t *testing.T) { + tests := []struct { + name string + url string + want string + wantErr bool + }{ + { + name: "valid connection_string with special characters", + url: "mongodb://lt_tyk:6}3cZQU.9KvM/hVR4qkm-hHqZTu3yg=G@localhost:27017/tyk_analytics", + want: "mongodb://lt_tyk:6%7D3cZQU.9KvM%2FhVR4qkm-hHqZTu3yg%3DG@localhost:27017/tyk_analytics", + }, + { + name: "already encoded valid url", + url: "mongodb://lt_tyk:6%7D3cZQU.9KvM%2FhVR4qkm-hHqZTu3yg%3DG@localhost:27017/tyk_analytics", + want: "mongodb://lt_tyk:6%7D3cZQU.9KvM%2FhVR4qkm-hHqZTu3yg%3DG@localhost:27017/tyk_analytics", + }, + { + name: "invalid connection_string", + url: "invalid_conn_string", + want: "", + wantErr: true, + }, + { + name: "valid connection string with @", + url: "mongodb://user:p@ssword@localhost:27017", + want: "mongodb://user:p%40ssword@localhost:27017", + }, + { + name: "valid connection string with @ and /", + url: "mongodb://u=s@r:p@sswor/d@localhost:27017/test", + want: "mongodb://u%3Ds%40r:p%40sswor%2Fd@localhost:27017/test", + }, + { + name: "valid connection string with @ and / and '?' outside of the credentials part", + url: "mongodb://user:p@sswor/d@localhost:27017/test?authSource=admin", + want: "mongodb://user:p%40sswor%2Fd@localhost:27017/test?authSource=admin", + }, + { + name: "special characters and multiple hosts", + url: "mongodb://user:p@sswor/d@localhost:27017,localhost:27018/test?authSource=admin", + want: "mongodb://user:p%40sswor%2Fd@localhost:27017,localhost:27018/test?authSource=admin", + }, + { + name: "url without credentials", + url: "mongodb://localhost:27017/test?authSource=admin", + want: "mongodb://localhost:27017/test?authSource=admin", + }, + { + name: "invalid connection string", + url: "test", + want: "", + wantErr: true, + }, + { + name: "connection string full of special characters", + url: "mongodb://user:p@ss:/?#[]wor/d@localhost:27017,localhost:27018", + want: "mongodb://user:p%40ss%3A%2F%3F%23%5B%5Dwor%2Fd@localhost:27017,localhost:27018", + }, + { + name: "srv connection string", + url: "mongodb+srv://tyk:tyk@clur0.zlgl.mongodb.net/tyk?w=majority", + want: "mongodb+srv://tyk:tyk@clur0.zlgl.mongodb.net/tyk?w=majority", + }, + { + name: "srv connection string with special characters", + url: "mongodb+srv://tyk:p@ssword@clur0.zlgl.mongodb.net/tyk?w=majority", + want: "mongodb+srv://tyk:p%40ssword@clur0.zlgl.mongodb.net/tyk?w=majority", + }, + { + name: "connection string without username", + url: "mongodb://:password@localhost:27017/test", + want: "mongodb://:password@localhost:27017/test", + }, + { + name: "connection string without password", + url: "mongodb://user:@localhost:27017/test", + want: "mongodb://user:@localhost:27017/test", + }, + { + name: "connection string without host", + url: "mongodb://user:password@/test", + want: "mongodb://user:password@/test", + }, + { + name: "connection string without database", + url: "mongodb://user:password@localhost:27017", + want: "mongodb://user:password@localhost:27017", + }, + { + name: "cosmosdb url", + url: "mongodb://4-0-qa:zFAQ==@4-0-qa.azure:10/a1?maxIdleTimeMS=120000&appName=@4-testing@", + want: "mongodb://4-0-qa:zFAQ%3D%3D@4-0-qa.azure:10/a1?maxIdleTimeMS=120000&appName=@4-testing@", + }, + { + name: "already encoded cosmosdb url", + url: "mongodb://4-0-qa:zFAQ%3D%3D@4-0-qa.azure:10/a1?maxIdleTimeMS=120000&appName=@4-testing@", + want: "mongodb://4-0-qa:zFAQ%3D%3D@4-0-qa.azure:10/a1?maxIdleTimeMS=120000&appName=@4-testing@", + }, + } + + for _, test := range tests { + parsedURL, _, err := parseURL(test.url) + assert.Equal(t, test.want, parsedURL) + assert.Equal(t, test.wantErr, err != nil) + if err != nil { + return + } + + } + +} + func TestClose(t *testing.T) { lc := &lifeCycle{} opts := &types.ClientOpts{ From 4ebb37e40cdb467d686a5a684c617cbac92c70fd Mon Sep 17 00:00:00 2001 From: Matias <83959431+mativm02@users.noreply.github.com> Date: Thu, 27 Jul 2023 09:54:24 -0300 Subject: [PATCH 02/13] restoring build flags --- persistent/internal/driver/mongo/life_cycle_test.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/persistent/internal/driver/mongo/life_cycle_test.go b/persistent/internal/driver/mongo/life_cycle_test.go index 349d3c06..7e087601 100644 --- a/persistent/internal/driver/mongo/life_cycle_test.go +++ b/persistent/internal/driver/mongo/life_cycle_test.go @@ -1,3 +1,6 @@ +//go:build mongo +// +build mongo + package mongo import ( From ce13d1754f89a09e104297d04a438a5402642391 Mon Sep 17 00:00:00 2001 From: Matias <83959431+mativm02@users.noreply.github.com> Date: Thu, 27 Jul 2023 09:59:27 -0300 Subject: [PATCH 03/13] updating test error --- persistent/internal/driver/mongo/mongo_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/persistent/internal/driver/mongo/mongo_test.go b/persistent/internal/driver/mongo/mongo_test.go index 1c448a90..15525b5a 100644 --- a/persistent/internal/driver/mongo/mongo_test.go +++ b/persistent/internal/driver/mongo/mongo_test.go @@ -98,7 +98,7 @@ func TestNewMongoDriver(t *testing.T) { }) assert.NotNil(t, err) - assert.Equal(t, "invalid connection string", err.Error()) + assert.Equal(t, "invalid connection string, not prefix found", err.Error()) assert.Nil(t, newDriver) }) t.Run("new driver without connection string", func(t *testing.T) { From 45d784aded54e7c66ee292e12a838f2d07d31ece Mon Sep 17 00:00:00 2001 From: Matias <83959431+mativm02@users.noreply.github.com> Date: Thu, 27 Jul 2023 10:02:32 -0300 Subject: [PATCH 04/13] updating test error --- persistent/internal/driver/mongo/mongo_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/persistent/internal/driver/mongo/mongo_test.go b/persistent/internal/driver/mongo/mongo_test.go index 15525b5a..ba473e79 100644 --- a/persistent/internal/driver/mongo/mongo_test.go +++ b/persistent/internal/driver/mongo/mongo_test.go @@ -98,7 +98,7 @@ func TestNewMongoDriver(t *testing.T) { }) assert.NotNil(t, err) - assert.Equal(t, "invalid connection string, not prefix found", err.Error()) + assert.Equal(t, "invalid connection string, no prefix found", err.Error()) assert.Nil(t, newDriver) }) t.Run("new driver without connection string", func(t *testing.T) { From 071c4de5641598d0e0735a4fa5004695fd069c23 Mon Sep 17 00:00:00 2001 From: Matias <83959431+mativm02@users.noreply.github.com> Date: Wed, 2 Aug 2023 09:54:27 -0300 Subject: [PATCH 05/13] linting + improving tests --- .../internal/driver/mongo/life_cycle.go | 25 ++++++- .../internal/driver/mongo/life_cycle_test.go | 68 ++++++++++++------- 2 files changed, 66 insertions(+), 27 deletions(-) diff --git a/persistent/internal/driver/mongo/life_cycle.go b/persistent/internal/driver/mongo/life_cycle.go index cb0c2c94..25c5f316 100644 --- a/persistent/internal/driver/mongo/life_cycle.go +++ b/persistent/internal/driver/mongo/life_cycle.go @@ -74,13 +74,19 @@ func isOptSep(c rune) bool { func parseURL(s string) (string, *urlInfo, error) { var info *urlInfo prefix := "" + if strings.HasPrefix(s, "mongodb://") { prefix = "mongodb://" - s = strings.TrimPrefix(s, "mongodb://") } else if strings.HasPrefix(s, "mongodb+srv://") { prefix = "mongodb+srv://" + } + + switch prefix { + case "mongodb://": + s = strings.TrimPrefix(s, "mongodb://") + case "mongodb+srv://": s = strings.TrimPrefix(s, "mongodb+srv://") - } else { + default: return "", info, errors.New("invalid connection string, no prefix found") } @@ -95,14 +101,17 @@ func parseURL(s string) (string, *urlInfo, error) { if info.user != "" { info.user = url.QueryEscape(info.user) connString += info.user + if info.pass != "" { info.pass = url.QueryEscape(info.pass) connString += ":" + info.pass } + connString += "@" } connString += strings.Join(info.addrs, ",") + if info.db != "" { connString += "/" + info.db } @@ -112,6 +121,7 @@ func parseURL(s string) (string, *urlInfo, error) { for k, v := range info.options { connString += k + "=" + v + "&" } + connString = connString[:len(connString)-1] } @@ -120,39 +130,50 @@ func parseURL(s string) (string, *urlInfo, error) { func extractURL(s string) (*urlInfo, error) { info := &urlInfo{options: make(map[string]string)} + if c := strings.Index(s, "?"); c != -1 { for _, pair := range strings.FieldsFunc(s[c+1:], isOptSep) { l := strings.SplitN(pair, "=", 2) if len(l) != 2 || l[0] == "" || l[1] == "" { return nil, errors.New("connection option must be key=value: " + pair) } + info.options[l[0]] = l[1] } + s = s[:c] } + if c := strings.Index(s, "@"); c != -1 { pair := strings.SplitN(s[:c], ":", 2) if len(pair) > 2 || pair[0] == "" { return nil, errors.New("credentials must be provided as user:pass@host") } + var err error + info.user, err = url.QueryUnescape(pair[0]) if err != nil { return nil, fmt.Errorf("cannot unescape username in URL: %q", pair[0]) } + if len(pair) > 1 { info.pass, err = url.QueryUnescape(pair[1]) if err != nil { return nil, fmt.Errorf("cannot unescape password in URL") } } + s = s[c+1:] } + if c := strings.Index(s, "/"); c != -1 { info.db = s[c+1:] s = s[:c] } + info.addrs = strings.Split(s, ",") + return info, nil } diff --git a/persistent/internal/driver/mongo/life_cycle_test.go b/persistent/internal/driver/mongo/life_cycle_test.go index 7e087601..737d5fc3 100644 --- a/persistent/internal/driver/mongo/life_cycle_test.go +++ b/persistent/internal/driver/mongo/life_cycle_test.go @@ -211,22 +211,22 @@ func TestParseURL(t *testing.T) { { name: "valid connection string with @", url: "mongodb://user:p@ssword@localhost:27017", - want: "mongodb://user:p%40ssword@localhost:27017", + want: "mongodb://user:p@ssword@localhost:27017", }, { name: "valid connection string with @ and /", url: "mongodb://u=s@r:p@sswor/d@localhost:27017/test", - want: "mongodb://u%3Ds%40r:p%40sswor%2Fd@localhost:27017/test", + want: "mongodb://u%3Ds@r:p@sswor/d@localhost:27017/test", }, { name: "valid connection string with @ and / and '?' outside of the credentials part", url: "mongodb://user:p@sswor/d@localhost:27017/test?authSource=admin", - want: "mongodb://user:p%40sswor%2Fd@localhost:27017/test?authSource=admin", + want: "mongodb://user:p@sswor/d@localhost:27017/test?authSource=admin", }, { name: "special characters and multiple hosts", url: "mongodb://user:p@sswor/d@localhost:27017,localhost:27018/test?authSource=admin", - want: "mongodb://user:p%40sswor%2Fd@localhost:27017,localhost:27018/test?authSource=admin", + want: "mongodb://user:p@sswor/d@localhost:27017,localhost:27018/test?authSource=admin", }, { name: "url without credentials", @@ -239,11 +239,6 @@ func TestParseURL(t *testing.T) { want: "", wantErr: true, }, - { - name: "connection string full of special characters", - url: "mongodb://user:p@ss:/?#[]wor/d@localhost:27017,localhost:27018", - want: "mongodb://user:p%40ss%3A%2F%3F%23%5B%5Dwor%2Fd@localhost:27017,localhost:27018", - }, { name: "srv connection string", url: "mongodb+srv://tyk:tyk@clur0.zlgl.mongodb.net/tyk?w=majority", @@ -252,17 +247,18 @@ func TestParseURL(t *testing.T) { { name: "srv connection string with special characters", url: "mongodb+srv://tyk:p@ssword@clur0.zlgl.mongodb.net/tyk?w=majority", - want: "mongodb+srv://tyk:p%40ssword@clur0.zlgl.mongodb.net/tyk?w=majority", + want: "mongodb+srv://tyk:p@ssword@clur0.zlgl.mongodb.net/tyk?w=majority", }, { - name: "connection string without username", - url: "mongodb://:password@localhost:27017/test", - want: "mongodb://:password@localhost:27017/test", + name: "connection string without username", + url: "mongodb://:password@localhost:27017/test", + want: "", + wantErr: true, }, { name: "connection string without password", url: "mongodb://user:@localhost:27017/test", - want: "mongodb://user:@localhost:27017/test", + want: "mongodb://user@localhost:27017/test", }, { name: "connection string without host", @@ -276,24 +272,22 @@ func TestParseURL(t *testing.T) { }, { name: "cosmosdb url", - url: "mongodb://4-0-qa:zFAQ==@4-0-qa.azure:10/a1?maxIdleTimeMS=120000&appName=@4-testing@", - want: "mongodb://4-0-qa:zFAQ%3D%3D@4-0-qa.azure:10/a1?maxIdleTimeMS=120000&appName=@4-testing@", + url: "mongodb+srv://4-0-qa:zFAQ==@4-0-qa.azure:10/a1?maxIdleTimeMS=120000&appName=@4-testing@", + want: "mongodb+srv://4-0-qa:zFAQ%3D%3D@4-0-qa.azure:10/a1?maxIdleTimeMS=120000&appName=@4-testing@", }, { name: "already encoded cosmosdb url", - url: "mongodb://4-0-qa:zFAQ%3D%3D@4-0-qa.azure:10/a1?maxIdleTimeMS=120000&appName=@4-testing@", - want: "mongodb://4-0-qa:zFAQ%3D%3D@4-0-qa.azure:10/a1?maxIdleTimeMS=120000&appName=@4-testing@", + url: "mongodb+srv://4-0-qa:zFAQ%3D%3D@4-0-qa.azure:10/a1?maxIdleTimeMS=120000&appName=@4-testing@", + want: "mongodb+srv://4-0-qa:zFAQ%3D%3D@4-0-qa.azure:10/a1?maxIdleTimeMS=120000&appName=@4-testing@", }, } for _, test := range tests { - parsedURL, _, err := parseURL(test.url) - assert.Equal(t, test.want, parsedURL) - assert.Equal(t, test.wantErr, err != nil) - if err != nil { - return - } - + t.Run(test.name, func(t *testing.T) { + parsedURL, _, err := parseURL(test.url) + assert.Equal(t, test.want, parsedURL) + assert.Equal(t, test.wantErr, err != nil) + }) } } @@ -329,3 +323,27 @@ func TestDBType(t *testing.T) { dbType := lc.DBType() assert.Equal(t, utils.StandardMongo, dbType) } + +func TestIsOptSep(t *testing.T) { + tests := []struct { + input rune + want bool + }{ + {';', true}, + {'&', true}, + {':', false}, + {'a', false}, + {'1', false}, + {' ', false}, + {'\t', false}, + {'\n', false}, + {'!', false}, + } + + for _, test := range tests { + got := isOptSep(test.input) + if got != test.want { + t.Errorf("isOptSep(%q) = %v, want %v", test.input, got, test.want) + } + } +} From cfd6ccc8663fe4c2bea267c59cc7097b872d4306 Mon Sep 17 00:00:00 2001 From: Matias <83959431+mativm02@users.noreply.github.com> Date: Wed, 2 Aug 2023 10:18:45 -0300 Subject: [PATCH 06/13] fixing test --- persistent/internal/driver/mongo/life_cycle_test.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/persistent/internal/driver/mongo/life_cycle_test.go b/persistent/internal/driver/mongo/life_cycle_test.go index 737d5fc3..83999c40 100644 --- a/persistent/internal/driver/mongo/life_cycle_test.go +++ b/persistent/internal/driver/mongo/life_cycle_test.go @@ -272,13 +272,13 @@ func TestParseURL(t *testing.T) { }, { name: "cosmosdb url", - url: "mongodb+srv://4-0-qa:zFAQ==@4-0-qa.azure:10/a1?maxIdleTimeMS=120000&appName=@4-testing@", - want: "mongodb+srv://4-0-qa:zFAQ%3D%3D@4-0-qa.azure:10/a1?maxIdleTimeMS=120000&appName=@4-testing@", + url: "mongodb+srv://4-0-qa:zFAQ==@4-0-qa.azure:10/a1?appName=@4-testing@&maxIdleTimeMS=120000", + want: "mongodb+srv://4-0-qa:zFAQ%3D%3D@4-0-qa.azure:10/a1?appName=@4-testing@&maxIdleTimeMS=120000", }, { name: "already encoded cosmosdb url", - url: "mongodb+srv://4-0-qa:zFAQ%3D%3D@4-0-qa.azure:10/a1?maxIdleTimeMS=120000&appName=@4-testing@", - want: "mongodb+srv://4-0-qa:zFAQ%3D%3D@4-0-qa.azure:10/a1?maxIdleTimeMS=120000&appName=@4-testing@", + url: "mongodb+srv://4-0-qa:zFAQ%3D%3D@4-0-qa.azure:10/a1?appName=@4-testing@&maxIdleTimeMS=120000", + want: "mongodb+srv://4-0-qa:zFAQ%3D%3D@4-0-qa.azure:10/a1?appName=@4-testing@&maxIdleTimeMS=120000", }, } From 66efcb7a0834f9f386c01772930143ddc7b7cd02 Mon Sep 17 00:00:00 2001 From: Matias <83959431+mativm02@users.noreply.github.com> Date: Wed, 2 Aug 2023 10:23:04 -0300 Subject: [PATCH 07/13] removing flaky test --- persistent/internal/driver/mongo/life_cycle_test.go | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/persistent/internal/driver/mongo/life_cycle_test.go b/persistent/internal/driver/mongo/life_cycle_test.go index 83999c40..9c73a026 100644 --- a/persistent/internal/driver/mongo/life_cycle_test.go +++ b/persistent/internal/driver/mongo/life_cycle_test.go @@ -274,12 +274,7 @@ func TestParseURL(t *testing.T) { name: "cosmosdb url", url: "mongodb+srv://4-0-qa:zFAQ==@4-0-qa.azure:10/a1?appName=@4-testing@&maxIdleTimeMS=120000", want: "mongodb+srv://4-0-qa:zFAQ%3D%3D@4-0-qa.azure:10/a1?appName=@4-testing@&maxIdleTimeMS=120000", - }, - { - name: "already encoded cosmosdb url", - url: "mongodb+srv://4-0-qa:zFAQ%3D%3D@4-0-qa.azure:10/a1?appName=@4-testing@&maxIdleTimeMS=120000", - want: "mongodb+srv://4-0-qa:zFAQ%3D%3D@4-0-qa.azure:10/a1?appName=@4-testing@&maxIdleTimeMS=120000", - }, + } } for _, test := range tests { From f60dda844ab995698197f6e0795dc5e056cb4489 Mon Sep 17 00:00:00 2001 From: Matias <83959431+mativm02@users.noreply.github.com> Date: Wed, 2 Aug 2023 11:10:07 -0300 Subject: [PATCH 08/13] adding missing comma --- persistent/internal/driver/mongo/life_cycle_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/persistent/internal/driver/mongo/life_cycle_test.go b/persistent/internal/driver/mongo/life_cycle_test.go index 9c73a026..5f8ceba0 100644 --- a/persistent/internal/driver/mongo/life_cycle_test.go +++ b/persistent/internal/driver/mongo/life_cycle_test.go @@ -274,7 +274,7 @@ func TestParseURL(t *testing.T) { name: "cosmosdb url", url: "mongodb+srv://4-0-qa:zFAQ==@4-0-qa.azure:10/a1?appName=@4-testing@&maxIdleTimeMS=120000", want: "mongodb+srv://4-0-qa:zFAQ%3D%3D@4-0-qa.azure:10/a1?appName=@4-testing@&maxIdleTimeMS=120000", - } + }, } for _, test := range tests { From 94cb84afc721d788d847551a3eda9101155fbc8c Mon Sep 17 00:00:00 2001 From: Matias <83959431+mativm02@users.noreply.github.com> Date: Thu, 3 Aug 2023 13:51:54 -0300 Subject: [PATCH 09/13] creating urlOptions struct to avoid flaky tests with maps --- .../internal/driver/mongo/life_cycle.go | 19 +++++++++++-------- .../internal/driver/mongo/life_cycle_test.go | 14 ++++++++------ 2 files changed, 19 insertions(+), 14 deletions(-) diff --git a/persistent/internal/driver/mongo/life_cycle.go b/persistent/internal/driver/mongo/life_cycle.go index 25c5f316..92071712 100644 --- a/persistent/internal/driver/mongo/life_cycle.go +++ b/persistent/internal/driver/mongo/life_cycle.go @@ -64,7 +64,12 @@ type urlInfo struct { user string pass string db string - options map[string]string + options []urlOptions +} + +type urlOptions struct { + key string + val string } func isOptSep(c rune) bool { @@ -112,14 +117,12 @@ func parseURL(s string) (string, *urlInfo, error) { connString += strings.Join(info.addrs, ",") - if info.db != "" { - connString += "/" + info.db - } + connString += "/" + info.db if len(info.options) > 0 { connString += "?" - for k, v := range info.options { - connString += k + "=" + v + "&" + for _, v := range info.options { + connString += v.key + "=" + v.val + "&" } connString = connString[:len(connString)-1] @@ -129,7 +132,7 @@ func parseURL(s string) (string, *urlInfo, error) { } func extractURL(s string) (*urlInfo, error) { - info := &urlInfo{options: make(map[string]string)} + info := &urlInfo{options: make([]urlOptions, 0)} if c := strings.Index(s, "?"); c != -1 { for _, pair := range strings.FieldsFunc(s[c+1:], isOptSep) { @@ -138,7 +141,7 @@ func extractURL(s string) (*urlInfo, error) { return nil, errors.New("connection option must be key=value: " + pair) } - info.options[l[0]] = l[1] + info.options = append(info.options, urlOptions{key: l[0], val: l[1]}) } s = s[:c] diff --git a/persistent/internal/driver/mongo/life_cycle_test.go b/persistent/internal/driver/mongo/life_cycle_test.go index 5f8ceba0..7ec53878 100644 --- a/persistent/internal/driver/mongo/life_cycle_test.go +++ b/persistent/internal/driver/mongo/life_cycle_test.go @@ -1,6 +1,3 @@ -//go:build mongo -// +build mongo - package mongo import ( @@ -147,7 +144,7 @@ func TestConnect(t *testing.T) { { name: "valid connection_string", opts: &types.ClientOpts{ - ConnectionString: "mongodb://localhost:27017/test", + ConnectionString: "mongodb+srv://tyk:6}3cZQU.9KvM/hVR4qkm-hHqZTu3yg=G@cluster0.zlgvyel.mongodb.net/?retryWrites=true&w=majority", UseSSL: false, Type: "mongodb", }, @@ -211,7 +208,7 @@ func TestParseURL(t *testing.T) { { name: "valid connection string with @", url: "mongodb://user:p@ssword@localhost:27017", - want: "mongodb://user:p@ssword@localhost:27017", + want: "mongodb://user:p@ssword@localhost:27017/", }, { name: "valid connection string with @ and /", @@ -268,13 +265,18 @@ func TestParseURL(t *testing.T) { { name: "connection string without database", url: "mongodb://user:password@localhost:27017", - want: "mongodb://user:password@localhost:27017", + want: "mongodb://user:password@localhost:27017/", }, { name: "cosmosdb url", url: "mongodb+srv://4-0-qa:zFAQ==@4-0-qa.azure:10/a1?appName=@4-testing@&maxIdleTimeMS=120000", want: "mongodb+srv://4-0-qa:zFAQ%3D%3D@4-0-qa.azure:10/a1?appName=@4-testing@&maxIdleTimeMS=120000", }, + { + name: "cosmosdb url without database with options", + url: "mongodb+srv://tyk:6}3cZQU.9KvM/hVR4qkm-hHqZTu3yg=G@cluster0.zlgvyel.mongodb.net/?retryWrites=true&w=majority", + want: "mongodb+srv://tyk:6%7D3cZQU.9KvM%2FhVR4qkm-hHqZTu3yg%3DG@cluster0.zlgvyel.mongodb.net/?retryWrites=true&w=majority", + }, } for _, test := range tests { From 9919a844d324c9d0a68ca65737fab847f5ab8ded Mon Sep 17 00:00:00 2001 From: Matias <83959431+mativm02@users.noreply.github.com> Date: Thu, 3 Aug 2023 13:52:53 -0300 Subject: [PATCH 10/13] adding comment to struct --- persistent/internal/driver/mongo/life_cycle.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/persistent/internal/driver/mongo/life_cycle.go b/persistent/internal/driver/mongo/life_cycle.go index 92071712..190fe773 100644 --- a/persistent/internal/driver/mongo/life_cycle.go +++ b/persistent/internal/driver/mongo/life_cycle.go @@ -67,6 +67,8 @@ type urlInfo struct { options []urlOptions } +// urlOptions is a key/value pair representing a single option in a URL. +// we need to use this struct instead of a map to avoid flaky tests due to the order of the options type urlOptions struct { key string val string From e5154188482621b0affd66496ddf5b5fdfb87ee4 Mon Sep 17 00:00:00 2001 From: Matias <83959431+mativm02@users.noreply.github.com> Date: Thu, 3 Aug 2023 13:53:47 -0300 Subject: [PATCH 11/13] restoring original conn string in test --- persistent/internal/driver/mongo/life_cycle_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/persistent/internal/driver/mongo/life_cycle_test.go b/persistent/internal/driver/mongo/life_cycle_test.go index 7ec53878..281d1dc3 100644 --- a/persistent/internal/driver/mongo/life_cycle_test.go +++ b/persistent/internal/driver/mongo/life_cycle_test.go @@ -144,7 +144,7 @@ func TestConnect(t *testing.T) { { name: "valid connection_string", opts: &types.ClientOpts{ - ConnectionString: "mongodb+srv://tyk:6}3cZQU.9KvM/hVR4qkm-hHqZTu3yg=G@cluster0.zlgvyel.mongodb.net/?retryWrites=true&w=majority", + ConnectionString: "mongodb://localhost:27017/test", UseSSL: false, Type: "mongodb", }, From b4e401c519fea2cd120a604efa988c5e0d2397b3 Mon Sep 17 00:00:00 2001 From: Matias <83959431+mativm02@users.noreply.github.com> Date: Fri, 4 Aug 2023 10:46:42 -0300 Subject: [PATCH 12/13] linting --- persistent/internal/driver/mongo/life_cycle_test.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/persistent/internal/driver/mongo/life_cycle_test.go b/persistent/internal/driver/mongo/life_cycle_test.go index 281d1dc3..195ba374 100644 --- a/persistent/internal/driver/mongo/life_cycle_test.go +++ b/persistent/internal/driver/mongo/life_cycle_test.go @@ -274,8 +274,8 @@ func TestParseURL(t *testing.T) { }, { name: "cosmosdb url without database with options", - url: "mongodb+srv://tyk:6}3cZQU.9KvM/hVR4qkm-hHqZTu3yg=G@cluster0.zlgvyel.mongodb.net/?retryWrites=true&w=majority", - want: "mongodb+srv://tyk:6%7D3cZQU.9KvM%2FhVR4qkm-hHqZTu3yg%3DG@cluster0.zlgvyel.mongodb.net/?retryWrites=true&w=majority", + url: "mongodb+srv://tyk:6}3c.9KvM/hVR4qkm-hu3yg=G@clu0.zl.mongodb.net/?retryWrites=true&w=majority", + want: "mongodb+srv://tyk:6%7D3c.9KvM%2FhVR4qkm-hu3yg%3DG@clu0.zl.mongodb.net/?retryWrites=true&w=majority", }, } @@ -286,7 +286,6 @@ func TestParseURL(t *testing.T) { assert.Equal(t, test.wantErr, err != nil) }) } - } func TestClose(t *testing.T) { From 01b6eff23b6a0cbb63ea05b326d720be8d6f73a7 Mon Sep 17 00:00:00 2001 From: Matias <83959431+mativm02@users.noreply.github.com> Date: Fri, 4 Aug 2023 11:08:59 -0300 Subject: [PATCH 13/13] fixing bug smells --- .../internal/driver/mongo/life_cycle.go | 60 ++++++++++++++----- 1 file changed, 45 insertions(+), 15 deletions(-) diff --git a/persistent/internal/driver/mongo/life_cycle.go b/persistent/internal/driver/mongo/life_cycle.go index 190fe773..de8aa164 100644 --- a/persistent/internal/driver/mongo/life_cycle.go +++ b/persistent/internal/driver/mongo/life_cycle.go @@ -28,6 +28,11 @@ type lifeCycle struct { var _ types.StorageLifecycle = &lifeCycle{} +const ( + MongoPrefix = "mongodb://" + MongoSRVPrefix = "mongodb+srv://" +) + // Connect connects to the mongo database given the ClientOpts. func (lc *lifeCycle) Connect(opts *types.ClientOpts) error { var err error @@ -82,17 +87,17 @@ func parseURL(s string) (string, *urlInfo, error) { var info *urlInfo prefix := "" - if strings.HasPrefix(s, "mongodb://") { - prefix = "mongodb://" - } else if strings.HasPrefix(s, "mongodb+srv://") { - prefix = "mongodb+srv://" + if strings.HasPrefix(s, MongoPrefix) { + prefix = MongoPrefix + } else if strings.HasPrefix(s, MongoSRVPrefix) { + prefix = MongoSRVPrefix } switch prefix { - case "mongodb://": - s = strings.TrimPrefix(s, "mongodb://") - case "mongodb+srv://": - s = strings.TrimPrefix(s, "mongodb+srv://") + case MongoPrefix: + s = strings.TrimPrefix(s, MongoPrefix) + case MongoSRVPrefix: + s = strings.TrimPrefix(s, MongoSRVPrefix) default: return "", info, errors.New("invalid connection string, no prefix found") } @@ -135,12 +140,31 @@ func parseURL(s string) (string, *urlInfo, error) { func extractURL(s string) (*urlInfo, error) { info := &urlInfo{options: make([]urlOptions, 0)} + var err error + + if s, err = extractOptions(s, info); err != nil { + return nil, err + } + + if s, err = extractCredentials(s, info); err != nil { + return nil, err + } + + if s, err = extractDatabase(s, info); err != nil { + return nil, err + } + info.addrs = strings.Split(s, ",") + + return info, nil +} + +func extractOptions(s string, info *urlInfo) (string, error) { if c := strings.Index(s, "?"); c != -1 { for _, pair := range strings.FieldsFunc(s[c+1:], isOptSep) { l := strings.SplitN(pair, "=", 2) if len(l) != 2 || l[0] == "" || l[1] == "" { - return nil, errors.New("connection option must be key=value: " + pair) + return s, errors.New("connection option must be key=value: " + pair) } info.options = append(info.options, urlOptions{key: l[0], val: l[1]}) @@ -149,37 +173,43 @@ func extractURL(s string) (*urlInfo, error) { s = s[:c] } + return s, nil +} + +func extractCredentials(s string, info *urlInfo) (string, error) { if c := strings.Index(s, "@"); c != -1 { pair := strings.SplitN(s[:c], ":", 2) if len(pair) > 2 || pair[0] == "" { - return nil, errors.New("credentials must be provided as user:pass@host") + return s, errors.New("credentials must be provided as user:pass@host") } var err error info.user, err = url.QueryUnescape(pair[0]) if err != nil { - return nil, fmt.Errorf("cannot unescape username in URL: %q", pair[0]) + return s, fmt.Errorf("cannot unescape username in URL: %q", pair[0]) } if len(pair) > 1 { info.pass, err = url.QueryUnescape(pair[1]) if err != nil { - return nil, fmt.Errorf("cannot unescape password in URL") + return s, fmt.Errorf("cannot unescape password in URL") } } s = s[c+1:] } + return s, nil +} + +func extractDatabase(s string, info *urlInfo) (string, error) { if c := strings.Index(s, "/"); c != -1 { info.db = s[c+1:] s = s[:c] } - info.addrs = strings.Split(s, ",") - - return info, nil + return s, nil } // Close finish the session.