Re: [pve-devel] [PATCH v3 proxmox 02/66] notify: preparation for the first endpoint plugin

[Wolfgang Bumiller <w.bumiller@proxmox.com>]

On Mon, Jul 17, 2023 at 04:59:47PM +0200, Lukas Wagner wrote:
> Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
> ---
(...)
> +/// Notification bus - distributes notifications to all registered endpoints
> +// The reason for the split between `Config` and this struct is to make testing with mocked
> +// endpoints a bit easier.
> +#[derive(Default)]
> +pub struct Bus {
> +    endpoints: HashMap<String, Box<dyn Endpoint>>,
> +}
> +
> +#[allow(unused_macros)]
> +macro_rules! parse_endpoints_with_private_config {
> +    ($config:ident, $public_config:ty, $private_config:ty, $endpoint_type:ident, $type_name:expr) => {
> +        (|| -> Result<Vec<Box<dyn Endpoint>>, Error> {
> +            let mut endpoints: Vec<Box<dyn Endpoint>> = Vec::new();

nit: Less to type would be
            let mut endpoints = Vec::<Box<dyn Endpoint>>::new();
;-)

> +
> +            let configs: Vec<$public_config> = $config
> +                .config
> +                .convert_to_typed_array($type_name)
> +                .map_err(|err| Error::ConfigDeserialization(err.into()))?;
> +
> +            let private_configs: Vec<$private_config> = $config
> +                .private_config
> +                .convert_to_typed_array($type_name)
> +                .map_err(|err| Error::ConfigDeserialization(err.into()))?;
> +
> +            for config in configs {
> +                if let Some(private_config) = private_configs.iter().find(|p| p.name == config.name)

If you use `private_config` only for this kind of lookup, maybe this
should use `$config.private_config.sections.get(&config.name)` and match
the type name after the lookup, since you're now linear-searching an ID
for something that was previously a HashMap from id to data ;-)

> +                {
> +                    endpoints.push(Box::new($endpoint_type {
> +                        config,
> +                        private_config: private_config.clone(),
> +                    }));
> +                } else {
> +                    log::error!(
> +                        "Could not instantiate endpoint '{name}': private config does not exist",
> +                        name = config.name
> +                    );
> +                }
> +            }
> +
> +            Ok(endpoints)
> +        })()
> +    };
> +}
> +
> +#[allow(unused_macros)]
> +macro_rules! parse_endpoints_without_private_config {
> +    ($config:ident, $public_config:ty, $endpoint_type:ident, $type_name:expr) => {
> +        (|| -> Result<Vec<Box<dyn Endpoint>>, Error> {
> +            let mut endpoints: Vec<Box<dyn Endpoint>> = Vec::new();
> +
> +            let configs: Vec<$public_config> = $config
> +                .config
> +                .convert_to_typed_array($type_name)
> +                .map_err(|err| Error::ConfigDeserialization(err.into()))?;
> +
> +            for config in configs {
> +                endpoints.push(Box::new($endpoint_type { config }));
> +            }
> +
> +            Ok(endpoints)
> +        })()
> +    };
> +}
> +
(...)
> diff --git a/proxmox-notify/src/schema.rs b/proxmox-notify/src/schema.rs
> new file mode 100644
> index 00000000..68f11959
> --- /dev/null
> +++ b/proxmox-notify/src/schema.rs
> @@ -0,0 +1,43 @@
> +use proxmox_schema::{const_regex, ApiStringFormat, Schema, StringSchema};
> +
> +// Copied from PBS
> +macro_rules! proxmox_safe_id_regex_str {

^ You can drop this, since you depend on proxmox_schema, where by now we
have `SAFE_ID_REGEX` and `SAFE_ID_FORMAT` if you enable the `api-types`
feature :-)

> +    () => {
> +        r"(?:[A-Za-z0-9_][A-Za-z0-9._\-]*)"
> +    };
> +}
> +
> +const_regex! {
> +    pub SINGLE_LINE_COMMENT_REGEX = r"^[[:^cntrl:]]*$";

^ Feel free to move this to `proxmox_schema::api_types` as well.

> +    pub PROXMOX_SAFE_ID_REGEX = concat!(r"^", proxmox_safe_id_regex_str!(), r"$");
> +}
> +
> +const SINGLE_LINE_COMMENT_FORMAT: ApiStringFormat =
> +    ApiStringFormat::Pattern(&SINGLE_LINE_COMMENT_REGEX);

^ And this

> +
> +pub const COMMENT_SCHEMA: Schema = StringSchema::new("Comment.")

^ And this.

> +    .format(&SINGLE_LINE_COMMENT_FORMAT)
> +    .max_length(128)
> +    .schema();
> +
> +pub const EMAIL_SCHEMA: Schema = StringSchema::new("E-Mail Address.")
> +    .format(&SINGLE_LINE_COMMENT_FORMAT)
> +    .min_length(2)
> +    .max_length(64)
> +    .schema();
> +
> +pub const PROXMOX_SAFE_ID_FORMAT: ApiStringFormat =
> +    ApiStringFormat::Pattern(&PROXMOX_SAFE_ID_REGEX);
> +
> +pub const BACKEND_NAME_SCHEMA: Schema = StringSchema::new("Notification backend name.")
> +    .format(&PROXMOX_SAFE_ID_FORMAT)
> +    .min_length(3)
> +    .max_length(32)
> +    .schema();
> +
> +pub const ENTITY_NAME_SCHEMA: Schema =
> +    StringSchema::new("Name schema for endpoints, filters and groups")
> +        .format(&PROXMOX_SAFE_ID_FORMAT)
> +        .min_length(2)
> +        .max_length(32)
> +        .schema();
> -- 
> 2.39.2