From f3871186fdf2cdfabbbc4a52a6cf51ea2ab0a5f5 Mon Sep 17 00:00:00 2001 From: Aerex Date: Wed, 8 Nov 2023 16:17:26 -0600 Subject: [PATCH 01/11] feat: added pagination cache config --- .../configuration/config_helpers/helpers.go | 17 ---- .../config_helpers/helpers_test.go | 79 ---------------- .../configuration/core_config/bx_config.go | 40 ++++++++ .../core_config/bx_config_test.go | 28 ++++++ .../configuration/core_config/repository.go | 21 +++++ bluemix/models/pagination.go | 15 +++ common/rest/request.go | 88 ++++++++++-------- common/rest/request_test.go | 93 +++++++++++++++++-- 8 files changed, 242 insertions(+), 139 deletions(-) create mode 100644 bluemix/models/pagination.go diff --git a/bluemix/configuration/config_helpers/helpers.go b/bluemix/configuration/config_helpers/helpers.go index ed1f2a8d..1dcf9938 100644 --- a/bluemix/configuration/config_helpers/helpers.go +++ b/bluemix/configuration/config_helpers/helpers.go @@ -21,23 +21,6 @@ func ConfigDir() string { return defaultConfigDirOld() } -// func MigrateFromOldConfig() error { -// new := defaultConfigDirNew() -// if file_helpers.FileExists(new) { -// return nil -// } - -// old := defaultConfigDirOld() -// if !file_helpers.FileExists(old) { -// return nil -// } - -// if err := file_helpers.CopyDir(old, new); err != nil { -// return err -// } -// return os.RemoveAll(old) -// } - func defaultConfigDirNew() string { return filepath.Join(homeDir(), ".ibmcloud") } diff --git a/bluemix/configuration/config_helpers/helpers_test.go b/bluemix/configuration/config_helpers/helpers_test.go index c82b6575..9c2ac48b 100644 --- a/bluemix/configuration/config_helpers/helpers_test.go +++ b/bluemix/configuration/config_helpers/helpers_test.go @@ -103,82 +103,3 @@ func TestConfigDir_IbmCloudConfigHomeSet_Exists(t *testing.T) { os.Setenv("IBMCLOUD_CONFIG_HOME", userHome) assert.Equal(userHome, ConfigDir()) } - -// func TestMigrateFromOldConfig(t *testing.T) { -// assert := assert.New(t) - -// err := prepareBluemixHome() -// assert.NoError(err) -// defer clearBluemixHome() - -// err = os.MkdirAll(oldConfigDir(), 0700) -// assert.NoError(err) -// oldConfigPath := filepath.Join(oldConfigDir(), "config.json") -// err = ioutil.WriteFile(oldConfigPath, []byte("old"), 0600) -// assert.NoError(err) - -// err = MigrateFromOldConfig() -// assert.NoError(err) - -// newConfigPath := filepath.Join(newConfigDir(), "config.json") -// assert.True(file_helpers.FileExists(newConfigPath)) -// content, err := ioutil.ReadFile(newConfigPath) -// assert.NoError(err) -// assert.Equal([]byte("old"), content, "should copy old config file") - -// assert.False(file_helpers.FileExists(oldConfigDir()), "old config dir should be deleted") -// } - -// func TestMigrateFromOldConfig_NewConfigExist(t *testing.T) { -// assert := assert.New(t) - -// err := prepareBluemixHome() -// assert.NoError(err) -// defer clearBluemixHome() - -// err = os.MkdirAll(oldConfigDir(), 0700) -// assert.NoError(err) -// oldConfigPath := filepath.Join(oldConfigDir(), "config.json") -// err = ioutil.WriteFile(oldConfigPath, []byte("old"), 0600) -// assert.NoError(err) - -// err = os.MkdirAll(newConfigDir(), 0700) -// assert.NoError(err) -// newConfigPath := filepath.Join(newConfigDir(), "config.json") -// err = ioutil.WriteFile(newConfigPath, []byte("new"), 0600) -// assert.NoError(err) - -// err = MigrateFromOldConfig() -// assert.NoError(err) - -// content, err := ioutil.ReadFile(newConfigPath) -// assert.NoError(err) -// assert.Equal([]byte("new"), content, "should not copy old config file") -// } - -// func TestMigrateFromOldConfig_OldConfigNotExist(t *testing.T) { -// assert := assert.New(t) - -// err := prepareBluemixHome() -// assert.NoError(err) -// defer clearBluemixHome() - -// err = MigrateFromOldConfig() -// assert.NoError(err) -// } - -// func prepareBluemixHome() error { -// temp, err := ioutil.TempDir("", "IBMCloudSDKConfigTest") -// if err != nil { -// return err -// } -// os.Setenv("BLUEMIX_HOME", temp) -// return nil -// } - -// func clearBluemixHome() { -// if homeDir := os.Getenv("BLUEMIX_HOME"); homeDir != "" { -// os.RemoveAll(homeDir) -// os.Unsetenv("BLUEMIX_HOME") -// } -// } diff --git a/bluemix/configuration/core_config/bx_config.go b/bluemix/configuration/core_config/bx_config.go index 31f87078..abae6597 100644 --- a/bluemix/configuration/core_config/bx_config.go +++ b/bluemix/configuration/core_config/bx_config.go @@ -2,6 +2,7 @@ package core_config import ( "encoding/json" + "sort" "strings" "sync" "time" @@ -63,6 +64,7 @@ type BXConfigData struct { UpdateCheckInterval time.Duration UpdateRetryCheckInterval time.Duration UpdateNotificationInterval time.Duration + PaginationURLs []models.PaginationURL raw raw } @@ -752,9 +754,47 @@ func (c *bxConfig) ClearSession() { c.data.IsLoggedInAsCRI = false c.data.ResourceGroup = models.ResourceGroup{} c.data.LoginAt = time.Time{} + c.data.PaginationURLs = []models.PaginationURL{} }) } +func (c *bxConfig) SetPaginationURLs(paginationURLs []models.PaginationURL) { + c.write(func() { + c.data.PaginationURLs = paginationURLs + }) +} + +func (c *bxConfig) ClearPaginationURLs() { + c.write(func() { + c.data.PaginationURLs = []models.PaginationURL{} + }) +} + +func (c *bxConfig) AddPaginationURL(index int, url string) error { + urls, err := c.PaginationURLs() + if err != nil { + return err + } + + urls = append(urls, models.PaginationURL{ + LastIndex: index, + NextURL: url, + }) + + // sort by last index for easier retrieval + sort.Sort(models.ByLastIndex(urls)) + c.SetPaginationURLs(urls) + return nil +} + +func (c *bxConfig) PaginationURLs() (paginationURLs []models.PaginationURL, err error) { + c.read(func() { + paginationURLs = c.data.PaginationURLs + }) + + return +} + func (c *bxConfig) UnsetAPI() { c.write(func() { c.data.APIEndpoint = "" diff --git a/bluemix/configuration/core_config/bx_config_test.go b/bluemix/configuration/core_config/bx_config_test.go index 3d329074..61f37553 100644 --- a/bluemix/configuration/core_config/bx_config_test.go +++ b/bluemix/configuration/core_config/bx_config_test.go @@ -469,6 +469,34 @@ func TestLastUpdateSessionTime(t *testing.T) { } +func TestPaginationURLs(t *testing.T) { + config := prepareConfigForCLI(`{}`, t) + + // check initial state + paginationURLs, err := config.PaginationURLs() + assert.Nil(t, err) + assert.Empty(t, paginationURLs) + + // update session + expected := []models.PaginationURL{ + { + NextURL: "https://api.example.com?token=dd3784000d9744acb2a23ad121a7bb4b", + LastIndex: 50, + }, + } + err = config.SetPaginationURLs(expected) + assert.Nil(t, err) + + paginationURLs, err = config.PaginationURLs() + assert.Nil(t, err) + assert.Equal(t, 1, len(paginationURLs)) + assert.Equal(t, expected[0].LastIndex, paginationURLs[0].LastIndex) + assert.Equal(t, expected[0].NextURL, paginationURLs[0].NextURL) + + t.Cleanup(cleanupConfigFiles) + +} + func checkUsageStats(enabled bool, timeStampExist bool, config core_config.Repository, t *testing.T) { assert.Equal(t, config.UsageStatsEnabled(), enabled) assert.Equal(t, config.UsageStatsEnabledLastUpdate().IsZero(), !timeStampExist) diff --git a/bluemix/configuration/core_config/repository.go b/bluemix/configuration/core_config/repository.go index 7465cf05..3c6839a4 100644 --- a/bluemix/configuration/core_config/repository.go +++ b/bluemix/configuration/core_config/repository.go @@ -125,6 +125,11 @@ type Repository interface { SetLastSessionUpdateTime() LastSessionUpdateTime() (session int64) + + SetPaginationURLs(paginationURLs []models.PaginationURL) + ClearPaginationURLs() + AddPaginationURL(lastIndex int, nextURL string) error + PaginationURLs() ([]models.PaginationURL, error) } // Deprecated @@ -368,6 +373,22 @@ func (c repository) SetLastSessionUpdateTime() { c.bxConfig.SetLastSessionUpdateTime() } +func (c repository) PaginationURLs() ([]models.PaginationURL, error) { + return c.bxConfig.PaginationURLs() +} + +func (c repository) AddPaginationURL(index int, url string) error { + return c.bxConfig.AddPaginationURL(index, url) +} + +func (c repository) SetPaginationURLs(paginationURLs []models.PaginationURL) { + c.bxConfig.SetPaginationURLs(paginationURLs) +} + +func (c repository) ClearPaginationURLs() { + c.bxConfig.ClearPaginationURLs() +} + func NewCoreConfig(errHandler func(error)) ReadWriter { // config_helpers.MigrateFromOldConfig() // error ignored return NewCoreConfigFromPath(config_helpers.CFConfigFilePath(), config_helpers.ConfigFilePath(), errHandler) diff --git a/bluemix/models/pagination.go b/bluemix/models/pagination.go new file mode 100644 index 00000000..b6b75360 --- /dev/null +++ b/bluemix/models/pagination.go @@ -0,0 +1,15 @@ +package models + +// ByLastIndex sorts PaginationURLs by LastIndex +type ByLastIndex []PaginationURL + +func (a ByLastIndex) Len() int { return len(a) } + +func (a ByLastIndex) Swap(i, j int) { a[i], a[j] = a[j], a[i] } + +func (a ByLastIndex) Less(i, j int) bool { return a[i].LastIndex < a[j].LastIndex } + +type PaginationURL struct { + LastIndex int + NextURL string +} diff --git a/common/rest/request.go b/common/rest/request.go index e53184cd..6b461007 100644 --- a/common/rest/request.go +++ b/common/rest/request.go @@ -1,44 +1,45 @@ // Request examples: -// // create a simple GET request -// req := GetRequest("http://www.example.com") // -// // set header -// req.Set("Accept", "application/json") +// // create a simple GET request +// req := GetRequest("http://www.example.com") // -// // set query parameters -// req.Query("foo1", "bar1") -// req.Query("foo2", "bar2") +// // set header +// req.Set("Accept", "application/json") // -// // Build to a HTTP request -// req.Build() +// // set query parameters +// req.Query("foo1", "bar1") +// req.Query("foo2", "bar2") // -// // method chaining is also supported -// // the above is equal to: -// GetRequest("http://www.example.com"). -// Set("Accept", "application/json"). -// Query("foo1", "bar1"). -// Query("foo2", "bar2"). -// Build() +// // Build to a HTTP request +// req.Build() // -// // struct body -// foo = Foo{Bar: "val"} -// PostRequest("http://www.example.com"). -// Body(foo) +// // method chaining is also supported +// // the above is equal to: +// GetRequest("http://www.example.com"). +// Set("Accept", "application/json"). +// Query("foo1", "bar1"). +// Query("foo2", "bar2"). +// Build() // -// // String body -// PostRequest("http://www.example.com"). -// Body("{\"bar\": \"val\"}") +// // struct body +// foo = Foo{Bar: "val"} +// PostRequest("http://www.example.com"). +// Body(foo) // -// // Stream body -// PostRequest("http://www.example.com"). -// Body(strings.NewReader("abcde")) +// // String body +// PostRequest("http://www.example.com"). +// Body("{\"bar\": \"val\"}") // -// // Multipart POST request -// var f *os.File -// PostRequest("http://www.example.com"). -// Field("foo", "bar"). -// File("file1", File{Name: f.Name(), Content: f}). -// File("file2", File{Name: "1.txt", Content: []byte("abcde"), Type: "text/plain"}) +// // Stream body +// PostRequest("http://www.example.com"). +// Body(strings.NewReader("abcde")) +// +// // Multipart POST request +// var f *os.File +// PostRequest("http://www.example.com"). +// Field("foo", "bar"). +// File("file1", File{Name: f.Name(), Content: f}). +// File("file2", File{Name: "1.txt", Content: []byte("abcde"), Type: "text/plain"}) package rest import ( @@ -51,12 +52,14 @@ import ( "net/textproto" "net/url" "strings" + + "github.com/IBM-Cloud/ibm-cloud-cli-sdk/bluemix/models" ) const ( - contentType = "Content-Type" - jsonContentType = "application/json" - formUrlEncodedContentType = "application/x-www-form-urlencoded" + ContentType = "Content-Type" + JSONContentType = "application/json" + FormUrlEncodedContentType = "application/x-www-form-urlencoded" ) // File represents a file upload in HTTP request @@ -138,6 +141,17 @@ func OptionsRequest(rawUrl string) *Request { return NewRequest(rawUrl).Method("OPTIONS") } +// CachedPaginationNextURL will attempt to return a cached URL with last index +// if there exists a URL with a last index smaller than the offset provided +func CachedPaginationNextURL(paginationURLs []models.PaginationURL, offset int) models.PaginationURL { + for _, p := range paginationURLs { + if p.LastIndex < offset { + return p + } + } + return models.PaginationURL{} +} + // Add adds the key, value pair to the request header. It appends to any // existing values associated with key. func (r *Request) Add(key string, value string) *Request { @@ -272,7 +286,7 @@ func (r *Request) buildFormMultipart() (io.Reader, error) { } } - r.header.Set(contentType, w.FormDataContentType()) + r.header.Set(ContentType, w.FormDataContentType()) return b, nil } @@ -296,7 +310,7 @@ func escapeQuotes(s string) string { } func (r *Request) buildFormFields() (io.Reader, error) { - r.header.Set(contentType, formUrlEncodedContentType) + r.header.Set(ContentType, FormUrlEncodedContentType) return strings.NewReader(r.formParams.Encode()), nil } diff --git a/common/rest/request_test.go b/common/rest/request_test.go index b9657ce1..6cbc9311 100644 --- a/common/rest/request_test.go +++ b/common/rest/request_test.go @@ -1,13 +1,16 @@ -package rest +package rest_test import ( "bytes" "io" - "io/ioutil" "os" "strings" "testing" + "github.com/IBM-Cloud/ibm-cloud-cli-sdk/bluemix/models" + . "github.com/IBM-Cloud/ibm-cloud-cli-sdk/common/rest" + testhelpers "github.com/IBM-Cloud/ibm-cloud-cli-sdk/testhelpers/configuration" + "github.com/stretchr/testify/assert" ) @@ -49,7 +52,7 @@ func TestRequestFormText(t *testing.T) { assert.NoError(err) err = req.ParseForm() assert.NoError(err) - assert.Equal(formUrlEncodedContentType, req.Header.Get(contentType)) + assert.Equal(FormUrlEncodedContentType, req.Header.Get(ContentType)) assert.Equal("bar", req.FormValue("foo")) } @@ -57,7 +60,7 @@ func TestRequestFormMultipart(t *testing.T) { assert := assert.New(t) var prepareFileWithContent = func(text string) (*os.File, error) { - f, err := ioutil.TempFile("", "BluemixCliRestTest") + f, err := os.CreateTemp("", "BluemixCliRestTest") if err != nil { return nil, err } @@ -84,7 +87,7 @@ func TestRequestFormMultipart(t *testing.T) { Build() assert.NoError(err) - assert.Contains(req.Header.Get(contentType), "multipart/form-data") + assert.Contains(req.Header.Get(ContentType), "multipart/form-data") err = req.ParseMultipartForm(int64(5000)) assert.NoError(err) @@ -125,7 +128,85 @@ func TestRequestJSON(t *testing.T) { req, err := PostRequest("http://www.example.com").Body(&foo).Build() assert.NoError(err) - body, err := ioutil.ReadAll(req.Body) + body, err := io.ReadAll(req.Body) assert.NoError(err) assert.Equal("{\"Name\":\"bar\"}", string(body)) } + +func TestCachedPaginationNextURL(t *testing.T) { + testCases := []struct { + name string + paginationURLs []models.PaginationURL + offset int + expectedNextURL string + }{ + { + name: "return cached next URL", + offset: 200, + paginationURLs: []models.PaginationURL{ + { + NextURL: "/v2/example.com/stuff?limit=100", + LastIndex: 100, + }, + }, + expectedNextURL: "/v2/example.com/stuff?limit=100", + }, + { + name: "return empty string if cache URL cannot be determined", + offset: 40, + paginationURLs: []models.PaginationURL{ + { + NextURL: "/v2/example.com/stuff?limit=100", + LastIndex: 60, + }, + }, + expectedNextURL: "", + }, + { + name: "return empty string if no cache available", + offset: 40, + paginationURLs: []models.PaginationURL{}, + expectedNextURL: "", + }, + } + + assert := assert.New(t) + + for _, tc := range testCases { + t.Run(tc.name, func(_ *testing.T) { + p := CachedPaginationNextURL(tc.paginationURLs, tc.offset) + assert.Equal(tc.expectedNextURL, p.NextURL) + }) + } +} + +func TestAddPaginationURL(t *testing.T) { + config := testhelpers.NewFakeCoreConfig() + assert := assert.New(t) + unsortedUrls := []models.PaginationURL{ + { + NextURL: "/v2/example.com/stuff?limit=200", + LastIndex: 200, + }, + { + NextURL: "/v2/example.com/stuff?limit=100", + LastIndex: 100, + }, + } + + var err error + for _, p := range unsortedUrls { + err = config.AddPaginationURL(p.LastIndex, p.NextURL) + assert.Nil(err) + } + + // expect url to be sorted in ascending order by LastIndex + sortedUrls, err := config.PaginationURLs() + assert.Nil(err) + + assert.Equal(2, len(sortedUrls)) + assert.Equal(sortedUrls[0].LastIndex, unsortedUrls[1].LastIndex) + assert.Equal(sortedUrls[0].NextURL, unsortedUrls[1].NextURL) + assert.Equal(sortedUrls[1].LastIndex, unsortedUrls[0].LastIndex) + assert.Equal(sortedUrls[1].NextURL, unsortedUrls[0].NextURL) +} From eb1ccc9c36cdb6e1b14116cbfcd17cf67e6045d7 Mon Sep 17 00:00:00 2001 From: Aerex Date: Mon, 13 Nov 2023 10:08:00 -0600 Subject: [PATCH 02/11] chore: updated go to 1.21 --- .travis.yml | 2 +- go.mod | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/.travis.yml b/.travis.yml index 5098eeb2..a6160882 100644 --- a/.travis.yml +++ b/.travis.yml @@ -1,7 +1,7 @@ language: go dist: bionic go: -- '1.17.x' +- '1.21.x' addons: apt: packages: diff --git a/go.mod b/go.mod index a06b0706..39cffbf1 100644 --- a/go.mod +++ b/go.mod @@ -1,6 +1,6 @@ module github.com/IBM-Cloud/ibm-cloud-cli-sdk -go 1.20 +go 1.21 require ( github.com/fatih/color v1.7.1-0.20180516100307-2d684516a886 From 27d61bb28be122b92aa83c12a2d64d0ad3fdee15 Mon Sep 17 00:00:00 2001 From: Aerex Date: Mon, 13 Nov 2023 10:16:51 -0600 Subject: [PATCH 03/11] build: updated travis to use latest ubuntu distro --- .travis.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.travis.yml b/.travis.yml index a6160882..aac929b0 100644 --- a/.travis.yml +++ b/.travis.yml @@ -1,5 +1,5 @@ language: go -dist: bionic +dist: mantic go: - '1.21.x' addons: From 5f331e31a9ff0fc9e26dcd536e55a80acf7eae2e Mon Sep 17 00:00:00 2001 From: Aerex Date: Mon, 13 Nov 2023 10:24:21 -0600 Subject: [PATCH 04/11] test: fixed test --- .../core_config/bx_config_test.go | 2 +- common/rest/request.go | 65 +++++++++---------- 2 files changed, 33 insertions(+), 34 deletions(-) diff --git a/bluemix/configuration/core_config/bx_config_test.go b/bluemix/configuration/core_config/bx_config_test.go index 61f37553..0dafb94d 100644 --- a/bluemix/configuration/core_config/bx_config_test.go +++ b/bluemix/configuration/core_config/bx_config_test.go @@ -484,7 +484,7 @@ func TestPaginationURLs(t *testing.T) { LastIndex: 50, }, } - err = config.SetPaginationURLs(expected) + config.SetPaginationURLs(expected) assert.Nil(t, err) paginationURLs, err = config.PaginationURLs() diff --git a/common/rest/request.go b/common/rest/request.go index 6b461007..f1badd9c 100644 --- a/common/rest/request.go +++ b/common/rest/request.go @@ -1,45 +1,44 @@ // Request examples: +// // create a simple GET request +// req := GetRequest("http://www.example.com") // -// // create a simple GET request -// req := GetRequest("http://www.example.com") +// // set header +// req.Set("Accept", "application/json") // -// // set header -// req.Set("Accept", "application/json") +// // set query parameters +// req.Query("foo1", "bar1") +// req.Query("foo2", "bar2") // -// // set query parameters -// req.Query("foo1", "bar1") -// req.Query("foo2", "bar2") +// // Build to a HTTP request +// req.Build() // -// // Build to a HTTP request -// req.Build() +// // method chaining is also supported +// // the above is equal to: +// GetRequest("http://www.example.com"). +// Set("Accept", "application/json"). +// Query("foo1", "bar1"). +// Query("foo2", "bar2"). +// Build() // -// // method chaining is also supported -// // the above is equal to: -// GetRequest("http://www.example.com"). -// Set("Accept", "application/json"). -// Query("foo1", "bar1"). -// Query("foo2", "bar2"). -// Build() +// // struct body +// foo = Foo{Bar: "val"} +// PostRequest("http://www.example.com"). +// Body(foo) // -// // struct body -// foo = Foo{Bar: "val"} -// PostRequest("http://www.example.com"). -// Body(foo) +// // String body +// PostRequest("http://www.example.com"). +// Body("{\"bar\": \"val\"}") // -// // String body -// PostRequest("http://www.example.com"). -// Body("{\"bar\": \"val\"}") +// // Stream body +// PostRequest("http://www.example.com"). +// Body(strings.NewReader("abcde")) // -// // Stream body -// PostRequest("http://www.example.com"). -// Body(strings.NewReader("abcde")) -// -// // Multipart POST request -// var f *os.File -// PostRequest("http://www.example.com"). -// Field("foo", "bar"). -// File("file1", File{Name: f.Name(), Content: f}). -// File("file2", File{Name: "1.txt", Content: []byte("abcde"), Type: "text/plain"}) +// // Multipart POST request +// var f *os.File +// PostRequest("http://www.example.com"). +// Field("foo", "bar"). +// File("file1", File{Name: f.Name(), Content: f}). +// File("file2", File{Name: "1.txt", Content: []byte("abcde"), Type: "text/plain"}) package rest import ( From 66e8380d553d163e24e450b3497900b91fca57c5 Mon Sep 17 00:00:00 2001 From: Aerex Date: Mon, 13 Nov 2023 13:18:35 -0600 Subject: [PATCH 05/11] build: used 20.04 for travis --- .travis.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.travis.yml b/.travis.yml index aac929b0..f8bfd173 100644 --- a/.travis.yml +++ b/.travis.yml @@ -1,5 +1,5 @@ language: go -dist: mantic +dist: focal go: - '1.21.x' addons: From 74869e5871192d53eb05646404e4f908d41ae774 Mon Sep 17 00:00:00 2001 From: Aerex Date: Mon, 13 Nov 2023 16:27:01 -0600 Subject: [PATCH 06/11] fix: removed unused error return --- bluemix/configuration/core_config/bx_config.go | 7 ++----- bluemix/configuration/core_config/bx_config_test.go | 7 ++----- bluemix/configuration/core_config/repository.go | 4 ++-- common/rest/request_test.go | 3 +-- 4 files changed, 7 insertions(+), 14 deletions(-) diff --git a/bluemix/configuration/core_config/bx_config.go b/bluemix/configuration/core_config/bx_config.go index abae6597..2e39d39b 100644 --- a/bluemix/configuration/core_config/bx_config.go +++ b/bluemix/configuration/core_config/bx_config.go @@ -771,10 +771,7 @@ func (c *bxConfig) ClearPaginationURLs() { } func (c *bxConfig) AddPaginationURL(index int, url string) error { - urls, err := c.PaginationURLs() - if err != nil { - return err - } + urls := c.PaginationURLs() urls = append(urls, models.PaginationURL{ LastIndex: index, @@ -787,7 +784,7 @@ func (c *bxConfig) AddPaginationURL(index int, url string) error { return nil } -func (c *bxConfig) PaginationURLs() (paginationURLs []models.PaginationURL, err error) { +func (c *bxConfig) PaginationURLs() (paginationURLs []models.PaginationURL) { c.read(func() { paginationURLs = c.data.PaginationURLs }) diff --git a/bluemix/configuration/core_config/bx_config_test.go b/bluemix/configuration/core_config/bx_config_test.go index 0dafb94d..3f451150 100644 --- a/bluemix/configuration/core_config/bx_config_test.go +++ b/bluemix/configuration/core_config/bx_config_test.go @@ -473,8 +473,7 @@ func TestPaginationURLs(t *testing.T) { config := prepareConfigForCLI(`{}`, t) // check initial state - paginationURLs, err := config.PaginationURLs() - assert.Nil(t, err) + paginationURLs := config.PaginationURLs() assert.Empty(t, paginationURLs) // update session @@ -485,10 +484,8 @@ func TestPaginationURLs(t *testing.T) { }, } config.SetPaginationURLs(expected) - assert.Nil(t, err) - paginationURLs, err = config.PaginationURLs() - assert.Nil(t, err) + paginationURLs = config.PaginationURLs() assert.Equal(t, 1, len(paginationURLs)) assert.Equal(t, expected[0].LastIndex, paginationURLs[0].LastIndex) assert.Equal(t, expected[0].NextURL, paginationURLs[0].NextURL) diff --git a/bluemix/configuration/core_config/repository.go b/bluemix/configuration/core_config/repository.go index 3c6839a4..f65252ea 100644 --- a/bluemix/configuration/core_config/repository.go +++ b/bluemix/configuration/core_config/repository.go @@ -129,7 +129,7 @@ type Repository interface { SetPaginationURLs(paginationURLs []models.PaginationURL) ClearPaginationURLs() AddPaginationURL(lastIndex int, nextURL string) error - PaginationURLs() ([]models.PaginationURL, error) + PaginationURLs() []models.PaginationURL } // Deprecated @@ -373,7 +373,7 @@ func (c repository) SetLastSessionUpdateTime() { c.bxConfig.SetLastSessionUpdateTime() } -func (c repository) PaginationURLs() ([]models.PaginationURL, error) { +func (c repository) PaginationURLs() []models.PaginationURL { return c.bxConfig.PaginationURLs() } diff --git a/common/rest/request_test.go b/common/rest/request_test.go index 6cbc9311..726ff81a 100644 --- a/common/rest/request_test.go +++ b/common/rest/request_test.go @@ -201,8 +201,7 @@ func TestAddPaginationURL(t *testing.T) { } // expect url to be sorted in ascending order by LastIndex - sortedUrls, err := config.PaginationURLs() - assert.Nil(err) + sortedUrls := config.PaginationURLs() assert.Equal(2, len(sortedUrls)) assert.Equal(sortedUrls[0].LastIndex, unsortedUrls[1].LastIndex) From d7b2f43761c376d433f0c0f4b7bf880e9e9c2010 Mon Sep 17 00:00:00 2001 From: Aerex Date: Tue, 14 Nov 2023 09:27:23 -0600 Subject: [PATCH 07/11] chore: moved tests and clean up code --- .../configuration/core_config/bx_config.go | 3 +- .../core_config/bx_config_test.go | 35 ++++++++++++++++++- .../configuration/core_config/repository.go | 6 ++-- common/rest/request_test.go | 30 ---------------- 4 files changed, 38 insertions(+), 36 deletions(-) diff --git a/bluemix/configuration/core_config/bx_config.go b/bluemix/configuration/core_config/bx_config.go index 2e39d39b..907740fb 100644 --- a/bluemix/configuration/core_config/bx_config.go +++ b/bluemix/configuration/core_config/bx_config.go @@ -770,7 +770,7 @@ func (c *bxConfig) ClearPaginationURLs() { }) } -func (c *bxConfig) AddPaginationURL(index int, url string) error { +func (c *bxConfig) AddPaginationURL(index int, url string) { urls := c.PaginationURLs() urls = append(urls, models.PaginationURL{ @@ -781,7 +781,6 @@ func (c *bxConfig) AddPaginationURL(index int, url string) error { // sort by last index for easier retrieval sort.Sort(models.ByLastIndex(urls)) c.SetPaginationURLs(urls) - return nil } func (c *bxConfig) PaginationURLs() (paginationURLs []models.PaginationURL) { diff --git a/bluemix/configuration/core_config/bx_config_test.go b/bluemix/configuration/core_config/bx_config_test.go index 3f451150..7a654742 100644 --- a/bluemix/configuration/core_config/bx_config_test.go +++ b/bluemix/configuration/core_config/bx_config_test.go @@ -476,7 +476,6 @@ func TestPaginationURLs(t *testing.T) { paginationURLs := config.PaginationURLs() assert.Empty(t, paginationURLs) - // update session expected := []models.PaginationURL{ { NextURL: "https://api.example.com?token=dd3784000d9744acb2a23ad121a7bb4b", @@ -494,6 +493,40 @@ func TestPaginationURLs(t *testing.T) { } +func TestAddPaginationURL(t *testing.T) { + config := prepareConfigForCLI(`{}`, t) + assert := assert.New(t) + unsortedUrls := []models.PaginationURL{ + { + NextURL: "/v2/example.com/stuff?limit=200", + LastIndex: 200, + }, + { + NextURL: "/v2/example.com/stuff?limit=100", + LastIndex: 50, + }, + { + NextURL: "/v2/example.com/stuff?limit=100", + LastIndex: 100, + }, + } + + for _, p := range unsortedUrls { + config.AddPaginationURL(p.LastIndex, p.NextURL) + } + + // expect url to be sorted in ascending order by LastIndex + sortedUrls := config.PaginationURLs() + + assert.Equal(3, len(sortedUrls)) + assert.Equal(sortedUrls[0].LastIndex, unsortedUrls[1].LastIndex) + assert.Equal(sortedUrls[0].NextURL, unsortedUrls[1].NextURL) + assert.Equal(sortedUrls[1].LastIndex, unsortedUrls[2].LastIndex) + assert.Equal(sortedUrls[1].NextURL, unsortedUrls[2].NextURL) + assert.Equal(sortedUrls[2].LastIndex, unsortedUrls[0].LastIndex) + assert.Equal(sortedUrls[2].NextURL, unsortedUrls[0].NextURL) +} + func checkUsageStats(enabled bool, timeStampExist bool, config core_config.Repository, t *testing.T) { assert.Equal(t, config.UsageStatsEnabled(), enabled) assert.Equal(t, config.UsageStatsEnabledLastUpdate().IsZero(), !timeStampExist) diff --git a/bluemix/configuration/core_config/repository.go b/bluemix/configuration/core_config/repository.go index f65252ea..8c035e64 100644 --- a/bluemix/configuration/core_config/repository.go +++ b/bluemix/configuration/core_config/repository.go @@ -128,7 +128,7 @@ type Repository interface { SetPaginationURLs(paginationURLs []models.PaginationURL) ClearPaginationURLs() - AddPaginationURL(lastIndex int, nextURL string) error + AddPaginationURL(lastIndex int, nextURL string) PaginationURLs() []models.PaginationURL } @@ -377,8 +377,8 @@ func (c repository) PaginationURLs() []models.PaginationURL { return c.bxConfig.PaginationURLs() } -func (c repository) AddPaginationURL(index int, url string) error { - return c.bxConfig.AddPaginationURL(index, url) +func (c repository) AddPaginationURL(index int, url string) { + c.bxConfig.AddPaginationURL(index, url) } func (c repository) SetPaginationURLs(paginationURLs []models.PaginationURL) { diff --git a/common/rest/request_test.go b/common/rest/request_test.go index 726ff81a..c147ec8f 100644 --- a/common/rest/request_test.go +++ b/common/rest/request_test.go @@ -179,33 +179,3 @@ func TestCachedPaginationNextURL(t *testing.T) { }) } } - -func TestAddPaginationURL(t *testing.T) { - config := testhelpers.NewFakeCoreConfig() - assert := assert.New(t) - unsortedUrls := []models.PaginationURL{ - { - NextURL: "/v2/example.com/stuff?limit=200", - LastIndex: 200, - }, - { - NextURL: "/v2/example.com/stuff?limit=100", - LastIndex: 100, - }, - } - - var err error - for _, p := range unsortedUrls { - err = config.AddPaginationURL(p.LastIndex, p.NextURL) - assert.Nil(err) - } - - // expect url to be sorted in ascending order by LastIndex - sortedUrls := config.PaginationURLs() - - assert.Equal(2, len(sortedUrls)) - assert.Equal(sortedUrls[0].LastIndex, unsortedUrls[1].LastIndex) - assert.Equal(sortedUrls[0].NextURL, unsortedUrls[1].NextURL) - assert.Equal(sortedUrls[1].LastIndex, unsortedUrls[0].LastIndex) - assert.Equal(sortedUrls[1].NextURL, unsortedUrls[0].NextURL) -} From 175d53977d1b4e468de055d6ff90634394862ebb Mon Sep 17 00:00:00 2001 From: Aerex Date: Tue, 14 Nov 2023 12:54:39 -0600 Subject: [PATCH 08/11] chore: removed extraneous import --- common/rest/request_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/common/rest/request_test.go b/common/rest/request_test.go index c147ec8f..8a8833cb 100644 --- a/common/rest/request_test.go +++ b/common/rest/request_test.go @@ -9,7 +9,6 @@ import ( "github.com/IBM-Cloud/ibm-cloud-cli-sdk/bluemix/models" . "github.com/IBM-Cloud/ibm-cloud-cli-sdk/common/rest" - testhelpers "github.com/IBM-Cloud/ibm-cloud-cli-sdk/testhelpers/configuration" "github.com/stretchr/testify/assert" ) From 9ac63c308ddd94fea98296f19cfedf059d408d69 Mon Sep 17 00:00:00 2001 From: Aerex Date: Thu, 16 Nov 2023 09:07:17 -0600 Subject: [PATCH 09/11] feat: added validation method for pagination cache --- .../configuration/config_helpers/helpers.go | 44 +++++++++++++ .../config_helpers/helpers_test.go | 61 +++++++++++++++++++ common/rest/request_test.go | 16 +++++ 3 files changed, 121 insertions(+) diff --git a/bluemix/configuration/config_helpers/helpers.go b/bluemix/configuration/config_helpers/helpers.go index 1dcf9938..0e5c2cf7 100644 --- a/bluemix/configuration/config_helpers/helpers.go +++ b/bluemix/configuration/config_helpers/helpers.go @@ -2,6 +2,8 @@ package config_helpers import ( + "encoding/base64" + gourl "net/url" "os" "path/filepath" "runtime" @@ -91,3 +93,45 @@ func UserHomeDir() string { return os.Getenv("HOME") } + +// IsValidPaginationNextURL will return true if the provided nextURL has the expected queries provided +func IsValidPaginationNextURL(nextURL string, cursorQueryParamName string, expectedQueries gourl.Values) bool { + parsedURL, parseErr := gourl.Parse(nextURL) + // NOTE: ignore handling error(s) since if there error(s) + // we can assume the url is invalid + if parseErr != nil { + return false + } + + // retrive encoded cursor + // eg. /api?cursor= + queries := parsedURL.Query() + encodedQuery := queries.Get(cursorQueryParamName) + if encodedQuery == "" { + return false + + } + // decode string and parse encoded queries + decodedQuery, decodedErr := base64.RawURLEncoding.DecodeString(encodedQuery) + if decodedErr != nil { + return false + } + queries, parsedErr := gourl.ParseQuery(string(decodedQuery)) + if parsedErr != nil { + return false + } + + // compare expected queries that should match + // NOTE: assume queries are single value queries. + // if multi-value queries will check the first query + for expectedQuery := range expectedQueries { + paginationQueryValue := queries.Get(expectedQuery) + expectedQueryValue := expectedQueries.Get(expectedQuery) + if paginationQueryValue != expectedQueryValue { + return false + } + + } + + return true +} diff --git a/bluemix/configuration/config_helpers/helpers_test.go b/bluemix/configuration/config_helpers/helpers_test.go index 9c2ac48b..15f38c02 100644 --- a/bluemix/configuration/config_helpers/helpers_test.go +++ b/bluemix/configuration/config_helpers/helpers_test.go @@ -1,7 +1,9 @@ package config_helpers import ( + "encoding/base64" "io/ioutil" + gourl "net/url" "os" "path/filepath" "strings" @@ -103,3 +105,62 @@ func TestConfigDir_IbmCloudConfigHomeSet_Exists(t *testing.T) { os.Setenv("IBMCLOUD_CONFIG_HOME", userHome) assert.Equal(userHome, ConfigDir()) } + +func TestIsValidPaginationNextURL(t *testing.T) { + assert := assert.New(t) + + testCases := []struct { + name string + nextURL string + encodedQueryParam string + expectedQueries gourl.Values + isValid bool + }{ + { + name: "return true for matching expected queries in pagination url", + nextURL: "/api/example?cursor=" + base64.RawURLEncoding.EncodeToString([]byte("limit=100&active=true")), + encodedQueryParam: "cursor", + expectedQueries: gourl.Values{ + "limit": []string{"100"}, + "active": []string{"true"}, + }, + isValid: true, + }, + { + name: "return true for matching expected queries with extraneous queries in pagination url", + nextURL: "/api/example?cursor=" + base64.RawURLEncoding.EncodeToString([]byte("limit=100&active=true&extra=foo")), + encodedQueryParam: "cursor", + expectedQueries: gourl.Values{ + "limit": []string{"100"}, + "active": []string{"true"}, + }, + isValid: true, + }, + { + name: "return false for different limit in pagination url", + nextURL: "/api/example?cursor=" + base64.RawURLEncoding.EncodeToString([]byte("limit=200")), + encodedQueryParam: "cursor", + expectedQueries: gourl.Values{ + "limit": []string{"100"}, + }, + isValid: false, + }, + { + name: "return false for different query among multiple parameters in the pagination url", + nextURL: "/api/example?cursor=" + base64.RawURLEncoding.EncodeToString([]byte("limit=100&active=true")), + encodedQueryParam: "cursor", + expectedQueries: gourl.Values{ + "limit": []string{"100"}, + "active": []string{"false"}, + }, + isValid: false, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(_ *testing.T) { + isValid := IsValidPaginationNextURL(tc.nextURL, tc.encodedQueryParam, tc.expectedQueries) + assert.Equal(tc.isValid, isValid) + }) + } +} diff --git a/common/rest/request_test.go b/common/rest/request_test.go index 8a8833cb..10cbc5da 100644 --- a/common/rest/request_test.go +++ b/common/rest/request_test.go @@ -137,11 +137,13 @@ func TestCachedPaginationNextURL(t *testing.T) { name string paginationURLs []models.PaginationURL offset int + limit int expectedNextURL string }{ { name: "return cached next URL", offset: 200, + limit: 100, paginationURLs: []models.PaginationURL{ { NextURL: "/v2/example.com/stuff?limit=100", @@ -153,6 +155,7 @@ func TestCachedPaginationNextURL(t *testing.T) { { name: "return empty string if cache URL cannot be determined", offset: 40, + limit: 100, paginationURLs: []models.PaginationURL{ { NextURL: "/v2/example.com/stuff?limit=100", @@ -164,9 +167,22 @@ func TestCachedPaginationNextURL(t *testing.T) { { name: "return empty string if no cache available", offset: 40, + limit: 100, paginationURLs: []models.PaginationURL{}, expectedNextURL: "", }, + { + name: "return empty string if limit in url is different than provided limit", + offset: 200, + limit: 200, + paginationURLs: []models.PaginationURL{ + { + NextURL: "/v2/example.com?stuff?limit=100", + LastIndex: 100, + }, + }, + expectedNextURL: "", + }, } assert := assert.New(t) From b67322dea5c6aa97ccabc362c00c8690527d7935 Mon Sep 17 00:00:00 2001 From: Aerex Date: Thu, 16 Nov 2023 15:29:46 -0600 Subject: [PATCH 10/11] test: removed unused test --- common/rest/request_test.go | 16 ---------------- 1 file changed, 16 deletions(-) diff --git a/common/rest/request_test.go b/common/rest/request_test.go index 10cbc5da..8a8833cb 100644 --- a/common/rest/request_test.go +++ b/common/rest/request_test.go @@ -137,13 +137,11 @@ func TestCachedPaginationNextURL(t *testing.T) { name string paginationURLs []models.PaginationURL offset int - limit int expectedNextURL string }{ { name: "return cached next URL", offset: 200, - limit: 100, paginationURLs: []models.PaginationURL{ { NextURL: "/v2/example.com/stuff?limit=100", @@ -155,7 +153,6 @@ func TestCachedPaginationNextURL(t *testing.T) { { name: "return empty string if cache URL cannot be determined", offset: 40, - limit: 100, paginationURLs: []models.PaginationURL{ { NextURL: "/v2/example.com/stuff?limit=100", @@ -167,22 +164,9 @@ func TestCachedPaginationNextURL(t *testing.T) { { name: "return empty string if no cache available", offset: 40, - limit: 100, paginationURLs: []models.PaginationURL{}, expectedNextURL: "", }, - { - name: "return empty string if limit in url is different than provided limit", - offset: 200, - limit: 200, - paginationURLs: []models.PaginationURL{ - { - NextURL: "/v2/example.com?stuff?limit=100", - LastIndex: 100, - }, - }, - expectedNextURL: "", - }, } assert := assert.New(t) From 5f4cf80044481e4f94784fc16cf0d22651eaf4a2 Mon Sep 17 00:00:00 2001 From: Aerex Date: Thu, 30 Nov 2023 13:05:03 -0600 Subject: [PATCH 11/11] chore: bump version to 1.2.0 --- bluemix/version.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bluemix/version.go b/bluemix/version.go index 0d25b84e..0c17f777 100644 --- a/bluemix/version.go +++ b/bluemix/version.go @@ -3,7 +3,7 @@ package bluemix import "fmt" // Version is the SDK version -var Version = VersionType{Major: 1, Minor: 1, Build: 2} +var Version = VersionType{Major: 1, Minor: 2, Build: 0} // VersionType describe version info type VersionType struct {