From bbf67b09d108207b4a70d1a1310d6f54360029a7 Mon Sep 17 00:00:00 2001 From: Stepan Koltsov Date: Thu, 22 Mar 2018 04:46:44 +0000 Subject: [PATCH 1/2] Store unknown enum values into unknown fields This fix long standing issue of protobuf returning error when parsing unknown enum values. New behavior matches proto2 (even is code is generated for proto3). This is backward-incompatible change. Issue #233 --- protobuf-codegen/src/field.rs | 21 ++++ .../v2/test_enum_unknown_values_preserved.rs | 19 ++++ ...est_enum_unknown_values_preserved_pb.proto | 25 +++++ protobuf/src/rt.rs | 101 ++++++++++++++++++ 4 files changed, 166 insertions(+) create mode 100644 protobuf-test/src/common/v2/test_enum_unknown_values_preserved.rs create mode 100644 protobuf-test/src/common/v2/test_enum_unknown_values_preserved_pb.proto diff --git a/protobuf-codegen/src/field.rs b/protobuf-codegen/src/field.rs index b42004068..7e013dd87 100644 --- a/protobuf-codegen/src/field.rs +++ b/protobuf-codegen/src/field.rs @@ -1420,6 +1420,19 @@ impl<'a> FieldGen<'a> { FieldElem::Primitive(FieldDescriptorProto_Type::TYPE_BYTES, ..) => { self.write_merge_from_field_message_string_bytes(w); } + FieldElem::Enum(..) => { + let version = match field.flag { + SingularFieldFlag::WithFlag { .. } => "proto2", + SingularFieldFlag::WithoutFlag => "proto3", + }; + w.write_line(&format!( + "::protobuf::rt::read_{}_enum_with_unknown_fields_into({}, is, &mut self.{}, {}, &mut self.unknown_fields)?", + version, + wire_type_var, + self.rust_name, + self.proto_field.number() + )); + } _ => { let read_proc = format!("{}?", self.proto_type.read("is")); @@ -1443,6 +1456,14 @@ impl<'a> FieldGen<'a> { FieldElem::Primitive(FieldDescriptorProto_Type::TYPE_BYTES, ..) => { self.write_merge_from_field_message_string_bytes(w); } + FieldElem::Enum(..) => { + w.write_line(&format!( + "::protobuf::rt::read_repeated_enum_with_unknown_fields_into({}, is, &mut self.{}, {}, &mut self.unknown_fields)?", + wire_type_var, + self.rust_name, + self.proto_field.number() + )); + } _ => { w.write_line(&format!( "::protobuf::rt::read_repeated_{}_into({}, is, &mut self.{})?;", diff --git a/protobuf-test/src/common/v2/test_enum_unknown_values_preserved.rs b/protobuf-test/src/common/v2/test_enum_unknown_values_preserved.rs new file mode 100644 index 000000000..4e74c2b39 --- /dev/null +++ b/protobuf-test/src/common/v2/test_enum_unknown_values_preserved.rs @@ -0,0 +1,19 @@ +use super::test_enum_unknown_values_preserved_pb::*; + +use protobuf::*; +use test::*; + +#[test] +fn unknown_values_preserved() { + let mut new = NewMessage::new(); + new.set_eee(NewEnum::C); + + test_serialize_deserialize("08 1e", &new); + + // `OldEnum` doesn't have variant `C = 30`, + // but message still properly serialized and deserialized. + + let old: OldMessage = parse_from_bytes(&hex::decode_hex("08 1e")).expect("parse"); + + test_serialize_deserialize("08 1e", &old); +} diff --git a/protobuf-test/src/common/v2/test_enum_unknown_values_preserved_pb.proto b/protobuf-test/src/common/v2/test_enum_unknown_values_preserved_pb.proto new file mode 100644 index 000000000..91487ad41 --- /dev/null +++ b/protobuf-test/src/common/v2/test_enum_unknown_values_preserved_pb.proto @@ -0,0 +1,25 @@ +syntax = "proto2"; + +package unknown_values_preserved; + +enum OldEnum { + // Add _OLD suffix to field name to avoid collision with NewEnum + UNKNOWN_OLD = 0; + A_OLD = 10; + B_OLD = 20; +} + +enum NewEnum { + UNKNOWN = 0; + A = 10; + B = 20; + C = 30; +} + +message OldMessage { + optional OldEnum eee = 1; +} + +message NewMessage { + optional NewEnum eee = 1; +} diff --git a/protobuf/src/rt.rs b/protobuf/src/rt.rs index 41d523be4..d523a2607 100644 --- a/protobuf/src/rt.rs +++ b/protobuf/src/rt.rs @@ -472,6 +472,7 @@ pub fn read_repeated_bool_into( } /// Read repeated `enum` field into given vec. +/// This function is no longer called from generated code, remove in 1.5. pub fn read_repeated_enum_into( wire_type: WireType, is: &mut CodedInputStream, @@ -487,6 +488,106 @@ pub fn read_repeated_enum_into( } } +/// Helper function to read single enum value. +#[inline] +fn read_enum_with_unknown_fields_into( + is: &mut CodedInputStream, + target: C, + field_number: u32, + unknown_fields: &mut UnknownFields, +) -> ProtobufResult<()> + where C : FnOnce(E) +{ + let i = is.read_int32()?; + match ProtobufEnum::from_i32(i) { + Some(e) => target(e), + None => unknown_fields.add_varint(field_number, i as i64 as u64), + } + Ok(()) +} + +fn read_repeated_packed_enum_with_unknown_fields_into( + is: &mut CodedInputStream, + target: &mut Vec, + field_number: u32, + unknown_fields: &mut UnknownFields, +) -> ProtobufResult<()> { + let len = is.read_raw_varint64()?; + let old_limit = is.push_limit(len)?; + while !is.eof()? { + read_enum_with_unknown_fields_into(is, |e| target.push(e), field_number, unknown_fields)?; + } + is.pop_limit(old_limit); + Ok(()) +} + +/// Read repeated `enum` field into given vec, +/// and when value is unknown store it in unknown fields +/// which matches proto2 spec. +/// +/// See explanation +/// [here](https://github.com/stepancheg/rust-protobuf/issues/233#issuecomment-375142710) +pub fn read_repeated_enum_with_unknown_fields_into( + wire_type: WireType, + is: &mut CodedInputStream, + target: &mut Vec, + field_number: u32, + unknown_fields: &mut UnknownFields, +) -> ProtobufResult<()> { + match wire_type { + WireTypeLengthDelimited => { + read_repeated_packed_enum_with_unknown_fields_into( + is, target, field_number, unknown_fields) + }, + WireTypeVarint => { + read_enum_with_unknown_fields_into(is, |e| target.push(e), field_number, unknown_fields) + } + _ => Err(unexpected_wire_type(wire_type)), + } +} + +/// Read repeated `enum` field into given vec, +/// and when value is unknown store it in unknown fields +/// which matches proto2 spec. +/// +/// See explanation +/// [here](https://github.com/stepancheg/rust-protobuf/issues/233#issuecomment-375142710) +pub fn read_proto3_enum_with_unknown_fields_into( + wire_type: WireType, + is: &mut CodedInputStream, + target: &mut E, + field_number: u32, + unknown_fields: &mut UnknownFields, +) -> ProtobufResult<()> +{ + if wire_type != WireType::WireTypeVarint { + return Err(unexpected_wire_type(wire_type)); + } + + read_enum_with_unknown_fields_into(is, |e| { *target = e }, field_number, unknown_fields) +} + +/// Read repeated `enum` field into given vec, +/// and when value is unknown store it in unknown fields +/// which matches proto2 spec. +/// +/// See explanation +/// [here](https://github.com/stepancheg/rust-protobuf/issues/233#issuecomment-375142710) +pub fn read_proto2_enum_with_unknown_fields_into( + wire_type: WireType, + is: &mut CodedInputStream, + target: &mut Option, + field_number: u32, + unknown_fields: &mut UnknownFields, +) -> ProtobufResult<()> +{ + if wire_type != WireType::WireTypeVarint { + return Err(unexpected_wire_type(wire_type)); + } + + read_enum_with_unknown_fields_into(is, |e| { *target = Some(e) }, field_number, unknown_fields) +} + /// Read repeated `string` field into given vec. pub fn read_repeated_string_into( wire_type: WireType, From b9af4635e5dba2b2c9a0cb36fb88cf9542fd207f Mon Sep 17 00:00:00 2001 From: Stepan Koltsov Date: Thu, 22 Mar 2018 05:43:24 +0000 Subject: [PATCH 2/2] Regenerate bundled code after changes in unknown enum values handling Issue #233 --- protobuf/src/descriptor.rs | 30 ++++-------------------- protobuf/src/well_known_types/api.rs | 12 ++-------- protobuf/src/well_known_types/type_pb.rs | 24 ++++--------------- 3 files changed, 11 insertions(+), 55 deletions(-) diff --git a/protobuf/src/descriptor.rs b/protobuf/src/descriptor.rs index ab38e4296..9cc8540e0 100644 --- a/protobuf/src/descriptor.rs +++ b/protobuf/src/descriptor.rs @@ -2261,18 +2261,10 @@ impl ::protobuf::Message for FieldDescriptorProto { self.number = ::std::option::Option::Some(tmp); }, 4 => { - if wire_type != ::protobuf::wire_format::WireTypeVarint { - return ::std::result::Result::Err(::protobuf::rt::unexpected_wire_type(wire_type)); - } - let tmp = is.read_enum()?; - self.label = ::std::option::Option::Some(tmp); + ::protobuf::rt::read_proto2_enum_with_unknown_fields_into(wire_type, is, &mut self.label, 4, &mut self.unknown_fields)? }, 5 => { - if wire_type != ::protobuf::wire_format::WireTypeVarint { - return ::std::result::Result::Err(::protobuf::rt::unexpected_wire_type(wire_type)); - } - let tmp = is.read_enum()?; - self.field_type = ::std::option::Option::Some(tmp); + ::protobuf::rt::read_proto2_enum_with_unknown_fields_into(wire_type, is, &mut self.field_type, 5, &mut self.unknown_fields)? }, 6 => { ::protobuf::rt::read_singular_string_into(wire_type, is, &mut self.type_name)?; @@ -4582,11 +4574,7 @@ impl ::protobuf::Message for FileOptions { self.java_string_check_utf8 = ::std::option::Option::Some(tmp); }, 9 => { - if wire_type != ::protobuf::wire_format::WireTypeVarint { - return ::std::result::Result::Err(::protobuf::rt::unexpected_wire_type(wire_type)); - } - let tmp = is.read_enum()?; - self.optimize_for = ::std::option::Option::Some(tmp); + ::protobuf::rt::read_proto2_enum_with_unknown_fields_into(wire_type, is, &mut self.optimize_for, 9, &mut self.unknown_fields)? }, 11 => { ::protobuf::rt::read_singular_string_into(wire_type, is, &mut self.go_package)?; @@ -5469,11 +5457,7 @@ impl ::protobuf::Message for FieldOptions { let (field_number, wire_type) = is.read_tag_unpack()?; match field_number { 1 => { - if wire_type != ::protobuf::wire_format::WireTypeVarint { - return ::std::result::Result::Err(::protobuf::rt::unexpected_wire_type(wire_type)); - } - let tmp = is.read_enum()?; - self.ctype = ::std::option::Option::Some(tmp); + ::protobuf::rt::read_proto2_enum_with_unknown_fields_into(wire_type, is, &mut self.ctype, 1, &mut self.unknown_fields)? }, 2 => { if wire_type != ::protobuf::wire_format::WireTypeVarint { @@ -5483,11 +5467,7 @@ impl ::protobuf::Message for FieldOptions { self.packed = ::std::option::Option::Some(tmp); }, 6 => { - if wire_type != ::protobuf::wire_format::WireTypeVarint { - return ::std::result::Result::Err(::protobuf::rt::unexpected_wire_type(wire_type)); - } - let tmp = is.read_enum()?; - self.jstype = ::std::option::Option::Some(tmp); + ::protobuf::rt::read_proto2_enum_with_unknown_fields_into(wire_type, is, &mut self.jstype, 6, &mut self.unknown_fields)? }, 5 => { if wire_type != ::protobuf::wire_format::WireTypeVarint { diff --git a/protobuf/src/well_known_types/api.rs b/protobuf/src/well_known_types/api.rs index 9de128845..ec08956d4 100644 --- a/protobuf/src/well_known_types/api.rs +++ b/protobuf/src/well_known_types/api.rs @@ -275,11 +275,7 @@ impl ::protobuf::Message for Api { ::protobuf::rt::read_repeated_message_into(wire_type, is, &mut self.mixins)?; }, 7 => { - if wire_type != ::protobuf::wire_format::WireTypeVarint { - return ::std::result::Result::Err(::protobuf::rt::unexpected_wire_type(wire_type)); - } - let tmp = is.read_enum()?; - self.syntax = tmp; + ::protobuf::rt::read_proto3_enum_with_unknown_fields_into(wire_type, is, &mut self.syntax, 7, &mut self.unknown_fields)? }, _ => { ::protobuf::rt::read_unknown_or_skip_group(field_number, wire_type, is, self.mut_unknown_fields())?; @@ -685,11 +681,7 @@ impl ::protobuf::Message for Method { ::protobuf::rt::read_repeated_message_into(wire_type, is, &mut self.options)?; }, 7 => { - if wire_type != ::protobuf::wire_format::WireTypeVarint { - return ::std::result::Result::Err(::protobuf::rt::unexpected_wire_type(wire_type)); - } - let tmp = is.read_enum()?; - self.syntax = tmp; + ::protobuf::rt::read_proto3_enum_with_unknown_fields_into(wire_type, is, &mut self.syntax, 7, &mut self.unknown_fields)? }, _ => { ::protobuf::rt::read_unknown_or_skip_group(field_number, wire_type, is, self.mut_unknown_fields())?; diff --git a/protobuf/src/well_known_types/type_pb.rs b/protobuf/src/well_known_types/type_pb.rs index 2faba5ce7..c5a2db341 100644 --- a/protobuf/src/well_known_types/type_pb.rs +++ b/protobuf/src/well_known_types/type_pb.rs @@ -240,11 +240,7 @@ impl ::protobuf::Message for Type { ::protobuf::rt::read_singular_message_into(wire_type, is, &mut self.source_context)?; }, 6 => { - if wire_type != ::protobuf::wire_format::WireTypeVarint { - return ::std::result::Result::Err(::protobuf::rt::unexpected_wire_type(wire_type)); - } - let tmp = is.read_enum()?; - self.syntax = tmp; + ::protobuf::rt::read_proto3_enum_with_unknown_fields_into(wire_type, is, &mut self.syntax, 6, &mut self.unknown_fields)? }, _ => { ::protobuf::rt::read_unknown_or_skip_group(field_number, wire_type, is, self.mut_unknown_fields())?; @@ -668,18 +664,10 @@ impl ::protobuf::Message for Field { let (field_number, wire_type) = is.read_tag_unpack()?; match field_number { 1 => { - if wire_type != ::protobuf::wire_format::WireTypeVarint { - return ::std::result::Result::Err(::protobuf::rt::unexpected_wire_type(wire_type)); - } - let tmp = is.read_enum()?; - self.kind = tmp; + ::protobuf::rt::read_proto3_enum_with_unknown_fields_into(wire_type, is, &mut self.kind, 1, &mut self.unknown_fields)? }, 2 => { - if wire_type != ::protobuf::wire_format::WireTypeVarint { - return ::std::result::Result::Err(::protobuf::rt::unexpected_wire_type(wire_type)); - } - let tmp = is.read_enum()?; - self.cardinality = tmp; + ::protobuf::rt::read_proto3_enum_with_unknown_fields_into(wire_type, is, &mut self.cardinality, 2, &mut self.unknown_fields)? }, 3 => { if wire_type != ::protobuf::wire_format::WireTypeVarint { @@ -1285,11 +1273,7 @@ impl ::protobuf::Message for Enum { ::protobuf::rt::read_singular_message_into(wire_type, is, &mut self.source_context)?; }, 5 => { - if wire_type != ::protobuf::wire_format::WireTypeVarint { - return ::std::result::Result::Err(::protobuf::rt::unexpected_wire_type(wire_type)); - } - let tmp = is.read_enum()?; - self.syntax = tmp; + ::protobuf::rt::read_proto3_enum_with_unknown_fields_into(wire_type, is, &mut self.syntax, 5, &mut self.unknown_fields)? }, _ => { ::protobuf::rt::read_unknown_or_skip_group(field_number, wire_type, is, self.mut_unknown_fields())?;