refactor: remove unnecessary bounds for encoder and decoder derive ma…

…cro (#3030) Fixed a long standing tech debt around Encoder and Decoder derived macro. With this fix, no longer need to add bounds to struct as before: ``` pub struct Message<C> where C: Encoder + Decoder + Debug, { pub header: MsgType, pub content: C, } ``` Instead, this can be simplified: ``` pub struct Message<C> { pub header: MsgType, pub content: C, } ``` Forcing to add structure bounds cause downstream code to deal with extras bounds which make certain implementation difficult if not outright implementation In addition, tracing code is only added if enabled. This should only necessary in development. This will reduce some code and improve performance slightly. ``` #[derive(Encoder,Decoder,Default] #[fluvio(trace)] MyStruct { ... } ```
infinyon · Mar 2, 2023 · cf09815 · cf09815 · github-actions · Mar 2, 2023
1 parent 6720fde
commit cf09815
Show file tree

Hide file tree

Showing 30 changed files with 547 additions and 245 deletions.
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/crates/fluvio-controlplane-metadata/Cargo.toml b/crates/fluvio-controlplane-metadata/Cargo.toml
@@ -1,7 +1,7 @@
 [package]
 name = "fluvio-controlplane-metadata"
 edition = "2021"
-version = "0.21.0"
+version = "0.22.0"
 authors = ["Fluvio Contributors <team@fluvio.io>"]
 description = "Metadata definition for Fluvio control plane"
 repository = "https://github.com/infinyon/fluvio"
@@ -34,7 +34,7 @@ flv-util = { version = "0.5.0" }
 
 fluvio-types = { version = "0.4.0", path = "../fluvio-types" }
 fluvio-stream-model = { path = "../fluvio-stream-model", version = "0.8.0" }
-fluvio-protocol = { path = "../fluvio-protocol", version = "0.8", features = [
+fluvio-protocol = { path = "../fluvio-protocol", version = "0.9", features = [
     "record",
 ] }
 

diff --git a/crates/fluvio-controlplane-metadata/src/message/messages.rs b/crates/fluvio-controlplane-metadata/src/message/messages.rs
@@ -10,17 +10,14 @@ use fluvio_protocol::{Encoder, Decoder};
 
 use super::Message;
 
-#[derive(Decoder, Encoder, Debug, Eq, PartialEq, Clone, Default)]
-pub struct Messages<S>
-where
-    S: Encoder + Decoder + Debug,
-{
+#[derive(Encoder, Decoder, Debug, Eq, PartialEq, Clone, Default)]
+pub struct Messages<S> {
     pub messages: Vec<Message<S>>,
 }
 
 impl<S> fmt::Display for Messages<S>
 where
-    S: Encoder + Decoder + Debug + Display,
+    S: Display,
 {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
         write!(f, "[")?;
@@ -31,10 +28,7 @@ where
     }
 }
 
-impl<S> Messages<S>
-where
-    S: Encoder + Decoder + Debug,
-{
+impl<S> Messages<S> {
     pub fn new(messages: Vec<Message<S>>) -> Self {
         Self { messages }
     }

diff --git a/crates/fluvio-controlplane-metadata/src/message/msg_type.rs b/crates/fluvio-controlplane-metadata/src/message/msg_type.rs
@@ -28,27 +28,21 @@ impl ::std::default::Default for MsgType {
 }
 
 #[derive(Decoder, Encoder, Debug, Eq, PartialEq, Clone, Default)]
-pub struct Message<C>
-where
-    C: Encoder + Decoder + Debug,
-{
+pub struct Message<C> {
     pub header: MsgType,
     pub content: C,
 }
 
 impl<C> fmt::Display for Message<C>
 where
-    C: Encoder + Decoder + Debug + Display,
+    C: Display,
 {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
         write!(f, "{:#?} {}", self.header, self.content)
     }
 }
 
-impl<C> Message<C>
-where
-    C: Encoder + Decoder + Debug,
-{
+impl<C> Message<C> {
     pub fn new(typ: MsgType, content: C) -> Self {
         Message {
             header: typ,
@@ -81,7 +75,6 @@ where
     S: Spec,
     S::Status: PartialEq,
     C: MetadataItem,
-    D: Encoder + Decoder + Debug,
     D: From<MetadataStoreObject<S, C>>,
 {
     fn from(change: LSChange<S, C>) -> Self {

diff --git a/crates/fluvio-controlplane/src/requests/request.rs b/crates/fluvio-controlplane/src/requests/request.rs
@@ -5,10 +5,7 @@ use fluvio_controlplane_metadata::message::Message;
 
 /// General control plane request
 #[derive(Decoder, Encoder, Debug, Default)]
-pub struct ControlPlaneRequest<S>
-where
-    S: Encoder + Decoder + Debug,
-{
+pub struct ControlPlaneRequest<S> {
     pub epoch: i64,
     pub changes: Vec<Message<S>>,
     pub all: Vec<S>,

diff --git a/crates/fluvio-extension-common/Cargo.toml b/crates/fluvio-extension-common/Cargo.toml
@@ -1,6 +1,6 @@
 [package]
 name = "fluvio-extension-common"
-version = "0.11.0"
+version = "0.12.0"
 edition = "2021"
 authors = ["Fluvio Contributors <team@fluvio.io>"]
 description = "Fluvio extension common"
@@ -27,5 +27,5 @@ thiserror = "1.0.20"
 semver = { version = "1.0.0", features = ["serde"] }
 anyhow = { workspace = true}
 
-fluvio = { version = "0.17", path = "../fluvio", optional = true }
+fluvio = { version = "0.18", path = "../fluvio", optional = true }
 fluvio-package-index = { version = "0.7.0", path = "../fluvio-package-index" }
diff --git a/crates/fluvio-protocol-derive/src/ast/container.rs b/crates/fluvio-protocol-derive/src/ast/container.rs
@@ -24,6 +24,7 @@ pub struct ContainerAttributes {
     pub api_key: Option<u8>,
     pub response: Option<String>,
     pub repr_type_name: Option<String>,
+    pub trace: bool,
 }
 
 impl ContainerAttributes {
@@ -63,6 +64,8 @@ impl ContainerAttributes {
                         } else if let NestedMeta::Meta(Meta::Path(path)) = kf_attr {
                             if path.is_ident("default") {
                                 cont_attr.default = true;
+                            } else if path.is_ident("trace") {
+                                cont_attr.trace = true;
                             } else if path.is_ident("encode_discriminant") {
                                 cont_attr.encode_discriminant = true;
                             } else {

diff --git a/crates/fluvio-protocol-derive/src/ast/mod.rs b/crates/fluvio-protocol-derive/src/ast/mod.rs
@@ -4,7 +4,9 @@ pub(crate) mod prop;
 pub(crate) mod r#struct;
 
 use syn::parse::{Parse, ParseStream};
-use syn::{Attribute, ItemEnum, ItemStruct, Result, Token, Visibility};
+use syn::{
+    parse_quote, Attribute, GenericParam, Generics, ItemEnum, ItemStruct, Result, Token, Visibility,
+};
 
 use crate::ast::container::ContainerAttributes;
 use crate::ast::r#enum::FluvioEnum;
@@ -34,3 +36,40 @@ impl Parse for DeriveItem {
         }
     }
 }
+
+pub(crate) enum FluvioBound {
+    Encoder,
+    Decoder,
+    Default,
+}
+
+pub(crate) fn add_bounds(
+    mut generics: Generics,
+    attr: &ContainerAttributes,
+    bounds: FluvioBound,
+) -> Generics {
+    for param in &mut generics.params {
+        if let GenericParam::Type(ref mut type_param) = *param {
+            match bounds {
+                FluvioBound::Encoder => {
+                    type_param
+                        .bounds
+                        .push(parse_quote!(fluvio_protocol::Encoder));
+                }
+                FluvioBound::Decoder => {
+                    type_param
+                        .bounds
+                        .push(parse_quote!(fluvio_protocol::Decoder));
+                }
+                FluvioBound::Default => {
+                    type_param.bounds.push(parse_quote!(Default));
+                }
+            }
+            if attr.trace {
+                type_param.bounds.push(parse_quote!(std::fmt::Debug));
+            }
+        }
+    }
+
+    generics
+}
diff --git a/crates/fluvio-protocol-derive/src/ast/prop.rs b/crates/fluvio-protocol-derive/src/ast/prop.rs
@@ -48,25 +48,45 @@ impl NamedProp {
         }
     }
 
-    pub fn version_check_token_stream(&self, field_stream: TokenStream) -> TokenStream {
+    pub fn version_check_token_stream(
+        &self,
+        field_stream: TokenStream,
+        trace: bool,
+    ) -> TokenStream {
         let min = self.attrs.min_version;
         let field_name = &self.field_name;
 
         if let Some(max) = self.attrs.max_version {
+            let trace = if trace {
+                quote! {
+                    else {
+                        tracing::trace!("Field: <{}> is skipped because version: {} is outside min: {}, max: {}",stringify!(#field_name),version,#min,#max);
+                    }
+                }
+            } else {
+                quote! {}
+            };
             quote! {
                 if (#min..=#max).contains(&version) {
                     #field_stream
-                } else {
-                    tracing::trace!("Field: <{}> is skipped because version: {} is outside min: {}, max: {}",stringify!(#field_name),version,#min,#max);
                 }
+                #trace
             }
         } else {
+            let trace = if trace {
+                quote! {
+                    else {
+                        tracing::trace!("Field: <{}> is skipped because version: {} is less than min: {}",stringify!(#field_name),version,#min);
+                    }
+                }
+            } else {
+                quote! {}
+            };
             quote! {
                 if version >= #min {
                     #field_stream
-                } else {
-                    tracing::trace!("Field: <{}> is skipped because version: {} is less than min: {}",stringify!(#field_name),version,#min);
                 }
+                #trace
             }
         }
     }
@@ -87,24 +107,46 @@ impl UnnamedProp {
         }
     }
 
-    pub fn version_check_token_stream(&self, field_stream: TokenStream) -> TokenStream {
+    pub fn version_check_token_stream(
+        &self,
+        field_stream: TokenStream,
+        trace: bool,
+    ) -> TokenStream {
         let min = self.attrs.min_version;
 
         if let Some(max) = self.attrs.max_version {
+            let trace = if trace {
+                quote! {
+                    else {
+                        tracing::trace!("Field from tuple struct:is skipped because version: {} is outside min: {}, max: {}",version,#min,#max);
+                    }
+                }
+            } else {
+                quote! {}
+            };
+
             quote! {
                 if (#min..=#max).contains(&version) {
                     #field_stream
-                } else {
-                    tracing::trace!("Field from tuple struct:is skipped because version: {} is outside min: {}, max: {}",version,#min,#max);
                 }
+                #trace
             }
         } else {
+            let trace = if trace {
+                quote! {
+                    else {
+                        tracing::trace!("Field from tuple struct: is skipped because version: {} is less than min: {}",version,#min);
+                    }
+                }
+            } else {
+                quote! {}
+            };
+
             quote! {
                 if version >= #min {
                     #field_stream
-                } else {
-                    tracing::trace!("Field from tuple struct: is skipped because version: {} is less than min: {}",version,#min);
                 }
+                #trace
             }
         }
     }

diff --git a/crates/fluvio-protocol-derive/src/ast/struct.rs b/crates/fluvio-protocol-derive/src/ast/struct.rs
@@ -9,7 +9,7 @@ pub(crate) enum FluvioStruct {
 pub(crate) struct FluvioNamedStruct {
     pub struct_ident: Ident,
     pub props: Vec<NamedProp>,
-    pub generics: Generics,
+    generics: Generics,
 }
 
 impl FluvioStruct {
Benchmark suite	Current: `cf09815`	Previous: `7a58d66`	Ratio
`vecu8 encoding`	`335122` ns/iter (`± 759381`)	`382067` ns/iter (`± 719271`)	`0.88`
`vecu8 decoding`	`446384` ns/iter (`± 186`)	`594623` ns/iter (`± 30935`)	`0.75`
`bytebuf encoding`	`7228` ns/iter (`± 188`)	`15353` ns/iter (`± 636`)	`0.47`
`bytebuf decoding`	`6934` ns/iter (`± 34`)	`15429` ns/iter (`± 603`)	`0.45`