fix: deep code review passes 19-20 - API hardening, test coverage, an… (#271)

* fix: deep code review passes 19-20 - API hardening, test coverage, and correctness fixes

Pass 19:
- clear_v9_templates/clear_ipfix_templates now also clear pending flows
- V9/IPFIX template validation rejects field_length=0
- Deduplicate validate_config (delegate to Config::validate)
- Strengthen 5 malformed-input test assertions
- Remove 7 vacuous initial-state tests
- Add round-trip tests: DurationMicrosNTP, DurationNanosNTP, U128, I128, U24, enterprise fields, multi-template flowset
- Add concurrent V9 template test and template collision data verification

Pass 20:
- Add #[non_exhaustive] to 9 public types (NetflowError, ConfigError, FieldValue, Config, PendingFlowsConfig, CacheStats, ParserCacheStats, CacheMetricsSnapshot, NoTemplateInfo)
- Remove dead MissingTemplate error variant
- Add root re-exports: FieldValue, DataNumber, FieldDataType, V9Field, V9FieldPair, V9FlowRecord
- InvalidErrorSampleSize/InvalidMaxSources now carry the rejected value
- CacheMetrics mutation methods scoped to pub(crate), dead reset() removed
- resize() now validates full PendingFlowsConfig
- README: fix error type list (removed MissingTemplate, added FilteredVersion), fix URP->UDP typo

* fix: fmt

Author: mikemiles-dev

README.md

@@ -489,7 +489,7 @@ for result in parser.iter_packets(&buffer) {
 }
 ```
 
-**Error types**: `Incomplete`, `UnsupportedVersion`, `Partial`, `MissingTemplate`, `ParseError`. All implement `Display` and `std::error::Error`.
+**Error types**: `Incomplete`, `UnsupportedVersion`, `FilteredVersion`, `ParseError`, `Partial`. All implement `Display` and `std::error::Error`.
 
 #### Error Sample Size Configuration
 
@@ -1173,7 +1173,7 @@ This library includes several performance optimizations:
 
 Examples have been included mainly for those who want to use this parser to read from a Socket and parse netflow.  In those cases with V9/IPFix it is best to create a new parser for each router.  There are both single threaded and multi-threaded examples in the examples directory.
 
-Examples that listen on a specific port use 9995 by default, however netflow can be configured to use a variety of URP ports:
+Examples that listen on a specific port use 9995 by default, however netflow can be configured to use a variety of UDP ports:
 * **2055**: The most widely recognized default for NetFlow.
 * **9995 / 9996**: Popular alternatives, especially with Cisco devices.
 * **9025, 9026**: Other recognized port options.

src/lib.rs

@@ -49,6 +49,11 @@ pub use variable_versions::{
 pub use variable_versions::ipfix::lookup::IpfixField;
 pub use variable_versions::ipfix::{Ipfix, IpfixFieldPair, IpfixFlowRecord, IpfixParser};
 
+// Re-export commonly used field/data types
+pub use variable_versions::field_value::{DataNumber, FieldDataType, FieldValue};
+pub use variable_versions::v9::lookup::V9Field;
+pub use variable_versions::v9::{V9FieldPair, V9FlowRecord};
+
 /// Enum of supported Netflow Versions
 #[derive(Debug, PartialEq, Clone, Serialize)]
 pub enum NetflowPacket {
@@ -233,6 +238,7 @@ pub struct NetflowParser {
 }
 
 /// Statistics about template cache utilization.
+#[non_exhaustive]
 #[derive(Debug, Clone)]
 pub struct CacheStats {
     /// Current number of cached templates (summed across all internal caches).
@@ -260,6 +266,7 @@ pub struct CacheStats {
 ///
 /// This struct provides named fields instead of positional tuples,
 /// making it clear which stats belong to V9 vs IPFIX.
+#[non_exhaustive]
 #[derive(Debug, Clone)]
 pub struct ParserCacheStats {
     /// V9 template cache statistics
@@ -891,6 +898,7 @@ pub(crate) enum ParsedNetflow<'a> {
 ///
 /// Provides rich context about parsing failures including offset, error kind,
 /// and relevant data for debugging.
+#[non_exhaustive]
 #[derive(Debug, PartialEq, Clone, Serialize)]
 pub enum NetflowError {
     /// Incomplete data - more bytes needed to parse a complete packet.
@@ -925,22 +933,6 @@ pub enum NetflowError {
         version: u16,
     },
 
-    /// Template definition is required but not found in cache.
-    ///
-    /// For V9 and IPFIX, data packets reference template IDs that must be
-    /// learned from template packets. This error occurs when data arrives
-    /// before (or without) its corresponding template.
-    MissingTemplate {
-        /// The template ID that was not found
-        template_id: u16,
-        /// The protocol (V9 or IPFIX)
-        protocol: TemplateProtocol,
-        /// List of currently cached template IDs for this protocol
-        available_templates: Vec<u16>,
-        /// Raw packet data that couldn't be parsed
-        raw_data: Vec<u8>,
-    },
-
     /// Parsing error with detailed context.
     ///
     /// Generic parsing failure with information about what failed and where.
@@ -991,18 +983,6 @@ impl std::fmt::Display for NetflowError {
                     version
                 )
             }
-            NetflowError::MissingTemplate {
-                template_id,
-                protocol,
-                available_templates,
-                ..
-            } => {
-                write!(
-                    f,
-                    "Missing template {} for {:?} (available: {:?})",
-                    template_id, protocol, available_templates
-                )
-            }
             NetflowError::ParseError {
                 offset,
                 context,
@@ -1380,6 +1360,7 @@ impl NetflowParser {
     pub fn clear_v9_templates(&mut self) {
         self.v9_parser.templates.clear();
         self.v9_parser.options_templates.clear();
+        self.v9_parser.clear_pending_flows();
     }
 
     /// Clears all cached IPFIX templates.
@@ -1399,6 +1380,7 @@ impl NetflowParser {
         self.ipfix_parser.v9_templates.clear();
         self.ipfix_parser.ipfix_options_templates.clear();
         self.ipfix_parser.v9_options_templates.clear();
+        self.ipfix_parser.clear_pending_flows();
     }
 
     /// Clears all pending V9 flows.

src/scoped_parser.rs

@@ -145,7 +145,7 @@ impl<K: Hash + Eq> RouterScopedParser<K> {
     /// Returns `ConfigError::InvalidMaxSources` if `max` is 0.
     pub fn with_max_sources(mut self, max: usize) -> Result<Self, ConfigError> {
         if max == 0 {
-            return Err(ConfigError::InvalidMaxSources);
+            return Err(ConfigError::InvalidMaxSources(0));
         }
         self.max_sources = max;
         self.parsers
@@ -598,7 +598,7 @@ impl AutoScopedParser {
     /// Returns `ConfigError::InvalidMaxSources` if `max` is 0.
     pub fn with_max_sources(mut self, max: usize) -> Result<Self, ConfigError> {
         if max == 0 {
-            return Err(ConfigError::InvalidMaxSources);
+            return Err(ConfigError::InvalidMaxSources(0));
         }
         self.max_sources = max;
         // Size each per-protocol LRU cache to max. The global cross-cache

src/tests.rs

@@ -276,11 +276,10 @@ mod base_tests {
 
         let packet = hex::decode(hex).unwrap();
         let result = parser.parse_bytes(&packet);
-        // The parser should either produce packets or an error, but never both be empty
-        // (at minimum it recognizes the IPFIX version and attempts parsing).
-        let handled = !result.packets.is_empty() || result.error.is_some();
+        // Partially malformed IPFIX — parser must not silently skip; it should
+        // produce at least one packet or report an error.
         assert!(
-            handled,
+            !result.packets.is_empty() || result.error.is_some(),
             "parser should produce packets or an error, not silently skip"
         );
         for p in &result.packets {
@@ -323,9 +322,12 @@ mod base_tests {
 
         let packet = hex::decode(hex).unwrap();
         let result = parser.parse_bytes(&packet);
-        // Parser should produce packets or an error, not silently skip the entire input.
-        let handled = !result.packets.is_empty() || result.error.is_some();
-        assert!(handled, "parser should produce packets or an error");
+        // Partially malformed V9 — parser must not silently skip; it should
+        // produce at least one packet or report an error.
+        assert!(
+            !result.packets.is_empty() || result.error.is_some(),
+            "parser should produce packets or an error, not silently skip"
+        );
         for p in &result.packets {
             assert!(p.is_v9(), "all parsed packets should be V9");
         }
@@ -849,8 +851,8 @@ mod malformed_packet_tests {
         packet.extend_from_slice(&[0u8; 8]);
         let result = parser.parse_bytes(&packet);
         assert!(
-            result.packets.is_empty() || result.error.is_some(),
-            "truncated V9 header should not produce valid packets without an error"
+            result.error.is_some(),
+            "truncated V9 header must produce an error"
         );
     }
 
@@ -871,10 +873,10 @@ mod malformed_packet_tests {
         // Flowset with id=0 (template), length=0 (malformed)
         packet.extend_from_slice(&[0x00, 0x00, 0x00, 0x00]);
         let result = parser.parse_bytes(&packet);
-        // Must not infinite loop or panic; zero-length flowset should produce no valid packets or an error
+        // Must not infinite loop or panic; zero-length flowset must produce an error
         assert!(
-            result.packets.is_empty() || result.error.is_some(),
-            "V9 with zero-length flowset should produce no valid packets or an error"
+            result.error.is_some(),
+            "V9 with zero-length flowset must produce an error"
         );
     }
 
@@ -895,8 +897,8 @@ mod malformed_packet_tests {
         // Only 4 bytes of flowset data actually present
         let result = parser.parse_bytes(&packet);
         assert!(
-            result.packets.is_empty() || result.error.is_some(),
-            "V9 with flowset length exceeding packet should produce no valid packets or an error"
+            result.error.is_some(),
+            "V9 with flowset length exceeding packet must produce an error"
         );
     }
 
@@ -916,8 +918,8 @@ mod malformed_packet_tests {
         let result = parser.parse_bytes(&packet);
         // Parser should handle the length mismatch without panicking
         assert!(
-            result.packets.is_empty() || result.error.is_some(),
-            "IPFIX with wrong length field should produce no valid packets or an error"
+            result.error.is_some(),
+            "IPFIX with wrong length field must produce an error"
         );
     }
 

src/variable_versions/config.rs

@@ -26,6 +26,7 @@ pub const DEFAULT_MAX_RECORDS_PER_FLOWSET: usize = 1024;
 /// Controls template cache size, field limits, TTL, enterprise field definitions,
 /// and pending flow caching. Use [`Config::new`] for defaults or construct directly
 /// for full control.
+#[non_exhaustive]
 #[derive(Debug, Clone)]
 pub struct Config {
     /// Maximum number of templates to keep in the LRU cache.
@@ -54,6 +55,7 @@ pub struct Config {
     pub pending_flows_config: Option<PendingFlowsConfig>,
 }
 
+#[non_exhaustive]
 #[derive(Debug, Clone, PartialEq, Eq)]
 pub enum ConfigError {
     /// Template cache size must be greater than 0
@@ -82,9 +84,9 @@ pub enum ConfigError {
         max_entry_size_bytes: usize,
     },
     /// max_sources must be greater than 0
-    InvalidMaxSources,
+    InvalidMaxSources(usize),
     /// max_error_sample_size must be greater than 0
-    InvalidErrorSampleSize,
+    InvalidErrorSampleSize(usize),
 }
 
 impl std::error::Error for ConfigError {}
@@ -167,11 +169,15 @@ impl std::fmt::Display for ConfigError {
                     max_total_bytes, max_entry_size_bytes
                 )
             }
-            ConfigError::InvalidMaxSources => {
-                write!(f, "Invalid max_sources: must be greater than 0.")
+            ConfigError::InvalidMaxSources(size) => {
+                write!(f, "Invalid max_sources: {}. Must be greater than 0.", size)
             }
-            ConfigError::InvalidErrorSampleSize => {
-                write!(f, "Invalid max_error_sample_size: must be greater than 0.")
+            ConfigError::InvalidErrorSampleSize(size) => {
+                write!(
+                    f,
+                    "Invalid max_error_sample_size: {}. Must be greater than 0.",
+                    size
+                )
             }
         }
     }
@@ -233,7 +239,7 @@ impl Config {
             return Err(ConfigError::InvalidRecordsPerFlowset(0));
         }
         if self.max_error_sample_size == 0 {
-            return Err(ConfigError::InvalidErrorSampleSize);
+            return Err(ConfigError::InvalidErrorSampleSize(0));
         }
         if let Some(ref ttl) = self.ttl_config
             && ttl.duration.is_zero()
@@ -286,7 +292,7 @@ pub trait ParserConfig: ParserFields {
             return Err(ConfigError::InvalidRecordsPerFlowset(0));
         }
         if config.max_error_sample_size == 0 {
-            return Err(ConfigError::InvalidErrorSampleSize);
+            return Err(ConfigError::InvalidErrorSampleSize(0));
         }
         if let Some(ref ttl) = config.ttl_config
             && ttl.duration.is_zero()

src/variable_versions/field_value.rs

@@ -467,6 +467,7 @@ impl Serialize for StringValue {
 }
 
 /// Holds the post parsed field with its relevant datatype
+#[non_exhaustive]
 #[derive(Debug, PartialEq, PartialOrd, Clone)]
 pub enum FieldValue {
     ApplicationId(ApplicationId),

src/variable_versions/ipfix/mod.rs

@@ -353,8 +353,9 @@ pub(crate) trait CommonTemplate {
         {
             return false;
         }
-        // Check fields are not empty and have at least one non-zero length field
-        if self.get_fields().is_empty() || !self.get_fields().iter().any(|f| f.field_length > 0)
+        // Check fields are not empty and no field has zero length
+        // (field_length=65535 is the RFC 7011 variable-length sentinel, which is valid)
+        if self.get_fields().is_empty() || self.get_fields().iter().any(|f| f.field_length == 0)
         {
             return false;
         }

src/variable_versions/ipfix/parser.rs

@@ -58,30 +58,7 @@ impl Default for IPFixParser {
 impl IPFixParser {
     /// Validates a configuration without allocating parser internals.
     pub fn validate_config(config: &Config) -> Result<(), ConfigError> {
-        NonZeroUsize::new(config.max_template_cache_size).ok_or(
-            ConfigError::InvalidCacheSize(config.max_template_cache_size),
-        )?;
-        if config.max_field_count == 0 {
-            return Err(ConfigError::InvalidFieldCount(0));
-        }
-        if config.max_template_total_size == 0 {
-            return Err(ConfigError::InvalidTemplateTotalSize(0));
-        }
-        if config.max_records_per_flowset == 0 {
-            return Err(ConfigError::InvalidRecordsPerFlowset(0));
-        }
-        if config.max_error_sample_size == 0 {
-            return Err(ConfigError::InvalidErrorSampleSize);
-        }
-        if let Some(ref ttl) = config.ttl_config
-            && ttl.duration.is_zero()
-        {
-            return Err(ConfigError::InvalidTtlDuration);
-        }
-        if let Some(ref pf) = config.pending_flows_config {
-            PendingFlowCache::validate_config(pf)?;
-        }
-        Ok(())
+        config.validate()
     }
 
     /// Create a new IPFixParser with a custom template cache size and optional TTL configuration.