[PR #1919] [CLOSED] Make the Name API more principled #2697

New issue

Closed

opened 2026-03-16 11:03:18 +03:00 by kerem · 0 comments

kerem commented

2026-03-16 11:03:18 +03:00

Owner

📋 Pull Request Information

Original PR: https://github.com/hickory-dns/hickory-dns/pull/1919
Author: @djc
Created: 4/5/2023
Status: ❌ Closed

Base: main ← Head: name-api

📝 Commits (4)

2ef67ed proto: group Name constructors at the top of the impl block
a741a3f proto: stop deriving Default for Name and LowerName
1dde4d4 proto: deprecate Name::from_utf8() in favor of FromStr impl
cf703c0 proto: parse UTF-8 as ASCII where possible

📊 Changes

7 files changed (+346 additions, -262 deletions)

View changed files

📝 Cargo.lock (+37 -0)
📝 crates/proto/src/rr/domain/label.rs (+24 -10)
📝 crates/proto/src/rr/domain/name.rs (+265 -241)
📝 crates/proto/src/rr/lower_name.rs (+1 -1)
📝 crates/proto/src/rr/rdata/svcb.rs (+6 -5)
📝 crates/resolver/src/async_resolver.rs (+3 -4)
📝 tests/integration-tests/src/mock_client.rs (+10 -1)

📄 Description

So I started to work towards a better API to accomodate the discussion in #1904. Ideally I think we'd want to allow underscore by default in host names, and potentially provide a separate API that does not allow them. So here are some things that I ran into that seem to have surprising error potential:

Name implements Default, and Default yields an empty Name that is not root (is_fqdn is false). This seems like a weird default value that's probably not valid in most cases? My second commit here removes the Default impl for Name and LowerName instead.
Name has a FromStr impl and several implementations for the local IntoName trait, but FromStr relies on Name::from_str_relaxed() while the IntoName impls rely on from_utf8(). I think a reasonably expectation would be that FromStr exactly calls from_utf8(), so I implemented that and deprecated Name::from_utf8() in favor of the FromStr impl for good measure, which seems like a cleaner API. I think maybe the IntoName trait predates TryFrom and could be removed in favor of TryFrom impls these days?
I noticed that from_ascii() and from_utf8() have quite different behavior. Label::from_utf8() switches to calling Label::from_ascii() when the first character of the label is _. However, Name::from_utf8() and Name::from_ascii() have the documented behavioral difference that from_utf8() always normalizes to lowercase whereas from_ascii() preserves case. This seems like very surprising behavior -- IMO from_ascii() should also normalize to lowercase. What do you think?

cc @nrempel @cpu

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/hickory-dns/hickory-dns/pull/1919 **Author:** [@djc](https://github.com/djc) **Created:** 4/5/2023 **Status:** ❌ Closed **Base:** `main` ← **Head:** `name-api` --- ### 📝 Commits (4) - [`2ef67ed`](https://github.com/hickory-dns/hickory-dns/commit/2ef67ed67f5c8b6129b1ce9bd0f7ea859db669bc) proto: group Name constructors at the top of the impl block - [`a741a3f`](https://github.com/hickory-dns/hickory-dns/commit/a741a3ff702db3b76d5e5e16d1bb7d38fdebab83) proto: stop deriving Default for Name and LowerName - [`1dde4d4`](https://github.com/hickory-dns/hickory-dns/commit/1dde4d4687724e782c5226c5f54f5ffe4c425181) proto: deprecate Name::from_utf8() in favor of FromStr impl - [`cf703c0`](https://github.com/hickory-dns/hickory-dns/commit/cf703c0b7df13b80bf2b88bcc6c5527a3c8f1065) proto: parse UTF-8 as ASCII where possible ### 📊 Changes **7 files changed** (+346 additions, -262 deletions) <details> <summary>View changed files</summary> 📝 `Cargo.lock` (+37 -0) 📝 `crates/proto/src/rr/domain/label.rs` (+24 -10) 📝 `crates/proto/src/rr/domain/name.rs` (+265 -241) 📝 `crates/proto/src/rr/lower_name.rs` (+1 -1) 📝 `crates/proto/src/rr/rdata/svcb.rs` (+6 -5) 📝 `crates/resolver/src/async_resolver.rs` (+3 -4) 📝 `tests/integration-tests/src/mock_client.rs` (+10 -1) </details> ### 📄 Description So I started to work towards a better API to accomodate the discussion in #1904. Ideally I think we'd want to allow underscore by default in host names, and potentially provide a separate API that does not allow them. So here are some things that I ran into that seem to have surprising error potential: * `Name` implements `Default`, and `Default` yields an empty `Name` that is not `root` (`is_fqdn` is `false`). This seems like a weird default value that's probably not valid in most cases? My second commit here removes the `Default` impl for `Name` and `LowerName` instead. * `Name` has a `FromStr` impl and several implementations for the local `IntoName` trait, but `FromStr` relies on `Name::from_str_relaxed()` while the `IntoName` impls rely on `from_utf8()`. I think a reasonably expectation would be that `FromStr` exactly calls `from_utf8()`, so I implemented that and deprecated `Name::from_utf8()` in favor of the `FromStr` impl for good measure, which seems like a cleaner API. I think maybe the `IntoName` trait predates `TryFrom` and could be removed in favor of `TryFrom` impls these days? * I noticed that `from_ascii()` and `from_utf8()` have quite different behavior. `Label::from_utf8()` switches to calling `Label::from_ascii()` when the first character of the label is `_`. However, `Name::from_utf8()` and `Name::from_ascii()` have the documented behavioral difference that `from_utf8()` always normalizes to lowercase whereas `from_ascii()` preserves case. This seems like very surprising behavior -- IMO `from_ascii()` should also normalize to lowercase. What do you think? cc @nrempel @cpu --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>