feat(pact_verifier_cli): Parse consumer version selector JSON when ev…

…aluating CLI args so errors are reported earlier
pact-foundation · Jul 10, 2024 · f03cd0d · f03cd0d
1 parent efa1295
commit f03cd0d
Show file tree

Hide file tree

Showing 5 changed files with 98 additions and 13 deletions.
diff --git a/rust/pact_ffi/src/verifier/mod.rs b/rust/pact_ffi/src/verifier/mod.rs
@@ -547,6 +547,9 @@ ffi_fn! {
     /// If a username and password is given, then basic authentication will be used when fetching
     /// the pact file. If a token is provided, then bearer token authentication will be used.
     ///
+    /// This function will return zero unless any of the consumer version selectors are not valid
+    /// JSON, in which case, it will return -1.
+    ///
     /// # Safety
     ///
     /// All string fields must contain valid UTF-8. Invalid UTF-8
@@ -567,7 +570,7 @@ ffi_fn! {
       consumer_version_selectors_len: c_ushort,
       consumer_version_tags: *const *const c_char,
       consumer_version_tags_len: c_ushort
-    ) {
+    ) -> c_int {
       let handle = as_mut!(handle);
       let url = safe_str!(url);
       let provider_branch: Option<String> = if provider_branch.is_null() {
@@ -604,15 +607,33 @@ ffi_fn! {
       let consumer_version_tags_vector = get_vector(consumer_version_tags, consumer_version_tags_len);
 
       let selectors = if consumer_version_selectors_vector.len() > 0 {
-        json_to_selectors(consumer_version_selectors_vector.iter().map(|s| &**s).collect())
+        let mut selectors = vec![];
+        let mut errors = false;
+        for s in consumer_version_selectors_vector {
+          match serde_json::from_str(s.as_str()) {
+            Ok(cvs) => selectors.push(cvs),
+            Err(err) => {
+              error!("Failed to parse consumer version selector '{}' as JSON: {}", s, err);
+              errors = true;
+            }
+          }
+        }
+
+        if errors {
+          return Ok(-1);
+        }
+
+        json_to_selectors(selectors)
       } else if consumer_version_tags_vector.len() > 0 {
         consumer_tags_to_selectors(consumer_version_tags_vector.iter().map(|s| &**s).collect())
       } else {
         vec![]
       };
 
       handle.add_pact_broker_source(url, enable_pending > 0, wip, provider_tags_vector, provider_branch, selectors, &auth);
-    }
+
+      0
+    } { -2 }
 }
 
 ffi_fn! {
@@ -902,6 +923,7 @@ ffi_fn! {
 #[cfg(test)]
 mod tests {
   use std::ffi::CString;
+  use std::ptr::null;
 
   use expectest::prelude::*;
   use libc::c_char;
@@ -956,4 +978,58 @@ We are tracking events anonymously to gather important usage statistics like Pac
 1) Verifying a pact between test_consumer and test_provider Given test state - test interaction - error sending request for url (http://localhost/): error trying to connect: tcp connect error: Connection refused (os error 111)\n\
 \n\nThere were 1 pact failures\n\n"));
   }
+
+  #[test]
+  fn pactffi_verifier_broker_source_with_selectors_test() {
+    let mut handle = VerifierHandle::new_for_application("test", "0.0.0");
+    let url = CString::new("http://127.0.0.1:8080").unwrap();
+    let provider_branch = CString::new("main").unwrap();
+    let cvs_1 = CString::new(r#"{"mainBranch":true}"#).unwrap();
+    let cvs_2 = CString::new(r#"{"deployedOrReleased":true}"#).unwrap();
+    let consumer_version_selectors = [ cvs_1.as_ptr(), cvs_2.as_ptr() ];
+    let result = super::pactffi_verifier_broker_source_with_selectors(
+      &mut handle,
+      url.as_ptr(),
+      null(),
+      null(),
+      null(),
+      0,
+      null(),
+      null(),
+      0,
+      provider_branch.as_ptr(),
+      consumer_version_selectors.as_ptr(),
+      2,
+      null(),
+      0
+    );
+    expect!(result).to(be_equal_to(0));
+  }
+
+  #[test_log::test]
+  fn pactffi_verifier_broker_source_with_selectors_error_test() {
+    let mut handle = VerifierHandle::new_for_application("test", "0.0.0");
+    let url = CString::new("http://127.0.0.1:8080").unwrap();
+    let provider_branch = CString::new("main").unwrap();
+    let cvs_1 = CString::new(r#"{"mainBranch":true"#).unwrap();
+    let cvs_2 = CString::new(r#"{"deployedOrReleased:true}"#).unwrap();
+    let consumer_version_selectors = [ cvs_1.as_ptr(), cvs_2.as_ptr() ];
+    let result = super::pactffi_verifier_broker_source_with_selectors(
+      &mut handle,
+      url.as_ptr(),
+      null(),
+      null(),
+      null(),
+      0,
+      null(),
+      null(),
+      0,
+      provider_branch.as_ptr(),
+      consumer_version_selectors.as_ptr(),
+      2,
+      null(),
+      0
+    );
+    expect!(result).to(be_equal_to(-1));
+  }
 }
diff --git a/rust/pact_ffi/src/verifier/verifier.rs b/rust/pact_ffi/src/verifier/verifier.rs
@@ -7,6 +7,7 @@ use std::sync::Arc;
 
 use clap::{AppSettings, ArgMatches, ErrorKind};
 use maplit::hashmap;
+use serde_json::Value;
 use pact_models::http_utils::HttpAuth;
 use pact_models::PactSpecification;
 use tracing::{debug, error, warn};
@@ -60,7 +61,10 @@ fn pact_source(matches: &ArgMatches) -> Vec<PactSource> {
       let provider_branch = matches.value_of("provider-branch").map(|v| v.to_string());
       let selectors = if matches.is_present("consumer-version-selectors") {
         matches.values_of("consumer-version-selectors")
-          .map_or_else(Vec::new, |s| json_to_selectors(s.collect::<Vec<_>>()))
+          .map_or_else(Vec::new, |s| json_to_selectors(s
+            .map(|cvs| serde_json::from_str::<Value>(cvs))
+            .flatten()
+            .collect::<Vec<_>>()))
       } else if matches.is_present("consumer-version-tags") {
         matches.values_of("consumer-version-tags")
           .map_or_else(Vec::new, |tags| consumer_tags_to_selectors(tags.collect::<Vec<_>>()))

diff --git a/rust/pact_verifier/src/selectors.rs b/rust/pact_verifier/src/selectors.rs
@@ -1,10 +1,12 @@
 //! Module to deal with consumer version selectors
 
+use serde_json::{from_value, Value};
+
 use crate::ConsumerVersionSelector;
 
-/// Parses a vector of JSON strings into a vector of consumer version selectors
-pub fn json_to_selectors(tags: Vec<&str>) -> Vec<ConsumerVersionSelector> {
-  tags.iter().map(|t| serde_json::from_str(t))
+/// Parses a vector of JSON into a vector of consumer version selectors
+pub fn json_to_selectors(json: Vec<Value>) -> Vec<ConsumerVersionSelector> {
+  json.iter().map(|t| from_value(t.clone()))
     .flatten()
     .collect()
 }

diff --git a/rust/pact_verifier_cli/src/args.rs b/rust/pact_verifier_cli/src/args.rs
@@ -2,6 +2,7 @@ use clap::{Arg, ArgAction, ArgGroup, Command, command};
 use clap::builder::{FalseyValueParser, NonEmptyStringValueParser, PossibleValuesParser};
 use lazy_static::lazy_static;
 use regex::Regex;
+use serde_json::Value;
 
 fn port_value(v: &str) -> Result<u16, String> {
   v.parse::<u16>().map_err(|e| format!("'{}' is not a valid port value: {}", v, e) )
@@ -21,6 +22,10 @@ fn validate_regex(val: &str) -> Result<String, String> {
   }
 }
 
+fn json_value(v: &str) -> Result<Value, String> {
+  serde_json::from_str(v).map_err(|err| format!("'{}' is not valid JSON: {}", v, err))
+}
+
 lazy_static! {
   static ref TRANSPORT_VALUE_RE: Regex = Regex::new(r#"^(\w+):(\d+)(\/[^\s]*)?$"#).unwrap();
 }
@@ -223,7 +228,6 @@ pub(crate) fn setup_app() -> Command {
     .arg(Arg::new("custom-header")
       .long("header")
       .short('H')
-      .action(ArgAction::Set)
       .action(ArgAction::Append)
       .value_parser(NonEmptyStringValueParser::new())
       .help("Add a custom header to be included in the calls to the provider. Values must be in the form KEY=VALUE, where KEY and VALUE contain ASCII characters (32-127) only. Can be repeated."))
@@ -278,7 +282,6 @@ pub(crate) fn setup_app() -> Command {
     .arg(Arg::new("filter-consumer")
       .short('c')
       .long("filter-consumer")
-      .action(ArgAction::Set)
       .action(ArgAction::Append)
       .value_parser(NonEmptyStringValueParser::new())
       .help("Consumer name to filter the pacts to be verified (can be repeated)"))
@@ -325,9 +328,8 @@ pub(crate) fn setup_app() -> Command {
       .help("Consumer tags to use when fetching pacts from the Broker. Accepts comma-separated values."))
     .arg(Arg::new("consumer-version-selectors")
       .long("consumer-version-selectors")
-      .action(ArgAction::Set)
       .action(ArgAction::Append)
-      .value_parser(NonEmptyStringValueParser::new())
+      .value_parser(json_value)
       .requires("broker-url")
       .conflicts_with("consumer-version-tags")
       .help("Consumer version selectors to use when fetching pacts from the Broker. Accepts a JSON string as per https://docs.pact.io/pact_broker/advanced_topics/consumer_version_selectors/. Can be repeated."))

diff --git a/rust/pact_verifier_cli/src/main.rs b/rust/pact_verifier_cli/src/main.rs
@@ -400,6 +400,7 @@ use clap::ArgMatches;
 use clap::error::ErrorKind;
 use log::{LevelFilter};
 use maplit::hashmap;
+use serde_json::Value;
 use pact_models::{PACT_RUST_VERSION, PactSpecification};
 use pact_models::prelude::HttpAuth;
 use tokio::time::sleep;
@@ -689,8 +690,8 @@ fn pact_source(matches: &ArgMatches) -> Vec<PactSource> {
         let provider_branch = matches.get_one::<String>("provider-branch").cloned();
 
         let selectors = if matches.contains_id("consumer-version-selectors") {
-          matches.get_many::<String>("consumer-version-selectors")
-            .map_or_else(Vec::new, |s| json_to_selectors(s.map(|v| v.as_str()).collect::<Vec<_>>()))
+          matches.get_many::<Value>("consumer-version-selectors")
+            .map_or_else(Vec::new, |s| json_to_selectors(s.into_iter().cloned().collect::<Vec<_>>()))
         } else if matches.contains_id("consumer-version-tags") {
           matches.get_many::<String>("consumer-version-tags")
             .map_or_else(Vec::new, |tags| consumer_tags_to_selectors(tags.map(|v| v.as_str()).collect::<Vec<_>>()))