Merge pull request #1222 from MrMortbury/master

Use new DNSListResponse object when paginating
cloudflare · Feb 28, 2023 · 5a6fa21 · 5a6fa21
2 parents f135761 + b75870f
commit 5a6fa21
Show file tree

Hide file tree

Showing 5 changed files with 126 additions and 14 deletions.
diff --git a/.changelog/1222.txt b/.changelog/1222.txt
@@ -0,0 +1,3 @@
+```release-note:bug
+dns: dont reuse DNSListResponse when using pagination to avoid Proxied pointer overwrite
+```
diff --git a/dns.go b/dns.go
@@ -172,25 +172,27 @@ func (api *API) ListDNSRecords(ctx context.Context, rc *ResourceContainer, param
 	}
 
 	var records []DNSRecord
-	var listResponse DNSListResponse
+	var lastResultInfo ResultInfo
 
 	for {
 		uri := buildURI(fmt.Sprintf("/zones/%s/dns_records", rc.Identifier), params)
 		res, err := api.makeRequestContext(ctx, http.MethodGet, uri, nil)
 		if err != nil {
 			return []DNSRecord{}, &ResultInfo{}, err
 		}
+		var listResponse DNSListResponse
 		err = json.Unmarshal(res, &listResponse)
 		if err != nil {
 			return []DNSRecord{}, &ResultInfo{}, fmt.Errorf("%s: %w", errUnmarshalError, err)
 		}
 		records = append(records, listResponse.Result...)
+		lastResultInfo = listResponse.ResultInfo
 		params.ResultInfo = listResponse.ResultInfo.Next()
 		if params.ResultInfo.Done() || !autoPaginate {
 			break
 		}
 	}
-	return records, &listResponse.ResultInfo, nil
+	return records, &lastResultInfo, nil
 }
 
 // ErrMissingDNSRecordID is for when DNS record ID is needed but not given.

diff --git a/dns_test.go b/dns_test.go
@@ -347,7 +347,7 @@ func TestListDNSRecordsSearch(t *testing.T) {
 
 func TestListDNSRecordsPagination(t *testing.T) {
 	// change listDNSRecordsDefaultPageSize value to 1 to force pagination
-	listDNSRecordsDefaultPageSize = 1
+	listDNSRecordsDefaultPageSize = 3
 
 	setup()
 	defer teardown()
@@ -378,7 +378,45 @@ func TestListDNSRecordsPagination(t *testing.T) {
 	require.NoError(t, err)
 	assert.True(t, page1Called)
 	assert.True(t, page2Called)
-	assert.Len(t, actual, 2)
+	assert.Len(t, actual, 5)
+
+	type ls struct {
+		Results []map[string]interface{} `json:"result"`
+	}
+
+	expectedRecords := make(map[string]map[string]interface{})
+
+	response1 := loadFixture("dns", "list_page_1")
+	var fixtureDataPage1 ls
+	err = json.Unmarshal([]byte(response1), &fixtureDataPage1)
+	assert.NoError(t, err)
+	for _, record := range fixtureDataPage1.Results {
+		expectedRecords[record["id"].(string)] = record
+	}
+
+	response2 := loadFixture("dns", "list_page_2")
+	var fixtureDataPage2 ls
+	err = json.Unmarshal([]byte(response2), &fixtureDataPage2)
+	assert.NoError(t, err)
+	for _, record := range fixtureDataPage2.Results {
+		expectedRecords[record["id"].(string)] = record
+	}
+
+	for _, actualRecord := range actual {
+		expected, exist := expectedRecords[actualRecord.ID]
+		assert.True(t, exist, "DNS record doesn't exist in fixtures")
+		assert.Equal(t, expected["type"].(string), actualRecord.Type)
+		assert.Equal(t, expected["name"].(string), actualRecord.Name)
+		assert.Equal(t, expected["content"].(string), actualRecord.Content)
+		assert.Equal(t, expected["proxiable"].(bool), actualRecord.Proxiable)
+		assert.Equal(t, expected["proxied"].(bool), *actualRecord.Proxied)
+		assert.Equal(t, int(expected["ttl"].(float64)), actualRecord.TTL)
+		assert.Equal(t, expected["locked"].(bool), actualRecord.Locked)
+		assert.Equal(t, expected["zone_id"].(string), actualRecord.ZoneID)
+		assert.Equal(t, expected["zone_name"].(string), actualRecord.ZoneName)
+		assert.Equal(t, expected["data"], actualRecord.Data)
+		assert.Equal(t, expected["meta"], actualRecord.Meta)
+	}
 }
 
 func TestGetDNSRecord(t *testing.T) {

diff --git a/testdata/fixtures/dns/list_page_1.json b/testdata/fixtures/dns/list_page_1.json
@@ -25,13 +25,59 @@
                 "tag1",
                 "tag2extended"
             ]
+        },
+        {
+            "id": "7eb0a9821aec4b1395bd8cc03d88c17d",
+            "type": "A",
+            "name": "sub1.example.com",
+            "content": "198.51.100.5",
+            "proxiable": true,
+            "proxied": false,
+            "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"
+            ]
+        },
+        {
+            "id": "4c2c40857e334a2d903dd28f65a99682",
+            "type": "A",
+            "name": "sub2.example.com",
+            "content": "198.51.100.6",
+            "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,
+        "count": 3,
         "page": 1,
-        "per_page": 1,
-        "total_count": 2,
+        "per_page": 3,
+        "total_count": 5,
         "total_pages": 2
     }
 }
diff --git a/testdata/fixtures/dns/list_page_2.json b/testdata/fixtures/dns/list_page_2.json
@@ -4,12 +4,35 @@
     "messages": [],
     "result": [
         {
-            "id": "372e67954025e0ba6aaa6d586b9e0b59",
+            "id": "97e1dc2d19204b448b6ee04724f005ba",
             "type": "A",
-            "name": "www.example.com",
-            "content": "198.51.100.4",
+            "name": "sub3.example.com",
+            "content": "198.51.100.7",
             "proxiable": true,
-            "proxied": true,
+            "proxied": false,
+            "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"
+            ]
+        },
+        {
+            "id": "5bafaa7059d3480da9f6e2ecd8468c33",
+            "type": "A",
+            "name": "sub4.example.com",
+            "content": "198.51.100.8",
+            "proxiable": true,
+            "proxied": false,
             "ttl": 120,
             "locked": false,
             "zone_id": "d56084adb405e0b7e32c52321bf07be6",
@@ -28,10 +51,10 @@
         }
     ],
     "result_info": {
-        "count": 1,
+        "count": 2,
         "page": 2,
-        "per_page": 1,
-        "total_count": 2,
+        "per_page": 3,
+        "total_count": 5,
         "total_pages": 2
     }
 }