fixup! Combine `patition_relays` and `matches` into `filter`relay-selector-refactor

2 files changed, 81 insertions, 114 deletions

diff --git a/mullvad-relay-selector/src/relay_selector/filter.rs b/mullvad-relay-selector/src/relay_selector/filter.rs
index b88c6199ff..33eb2040d9 100644
--- a/mullvad-relay-selector/src/relay_selector/filter.rs
+++ b/mullvad-relay-selector/src/relay_selector/filter.rs
@@ -3,10 +3,10 @@ use super::{
     endpoint_set::{RelayEndpointSet, Verdict, VerdictExt},
 };
 use mullvad_types::{
-    constraints::Constraint,
+    constraints::{Constraint, Match},
     custom_list::CustomListsSettings,
-    relay_constraints::LocationConstraint,
-    relay_list::WireguardRelay,
+    relay_constraints::{GeographicLocationConstraint, LocationConstraint},
+    relay_list::{WireguardRelay, WireguardRelayEndpointData},
     relay_selector::{
         EntryConstraints, EntrySpecificConstraints, ExitConstraints, MultihopConstraints,
         Predicate, Reason, RelayPartitions,
@@ -190,8 +190,7 @@ impl RelaySelector {
         endpoint_set: &RelayEndpointSet,
         constraints: &EntrySpecificConstraints,
     ) -> Verdict {
-        let daita_on = constraints.daita;
-        let daita = matcher::filter_on_daita(daita_on, relay).if_false(Reason::Daita);
+        let daita = filter_on_daita(constraints.daita, relay).if_false(Reason::Daita);
 
         let obfuscation_verdict = endpoint_set.obfuscation_verdict(constraints);
         daita.and(obfuscation_verdict)
@@ -207,10 +206,8 @@ impl RelaySelector {
             ownership,
         }: &ExitConstraints,
     ) -> Verdict {
-        let ownership =
-            matcher::filter_on_ownership(ownership.as_ref(), relay).if_false(Reason::Ownership);
-        let providers =
-            matcher::filter_on_providers(providers.as_ref(), relay).if_false(Reason::Providers);
+        let ownership = ownership.matches(relay).if_false(Reason::Ownership);
+        let providers = providers.matches(relay).if_false(Reason::Providers);
         let location = self.location_criteria(relay, location);
         let active = relay.active.if_false(Reason::Inactive);
 
@@ -224,10 +221,10 @@ impl RelaySelector {
     ) -> Verdict {
         let custom_lists: CustomListsSettings = self.custom_lists();
 
-        let resolved =
-            matcher::ResolvedLocationConstraint::from_constraint(location.as_ref(), &custom_lists);
+        let location_constraint =
+            ResolvedLocationConstraint::from_constraint(location.as_ref(), &custom_lists);
 
-        if !matcher::filter_on_location(resolved.as_ref(), relay) {
+        if !location_constraint.matches(relay) {
             return Verdict::reject(Reason::Location);
         }
 
@@ -235,7 +232,7 @@ impl RelaySelector {
         // constraint only targets the country (or is unconstrained). A city- or
         // hostname-level constraint that matches the relay overrides this — the user
         // has made an explicit, specific choice.
-        if !relay.include_in_country && matcher::is_country_only_match(resolved.as_ref(), relay) {
+        if !relay.include_in_country && is_country_only_match(location_constraint.as_ref(), relay) {
             Verdict::reject(Reason::IncludeInCountry)
         } else {
             Verdict::Accept
@@ -289,116 +286,86 @@ fn move_unique_conflict(from: &RelayPartitions, into: &mut RelayPartitions) {
     into.discards.push((relay, vec![Reason::Conflict]));
 }
 
-mod matcher {
-    use mullvad_types::constraints::Constraint;
-    use mullvad_types::constraints::Match;
-    use mullvad_types::custom_list::CustomListsSettings;
-    use mullvad_types::relay_constraints::GeographicLocationConstraint;
-    use mullvad_types::relay_constraints::LocationConstraint;
-    use mullvad_types::relay_constraints::Ownership;
-    use mullvad_types::relay_constraints::Providers;
-    use mullvad_types::relay_list::WireguardRelay;
-    use mullvad_types::relay_list::WireguardRelayEndpointData;
-
-    /// Returns whether `relay` satisfy the location constraint posed by `filter`.
-    pub fn filter_on_location(
-        filter: Constraint<&ResolvedLocationConstraint<'_>>,
-        relay: &WireguardRelay,
-    ) -> bool {
-        filter.matches(relay)
-    }
-
-    /// Returns whether `relay` satisfy the ownership constraint posed by `filter`.
-    pub fn filter_on_ownership(filter: Constraint<&Ownership>, relay: &WireguardRelay) -> bool {
-        filter.matches(relay)
-    }
-
-    /// Returns whether `relay` satisfy the providers constraint posed by `filter`.
-    pub fn filter_on_providers(filter: Constraint<&Providers>, relay: &WireguardRelay) -> bool {
-        filter.matches(relay)
-    }
-
-    /// Returns whether `relay` satisfy the daita constraint posed by `filter`.
-    pub fn filter_on_daita(filter: Constraint<bool>, relay: &WireguardRelay) -> bool {
-        match (filter, &relay.endpoint_data) {
-            // Only a subset of relays support DAITA, so filter out ones that don't.
-            (Constraint::Only(true), WireguardRelayEndpointData { daita, .. }) => *daita,
-            // If we don't require DAITA, any relay works.
-            _ => true,
-        }
+/// Returns whether `relay` satisfy the daita constraint posed by `filter`.
+pub fn filter_on_daita(filter: Constraint<bool>, relay: &WireguardRelay) -> bool {
+    match (filter, &relay.endpoint_data) {
+        // Only a subset of relays support DAITA, so filter out ones that don't.
+        (Constraint::Only(true), WireguardRelayEndpointData { daita, .. }) => *daita,
+        // If we don't require DAITA, any relay works.
+        _ => true,
     }
+}
 
-    /// Returns `true` if no city- or hostname-level [`GeographicLocationConstraint`] in `location`
-    /// matches `relay`. This covers both `Constraint::Any` (no constraint at all, meaning the relay
-    /// is not specifically targeted) and constraints that only mention the relay's country.
-    ///
-    /// Used to determine whether a relay with `include_in_country = false` should be treated as a
-    /// fallback: if the user has pinpointed a specific city or hostname that contains this relay,
-    /// we honour that explicit choice and promote it to a primary match.
-    pub fn is_country_only_match(
-        location: Constraint<&ResolvedLocationConstraint<'_>>,
-        relay: &WireguardRelay,
-    ) -> bool {
-        match location {
-            // No location constraint — relay is not specifically targeted.
-            Constraint::Any => true,
-            Constraint::Only(resolved) => {
-                // It is a country-only match as long as none of the matching constraints
-                // is more specific than a country (i.e. city or hostname).
-                !resolved
-                    .into_iter()
-                    .filter(|loc| loc.matches(relay))
-                    .any(|loc| !loc.is_country())
-            }
+/// Returns `true` if no city- or hostname-level [`GeographicLocationConstraint`] in `location`
+/// matches `relay`. This covers both `Constraint::Any` (no constraint at all, meaning the relay
+/// is not specifically targeted) and constraints that only mention the relay's country.
+///
+/// Used to determine whether a relay with `include_in_country = false` should be treated as a
+/// fallback: if the user has pinpointed a specific city or hostname that contains this relay,
+/// we honour that explicit choice and promote it to a primary match.
+pub fn is_country_only_match(
+    location: Constraint<&ResolvedLocationConstraint<'_>>,
+    relay: &WireguardRelay,
+) -> bool {
+    match location {
+        // No location constraint — relay is not specifically targeted.
+        Constraint::Any => true,
+        Constraint::Only(resolved) => {
+            // It is a country-only match as long as none of the matching constraints
+            // is more specific than a country (i.e. city or hostname).
+            !resolved
+                .into_iter()
+                .filter(|loc| loc.matches(relay))
+                .any(|loc| !loc.is_country())
         }
     }
+}
 
-    /// Wrapper around [`GeographicLocationConstraint`].
-    /// Useful for iterating over a set of [`GeographicLocationConstraint`] where custom lists
-    /// are considered.
-    #[derive(Debug, Clone)]
-    pub struct ResolvedLocationConstraint<'a>(Vec<&'a GeographicLocationConstraint>);
+/// Wrapper around [`GeographicLocationConstraint`].
+/// Useful for iterating over a set of [`GeographicLocationConstraint`] where custom lists
+/// are considered.
+#[derive(Debug, Clone)]
+pub struct ResolvedLocationConstraint<'a>(Vec<&'a GeographicLocationConstraint>);
 
-    impl<'a> ResolvedLocationConstraint<'a> {
-        /// Define the mapping from a [location][`LocationConstraint`] and a set of
-        /// [custom lists][`CustomListsSettings`] to [`ResolvedLocationConstraint`].
-        pub fn from_constraint(
-            location_constraint: Constraint<&'a LocationConstraint>,
-            custom_lists: &'a CustomListsSettings,
-        ) -> Constraint<ResolvedLocationConstraint<'a>> {
-            match location_constraint {
-                Constraint::Any => Constraint::Any,
-                Constraint::Only(location) => Constraint::Only(match location {
-                    LocationConstraint::Location(location) => {
-                        ResolvedLocationConstraint(vec![location])
-                    }
-                    LocationConstraint::CustomList { list_id } => custom_lists
-                        .iter()
-                        .find(|list| list.id() == *list_id)
-                        .map(|custom_list| {
-                            ResolvedLocationConstraint(custom_list.locations.iter().collect())
-                        })
-                        .unwrap_or_else(|| {
-                            log::warn!("Resolved non-existent custom list with id {list_id:?}");
-                            ResolvedLocationConstraint(vec![])
-                        }),
-                }),
-            }
+impl<'a> ResolvedLocationConstraint<'a> {
+    /// Define the mapping from a [location][`LocationConstraint`] and a set of
+    /// [custom lists][`CustomListsSettings`] to [`ResolvedLocationConstraint`].
+    pub fn from_constraint(
+        location_constraint: Constraint<&'a LocationConstraint>,
+        custom_lists: &'a CustomListsSettings,
+    ) -> Constraint<ResolvedLocationConstraint<'a>> {
+        match location_constraint {
+            Constraint::Any => Constraint::Any,
+            Constraint::Only(location) => Constraint::Only(match location {
+                LocationConstraint::Location(location) => {
+                    ResolvedLocationConstraint(vec![location])
+                }
+                LocationConstraint::CustomList { list_id } => custom_lists
+                    .iter()
+                    .find(|list| list.id() == *list_id)
+                    .map(|custom_list| {
+                        ResolvedLocationConstraint(custom_list.locations.iter().collect())
+                    })
+                    .unwrap_or_else(|| {
+                        log::warn!("Resolved non-existent custom list with id {list_id:?}");
+                        ResolvedLocationConstraint(vec![])
+                    }),
+            }),
         }
     }
+}
 
-    impl<'a> IntoIterator for &'a ResolvedLocationConstraint<'a> {
-        type Item = &'a GeographicLocationConstraint;
-        type IntoIter = std::iter::Copied<std::slice::Iter<'a, &'a GeographicLocationConstraint>>;
+impl<'a> IntoIterator for &'a ResolvedLocationConstraint<'a> {
+    type Item = &'a GeographicLocationConstraint;
+    type IntoIter = std::iter::Copied<std::slice::Iter<'a, &'a GeographicLocationConstraint>>;
 
-        fn into_iter(self) -> Self::IntoIter {
-            self.0.iter().copied()
-        }
+    fn into_iter(self) -> Self::IntoIter {
+        self.0.iter().copied()
     }
+}
 
-    impl Match<WireguardRelay> for &ResolvedLocationConstraint<'_> {
-        fn matches(&self, relay: &WireguardRelay) -> bool {
-            self.into_iter().any(|location| location.matches(relay))
-        }
+impl Match<WireguardRelay> for ResolvedLocationConstraint<'_> {
+    fn matches(&self, relay: &WireguardRelay) -> bool {
+        self.into_iter().any(|location| location.matches(relay))
     }
 }
diff --git a/mullvad-types/src/relay_constraints.rs b/mullvad-types/src/relay_constraints.rs
index 3baee6f0ac..667b80a684 100644
--- a/mullvad-types/src/relay_constraints.rs
+++ b/mullvad-types/src/relay_constraints.rs
@@ -248,7 +248,7 @@ impl Ownership {
     }
 }
 
-impl Match<WireguardRelay> for &Ownership {
+impl Match<WireguardRelay> for Ownership {
     fn matches(&self, relay: &WireguardRelay) -> bool {
         match self {
             Ownership::MullvadOwned => relay.owned,
@@ -321,7 +321,7 @@ impl Providers {
     }
 }
 
-impl Match<WireguardRelay> for &Providers {
+impl Match<WireguardRelay> for Providers {
     fn matches(&self, relay: &WireguardRelay) -> bool {
         self.providers.contains(&relay.provider)
     }