From 4abc685efb02847c637cafb903d272aa5582cc36 Mon Sep 17 00:00:00 2001
From: mikemiles-dev <justmike2000@gmail.com>
Date: Sun, 15 Mar 2026 17:00:08 -0500
Subject: [PATCH] fix: deep code review pass 34 - 3 issues fixed

- Data::with_template_field_lengths() returns Result instead of panicking
  via assert_eq! (public API should never panic on invalid input)
- OptionsData::with_template_field_lengths() same treatment for consistency
- Data::new() adds debug_assert catching variable-length field misuse
- Remove unreachable template_size==0 dead code in IPFIX parse_inner
---
 src/variable_versions/ipfix/parser.rs |  8 +---
 src/variable_versions/ipfix/types.rs  | 53 ++++++++++++++++++++-------
 2 files changed, 41 insertions(+), 20 deletions(-)

diff --git a/src/variable_versions/ipfix/parser.rs b/src/variable_versions/ipfix/parser.rs
index 550889b..028816d 100644
--- a/src/variable_versions/ipfix/parser.rs
+++ b/src/variable_versions/ipfix/parser.rs
@@ -1003,13 +1003,7 @@ impl<'a> FieldParser {
             })
             .sum();
         // template_fields is non-empty (checked above) and each contributes >= 1 byte,
-        // so template_size is always > 0 here. Return error to match V9 behavior.
-        if template_size == 0 {
-            return Err(nom::Err::Error(nom::error::Error::new(
-                i,
-                nom::error::ErrorKind::Verify,
-            )));
-        }
+        // so template_size is always > 0 here.
         let estimated_records = (i.len() / template_size).min(max_records);
         let mut res = Vec::with_capacity(estimated_records);
 
diff --git a/src/variable_versions/ipfix/types.rs b/src/variable_versions/ipfix/types.rs
index a59d21c..abe6688 100644
--- a/src/variable_versions/ipfix/types.rs
+++ b/src/variable_versions/ipfix/types.rs
@@ -187,6 +187,13 @@ impl Data {
     /// [`Data::with_template_field_lengths`] when the template contains
     /// variable-length fields (field_length == 65535).
     pub fn new(fields: Vec<IPFixFlowRecord>) -> Self {
+        debug_assert!(
+            !fields.iter().any(|record| record
+                .iter()
+                .any(|(_, v)| matches!(v, FieldValue::Vec(b) if b.len() > 254))),
+            "Data::new() should not be used with fields that may need variable-length \
+             encoding; use Data::with_template_field_lengths() instead"
+        );
         Self {
             fields,
             padding: vec![],
@@ -206,25 +213,30 @@ impl Data {
     /// Each entry in `template_field_lengths` corresponds to the template
     /// field at the same index; entries with value 65535 cause an RFC 7011
     /// variable-length prefix to be emitted during serialization.
+    ///
+    /// # Errors
+    ///
+    /// Returns an error if `template_field_lengths` is non-empty and its
+    /// length does not match the number of fields in the first record.
     pub fn with_template_field_lengths(
         fields: Vec<IPFixFlowRecord>,
         template_field_lengths: Vec<u16>,
-    ) -> Self {
+    ) -> Result<Self, String> {
         if !fields.is_empty() && !template_field_lengths.is_empty() {
             let record_len = fields[0].len();
-            assert_eq!(
-                template_field_lengths.len(),
-                record_len,
-                "template_field_lengths length ({}) must match record field count ({})",
-                template_field_lengths.len(),
-                record_len,
-            );
+            if template_field_lengths.len() != record_len {
+                return Err(format!(
+                    "template_field_lengths length ({}) must match record field count ({})",
+                    template_field_lengths.len(),
+                    record_len,
+                ));
+            }
         }
-        Self {
+        Ok(Self {
             fields,
             padding: vec![],
             template_field_lengths,
-        }
+        })
     }
 }
 
@@ -271,15 +283,30 @@ impl OptionsData {
     }
 
     /// Creates a new OptionsData instance with explicit template field lengths.
+    ///
+    /// # Errors
+    ///
+    /// Returns an error if `template_field_lengths` is non-empty and its
+    /// length does not match the number of fields in the first record.
     pub fn with_template_field_lengths(
         fields: Vec<Vec<IPFixFieldPair>>,
         template_field_lengths: Vec<u16>,
-    ) -> Self {
-        Self {
+    ) -> Result<Self, String> {
+        if !fields.is_empty() && !template_field_lengths.is_empty() {
+            let record_len = fields[0].len();
+            if template_field_lengths.len() != record_len {
+                return Err(format!(
+                    "template_field_lengths length ({}) must match record field count ({})",
+                    template_field_lengths.len(),
+                    record_len,
+                ));
+            }
+        }
+        Ok(Self {
             fields,
             padding: vec![],
             template_field_lengths,
-        }
+        })
     }
 }