Check musicbrainz url when loading more carefully

src/core/collection/artist.rs

@@ -220,15 +220,15 @@ mod tests {
         assert_eq!(artist.musicbrainz, expected);
 
         // Setting a URL on an artist.
-        artist.set_musicbrainz_url(MUSICBRAINZ.try_into().unwrap());
+        artist.set_musicbrainz_url(MusicBrainz::artist_from_str(MUSICBRAINZ).unwrap());
-        _ = expected.insert(MUSICBRAINZ.try_into().unwrap());
+        _ = expected.insert(MusicBrainz::artist_from_str(MUSICBRAINZ).unwrap());
         assert_eq!(artist.musicbrainz, expected);
 
-        artist.set_musicbrainz_url(MUSICBRAINZ.try_into().unwrap());
+        artist.set_musicbrainz_url(MusicBrainz::artist_from_str(MUSICBRAINZ).unwrap());
         assert_eq!(artist.musicbrainz, expected);
 
-        artist.set_musicbrainz_url(MUSICBRAINZ_2.try_into().unwrap());
+        artist.set_musicbrainz_url(MusicBrainz::artist_from_str(MUSICBRAINZ_2).unwrap());
-        _ = expected.insert(MUSICBRAINZ_2.try_into().unwrap());
+        _ = expected.insert(MusicBrainz::artist_from_str(MUSICBRAINZ_2).unwrap());
         assert_eq!(artist.musicbrainz, expected);
 
         // Clearing URLs.

src/core/collection/musicbrainz.rs

@@ -1,7 +1,4 @@
-use std::{
+use std::fmt::{Debug, Display};
-    fmt::{Debug, Display},
-    str::FromStr,
-};
 
 use url::Url;
 use uuid::Uuid;
@@ -19,32 +16,50 @@ pub trait IMbid {
 pub struct MusicBrainz(Url);
 
 impl MusicBrainz {
-    /// Validate and wrap a MusicBrainz URL.
+    pub fn artist_from_str<S: AsRef<str>>(url: S) -> Result<Self, Error> {
-    pub fn new_from_str<S: AsRef<str>>(url: S) -> Result<Self, Error> {
+        Self::artist_from_url(url.as_ref().try_into()?)
-        let url = Url::parse(url.as_ref())?;
-        Self::new_from_url(url)
     }
 
-    /// Validate and wrap a MusicBrainz URL.
+    pub fn album_from_str<S: AsRef<str>>(url: S) -> Result<Self, Error> {
-    pub fn new_from_url(url: Url) -> Result<Self, Error> {
+        Self::album_from_url(url.as_ref().try_into()?)
+    }
+
+    pub fn artist_from_url(url: Url) -> Result<Self, Error> {
+        Self::new(url, "artist")
+    }
+
+    pub fn album_from_url(url: Url) -> Result<Self, Error> {
+        Self::new(url, "release-group")
+    }
+
+    fn new(url: Url, mb_type: &str) -> Result<Self, Error> {
         if !url
             .domain()
             .map(|u| u.ends_with("musicbrainz.org"))
             .unwrap_or(false)
         {
-            return Err(Self::invalid_url_error(url));
+            return Err(Self::invalid_url_error(url, mb_type));
         }
 
+        match url.path_segments().and_then(|mut ps| ps.nth(0)) {
+            Some(typ) => {
+                if typ != mb_type {
+                    return Err(Self::invalid_url_error(url, mb_type));
+                }
+            }
+            None => return Err(Self::invalid_url_error(url, mb_type)),
+        };
+
         match url.path_segments().and_then(|mut ps| ps.nth(1)) {
             Some(segment) => Uuid::try_parse(segment)?,
-            None => return Err(Self::invalid_url_error(url)),
+            None => return Err(Self::invalid_url_error(url, mb_type)),
         };
 
         Ok(MusicBrainz(url))
     }
 
-    fn invalid_url_error<U: Display>(url: U) -> Error {
+    fn invalid_url_error<U: Display>(url: U, mb_type: &str) -> Error {
-        Error::UrlError(format!("invalid MusicBrainz URL: {url}"))
+        Error::UrlError(format!("invalid {mb_type} MusicBrainz URL: {url}"))
     }
 }
 
@@ -54,39 +69,6 @@ impl AsRef<str> for MusicBrainz {
     }
 }
 
-impl FromStr for MusicBrainz {
-    type Err = Error;
-
-    fn from_str(s: &str) -> Result<Self, Self::Err> {
-        MusicBrainz::new_from_str(s)
-    }
-}
-
-// A blanket TryFrom would be better, but https://stackoverflow.com/a/64407892
-macro_rules! impl_try_from_for_musicbrainz {
-    ($from:ty) => {
-        impl TryFrom<$from> for MusicBrainz {
-            type Error = Error;
-
-            fn try_from(value: $from) -> Result<Self, Self::Error> {
-                MusicBrainz::new_from_str(value)
-            }
-        }
-    };
-}
-
-impl_try_from_for_musicbrainz!(&str);
-impl_try_from_for_musicbrainz!(&String);
-impl_try_from_for_musicbrainz!(String);
-
-impl TryFrom<Url> for MusicBrainz {
-    type Error = Error;
-
-    fn try_from(value: Url) -> Result<Self, Self::Error> {
-        MusicBrainz::new_from_url(value)
-    }
-}
-
 impl IMbid for MusicBrainz {
     fn mbid(&self) -> &str {
         // The URL is assumed to have been validated.
@@ -98,50 +80,88 @@ impl IMbid for MusicBrainz {
 mod tests {
     use super::*;
 
-    static MUSICBRAINZ: &str =
-        "https://musicbrainz.org/artist/d368baa8-21ca-4759-9731-0b2753071ad8";
-    static MUSICBUTLER: &str = "https://www.musicbutler.io/artist-page/483340948";
-
     #[test]
-    fn musicbrainz() {
+    fn artist() {
         let uuid = "d368baa8-21ca-4759-9731-0b2753071ad8";
         let url_str = format!("https://musicbrainz.org/artist/{uuid}");
-        let url: Url = url_str.as_str().try_into().unwrap();
+
-        let mb: MusicBrainz = url.try_into().unwrap();
+        let mb = MusicBrainz::artist_from_str(&url_str).unwrap();
         assert_eq!(url_str, mb.as_ref());
         assert_eq!(uuid, mb.mbid());
 
-        let url = "not a url at all".to_string();
+        let url: Url = url_str.as_str().try_into().unwrap();
+        let mb = MusicBrainz::artist_from_url(url).unwrap();
+        assert_eq!(url_str, mb.as_ref());
+        assert_eq!(uuid, mb.mbid());
+    }
+
+    #[test]
+    fn album() {
+        let uuid = "d368baa8-21ca-4759-9731-0b2753071ad8";
+        let url_str = format!("https://musicbrainz.org/release-group/{uuid}");
+
+        let mb = MusicBrainz::album_from_str(&url_str).unwrap();
+        assert_eq!(url_str, mb.as_ref());
+        assert_eq!(uuid, mb.mbid());
+
+        let url: Url = url_str.as_str().try_into().unwrap();
+        let mb = MusicBrainz::album_from_url(url).unwrap();
+        assert_eq!(url_str, mb.as_ref());
+        assert_eq!(uuid, mb.mbid());
+    }
+
+    #[test]
+    fn not_a_url() {
+        let url = "not a url at all";
         let expected_error: Error = url::ParseError::RelativeUrlWithoutBase.into();
-        let actual_error = MusicBrainz::from_str(&url).unwrap_err();
+        let actual_error = MusicBrainz::artist_from_str(url).unwrap_err();
         assert_eq!(actual_error, expected_error);
         assert_eq!(actual_error.to_string(), expected_error.to_string());
+    }
 
-        let url = "https://musicbrainz.org/artist/i-am-not-a-uuid".to_string();
+    #[test]
+    fn invalid_url() {
+        let url = "https://www.musicbutler.io/artist-page/483340948";
+        let expected_error = Error::UrlError(format!("invalid artist MusicBrainz URL: {url}"));
+        let actual_error = MusicBrainz::artist_from_str(url).unwrap_err();
+        assert_eq!(actual_error, expected_error);
+        assert_eq!(actual_error.to_string(), expected_error.to_string());
+    }
+
+    #[test]
+    fn artist_invalid_type() {
+        let url = "https://musicbrainz.org/release-group/i-am-not-a-uuid";
+        let expected_error = Error::UrlError(format!("invalid artist MusicBrainz URL: {url}"));
+        let actual_error = MusicBrainz::artist_from_str(url).unwrap_err();
+        assert_eq!(actual_error, expected_error);
+        assert_eq!(actual_error.to_string(), expected_error.to_string());
+    }
+
+    #[test]
+    fn album_invalid_type() {
+        let url = "https://musicbrainz.org/artist/i-am-not-a-uuid";
+        let expected_error =
+            Error::UrlError(format!("invalid release-group MusicBrainz URL: {url}"));
+        let actual_error = MusicBrainz::album_from_str(url).unwrap_err();
+        assert_eq!(actual_error, expected_error);
+        assert_eq!(actual_error.to_string(), expected_error.to_string());
+    }
+
+    #[test]
+    fn invalid_uuid() {
+        let url = "https://musicbrainz.org/artist/i-am-not-a-uuid";
         let expected_error: Error = Uuid::try_parse("i-am-not-a-uuid").unwrap_err().into();
-        let actual_error = MusicBrainz::from_str(&url).unwrap_err();
+        let actual_error = MusicBrainz::artist_from_str(url).unwrap_err();
-        assert_eq!(actual_error, expected_error);
-        assert_eq!(actual_error.to_string(), expected_error.to_string());
-
-        let url = "https://musicbrainz.org/artist".to_string();
-        let expected_error = Error::UrlError(format!("invalid MusicBrainz URL: {url}"));
-        let actual_error = MusicBrainz::from_str(&url).unwrap_err();
         assert_eq!(actual_error, expected_error);
         assert_eq!(actual_error.to_string(), expected_error.to_string());
     }
 
     #[test]
-    fn from_str() {
+    fn missing_uuid() {
-        assert!(MusicBrainz::from_str(MUSICBRAINZ).is_ok());
+        let url = "https://musicbrainz.org/artist";
-        assert!(MusicBrainz::from_str(MUSICBUTLER).is_err());
+        let expected_error = Error::UrlError(format!("invalid artist MusicBrainz URL: {url}"));
-    }
+        let actual_error = MusicBrainz::artist_from_str(url).unwrap_err();
-
+        assert_eq!(actual_error, expected_error);
-    #[test]
+        assert_eq!(actual_error.to_string(), expected_error.to_string());
-    fn try_into() {
-        let result: Result<MusicBrainz, Error> = MUSICBUTLER.try_into();
-        assert!(result.is_err());
-
-        let result: Result<MusicBrainz, Error> = MUSICBRAINZ.try_into();
-        assert!(result.is_ok());
     }
 }

src/core/database/serde/deserialize.rs

@@ -6,6 +6,7 @@ use crate::core::{
     collection::{
         album::{Album, AlbumDate, AlbumId, AlbumSeq},
         artist::{Artist, ArtistId},
+        musicbrainz::MusicBrainz,
         Collection,
     },
     database::LoadError,
@@ -52,7 +53,10 @@ impl TryFrom<DeserializeArtist> for Artist {
         Ok(Artist {
             id: ArtistId::new(artist.name),
             sort: artist.sort.map(ArtistId::new),
-            musicbrainz: artist.musicbrainz.map(TryInto::try_into).transpose()?,
+            musicbrainz: artist
+                .musicbrainz
+                .map(MusicBrainz::artist_from_str)
+                .transpose()?,
             properties: artist.properties,
             albums: artist
                 .albums
@@ -71,7 +75,10 @@ impl TryFrom<DeserializeAlbum> for Album {
             id: AlbumId { title: album.title },
             date: AlbumDate::default(),
             seq: AlbumSeq(album.seq),
-            musicbrainz: album.musicbrainz.map(TryInto::try_into).transpose()?,
+            musicbrainz: album
+                .musicbrainz
+                .map(MusicBrainz::album_from_str)
+                .transpose()?,
             tracks: vec![],
         })
     }

src/core/musichoard/musichoard.rs

@@ -353,15 +353,12 @@ impl<LIB, DB: IDatabase> MusicHoard<LIB, DB> {
         )
     }
 
-    pub fn set_artist_musicbrainz<Id: AsRef<ArtistId>, Mb: TryInto<MusicBrainz, Error = E>, E>(
+    pub fn set_artist_musicbrainz<Id: AsRef<ArtistId>, S: AsRef<str>>(
         &mut self,
         artist_id: Id,
-        url: Mb,
+        url: S,
-    ) -> Result<(), Error>
+    ) -> Result<(), Error> {
-    where
+        let mb = MusicBrainz::artist_from_str(url)?;
-        Error: From<E>,
-    {
-        let mb = url.try_into()?;
         self.update_artist(artist_id.as_ref(), |artist| artist.set_musicbrainz_url(mb))
     }
 
@@ -595,7 +592,7 @@ mod tests {
             .set_artist_musicbrainz(&artist_id, MUSICBUTLER)
             .unwrap_err();
         let expected_err = Error::CollectionError(format!(
-            "an error occurred when processing a URL: invalid MusicBrainz URL: {MUSICBUTLER}"
+            "an error occurred when processing a URL: invalid artist MusicBrainz URL: {MUSICBUTLER}"
         ));
         assert_eq!(actual_err, expected_err);
         assert_eq!(actual_err.to_string(), expected_err.to_string());
@@ -626,7 +623,7 @@ mod tests {
         assert!(music_hoard
             .set_artist_musicbrainz(&artist_id, MUSICBRAINZ)
             .is_ok());
-        _ = expected.insert(MUSICBRAINZ.try_into().unwrap());
+        _ = expected.insert(MusicBrainz::artist_from_str(MUSICBRAINZ).unwrap());
         assert_eq!(music_hoard.collection[0].musicbrainz, expected);
 
         // Clearing URLs on an artist that does not exist is an error.

src/core/testmod.rs

@@ -1,5 +1,5 @@
 use once_cell::sync::Lazy;
-use std::{collections::HashMap, str::FromStr};
+use std::collections::HashMap;
 
 use crate::core::collection::{
     album::{Album, AlbumDate, AlbumId, AlbumMonth, AlbumSeq},

src/tests.rs

@@ -464,7 +464,7 @@ macro_rules! full_collection {
         let artist_a = iter.next().unwrap();
         assert_eq!(artist_a.id.name, "Album_Artist ‘A’");
 
-        artist_a.musicbrainz = Some(MusicBrainz::from_str(
+        artist_a.musicbrainz = Some(MusicBrainz::artist_from_str(
             "https://musicbrainz.org/artist/00000000-0000-0000-0000-000000000000",
         ).unwrap());
 
@@ -482,14 +482,14 @@ macro_rules! full_collection {
         artist_a.albums[0].seq = AlbumSeq(1);
         artist_a.albums[1].seq = AlbumSeq(1);
 
-        artist_a.albums[0].musicbrainz = Some(MusicBrainz::from_str(
+        artist_a.albums[0].musicbrainz = Some(MusicBrainz::album_from_str(
             "https://musicbrainz.org/release-group/00000000-0000-0000-0000-000000000000"
         ).unwrap());
 
         let artist_b = iter.next().unwrap();
         assert_eq!(artist_b.id.name, "Album_Artist ‘B’");
 
-        artist_b.musicbrainz = Some(MusicBrainz::from_str(
+        artist_b.musicbrainz = Some(MusicBrainz::artist_from_str(
             "https://musicbrainz.org/artist/11111111-1111-1111-1111-111111111111",
         ).unwrap());
 
@@ -511,18 +511,18 @@ macro_rules! full_collection {
         artist_b.albums[2].seq = AlbumSeq(2);
         artist_b.albums[3].seq = AlbumSeq(4);
 
-        artist_b.albums[1].musicbrainz = Some(MusicBrainz::from_str(
+        artist_b.albums[1].musicbrainz = Some(MusicBrainz::album_from_str(
             "https://musicbrainz.org/release-group/11111111-1111-1111-1111-111111111111"
         ).unwrap());
 
-        artist_b.albums[2].musicbrainz = Some(MusicBrainz::from_str(
+        artist_b.albums[2].musicbrainz = Some(MusicBrainz::album_from_str(
             "https://musicbrainz.org/release-group/11111111-1111-1111-1111-111111111112"
         ).unwrap());
 
         let artist_c = iter.next().unwrap();
         assert_eq!(artist_c.id.name, "The Album_Artist ‘C’");
 
-        artist_c.musicbrainz = Some(MusicBrainz::from_str(
+        artist_c.musicbrainz = Some(MusicBrainz::artist_from_str(
             "https://musicbrainz.org/artist/11111111-1111-1111-1111-111111111111",
         ).unwrap());
 

src/tui/testmod.rs

@@ -1,4 +1,4 @@
-use std::{collections::HashMap, str::FromStr};
+use std::collections::HashMap;
 
 use musichoard::collection::{
     album::{Album, AlbumDate, AlbumId, AlbumMonth, AlbumSeq},

tests/testlib.rs

@@ -1,5 +1,5 @@
 use once_cell::sync::Lazy;
-use std::{collections::HashMap, str::FromStr};
+use std::collections::HashMap;
 
 use musichoard::collection::{
     album::{Album, AlbumDate, AlbumId, AlbumMonth, AlbumSeq},
@@ -18,7 +18,7 @@ pub static COLLECTION: Lazy<Vec<Artist>> = Lazy::new(|| -> Collection {
             sort: Some(ArtistId{
                 name: String::from("Arkona")
             }),
-            musicbrainz: Some(MusicBrainz::from_str(
+            musicbrainz: Some(MusicBrainz::artist_from_str(
                 "https://musicbrainz.org/artist/baad262d-55ef-427a-83c7-f7530964f212"
             ).unwrap()),
             properties: HashMap::from([
@@ -206,7 +206,7 @@ pub static COLLECTION: Lazy<Vec<Artist>> = Lazy::new(|| -> Collection {
                 name: String::from("Eluveitie"),
             },
             sort: None,
-            musicbrainz: Some(MusicBrainz::from_str(
+            musicbrainz: Some(MusicBrainz::artist_from_str(
                 "https://musicbrainz.org/artist/8000598a-5edb-401c-8e6d-36b167feaf38",
             ).unwrap()),
             properties: HashMap::from([
@@ -451,7 +451,7 @@ pub static COLLECTION: Lazy<Vec<Artist>> = Lazy::new(|| -> Collection {
                 name: String::from("Frontside"),
             },
             sort: None,
-            musicbrainz: Some(MusicBrainz::from_str(
+            musicbrainz: Some(MusicBrainz::artist_from_str(
                 "https://musicbrainz.org/artist/3a901353-fccd-4afd-ad01-9c03f451b490",
             ).unwrap()),
             properties: HashMap::from([
@@ -605,7 +605,7 @@ pub static COLLECTION: Lazy<Vec<Artist>> = Lazy::new(|| -> Collection {
             sort: Some(ArtistId {
                 name: String::from("Heaven’s Basement"),
             }),
-            musicbrainz: Some(MusicBrainz::from_str(
+            musicbrainz: Some(MusicBrainz::artist_from_str(
                 "https://musicbrainz.org/artist/c2c4d56a-d599-4a18-bd2f-ae644e2198cc",
             ).unwrap()),
             properties: HashMap::from([
@@ -737,7 +737,7 @@ pub static COLLECTION: Lazy<Vec<Artist>> = Lazy::new(|| -> Collection {
                 name: String::from("Metallica"),
             },
             sort: None,
-            musicbrainz: Some(MusicBrainz::from_str(
+            musicbrainz: Some(MusicBrainz::artist_from_str(
                 "https://musicbrainz.org/artist/65f4f0c5-ef9e-490c-aee3-909e7ae6b2ab",
             ).unwrap()),
             properties: HashMap::from([