Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ResolvedService: a new plain struct for attributes of a service #302

Merged
merged 6 commits into from
Feb 15, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,9 @@ pub use service_daemon::{
ServiceDaemon, ServiceEvent, UnregisterStatus, SERVICE_NAME_LEN_MAX_DEFAULT,
VERIFY_TIMEOUT_DEFAULT,
};
pub use service_info::{AsIpAddrs, IntoTxtProperties, ServiceInfo, TxtProperties, TxtProperty};
pub use service_info::{
AsIpAddrs, IntoTxtProperties, ResolvedService, ServiceInfo, TxtProperties, TxtProperty,
};

/// A handler to receive messages from [ServiceDaemon]. Re-export from `flume` crate.
pub use flume::Receiver;
54 changes: 54 additions & 0 deletions src/service_info.rs
Original file line number Diff line number Diff line change
Expand Up @@ -366,6 +366,19 @@ impl ServiceInfo {
.cloned()
.unwrap_or(ServiceStatus::Unknown)
}

/// Consumes self and returns a resolved service, i.e. a lite version of `ServiceInfo`.
pub fn as_resolved_service(self) -> ResolvedService {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Do we use Option<ResolvedService> for empty addresses?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have thought about this, and wanted to keep returning just ResolvedService for a couple of reasons:

  1. Better ergonomics for the user. (Having Option can complicate many things for the user, in my experience).
  2. Anyway ResolvedService struct cannot guarantee its fields are all valid, for example what if .fullname is empty? So I think we can implement methods in ResolvedService to valid its content, something like is_valid(), etc.

Copy link

@Luni-4 Luni-4 Feb 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with your points, but I'm a bit confused.

Currently, we retrieve a service client side in this way (just an example with timeout):

while let Ok(event) = receiver.recv_timeout(Duration::from_secs(1)) {
        if let ServiceEvent::ServiceResolved(info) = event {
            info!("{:?}", info);
        }
}

Now we are going to replace info variable, which is a ServiceInfo structure, with ResolvedService. But if there isn't a valid fullname or an empty vector of addresses, can't we block all of this before, when we are evaluating the ServiceEvent::ServiceResolved(info) condition? I mean, we can return a different event variant when the conditions above are not satisfied. Just talking for the client side.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now we are going to replace info variable, which is a ServiceInfo structure, with ResolvedService.

Actually I'm not saying to replace info here. Because such change will break the backward compatibility, we said going with option 1 instead: The user will call info.as_resolved_service to consume info and get a ResolvedService struct.

But if there isn't a valid fullname or an empty vector of addresses, can't we block all of this before, when we are evaluating the ServiceEvent::ServiceResolved(info) condition?

Yes, from this event the info is always valid, which means ResolvedService struct will also be valid in this context. But this is a business logic, we can document it but we cannot enforce it (via the type system or compiler).

But for any given ResolvedService without such context, we would need something like is_valid (or is_ready to match the current code in service_info.rs).

Copy link

@Luni-4 Luni-4 Feb 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I see now, thank you! The changes are fine for me, and the is_valid method is a simpler solution, so let's stick with that

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I've updated the diff with a test case and export in lib.rs. However, I have a new question regarding sub_domain below. Any suggestions or comments?

ResolvedService {
ty_domain: self.ty_domain,
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another question: what if this service info has some sub_domain ? Currently, ServiceInfo stores only the regular service type & domain in ty_domain, and the full subtype & domain in sub_domain. Do we want the same thing in ResolvedService?

Copy link

@Luni-4 Luni-4 Feb 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should make sub_domain public too. If we are consuming ServingInfo, we can transfer it to ResolvedService. We do not know all kinds of use cases out there, perhaps someone needs that information. If it was public and accessible before, in my opinion, it should be accessible even in ResolvedService.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ty_domain and sub_domain was not public, but they are indeed accessible via a public API (getter). I agree with you, I think it's better to include sub_domain in ResolvedService as well.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With public, I meant accessible via public interface :P

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated the diff and added sub_ty_domain. No longer a draft ;-). Let me know if this patch works for you. Thanks!

sub_ty_domain: self.sub_domain,
fullname: self.fullname,
host: self.server,
port: self.port,
addresses: self.addresses,
txt_properties: self.txt_properties,
}
}
}

/// Removes potentially duplicated ".local." at the end of "hostname".
Expand Down Expand Up @@ -1086,6 +1099,47 @@ pub(crate) fn split_sub_domain(domain: &str) -> (&str, Option<&str>) {
}
}

/// Represents a resolved service as a plain data struct.
/// This is from a client (i.e. querier) point of view.
#[non_exhaustive]
pub struct ResolvedService {
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One edge case is: if addresses is empty in this struct, then it would not be resolved. Should we call this struct just Service instead?

Copy link

@Luni-4 Luni-4 Feb 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we have decided to split among client and server, we can return an Option like we are doing right now, indeed Option<ServiceInfo> can have empty addresses. Therefore, Option<ResolvedService> could it be a solution?

/// Service type and domain. For example, "_http._tcp.local."
pub ty_domain: String,

/// Optional service subtype and domain.
///
/// See RFC6763 section 7.1 about "Subtypes":
/// <https://datatracker.ietf.org/doc/html/rfc6763#section-7.1>
/// For example, "_printer._sub._http._tcp.local."
pub sub_ty_domain: Option<String>,

/// Full name of the service. For example, "my-service._http._tcp.local."
pub fullname: String,

/// Host name of the service. For example, "my-server1.local."
pub host: String,

/// Port of the service. I.e. TCP or UDP port.
pub port: u16,

/// Addresses of the service. IPv4 or IPv6 addresses.
pub addresses: HashSet<IpAddr>,

/// Properties of the service, decoded from TXT record.
pub txt_properties: TxtProperties,
}

impl ResolvedService {
/// Returns true if the service data is valid, i.e. ready to be used.
pub fn is_valid(&self) -> bool {
let some_missing = self.ty_domain.is_empty()
|| self.fullname.is_empty()
|| self.host.is_empty()
|| self.addresses.is_empty();
!some_missing
}
}

#[cfg(test)]
mod tests {
use super::{
Expand Down
31 changes: 31 additions & 0 deletions tests/mdns_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -498,6 +498,37 @@ fn test_into_txt_properties() {
assert_eq!(txt_props.get_property_val_str("key2").unwrap(), "val2");
}

#[test]
fn test_info_as_resolved_service() {
let sub_ty_domain = "_printer._sub._test._tcp.local.";
let service_info = ServiceInfo::new(
sub_ty_domain,
"my_instance",
"my_host.local.",
"192.168.0.1",
5200,
None,
)
.unwrap();
let resolved_service = service_info.as_resolved_service();
assert!(resolved_service.is_valid());
assert_eq!(resolved_service.sub_ty_domain.unwrap(), sub_ty_domain);
assert_eq!(resolved_service.ty_domain, "_test._tcp.local.");

let info_missing_addr = ServiceInfo::new(
"_test._tcp.local.",
"my_instance",
"my_host.local.",
"",
5200,
None,
)
.unwrap();
let invalid_service = info_missing_addr.as_resolved_service();
assert!(!invalid_service.is_valid());
assert!(invalid_service.sub_ty_domain.is_none());
}

/// Test enabling an interface using its name, for example "en0".
/// Also tests an instance name with Upper Case.
#[test]
Expand Down
Loading