Add settings migration code, refactor and cleanup

Bump the settings version. Add code for migrating settings to new
version since it is now not backwards compatible.
Refactor LocationConstraint and related types to be more lean.
Cleanup issues and fix formatting.
Refactor LocationConstraint and add migration code

10 files changed, 338 insertions, 68 deletions

diff --git a/android/app/src/main/kotlin/net/mullvad/mullvadvpn/ui/serviceconnection/RelayListListener.kt b/android/app/src/main/kotlin/net/mullvad/mullvadvpn/ui/serviceconnection/RelayListListener.kt
index 5efca2648c..c6142694b9 100644
--- a/android/app/src/main/kotlin/net/mullvad/mullvadvpn/ui/serviceconnection/RelayListListener.kt
+++ b/android/app/src/main/kotlin/net/mullvad/mullvadvpn/ui/serviceconnection/RelayListListener.kt
@@ -6,7 +6,6 @@ import net.mullvad.mullvadvpn.ipc.EventDispatcher
 import net.mullvad.mullvadvpn.ipc.Request
 import net.mullvad.mullvadvpn.model.Constraint
 import net.mullvad.mullvadvpn.model.GeographicLocationConstraint
-import net.mullvad.mullvadvpn.model.LocationConstraint
 import net.mullvad.mullvadvpn.model.PortRange
 import net.mullvad.mullvadvpn.model.RelayConstraints
 import net.mullvad.mullvadvpn.model.RelaySettings
diff --git a/mullvad-cli/src/cmds/bridge.rs b/mullvad-cli/src/cmds/bridge.rs
index e43fce1b51..3cc1a95b37 100644
--- a/mullvad-cli/src/cmds/bridge.rs
+++ b/mullvad-cli/src/cmds/bridge.rs
@@ -168,7 +168,7 @@ impl Bridge {
                     } else {
                         Constraint::from(location)
                     };
-                let location = location.map(|location| LocationConstraint::Location { location });
+                let location = location.map(LocationConstraint::Location);
                 Self::update_bridge_settings(&mut rpc, Some(location), None, None).await
             }
             SetCommands::CustomList { custom_list_name } => {
diff --git a/mullvad-cli/src/cmds/relay.rs b/mullvad-cli/src/cmds/relay.rs
index 1bb395c4e5..83f4ad4fdd 100644
--- a/mullvad-cli/src/cmds/relay.rs
+++ b/mullvad-cli/src/cmds/relay.rs
@@ -450,7 +450,7 @@ impl Relay {
                 // The country field is assumed to be hostname due to CLI argument parsing
                 find_relay_by_hostname(&countries, &location_constraint_args.country)
             {
-                Constraint::Only(LocationConstraint::Location { location: relay })
+                Constraint::Only(LocationConstraint::Location(relay))
             } else {
                 let location_constraint: Constraint<GeographicLocationConstraint> =
                     Constraint::from(location_constraint_args);
@@ -468,7 +468,7 @@ impl Relay {
                         }
                     }
                 }
-                location_constraint.map(|location| LocationConstraint::Location { location })
+                location_constraint.map(LocationConstraint::Location)
             };
 
         Self::update_constraints(RelaySettingsUpdate::Normal(RelayConstraintsUpdate {
@@ -576,7 +576,7 @@ impl Relay {
                 // The country field is assumed to be hostname due to CLI argument parsing
                 wireguard_constraints.entry_location =
                     if let Some(relay) = find_relay_by_hostname(&countries, &entry.country) {
-                        Constraint::Only(LocationConstraint::Location { location: relay })
+                        Constraint::Only(LocationConstraint::Location(relay))
                     } else {
                         Constraint::from(entry)
                     };
diff --git a/mullvad-cli/src/cmds/relay_constraints.rs b/mullvad-cli/src/cmds/relay_constraints.rs
index dc79e79f0b..626754edcb 100644
--- a/mullvad-cli/src/cmds/relay_constraints.rs
+++ b/mullvad-cli/src/cmds/relay_constraints.rs
@@ -49,6 +49,6 @@ impl From<LocationArgs> for Constraint<LocationConstraint> {
             }
             _ => unreachable!("invalid location arguments"),
         };
-        Constraint::Only(LocationConstraint::Location { location })
+        Constraint::Only(LocationConstraint::Location(location))
     }
 }
diff --git a/mullvad-daemon/src/migrations/v6.rs b/mullvad-daemon/src/migrations/v6.rs
index 54dc2268e4..3a842923f0 100644
--- a/mullvad-daemon/src/migrations/v6.rs
+++ b/mullvad-daemon/src/migrations/v6.rs
@@ -36,16 +36,55 @@ pub fn migrate(settings: &mut serde_json::Value) -> Result<()> {
 
     migrate_udp2tcp_port_443(settings);
 
-    // TODO
-    // log::info!("Migrating settings format to V7");
+    migrate_location_constraint(settings)?;
 
-    // Note: Not incrementing the version number yet, since this migration is still open
-    // for future modification.
-    // settings["settings_version"] = serde_json::json!(SettingsVersion::V7);
+    log::info!("Migrating settings format to V7");
+
+    settings["settings_version"] = serde_json::json!(SettingsVersion::V7);
+
+    Ok(())
+}
+
+fn migrate_location_constraint(settings: &mut serde_json::Value) -> Result<()> {
+    if let Some(location) = settings
+        .get_mut("relay_settings")
+        .and_then(|relay_settings| relay_settings.get_mut("normal"))
+        .and_then(|normal_relay_settings| normal_relay_settings.get_mut("location"))
+    {
+        wrap_location(location)?;
+    }
+
+    if let Some(location) = settings
+        .get_mut("relay_settings")
+        .and_then(|relay_settings| relay_settings.get_mut("normal"))
+        .and_then(|normal_relay_settings| normal_relay_settings.get_mut("wireguard_constraints"))
+        .and_then(|normal_relay_settings| normal_relay_settings.get_mut("entry_location"))
+    {
+        wrap_location(location)?;
+    }
+
+    if let Some(location) = settings
+        .get_mut("bridge_settings")
+        .and_then(|relay_settings| relay_settings.get_mut("normal"))
+        .and_then(|normal_relay_settings| normal_relay_settings.get_mut("location"))
+    {
+        wrap_location(location)?;
+    }
 
     Ok(())
 }
 
+fn wrap_location(location: &mut serde_json::Value) -> Result<()> {
+    if let Some(only) = location.get_mut("only") {
+        only["location"] = only.clone();
+        let only = only.as_object_mut().ok_or(Error::InvalidSettingsContent)?;
+        only.remove("country");
+        only.remove("city");
+        only.remove("hostname");
+    }
+    Ok(())
+}
+
 fn migrate_pq_setting(settings: &mut serde_json::Value) -> Result<()> {
     if let Some(tunnel_options) = settings
         .get_mut("tunnel_options")
@@ -89,7 +128,7 @@ fn version_matches(settings: &mut serde_json::Value) -> bool {
 
 #[cfg(test)]
 mod test {
-    use super::{migrate, migrate_pq_setting, version_matches};
+    use super::{migrate, migrate_location_constraint, migrate_pq_setting, version_matches};
 
     pub const V6_SETTINGS: &str = r#"
 {
@@ -175,7 +214,9 @@ mod test {
     "normal": {
       "location": {
         "only": {
-          "country": "se"
+          "location": {
+            "country": "se"
+          }
         }
       },
       "tunnel_protocol": "any",
@@ -241,7 +282,7 @@ mod test {
       }
     }
   },
-  "settings_version": 6
+  "settings_version": 7
 }
 "#;
 
@@ -256,6 +297,252 @@ mod test {
         assert_eq!(&old_settings, &new_settings);
     }
 
+    /// For relay settings
+    /// location: { only: { country : "se" } } should be replaced with
+    /// location: { only: { location: { country: "se" } } }
+    #[test]
+    fn test_from_relay_settings_location_constraint_country() {
+        let mut migrated_settings: serde_json::Value = serde_json::from_str(
+            r#"
+        {
+            "relay_settings": {
+                "normal": {
+                    "location": {
+                        "only": {
+                            "country": "se"
+                        }
+                    }
+                }
+            }
+        }
+        "#,
+        )
+        .unwrap();
+        migrate_location_constraint(&mut migrated_settings).unwrap();
+
+        let expected_settings: serde_json::Value = serde_json::from_str(
+            r#"
+        {
+            "relay_settings": {
+                "normal": {
+                    "location": {
+                        "only": {
+                            "location": {
+                                "country": "se"
+                            }
+                        }
+                    }
+                }
+            }
+        }
+        "#,
+        )
+        .unwrap();
+
+        assert_eq!(migrated_settings, expected_settings);
+    }
+
+    /// For relay settings
+    /// location: { only: { country : "se", city: "got" } } should be replaced with
+    /// location: { only: { location: { country: "se", city: "got" } } }
+    #[test]
+    fn test_from_relay_settings_location_constraint_city() {
+        let mut migrated_settings: serde_json::Value = serde_json::from_str(
+            r#"
+        {
+            "relay_settings": {
+                "normal": {
+                    "location": {
+                        "only": {
+                            "country": "se",
+                            "city": "got"
+                        }
+                    }
+                }
+            }
+        }
+        "#,
+        )
+        .unwrap();
+        migrate_location_constraint(&mut migrated_settings).unwrap();
+
+        let expected_settings: serde_json::Value = serde_json::from_str(
+            r#"
+        {
+            "relay_settings": {
+                "normal": {
+                    "location": {
+                        "only": {
+                            "location": {
+                                "country": "se",
+                                "city": "got"
+                            }
+                        }
+                    }
+                }
+            }
+        }
+        "#,
+        )
+        .unwrap();
+
+        assert_eq!(migrated_settings, expected_settings);
+    }
+
+    /// For relay settings
+    /// location: { only: { country : "se", city: "got", hostname: "se-got-wg-001" } } should be
+    /// replaced with location: { only: { location: { country: "se", city: "got", hostname:
+    /// "se-got-wg-001" } } }
+    #[test]
+    fn test_from_relay_settings_location_constraint_hostname() {
+        let mut migrated_settings: serde_json::Value = serde_json::from_str(
+            r#"
+        {
+            "relay_settings": {
+                "normal": {
+                    "location": {
+                        "only": {
+                            "country": "se",
+                            "city": "got",
+                            "hostname": "se-got-wg-001"
+                        }
+                    }
+                }
+            }
+        }
+        "#,
+        )
+        .unwrap();
+        migrate_location_constraint(&mut migrated_settings).unwrap();
+
+        let expected_settings: serde_json::Value = serde_json::from_str(
+            r#"
+        {
+            "relay_settings": {
+                "normal": {
+                    "location": {
+                        "only": {
+                            "location": {
+                                "country": "se",
+                                "city": "got",
+                                "hostname": "se-got-wg-001"
+                            }
+                        }
+                    }
+                }
+            }
+        }
+        "#,
+        )
+        .unwrap();
+
+        assert_eq!(migrated_settings, expected_settings);
+    }
+
+    /// For bridge settings
+    /// location: { only: { country : "se", city: "got", hostname: "se-got-wg-001" } } should be
+    /// replaced with location: { only: { location: { country: "se", city: "got", hostname:
+    /// "se-got-wg-001" } } }
+    #[test]
+    fn test_from_bridge_location_constraint_hostname() {
+        let mut migrated_settings: serde_json::Value = serde_json::from_str(
+            r#"
+        {
+            "bridge_settings": {
+                "normal": {
+                    "location": {
+                        "only": {
+                            "country": "se",
+                            "city": "got",
+                            "hostname": "se-got-wg-001"
+                        }
+                    }
+                }
+            }
+        }
+        "#,
+        )
+        .unwrap();
+        migrate_location_constraint(&mut migrated_settings).unwrap();
+
+        let expected_settings: serde_json::Value = serde_json::from_str(
+            r#"
+        {
+            "bridge_settings": {
+                "normal": {
+                    "location": {
+                        "only": {
+                            "location": {
+                                "country": "se",
+                                "city": "got",
+                                "hostname": "se-got-wg-001"
+                            }
+                        }
+                    }
+                }
+            }
+        }
+        "#,
+        )
+        .unwrap();
+
+        assert_eq!(migrated_settings, expected_settings);
+    }
+
+    /// For wireguard constraints
+    /// location: { only: { country : "se", city: "got", hostname: "se-got-wg-001" } } should be
+    /// replaced with location: { only: { location: { country: "se", city: "got", hostname:
+    /// "se-got-wg-001" } } }
+    #[test]
+    fn test_from_wireguard_constraint_location_constraint_hostname() {
+        let mut migrated_settings: serde_json::Value = serde_json::from_str(
+            r#"
+        {
+            "relay_settings": {
+                "normal": {
+                    "wireguard_constraints": {
+                        "entry_location": {
+                            "only": {
+                                "country": "se",
+                                "city": "got",
+                                "hostname": "se-got-wg-001"
+                            }
+                        }
+                    }
+                }
+            }
+        }
+        "#,
+        )
+        .unwrap();
+        migrate_location_constraint(&mut migrated_settings).unwrap();
+
+        let expected_settings: serde_json::Value = serde_json::from_str(
+            r#"
+        {
+            "relay_settings": {
+                "normal": {
+                    "wireguard_constraints": {
+                        "entry_location": {
+                            "only": {
+                                "location": {
+                                    "country": "se",
+                                    "city": "got",
+                                    "hostname": "se-got-wg-001"
+                                }
+                            }
+                        }
+                    }
+                }
+            }
+        }
+        "#,
+        )
+        .unwrap();
+
+        assert_eq!(migrated_settings, expected_settings);
+    }
+
     /// use_pq_safe_psk=false should be replaced with quantum_resistant=null
     #[test]
     fn test_from_pq_safe_psk_false() {
diff --git a/mullvad-daemon/src/settings.rs b/mullvad-daemon/src/settings.rs
index ee3f355598..794b0381ca 100644
--- a/mullvad-daemon/src/settings.rs
+++ b/mullvad-daemon/src/settings.rs
@@ -378,9 +378,7 @@ mod test {
                   "location": {
                     "only": {
                       "location": {
-                        "location": {
-                          "country": "gb"
-                        }
+                        "country": "gb"
                       }
                     }
                   },
diff --git a/mullvad-management-interface/src/types/conversions/relay_constraints.rs b/mullvad-management-interface/src/types/conversions/relay_constraints.rs
index 221c2140b2..e987ca102b 100644
--- a/mullvad-management-interface/src/types/conversions/relay_constraints.rs
+++ b/mullvad-management-interface/src/types/conversions/relay_constraints.rs
@@ -488,7 +488,7 @@ impl From<mullvad_types::relay_constraints::LocationConstraint> for proto::Locat
     fn from(location: mullvad_types::relay_constraints::LocationConstraint) -> Self {
         use mullvad_types::relay_constraints::LocationConstraint;
         match location {
-            LocationConstraint::Location { location } => Self {
+            LocationConstraint::Location(location) => Self {
                 r#type: Some(proto::location_constraint::Type::Location(
                     proto::RelayLocation::from(location),
                 )),
@@ -515,7 +515,7 @@ impl TryFrom<proto::LocationConstraint>
                 match location {
                     Constraint::Any => Ok(Constraint::Any),
                     Constraint::Only(location) => {
-                        Ok(Constraint::Only(LocationConstraint::Location { location }))
+                        Ok(Constraint::Only(LocationConstraint::Location(location)))
                     }
                 }
             }
diff --git a/mullvad-relay-selector/src/lib.rs b/mullvad-relay-selector/src/lib.rs
index 9fd6e3c6da..e62c63f3db 100644
--- a/mullvad-relay-selector/src/lib.rs
+++ b/mullvad-relay-selector/src/lib.rs
@@ -1564,7 +1564,7 @@ mod test {
                     BridgeState::Off,
                     attempt,
                     TunnelType::OpenVpn,
-                    &CustomListsSettings::default()
+                    &CustomListsSettings::default(),
                 );
                 assert_eq!(
                     preferred.tunnel_protocol,
@@ -1575,7 +1575,7 @@ mod test {
                     BridgeState::Off,
                     attempt,
                     TunnelType::OpenVpn,
-                    &CustomListsSettings::default()
+                    &CustomListsSettings::default(),
                 ) {
                     Ok(result) if matches!(result.endpoint, MullvadEndpoint::OpenVpn(_)) => (),
                     _ => panic!("OpenVPN endpoint was not selected"),
diff --git a/mullvad-types/src/relay_constraints.rs b/mullvad-types/src/relay_constraints.rs
index 18ab4eef03..ab78ceca70 100644
--- a/mullvad-types/src/relay_constraints.rs
+++ b/mullvad-types/src/relay_constraints.rs
@@ -235,27 +235,19 @@ impl RelaySettings {
     }
 }
 
-#[derive(Debug, Clone, Eq, PartialEq, Deserialize, Serialize)]
+#[derive(Debug, Clone, Eq, PartialEq, Serialize, Deserialize)]
 #[serde(rename_all = "snake_case")]
 #[cfg_attr(target_os = "android", derive(IntoJava, FromJava))]
 #[cfg_attr(target_os = "android", jnix(package = "net.mullvad.mullvadvpn.model"))]
 pub enum LocationConstraint {
-    Location {
-        location: GeographicLocationConstraint,
-    },
-    CustomList {
-        list_id: Id,
-    },
+    Location(GeographicLocationConstraint),
+    CustomList { list_id: Id },
 }
 
 #[derive(Debug, Clone)]
 pub enum ResolvedLocationConstraint {
-    Location {
-        location: GeographicLocationConstraint,
-    },
-    Locations {
-        locations: Vec<GeographicLocationConstraint>,
-    },
+    Location(GeographicLocationConstraint),
+    Locations(Vec<GeographicLocationConstraint>),
 }
 
 impl ResolvedLocationConstraint {
@@ -265,18 +257,14 @@ impl ResolvedLocationConstraint {
     ) -> Constraint<ResolvedLocationConstraint> {
         match location {
             Constraint::Any => Constraint::Any,
-            Constraint::Only(LocationConstraint::Location { location }) => {
-                Constraint::Only(Self::Location { location })
+            Constraint::Only(LocationConstraint::Location(location)) => {
+                Constraint::Only(Self::Location(location))
             }
             Constraint::Only(LocationConstraint::CustomList { list_id }) => custom_lists
                 .custom_lists
                 .iter()
                 .find(|custom_list| custom_list.id == list_id)
-                .map(|custom_list| {
-                    Constraint::Only(Self::Locations {
-                        locations: custom_list.locations.clone(),
-                    })
-                })
+                .map(|custom_list| Constraint::Only(Self::Locations(custom_list.locations.clone())))
                 .unwrap_or(Constraint::Any),
         }
     }
@@ -284,7 +272,7 @@ impl ResolvedLocationConstraint {
 
 impl From<GeographicLocationConstraint> for LocationConstraint {
     fn from(location: GeographicLocationConstraint) -> Self {
-        Self::Location { location }
+        Self::Location(location)
     }
 }
 
@@ -292,27 +280,23 @@ impl Set<Constraint<ResolvedLocationConstraint>> for Constraint<ResolvedLocation
     fn is_subset(&self, other: &Self) -> bool {
         match self {
             Constraint::Any => other.is_any(),
-            Constraint::Only(ResolvedLocationConstraint::Location { location }) => match other {
+            Constraint::Only(ResolvedLocationConstraint::Location(location)) => match other {
                 Constraint::Any => true,
-                Constraint::Only(ResolvedLocationConstraint::Location {
-                    location: other_location,
-                }) => location.is_subset(other_location),
-                Constraint::Only(ResolvedLocationConstraint::Locations {
-                    locations: other_locations,
-                }) => other_locations
-                    .iter()
-                    .any(|other_location| location.is_subset(other_location)),
+                Constraint::Only(ResolvedLocationConstraint::Location(other_location)) => {
+                    location.is_subset(other_location)
+                }
+                Constraint::Only(ResolvedLocationConstraint::Locations(other_locations)) => {
+                    other_locations
+                        .iter()
+                        .any(|other_location| location.is_subset(other_location))
+                }
             },
-            Constraint::Only(ResolvedLocationConstraint::Locations { locations }) => match other {
+            Constraint::Only(ResolvedLocationConstraint::Locations(locations)) => match other {
                 Constraint::Any => true,
-                Constraint::Only(ResolvedLocationConstraint::Location {
-                    location: other_location,
-                }) => locations
+                Constraint::Only(ResolvedLocationConstraint::Location(other_location)) => locations
                     .iter()
                     .all(|location| location.is_subset(other_location)),
-                Constraint::Only(ResolvedLocationConstraint::Locations {
-                    locations: other_locations,
-                }) => {
+                Constraint::Only(ResolvedLocationConstraint::Locations(other_locations)) => {
                     for location in locations {
                         if !other_locations
                             .iter()
@@ -332,10 +316,10 @@ impl Constraint<ResolvedLocationConstraint> {
     pub fn matches_with_opts(&self, relay: &Relay, ignore_include_in_country: bool) -> bool {
         match self {
             Constraint::Any => true,
-            Constraint::Only(ResolvedLocationConstraint::Location { location }) => {
+            Constraint::Only(ResolvedLocationConstraint::Location(location)) => {
                 location.matches_with_opts(relay, ignore_include_in_country)
             }
-            Constraint::Only(ResolvedLocationConstraint::Locations { locations }) => locations
+            Constraint::Only(ResolvedLocationConstraint::Locations(locations)) => locations
                 .iter()
                 .any(|loc| loc.matches_with_opts(relay, ignore_include_in_country)),
         }
@@ -345,7 +329,7 @@ impl Constraint<ResolvedLocationConstraint> {
 impl LocationConstraint {
     fn format(&self, f: &mut String, custom_lists: &CustomListsSettings) -> Result<(), fmt::Error> {
         match self {
-            Self::Location { location } => writeln!(f, "location - {location}"),
+            Self::Location(location) => writeln!(f, "location - {location}"),
             Self::CustomList { list_id } => match custom_lists
                 .custom_lists
                 .iter()
diff --git a/mullvad-types/src/settings/mod.rs b/mullvad-types/src/settings/mod.rs
index 5af6ecfe08..12f5d831e1 100644
--- a/mullvad-types/src/settings/mod.rs
+++ b/mullvad-types/src/settings/mod.rs
@@ -21,7 +21,7 @@ mod dns;
 /// latest version that exists in `SettingsVersion`.
 /// This should be bumped when a new version is introduced along with a migration
 /// being added to `mullvad-daemon`.
-pub const CURRENT_SETTINGS_VERSION: SettingsVersion = SettingsVersion::V6;
+pub const CURRENT_SETTINGS_VERSION: SettingsVersion = SettingsVersion::V7;
 
 #[derive(Debug, PartialEq, Eq, PartialOrd, Clone, Copy)]
 #[repr(u32)]
@@ -31,6 +31,7 @@ pub enum SettingsVersion {
     V4 = 4,
     V5 = 5,
     V6 = 6,
+    V7 = 7,
 }
 
 impl<'de> Deserialize<'de> for SettingsVersion {
@@ -44,6 +45,7 @@ impl<'de> Deserialize<'de> for SettingsVersion {
             v if v == SettingsVersion::V4 as u32 => Ok(SettingsVersion::V4),
             v if v == SettingsVersion::V5 as u32 => Ok(SettingsVersion::V5),
             v if v == SettingsVersion::V6 as u32 => Ok(SettingsVersion::V6),
+            v if v == SettingsVersion::V7 as u32 => Ok(SettingsVersion::V7),
             v => Err(serde::de::Error::custom(format!(
                 "{v} is not a valid SettingsVersion"
             ))),
@@ -121,13 +123,13 @@ impl Default for Settings {
     fn default() -> Self {
         Settings {
             relay_settings: RelaySettings::Normal(RelayConstraints {
-                location: Constraint::Only(LocationConstraint::Location {
-                    location: GeographicLocationConstraint::Country("se".to_owned()),
-                }),
+                location: Constraint::Only(LocationConstraint::Location(
+                    GeographicLocationConstraint::Country("se".to_owned()),
+                )),
                 wireguard_constraints: WireguardConstraints {
-                    entry_location: Constraint::Only(LocationConstraint::Location {
-                        location: GeographicLocationConstraint::Country("se".to_owned()),
-                    }),
+                    entry_location: Constraint::Only(LocationConstraint::Location(
+                        GeographicLocationConstraint::Country("se".to_owned()),
+                    )),
                     ..Default::default()
                 },
                 ..Default::default()