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

keepsimple1 · 2025-02-06T06:25:04Z

This patch is to follow up on the discussions in issue #299.

keepsimple1 · 2025-02-06T06:26:24Z

src/service_info.rs

@@ -1060,6 +1072,16 @@ pub(crate) fn split_sub_domain(domain: &str) -> (&str, Option<&str>) {
    }
 }

+#[non_exhaustive]
+pub struct ResolvedService {


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?

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?

src/service_info.rs

Luni-4 · 2025-02-06T09:18:39Z

This approach does not consume ServiceInfo, just clones its internal fields, so you have two structs in memory at the same time

keepsimple1 · 2025-02-07T06:36:02Z

This approach does not consume ServiceInfo, just clones its internal fields, so you have two structs in memory at the same time

I made the mistake in the 1st commit, please check the latest one. ;-)

Luni-4

This is fine for me, thanks a lot! :)

Just a simple nit

Luni-4 · 2025-02-07T07:35:14Z

src/service_info.rs

@@ -366,6 +366,18 @@ impl ServiceInfo {
            .cloned()
            .unwrap_or(ServiceStatus::Unknown)
    }
+
+    /// Returns a resolved service instance.
+    pub fn as_resolved_service(self) -> ResolvedService {


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

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

Better ergonomics for the user. (Having Option can complicate many things for the user, in my experience).

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.

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.

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).

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

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?

keepsimple1 · 2025-02-12T05:26:31Z

src/service_info.rs

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


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?

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.

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.

With public, I meant accessible via public interface :P

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

Luni-4

Thanks a lot! It is fine for me!

Just a small nit, you can merge for me after that

Luni-4 · 2025-02-14T08:06:49Z

src/service_info.rs

+/// This is from a client (i.e. querier) point of view.
+#[non_exhaustive]
+pub struct ResolvedService {
+    /// service type and domain. For example, "_http._tcp.local."


Nit: Shouldn't the comments of this structure capitalized? The sub_ty_domain is capitalized and even those of functions

keepsimple1 commented Feb 6, 2025

View reviewed changes

src/service_info.rs Outdated Show resolved Hide resolved

keepsimple1 commented Feb 6, 2025

View reviewed changes

src/service_info.rs Outdated Show resolved Hide resolved

Luni-4 approved these changes Feb 7, 2025

View reviewed changes

Luni-4 reviewed Feb 7, 2025

View reviewed changes

keepsimple1 force-pushed the resolved-service branch from e2ccfcc to 23f3016 Compare February 9, 2025 20:38

draft: a new struct for attributes of a resolve service

9f54e37

keepsimple1 force-pushed the resolved-service branch from 23f3016 to 9f54e37 Compare February 9, 2025 20:45

keepsimple1 added 3 commits February 11, 2025 19:43

add is_valid for ResolvedService

0b727f0

add comments

7659174

add a test case

6eb78f4

keepsimple1 commented Feb 12, 2025

View reviewed changes

add sub_ty_domain

7ca7e06

keepsimple1 changed the title ~~Draft: a new struct for attributes of a resolve service~~ ServiceInfo: a new struct for attributes of a resolve service Feb 14, 2025

keepsimple1 changed the title ~~ServiceInfo: a new struct for attributes of a resolve service~~ ResolvedService: a new plain struct for attributes of a resolve service from service info Feb 14, 2025

keepsimple1 changed the title ~~ResolvedService: a new plain struct for attributes of a resolve service from service info~~ ResolvedService: a new plain struct for attributes of a service Feb 14, 2025

Luni-4 approved these changes Feb 14, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

keepsimple1 commented Feb 6, 2025

keepsimple1 Feb 6, 2025

Luni-4 Feb 6, 2025 •

edited

Loading

Luni-4 commented Feb 6, 2025

keepsimple1 commented Feb 7, 2025

Luni-4 left a comment

Luni-4 Feb 7, 2025

keepsimple1 Feb 9, 2025

Luni-4 Feb 10, 2025 •

edited

Loading

keepsimple1 Feb 11, 2025

Luni-4 Feb 11, 2025 •

edited

Loading

keepsimple1 Feb 12, 2025

keepsimple1 Feb 12, 2025

Luni-4 Feb 12, 2025 •

edited

Loading

keepsimple1 Feb 13, 2025

Luni-4 Feb 13, 2025

keepsimple1 Feb 14, 2025

Luni-4 left a comment

Luni-4 Feb 14, 2025 •

edited

Loading

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

Are you sure you want to change the base?

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

Conversation

keepsimple1 commented Feb 6, 2025

Choose a reason for hiding this comment

Luni-4 Feb 6, 2025 • edited Loading

Choose a reason for hiding this comment

Luni-4 commented Feb 6, 2025

keepsimple1 commented Feb 7, 2025

Luni-4 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Luni-4 Feb 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Luni-4 Feb 11, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Luni-4 Feb 12, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Luni-4 left a comment

Choose a reason for hiding this comment

Luni-4 Feb 14, 2025 • edited Loading

Choose a reason for hiding this comment

Luni-4 Feb 6, 2025 •

edited

Loading

Luni-4 Feb 10, 2025 •

edited

Loading

Luni-4 Feb 11, 2025 •

edited

Loading

Luni-4 Feb 12, 2025 •

edited

Loading

Luni-4 Feb 14, 2025 •

edited

Loading