fix: Resolve Envoy xDS proto issues (#103)

* fix: Escape Self since it is a reserved keyword * fix: Allow duplicate numbers in enums when aliases are enabled * fix: Avoid field name collisions with the map ident
influxdata · Sep 17, 2023 · 688c8b4 · 688c8b4
1 parent fc231ac
commit 688c8b4
Show file tree

Hide file tree

Showing 8 changed files with 69 additions and 28 deletions.
diff --git a/pbjson-build/src/descriptor.rs b/pbjson-build/src/descriptor.rs
@@ -12,7 +12,7 @@ use prost_types::{
     FileDescriptorProto, FileDescriptorSet, MessageOptions, OneofDescriptorProto,
 };
 
-use crate::escape::escape_ident;
+use crate::escape::{escape_ident, escape_type};
 
 #[derive(Debug, Clone, Ord, PartialOrd, Eq, PartialEq, Hash)]
 pub struct Package {
@@ -75,7 +75,7 @@ impl TypeName {
 
     pub fn to_upper_camel_case(&self) -> String {
         use heck::ToUpperCamelCase;
-        self.0.to_upper_camel_case()
+        escape_type(self.0.to_upper_camel_case())
     }
 }
 

diff --git a/pbjson-build/src/escape.rs b/pbjson-build/src/escape.rs
@@ -24,3 +24,11 @@ pub fn escape_ident(mut ident: String) -> String {
     };
     ident
 }
+
+pub fn escape_type(mut ident: String) -> String {
+    // this keyword is not supported as a raw identifier and is therefore suffixed with an underscore.
+    if ident == "Self" {
+        ident += "_";
+    }
+    ident
+}
diff --git a/pbjson-build/src/generator/enumeration.rs b/pbjson-build/src/generator/enumeration.rs
@@ -11,6 +11,7 @@ use super::{
 use crate::descriptor::{EnumDescriptor, TypePath};
 use crate::generator::write_fields_array;
 use crate::resolver::Resolver;
+use std::collections::HashSet;
 use std::io::{Result, Write};
 
 pub fn generate_enum<W: Write>(
@@ -22,9 +23,13 @@ pub fn generate_enum<W: Write>(
 ) -> Result<()> {
     let rust_type = resolver.rust_type(path);
 
+    let mut seen_numbers = HashSet::new();
     let variants: Vec<_> = descriptor
         .values
         .iter()
+        // Skip duplicates if we've seen the number before
+        // Protobuf's `allow_alias` option permits duplicates if set
+        .filter(|variant| seen_numbers.insert(variant.number()))
         .map(|variant| {
             let variant_name = variant.name.clone().unwrap();
             let variant_number = variant.number();

diff --git a/pbjson-build/src/generator/message.rs b/pbjson-build/src/generator/message.rs
@@ -30,6 +30,7 @@ use super::{
     Indent,
 };
 use crate::descriptor::TypePath;
+use crate::escape::escape_type;
 use crate::generator::write_fields_array;
 use crate::resolver::Resolver;
 
@@ -522,7 +523,7 @@ fn write_deserialize_message<W: Write>(
 {indent}        formatter.write_str("struct {name}")
 {indent}    }}
 
-{indent}    fn visit_map<V>(self, mut map: V) -> std::result::Result<{rust_type}, V::Error>
+{indent}    fn visit_map<V>(self, mut map_: V) -> std::result::Result<{rust_type}, V::Error>
 {indent}        where
 {indent}            V: serde::de::MapAccess<'de>,
 {indent}    {{"#,
@@ -552,7 +553,7 @@ fn write_deserialize_message<W: Write>(
     if !message.fields.is_empty() || !message.one_ofs.is_empty() {
         writeln!(
             writer,
-            "{}while let Some(k) = map.next_key()? {{",
+            "{}while let Some(k) = map_.next_key()? {{",
             Indent(indent + 2)
         )?;
 
@@ -587,7 +588,7 @@ fn write_deserialize_message<W: Write>(
             )?;
             writeln!(
                 writer,
-                "{}let _ = map.next_value::<serde::de::IgnoredAny>()?;",
+                "{}let _ = map_.next_value::<serde::de::IgnoredAny>()?;",
                 Indent(indent + 5),
             )?;
             writeln!(writer, "{}}}", Indent(indent + 4))?;
@@ -598,12 +599,12 @@ fn write_deserialize_message<W: Write>(
     } else {
         writeln!(
             writer,
-            "{}while map.next_key::<GeneratedField>()?.is_some() {{",
+            "{}while map_.next_key::<GeneratedField>()?.is_some() {{",
             Indent(indent + 2)
         )?;
         writeln!(
             writer,
-            "{}let _ = map.next_value::<serde::de::IgnoredAny>()?;",
+            "{}let _ = map_.next_value::<serde::de::IgnoredAny>()?;",
             Indent(indent + 3)
         )?;
         writeln!(writer, "{}}}", Indent(indent + 2))?;
@@ -725,15 +726,15 @@ fn write_deserialize_field_name<W: Write>(
                     Indent(indent + 5),
                     json_name,
                     proto_name,
-                    type_name
+                    escape_type(type_name.to_string())
                 )?;
             } else {
                 writeln!(
                     writer,
                     "{}\"{}\" => Ok(GeneratedField::{}),",
                     Indent(indent + 5),
                     json_name,
-                    type_name
+                    escape_type(type_name.to_string())
                 )?;
             }
         }
@@ -789,7 +790,12 @@ fn write_fields_enum<'a, W: Write, I: Iterator<Item = &'a str>>(
     )?;
     writeln!(writer, "{}enum GeneratedField {{", Indent(indent))?;
     for type_name in fields {
-        writeln!(writer, "{}{},", Indent(indent + 1), type_name)?;
+        writeln!(
+            writer,
+            "{}{},",
+            Indent(indent + 1),
+            escape_type(type_name.to_string())
+        )?;
     }
 
     if ignore_unknown_fields {
@@ -842,7 +848,7 @@ fn write_deserialize_field<W: Write>(
                 Some(deserializer) => {
                     write!(
                         writer,
-                        "map.next_value::<::std::option::Option<{}>>()?.map(|x| {}::{}(x.0))",
+                        "map_.next_value::<::std::option::Option<{}>>()?.map(|x| {}::{}(x.0))",
                         deserializer,
                         resolver.rust_type(&one_of.path),
                         field.rust_type_name()
@@ -851,7 +857,7 @@ fn write_deserialize_field<W: Write>(
                 None => {
                     write!(
                         writer,
-                        "map.next_value::<::std::option::Option<_>>()?.map({}::{})",
+                        "map_.next_value::<::std::option::Option<_>>()?.map({}::{})",
                         resolver.rust_type(&one_of.path),
                         field.rust_type_name()
                     )?;
@@ -860,15 +866,15 @@ fn write_deserialize_field<W: Write>(
             FieldType::Enum(path) => {
                 write!(
                     writer,
-                    "map.next_value::<::std::option::Option<{}>>()?.map(|x| {}::{}(x as i32))",
+                    "map_.next_value::<::std::option::Option<{}>>()?.map(|x| {}::{}(x as i32))",
                     resolver.rust_type(path),
                     resolver.rust_type(&one_of.path),
                     field.rust_type_name()
                 )?;
             }
             FieldType::Message(_) => writeln!(
                 writer,
-                "map.next_value::<::std::option::Option<_>>()?.map({}::{})",
+                "map_.next_value::<::std::option::Option<_>>()?.map({}::{})",
                 resolver.rust_type(&one_of.path),
                 field.rust_type_name()
             )?,
@@ -883,21 +889,21 @@ fn write_deserialize_field<W: Write>(
                 FieldModifier::Optional => {
                     write!(
                         writer,
-                        "map.next_value::<::std::option::Option<{}>>()?.map(|x| x as i32)",
+                        "map_.next_value::<::std::option::Option<{}>>()?.map(|x| x as i32)",
                         resolver.rust_type(path)
                     )?;
                 }
                 FieldModifier::Repeated => {
                     write!(
                         writer,
-                        "Some(map.next_value::<Vec<{}>>()?.into_iter().map(|x| x as i32).collect())",
+                        "Some(map_.next_value::<Vec<{}>>()?.into_iter().map(|x| x as i32).collect())",
                         resolver.rust_type(path)
                     )?;
                 }
                 _ => {
                     write!(
                         writer,
-                        "Some(map.next_value::<{}>()? as i32)",
+                        "Some(map_.next_value::<{}>()? as i32)",
                         resolver.rust_type(path)
                     )?;
                 }
@@ -908,12 +914,12 @@ fn write_deserialize_field<W: Write>(
                 match btree_map {
                     true => write!(
                         writer,
-                        "{}map.next_value::<std::collections::BTreeMap<",
+                        "{}map_.next_value::<std::collections::BTreeMap<",
                         Indent(indent + 2),
                     )?,
                     false => write!(
                         writer,
-                        "{}map.next_value::<std::collections::HashMap<",
+                        "{}map_.next_value::<std::collections::HashMap<",
                         Indent(indent + 2),
                     )?,
                 }
@@ -975,9 +981,9 @@ fn write_deserialize_field<W: Write>(
             FieldType::Message(_) => match field.field_modifier {
                 FieldModifier::Repeated => {
                     // No explicit presence for repeated fields
-                    write!(writer, "Some(map.next_value()?)")?;
+                    write!(writer, "Some(map_.next_value()?)")?;
                 }
-                _ => write!(writer, "map.next_value()?")?,
+                _ => write!(writer, "map_.next_value()?")?,
             },
         },
     }
@@ -1004,9 +1010,9 @@ fn write_encode_scalar_field<W: Write>(
         None => {
             return match field_modifier {
                 FieldModifier::Optional => {
-                    write!(writer, "map.next_value()?")
+                    write!(writer, "map_.next_value()?")
                 }
-                _ => write!(writer, "Some(map.next_value()?)"),
+                _ => write!(writer, "Some(map_.next_value()?)"),
             };
         }
     };
@@ -1017,15 +1023,15 @@ fn write_encode_scalar_field<W: Write>(
         FieldModifier::Optional => {
             writeln!(
                 writer,
-                "{}map.next_value::<::std::option::Option<{}>>()?.map(|x| x.0)",
+                "{}map_.next_value::<::std::option::Option<{}>>()?.map(|x| x.0)",
                 Indent(indent + 1),
                 deserializer
             )?;
         }
         FieldModifier::Repeated => {
             writeln!(
                 writer,
-                "{}Some(map.next_value::<Vec<{}>>()?",
+                "{}Some(map_.next_value::<Vec<{}>>()?",
                 Indent(indent + 1),
                 deserializer
             )?;
@@ -1038,7 +1044,7 @@ fn write_encode_scalar_field<W: Write>(
         _ => {
             writeln!(
                 writer,
-                "{}Some(map.next_value::<{}>()?.0)",
+                "{}Some(map_.next_value::<{}>()?.0)",
                 Indent(indent + 1),
                 deserializer
             )?;

diff --git a/pbjson-build/src/message.rs b/pbjson-build/src/message.rs
@@ -10,7 +10,7 @@ use prost_types::{
 };
 
 use crate::descriptor::{Descriptor, DescriptorSet, MessageDescriptor, Syntax, TypeName, TypePath};
-use crate::escape::escape_ident;
+use crate::escape::{escape_ident, escape_type};
 
 #[derive(Debug, Clone, Copy)]
 pub enum ScalarType {
@@ -81,7 +81,7 @@ pub struct Field {
 impl Field {
     pub fn rust_type_name(&self) -> String {
         use heck::ToUpperCamelCase;
-        self.name.to_upper_camel_case()
+        escape_type(self.name.to_upper_camel_case())
     }
 
     pub fn rust_field_name(&self) -> String {

diff --git a/pbjson-test/build.rs b/pbjson-test/build.rs
@@ -13,6 +13,7 @@ fn main() -> Result<()> {
         root.join("syntax3.proto"),
         root.join("common.proto"),
         root.join("duplicate_name.proto"),
+        root.join("duplicate_number.proto"),
         root.join("escape.proto"),
     ];
 

diff --git a/pbjson-test/protos/duplicate_number.proto b/pbjson-test/protos/duplicate_number.proto
@@ -0,0 +1,16 @@
+syntax = "proto3";
+
+package test.duplicate_number;
+
+message Compressor {
+    enum CompressionLevel {
+        option allow_alias = true;
+
+        DEFAULT = 0;
+        BEST_SPEED = 1;
+        COMPRESSION_LEVEL_1 = 1;
+        COMPRESSION_LEVEL_2 = 2;
+        COMPRESSION_LEVEL_3 = 3;
+        BEST_COMPRESSION = 3;
+    }
+}
diff --git a/pbjson-test/src/lib.rs b/pbjson-test/src/lib.rs
@@ -39,6 +39,11 @@ pub mod test {
         include!(concat!(env!("OUT_DIR"), "/test.duplicate_name.serde.rs"));
     }
 
+    pub mod duplicate_number {
+        include!(concat!(env!("OUT_DIR"), "/test.duplicate_number.rs"));
+        include!(concat!(env!("OUT_DIR"), "/test.duplicate_number.serde.rs"));
+    }
+
     pub mod escape {
         include!(concat!(
             env!("OUT_DIR"),