Feat/optional query parameter values #411
Conversation
JP-Ellis
left a comment
JP-Ellis
left a comment
There was a problem hiding this comment.
If I understand correctly, the internal representations are:
| Query String | Key | Value |
|---|---|---|
foo=bar |
"foo" |
Some("bar") |
foo= |
"foo" |
Some("") |
foo |
"foo" |
None |
If so, I just noticed a few instances where unwrap_or_default() might be used a bit too eagerly, resulting in a loss of distinction between Some("") and None (see this Rust Playground demo)
I'm flagging them to make sure they are all intentional, and won't result in inconsistent behaviours across different parts of the codebase.
| /// | ||
| /// For a query parameter with no value, the value parameter can be set to a NULL pointer. | ||
| /// | ||
| /// # Safety | ||
| /// The name and value parameters must be valid pointers to NULL terminated strings. | ||
| /// The name parameter must be a valid pointer to a NULL terminated string. If the value | ||
| /// parameter is not NULL, it must point to a valid NULL terminated string. |
There was a problem hiding this comment.
I think it would be good to make the documentation more explicit, with perhaps some example usage to distinguish an "empty value" from a "null".
E.g.:
For query parameters with no value, two distinct formats are provided:
-
Parameters with blank values, as specified by
?foo=&bar=, require an empty string:pactffi_with_query_parameter_v2(handle, "foo", 0, ""); pactffi_with_query_parameter_v2(handle, "bar", 0, "");
-
Parameters with no associated valued, as specified by
?foo&bar, require aNULLpointer:pactffi_with_query_parameter_v2(handle, "foo", 0, NULL); pactffi_with_query_parameter_v2(handle, "bar", 0, NULL);
Note that the two formats are distinct; however, support for either variant can be provided through the integration JSON:
const char* value = "...";
pactffi_with_query_parameter_v2(handle, "foo", 0, value);There was a problem hiding this comment.
I will copy your example docs :-D
There was a problem hiding this comment.
Nice, the only one to fill out would be the last one (as I didn't bother filling out the Integration JSON text) 👍
There was a problem hiding this comment.
Integration JSON won't support it, it will always resolve to the empty string.
| let v = v.as_ref().map(|v| v.as_str()).unwrap_or_default(); | ||
| regex.is_match(v) |
There was a problem hiding this comment.
Is the loss of distinction intentional?
There was a problem hiding this comment.
This is in the compatibility-suite, it doesn't matter
| let value = val.clone().unwrap_or_default(); | ||
| if let Ok(v) = generator.generate_value(&value, context, &DefaultVariantMatcher.boxed()) { | ||
| generated[index] = Some(v); |
There was a problem hiding this comment.
As above, just double checking whether the loss of distinction between Some("") and None is intentional.
There was a problem hiding this comment.
Generator code, value doesn't matter.
| ) -> HashMap<String, Vec<Mismatch>> { | ||
| let mut result: HashMap<String, Vec<Mismatch>> = hashmap!{}; | ||
| for (key, value) in &expected { | ||
| let expected_value = value.iter().map(|v| v.clone().unwrap_or_default()).collect_vec(); |
There was a problem hiding this comment.
Just for display purposes, keeps previous behaviour
| match actual.get(key) { | ||
| Some(actual_value) => { | ||
| let mismatches: Result<(), Vec<super::Mismatch>> = match_query_values(key, value, actual_value, context) | ||
| let actual_value = actual_value.iter().map(|v| v.clone().unwrap_or_default()).collect_vec(); |
There was a problem hiding this comment.
Just for display purposes, keeps previous behaviour
| parameter: key.clone(), | ||
| expected: "".to_string(), | ||
| actual: format!("{:?}", value), | ||
| actual: format!("{:?}", value.iter().map(|v| v.clone().unwrap_or_default()).collect_vec()), |
There was a problem hiding this comment.
For debugging, it probably would be best to distinguish Some("") from None.
There was a problem hiding this comment.
Just for display purposes, keeps previous behavior. The expected and actual values are not actually used by anything.
Supports optional query parameter values. With this change we can support:
name=value- query parameter with a valuename=- query parameter with empty valuename- query parameter with no value