From d6b1626aaba24492a74dd3c637b238c3eed19c5d Mon Sep 17 00:00:00 2001 From: Ronald Holshausen Date: Mon, 18 Nov 2024 10:21:10 +1100 Subject: [PATCH] fix: Fixed regression introduced by 0.5.3 release --- src/dynamic_message.rs | 19 ++++- src/matching.rs | 16 ++-- src/message_decoder/mod.rs | 18 ++++- src/mock_service.rs | 151 ++++++++++++++++++++++++++++++++++++- 4 files changed, 190 insertions(+), 14 deletions(-) diff --git a/src/dynamic_message.rs b/src/dynamic_message.rs index 129c10d..b3c2891 100644 --- a/src/dynamic_message.rs +++ b/src/dynamic_message.rs @@ -5,7 +5,7 @@ use std::iter::Peekable; use std::slice::Iter; use anyhow::{anyhow, bail}; -use bytes::{BufMut, Bytes}; +use bytes::{Buf, BufMut, Bytes}; use itertools::Itertools; use pact_matching::generators::DefaultVariantMatcher; use pact_models::expression_parser::DataValue; @@ -108,6 +108,20 @@ impl DynamicMessage { self.fields.values().cloned().collect() } + /// Return a flattened vector of the fields. This will expand repeated fields. + pub fn flatten_fields(&self) -> Vec { + self.fields.values() + .flat_map(|f| { + let mut result = vec![ f.clone() ]; + if f.repeated_field() && !f.additional_data.is_empty() { + result.extend(f.additional_data.iter() + .map(|d| f.clone_with_data(d))); + } + result + }) + .collect() + } + /// Encode this message to the provided buffer pub fn write_to(&self, buffer: &mut B) -> anyhow::Result<()> where B: BufMut { for (field_num, field) in self.fields.iter() @@ -416,8 +430,9 @@ impl Decoder for DynamicMessageDecoder { type Item = DynamicMessage; type Error = Status; - #[instrument] + #[instrument(skip_all, fields(bytes = src.remaining()))] fn decode(&mut self, src: &mut DecodeBuf<'_>) -> Result, Self::Error> { + trace!("Incoming bytes = {:?}", src); match decode_message(src, &self.descriptor, &self.file_descriptor_set) { Ok(fields) => Ok(Some(DynamicMessage::new(fields.as_slice(), &self.file_descriptor_set))), Err(err) => { diff --git a/src/matching.rs b/src/matching.rs index b3e280f..4594cd6 100644 --- a/src/matching.rs +++ b/src/matching.rs @@ -142,7 +142,7 @@ pub fn match_service( } /// Compare the expected message to the actual one -#[tracing::instrument(ret, skip_all)] +#[tracing::instrument(ret, skip_all, fields(expected_fields = expected_message.len(), actual_fields = actual_message.len()) )] pub(crate) fn compare( message_descriptor: &DescriptorProto, expected_message: &[ProtobufField], @@ -205,7 +205,7 @@ fn should_use_default(descriptor: &FieldDescriptorProto) -> bool { /// Compare the fields of the expected and actual messages #[tracing::instrument(ret, skip_all, - fields(%path, expected_message_fields, actual_message_fields) + fields(%path, expected_fields = expected_message_fields.len(), actual_fields=actual_message_fields.len()) )] pub fn compare_message( path: DocPath, @@ -565,7 +565,7 @@ fn compare_value( } /// Compare a repeated field -#[tracing::instrument(ret, skip_all, fields(%path))] +#[tracing::instrument(ret, skip_all, fields(%path, expected = expected_fields.len(), actual = actual_fields.len()))] fn compare_repeated_field( path: &DocPath, descriptor: &FieldDescriptorProto, @@ -595,12 +595,12 @@ fn compare_repeated_field( } } } else if expected_fields.is_empty() && !actual_fields.is_empty() { - debug!("Expected an empty list, but actual has {} fields", actual_fields.len()); + debug!("Expected an empty list, but actual has {} field(s)", actual_fields.len()); result.push(Mismatch::BodyMismatch { path: path.to_string(), expected: None, actual: None, - mismatch: format!("Expected repeated field '{}' to be empty but received {} values", + mismatch: format!("Expected repeated field '{}' to be empty but received {} value(s)", descriptor.name.clone().unwrap_or_else(|| descriptor.number.unwrap_or_default().to_string()), actual_fields.len() ) @@ -613,10 +613,12 @@ fn compare_repeated_field( path: path.to_string(), expected: None, actual: None, - mismatch: format!("Expected repeated field '{}' to have {} values but received {} values", + mismatch: format!("Expected repeated field '{}' to have {} value{} but received {} value{}", descriptor.name.clone().unwrap_or_else(|| descriptor.number.unwrap_or_default().to_string()), expected_fields.len(), - actual_fields.len() + if expected_fields.len() > 1 { "s" } else { "" }, + actual_fields.len(), + if actual_fields.len() > 1 { "s" } else { "" } ) }) } diff --git a/src/message_decoder/mod.rs b/src/message_decoder/mod.rs index 25a3201..ffe291e 100644 --- a/src/message_decoder/mod.rs +++ b/src/message_decoder/mod.rs @@ -75,6 +75,15 @@ impl ProtobufField { pub fn repeated_field(&self) -> bool { is_repeated_field(&self.descriptor) } + + /// Creates a new field that is a clone of this one but with the data set + pub fn clone_with_data(&self, data: &ProtobufFieldData) -> Self { + ProtobufField { + data: data.clone(), + additional_data: vec![], + .. self.clone() + } + } } fn default_field_data( @@ -389,7 +398,6 @@ impl Display for ProtobufFieldData { /// Decodes the Protobuf message using the descriptors and returns an array of ProtobufField values. /// This will return a value for each field value in the incoming bytes in the same order, and will /// not consolidate repeated fields. -#[tracing::instrument(ret, skip_all)] pub fn decode_message( buffer: &mut B, descriptor: &DescriptorProto, @@ -550,7 +558,13 @@ pub fn decode_message( } } - Ok(fields.iter().sorted_by(|a, b| Ord::cmp(&a.field_num, &b.field_num)).cloned().collect()) + let result = fields.iter() + .sorted_by(|a, b| Ord::cmp(&a.field_num, &b.field_num)) + .cloned() + .collect_vec(); + debug!("Decoded message has {} fields", result.len()); + trace!("Decoded message = {:?}", result); + Ok(result) } fn decode_enum( diff --git a/src/mock_service.rs b/src/mock_service.rs index d99350b..f0c5030 100644 --- a/src/mock_service.rs +++ b/src/mock_service.rs @@ -61,10 +61,12 @@ impl MockService { response_descriptor: DescriptorProto, request_metadata: MetadataMap ) -> Result, Status> { + trace!(?request, "Handling request message"); // 1. Compare the incoming message to the request message from the interaction let mut expected_message_bytes = self.message.request.contents.value().unwrap_or_default(); let expected_message = decode_message(&mut expected_message_bytes, &message_descriptor, &self.file_descriptor_set) .map_err(|err| Status::invalid_argument(err.to_string()))?; + trace!("Expected message has {} fields", expected_message.len()); let plugin_config = self.pact.plugin_data().iter() .map(|pd| { (pd.name.clone(), PluginInteractionConfig { @@ -76,8 +78,14 @@ impl MockService { let context = CoreMatchingContext::new(DiffConfig::NoUnexpectedKeys, &self.message.request.matching_rules.rules_for_category("body").unwrap_or_default(), &plugin_config); - let mismatches = compare(&message_descriptor, &expected_message, request.proto_fields().as_slice(), &context, - &expected_message_bytes, &self.file_descriptor_set); + let mismatches = compare( + &message_descriptor, + &expected_message, + request.flatten_fields().as_slice(), + &context, + &expected_message_bytes, + &self.file_descriptor_set + ); // 2. Compare any metadata from the incoming message let md_context = CoreMatchingContext::new(DiffConfig::NoUnexpectedKeys, @@ -231,6 +239,7 @@ impl Service> for MockService { fn call(&mut self, req: Request) -> Self::Future { let (request_metadata, _, request) = req.into_parts(); + trace!(?request, "Incoming message received"); let message_descriptor = self.input_message.clone(); let response_descriptor = self.output_message.clone(); let service = self.clone(); @@ -250,7 +259,7 @@ mod tests { use prost::Message; use prost_types::FileDescriptorSet; use serde_json::json; - use tonic::metadata::MetadataMap; + use tonic::metadata::{MetadataMap, MetadataKey, MetadataValue}; use crate::dynamic_message::DynamicMessage; use crate::message_decoder::decode_message; @@ -385,4 +394,140 @@ mod tests { let area = &response_fields[0]; expect!(area.data.to_string()).to_not(be_equal_to("12")); } + + #[test_log::test(tokio::test)] + async fn handle_message_handles_multiple_field_values() { + // taken from https://github.com/pact-foundation/pact-plugins/tree/main/examples/gRPC/area_calculator + let descriptor_encoded = "CsoHChVhcmVhX2NhbGN1bGF0b3IucHJvdG8SD2FyZWFfY2FsY3VsYXRvciK6Ago\ + MU2hhcGVNZXNzYWdlEjEKBnNxdWFyZRgBIAEoCzIXLmFyZWFfY2FsY3VsYXRvci5TcXVhcmVIAFIGc3F1YXJlEjoKC\ + XJlY3RhbmdsZRgCIAEoCzIaLmFyZWFfY2FsY3VsYXRvci5SZWN0YW5nbGVIAFIJcmVjdGFuZ2xlEjEKBmNpcmNsZRg\ + DIAEoCzIXLmFyZWFfY2FsY3VsYXRvci5DaXJjbGVIAFIGY2lyY2xlEjcKCHRyaWFuZ2xlGAQgASgLMhkuYXJlYV9jY\ + WxjdWxhdG9yLlRyaWFuZ2xlSABSCHRyaWFuZ2xlEkYKDXBhcmFsbGVsb2dyYW0YBSABKAsyHi5hcmVhX2NhbGN1bGF\ + 0b3IuUGFyYWxsZWxvZ3JhbUgAUg1wYXJhbGxlbG9ncmFtQgcKBXNoYXBlIikKBlNxdWFyZRIfCgtlZGdlX2xlbmd0a\ + BgBIAEoAlIKZWRnZUxlbmd0aCI5CglSZWN0YW5nbGUSFgoGbGVuZ3RoGAEgASgCUgZsZW5ndGgSFAoFd2lkdGgYAiA\ + BKAJSBXdpZHRoIiAKBkNpcmNsZRIWCgZyYWRpdXMYASABKAJSBnJhZGl1cyJPCghUcmlhbmdsZRIVCgZlZGdlX2EYAS\ + ABKAJSBWVkZ2VBEhUKBmVkZ2VfYhgCIAEoAlIFZWRnZUISFQoGZWRnZV9jGAMgASgCUgVlZGdlQyJICg1QYXJhbGxlbG\ + 9ncmFtEh8KC2Jhc2VfbGVuZ3RoGAEgASgCUgpiYXNlTGVuZ3RoEhYKBmhlaWdodBgCIAEoAlIGaGVpZ2h0IkQKC0FyZW\ + FSZXF1ZXN0EjUKBnNoYXBlcxgBIAMoCzIdLmFyZWFfY2FsY3VsYXRvci5TaGFwZU1lc3NhZ2VSBnNoYXBlcyIkCgxBcm\ + VhUmVzcG9uc2USFAoFdmFsdWUYASADKAJSBXZhbHVlMq0BCgpDYWxjdWxhdG9yEk4KDGNhbGN1bGF0ZU9uZRIdLmFyZW\ + FfY2FsY3VsYXRvci5TaGFwZU1lc3NhZ2UaHS5hcmVhX2NhbGN1bGF0b3IuQXJlYVJlc3BvbnNlIgASTwoOY2FsY3VsY\ + XRlTXVsdGkSHC5hcmVhX2NhbGN1bGF0b3IuQXJlYVJlcXVlc3QaHS5hcmVhX2NhbGN1bGF0b3IuQXJlYVJlc3BvbnNl\ + IgBCHFoXaW8ucGFjdC9hcmVhX2NhbGN1bGF0b3LQAgFiBnByb3RvMw=="; + let bytes = BASE64.decode(descriptor_encoded).unwrap(); + let bytes1 = Bytes::copy_from_slice(bytes.as_slice()); + let file_descriptor_set = FileDescriptorSet::decode(bytes1).unwrap(); + + let ac_desc = file_descriptor_set.file.iter() + .find(|ds| ds.name.clone().unwrap_or_default() == "area_calculator.proto") + .cloned() + .unwrap(); + let service_desc = ac_desc.service.iter() + .find(|sd| sd.name.clone().unwrap_or_default() == "Calculator") + .unwrap(); + let method = service_desc.method.iter() + .find(|md| md.name.clone().unwrap_or_default() == "calculateMulti") + .unwrap(); + let input_message = ac_desc.message_type.iter() + .find(|md| md.name.clone().unwrap_or_default() == "AreaRequest") + .unwrap(); + let output_message = ac_desc.message_type.iter() + .find(|md| md.name.clone().unwrap_or_default() == "AreaResponse") + .unwrap(); + + let request_bytes: &[u8] = [10, 12, 18, 10, 13, 0, 0, 64, 64, 21, 0, 0, 128, 64, 10, 7, 10, 5, 13, 0, 0, 64, 64].as_slice(); + let pact_json = json!({ + "interactions": [ + { + "type": "Synchronous/Messages", + "description": "calculate rectangle area request", + "request": { + "contents": { + "content": BASE64.encode(request_bytes), + "contentType": "application/protobuf; message=AreaRequest", + "contentTypeHint": "BINARY", + "encoded": "base64" + }, + "metadata": { + "contentType": "application/protobuf;message=.area_calculator.AreaRequest" + }, + "matchingRules": { + "body": { + "$.shapes[0].rectangle.length": { + "combine": "AND", + "matchers": [ + { + "match": "number" + } + ] + }, + "$.shapes[0].rectangle.width": { + "combine": "AND", + "matchers": [ + { + "match": "number" + } + ] + }, + "$.shapes[1].square.edge_length": { + "combine": "AND", + "matchers": [ + { + "match": "number" + } + ] + } + } + } + }, + "response": [ + { + "contents": { + "content": "CgQAAEBB", + "contentType": "application/protobuf; message=.area_calculator.AreaResponse", + "contentTypeHint": "BINARY", + "encoded": "base64" + }, + "metadata": { + "contentType": "application/protobuf;message=.area_calculator.AreaResponse" + } + } + ], + "pluginConfiguration": { + "protobuf": { + "descriptorKey": "d58838959e37498cddf51805bedf4dca", + "service": ".area_calculator.Calculator/calculateMulti" + } + }, + "transport": "grpc" + } + ], + "metadata": { + "pactSpecification": { "version": "4.0" } + } + }); + + let pact = V4Pact::pact_from_json(&pact_json, "<>").unwrap(); + let message = pact.interactions.first().unwrap(); + + let mut bytes2 = Bytes::from(b"\n\x0c\x12\n\r\0\0@@\x15\0\0\x80@\n\x07\n\x05\r\0\0@@".as_slice()); + let fields = decode_message(&mut bytes2, input_message, &file_descriptor_set).unwrap(); + let request = DynamicMessage::new(fields.as_slice(), &file_descriptor_set); + + let mock_service = MockService { + file_descriptor_set: file_descriptor_set.clone(), + service_name: "Calculator".to_string(), + message: message.as_v4_sync_message().unwrap(), + method_descriptor: method.clone(), + input_message: input_message.clone(), + output_message: output_message.clone(), + server_key: "9876789".to_string(), + pact + }; + + let mut md = MetadataMap::new(); + md.insert(MetadataKey::from_static("contenttype"), MetadataValue::from_static("application/protobuf;message=.area_calculator.AreaRequest")); + let response = mock_service.handle_message(request, input_message.clone(), output_message.clone(), + md).await; + expect!(response).to(be_ok()); + } }