Fix WatchParams bookmarks for watch_metadata (#1193)

* Fix `WatchParams` bookmarks for `watch_metadata` Intended to factor out `populate_qp` from `ListParams` and do the same in `WatchParams`, but accidentally found a place where the duplication caused a split. This brings the query param serialization in line for all variants of list, and does the same for all variants of watch (where the inconsistency was wound). Signed-off-by: clux <sszynrae@gmail.com> * fix a doc issue + fmt Signed-off-by: clux <sszynrae@gmail.com> --------- Signed-off-by: clux <sszynrae@gmail.com>
kube-rs · Apr 12, 2023 · d294847 · d294847
1 parent 6f3cfac
commit d294847
Show file tree

Hide file tree

Showing 3 changed files with 60 additions and 89 deletions.
diff --git a/kube-core/src/admission.rs b/kube-core/src/admission.rs
@@ -77,7 +77,7 @@ impl<T: Resource> TryInto<AdmissionRequest<T>> for AdmissionReview<T> {
 /// In an admission controller scenario, this is extracted from an [`AdmissionReview`] via [`TryInto`]
 ///
 /// ```no_run
-/// use kube::api::{admission::{AdmissionRequest, AdmissionReview}, DynamicObject};
+/// use kube::core::{admission::{AdmissionRequest, AdmissionReview}, DynamicObject};
 ///
 /// // The incoming AdmissionReview received by the controller.
 /// let body: AdmissionReview<DynamicObject> = todo!();
@@ -205,9 +205,9 @@ pub enum Operation {
 /// An outgoing [`AdmissionReview`] response. Constructed from the corresponding
 /// [`AdmissionRequest`].
 /// ```no_run
-/// use kube::api::{
-///         admission::{AdmissionRequest, AdmissionResponse, AdmissionReview},
-///         DynamicObject,
+/// use kube::core::{
+///     admission::{AdmissionRequest, AdmissionResponse, AdmissionReview},
+///     DynamicObject,
 /// };
 ///
 /// // The incoming AdmissionReview received by the controller.

diff --git a/kube-core/src/params.rs b/kube-core/src/params.rs
@@ -89,6 +89,35 @@ impl ListParams {
         }
         Ok(())
     }
+
+    // Partially populate query parameters (needs resourceVersion out of band)
+    pub(crate) fn populate_qp(&self, qp: &mut form_urlencoded::Serializer<String>) {
+        if let Some(fields) = &self.field_selector {
+            qp.append_pair("fieldSelector", fields);
+        }
+        if let Some(labels) = &self.label_selector {
+            qp.append_pair("labelSelector", labels);
+        }
+        if let Some(limit) = &self.limit {
+            qp.append_pair("limit", &limit.to_string());
+        }
+        if let Some(continue_token) = &self.continue_token {
+            qp.append_pair("continue", continue_token);
+        }
+
+        if let Some(rv) = &self.resource_version {
+            qp.append_pair("resourceVersion", rv.as_str());
+        }
+        match &self.version_match {
+            None => {}
+            Some(VersionMatch::NotOlderThan) => {
+                qp.append_pair("resourceVersionMatch", "NotOlderThan");
+            }
+            Some(VersionMatch::Exact) => {
+                qp.append_pair("resourceVersionMatch", "Exact");
+            }
+        }
+    }
 }
 
 /// Builder interface to ListParams
@@ -253,6 +282,24 @@ impl WatchParams {
         }
         Ok(())
     }
+
+    // Partially populate query parameters (needs resourceVersion out of band)
+    pub(crate) fn populate_qp(&self, qp: &mut form_urlencoded::Serializer<String>) {
+        qp.append_pair("watch", "true");
+
+        // https://github.com/kubernetes/kubernetes/issues/6513
+        qp.append_pair("timeoutSeconds", &self.timeout.unwrap_or(290).to_string());
+
+        if let Some(fields) = &self.field_selector {
+            qp.append_pair("fieldSelector", fields);
+        }
+        if let Some(labels) = &self.label_selector {
+            qp.append_pair("labelSelector", labels);
+        }
+        if self.bookmarks {
+            qp.append_pair("allowWatchBookmarks", "true");
+        }
+    }
 }
 
 impl Default for WatchParams {

diff --git a/kube-core/src/request.rs b/kube-core/src/request.rs
@@ -1,7 +1,7 @@
 //! Request builder type for arbitrary api types
 use thiserror::Error;
 
-use super::params::{DeleteParams, ListParams, Patch, PatchParams, PostParams, VersionMatch, WatchParams};
+use super::params::{DeleteParams, ListParams, Patch, PatchParams, PostParams, WatchParams};
 
 pub(crate) const JSON_MIME: &str = "application/json";
 /// Extended Accept Header
@@ -58,33 +58,7 @@ impl Request {
         let target = format!("{}?", self.url_path);
         let mut qp = form_urlencoded::Serializer::new(target);
         lp.validate()?;
-
-        if let Some(fields) = &lp.field_selector {
-            qp.append_pair("fieldSelector", fields);
-        }
-        if let Some(labels) = &lp.label_selector {
-            qp.append_pair("labelSelector", labels);
-        }
-        if let Some(limit) = &lp.limit {
-            qp.append_pair("limit", &limit.to_string());
-        }
-        if let Some(continue_token) = &lp.continue_token {
-            qp.append_pair("continue", continue_token);
-        }
-
-        if let Some(rv) = &lp.resource_version {
-            qp.append_pair("resourceVersion", rv.as_str());
-        }
-        match &lp.version_match {
-            None => {}
-            Some(VersionMatch::NotOlderThan) => {
-                qp.append_pair("resourceVersionMatch", "NotOlderThan");
-            }
-            Some(VersionMatch::Exact) => {
-                qp.append_pair("resourceVersionMatch", "Exact");
-            }
-        }
-
+        lp.populate_qp(&mut qp);
         let urlstr = qp.finish();
         let req = http::Request::get(urlstr);
         req.body(vec![]).map_err(Error::BuildRequest)
@@ -95,22 +69,8 @@ impl Request {
         let target = format!("{}?", self.url_path);
         let mut qp = form_urlencoded::Serializer::new(target);
         wp.validate()?;
-
-        qp.append_pair("watch", "true");
+        wp.populate_qp(&mut qp);
         qp.append_pair("resourceVersion", ver);
-
-        // https://github.com/kubernetes/kubernetes/issues/6513
-        qp.append_pair("timeoutSeconds", &wp.timeout.unwrap_or(290).to_string());
-        if let Some(fields) = &wp.field_selector {
-            qp.append_pair("fieldSelector", fields);
-        }
-        if let Some(labels) = &wp.label_selector {
-            qp.append_pair("labelSelector", labels);
-        }
-        if wp.bookmarks {
-            qp.append_pair("allowWatchBookmarks", "true");
-        }
-
         let urlstr = qp.finish();
         let req = http::Request::get(urlstr);
         req.body(vec![]).map_err(Error::BuildRequest)
@@ -302,33 +262,7 @@ impl Request {
         let target = format!("{}?", self.url_path);
         let mut qp = form_urlencoded::Serializer::new(target);
         lp.validate()?;
-
-        if let Some(fields) = &lp.field_selector {
-            qp.append_pair("fieldSelector", fields);
-        }
-        if let Some(labels) = &lp.label_selector {
-            qp.append_pair("labelSelector", labels);
-        }
-        if let Some(limit) = &lp.limit {
-            qp.append_pair("limit", &limit.to_string());
-        }
-        if let Some(continue_token) = &lp.continue_token {
-            qp.append_pair("continue", continue_token);
-        }
-
-        if let Some(rv) = &lp.resource_version {
-            qp.append_pair("resourceVersion", rv.as_str());
-        }
-        match &lp.version_match {
-            None => {}
-            Some(VersionMatch::NotOlderThan) => {
-                qp.append_pair("resourceVersionMatch", "NotOlderThan");
-            }
-            Some(VersionMatch::Exact) => {
-                qp.append_pair("resourceVersionMatch", "Exact");
-            }
-        }
-
+        lp.populate_qp(&mut qp);
         let urlstr = qp.finish();
         let req = http::Request::get(urlstr)
             .header(http::header::ACCEPT, JSON_METADATA_LIST_MIME)
@@ -342,19 +276,9 @@ impl Request {
         let target = format!("{}?", self.url_path);
         let mut qp = form_urlencoded::Serializer::new(target);
         wp.validate()?;
-
-        qp.append_pair("watch", "true");
+        wp.populate_qp(&mut qp);
         qp.append_pair("resourceVersion", ver);
 
-        qp.append_pair("timeoutSeconds", &wp.timeout.unwrap_or(290).to_string());
-
-        if let Some(fields) = &wp.field_selector {
-            qp.append_pair("fieldSelector", fields);
-        }
-        if let Some(labels) = &wp.label_selector {
-            qp.append_pair("labelSelector", labels);
-        }
-
         let urlstr = qp.finish();
         http::Request::get(urlstr)
             .header(http::header::ACCEPT, JSON_METADATA_MIME)
@@ -548,7 +472,7 @@ mod test {
         let req = Request::new(url).watch(&wp, "0").unwrap();
         assert_eq!(
             req.uri(),
-            "/api/v1/namespaces/ns/pods?&watch=true&resourceVersion=0&timeoutSeconds=290&allowWatchBookmarks=true"
+            "/api/v1/namespaces/ns/pods?&watch=true&timeoutSeconds=290&allowWatchBookmarks=true&resourceVersion=0"
         );
     }
     #[test]
@@ -558,7 +482,7 @@ mod test {
         let req = Request::new(url).watch_metadata(&wp, "0").unwrap();
         assert_eq!(
             req.uri(),
-            "/api/v1/namespaces/ns/pods?&watch=true&resourceVersion=0&timeoutSeconds=290"
+            "/api/v1/namespaces/ns/pods?&watch=true&timeoutSeconds=290&allowWatchBookmarks=true&resourceVersion=0"
         );
         assert_eq!(req.headers().get(header::CONTENT_TYPE).unwrap(), super::JSON_MIME);
         assert_eq!(
@@ -792,10 +716,10 @@ mod test {
             .disable_bookmarks()
             .fields("metadata.name=pod=1")
             .labels("app=web");
-        let req = Request::new(url).watch(&wp, "").unwrap();
+        let req = Request::new(url).watch(&wp, "0").unwrap();
         assert_eq!(
             req.uri().query().unwrap(),
-            "&watch=true&resourceVersion=&timeoutSeconds=290&fieldSelector=metadata.name%3Dpod%3D1&labelSelector=app%3Dweb"
+            "&watch=true&timeoutSeconds=290&fieldSelector=metadata.name%3Dpod%3D1&labelSelector=app%3Dweb&resourceVersion=0"
         );
     }