[PR #2495] [MERGED] Use custom serde visitor to fix store error messages #3086

New issue

Closed

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

kerem commented

2026-03-16 11:24:29 +03:00

Owner

📋 Pull Request Information

Original PR: https://github.com/hickory-dns/hickory-dns/pull/2495
Author: @marcus0x62
Created: 10/7/2024
Status: ✅ Merged
Merged: 10/7/2024
Merged by: @marcus0x62

Base: main ← Head: fix_store_errors

📝 Commits (2)

12eedb2 Use custom serde visitor to fix store error messages
2081900 Merge branch 'main' into fix_store_errors

📊 Changes

2 files changed (+132 additions, -43 deletions)

View changed files

📝 bin/src/hickory-dns.rs (+7 -16)
📝 bin/src/lib.rs (+125 -27)

📄 Description

Background

This change introduces a custom serde visitor to handle single-store and chained-store variants while still producing useful error messages. The chained authority PR used the serde untagged enum representation in order to represent single-store (TOML table, serde map) or chained-store (TOML array-of-tables, serde sequence) variants. Unfortunately, serde is not able to show useful error messages when a syntax error is encountered and untagged enums are being used. For example, misspelling the "type" option in a zone store configuration results in this error message to the user:

1727879524:INFO:hickory_dns:443:loading configuration from: "tests/test-data/test_configs/chained_blocklist.toml"
Error: failed to read config file from "tests/test-data/test_configs/chained_blocklist.toml": toml decode error: TOML parse error at line 38, column 1
   |
38 | [[zones.stores]]
   | ^^^^^^^^^^^^^^^^
data did not match any variant of untagged enum StoreConfigContainer

By using a minimal custom visitor, we can restore the previous, more useful error messages:

Single Store

1728310440:INFO:hickory_dns:431:loading configuration from: "tests/test-data/test_configs/example_recursor.toml"
Error: failed to read config file from "tests/test-data/test_configs/example_recursor.toml": toml decode error: TOML parse error at line 38, column 1
   |
38 | [zones.stores]
   | ^^^^^^^^^^^^^^
missing field `type`

Chained Stores

1728311606:INFO:hickory_dns:431:loading configuration from: "tests/test-data/test_configs/chained_blocklist.toml"
Error: failed to read config file from "tests/test-data/test_configs/chained_blocklist.toml": toml decode error: TOML parse error at line 47, column 1
   |
47 | [[zones.stores]]
   | ^^^^^^^^^^^^^^^^
missing field `type`

A side effect of this change is that StoreConfig is always represented as a Vec, which simplifies a bit of the code that iterates over the stores in bin/src/hickory-dns.rs.

Other Options

File a bug report with the serde team. This has been done by others, and it appears the serde maintainers have no intention of fixing this in serde proper, rejecting multiple PRs that address this issue. See, for example, here
Use one of the work-around crates published by the serde team.
1. Path to Error. This appears to work, but adds an additional crate to our dependency tree.
2. Serde Untagged. Has unacceptable type limitations (it only supports bool, i8, i16, i32, i64, i128, u8, u16, u32, u64, u128, f32, f64, char, string, borrowed_str, bytes, borrowed_bytes, byte_buf, unit, seq, map fields)
Make all stores an array-of-tables. In addition to breaking existing configurations, this seems counterintuitive to me.
Use a different configuration name for single- vs chained-stores: [zones.stores] for single stores and zones.chained_stores for chained stores. This still seems kind of clunky to me, particularly with the existing "stores" being plural for a single-store configuration, but is probably the next-best alternative.

_{🔄 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/2495 **Author:** [@marcus0x62](https://github.com/marcus0x62) **Created:** 10/7/2024 **Status:** ✅ Merged **Merged:** 10/7/2024 **Merged by:** [@marcus0x62](https://github.com/marcus0x62) **Base:** `main` ← **Head:** `fix_store_errors` --- ### 📝 Commits (2) - [`12eedb2`](https://github.com/hickory-dns/hickory-dns/commit/12eedb2b4ae6aa379be3963d51f2c02ef663c043) Use custom serde visitor to fix store error messages - [`2081900`](https://github.com/hickory-dns/hickory-dns/commit/208190042c945d9571ac6151322164b11d74831c) Merge branch 'main' into fix_store_errors ### 📊 Changes **2 files changed** (+132 additions, -43 deletions) <details> <summary>View changed files</summary> 📝 `bin/src/hickory-dns.rs` (+7 -16) 📝 `bin/src/lib.rs` (+125 -27) </details> ### 📄 Description Background ---------- This change introduces a custom serde visitor to handle single-store and chained-store variants while still producing useful error messages. The [chained authority PR](https://github.com/hickory-dns/hickory-dns/pull/2161) used the serde [untagged enum representation](https://serde.rs/enum-representations.html#untagged) in order to represent single-store (TOML table, serde map) or chained-store (TOML array-of-tables, serde sequence) variants. Unfortunately, serde is not able to show useful error messages when a syntax error is encountered and untagged enums are being used. For example, misspelling the "type" option in a zone store configuration results in this error message to the user: ``` 1727879524:INFO:hickory_dns:443:loading configuration from: "tests/test-data/test_configs/chained_blocklist.toml" Error: failed to read config file from "tests/test-data/test_configs/chained_blocklist.toml": toml decode error: TOML parse error at line 38, column 1 | 38 | [[zones.stores]] | ^^^^^^^^^^^^^^^^ data did not match any variant of untagged enum StoreConfigContainer ``` By using a minimal custom visitor, we can restore the previous, more useful error messages: **Single Store** ``` 1728310440:INFO:hickory_dns:431:loading configuration from: "tests/test-data/test_configs/example_recursor.toml" Error: failed to read config file from "tests/test-data/test_configs/example_recursor.toml": toml decode error: TOML parse error at line 38, column 1 | 38 | [zones.stores] | ^^^^^^^^^^^^^^ missing field `type` ``` **Chained Stores** ``` 1728311606:INFO:hickory_dns:431:loading configuration from: "tests/test-data/test_configs/chained_blocklist.toml" Error: failed to read config file from "tests/test-data/test_configs/chained_blocklist.toml": toml decode error: TOML parse error at line 47, column 1 | 47 | [[zones.stores]] | ^^^^^^^^^^^^^^^^ missing field `type` ``` A side effect of this change is that StoreConfig is always represented as a Vec<StoreConfig>, which simplifies a bit of the code that iterates over the stores in bin/src/hickory-dns.rs. Other Options ------------- * File a bug report with the serde team. This has been done by others, and it appears the serde maintainers have no intention of fixing this in serde proper, rejecting multiple PRs that address this issue. See, for example, [here](https://github.com/serde-rs/serde/pull/1544) * Use one of the work-around crates published by the serde team. 1. [Path to Error](https://docs.rs/serde_path_to_error/latest/serde_path_to_error/). This appears to work, but adds an additional crate to our dependency tree. 2. [Serde Untagged](https://docs.rs/serde-untagged/latest/serde_untagged/). Has unacceptable type limitations (it only supports bool, i8, i16, i32, i64, i128, u8, u16, u32, u64, u128, f32, f64, char, string, borrowed_str, bytes, borrowed_bytes, byte_buf, unit, seq, map fields) * Make all stores an array-of-tables. In addition to breaking existing configurations, this seems counterintuitive to me. * Use a different configuration name for single- vs chained-stores: [zones.stores] for single stores and [[zones.chained_stores]] for chained stores. This still seems kind of clunky to me, particularly with the existing "stores" being plural for a single-store configuration, but is probably the next-best alternative. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>