Re: [pve-devel] [PATCH v3 proxmox 20/66] notify: on deletion, check if a filter/endp. is still used by anything

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

On Mon, Jul 17, 2023 at 05:00:05PM +0200, Lukas Wagner wrote:
> Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
> ---
>  proxmox-notify/src/api/filter.rs   |   1 +
>  proxmox-notify/src/api/gotify.rs   |   1 +
>  proxmox-notify/src/api/mod.rs      | 113 ++++++++++++++++++++++++++---
>  proxmox-notify/src/api/sendmail.rs |   1 +
>  4 files changed, 106 insertions(+), 10 deletions(-)
> 
> diff --git a/proxmox-notify/src/api/filter.rs b/proxmox-notify/src/api/filter.rs
> index 3fcff6b9..824f802d 100644
> --- a/proxmox-notify/src/api/filter.rs
> +++ b/proxmox-notify/src/api/filter.rs
> @@ -115,6 +115,7 @@ pub fn update_filter(
>  pub fn delete_filter(config: &mut Config, name: &str) -> Result<(), ApiError> {
>      // Check if the filter exists
>      let _ = get_filter(config, name)?;
> +    super::ensure_unused(config, name)?;
>  
>      config.config.sections.remove(name);
>  
> diff --git a/proxmox-notify/src/api/gotify.rs b/proxmox-notify/src/api/gotify.rs
> index d6f33064..5c4db4be 100644
> --- a/proxmox-notify/src/api/gotify.rs
> +++ b/proxmox-notify/src/api/gotify.rs
> @@ -145,6 +145,7 @@ pub fn update_endpoint(
>  pub fn delete_gotify_endpoint(config: &mut Config, name: &str) -> Result<(), ApiError> {
>      // Check if the endpoint exists
>      let _ = get_endpoint(config, name)?;
> +    super::ensure_unused(config, name)?;
>  
>      remove_private_config_entry(config, name)?;
>      config.config.sections.remove(name);
> diff --git a/proxmox-notify/src/api/mod.rs b/proxmox-notify/src/api/mod.rs
> index d8a44bf2..81c182c7 100644
> --- a/proxmox-notify/src/api/mod.rs
> +++ b/proxmox-notify/src/api/mod.rs
> @@ -102,6 +102,59 @@ fn endpoint_exists(config: &Config, name: &str) -> bool {
>      exists
>  }
>  
> +fn get_referrers(config: &Config, entity: &str) -> Result<HashSet<String>, ApiError> {
> +    let mut referrers = HashSet::new();
> +
> +    for group in group::get_groups(config)? {
> +        for endpoint in group.endpoint {
> +            if endpoint == entity {
> +                referrers.insert(group.name.clone());

We could `break` here, since `group` comes from the outer loop.

Or, in fact, replace the inner for loop with:

    if group.endpoint.iter().any(|endpoint| endpoint == entity) {
        referrers.insert(group.name.clone());
    }

> +            }
> +        }
> +
> +        if let Some(filter) = group.filter {
> +            if filter == entity {
> +                referrers.insert(group.name);
> +            }
> +        }
> +    }
> +
> +    #[cfg(feature = "sendmail")]
> +    for endpoint in sendmail::get_endpoints(config)? {
> +        if let Some(filter) = endpoint.filter {
> +            if filter == entity {
> +                referrers.insert(endpoint.name);
> +            }
> +        }
> +    }
> +
> +    #[cfg(feature = "gotify")]
> +    for endpoint in gotify::get_endpoints(config)? {
> +        if let Some(filter) = endpoint.filter {
> +            if filter == entity {
> +                referrers.insert(endpoint.name);
> +            }
> +        }
> +    }
> +
> +    Ok(referrers)
> +}
> +
> +fn ensure_unused(config: &Config, entity: &str) -> Result<(), ApiError> {
> +    let referrers = get_referrers(config, entity)?;
> +
> +    if !referrers.is_empty() {
> +        let used_by = referrers.into_iter().collect::<Vec<_>>().join(", ");

*sigh*...
iterators vs join...
Some day this can hopefully be
    refererrs
        .into_iter()
        .map(Cow::Owned)
        .intersperse(Cow::Borrowed(", "))
        .collect::<String>();
Yeah okay fine, the ergonomics with `String` vs the `&'static str`
aren't great either... exactly as long as the for loop variant, but w/e.
Could also just be
    referrers.iter().map(String::as_str).intersperse(", ").collect::<String>();


... Oh well ...

> +
> +        return Err(ApiError::bad_request(
> +            format!("cannot delete '{entity}', referenced by: {used_by}"),
> +            None,
> +        ));
> +    }
> +
> +    Ok(())
> +}
> +
>  fn get_referenced_entities(config: &Config, entity: &str) -> HashSet<String> {
>      let mut to_expand = HashSet::new();
>      let mut expanded = HashSet::new();
(...)