fix: deep code review pass 35 - 6 issues fixed

RELEASES.md

@@ -81,18 +81,21 @@
   - Consistent with `NetflowParser::parse_bytes()` return type
   - Builder errors now return `ParseResult { packets: vec![], error: Some(...) }` instead of `Err(...)`
 
-* **Deprecated APIs**
-  - `with_builder()` on `RouterScopedParser` and `AutoScopedParser` — use `try_with_builder()` (returns `Result<Self, ConfigError>`)
-  - `multi_source()` on `NetflowParserBuilder` — use `try_multi_source()` (returns `Result<AutoScopedParser, ConfigError>`)
-
 * **Renamed types and variants**
   - `V9Field::BpgIpv6NextHop` → `V9Field::BgpIpv6NextHop` (typo fix)
   - `V9Field::ImpIpv6CodeValue` → `V9Field::IcmpIpv6CodeValue` (field ID 179, typo fix)
-  - `IpFixFlowRecord` → `IPFixFlowRecord` for consistent casing (deprecated alias preserves backward compatibility)
-  - Module `variable_versions::data_number` → `variable_versions::field_value` (deprecated re-export preserves backward compatibility)
+  - `IpFixFlowRecord` → `IPFixFlowRecord` for consistent casing
+  - Module `variable_versions::data_number` → `variable_versions::field_value`
 
 * **Removed deprecated items**
   - `NetflowPacketError` and `NetflowParseError` type aliases — use `NetflowError` directly
+  - `with_builder()` on `RouterScopedParser` and `AutoScopedParser` — use `try_with_builder()`
+  - `multi_source()` on `NetflowParserBuilder` — use `try_multi_source()`
+  - `IpFixFlowRecord` type alias — use `IPFixFlowRecord`
+  - `variable_versions::data_number` module — use `variable_versions::field_value`
+  - `crate::field_types` module — use `variable_versions::field_types`
+  - `crate::template_events` module — use `variable_versions::template_events`
+  - `FieldValue::Unknown` variant — use `FieldValue::Vec`
 
 * **New enum variants (exhaustive match impact)**
   - `ConfigError` gains `InvalidAllowedVersion(u16)`, `InvalidFieldCount(usize)`, `InvalidTemplateTotalSize(usize)`, `InvalidEntriesPerTemplate(usize)`, `InvalidEntrySize(usize)`, `InvalidTtlDuration`, `EmptyAllowedVersions`, `InvalidPendingTotalBytes { max_total_bytes, max_entry_size_bytes }`
@@ -388,9 +391,9 @@
 * **Module restructuring**
   - Split `v9.rs` into `v9/{mod.rs, parser.rs, serializer.rs}`
   - Split `ipfix.rs` into `ipfix/{mod.rs, parser.rs, serializer.rs}`
-  - Renamed `data_number.rs` → `field_value.rs` (deprecated re-export module preserves backward compatibility)
-  - Moved `field_types` from crate root to `variable_versions::field_types` (deprecated re-export at `crate::field_types`)
-  - Moved `template_events` from crate root to `variable_versions::template_events` (deprecated re-export at `crate::template_events`)
+  - Renamed `data_number.rs` → `field_value.rs`
+  - Moved `field_types` from crate root to `variable_versions::field_types`
+  - Moved `template_events` from crate root to `variable_versions::template_events`
 
 * **Code cleanup**
   - Removed unused `enterprise_registry` field from `V9Parser` (was `#[allow(dead_code)]`)

src/lib.rs

@@ -1,24 +1,11 @@
 #![forbid(unsafe_code)]
 #![doc = include_str!("../README.md")]
 
-#[deprecated(since = "1.0.0", note = "moved to `variable_versions::field_types`")]
-pub mod field_types {
-    //! Deprecated re-export — use [`super::variable_versions::field_types`] instead.
-    pub use super::variable_versions::field_types::*;
-}
 #[cfg(feature = "netflow_common")]
 pub mod netflow_common;
 pub mod protocol;
 pub mod scoped_parser;
 pub mod static_versions;
-#[deprecated(
-    since = "1.0.0",
-    note = "moved to `variable_versions::template_events`"
-)]
-pub mod template_events {
-    //! Deprecated re-export — use [`super::variable_versions::template_events`] instead.
-    pub use super::variable_versions::template_events::*;
-}
 mod tests;
 pub mod variable_versions;
 
@@ -746,21 +733,6 @@ impl NetflowParserBuilder {
         AutoScopedParser::try_with_builder(self)
     }
 
-    /// Creates an AutoScopedParser for multi-source deployments.
-    ///
-    /// # Panics
-    ///
-    /// Panics if the builder configuration is invalid. Prefer [`try_multi_source`](Self::try_multi_source)
-    /// for fallible construction.
-    #[deprecated(
-        since = "1.0.0",
-        note = "use try_multi_source() for fallible construction"
-    )]
-    pub fn multi_source(self) -> AutoScopedParser {
-        #[allow(deprecated)]
-        AutoScopedParser::with_builder(self)
-    }
-
     /// Registers a callback for template lifecycle events.
     ///
     /// This allows you to monitor template operations in real-time, including:
@@ -798,10 +770,7 @@ impl NetflowParserBuilder {
     #[must_use = "builder methods consume self and return a new builder; the return value must be used"]
     pub fn on_template_event<F>(mut self, hook: F) -> Self
     where
-        F: Fn(&TemplateEvent) -> Result<(), template_events::TemplateHookError>
-            + Send
-            + Sync
-            + 'static,
+        F: Fn(&TemplateEvent) -> Result<(), TemplateHookError> + Send + Sync + 'static,
     {
         self.template_hooks.register(hook);
         self

src/scoped_parser.rs

@@ -153,20 +153,6 @@ impl<K: Hash + Eq> RouterScopedParser<K> {
         Ok(self)
     }
 
-    /// Create a new scoped parser with a custom parser builder.
-    ///
-    /// # Panics
-    ///
-    /// Panics if the builder configuration is invalid. Prefer [`try_with_builder`](Self::try_with_builder)
-    /// for fallible construction.
-    #[deprecated(
-        since = "1.0.0",
-        note = "use try_with_builder() for fallible construction"
-    )]
-    pub fn with_builder(builder: NetflowParserBuilder) -> Self {
-        Self::try_with_builder(builder).expect("invalid builder configuration")
-    }
-
     /// Parse NetFlow data from a specific source.
     ///
     /// This will automatically create a new parser instance for new sources,
@@ -611,20 +597,6 @@ impl AutoScopedParser {
         Ok(self)
     }
 
-    /// Create a new auto-scoped parser with a custom parser builder.
-    ///
-    /// # Panics
-    ///
-    /// Panics if the builder configuration is invalid. Prefer [`try_with_builder`](Self::try_with_builder)
-    /// for fallible construction.
-    #[deprecated(
-        since = "1.0.0",
-        note = "use try_with_builder() for fallible construction"
-    )]
-    pub fn with_builder(builder: NetflowParserBuilder) -> Self {
-        Self::try_with_builder(builder).expect("invalid builder configuration")
-    }
-
     /// Parse NetFlow data from a source with automatic RFC-compliant scoping.
     ///
     /// This method automatically:

src/variable_versions/field_types/forwarding_status.rs

@@ -7,7 +7,7 @@
 //! # Examples
 //!
 //! ```
-//! use netflow_parser::field_types::ForwardingStatus;
+//! use netflow_parser::variable_versions::field_types::ForwardingStatus;
 //!
 //! // Forwarded with unknown reason
 //! let status = ForwardingStatus::from(0b01_000000);

src/variable_versions/field_value.rs

@@ -406,14 +406,20 @@ impl DataNumber {
             DataNumber::U16(n) => buf.extend_from_slice(&n.to_be_bytes()),
             DataNumber::I16(n) => buf.extend_from_slice(&n.to_be_bytes()),
             DataNumber::U24(n) => {
-                // Mask to 24 bits to prevent silent data loss from out-of-range values
+                debug_assert!(
+                    *n <= 0x00FF_FFFF,
+                    "U24 value {n} out of range (max 16777215)"
+                );
                 let masked = *n & 0x00FF_FFFF;
                 buf.push((masked >> 16) as u8);
                 buf.push((masked >> 8) as u8);
                 buf.push(masked as u8);
             }
             DataNumber::I24(n) => {
-                // Mask to 24 bits to preserve two's complement representation
+                debug_assert!(
+                    (-8_388_608..=8_388_607).contains(n),
+                    "I24 value {n} out of range (-8388608..=8388607)"
+                );
                 let masked = *n & 0x00FF_FFFF;
                 buf.push((masked >> 16) as u8);
                 buf.push((masked >> 8) as u8);
@@ -431,7 +437,7 @@ impl DataNumber {
     }
 }
 
-#[derive(Debug, PartialEq, Eq, PartialOrd, Clone, Serialize)]
+#[derive(Debug, PartialEq, Eq, PartialOrd, Ord, Clone, Serialize)]
 pub struct ApplicationId {
     pub classification_engine_id: u8,
     pub selector_id: Option<DataNumber>,
@@ -559,11 +565,6 @@ pub enum FieldValue {
     FirewallEvent(FirewallEvent),
     MplsTopLabelType(MplsTopLabelType),
     NatOriginatingAddressRealm(NatOriginatingAddressRealm),
-    #[deprecated(
-        since = "1.0.0",
-        note = "unused by the parser; use `FieldValue::Vec` instead"
-    )]
-    Unknown(Vec<u8>),
 }
 
 impl Serialize for FieldValue {
@@ -654,10 +655,6 @@ impl Serialize for FieldValue {
                 "NatOriginatingAddressRealm",
                 v,
             ),
-            #[allow(deprecated)]
-            FieldValue::Unknown(v) => {
-                serializer.serialize_newtype_variant("FieldValue", 23, "Unknown", v)
-            }
         }
     }
 }
@@ -739,8 +736,6 @@ impl FieldValue {
             FieldValue::MplsTopLabelType(_) => 1,
             FieldValue::NatOriginatingAddressRealm(_) => 1,
             FieldValue::Vec(v) => v.len(),
-            #[allow(deprecated)]
-            FieldValue::Unknown(v) => v.len(),
         }
     }
 
@@ -809,8 +804,6 @@ impl FieldValue {
             FieldValue::MplsTopLabelType(t) => buf.push(u8::from(*t)),
             FieldValue::NatOriginatingAddressRealm(r) => buf.push(u8::from(*r)),
             FieldValue::Vec(v) => buf.extend_from_slice(v),
-            #[allow(deprecated)]
-            FieldValue::Unknown(v) => buf.extend_from_slice(v),
         }
         Ok(())
     }
@@ -1192,12 +1185,6 @@ mod field_value_tests {
         buf.clear();
         field_value.write_be_bytes(&mut buf).unwrap();
         assert_eq!(buf, vec![1, 2, 3, 4]);
-
-        #[allow(deprecated)]
-        let field_value = FieldValue::Unknown(vec![255, 254, 253]);
-        buf.clear();
-        field_value.write_be_bytes(&mut buf).unwrap();
-        assert_eq!(buf, vec![255, 254, 253]);
     }
 
     #[test]

src/variable_versions/ipfix/parser.rs

@@ -210,11 +210,14 @@ impl IPFixParser {
         for (i, flowset) in ipfix.flowsets.iter_mut().enumerate() {
             match &mut flowset.body {
                 FlowSetBody::NoTemplate(info) => {
-                    // If raw_data was truncated at parse time (oversized
-                    // entry), skip caching — the data can't be replayed.
-                    // The truncated flowset is kept in output as diagnostic
-                    // data (truncated to max_error_sample_size).
-                    let body_len = (flowset.header.length as usize).saturating_sub(4);
+                    // Reject flowsets with impossibly small headers (RFC minimum is 4).
+                    // Also reject truncated raw_data (oversized entry at parse time).
+                    // The flowset is kept in output as diagnostic data.
+                    if flowset.header.length < 4 {
+                        metrics.record_pending_dropped();
+                        continue;
+                    }
+                    let body_len = (flowset.header.length as usize) - 4;
                     if info.raw_data.len() < body_len {
                         metrics.record_pending_dropped();
                         continue;
@@ -270,16 +273,19 @@ impl IPFixParser {
                 _ => {}
             }
         }
-        // Subtract lengths of cached flowsets from header, then remove them.
-        for (i, fs) in ipfix.flowsets.iter().enumerate() {
-            if remove_mask[i] {
-                ipfix.header.length = ipfix.header.length.saturating_sub(fs.header.length);
-            }
-        }
+        // Remove successfully-cached flowsets and reconcile header length.
         let mut mask_iter = remove_mask.into_iter();
         ipfix
             .flowsets
             .retain(|_| !mask_iter.next().unwrap_or(false));
+        // Reconcile header length from remaining flowsets (avoids drift from
+        // saturating arithmetic on corrupt input).
+        let body_len: u16 = ipfix
+            .flowsets
+            .iter()
+            .fold(0u16, |acc, fs| acc.saturating_add(fs.header.length));
+        // IPFIX header is 16 bytes; total message length = header + body.
+        ipfix.header.length = 16u16.saturating_add(body_len);
         learned_template_ids
     }
 

src/variable_versions/ipfix/types.rs

@@ -31,13 +31,6 @@ pub(crate) const DATA_TEMPLATE_IPFIX_ID: u16 = 2;
 pub(crate) const OPTIONS_TEMPLATE_IPFIX_ID: u16 = 3;
 pub type IPFixFieldPair = (IPFixField, FieldValue);
 pub type IPFixFlowRecord = Vec<IPFixFieldPair>;
-/// Deprecated alias for [`IPFixFlowRecord`].
-#[deprecated(
-    since = "1.0.0",
-    note = "renamed to `IPFixFlowRecord` for consistent casing"
-)]
-pub type IpFixFlowRecord = IPFixFlowRecord;
-
 /// Stateful IPFIX parser with LRU template caches and optional pending flow support.
 /// Supports both native IPFIX templates and V9-style templates embedded in IPFIX messages.
 #[derive(Debug)]

src/variable_versions/mod.rs

@@ -68,14 +68,9 @@
 //! - [`metrics`] - Template cache performance metrics
 
 pub(crate) mod config;
+pub mod enterprise_registry;
 pub mod field_types;
 pub mod field_value;
-#[deprecated(since = "1.0.0", note = "renamed to `field_value`")]
-pub mod data_number {
-    //! Deprecated re-export — use [`super::field_value`] instead.
-    pub use super::field_value::*;
-}
-pub mod enterprise_registry;
 pub mod ipfix;
 pub mod metrics;
 pub(crate) mod pending_flows;

src/variable_versions/v9/parser.rs

@@ -214,11 +214,14 @@ impl V9Parser {
         for (i, flowset) in v9.flowsets.iter_mut().enumerate() {
             match &mut flowset.body {
                 FlowSetBody::NoTemplate(info) => {
-                    // If raw_data was truncated at parse time (oversized
-                    // entry), skip caching — the data can't be replayed.
-                    // The truncated flowset is kept in output as diagnostic
-                    // data (truncated to max_error_sample_size).
-                    let body_len = (flowset.header.length as usize).saturating_sub(4);
+                    // Reject flowsets with impossibly small headers (RFC minimum is 4).
+                    // Also reject truncated raw_data (oversized entry at parse time).
+                    // The flowset is kept in output as diagnostic data.
+                    if flowset.header.length < 4 {
+                        metrics.record_pending_dropped();
+                        continue;
+                    }
+                    let body_len = (flowset.header.length as usize) - 4;
                     if info.raw_data.len() < body_len {
                         metrics.record_pending_dropped();
                         continue;

tests/cache_collision.rs

@@ -70,7 +70,10 @@ fn test_cache_hit_and_miss_tracking() {
         0, 9, 0, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 12, 1, 0, 0, 1, 0,
         1, 0, 4,
     ];
-    let _ = parser.parse_bytes(&v9_template_packet);
+    assert!(
+        !parser.parse_bytes(&v9_template_packet).packets.is_empty(),
+        "Template packet should parse successfully"
+    );
 
     // V9 data packet using template 256
     let v9_data_packet: Vec<u8> = vec![
@@ -79,7 +82,10 @@ fn test_cache_hit_and_miss_tracking() {
         0, 8, // length = 8 (header(4) + 1 record of 4 bytes)
         0, 0, 0, 42, // IN_BYTES = 42
     ];
-    let _ = parser.parse_bytes(&v9_data_packet);
+    assert!(
+        !parser.parse_bytes(&v9_data_packet).packets.is_empty(),
+        "Data packet should parse successfully with cached template"
+    );
 
     let v9_info = parser.v9_cache_info();
     assert_eq!(
@@ -98,7 +104,13 @@ fn test_cache_hit_and_miss_tracking() {
         0, 8, // length = 8
         0, 0, 0, 99, // data
     ];
-    let _ = parser.parse_bytes(&v9_missing_template_packet);
+    assert!(
+        !parser
+            .parse_bytes(&v9_missing_template_packet)
+            .packets
+            .is_empty(),
+        "Missing-template packet should still produce a parse result"
+    );
 
     let v9_info = parser.v9_cache_info();
     assert_eq!(

tests/memory_bounds.rs

@@ -33,7 +33,11 @@ fn test_cache_info_stay_within_bounds() {
             0, 1, // field_type = IN_BYTES
             0, 4, // field_length = 4
         ];
-        let _ = parser.parse_bytes(&v9_template_packet);
+        assert!(
+            !parser.parse_bytes(&v9_template_packet).packets.is_empty(),
+            "Template packet for tid {} should parse successfully",
+            template_id
+        );
     }
 
     let v9_info = parser.v9_cache_info();