fix!: allow clearing comments from the Go API

[favonia]

Description

Currently, empty DNS record comments will be understood as "unset" or "unspecified", making it impossible to clear the comment of an existing record. This PR fixes that. This is a breaking change, but since comments were introduced only in 0.58.0, chances are almost no applications depend on them yet. Technically, a new type DNSRecordComment was introduced to facilitate the correct marshaling of empty comments. See #1158 for further discussions of the current API behavior.

The field Comment in DNSRecord and CreateDNSRecordParam is of type DNSRecordComment so that the zero value means "empty comment". However, the field comment in UpdateDNSRecordParam and ListDNSRecordsParam is of type *DNSRecordComment (notice the star) so that the zero value means "not filtering / not changing".

One possible design choice is to make the field Comment of CreateDNSRecordParam of type *DNSRecordComment so that the zero value means "not specifying the comment", potentially matching the current behavior and also the design of Proxied field better (which is of type *bool instead of bool). I honestly do not know which one is better. Theoretically, we can and probably should change the type of the field Proxied in both DNSRecord and CreateDNSRecordParam to bool, but that will be out of the scope of this PR.

Tagging @janik-cloudflare.

This PR is incomplete. It still needs more test cases and entries for the Changelog.

Has your change been tested?

Barely, but it is tested at the same level of existing code about record comments. The official API documentation does not specify the expected behavior of the API for empty comments, either. I prefer waiting for people working at Cloudflare to add the test cases that they think are correct. I have manually tested the UpdateDNSRecord against the real Cloudflare server and it seemed to match what I expected.

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)

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.
• This change is using publicly documented (api.cloudflare.com or developers.cloudflare.com) and stable APIs.

[github-actions]

Oops! It looks like no changelog entry is attached to this PR. Please include a release note as described in https://github.com/cloudflare/cloudflare-go/blob/master/docs/changelog-process.md.

Example:

```release-note:TYPE
Release note
```

If you do not require a release note to be included, please add the workflow/skip-changelog-entry label.

[codecov-commenter]

Codecov Report

Merging #1166 (e982cdc) into master (6153c1e) will decrease coverage by 0.14%.
The diff coverage is 45.88%.

@@            Coverage Diff             @@
##           master    #1166      +/-   ##
==========================================
- Coverage   49.40%   49.26%   -0.15%     
==========================================
  Files         127      128       +1     
  Lines       12290    12434     +144     
==========================================
+ Hits         6072     6125      +53     
- Misses       4840     4905      +65     
- Partials     1378     1404      +26     

Impacted Files	Coverage Δ
cloudflare_experimental.go	0.00% <0.00%> (ø)
utils.go	72.72% <ø> (ø)
cloudflare.go	68.37% <14.28%> (-0.34%)	⬇️
mtls_certificates.go	26.59% <26.59%> (ø)
dns.go	63.90% <77.46%> (+0.18%)	⬆️
origin_ca.go	57.26% <90.90%> (-2.09%)	⬇️
email_routing_destination.go	66.66% <100.00%> (+0.41%)	⬆️
email_routing_rules.go	65.64% <100.00%> (+0.26%)	⬆️
filter.go	43.38% <100.00%> (+0.41%)	⬆️
firewall_rules.go	54.38% <100.00%> (+0.40%)	⬆️
... and 5 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[jacobbednarz]

I like the approach of using pointers for two reasons. The first is that it gives us the states we need (no comment aka uninitialised, empty comment and a stringy value). The second is that in a future iteration of the SDK we will be moving all fields in struct to be pointers to account for the first point where we need to differentiate between uninitialised values and zero values (which has bit us a couple of times). This will keep us working towards that path.

I don't think we need a custom type here and in fact, it may make end use harder since we already have StringPtr and String helper methods for this sort of thing.

[favonia]

@jacobbednarz I will update this PR, but I would like to understand the following comment better:

I don't think we need a custom type here

I am happy to be proved wrong but I do not see how *string can work. First, we need omitempty to handle missing fields. Second, we need the following JSON marshaling:

• nil: omitted, by omitempty
• &"": using null---this is the problematic case
• &"non-empty string": as "non-empty string"

I believe for &"" the current marshaler of *string will give "", not null. @janik-cloudflare explicitly said the team in charge of this part of the API will not accept the empty string as null. So, we need a custom JSON marshaler that turns "" into null. I do not see an elegant way to avoid custom types if we want to achieve this. Could you possibly clarify what I might have missed?

[jacobbednarz]

I'll have to check when I'm in front of a computer with this open next. I was sure we've done this elsewhere (likely for edge rules engine) but I'll need to have a dig around.

[jacobbednarz]

i think the bit that is tripping you up here is the omitempty on the pointer. here is an example of how i would envision it working - https://go.dev/play/p/x2z0cYwy2-G.

if the team want to support three states (null/nil, empty and strings) then that needs to possible and we leave out the omitempty on this field and offload the responsibility of setting that to the library consumer. we wouldn't need a custom type as the end user would be responsible for determining exactly what they need to send (which we can support with some inline comments).

[favonia]

@jacobbednarz Thanks, but I am still not convinced by the *string approach. omitempty is needed because the Cloudflare API is using null, NOT empty strings, as empty comments. (See the discussion at #1158 by @janik-cloudflare.) With *string but without omitempty, a library user only has these choices for updating a DNS record:

• nil: clearing the comment
• StringPtr(""): rejected by the server
• StringPtr("non-empty string"): setting the comment to "non-empty string"

None of these choices can keep the existing comment of a record. I think this is worse than not being able to clear comments (the current status).

[jacobbednarz]

i'm also curious how much of an issue this really is (setting the value to an empty string). what would the use case be to set it back to null/nil? would an empty string suffice instead? if not, why is nil/null important?

[favonia]

@jacobbednarz It's either nil/null or a non-empty comment. There's no such thing as "empty-string comments" in the Cloudflare API. Empty strings are not allowed. I already proposed a different API design in #1158, which will avoid all our trouble, but that was rejected. Maybe I did not describe my proposal well. Perhaps you want to take a look at #1158 (comment) and its follow-up? I fully agree that the API could have been redesigned to make *string work, but it's simply not the current API. All the complicated code here is due to the mismatch between the Go idiom and the Cloudflare API.

[jacobbednarz]

taking a further step back here, is this enough of a pattern that we should we swap to a 3rd party JSON package that supports the omitnil tag, like https://github.com/wI2L/jettison? since encoding/json from stdlib is basically frozen - golang/go#22480.

given the benchmarks, I'm tempted to make the swap irrespective of whether we end up using the omitnil.

[favonia]

@jacobbednarz The library looks interesting. omitnil still won't solve our problem because omitempty also only omits nil. For pointer types, they are probably the same, though there are other reasons to adopt the library. I want to repeat that the core issue is that "empty comments" are currently encoded as null in the Cloudflare API, and this encoding does not match any built-in marshaller (including omitnil). *string with omitnil will not allow users to clear the comment of an existing record, and by "clearing", I meant any method to empty or reset the comment.

[jacobbednarz]

another option, in https://github.com/cloudflare/cloudflare-go/blob/master/dns.go#L236, we already fetch the existing record for a couple of conditions so we can potentially extend this for a scenario where we haven't provided an explicit nil or string value to Comment and use the existing value from the API call. this would keep the ability to:

• send null (clearing the comment)
• send a new value to update
• when omitted use the existing value from the record lookup

thoughts?

[favonia]

A record lookup would work, but it is perhaps "inelegant" and the benefits of avoiding custom marshalers are shrinking. I have two objections:

1. According to the official API documentation, if PATCH is used, all parameters are optional, so maybe we should have removed the record lookup instead. In other words, I already have doubts about the lookup, although its deletion is probably outside the scope of this PR.
2. Record comments are new, and 99.9% of the library clients will not use them, and thus we will do the extra lookup almost every time.

[jacobbednarz]

i'm going to close this off and take it internally with the team. we avoid custom unmarshal/marshal logic where possible since it makes debugging issues increasingly difficult when it is included in another struct so adding this change would be behind updating the public API in my books.

the DNS team or myself will provide a PR if/when we need to adjust anything here after we reassess the API and it's integrations in the coming week or so.

thanks for the PR and discussion so far @favonia!

[jacobbednarz]

circling back on this issue via #1194 now that the API allows empty comments to clear it.

[github-actions]

This functionality has been released in v0.60.0.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you!