Add pagination for listing KV worker namespaces by jbergknoff-rival · Pull Request #394 · cloudflare/cloudflare-go

jbergknoff-rival · 2019-12-11T16:31:57Z

Description

Listing KV worker namespaces only brings back the first page of (20) results. While that's the default quota for a CF account, it is possible to have more than 20 namespaces, in which case the behavior is inadequate. We're seeing issues with this downstream in the Cloudflare Terraform provider, which is incorrectly trying to create a namespace that already exists (on the second page of results).

This PR updates ListWorkersKVNamespaces to return all namespaces by paging through the result set. This was modeled after the work done in ~~#288 and #377~~ #389 .

This changes the signature of the ListWorkersKVNamespaces function. Does that require a change to the docs? Where would that be made?

Types of changes

What sort of change does your code introduce/modify?

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change) (changes the signature of ListWorkersKVNamespaces)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have added tests to cover my changes.
All new and existing tests passed.

jbergknoff-rival · 2019-12-11T16:32:23Z

 // API reference: https://api.cloudflare.com/#workers-kv-namespace-list-namespaces
 func (api *API) ListWorkersKVNamespaces(ctx context.Context) (ListWorkersKVNamespacesResponse, error) {
-	uri := fmt.Sprintf("/accounts/%s/storage/kv/namespaces", api.AccountID)
+	uri := fmt.Sprintf("/accounts/%s/storage/kv/namespaces?per_page=100", api.AccountID)


Per #377, increasing this to reduce the number of API calls made.

I'm fine with this for now but if we get any more query parameters, we are going to need to swap to using url.Values to ensure we don't thrash the others.

jbergknoff-rival · 2019-12-11T16:34:27Z

-	result := ListWorkersKVNamespacesResponse{}
-	if err := json.Unmarshal(res, &result); err != nil {
-		return result, errors.Wrap(err, errUnmarshalError)
+	overallResult := ListWorkersKVNamespacesResponse{}


This function returns a ListWorkersKVNamespacesResponse as opposed to, for example, []WorkersKVNamespace. This makes it awkward to return the full list. In the current changeset, I'm hijacking the first ListWorkersKVNamespacesResponse and augmenting its Result list, but leaving ResultInfo and Response alone.

Would it make more sense to move the paging into a separate method which returns a []WorkersKVNamespace?

Yeah, I would expect this to match the other methods like it and return []WorkersKVNamespace. I'm not sure if we have use for the ListWorkersKVNamespacesResponse as is.

Would it make sense to have that breaking change in this PR?

jbergknoff-rival · 2019-12-11T16:35:51Z

+	wg.Add(totalPageCount - 1)
+	errc := make(chan error)
+
+	for i := 2; i <= totalPageCount; i++ {


In #288, we seem to duplicate the request for the first page of zones. Maybe there's a reason for that? Here, we don't duplicate the request, so I'm calling that out.

If I recall, it was for requests that only had a single page which wasn't getting returned otherwise. Memory is fuzzy though. An approach like #389 might be good to follow here. We can get #288 updated in another PR.

jbergknoff-rival · 2019-12-11T16:36:56Z

+
+			select {
+			case err := <-errc:
+				return err


This error handling is modeled on #288. I don't do much work in golang, so please advise if this is inadequate. I'm not sure that these errors are actually being consumed in any way.

jbergknoff-rival · 2019-12-11T16:37:42Z

+
+	wg.Wait()
+
+	return overallResult, nil


Previously was doing return result, err. To my eye, any error would have been returned prior to reaching that point, so this returns nil for the error. Is that a mistake?

jacobbednarz · 2019-12-12T02:13:36Z

+
+	for i := 2; i <= totalPageCount; i++ {
+		go func(pageNumber int) error {
+			res, err = api.makeRequestContext(ctx, http.MethodGet, fmt.Sprintf("%s&page=%d", uri, pageNumber), nil)


I don't think we really need the makeRequestContext here and makerequest should suffice.

jbergknoff-rival

Thanks for the comments, @jacobbednarz. Please see if these changes bring the PR closer to what you had in mind. I prefer the new implementation, though I wonder about whether it's alright to make the breaking change to the function's signature.

jbergknoff-rival · 2019-12-12T15:27:25Z

-	if err != nil {
-		return ListWorkersKVNamespacesResponse{}, errors.Wrap(err, errMakeRequestError)
-	}
+func (api *API) ListWorkersKVNamespaces() ([]WorkersKVNamespace, error) {


I've changed the signature of this function, which is a breaking change for the library, after the discussion on the first iteration of the PR. Is this acceptable? Now this function is a bit unique in workers_kv.go, but it's a cleaner implementation than my first attempt, and, IMO, more useful functionality for most users of the library.

I'd keep context, reasoning in comment below. Changing return type is fine.

jbergknoff-rival · 2019-12-12T15:30:02Z

+	for {
+		v.Set("page", strconv.Itoa(page))
+		uri := fmt.Sprintf("/accounts/%s/storage/kv/namespaces?%s", api.AccountID, v.Encode())
+		res, err := api.makeRequest(http.MethodGet, uri, nil)


I've replaced api.makeRequestContext by api.makeRequest per the comments on the first version of this PR. I also dropped context as an argument to this function.

I would keep context here, we might want to eventually make all API calls context-aware (issue #324).

patryk · 2020-01-02T23:46:48Z

+	for {
+		v.Set("page", strconv.Itoa(page))
+		uri := fmt.Sprintf("/accounts/%s/storage/kv/namespaces?%s", api.AccountID, v.Encode())
+		res, err := api.makeRequest(http.MethodGet, uri, nil)


I would keep context here, we might want to eventually make all API calls context-aware (issue #324).

patryk · 2020-01-02T23:48:06Z

-	if err != nil {
-		return ListWorkersKVNamespacesResponse{}, errors.Wrap(err, errMakeRequestError)
-	}
+func (api *API) ListWorkersKVNamespaces() ([]WorkersKVNamespace, error) {


I'd keep context, reasoning in comment below. Changing return type is fine.

jbergknoff-rival · 2020-01-03T00:15:33Z

Thanks for taking a look, @patryk. I've reinstated the request context.

patryk · 2020-01-20T18:45:10Z

Thanks!

Add pagination for listing KV worker namespaces

d59fac6

jbergknoff-rival commented Dec 11, 2019

View reviewed changes

jacobbednarz reviewed Dec 12, 2019

View reviewed changes

Updates in response to code review

f0bd6f6

jbergknoff-rival commented Dec 12, 2019

View reviewed changes

patryk suggested changes Jan 2, 2020

View reviewed changes

Reinstate request context

ec3d467

patryk approved these changes Jan 20, 2020

View reviewed changes

patryk merged commit 4a4622e into cloudflare:master Jan 20, 2020

jbergknoff-rival deleted the jbergknoff/kv-namespace-pagination branch January 21, 2020 15:05

Conversation

jbergknoff-rival commented Dec 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Types of changes

Checklist:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jbergknoff-rival Dec 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jbergknoff-rival left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jbergknoff-rival Dec 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jbergknoff-rival commented Jan 3, 2020

Uh oh!

patryk commented Jan 20, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jbergknoff-rival commented Dec 11, 2019 •

edited

Loading

jbergknoff-rival Dec 11, 2019 •

edited

Loading

jbergknoff-rival Dec 12, 2019 •

edited

Loading