feat: discovery document macro

[Alxandr]

BREAKING-CHANGE: fields are no longer public

[Alxandr]

@arcnmx before I steam-roll my way to a destination I figured I'd ask if you had any input on this. The proof of concept macro for creating the entity types is starting to take shape. It currently does the following:

1. add all fields that are common to entity (no more nested entity struct).
2. create a builder
3. create a invalidity enum
4. implement validate for the builder
5. creates a build method for the builder that does validation and returns ok only if it validates

In addition to this, the fields on the entity structs have been made private (so that if you have a Light or whatever - it has already been validated). It also delegates deserialization through this path.

Example:

use hass_mqtt_discovery::{
  availability::Availability, availability::AvailabilityMode, device::Device,
  device_class::DeviceClass, entity_category::EntityCategory, icon::Icon, name::Name,
  payload::Payload, qos::MqttQoS, template::Template, topic::Topic, unique_id::UniqueId,
};
use hass_mqtt_discovery_macros::entity_document;
use std::borrow::Cow;

/// The mqtt switch platform lets you control your MQTT enabled switches.
///
/// See: <https://www.home-assistant.io/integrations/switch.mqtt/>
#[entity_document]
pub struct Switch<'a> {
  /// The MQTT topic to publish commands to change the switch state.
  #[entity(validate)]
  #[serde(borrow)]
  command_topic: Topic<'a>,

  /// The [type/class][device_class] of the switch to set the icon in the frontend.
  ///
  /// [device_class]: https://www.home-assistant.io/integrations/switch/#device-class
  #[serde(default, skip_serializing_if = "Option::is_none")]
  device_class: Option<DeviceClass>,

  /// Flag that defines if switch works in optimistic mode.
  /// Defaults to `true` if no `state_topic` defined, else `false`.
  #[serde(default, skip_serializing_if = "Option::is_none")]
  optimistic: Option<bool>,

  /// The payload that represents `off` state. If specified, will be
  /// used for both comparing to the value in the `state_topic` (see
  /// `value_template` and `state_off` for details) and sending as
  /// `off` command to the `command_topic`.
  /// Defaults to `"OFF"`.
  #[entity(validate)]
  #[serde(borrow, default, skip_serializing_if = "Option::is_none")]
  payload_off: Option<Payload<'a>>,

  /// The payload that represents `on` state. If specified, will be
  /// used for both comparing to the value in the `state_topic` (see
  /// `value_template` and `state_on` for details) and sending as
  /// `on` command to the `command_topic`.
  /// Defaults to `"ON"`.
  #[entity(validate)]
  #[serde(borrow, default, skip_serializing_if = "Option::is_none")]
  payload_on: Option<Payload<'a>>,

  /// If the published message should have the retain flag on or not.
  /// Defaults to `false`.
  #[serde(default, skip_serializing_if = "Option::is_none")]
  retain: Option<bool>,

  /// The payload that represents the `off` state. Used when value that
  /// represents `off` state in the `state_topic` is different from value that
  /// should be sent to the `command_topic` to turn the device `off`.
  /// Defaults to `payload_off` if defined, else `"OFF"`.
  #[entity(validate)]
  #[serde(borrow, default, skip_serializing_if = "Option::is_none")]
  state_off: Option<Payload<'a>>,

  /// The payload that represents the `on` state. Used when value that
  /// represents on state in the `state_topic` is different from value that
  /// should be sent to the `command_topic` to turn the device `on`.
  /// Defaults to `payload_on` if defined, else `"ON"`.
  #[entity(validate)]
  #[serde(borrow, default, skip_serializing_if = "Option::is_none")]
  state_on: Option<Payload<'a>>,

  /// The MQTT topic subscribed to receive state updates.
  #[entity(validate)]
  #[serde(borrow, default, skip_serializing_if = "Option::is_none")]
  state_topic: Option<Topic<'a>>,

  /// Defines a [template][template] to extract device’s state from the
  /// `state_topic`. To determine the switches’s state result of this
  /// template will be compared to `state_on` and `state_off`.
  ///
  /// [template]: https://www.home-assistant.io/docs/configuration/templating/#processing-incoming-data
  #[entity(validate)]
  #[serde(borrow, default, skip_serializing_if = "Option::is_none")]
  value_template: Option<Template<'a>>,
}

[Alxandr]

Also - what I was mainly going to ask about was method names for builder setters. I'm currently considering a couple:

pub fn with_topic(self, topic: Topic<'a>) -> Self;
pub fn topic(&mut self) -> &mut Option<Topic<'a>>;

or

pub fn with_topic(self, topic: Topic<'a>) -> Self;
pub fn topic(&mut self, topic: Topic<'a>) -> &mut Self;

[arcnmx]

I'll try to take a proper look in the next few days when I can, but wanted to mention...

the fields on the entity structs have been made private (so that if you have a Light or whatever - it has already been validated).

I'm not really in favour of this, especially since a read-only wrapper type already exists for exactly this purpose: semval::Validated<T>. It's unfortunate that serde isn't an optional dep of semval such that you could deserialize directly into Validated<T: Deserialize>, but I don't see the need to duplicate semval's functionality/api while making the underlying structs less functional, if that makes sense?

[Alxandr]

I'm not really in favour of this, especially since a read-only wrapper type already exists for exactly this purpose: semval::Validated<T>

The main issue I have with this is that you end up with the builder being mostly useless - the moment you have a builder API, I'm in flavor of the builder being the scratch surface where you can have invalid data, and when you "build" the entity, it's guaranteed to be valid. If the builder allows you to create invalid data, I feel it's less useful. But then, giving you direct access to the fields makes it so you can re-add invalid data.

What I've done instead is make it easy to convert back to a builder (where you can then modify things), and build that back into a new entity.

[arcnmx]

But then, giving you direct access to the fields makes it so you can re-add invalid data. What I've done instead is make it easy to convert back to a builder (where you can then modify things), and build that back into a new entity.

My issue with this is that it just sounds like a complex reimplementation of Validated<T>, which already seems like an elegant design to me; it starts with the raw mutable data (a normal struct, T) - whether created by a builder, constructor, field-by-field, or whatever. Then you convert it to the Validated<T> immutable form (which provides read-only access to all fields via Deref) by calling t.into_validated(). Finally, it allows conversion back to the fields via validated.into(), which you can then mutate and validate again.

Another advantage is that it's a standard way to do this provided by the validation library that's already in use - Validated<T> is just how you're supposed to use semval to begin with when passing around types that impl Validate.

The main issue I have with this is that you end up with the builder being mostly useless

well, builders have basically one purpose: to facilitate the construction of values that would be cumbersome to build otherwise; often (and in this case) due to data structures containing a large number of optional fields.
That's all that makes them useful to begin with, and constructors are a perfectly valid alternative to a builder in this case for a reason (builders also just happen to be a decent way to approximate named arguments in rust).

Validating a data struct is already a single function call away (x.validate() or x.into_validated()), so I don't see how a builder calling that for me makes it useful 😛
I I do agree that a builder isn't particularly useful in this scenario... For a low-level api modelling a json payload that is likely to be abstracted away from most sane people, you could live just fine with constructors as long as the number of required fields stays reasonable. It's also possible to implement Switch::default() or Switch::new() even if it produces an invalid Switch that would not pass validation until you explicitly set the command_topic field to a non-empty string.

[Alxandr]

I tend to try to follow the mantra of "make invalid state unrepresentable" - but in tihs case I've (after trying a lot) figured that I can't do it without loosing part of the point of the discovery library. The discovery library is supposed to be low-level and (hopefully) low overhead, with nicer APIs built on top - the issue with builder -> value paradigm is that if you have a list of builders that you want to build&validate, you need to allocate a list of results. Or do some really ugly schenanigans with mutable enums of valid and invalid data intermingled.

That's one of the nice things about the semval::Validated - it just asserts the whole tree is validated without actually touching it. But I do loose the property that any Topic you may possess is guaranteed to be valid (str style).

Anyways - I think I've closing in on a design that will both work well, and I'm reasonably happy with. I currently have the below working. Note that the fields are back to being public - but there are accessor/modifier functions generated with the same names. Basically, I made the actual entity-documents into the builder - so we only have builders and Validated<T> is the "built" data. I've also made it so that trying to serialize invalid data will fail with a validation error. This can in theory cause overhead if you serialize the same thing many times - but that's not really what this is designed for. The documents are supposed to be short lived just to aid in serialization.

I'm also contemplating getting rid of all the Cows - and just use raw references - forcing the user of the library to own the data. It means I won't be able to create a trait like this:

pub trait Switch {
  fn as_document(&self) -> discovery::Switch;
}

and will instead have to do something like a visitor pattern - but I think that's an ok compromise.

use hass_mqtt_discovery::{
  device_class::DeviceClass, payload::Payload, template::Template, topic::Topic,
};
use hass_mqtt_discovery_macros::entity_document;
use std::borrow::Cow;

/// The mqtt switch platform lets you control your MQTT enabled switches.
///
/// See: <https://www.home-assistant.io/integrations/switch.mqtt/>
#[entity_document]
pub struct Switch<'a> {
  /// The MQTT topic to publish commands to change the switch state.
  #[entity(validate)]
  #[serde(borrow)]
  pub command_topic: Topic<'a>,

  /// The [type/class][device_class] of the switch to set the icon in the frontend.
  ///
  /// [device_class]: https://www.home-assistant.io/integrations/switch/#device-class
  #[serde(default, skip_serializing_if = "Option::is_none")]
  pub device_class: Option<DeviceClass>,

  /// Flag that defines if switch works in optimistic mode.
  /// Defaults to `true` if no `state_topic` defined, else `false`.
  #[serde(default, skip_serializing_if = "Option::is_none")]
  pub optimistic: Option<bool>,

  /// The payload that represents `off` state. If specified, will be
  /// used for both comparing to the value in the `state_topic` (see
  /// `value_template` and `state_off` for details) and sending as
  /// `off` command to the `command_topic`.
  /// Defaults to `"OFF"`.
  #[entity(validate)]
  #[serde(borrow, default, skip_serializing_if = "Option::is_none")]
  pub payload_off: Option<Payload<'a>>,

  /// The payload that represents `on` state. If specified, will be
  /// used for both comparing to the value in the `state_topic` (see
  /// `value_template` and `state_on` for details) and sending as
  /// `on` command to the `command_topic`.
  /// Defaults to `"ON"`.
  #[entity(validate)]
  #[serde(borrow, default, skip_serializing_if = "Option::is_none")]
  pub payload_on: Option<Payload<'a>>,

  /// If the published message should have the retain flag on or not.
  /// Defaults to `false`.
  #[serde(default, skip_serializing_if = "Option::is_none")]
  pub retain: Option<bool>,

  /// The payload that represents the `off` state. Used when value that
  /// represents `off` state in the `state_topic` is different from value that
  /// should be sent to the `command_topic` to turn the device `off`.
  /// Defaults to `payload_off` if defined, else `"OFF"`.
  #[entity(validate)]
  #[serde(borrow, default, skip_serializing_if = "Option::is_none")]
  pub state_off: Option<Payload<'a>>,

  /// The payload that represents the `on` state. Used when value that
  /// represents on state in the `state_topic` is different from value that
  /// should be sent to the `command_topic` to turn the device `on`.
  /// Defaults to `payload_on` if defined, else `"ON"`.
  #[entity(validate)]
  #[serde(borrow, default, skip_serializing_if = "Option::is_none")]
  pub state_on: Option<Payload<'a>>,

  /// The MQTT topic subscribed to receive state updates.
  #[entity(validate)]
  #[serde(borrow, default, skip_serializing_if = "Option::is_none")]
  pub state_topic: Option<Topic<'a>>,

  /// Defines a [template][template] to extract device’s state from the
  /// `state_topic`. To determine the switches’s state result of this
  /// template will be compared to `state_on` and `state_off`.
  ///
  /// [template]: https://www.home-assistant.io/docs/configuration/templating/#processing-incoming-data
  #[entity(validate)]
  #[serde(borrow, default, skip_serializing_if = "Option::is_none")]
  pub value_template: Option<Template<'a>>,
}

// pub enum SwitchInvalidity {
//   Availability(<Cow<'static, [Availability<'static>]> as ::semval::Validate>::Invalidity),
//   Device(<Option<Device<'static>> as ::semval::Validate>::Invalidity),
//   Icon(<Option<Icon<'static>> as ::semval::Validate>::Invalidity),
//   JsonAttributesTemplate(<Option<Template<'static>> as ::semval::Validate>::Invalidity),
//   JsonAttributesTopic(<Option<Topic<'static>> as ::semval::Validate>::Invalidity),
//   Name(<Option<Name<'static>> as ::semval::Validate>::Invalidity),
//   UniqueId(<Option<UniqueId<'static>> as ::semval::Validate>::Invalidity),
// }

fn main() {
  let switch = Switch::new("switch/foo")
    .optimistic(true)
    .state_on("ON")
    .state_off("OFF")
    .state_topic("switch/foo/state");
  println!("{}", serde_json::to_string_pretty(&switch).unwrap());
}

[arcnmx]

(I will take an actual look at the code soon - example definition looks nice though!)

I tend to try to follow the mantra of "make invalid state unrepresentable"

I do agree and aim for this typically, but it can depend on abstraction levels/boundaries? At some point, you are just working with a basic struct layout that just represents the json directly, and I'm not often a fan of hiding that behind private types - usually because it means that the crate itself is privileged in what it can accomplish, and downstream crates don't have as much flexibility. But there's always a balance between that and not exposing irrelevant implementation details...

But I do loose the property that any Topic you may possess is guaranteed to be valid (str style).

It's too bad that fn topic(self: Validated<Self>) -> Validated<Topic> isn't valid, but it is possible to work around this using an extension trait to emulate this. If semval were more mature, maybe it could provide a map(|v| &v.topic) method so you can get fields out and retain their validity? (safety of this is likely related to similar concerns that Pin::map_unchecked has, so I imagine the API design might be similar)

I'm also contemplating getting rid of all the Cows - and just use raw references - forcing the user of the library to own the data.

Cow is a little unwieldy, but what are the gains? As a data point basically all of my constructors for these types use topics that are dynamically format!()'d, so this forces me to have a MySwitch wrapper type that owns the data, then expose an as_switch accessor. Currently, I have functions that just generate Switch values from a set of input data.
It also complicates the Deserialize impls for these types (moving from Cow to ref will require deserialization from borrowed data only, won't impl DeserializeOwned).

I've also made it so that trying to serialize invalid data will fail with a validation error. This can in theory cause overhead if you serialize the same thing many times - but that's not really what this is designed for

Seems a tad excessive (higher-level code is likely to already be dealing with Validated<T>?), but I won't complain too much as it doesn't really matter 😛

[Alxandr]

Cow is a little unwieldy, but what are the gains? As a data point basically all of my constructors for these types use topics that are dynamically format!()'d, so this forces me to have a MySwitch wrapper type that owns the data, then expose an as_switch accessor. Currently, I have functions that just generate Switch values from a set of input data. It also complicates the Deserialize impls for these types (moving from Cow to ref will require deserialization from borrowed data only, won't impl DeserializeOwned).

Deserialize is a reason to keep the cows... I'll have to think about this a bit.

That being said - I've shifted my thinking of the use of this library slightly. So let's imagine you have a MySwitch that implements/holds switch state - instead of doing something like this:

impl AsDiscoveryDocument for MySwitch {
  fn as_discovery_document<'a>(&'a self) -> Switch<'a> {
    // make cows
    // return cows
  }
}

Instead you'd do something like this:

impl DiscoveryDocument for MySwitch {
  fn serialize<S: serde::Serializer>(&self, serializer: S) -> Result<S::Ok, S::Error> {
    let doc = Switch::new(self.blah).topic(format!("blah")); // short lived - doesn't need cows
    doc.serialize(serializer)
  }
}

[arcnmx]

make cows

To be fair you don't really have to make them yourself if the topic() builder method does it for you and accepts either a borrowed or owned value (impl Into<Cow>), the cow becomes transparent unless you access the field directly.

Instead you'd do something like this

That would actually need to be:

let topic = format!("blah");
let doc = Switch::new(self.blah).topic(&topic);
// or all in one expression so the temporary string lives only as long as the doc:
Switch::new(self.blah).topic(&format!("blah")).serialize()

But that's really still just an inline/private implementation of as_discovery_document(), except now we've made it much trickier to implement separately:

impl AsDiscoveryDocument for MySwitch {
  fn process_discovery_document<R, for <'a> F: FnOnce(Switch<'a>) -> R>(&self, f: F) -> R {
    let topic = format!("blah");
    let doc = Switch::new(self.blah).topic(&topic);
    f(doc)
  }
}

impl DiscoveryDocument for MySwitch {
  fn serialize<S: serde::Serializer>(&self, serializer: S) -> Result<S::Ok, S::Error> {
    self.process_discovery_document(|doc| doc.serialize(serializer))
  }
}

---

I'm not even disagreeing btw, just exploring the design space. Types with lifetimes that can't be 'static are indeed often used as temporary serialization helpers - they just also happen to not be very good at deserialization. I do believe these same types should support full deserialization though, so I don't think it's really worth it? Especially since it is likely that values created outside of deserialization are likely to be a mix of borrowed and owned/generated data. The downsides of using Cow are basically just that you need to use Into<> more so the user doesn't need to deal with them directly, which doesn't seem all that bad to me. Exposed setters/builders/accessors can make them mostly invisible (outside of accessing fields directly) if desired.
It is unfortunate that Rust's limitations prevent Cow from being as automagically useful as it could be, but we work with what we have...

[Alxandr]

I've added Light - which had some rather annoying properties (like "schema" which should always be "json"). The way I've done it currently will fail on lights which aren't schema:json - but I don't reaaaaaly care about that tbh. The alternative is implementing 3 lite structures, and an enum Light { JsonLight, TemplateLight, DefaultLight } - and I din't bother doing that at this point in time at least.

It also had a custom validation rule which forced me to figure out how to extend the validation the macro generates. I'm pretty happy with the system as it stands having tested it a bit - and unless you have any major worries I will probably be merging this as is (keeping the Cows). It's not like it can't be changed later anyways. I wanna try start building a layer on top of the discovery crate - and that might mean looking at more macros and maybe even some specialization :D

[arcnmx]

So I'm trying to use it but my builds are failing, I think because cargo workspace dependencies are too new of a feature? 😞

EDIT: confirmed, removing them fixes my problem. It was failing for me even when I use 1.64.0 (which is supposed to support them) because the build system vendors dependencies and apparently doesn't support it...

[Alxandr]

build system vendors dependencies

Which build system is it you're using that's not supporting this?

[arcnmx]

Which build system is it you're using that's not supporting this?

nix and nixpkgs. I've created an example reproducing the problem and reported it, basically anything that depends on a package that uses workspace dependencies fails because when they get vendored as part of the build, the package presumably gets plucked out of the workspace and can no longer find the versions.

[Alxandr]

I'd argue that's more of a "people who aren't using the official rust tools will have issues" problem - but on the other hand that's apparently also 100% of the userbase of this library :P. I'll revert to not using workspace packages and then probably merge this soon.

I've made an issue for migrating to workspace packages: #21 - can you link the upstream issue there so it's easier to track?

[arcnmx]

Yeah, sorry to be a pain... But to be fair, the feature was just released one version ago - if I weren't using the rolling unstable/unreleased version of NixOS I wouldn't even have a cargo version new enough to support it ><

[Alxandr]

I tend to not use developer tools from my system package manager for that exact reason (even though I'm on arch linux :P)

Also - I stole your commit that "fixed" the dependencies. I'll leave it to renovate to keep them in sync for now.