Skip to content

Conversation

anonyco
Copy link

@anonyco anonyco commented Oct 3, 2025

I put significant effort into ensuring this is a deceptively small change guaranteed to break nothing and cause you no issues:

  • I tested this new version against upstream stalwart and stalwart builds and tests with no additional errors. (Baseline stalwart is currently failing with 20 or 21 errors prior to these changes.)
  • I added unit tests for all the new features I'm adding
  • I carefully documented all changes with descriptive commits and comments in the code
  • I carefully broke up this PR into many bite-size commits, e.g. each of which builds and tests successfully. git bisect away!
  • No publicly exported Rust APIs were changed in a breaking way, e.g. no variables were renamed in the process
  • Only one new dependency—thiserror—as suggested by dns_update::Error does not implement std::error::Error #8. I checked thiserror's source code and, if I were to fix dns_update::Error does not implement std::error::Error #8 without adding thiserror, I'd end up duplicating half of the code in thiserror (and greatly increasing maintenance efforts of dns-update). Also, thiserror pulls in 0 subdependencies. I think it's a pretty unobjectionable decision to add the thiserror crate.
  • I created a github-style squashed merge single commit thing as that seems to be the project style here

Three issues were not addressed by these changes:

I hope this isn't too big of a change, and I hope you see the effort I put in to ensure my changes are as little hassle for you as possible 👍

The commit d5b3b69 fixed the behavior of desec for Stalwart but broke the test and the example.

Desec requires an additional level of JSON quoting for items in arrays of TXT records: https://desec.readthedocs.io/en/latest/dns/rrsets.html#caveats

Also, add copyright to desec and lib tests and add flexibility for future continuous Desec tests via secret environment variables

Signed-off-by: Jack Giffin <[email protected]>
…iders

For hygienic history and time-traveling rebasers

Signed-off-by: Jack Giffin <[email protected]>
This is a necessary change for the next commit, implementing a cache and
 I thought it'd make the history nicer/cleaner to separate this change
 into its own commit.

Signed-off-by: Jack Giffin <[email protected]>
This is a very simple, very basic, yet extraordinarily effective way
 to reduce the numer of API requests to DNS update API servers.

Signed-off-by: Jack Giffin <[email protected]>
Continuation of the previous commit. These commits were split into
 two commits for better history hygiene and organization.

Signed-off-by: Jack Giffin <[email protected]>
This script doesn't do anything you can't do youself;
 really the main reason I wrote it is the non-obvious
 (undocumented?) argument of --all-targets required
 to get `cargo clippy` to actually work. I expect
 ./tool to be very useful for other things as well,
 whatever quick shortcuts people find useful.

Signed-off-by: Jack Giffin <[email protected]>
These conversion methods will be used in the next commit to massively
 debloat and deduplicate the project's providers codes

Signed-off-by: Jack Giffin <[email protected]>
anonyco added a commit to anonyco/dns-update that referenced this pull request Oct 4, 2025
Bump version 0.1.6

Signed-off-by: Jack Giffin <[email protected]>
@mdecimus
Copy link
Member

mdecimus commented Oct 5, 2025

Hi,

Thanks for the PR! A few comments:

  • Remove the caching code, this library is a thin layer for DNS updates. Caching should be done in the application using this library.
  • Please remove the thiserror dependency. We try to keep the number of external dependencies to a minimum.

For upstream users of the dns-update crate

Signed-off-by: Jack Giffin <[email protected]>
Actual consolidation coming up in the next commit

Signed-off-by: Jack Giffin <[email protected]>
Emphasize non-breaking. Great effort went into ensuring
 this is a non-breaking change

Signed-off-by: Jack Giffin <[email protected]>
…oviders

Emphasize non-breaking. Great effort went into ensuring
 this is a non-breaking change

Signed-off-by: Jack Giffin <[email protected]>
Signed-off-by: Jack Giffin <[email protected]>
Includes an integration test on a live real domain

Signed-off-by: Jack Giffin <[email protected]>
Bump version 0.1.6

Signed-off-by: Jack Giffin <[email protected]>
@anonyco
Copy link
Author

anonyco commented Oct 6, 2025

Hi,

Thank you for all the work you do on Stalwart. I removed the thiserror dependency but I'd like to confirm with you if removing the caching is what you really want to do before I remove it:

  1. The application using the library can't cache the temporary domain/record IDs, and that's all my cache code caches. It's still up to the application to cache redundant requests. All my cache does is speed up common transaction patterns where you query a record, update it, and delete it all from the same DnsUpdater object. (Without my cache, each would require up to 2 additional HTTP requests to get the domain and record IDs for the transaction depending on the provider.)
  2. The cache never changes the observed behavior of DnsUpdater; it only caches temporary domain/record IDs. These are unchanging constants for all DNS providers.
  3. Error conditions aren't changed either for any DNS provider; API requests using cached domain/record IDs merely fail at the final transaction to the API server instead of at the upfront API request to fetch the domain/record ID (in the scenario an external actor deletes the domain/record while DnsUpdater is in use.)
  4. The cache costs no overhead/baggage—it adds neither static globals nor additional dynamic memory allocation to DnsUpdater usage
  5. I added unit tests for all the above cases in order to ensure the cache works properly.

I'll remove the cache if that's what you want, it's not my intention to obstruct. I just thought you didn't have the full info on the feature 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Consider reusing hickory_proto::rr::record_data::RData instead of own DnsRecord type dns_update::Error does not implement std::error::Error

2 participants