From f0f163bcc884a4caa652e4f3d93306ecabf2967e Mon Sep 17 00:00:00 2001 From: Artur Rodrigues Date: Mon, 9 Jan 2023 09:48:12 +0000 Subject: [PATCH 1/6] Match ListDNSRecords per_page value to the API's As outlined in github.com/cloudflare/cloudflare-go/pull/1169, when no pagination options are passed in to ListDNSRecords, autopagination is performed. However, the current PerPage value doesn't match that of the official API. This commit makes both values match. A new test is also implemented that more explicitly tests the auto pagination behaviour. Signed-off-by: Artur Rodrigues --- dns.go | 10 +++++-- dns_test.go | 37 ++++++++++++++++++++++++-- testdata/fixtures/dns/list_page_1.json | 37 ++++++++++++++++++++++++++ testdata/fixtures/dns/list_page_2.json | 37 ++++++++++++++++++++++++++ 4 files changed, 117 insertions(+), 4 deletions(-) create mode 100644 testdata/fixtures/dns/list_page_1.json create mode 100644 testdata/fixtures/dns/list_page_2.json diff --git a/dns.go b/dns.go index 473f9ae7de6..99aec41ded8 100644 --- a/dns.go +++ b/dns.go @@ -89,6 +89,9 @@ type DNSListResponse struct { ResultInfo `json:"result_info"` } +// listDNSRecordsDefaultPageSize represents the default per_page size of the API. +var listDNSRecordsDefaultPageSize int = 100 + // nontransitionalLookup implements the nontransitional processing as specified in // Unicode Technical Standard 46 with almost all checkings off to maximize user freedom. var nontransitionalLookup = idna.New( @@ -152,7 +155,10 @@ func (api *API) CreateDNSRecord(ctx context.Context, rc *ResourceContainer, para return recordResp, nil } -// ListDNSRecords returns a slice of DNS records for the given zone identifier. +// ListDNSRecords returns a slice of DNS records for the given zone +// identifier. If params doesn't include any pagination (ResultInfo) +// options, auto pagination is performed with the default page size +// of 100 records per request. // // API reference: https://api.cloudflare.com/#dns-records-for-a-zone-list-dns-records func (api *API) ListDNSRecords(ctx context.Context, rc *ResourceContainer, params ListDNSRecordsParams) ([]DNSRecord, *ResultInfo, error) { @@ -170,7 +176,7 @@ func (api *API) ListDNSRecords(ctx context.Context, rc *ResourceContainer, param } if params.PerPage < 1 { - params.PerPage = 50 + params.PerPage = listDNSRecordsDefaultPageSize } if params.Page < 1 { diff --git a/dns_test.go b/dns_test.go index 08a55e4eb85..4c2fb94957e 100644 --- a/dns_test.go +++ b/dns_test.go @@ -153,7 +153,7 @@ func TestCreateDNSRecord(t *testing.T) { assert.Equal(t, want, actual) } -func TestDNSRecords(t *testing.T) { +func TestListDNSRecords(t *testing.T) { setup() defer teardown() @@ -242,7 +242,7 @@ func TestDNSRecords(t *testing.T) { assert.Equal(t, want, actual) } -func TestDNSRecordsSearch(t *testing.T) { +func TestListDNSRecordsSearch(t *testing.T) { setup() defer teardown() @@ -341,6 +341,39 @@ func TestDNSRecordsSearch(t *testing.T) { assert.Equal(t, want, actual) } +func TestListDNSRecordsPagination(t *testing.T) { + // change listDNSRecordsDefaultPageSize value to 1 to force pagination + listDNSRecordsDefaultPageSize = 1 + + setup() + defer teardown() + + var page1Called, page2Called bool + handler := func(w http.ResponseWriter, r *http.Request) { + page := r.URL.Query().Get("page") + w.Header().Set("content-type", "application/json") + + var response string + switch page { + case "1": + response = loadFixture("dns", "list_page_1") + page1Called = true + case "2": + response = loadFixture("dns", "list_page_2") + page2Called = true + } + fmt.Fprint(w, response) + } + + mux.HandleFunc("/zones/"+testZoneID+"/dns_records", handler) + + actual, _, err := client.ListDNSRecords(context.Background(), ZoneIdentifier(testZoneID), ListDNSRecordsParams{}) + require.NoError(t, err) + assert.True(t, page1Called) + assert.True(t, page2Called) + assert.Len(t, actual, 2) +} + func TestDNSRecord(t *testing.T) { setup() defer teardown() diff --git a/testdata/fixtures/dns/list_page_1.json b/testdata/fixtures/dns/list_page_1.json new file mode 100644 index 00000000000..15acc55332e --- /dev/null +++ b/testdata/fixtures/dns/list_page_1.json @@ -0,0 +1,37 @@ +{ + "success": true, + "errors": [], + "messages": [], + "result": [ + { + "id": "372e67954025e0ba6aaa6d586b9e0b59", + "type": "A", + "name": "example.com", + "content": "198.51.100.4", + "proxiable": true, + "proxied": true, + "ttl": 120, + "locked": false, + "zone_id": "d56084adb405e0b7e32c52321bf07be6", + "zone_name": "example.com", + "created_on": "2014-01-01T05:20:00Z", + "modified_on": "2014-01-01T05:20:00Z", + "data": {}, + "meta": { + "auto_added": true, + "source": "primary" + }, + "tags": [ + "tag1", + "tag2extended" + ] + } + ], + "result_info": { + "count": 1, + "page": 1, + "per_page": 1, + "total_count": 2, + "total_pages": 2 + } +} diff --git a/testdata/fixtures/dns/list_page_2.json b/testdata/fixtures/dns/list_page_2.json new file mode 100644 index 00000000000..b7836c01c36 --- /dev/null +++ b/testdata/fixtures/dns/list_page_2.json @@ -0,0 +1,37 @@ +{ + "success": true, + "errors": [], + "messages": [], + "result": [ + { + "id": "372e67954025e0ba6aaa6d586b9e0b59", + "type": "A", + "name": "www.example.com", + "content": "198.51.100.4", + "proxiable": true, + "proxied": true, + "ttl": 120, + "locked": false, + "zone_id": "d56084adb405e0b7e32c52321bf07be6", + "zone_name": "example.com", + "created_on": "2014-01-01T05:20:00Z", + "modified_on": "2014-01-01T05:20:00Z", + "data": {}, + "meta": { + "auto_added": true, + "source": "primary" + }, + "tags": [ + "tag1", + "tag2extended" + ] + } + ], + "result_info": { + "count": 1, + "page": 2, + "per_page": 1, + "total_count": 2, + "total_pages": 2 + } +} From ff35c3cb230279bdce8ec157112155a9668e0dfe Mon Sep 17 00:00:00 2001 From: Artur Rodrigues Date: Mon, 9 Jan 2023 10:01:59 +0000 Subject: [PATCH 2/6] Add changelog entry Signed-off-by: Artur Rodrigues --- .changelog/1171.txt | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 .changelog/1171.txt diff --git a/.changelog/1171.txt b/.changelog/1171.txt new file mode 100644 index 00000000000..020c63e9775 --- /dev/null +++ b/.changelog/1171.txt @@ -0,0 +1,3 @@ +```release-note:enhancement +dns: Match ListDNSRecords per_page value to the API's (100) +``` From 99374ea5d23b4d061878202a86d628a069c23209 Mon Sep 17 00:00:00 2001 From: Jacob Bednarz Date: Tue, 10 Jan 2023 14:09:06 +1100 Subject: [PATCH 3/6] comments cleanup --- .changelog/1171.txt | 2 +- dns.go | 4 +--- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/.changelog/1171.txt b/.changelog/1171.txt index 020c63e9775..174071f005a 100644 --- a/.changelog/1171.txt +++ b/.changelog/1171.txt @@ -1,3 +1,3 @@ ```release-note:enhancement -dns: Match ListDNSRecords per_page value to the API's (100) +dns: update default `per_page` attribute to 100 records ``` diff --git a/dns.go b/dns.go index 99aec41ded8..7f86733a973 100644 --- a/dns.go +++ b/dns.go @@ -156,9 +156,7 @@ func (api *API) CreateDNSRecord(ctx context.Context, rc *ResourceContainer, para } // ListDNSRecords returns a slice of DNS records for the given zone -// identifier. If params doesn't include any pagination (ResultInfo) -// options, auto pagination is performed with the default page size -// of 100 records per request. +// identifier. // // API reference: https://api.cloudflare.com/#dns-records-for-a-zone-list-dns-records func (api *API) ListDNSRecords(ctx context.Context, rc *ResourceContainer, params ListDNSRecordsParams) ([]DNSRecord, *ResultInfo, error) { From cf51b38ad2e7402bf872d3513738d18029e5c2c8 Mon Sep 17 00:00:00 2001 From: Artur Rodrigues Date: Tue, 10 Jan 2023 08:55:56 +0000 Subject: [PATCH 4/6] dns_test: default case for unexpected pages As per @favonia suggestion --- dns_test.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/dns_test.go b/dns_test.go index 4c2fb94957e..8e81159e71d 100644 --- a/dns_test.go +++ b/dns_test.go @@ -361,6 +361,9 @@ func TestListDNSRecordsPagination(t *testing.T) { case "2": response = loadFixture("dns", "list_page_2") page2Called = true + default: + assert.Failf(t, "Unexpeted page requested: %s", page) + return } fmt.Fprint(w, response) } From 40468f73361d625db653600274ecca2ff2731b8a Mon Sep 17 00:00:00 2001 From: Artur Rodrigues Date: Tue, 10 Jan 2023 08:57:13 +0000 Subject: [PATCH 5/6] Got fmt --- dns.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dns.go b/dns.go index 7f86733a973..eaad5250cfe 100644 --- a/dns.go +++ b/dns.go @@ -156,7 +156,7 @@ func (api *API) CreateDNSRecord(ctx context.Context, rc *ResourceContainer, para } // ListDNSRecords returns a slice of DNS records for the given zone -// identifier. +// identifier. // // API reference: https://api.cloudflare.com/#dns-records-for-a-zone-list-dns-records func (api *API) ListDNSRecords(ctx context.Context, rc *ResourceContainer, params ListDNSRecordsParams) ([]DNSRecord, *ResultInfo, error) { From f59c0fcdcda5c1511b5b02a72397bb18ac8de786 Mon Sep 17 00:00:00 2001 From: Artur Rodrigues Date: Tue, 10 Jan 2023 09:04:50 +0000 Subject: [PATCH 6/6] dns: Shorten docstring --- dns.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/dns.go b/dns.go index eaad5250cfe..c2e8b886e64 100644 --- a/dns.go +++ b/dns.go @@ -155,8 +155,7 @@ func (api *API) CreateDNSRecord(ctx context.Context, rc *ResourceContainer, para return recordResp, nil } -// ListDNSRecords returns a slice of DNS records for the given zone -// identifier. +// ListDNSRecords returns a slice of DNS records for the given zone identifier. // // API reference: https://api.cloudflare.com/#dns-records-for-a-zone-list-dns-records func (api *API) ListDNSRecords(ctx context.Context, rc *ResourceContainer, params ListDNSRecordsParams) ([]DNSRecord, *ResultInfo, error) {