Address review feedback on TR-10-9 DNS-SD browse strategy

- Use string_enum for dns_sd_browse_mode (nmos::dns_sd_browse_modes::{both,unicast,mdns})
- Add dns_sd_browse_mode example + behaviour table to nmos-cpp-node config.json
- Add expected-behaviour table to settings.h
- Mark unused URI-returning resolve_service overload as deprecated (comment)
- Refresh resolve_service / resolve_service_ doc comments in mdns.h/.cpp

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Author: simonbaren

Development/cmake/NmosCppLibraries.cmake

@@ -1133,6 +1133,7 @@ set(NMOS_CPP_NMOS_HEADERS
     nmos/control_protocol_ws_api.h
     nmos/device_type.h
     nmos/did_sdid.h
+    nmos/dns_sd_browse_mode.h
     nmos/event_type.h
     nmos/events_api.h
     nmos/events_resources.h

Development/nmos-cpp-node/config.json

@@ -85,6 +85,21 @@
     // domain [registry, node]: the domain on which to browse for services or an empty string to use the default domain (specify "local." to explictly select mDNS)
     //"domain": "",
 
+    // dns_sd_browse_mode [node]: DNS-SD browse method per TR-10-9 Section 15
+    //   "both" (default) = unicast DNS first, mDNS fallback if unsuccessful
+    //   "unicast"        = unicast DNS only
+    //   "mdns"           = mDNS only
+    // Expected resolve behaviour for each (mode, domain) combination:
+    //   mode    | domain      | behaviour
+    //   --------+-------------+--------------------
+    //   both    | example.com | unicast -> mdns
+    //   both    | local.      | mdns
+    //   unicast | example.com | unicast
+    //   unicast | local.      | mdns
+    //   mdns    | example.com | mdns
+    //   mdns    | local.      | mdns
+    //"dns_sd_browse_mode": "both",
+
     // host_address/host_addresses [registry, node]: IP addresses used to construct response headers (e.g. 'Link' or 'Location'), and host and URL fields in the data model
     //"host_address": "127.0.0.1",
     //"host_addresses": array-of-ip-address-strings,

Development/nmos/dns_sd_browse_mode.h

@@ -0,0 +1,19 @@
+#ifndef NMOS_DNS_SD_BROWSE_MODE_H
+#define NMOS_DNS_SD_BROWSE_MODE_H
+
+#include "nmos/string_enum.h"
+
+namespace nmos
+{
+    // DNS-SD browse method per TR-10-9 Section 15
+    // Selected via the dns_sd_browse_mode setting in nmos::fields
+    DEFINE_STRING_ENUM(dns_sd_browse_mode)
+    namespace dns_sd_browse_modes
+    {
+        const dns_sd_browse_mode both{ U("both") };       // unicast DNS first, mDNS fallback if unsuccessful
+        const dns_sd_browse_mode unicast{ U("unicast") }; // unicast DNS only
+        const dns_sd_browse_mode mdns{ U("mdns") };       // mDNS only
+    }
+}
+
+#endif

Development/nmos/mdns.cpp

@@ -12,6 +12,7 @@
 #include "cpprest/uri_builder.h"
 #include "mdns/service_advertiser.h"
 #include "mdns/service_discovery.h"
+#include "nmos/dns_sd_browse_mode.h"
 #include "nmos/is09_versions.h"
 #include "nmos/is10_versions.h"
 #include "nmos/random.h"
@@ -600,8 +601,9 @@ namespace nmos
             }
         }
 
-        // helper function for resolving instances of the specified service (API)
-        // with the highest version, highest priority instances at the front, and optionally services with the same priority ordered randomly
+        // Helper function for resolving instances of the specified service (API), returning ((api_version, priority), uri)
+        // tuples so callers can inspect the matched version and priority. Highest version and highest priority first,
+        // and optionally services with the same priority ordered randomly.
         pplx::task<std::list<resolved_service>> resolve_service_(mdns::service_discovery& discovery, const nmos::service_type& service, const std::string& browse_domain, const std::set<nmos::api_version>& api_ver, const std::pair<nmos::service_priority, nmos::service_priority>& priorities, const std::set<nmos::service_protocol>& api_proto, const std::set<bool>& api_auth, bool randomize, const std::chrono::steady_clock::duration& timeout, const pplx::cancellation_token& token)
         {
             const auto absolute_timeout = std::chrono::steady_clock::now() + timeout;
@@ -706,8 +708,10 @@ namespace nmos
             });
         }
 
-        // helper function for resolving instances of the specified service (API)
-        // with the highest version, highest priority instances at the front, and optionally services with the same priority ordered randomly
+        // DEPRECATED: this overload is unused; prefer resolve_service_ which also returns api_ver/priority info.
+        // Helper function for resolving instances of the specified service (API), returning a list of base URIs
+        // (with the API version path appended), highest version and highest priority first, and optionally
+        // services with the same priority ordered randomly.
         pplx::task<std::list<web::uri>> resolve_service(mdns::service_discovery& discovery, const nmos::service_type& service, const std::string& browse_domain, const std::set<nmos::api_version>& api_ver, const std::pair<nmos::service_priority, nmos::service_priority>& priorities, const std::set<nmos::service_protocol>& api_proto, const std::set<bool>& api_auth, bool randomize, const std::chrono::steady_clock::duration& timeout, const pplx::cancellation_token& token)
         {
             return resolve_service_(discovery, service, browse_domain, api_ver, priorities, api_proto, api_auth, randomize, timeout, token).then([](std::list<resolved_service> resolved_services)
@@ -720,12 +724,13 @@ namespace nmos
             });
         }
 
-        // helper function for resolving instances of the specified service (API) based on the specified settings
-        // with the highest version, highest priority instances at the front, and services with the same priority ordered randomly
-        // TR-10-9 Section 15: supports "both" (unicast DNS first, mDNS fallback), "unicast", and "mdns" browse modes
+        // Helper function for resolving instances of the specified service (API) based on the specified settings,
+        // returning a list of base URIs (with the API version path appended), highest version and highest priority
+        // first, services with the same priority ordered randomly.
+        // The browse method is selected by the dns_sd_browse_mode setting per TR-10-9 Section 15
+        // (delegates to resolve_service_, which carries the dual-discovery logic).
         pplx::task<std::list<web::uri>> resolve_service(mdns::service_discovery& discovery, const nmos::service_type& service, const nmos::settings& settings, const pplx::cancellation_token& token)
         {
-            // delegate to resolve_service_ (which has the dual-discovery logic) and transform results
             return resolve_service_(discovery, service, settings, token).then([](std::list<resolved_service> resolved_services)
             {
                 return boost::copy_range<std::list<web::uri>>(resolved_services | boost::adaptors::transformed([](const resolved_service& s)
@@ -735,13 +740,17 @@ namespace nmos
             });
         }
 
-        // helper function for resolving instances of the specified service (API) based on the specified settings
-        // with the highest version, highest priority instances at the front, and services with the same priority ordered randomly
-        // TR-10-9 Section 15: supports "both" (unicast DNS first, mDNS fallback), "unicast", and "mdns" browse modes
+        // Helper function for resolving instances of the specified service (API) based on the specified settings,
+        // returning ((api_version, priority), uri) tuples. Highest version and highest priority first, services
+        // with the same priority ordered randomly.
+        // The browse method is selected by the dns_sd_browse_mode setting per TR-10-9 Section 15:
+        //  - "both" (default): unicast DNS first, mDNS fallback if unsuccessful
+        //  - "unicast"       : unicast DNS only
+        //  - "mdns"          : mDNS only
         pplx::task<std::list<resolved_service>> resolve_service_(mdns::service_discovery& discovery, const nmos::service_type& service, const nmos::settings& settings, const pplx::cancellation_token& token)
         {
             const auto browse_domain = utility::us2s(nmos::get_domain(settings));
-            const auto browse_mode = utility::us2s(nmos::fields::dns_sd_browse_mode(settings));
+            const auto browse_mode = nmos::dns_sd_browse_mode(nmos::fields::dns_sd_browse_mode(settings));
             const auto versions = details::service_versions(service, settings);
             const auto priorities = details::service_priorities(service, settings);
             const auto protocols = std::set<nmos::service_protocol>{ nmos::get_service_protocol(service, settings) };
@@ -753,8 +762,8 @@ namespace nmos
             const auto timeout_dur = std::chrono::duration_cast<std::chrono::steady_clock::duration>(std::chrono::seconds(timeout));
 
             // determine primary browse domain based on mode
-            const auto primary_domain = (browse_mode == "mdns") ? std::string("local.") : browse_domain;
-            const bool has_fallback = (browse_mode == "both") && !is_local_domain(browse_domain);
+            const auto primary_domain = (browse_mode == nmos::dns_sd_browse_modes::mdns) ? std::string("local.") : browse_domain;
+            const bool has_fallback = (browse_mode == nmos::dns_sd_browse_modes::both) && !is_local_domain(browse_domain);
 
             // when there's a fallback, give the primary browse half the budget
             // so the mDNS fallback gets a meaningful allocation

Development/nmos/mdns.h

@@ -157,39 +157,50 @@ namespace nmos
         // helper function for updating the specified service (API) TXT records
         void update_service(mdns::service_advertiser& advertiser, const nmos::service_type& service, const nmos::settings& settings, mdns::structured_txt_records add_records = {});
 
-        // helper function for resolving instances of the specified service (API)
-        // with the highest version, highest priority instances at the front, and (by default) services with the same priority ordered randomly
+        // DEPRECATED: this overload is unused; prefer resolve_service_ which also returns api_ver/priority info.
+        // Helper function for resolving instances of the specified service (API), returning a list of base URIs
+        // (with the API version path appended), highest version and highest priority first, and (by default)
+        // services with the same priority ordered randomly.
         pplx::task<std::list<web::uri>> resolve_service(mdns::service_discovery& discovery, const nmos::service_type& service, const std::string& browse_domain, const std::set<nmos::api_version>& api_ver, const std::pair<nmos::service_priority, nmos::service_priority>& priorities, const std::set<nmos::service_protocol>& api_proto, const std::set<bool>& api_auth, bool randomize, const std::chrono::steady_clock::duration& timeout, const pplx::cancellation_token& token = pplx::cancellation_token::none());
 
-        // helper function for resolving instances of the specified service (API) based on the specified options or defaults
-        // with the highest version, highest priority instances at the front, and (by default) services with the same priority ordered randomly
+        // DEPRECATED: convenience wrapper around the deprecated explicit-args resolve_service overload above.
+        // Same behaviour, with default values for unspecified arguments.
         template <typename Rep = std::chrono::seconds::rep, typename Period = std::chrono::seconds::period>
         inline pplx::task<std::list<web::uri>> resolve_service(mdns::service_discovery& discovery, const nmos::service_type& service, const std::string& browse_domain = {}, const std::set<nmos::api_version>& api_ver = nmos::is04_versions::all, const std::pair<nmos::service_priority, nmos::service_priority>& priorities = { service_priorities::highest_active_priority, service_priorities::no_priority }, const std::set<nmos::service_protocol>& api_proto = nmos::service_protocols::all, const std::set<bool>& api_auth = { false, true }, bool randomize = true, const std::chrono::duration<Rep, Period>& timeout = std::chrono::seconds(mdns::default_timeout_seconds), const pplx::cancellation_token& token = pplx::cancellation_token::none())
         {
             return resolve_service(discovery, service, browse_domain, api_ver, api_proto, api_auth, randomize, std::chrono::duration_cast<std::chrono::steady_clock::duration>(timeout), token);
         }
 
-        // helper function for resolving instances of the specified service (API) based on the specified settings
-        // with the highest version, highest priority instances at the front, and services with the same priority ordered randomly
+        // Helper function for resolving instances of the specified service (API) based on the specified settings,
+        // returning a list of base URIs (with the API version path appended), highest version and highest priority
+        // first, services with the same priority ordered randomly.
+        // The browse method is selected by the dns_sd_browse_mode setting per TR-10-9 Section 15
+        // (delegates to resolve_service_, which carries the dual-discovery logic).
         pplx::task<std::list<web::uri>> resolve_service(mdns::service_discovery& discovery, const nmos::service_type& service, const nmos::settings& settings, const pplx::cancellation_token& token = pplx::cancellation_token::none());
 
         typedef std::pair<api_version, service_priority> api_ver_pri;
         typedef std::pair<api_ver_pri, web::uri> resolved_service;
 
-        // helper function for resolving instances of the specified service (API)
-        // with the highest version, highest priority instances at the front, and (by default) services with the same priority ordered randomly
+        // Helper function for resolving instances of the specified service (API), returning ((api_version, priority), uri)
+        // tuples so callers can inspect the matched version and priority. Highest version and highest priority first,
+        // and (by default) services with the same priority ordered randomly.
         pplx::task<std::list<resolved_service>> resolve_service_(mdns::service_discovery& discovery, const nmos::service_type& service, const std::string& browse_domain, const std::set<nmos::api_version>& api_ver, const std::pair<nmos::service_priority, nmos::service_priority>& priorities, const std::set<nmos::service_protocol>& api_proto, const std::set<bool>& api_auth, bool randomize, const std::chrono::steady_clock::duration& timeout, const pplx::cancellation_token& token = pplx::cancellation_token::none());
 
-        // helper function for resolving instances of the specified service (API) based on the specified options or defaults
-        // with the highest version, highest priority instances at the front, and (by default) services with the same priority ordered randomly
+        // Convenience wrapper around the explicit-args resolve_service_ overload above, with default values
+        // for unspecified arguments.
         template <typename Rep = std::chrono::seconds::rep, typename Period = std::chrono::seconds::period>
         inline pplx::task<std::list<resolved_service>> resolve_service_(mdns::service_discovery& discovery, const nmos::service_type& service, const std::string& browse_domain = {}, const std::set<nmos::api_version>& api_ver = nmos::is04_versions::all, const std::pair<nmos::service_priority, nmos::service_priority>& priorities = { service_priorities::highest_active_priority, service_priorities::no_priority }, const std::set<nmos::service_protocol>& api_proto = nmos::service_protocols::all, const std::set<bool>& api_auth = { false, true }, bool randomize = true, const std::chrono::duration<Rep, Period>& timeout = std::chrono::seconds(mdns::default_timeout_seconds), const pplx::cancellation_token& token = pplx::cancellation_token::none())
         {
             return resolve_service_(discovery, service, browse_domain, api_ver, api_proto, api_auth, randomize, std::chrono::duration_cast<std::chrono::steady_clock::duration>(timeout), token);
         }
 
-        // helper function for resolving instances of the specified service (API) based on the specified settings
-        // with the highest version, highest priority instances at the front, and services with the same priority ordered randomly
+        // Helper function for resolving instances of the specified service (API) based on the specified settings,
+        // returning ((api_version, priority), uri) tuples. Highest version and highest priority first, services
+        // with the same priority ordered randomly.
+        // The browse method is selected by the dns_sd_browse_mode setting per TR-10-9 Section 15:
+        //  - "both" (default): unicast DNS first, mDNS fallback if unsuccessful
+        //  - "unicast"       : unicast DNS only
+        //  - "mdns"          : mDNS only
         pplx::task<std::list<resolved_service>> resolve_service_(mdns::service_discovery& discovery, const nmos::service_type& service, const nmos::settings& settings, const pplx::cancellation_token& token = pplx::cancellation_token::none());
     }
 }

Development/nmos/settings.h

@@ -82,10 +82,20 @@ namespace nmos
         // domain [registry, node]: the domain on which to browse for services or an empty string to use the default domain (specify "local." to explictly select mDNS)
         const web::json::field_as_string_or domain{ U("domain"), U("") };
 
-        // dns_sd_browse_mode [node]: controls DNS-SD browse method per TR-10-9 Section 15
-        // "both" (default) = unicast DNS first, mDNS fallback if unsuccessful
-        // "unicast" = unicast DNS only (requires domain to be set to a non-local value)
-        // "mdns" = mDNS only
+        // dns_sd_browse_mode [node]: DNS-SD browse method per TR-10-9 Section 15
+        // (see nmos::dns_sd_browse_modes for valid values)
+        //   "both" (default) = unicast DNS first, mDNS fallback if unsuccessful
+        //   "unicast"        = unicast DNS only
+        //   "mdns"           = mDNS only
+        // Expected resolve behaviour for each (mode, domain) combination:
+        //   mode    | domain      | behaviour
+        //   --------+-------------+--------------------
+        //   both    | example.com | unicast -> mdns
+        //   both    | local.      | mdns
+        //   unicast | example.com | unicast
+        //   unicast | local.      | mdns
+        //   mdns    | example.com | mdns
+        //   mdns    | local.      | mdns
         const web::json::field_as_string_or dns_sd_browse_mode{ U("dns_sd_browse_mode"), U("both") };
 
         // host_address/host_addresses [registry, node]: IP addresses used to construct response headers (e.g. 'Link' or 'Location'), and host and URL fields in the data model