Repository: favonia/cloudflare-ddns (go)

Issue:
Consistently unquote record IDs
There's some inconsistency among different messages. Sometimes it's `(ID: "id")` sometimes it's `(ID: id)`

Pull request:
fix(setter): do not quote DNS record IDs
Close #841.

Patch:
diff --git a/docs/DESIGN.markdown b/docs/DESIGN.markdown
index c6038aa..3c608eb 100644
--- a/docs/DESIGN.markdown
+++ b/docs/DESIGN.markdown
@@ -29,6 +29,10 @@ The source code follows the [standard Go project layout](https://github.com/gola
 
 See [Issues](https://github.com/favonia/cloudflare-ddns/issues) and [Milestones](https://github.com/favonia/cloudflare-ddns/milestones).
 
+### Logging Message Convention
+
+1. Cloudflare IDs (zone IDs, DNS record IDs, etc.) are already designed to use only “very safe” characters and should not be quoted. The formatter `%s` should be used instead of `%q`.
+
 ## Network Security Threat Model
 
 ### Assumptions
diff --git a/internal/setter/setter.go b/internal/setter/setter.go
index 42737ed..5b630f6 100644
--- a/internal/setter/setter.go
+++ b/internal/setter/setter.go
@@ -117,7 +117,7 @@ func (s setter) Set(ctx context.Context, ppfmt pp.PP,
 				//
 				// Note that there can still be stale records at this point.
 				ppfmt.Noticef(pp.EmojiUpdate,
-					"Updated a stale %s record of %q (ID: %q)",
+					"Updated a stale %s record of %q (ID: %s)",
 					recordType, domainDescription, id)
 
 				// Now it's up to date! Note that unprocessedMatched must be empty
@@ -135,7 +135,7 @@ func (s setter) Set(ctx context.Context, ppfmt pp.PP,
 			// If the updating fails, we will delete it.
 			if s.Handle.DeleteRecord(ctx, ppfmt, domain, ipnet, id) {
 				ppfmt.Noticef(pp.EmojiDeletion,
-					"Deleted a stale %s record of %q (ID: %q)",
+					"Deleted a stale %s record of %q (ID: %s)",
 					recordType, domainDescription, id)
 
 				// Only when the deletion succeeds, we decrease the counter of remaining stale records.
@@ -156,7 +156,7 @@ func (s setter) Set(ctx context.Context, ppfmt pp.PP,
 		if id, ok := s.Handle.CreateRecord(ctx, ppfmt,
 			domain, ipnet, ip, ttl, proxied, recordComment); ok {
 			ppfmt.Noticef(pp.EmojiCreation,
-				"Added a new %s record of %q (ID: %q)",
+				"Added a new %s record of %q (ID: %s)",
 				recordType, domainDescription, id)
 
 			// Now it's up to date! unprocessedMatched and unprocessedUnmatched
@@ -171,7 +171,7 @@ func (s setter) Set(ctx context.Context, ppfmt pp.PP,
 	for _, id := range unprocessedUnmatched {
 		if s.Handle.DeleteRecord(ctx, ppfmt, domain, ipnet, id) {
 			ppfmt.Noticef(pp.EmojiDeletion,
-				"Deleted a stale %s record of %q (ID: %q)",
+				"Deleted a stale %s record of %q (ID: %s)",
 				recordType, domainDescription, id)
 			numUndeletedUnmatched--
 		} else if ctx.Err() != nil {
@@ -184,7 +184,7 @@ func (s setter) Set(ctx context.Context, ppfmt pp.PP,
 	for _, id := range unprocessedMatched {
 		if s.Handle.DeleteRecord(ctx, ppfmt, domain, ipnet, id) {
 			ppfmt.Noticef(pp.EmojiDeletion,
-				"Deleted a duplicate %s record of %q (ID: %q)",
+				"Deleted a duplicate %s record of %q (ID: %s)",
 				recordType, domainDescription, id)
 		} else if ctx.Err() != nil {
 			goto timeout
@@ -244,7 +244,7 @@ func (s setter) Delete(ctx context.Context, ppfmt pp.PP, domain domain.Domain, i
 			continue
 		}
 
-		ppfmt.Noticef(pp.EmojiDeletion, "Deleted a stale %s record of %q (ID: %q)", recordType, domainDescription, id)
+		ppfmt.Noticef(pp.EmojiDeletion, "Deleted a stale %s record of %q (ID: %s)", recordType, domainDescription, id)
 	}
 	if !allOk {
 		ppfmt.Warningf(pp.EmojiError,


Test patch:
diff --git a/internal/setter/setter_test.go b/internal/setter/setter_test.go
index 2f06e06..8756063 100644
--- a/internal/setter/setter_test.go
+++ b/internal/setter/setter_test.go
@@ -94,7 +94,7 @@ func TestSet(t *testing.T) {
 				gomock.InOrder(
 					h.EXPECT().ListRecords(ctx, p, domain, ipNetwork).Return(map[string]netip.Addr{}, true, true),
 					h.EXPECT().CreateRecord(ctx, p, domain, ipNetwork, ip1, api.TTLAuto, false, "hello").Return(record1, true),
-					p.EXPECT().Noticef(pp.EmojiCreation, "Added a new %s record of %q (ID: %q)", "AAAA", "sub.test.org", record1),
+					p.EXPECT().Noticef(pp.EmojiCreation, "Added a new %s record of %q (ID: %s)", "AAAA", "sub.test.org", record1),
 				)
 			},
 		},
@@ -106,7 +106,7 @@ func TestSet(t *testing.T) {
 					h.EXPECT().ListRecords(ctx, p, domain, ipNetwork).Return(map[string]netip.Addr{record1: ip2}, true, true),
 					h.EXPECT().UpdateRecord(ctx, p, domain, ipNetwork, record1, ip1).Return(true),
 					p.EXPECT().Noticef(pp.EmojiUpdate,
-						"Updated a stale %s record of %q (ID: %q)",
+						"Updated a stale %s record of %q (ID: %s)",
 						"AAAA",
 						"sub.test.org",
 						record1,
@@ -122,9 +122,9 @@ func TestSet(t *testing.T) {
 					h.EXPECT().ListRecords(ctx, p, domain, ipNetwork).Return(map[string]netip.Addr{record1: ip2}, true, true),
 					h.EXPECT().UpdateRecord(ctx, p, domain, ipNetwork, record1, ip1).Return(false),
 					h.EXPECT().DeleteRecord(ctx, p, domain, ipNetwork, record1).Return(true),
-					p.EXPECT().Noticef(pp.EmojiDeletion, "Deleted a stale %s record of %q (ID: %q)", "AAAA", "sub.test.org", record1), //nolint:lll
+					p.EXPECT().Noticef(pp.EmojiDeletion, "Deleted a stale %s record of %q (ID: %s)", "AAAA", "sub.test.org", record1), //nolint:lll
 					h.EXPECT().CreateRecord(ctx, p, domain, ipNetwork, ip1, api.TTLAuto, false, "hello").Return(record2, true),
-					p.EXPECT().Noticef(pp.EmojiCreation, "Added a new %s record of %q (ID: %q)", "AAAA", "sub.test.org", record2),
+					p.EXPECT().Noticef(pp.EmojiCreation, "Added a new %s record of %q (ID: %s)", "AAAA", "sub.test.org", record2),
 				)
 			},
 		},
@@ -187,7 +187,7 @@ func TestSet(t *testing.T) {
 					h.EXPECT().DeleteRecord(ctx, p, domain, ipNetwork, record2).Return(true),
 					p.EXPECT().Noticef(
 						pp.EmojiDeletion,
-						"Deleted a duplicate %s record of %q (ID: %q)",
+						"Deleted a duplicate %s record of %q (ID: %s)",
 						"AAAA",
 						"sub.test.org",
 						record2,
@@ -229,14 +229,14 @@ func TestSet(t *testing.T) {
 					h.EXPECT().UpdateRecord(ctx, p, domain, ipNetwork, record1, ip1).Return(true),
 					p.EXPECT().Noticef(
 						pp.EmojiUpdate,
-						"Updated a stale %s record of %q (ID: %q)",
+						"Updated a stale %s record of %q (ID: %s)",
 						"AAAA",
 						"sub.test.org",
 						record1,
 					),
 					h.EXPECT().DeleteRecord(ctx, p, domain, ipNetwork, record2).Return(true),
 					p.EXPECT().Noticef(pp.EmojiDeletion,
-						"Deleted a stale %s record of %q (ID: %q)",
+						"Deleted a stale %s record of %q (ID: %s)",
 						"AAAA",
 						"sub.test.org",
 						record2),
@@ -253,7 +253,7 @@ func TestSet(t *testing.T) {
 					h.EXPECT().UpdateRecord(ctx, p, domain, ipNetwork, record1, ip1).Return(true),
 					p.EXPECT().Noticef(
 						pp.EmojiUpdate,
-						"Updated a stale %s record of %q (ID: %q)",
+						"Updated a stale %s record of %q (ID: %s)",
 						"AAAA",
 						"sub.test.org",
 						record1,
@@ -275,14 +275,14 @@ func TestSet(t *testing.T) {
 					h.EXPECT().DeleteRecord(ctx, p, domain, ipNetwork, record1).Return(true),
 					p.EXPECT().Noticef(
 						pp.EmojiDeletion,
-						"Deleted a stale %s record of %q (ID: %q)",
+						"Deleted a stale %s record of %q (ID: %s)",
 						"AAAA",
 						"sub.test.org",
 						record1),
 					h.EXPECT().UpdateRecord(ctx, p, domain, ipNetwork, record2, ip1).Return(true),
 					p.EXPECT().Noticef(
 						pp.EmojiUpdate,
-						"Updated a stale %s record of %q (ID: %q)",
+						"Updated a stale %s record of %q (ID: %s)",
 						"AAAA",
 						"sub.test.org",
 						record2,
@@ -299,12 +299,12 @@ func TestSet(t *testing.T) {
 						Return(map[string]netip.Addr{record1: ip2, record2: ip2}, true, true),
 					h.EXPECT().UpdateRecord(ctx, p, domain, ipNetwork, record1, ip1).Return(false),
 					h.EXPECT().DeleteRecord(ctx, p, domain, ipNetwork, record1).Return(true),
-					p.EXPECT().Noticef(pp.EmojiDeletion, "Deleted a stale %s record of %q (ID: %q)", "AAAA", "sub.test.org", record1), //nolint:lll
+					p.EXPECT().Noticef(pp.EmojiDeletion, "Deleted a stale %s record of %q (ID: %s)", "AAAA", "sub.test.org", record1), //nolint:lll
 					h.EXPECT().UpdateRecord(ctx, p, domain, ipNetwork, record2, ip1).Return(false),
 					h.EXPECT().DeleteRecord(ctx, p, domain, ipNetwork, record2).Return(true),
-					p.EXPECT().Noticef(pp.EmojiDeletion, "Deleted a stale %s record of %q (ID: %q)", "AAAA", "sub.test.org", record2), //nolint:lll
+					p.EXPECT().Noticef(pp.EmojiDeletion, "Deleted a stale %s record of %q (ID: %s)", "AAAA", "sub.test.org", record2), //nolint:lll
 					h.EXPECT().CreateRecord(ctx, p, domain, ipNetwork, ip1, api.TTLAuto, false, "hello").Return(record3, true),
-					p.EXPECT().Noticef(pp.EmojiCreation, "Added a new %s record of %q (ID: %q)", "AAAA", "sub.test.org", record3),
+					p.EXPECT().Noticef(pp.EmojiCreation, "Added a new %s record of %q (ID: %s)", "AAAA", "sub.test.org", record3),
 				)
 			},
 		},
@@ -318,9 +318,9 @@ func TestSet(t *testing.T) {
 					h.EXPECT().DeleteRecord(ctx, p, domain, ipNetwork, record1).Return(false),
 					h.EXPECT().UpdateRecord(ctx, p, domain, ipNetwork, record2, ip1).Return(false),
 					h.EXPECT().DeleteRecord(ctx, p, domain, ipNetwork, record2).Return(true),
-					p.EXPECT().Noticef(pp.EmojiDeletion, "Deleted a stale %s record of %q (ID: %q)", "AAAA", "sub.test.org", record2), //nolint:lll
+					p.EXPECT().Noticef(pp.EmojiDeletion, "Deleted a stale %s record of %q (ID: %s)", "AAAA", "sub.test.org", record2), //nolint:lll
 					h.EXPECT().CreateRecord(ctx, p, domain, ipNetwork, ip1, api.TTLAuto, false, "hello").Return(record3, true),
-					p.EXPECT().Noticef(pp.EmojiCreation, "Added a new %s record of %q (ID: %q)", "AAAA", "sub.test.org", record3),                           //nolint:lll
+					p.EXPECT().Noticef(pp.EmojiCreation, "Added a new %s record of %q (ID: %s)", "AAAA", "sub.test.org", record3),                           //nolint:lll
 					p.EXPECT().Warningf(pp.EmojiError, "Failed to finish updating %s records of %q; records might be inconsistent", "AAAA", "sub.test.org"), //nolint:lll
 				)
 			},
@@ -333,10 +333,10 @@ func TestSet(t *testing.T) {
 					h.EXPECT().ListRecords(ctx, p, domain, ipNetwork).Return(map[string]netip.Addr{record1: ip2, record2: ip2}, true, true), //nolint:lll
 					h.EXPECT().UpdateRecord(ctx, p, domain, ipNetwork, record1, ip1).Return(false),
 					h.EXPECT().DeleteRecord(ctx, p, domain, ipNetwork, record1).Return(true),
-					p.EXPECT().Noticef(pp.EmojiDeletion, "Deleted a stale %s record of %q (ID: %q)", "AAAA", "sub.test.org", record1), //nolint:lll
+					p.EXPECT().Noticef(pp.EmojiDeletion, "Deleted a stale %s record of %q (ID: %s)", "AAAA", "sub.test.org", record1), //nolint:lll
 					h.EXPECT().UpdateRecord(ctx, p, domain, ipNetwork, record2, ip1).Return(false),
 					h.EXPECT().DeleteRecord(ctx, p, domain, ipNetwork, record2).Return(true),
-					p.EXPECT().Noticef(pp.EmojiDeletion, "Deleted a stale %s record of %q (ID: %q)", "AAAA", "sub.test.org", record2), //nolint:lll
+					p.EXPECT().Noticef(pp.EmojiDeletion, "Deleted a stale %s record of %q (ID: %s)", "AAAA", "sub.test.org", record2), //nolint:lll
 					h.EXPECT().CreateRecord(ctx, p, domain, ipNetwork, ip1, api.TTLAuto, false, "hello").Return(record3, false),
 					p.EXPECT().Warningf(pp.EmojiError, "Failed to finish updating %s records of %q; records might be inconsistent", "AAAA", "sub.test.org"), //nolint:lll
 				)
@@ -351,10 +351,10 @@ func TestSet(t *testing.T) {
 						Return(map[string]netip.Addr{record1: ip2, record2: ip2}, true, true),
 					h.EXPECT().UpdateRecord(ctx, p, domain, ipNetwork, record1, ip1).Return(false),
 					h.EXPECT().DeleteRecord(ctx, p, domain, ipNetwork, record1).Return(true),
-					p.EXPECT().Noticef(pp.EmojiDeletion, "Deleted a stale %s record of %q (ID: %q)", "AAAA", "sub.test.org", record1), //nolint:lll
+					p.EXPECT().Noticef(pp.EmojiDeletion, "Deleted a stale %s record of %q (ID: %s)", "AAAA", "sub.test.org", record1), //nolint:lll
 					h.EXPECT().UpdateRecord(ctx, p, domain, ipNetwork, record2, ip1).Return(false),
 					h.EXPECT().DeleteRecord(ctx, p, domain, ipNetwork, record2).Return(true),
-					p.EXPECT().Noticef(pp.EmojiDeletion, "Deleted a stale %s record of %q (ID: %q)", "AAAA", "sub.test.org", record2), //nolint:lll
+					p.EXPECT().Noticef(pp.EmojiDeletion, "Deleted a stale %s record of %q (ID: %s)", "AAAA", "sub.test.org", record2), //nolint:lll
 					h.EXPECT().CreateRecord(ctx, p, domain, ipNetwork, ip1, api.TTLAuto, false, "hello").
 						Do(wrapCancelAsCreate(cancel)).Return(record3, false),
 					p.EXPECT().Infof(pp.EmojiBailingOut, "Operation aborted (%v); bailing out . . .", gomock.Any()),
@@ -435,7 +435,7 @@ func TestDelete(t *testing.T) {
 				gomock.InOrder(
 					h.EXPECT().ListRecords(ctx, p, domain, ipNetwork).Return(map[string]netip.Addr{record1: ip1}, true, true),
 					h.EXPECT().DeleteRecord(ctx, p, domain, ipNetwork, record1).Return(true),
-					p.EXPECT().Noticef(pp.EmojiDeletion, "Deleted a stale %s record of %q (ID: %q)", "AAAA", "sub.test.org", record1), //nolint:lll
+					p.EXPECT().Noticef(pp.EmojiDeletion, "Deleted a stale %s record of %q (ID: %s)", "AAAA", "sub.test.org", record1), //nolint:lll
 				)
 			},
 		},
@@ -467,9 +467,9 @@ func TestDelete(t *testing.T) {
 				gomock.InOrder(
 					h.EXPECT().ListRecords(ctx, p, domain, ipNetwork).Return(map[string]netip.Addr{record1: ip1, record2: invalidIP}, true, true), //nolint:lll
 					h.EXPECT().DeleteRecord(ctx, p, domain, ipNetwork, record1).Return(true),
-					p.EXPECT().Noticef(pp.EmojiDeletion, "Deleted a stale %s record of %q (ID: %q)", "AAAA", "sub.test.org", record1), //nolint:lll
+					p.EXPECT().Noticef(pp.EmojiDeletion, "Deleted a stale %s record of %q (ID: %s)", "AAAA", "sub.test.org", record1), //nolint:lll
 					h.EXPECT().DeleteRecord(ctx, p, domain, ipNetwork, record2).Return(true),
-					p.EXPECT().Noticef(pp.EmojiDeletion, "Deleted a stale %s record of %q (ID: %q)", "AAAA", "sub.test.org", record2), //nolint:lll
+					p.EXPECT().Noticef(pp.EmojiDeletion, "Deleted a stale %s record of %q (ID: %s)", "AAAA", "sub.test.org", record2), //nolint:lll
 				)
 			},
 		},