feat(): fetch coverart images for releases

[ritiek]

This PR adds method(s) for fetching coverart for release entities. I have a few concerns:

---

I've had to change the type for Release::ID and Release::Title attributes to an Option, since the coverart API doesn't return these values. Is this going to be okay? Since we do not treat these two attributes as an Option for other entities.

---

It seems like the coverart API returns an inconsistent response depending on the MBID. For example, the release entity 76df3287-6cda-33eb-8e9a-044b5e15ffdd returns the value of the key ID in the json response as an integer. https://coverartarchive.org/release/76df3287-6cda-33eb-8e9a-044b5e15ffdd?fmt=json

While, the release entity 18d4e9b4-9247-4b44-914a-8ddec3502103 treats the value of ID as a string (in quotes) in the json response. https://coverartarchive.org/release/18d4e9b4-9247-4b44-914a-8ddec3502103?fmt=json

In this PR, I've assumed them as an integer but our json parser breaks in cases it receives a string for ID (since it expects an integer). Is this a bug in the web API and should be reported?

---

I've yet to work on the builder pattern which looks like:

let in_utero = Release::fetch_coverart()
        .id("18d4e9b4-9247-4b44-914a-8ddec3502103")
        .back() // back/front
        .250()  // 250/500/1200
        .execute()
        .expect("Unable to get coverart"); 

as you mentioned in https://github.com/oknozor/musicbrainz_rs/issues/28#issuecomment-803539050.

I wanted to get some feedback before on the current work so far, whether or not is this the right way to implement coverart? What the current usage looks like can be seen from tests/release/release_coverart.rs.

[oknozor]

Changing Release id and title to optional is not the way to go in my opinion. I don't see why the fetch_coverart method would be returning a Release or ReleaseGroup struct when the coverart api return nothing relevant regarding those.

As far I understand coverart entities for release and release groups are the same right ? Assuming that, we could have the following signature for the associated method :

impl<'a, T> FetchCoverartQuery<T>
    where
        T: Clone + FetchCoverart<'a>
{
    pub fn execute(&mut self) -> Result<Coverarts, Error>
// ... 

Also if we really want to had something to get the coverart from an entity we already got from the musicbrainz api, we could add another method to mutate the release/release-group entity :

pub trait FetchCoverart<'a> {
    fn id(&self) -> &str;
    fn set_coverart(&mut self, coverarts: Coverarts) -> &mut Self;

    fn get_coverarts(&mut self) -> Result<&mut Self, Error>
        where Self: Sized + Path<'a> + Clone + Fetch<'a> + DeserializeOwned {

        self.set_coverart(Self::fetch_coverart()
            .id(self.id())
            .execute()?);

        Ok(self)
    }
// ... 

This would allow to write something like this :

    // Fetch coverarts only
    let in_utero_coverarts: Coverarts = Release::fetch_coverart()
        .id("76df3287-6cda-33eb-8e9a-044b5e15ffdd")
        .execute()
        .expect("Unable to get cover art");

    // Fetch release
    let mut in_utero: Release = Release::fetch()
        .id("76df3287-6cda-33eb-8e9a-044b5e15ffdd")
        .execute()
        .expect("Unable to get release");

    // Add coverart
    let in_utero_with_coverarts = in_utero
        .get_coverarts().expect("Unable to get coverart");

    assert!(in_utero_with_coverarts.coverarts.is_some())

Also you probably want to move the coverart struct to a dedicated module.

[ritiek]

Changing Release id and title to optional is not the way to go in my opinion. I don't see why the fetch_coverart method would be returning a Release or ReleaseGroup struct when the coverart api return nothing relevant regarding those.

This indeed sounds better than how I previously implemented it.

As far I understand coverart entities for release and release groups are the same right ?

Yup, right.

I've made an attempt on addressing these proposed changes. For now, I've implemented only one way to fetch the coverart, which looks like:

    let in_utero_coverarts: Coverarts = Release::fetch_coverart()
        .id("76df3287-6cda-33eb-8e9a-044b5e15ffdd")
        .execute()
        .expect("Unable to get cover art");

If this looks good so far, I can attempt working on other ways to fetch the coverart.

[oknozor]

Great work, we shall see later if we feel the need to implement fetching coverart for fetched entities.

[ritiek]

Indeed, thanks!

It seems like the coverart API returns an inconsistent response depending on the MBID. For example, the release entity 76df3287-6cda-33eb-8e9a-044b5e15ffdd returns the value of the key ID in the json response as an integer. https://coverartarchive.org/release/76df3287-6cda-33eb-8e9a-044b5e15ffdd?fmt=json

While, the release entity 18d4e9b4-9247-4b44-914a-8ddec3502103 treats the value of ID as a string (in quotes) in the json response. https://coverartarchive.org/release/18d4e9b4-9247-4b44-914a-8ddec3502103?fmt=json

By the way, I raised this on the #metabrainz IRC and this does seems like a bug with coverartarchive.org.