fix(cloudflare): handle multiple same-name dns records in update (#1342)

john-royal · web-flow · commit c9979d96f6b6 · 2026-02-24T23:06:26.000-05:00
diff --git a/alchemy/src/cloudflare/dns-records.ts b/alchemy/src/cloudflare/dns-records.ts
@@ -6,6 +6,7 @@ import type {
 } from "../dns/record.ts";
 import { Resource } from "../resource.ts";
 import { logger } from "../util/logger.ts";
+import { extractCloudflareResult } from "./api-response.ts";
 import {
   type CloudflareApi,
   type CloudflareApiOptions,
@@ -162,17 +163,16 @@ export const DnsRecords = Resource(
 
     if (this.phase === "update" && this.output?.records) {
       // Get current records to compare with desired state
-      const currentRecords = this.output.records;
-      const desiredRecords = props.records;
+      const currentRecords = deduplicateRecords(this.output.records);
+      const desiredRecords = deduplicateRecords(props.records);
 
       // Find records to delete (exist in current but not in desired)
-      const recordsToDelete = currentRecords.filter(
-        (current) =>
-          !desiredRecords.some(
-            (desired) =>
-              desired.name === current.name && desired.type === current.type,
-          ),
-      );
+      const recordsToDelete: DnsRecord[] = [];
+      for (const [key, record] of currentRecords.entries()) {
+        if (!desiredRecords.has(key)) {
+          recordsToDelete.push(record);
+        }
+      }
 
       // Delete orphaned records
       await Promise.all(
@@ -194,12 +194,9 @@ export const DnsRecords = Resource(
 
       // Update or create records
       const updatedRecords = await Promise.all(
-        desiredRecords.map(async (desired) => {
+        desiredRecords.entries().map(async ([key, desired]) => {
           // Find matching existing record
-          const existing = currentRecords.find(
-            (current) =>
-              current.name === desired.name && current.type === desired.type,
-          );
+          const existing = currentRecords.get(key);
 
           if (existing) {
             // Update if content or other properties changed
@@ -226,46 +223,17 @@ export const DnsRecords = Resource(
     }
 
     // Create new records
-    const uniqueRecords = props.records.reduce(
-      (acc, record) => {
-        // For record types that can have multiple entries with the same name (MX, TXT, NS, etc.),
-        // include content and/or priority in the key to avoid deduplication
-        let key = `${record.name}-${record.type}`;
-
-        // If it's a record type that can have multiple entries with the same name, make the key unique
-        if (["MX", "TXT", "NS", "SRV", "CAA"].includes(record.type)) {
-          // For MX, include priority in the key
-          if (record.type === "MX" || record.type === "SRV") {
-            key = `${key}-${record.priority}-${record.content}`;
-          } else {
-            // For other multi-record types, content is the differentiator
-            key = `${key}-${record.content}`;
-          }
-        }
-
-        acc[key] = record;
-        return acc;
-      },
-      {} as Record<string, DnsRecordProps>,
-    );
+    const uniqueRecords = deduplicateRecords(props.records);
 
     const createdRecords = await Promise.all(
-      Object.values(uniqueRecords).map(async (record) => {
-        // First check if record exists
-        const listResponse = await api.get(
-          `/zones/${zoneId}/dns_records?type=${record.type}&name=${record.name}`,
+      uniqueRecords.entries().map(async ([key, record]) => {
+        const existingRecords = await listRecords(api, zoneId, {
+          name: record.name,
+          type: record.type,
+        });
+        const existingRecord = existingRecords.find(
+          (r) => makeRecordKey(r) === key,
         );
-        if (!listResponse.ok) {
-          throw new Error(
-            `Failed to check existing DNS records: ${listResponse.statusText}`,
-          );
-        }
-
-        const listResult = (await listResponse.json()) as CloudflareResponse<
-          CloudflareDnsRecord[]
-        >;
-        const existingRecord = listResult.result[0];
-
         return createOrUpdateRecord(api, zoneId, record, existingRecord?.id);
       }),
     );
@@ -277,6 +245,35 @@ export const DnsRecords = Resource(
   },
 );
 
+function deduplicateRecords<T extends DnsRecordProps>(
+  records: T[],
+): Map<string, T> {
+  const map = new Map<string, T>();
+  for (const record of records) {
+    map.set(makeRecordKey(record), record);
+  }
+  return map;
+}
+
+function makeRecordKey<T extends DnsRecordProps>(record: T) {
+  // For record types that can have multiple entries with the same name (MX, TXT, NS, etc.),
+  // include content and/or priority in the key to avoid deduplication
+  let key = `${record.name}-${record.type}`;
+
+  // If it's a record type that can have multiple entries with the same name, make the key unique
+  if (["MX", "TXT", "NS", "SRV", "CAA"].includes(record.type)) {
+    // For MX, include priority in the key
+    if (record.type === "MX" || record.type === "SRV") {
+      key = `${key}-${record.priority}-${record.content}`;
+    } else {
+      // For other multi-record types, content is the differentiator
+      key = `${key}-${record.content}`;
+    }
+  }
+
+  return key;
+}
+
 /**
  * Create or update a DNS record
  */
@@ -325,6 +322,29 @@ async function createOrUpdateRecord(
   return convertCloudflareRecord(result.result, zoneId);
 }
 
+export async function listRecords(
+  api: CloudflareApi,
+  zoneId: string,
+  filter: {
+    type?: DnsRecordType;
+    name?: string;
+  } = {},
+): Promise<DnsRecord[]> {
+  const queryParams = new URLSearchParams();
+  if (filter.type) {
+    queryParams.set("type", filter.type);
+  }
+  if (filter.name) {
+    queryParams.set("name", filter.name);
+  }
+  const queryString = queryParams.size > 0 ? `?${queryParams.toString()}` : "";
+  const result = await extractCloudflareResult<CloudflareDnsRecord[]>(
+    `list DNS records for zone ${zoneId}`,
+    api.get(`/zones/${zoneId}/dns_records${queryString}`),
+  );
+  return result.map((record) => convertCloudflareRecord(record, zoneId));
+}
+
 /**
  * Get the record payload for create/update operations
  */
diff --git a/alchemy/test/cloudflare/dns-records.test.ts b/alchemy/test/cloudflare/dns-records.test.ts
@@ -1,7 +1,7 @@
-import { afterAll, describe, expect } from "vitest";
+import { afterAll, assert, describe, expect } from "vitest";
 import { alchemy } from "../../src/alchemy.ts";
 import { createCloudflareApi } from "../../src/cloudflare/api.ts";
-import { DnsRecords } from "../../src/cloudflare/dns-records.ts";
+import { DnsRecords, listRecords } from "../../src/cloudflare/dns-records.ts";
 import { Zone } from "../../src/cloudflare/zone.ts";
 import { destroy } from "../../src/destroy.ts";
 import { BRANCH_PREFIX } from "../util.ts";
@@ -15,13 +15,11 @@ const test = alchemy.test(import.meta, {
 
 const testDomain = `${BRANCH_PREFIX}-test-2.com`;
 
-const isEnabled = process.env.ALL_TESTS;
-
+const api = await createCloudflareApi();
 let zone: Zone;
-
 let scope: Scope | undefined;
+
 test.beforeAll(async (_scope) => {
-  if (!isEnabled) return;
   zone = await Zone(`${BRANCH_PREFIX}-zone`, {
     name: testDomain,
   });
@@ -34,9 +32,8 @@ afterAll(async () => {
   }
 });
 
-describe.skipIf(!isEnabled)("DnsRecords Resource", async () => {
+describe.sequential("DnsRecords Resource", async () => {
   // Use BRANCH_PREFIX for deterministic, non-colliding resource names
-  const api = await createCloudflareApi();
 
   test("create, update, and delete DNS records", async (scope) => {
     let dnsRecords;
@@ -73,39 +70,7 @@ describe.skipIf(!isEnabled)("DnsRecords Resource", async () => {
       expect(dnsRecords.records).toHaveLength(3);
 
       // Verify records were created by querying the API directly
-      for (const record of dnsRecords.records) {
-        let response;
-        const start = Date.now();
-        const timeout = 120_000; // 120 seconds
-        const interval = 500; // 0.5 seconds
-        while (true) {
-          response = await api.get(
-            `/zones/${dnsRecords.zoneId}/dns_records/${record.id}`,
-          );
-          if (response.ok) {
-            try {
-              const data: any = await response.json();
-              expect(data.result.name).toBe(record.name);
-              expect(data.result.type).toBe(record.type);
-              expect(data.result.content).toBe(record.content);
-              expect(data.result.proxied).toBe(record.proxied);
-              expect(data.result.comment).toBe(record.comment);
-              if (record.priority) {
-                expect(data.result.priority).toBe(record.priority);
-              }
-              break;
-            } catch (err) {
-              console.error("Error parsing response:", err);
-            }
-          }
-          if (Date.now() - start > timeout) {
-            throw new Error(
-              `DNS record ${record.id} did not become available within 10s`,
-            );
-          }
-          await new Promise((resolve) => setTimeout(resolve, interval));
-        }
-      }
+      await assertRecordsMatch(dnsRecords);
 
       // Update records - modify one record, add one record, remove one record
       dnsRecords = await DnsRecords(`${testDomain}-dns`, {
@@ -162,34 +127,13 @@ describe.skipIf(!isEnabled)("DnsRecords Resource", async () => {
       expect(mailRecord).toBeUndefined();
 
       // Verify directly with API
-      const listResponse = await api.get(
-        `/zones/${dnsRecords.zoneId}/dns_records`,
-      );
-      expect(listResponse.ok).toBe(true);
-
-      const listData: any = await listResponse.json();
-      const apiRecords = listData.result;
-
-      // Should find our 3 records
-      const testRecords = apiRecords.filter((r: any) =>
-        r.name.includes(testDomain),
-      );
-      expect(testRecords).toHaveLength(3);
+      await assertRecordsMatch(dnsRecords);
     } catch (err) {
       console.error("Test failed:", err);
       throw err;
     } finally {
-      // Clean up all resources
       await destroy(scope);
-      // Verify records were deleted
-      if (dnsRecords?.records) {
-        for (const record of dnsRecords.records) {
-          const response = await api.get(
-            `/zones/${dnsRecords.zoneId}/dns_records/${record.id}`,
-          );
-          expect(response.status).toBe(404);
-        }
-      }
+      await assertRecordsDeleted(zone.id);
     }
   });
 
@@ -230,6 +174,78 @@ describe.skipIf(!isEnabled)("DnsRecords Resource", async () => {
       expect(dnsRecords.records[0].content).toBe("192.0.2.2");
     } finally {
       await destroy(scope);
+      await assertRecordsDeleted(zone.id);
+    }
+  });
+
+  test("handles multiple TXT records", async (scope) => {
+    try {
+      let dnsRecords = await DnsRecords(`${testDomain}-txt-dns`, {
+        zoneId: zone.id,
+        records: [
+          {
+            name: `www.${testDomain}`,
+            type: "TXT",
+            content: "v=spf1 include:_spf.google.com ~all",
+          },
+          {
+            name: `www.${testDomain}`,
+            type: "TXT",
+            content: "google-site-verification=1234567890",
+          },
+          {
+            name: `www.${testDomain}`,
+            type: "A",
+            content: "192.0.2.1",
+          },
+        ],
+      });
+      await assertRecordsMatch(dnsRecords);
+
+      dnsRecords = await DnsRecords(`${testDomain}-txt-dns`, {
+        zoneId: zone.id,
+        records: [
+          {
+            name: `www.${testDomain}`,
+            type: "TXT",
+            content: "v=spf1 include:_spf.google.com ~all",
+          },
+          {
+            name: `www.${testDomain}`,
+            type: "TXT",
+            content: "google-site-verification=1234567890",
+          },
+          {
+            name: `www.${testDomain}`,
+            type: "A",
+            content: "192.0.2.2",
+          },
+        ],
+      });
+
+      await assertRecordsMatch(dnsRecords);
+    } finally {
+      await destroy(scope);
+      await assertRecordsDeleted(zone.id);
     }
   });
 });
+
+async function assertRecordsMatch(dnsRecords: DnsRecords) {
+  const apiRecords = await listRecords(api, dnsRecords.zoneId);
+  expect(apiRecords).toHaveLength(dnsRecords.records.length);
+  for (const record of dnsRecords.records) {
+    const apiRecord = apiRecords.find((r) => r.id === record.id);
+    assert(apiRecord, `Record ${record.name} (${record.id}) not found in API`);
+    expect(apiRecord.name).toBe(record.name);
+    expect(apiRecord.type).toBe(record.type);
+    expect(apiRecord.content).toBe(record.content);
+    expect(apiRecord.proxied).toBe(record.proxied);
+    expect(apiRecord.comment).toBe(record.comment);
+  }
+}
+
+async function assertRecordsDeleted(zoneId: string) {
+  const apiRecords = await listRecords(api, zoneId);
+  expect(apiRecords).toHaveLength(0);
+}
