[pbs-devel] [PATCH] metrics: influxdb test uri creation

[pbs-devel] [PATCH] metrics: influxdb test uri creation

[Gabriel Goller]

Extract the URI creation for write and health URIs. Add unit test to
test the encoding of special characters in the organization and bucket
parameters.
This is a follow-up to:
https://lists.proxmox.com/pipermail/pbs-devel/2024-April/009096.html

Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
---
 proxmox-metrics/src/influxdb/http.rs | 93 ++++++++++++++++++++--------
 1 file changed, 67 insertions(+), 26 deletions(-)

diff --git a/proxmox-metrics/src/influxdb/http.rs b/proxmox-metrics/src/influxdb/http.rs
index 8167b59..1d53794 100644
--- a/proxmox-metrics/src/influxdb/http.rs
+++ b/proxmox-metrics/src/influxdb/http.rs
@@ -84,6 +84,25 @@ impl InfluxDbHttp {
             Client::with_ssl_connector(ssl_connector.build(), HttpOptions::default())
         };
 
+        let (writeuri, healthuri) = Self::create_uris(uri, organization, bucket)?;
+
+        Ok(InfluxDbHttp {
+            client,
+            writeuri,
+            healthuri,
+            token: token.map(String::from),
+            max_body_size,
+            data: String::new(),
+            channel,
+        })
+    }
+
+    /// Return a tuple with the write_uri and the health_uri
+    fn create_uris(
+        uri: &str,
+        org: &str,
+        bucket: &str,
+    ) -> Result<(http::uri::Uri, http::uri::Uri), anyhow::Error> {
         let uri: http::uri::Uri = uri.parse()?;
         let uri_parts = uri.into_parts();
 
@@ -93,35 +112,25 @@ impl InfluxDbHttp {
             ""
         };
 
-        let encoded_org: String =
-            url::form_urlencoded::byte_serialize(organization.as_bytes()).collect();
+        let encoded_org: String = url::form_urlencoded::byte_serialize(org.as_bytes()).collect();
         let encoded_bucket: String =
             url::form_urlencoded::byte_serialize(bucket.as_bytes()).collect();
 
-        let writeuri = http::uri::Builder::new()
-            .scheme(uri_parts.scheme.clone().unwrap())
-            .authority(uri_parts.authority.clone().unwrap())
-            .path_and_query(format!(
-                "{}/api/v2/write?org={}&bucket={}",
-                base_path, encoded_org, encoded_bucket
-            ))
-            .build()?;
-
-        let healthuri = http::uri::Builder::new()
-            .scheme(uri_parts.scheme.unwrap())
-            .authority(uri_parts.authority.unwrap())
-            .path_and_query(format!("{}/health", base_path))
-            .build()?;
-
-        Ok(InfluxDbHttp {
-            client,
-            writeuri,
-            healthuri,
-            token: token.map(String::from),
-            max_body_size,
-            data: String::new(),
-            channel,
-        })
+        Ok((
+            http::uri::Builder::new()
+                .scheme(uri_parts.scheme.clone().unwrap())
+                .authority(uri_parts.authority.clone().unwrap())
+                .path_and_query(format!(
+                    "{}/api/v2/write?org={}&bucket={}",
+                    base_path, encoded_org, encoded_bucket
+                ))
+                .build()?,
+            http::uri::Builder::new()
+                .scheme(uri_parts.scheme.unwrap())
+                .authority(uri_parts.authority.unwrap())
+                .path_and_query(format!("{}/health", base_path))
+                .build()?,
+        ))
     }
 
     async fn test_connection(&self) -> Result<(), Error> {
@@ -188,3 +197,35 @@ impl InfluxDbHttp {
         Ok(())
     }
 }
+
+#[cfg(test)]
+mod test {
+    use super::InfluxDbHttp;
+
+    #[test]
+    fn uri_encoding() {
+        let (writeuri, healthuri) =
+            InfluxDbHttp::create_uris("http://localhost/", "c ool/org", "a🔒nother&bu=ck?et")
+                .unwrap();
+        assert_eq!(writeuri.host(), Some("localhost"));
+        assert_eq!(writeuri.path(), "/api/v2/write");
+        assert_eq!(
+            writeuri.query(),
+            Some("org=c+ool%2Forg&bucket=a%F0%9F%94%92nother%26bu%3Dck%3Fet")
+        );
+
+        assert_eq!(healthuri.host(), Some("localhost"));
+        assert_eq!(healthuri.path(), "/health");
+        assert_eq!(healthuri.query(), None);
+
+        let (writeuri, healthuri) =
+            InfluxDbHttp::create_uris("http://localhost/", "org", "bucket").unwrap();
+        assert_eq!(writeuri.host(), Some("localhost"));
+        assert_eq!(writeuri.path(), "/api/v2/write");
+        assert_eq!(writeuri.query(), Some("org=org&bucket=bucket"));
+
+        assert_eq!(healthuri.host(), Some("localhost"));
+        assert_eq!(healthuri.path(), "/health");
+        assert_eq!(healthuri.query(), None);
+    }
+}
-- 
2.43.0



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

Re: [pbs-devel] [PATCH] metrics: influxdb test uri creation

[Fabian Grünbichler]

doesn't apply anymore.

two small things:
- form_urlencoded is just a re-export in url, maybe we want to actually
  use it instead since we don't otherwise need url?
- IMHO it would be more elegant to use the provided Serializer instead
  of encoding manually, something like this:

        let writeuri_query = url::form_urlencoded::Serializer::new(String::new())
            .append_pair("org", organization)
            .append_pair("bucket", bucket)
            .finish();
        let writeuri = http::uri::Builder::new()
            .scheme(uri_parts.scheme.clone().unwrap())
            .authority(uri_parts.authority.clone().unwrap())
            .path_and_query(format!("{base_path}/api/v2/write?{writeuri_query}",))
            .build()?;

AFAICT, the output is the same, but this is a much nicer interface?

On April 30, 2024 12:10 pm, Gabriel Goller wrote:
> Extract the URI creation for write and health URIs. Add unit test to
> test the encoding of special characters in the organization and bucket
> parameters.
> This is a follow-up to:
> https://lists.proxmox.com/pipermail/pbs-devel/2024-April/009096.html
> 
> Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
> ---
>  proxmox-metrics/src/influxdb/http.rs | 93 ++++++++++++++++++++--------
>  1 file changed, 67 insertions(+), 26 deletions(-)
> 
> diff --git a/proxmox-metrics/src/influxdb/http.rs b/proxmox-metrics/src/influxdb/http.rs
> index 8167b59..1d53794 100644
> --- a/proxmox-metrics/src/influxdb/http.rs
> +++ b/proxmox-metrics/src/influxdb/http.rs
> @@ -84,6 +84,25 @@ impl InfluxDbHttp {
>              Client::with_ssl_connector(ssl_connector.build(), HttpOptions::default())
>          };
>  
> +        let (writeuri, healthuri) = Self::create_uris(uri, organization, bucket)?;
> +
> +        Ok(InfluxDbHttp {
> +            client,
> +            writeuri,
> +            healthuri,
> +            token: token.map(String::from),
> +            max_body_size,
> +            data: String::new(),
> +            channel,
> +        })
> +    }
> +
> +    /// Return a tuple with the write_uri and the health_uri
> +    fn create_uris(
> +        uri: &str,
> +        org: &str,
> +        bucket: &str,
> +    ) -> Result<(http::uri::Uri, http::uri::Uri), anyhow::Error> {
>          let uri: http::uri::Uri = uri.parse()?;
>          let uri_parts = uri.into_parts();
>  
> @@ -93,35 +112,25 @@ impl InfluxDbHttp {
>              ""
>          };
>  
> -        let encoded_org: String =
> -            url::form_urlencoded::byte_serialize(organization.as_bytes()).collect();
> +        let encoded_org: String = url::form_urlencoded::byte_serialize(org.as_bytes()).collect();
>          let encoded_bucket: String =
>              url::form_urlencoded::byte_serialize(bucket.as_bytes()).collect();
>  
> -        let writeuri = http::uri::Builder::new()
> -            .scheme(uri_parts.scheme.clone().unwrap())
> -            .authority(uri_parts.authority.clone().unwrap())
> -            .path_and_query(format!(
> -                "{}/api/v2/write?org={}&bucket={}",
> -                base_path, encoded_org, encoded_bucket
> -            ))
> -            .build()?;
> -
> -        let healthuri = http::uri::Builder::new()
> -            .scheme(uri_parts.scheme.unwrap())
> -            .authority(uri_parts.authority.unwrap())
> -            .path_and_query(format!("{}/health", base_path))
> -            .build()?;
> -
> -        Ok(InfluxDbHttp {
> -            client,
> -            writeuri,
> -            healthuri,
> -            token: token.map(String::from),
> -            max_body_size,
> -            data: String::new(),
> -            channel,
> -        })
> +        Ok((
> +            http::uri::Builder::new()
> +                .scheme(uri_parts.scheme.clone().unwrap())
> +                .authority(uri_parts.authority.clone().unwrap())
> +                .path_and_query(format!(
> +                    "{}/api/v2/write?org={}&bucket={}",
> +                    base_path, encoded_org, encoded_bucket
> +                ))
> +                .build()?,
> +            http::uri::Builder::new()
> +                .scheme(uri_parts.scheme.unwrap())
> +                .authority(uri_parts.authority.unwrap())
> +                .path_and_query(format!("{}/health", base_path))
> +                .build()?,
> +        ))
>      }
>  
>      async fn test_connection(&self) -> Result<(), Error> {
> @@ -188,3 +197,35 @@ impl InfluxDbHttp {
>          Ok(())
>      }
>  }
> +
> +#[cfg(test)]
> +mod test {
> +    use super::InfluxDbHttp;
> +
> +    #[test]
> +    fn uri_encoding() {
> +        let (writeuri, healthuri) =
> +            InfluxDbHttp::create_uris("http://localhost/", "c ool/org", "a🔒nother&bu=ck?et")
> +                .unwrap();
> +        assert_eq!(writeuri.host(), Some("localhost"));
> +        assert_eq!(writeuri.path(), "/api/v2/write");
> +        assert_eq!(
> +            writeuri.query(),
> +            Some("org=c+ool%2Forg&bucket=a%F0%9F%94%92nother%26bu%3Dck%3Fet")
> +        );
> +
> +        assert_eq!(healthuri.host(), Some("localhost"));
> +        assert_eq!(healthuri.path(), "/health");
> +        assert_eq!(healthuri.query(), None);
> +
> +        let (writeuri, healthuri) =
> +            InfluxDbHttp::create_uris("http://localhost/", "org", "bucket").unwrap();
> +        assert_eq!(writeuri.host(), Some("localhost"));
> +        assert_eq!(writeuri.path(), "/api/v2/write");
> +        assert_eq!(writeuri.query(), Some("org=org&bucket=bucket"));
> +
> +        assert_eq!(healthuri.host(), Some("localhost"));
> +        assert_eq!(healthuri.path(), "/health");
> +        assert_eq!(healthuri.query(), None);
> +    }
> +}
> -- 
> 2.43.0
> 
> 
> 
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
> 


_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

Re: [pbs-devel] [PATCH] metrics: influxdb test uri creation

[Gabriel Goller]

On 23.05.2024 12:22, Fabian Grünbichler wrote:
>doesn't apply anymore.
>
>two small things:
>- form_urlencoded is just a re-export in url, maybe we want to actually
>  use it instead since we don't otherwise need url?

Sure, we can do this.

>- IMHO it would be more elegant to use the provided Serializer instead
>  of encoding manually, something like this:
>
>        let writeuri_query = url::form_urlencoded::Serializer::new(String::new())
>            .append_pair("org", organization)
>            .append_pair("bucket", bucket)
>            .finish();
>        let writeuri = http::uri::Builder::new()
>            .scheme(uri_parts.scheme.clone().unwrap())
>            .authority(uri_parts.authority.clone().unwrap())
>            .path_and_query(format!("{base_path}/api/v2/write?{writeuri_query}",))
>            .build()?;
>
>AFAICT, the output is the same, but this is a much nicer interface?

Right, yes, this is cleaner :)

Will submit a new patch shortly!


_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel