Re: [pbs-devel] [PATCH proxmox-ldap 3/6] add helpers for constructing LDAP filters

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

On Tue, Jan 17, 2023 at 03:20:34PM +0100, Lukas Wagner wrote:
> Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
> ---
>  src/lib.rs | 70 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 70 insertions(+)
> 
> diff --git a/src/lib.rs b/src/lib.rs
> index 48eb863..40c4f6d 100644
> --- a/src/lib.rs
> +++ b/src/lib.rs
> @@ -226,3 +226,73 @@ impl LdapConnection {
>          bail!("user not found")
>      }
>  }
> +
> +#[allow(dead_code)]
> +enum FilterElement {

Your only user of this is short-lived and can easily just borrow the
strings. I think we should give this a lifetime and use a str ref
instead of strings.

> +    And(Vec<FilterElement>),
> +    Or(Vec<FilterElement>),
> +    Condition(String, String),
> +    Not(Box<FilterElement>),
> +    Verbatim(String),
> +}
> +
> +impl ToString for FilterElement {

Is there a particular reason to implement `ToString` instead of
`Display`?
If not, a `Display` implementation would need much fewer temporary
allocated strings as it would just keep appending to the same
`fmt::Formatter`.

> +    fn to_string(&self) -> String {
> +        fn children_to_string(children: &[FilterElement]) -> String {
> +            children.iter().fold(String::new(), |mut acc, v| {
> +                acc.push_str(&v.to_string());
> +                acc
> +            })
> +        }
> +
> +        match self {
> +            FilterElement::And(children) => {
> +                format!("(&{})", children_to_string(children))
> +            }
> +            FilterElement::Or(children) => {
> +                format!("(|{})", children_to_string(children))
> +            }
> +            FilterElement::Not(element) => {
> +                format!("(!{})", element.to_string())
> +            }
> +            FilterElement::Condition(attr, value) => {
> +                format!("({attr}={value})")
> +            }
> +            FilterElement::Verbatim(verbatim) => verbatim.clone(),
> +        }
> +    }
> +}
> +
> +#[cfg(test)]
> +mod tests {
> +    use super::FilterElement::*;
> +
> +    #[test]
> +    fn test_filter_elements_to_string() {
> +        assert_eq!(
> +            "(uid=john)",
> +            Condition("uid".into(), "john".into()).to_string()
> +        );
> +        assert_eq!(
> +            "(!(uid=john))",
> +            Not(Box::new(Condition("uid".into(), "john".into()))).to_string()
> +        );
> +
> +        assert_eq!("(foo=bar)", Verbatim("(foo=bar)".into()).to_string());
> +
> +        let filter_string = And(vec![
> +            Condition("givenname".into(), "john".into()),
> +            Condition("sn".into(), "doe".into()),
> +            Or(vec![
> +                Condition("email".into(), "john@foo".into()),
> +                Condition("email".into(), "john@bar".into()),
> +            ]),
> +        ])
> +        .to_string();
> +
> +        assert_eq!(
> +            "(&(givenname=john)(sn=doe)(|(email=john@foo)(email=john@bar)))".to_owned(),

^ The `.to_owned()` is not needed there.

> +            filter_string
> +        );
> +    }
> +}
> -- 
> 2.30.2