Merge pull request #483 from brave/v0.10-cleanup

v0.10 cleanup

Author: antonok-edm

src/data_format/mod.rs

@@ -11,7 +11,7 @@ pub(crate) mod utils;
 
 use crate::blocker::Blocker;
 use crate::cosmetic_filter_cache::CosmeticFilterCache;
-use crate::network_filter_list::FlatBufferParsingError;
+use crate::network_filter_list::NetworkFilterListParsingError;
 
 /// Newer formats start with this magic byte sequence.
 /// Calculated as the leading 4 bytes of `echo -n 'brave/adblock-rust' | sha512sum`.
@@ -34,8 +34,8 @@ pub enum DeserializationError {
     RmpSerdeError(rmp_serde::decode::Error),
     UnsupportedFormatVersion(u8),
     NoHeaderFound,
-    LegacyFormatNoLongerSupported, // not used, for backward compatibility
-    FlatBufferParsingError(FlatBufferParsingError),
+    FlatBufferParsingError(flatbuffers::InvalidFlatbuffer),
+    ValidationError,
 }
 
 impl From<std::convert::Infallible> for DeserializationError {
@@ -50,9 +50,14 @@ impl From<rmp_serde::decode::Error> for DeserializationError {
     }
 }
 
-impl From<FlatBufferParsingError> for DeserializationError {
-    fn from(e: FlatBufferParsingError) -> Self {
-        Self::FlatBufferParsingError(e)
+impl From<NetworkFilterListParsingError> for DeserializationError {
+    fn from(e: NetworkFilterListParsingError) -> Self {
+        match e {
+            NetworkFilterListParsingError::InvalidFlatbuffer(invalid_flatbuffer) => {
+                Self::FlatBufferParsingError(invalid_flatbuffer)
+            }
+            NetworkFilterListParsingError::UniqueDomainsOutOfBounds(_) => Self::ValidationError,
+        }
     }
 }
 

src/engine.rs

@@ -250,8 +250,11 @@ impl Engine {
         crate::data_format::serialize_engine(&self.blocker, &self.cosmetic_cache)
     }
 
-    /// Deserialize the `Engine` from the binary format generated by `Engine::serialize`. The
-    /// method will automatically select the correct deserialization implementation.
+    /// Deserialize the `Engine` from the binary format generated by `Engine::serialize`.
+    ///
+    /// Note that the binary format has a built-in version number that may be incremented. There is
+    /// no guarantee that later versions of the format will be deserializable across minor versions
+    /// of adblock-rust; the format is provided only as a caching optimization.
     pub fn deserialize(
         &mut self,
         serialized: &[u8],

src/filters/fb_network.rs

@@ -1,3 +1,5 @@
+//! Flatbuffer-compatible versions of [NetworkFilter] and related functionality.
+
 use std::collections::HashMap;
 use std::vec;
 
@@ -18,6 +20,7 @@ use crate::utils::{Hash, ShortHash};
 pub mod flat;
 use flat::fb;
 
+/// Builder for [NetworkFilterList].
 pub(crate) struct FlatNetworkFiltersListBuilder<'a> {
     builder: flatbuffers::FlatBufferBuilder<'a>,
     filters: Vec<WIPOffset<fb::NetworkFilter<'a>>>,
@@ -155,6 +158,8 @@ impl FlatNetworkFiltersListBuilder<'_> {
         VerifiedFlatFilterListMemory::from_builder(&self.builder)
     }
 }
+
+/// A list of string parts that can be matched against a URL.
 pub(crate) struct FlatPatterns<'a> {
     patterns: Option<flatbuffers::Vector<'a, flatbuffers::ForwardsUOffset<&'a str>>>,
 }
@@ -177,6 +182,7 @@ impl<'a> FlatPatterns<'a> {
     }
 }
 
+/// Iterator over [FlatPatterns].
 pub(crate) struct FlatPatternsIterator<'a> {
     patterns: &'a FlatPatterns<'a>,
     len: usize,
@@ -206,6 +212,7 @@ impl ExactSizeIterator for FlatPatternsIterator<'_> {
     }
 }
 
+/// Internal implementation of [NetworkFilter] that is compatible with flatbuffers.
 pub(crate) struct FlatNetworkFilter<'a> {
     key: u64,
     owner: &'a NetworkFilterList,

src/filters/flat_filter_map.rs

@@ -1,3 +1,5 @@
+//! Holds the implementation of [FlatFilterMap].
+
 use flatbuffers::{Follow, ForwardsUOffset, Vector};
 use std::cmp::PartialOrd;
 
@@ -8,7 +10,7 @@ pub(crate) struct FlatFilterMap<'a, I: PartialOrd + Copy, V> {
     values: Vector<'a, ForwardsUOffset<V>>,
 }
 
-/// Iterator over NetworkFilter objects from FlatFilterMap
+/// Iterator over NetworkFilter objects from [FlatFilterMap]
 pub(crate) struct FlatFilterMapIterator<'a, I: PartialOrd + Copy, V> {
     current_index: usize,
     key: I,
@@ -39,9 +41,9 @@ where
 }
 
 impl<'a, I: PartialOrd + Copy, V> FlatFilterMap<'a, I, V> {
-    // Construct FlatFilterMap from two vectors:
-    // - index: sorted array of keys
-    // - values: array of values, same length as index
+    /// Construct [FlatFilterMap] from two vectors:
+    /// - index: sorted array of keys
+    /// - values: array of values, same length as index
     pub fn new(index: &'a [I], values: Vector<'a, ForwardsUOffset<V>>) -> Self {
         // Sanity check the size are equal. Note: next() will handle |values| correctly.
         debug_assert!(index.len() == values.len());

src/filters/mod.rs

@@ -4,7 +4,7 @@ mod abstract_network;
 mod network_matchers;
 
 pub mod cosmetic;
-pub mod fb_network;
+pub(crate) mod fb_network;
 pub(crate) mod flat_filter_map;
 pub mod network;
 pub(crate) mod unsafe_tools;

src/network_filter_list.rs

@@ -1,3 +1,5 @@
+//! Holds the implementation of [NetworkFilterList] and related functionality.
+
 use std::{collections::HashMap, collections::HashSet, fmt};
 
 use crate::filters::fb_network::flat::fb;
@@ -12,6 +14,8 @@ use crate::regex_manager::RegexManager;
 use crate::request::Request;
 use crate::utils::{fast_hash, to_short_hash, Hash, ShortHash};
 
+/// Holds relevant information from a single matchin gnetwork filter rule as a result of querying a
+/// [NetworkFilterList] for a given request.
 pub struct CheckResult {
     pub filter_mask: NetworkFilterMask,
     pub modifier_option: Option<String>,
@@ -46,11 +50,12 @@ impl NetworkFilterMaskHelper for CheckResult {
 }
 
 #[derive(Debug, Clone)]
-pub enum FlatBufferParsingError {
+pub enum NetworkFilterListParsingError {
     InvalidFlatbuffer(flatbuffers::InvalidFlatbuffer),
     UniqueDomainsOutOfBounds(usize),
 }
 
+/// Internal structure to keep track of a collection of network filters.
 pub(crate) struct NetworkFilterList {
     pub(crate) memory: VerifiedFlatFilterListMemory,
     pub(crate) unique_domains_hashes_map: HashMap<Hash, u32>,
@@ -68,19 +73,19 @@ impl Default for NetworkFilterList {
 }
 
 impl NetworkFilterList {
-    /// Create a new NetworkFilterList from raw memory (includes verification).
+    /// Create a new [NetworkFilterList] from raw memory (includes verification).
     pub(crate) fn try_from_unverified_memory(
         flatbuffer_memory: Vec<u8>,
-    ) -> Result<NetworkFilterList, FlatBufferParsingError> {
+    ) -> Result<NetworkFilterList, NetworkFilterListParsingError> {
         let memory = VerifiedFlatFilterListMemory::from_raw(flatbuffer_memory)
-            .map_err(FlatBufferParsingError::InvalidFlatbuffer)?;
+            .map_err(NetworkFilterListParsingError::InvalidFlatbuffer)?;
 
         Self::try_from_verified_memory(memory)
     }
 
     pub(crate) fn try_from_verified_memory(
         memory: VerifiedFlatFilterListMemory,
-    ) -> Result<NetworkFilterList, FlatBufferParsingError> {
+    ) -> Result<NetworkFilterList, NetworkFilterListParsingError> {
         let root = memory.filter_list();
 
         // Reconstruct the unique_domains_hashes_map from the flatbuffer data
@@ -91,7 +96,7 @@ impl NetworkFilterList {
             unique_domains_hashes_map.insert(
                 hash,
                 u32::try_from(index)
-                    .map_err(|_| FlatBufferParsingError::UniqueDomainsOutOfBounds(index))?,
+                    .map_err(|_| NetworkFilterListParsingError::UniqueDomainsOutOfBounds(index))?,
             );
         }
 
@@ -185,6 +190,7 @@ impl NetworkFilterList {
 
         Self::try_from_verified_memory(memory).unwrap_or_default()
     }
+
     /// Returns the first found filter, if any, that matches the given request. The backing storage
     /// has a non-deterministic order, so this should be used for any category of filters where a
     /// match from each would be functionally equivalent. For example, if two different exception

tests/unit/engine.rs

@@ -173,17 +173,18 @@ mod tests {
         });
     }
 
-    const HASH_MISSMATCH_MSG: &str = r#"
-      Changed in the serialized format detected!
-      1. Update ADBLOCK_RUST_DAT_VERSION before updating the expectations
-      2. DON'T rely on an approach: it's backwards compatible with the old one
+    const HASH_MISMATCH_MSG: &str = r#"
+      A change has been detected in the serialized format! If the change is intentional:
+      1. Update ADBLOCK_RUST_DAT_VERSION before updating the expected hashes
+      2. DON'T rely on backwards compatibility with the old format
       Backwards compatibility isn't covered by the tests"#;
 
     #[test]
     fn deserialization_generate_simple() {
         let mut engine = Engine::from_rules(["ad-banner"], Default::default());
         let data = engine.serialize().unwrap();
-        assert_eq!(hash(&data), 5723845290597955159, "{}", HASH_MISSMATCH_MSG);
+        const EXPECTED_HASH: u64 = 5723845290597955159;
+        assert_eq!(hash(&data), EXPECTED_HASH, "{}", HASH_MISMATCH_MSG);
         engine.deserialize(&data).unwrap();
     }
 
@@ -192,7 +193,8 @@ mod tests {
         let mut engine = Engine::from_rules(["ad-banner$tag=abc"], Default::default());
         engine.use_tags(&["abc"]);
         let data = engine.serialize().unwrap();
-        assert_eq!(hash(&data), 9626816743810307798, "{}", HASH_MISSMATCH_MSG);
+        const EXPECTED_HASH: u64 = 9626816743810307798;
+        assert_eq!(hash(&data), EXPECTED_HASH, "{}", HASH_MISMATCH_MSG);
         engine.deserialize(&data).unwrap();
     }
 
@@ -222,7 +224,7 @@ mod tests {
             6839468684492187294
         };
 
-        assert_eq!(hash(&data), expected_hash, "{}", HASH_MISSMATCH_MSG);
+        assert_eq!(hash(&data), expected_hash, "{}", HASH_MISMATCH_MSG);
 
         engine.deserialize(&data).unwrap();
     }

tests/unit/filters/network.rs

@@ -1174,56 +1174,6 @@ mod parse_tests {
         }
     }
 
-    #[test]
-    fn binary_serialization_works() {
-        use rmp_serde::{Deserializer, Serializer};
-        {
-            let filter =
-                NetworkFilter::parse("||foo.com/bar/baz$important", true, Default::default())
-                    .unwrap();
-
-            let mut encoded = Vec::new();
-            filter
-                .serialize(&mut Serializer::new(&mut encoded))
-                .unwrap();
-            let mut de = Deserializer::new(&encoded[..]);
-            let decoded: NetworkFilter = Deserialize::deserialize(&mut de).unwrap();
-
-            let mut defaults = default_network_filter_breakdown();
-            defaults.hostname = Some(String::from("foo.com"));
-            defaults.filter = Some(String::from("/bar/baz"));
-            defaults.is_plain = true;
-            defaults.is_hostname_anchor = true;
-            defaults.is_important = true;
-            defaults.is_left_anchor = true;
-            assert_eq!(defaults, NetworkFilterBreakdown::from(&decoded))
-        }
-        {
-            let filter = NetworkFilter::parse("||foo.com*bar^", true, Default::default()).unwrap();
-            let mut defaults = default_network_filter_breakdown();
-            defaults.hostname = Some(String::from("foo.com"));
-            defaults.filter = Some(String::from("bar^"));
-            defaults.is_hostname_anchor = true;
-            defaults.is_regex = true;
-            defaults.is_plain = false;
-
-            let mut encoded = Vec::new();
-            filter
-                .serialize(&mut Serializer::new(&mut encoded))
-                .unwrap();
-            let mut de = Deserializer::new(&encoded[..]);
-            let decoded: NetworkFilter = Deserialize::deserialize(&mut de).unwrap();
-
-            assert_eq!(defaults, NetworkFilterBreakdown::from(&decoded));
-            assert!(RegexManager::default().matches(
-                decoded.mask,
-                decoded.filter.iter(),
-                (&decoded as *const NetworkFilter) as u64,
-                "bar/"
-            ));
-        }
-    }
-
     #[test]
     fn parse_empty_host_anchor_exception() {
         let filter_parsed =