Failing artist test case

[ritiek]

It seems like the database entry for the artist id b0122194-c49a-46a1-ade7-84d1d76bd8e9 was updated or something happened recently as some tests for this artist id are failing. This is when I ran the test suite locally:

$ cargo test -- --test-threads 1
...
failures:

---- artist::artist_includes::should_get_artist_recordings stdout ----
thread 'main' panicked at 'assertion failed: recordings.iter().any(|recording| recording.title == \"A Little Bit Higher\")', tests/artist/artist_includes.rs:85:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

---- artist::artist_includes::should_get_artist_recordings_test stdout ----
thread 'main' panicked at 'assertion failed: recordings.iter().any(|recording| recording.title == \"A Little Bit Higher\")', tests/artist/artist_includes.rs:18:5

---- artist::artist_includes::should_get_artist_release_groups stdout ----
thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: Error(Json(Error("invalid type: null, expected a string", line: 1, column: 640)))', tests/artist/artist_includes.rs:63:10

failures:
    artist::artist_includes::should_get_artist_recordings
    artist::artist_includes::should_get_artist_recordings_test
    artist::artist_includes::should_get_artist_release_groups

test result: FAILED. 111 passed; 3 failed; 0 ignored; 0 measured; 0 filtered out; finished in 144.20s

We could probably update the recording title in should_get_artist_recordings, and we could get rid of the next test case (should_get_artist_recordings_test) since it's exactly the same as this test case.

In the final test case (should_get_artist_release_groups), it seems like we aren't able to correctly parse the json response. Any ideas on how should I debug further on what json parameter(s) with the null value are breaking our parser?

[oknozor]

Indeed those test are duplicated, We can remove one of them and update the assertion on the other one.

Unfortunately there is no straightforward way to debug when it comes to serialization. The easiest way is probably to compare the json response with our structs.

Seems there are some release groups with null primary-type-id and primary-type. There might be other problematic null values. Turning them into Option<T> one at a time should do it.

[ritiek]

I see, thanks!

[ritiek]

So I created a minimal example to reproduce the error:

use musicbrainz_rs::entity::artist::Artist;
use musicbrainz_rs::prelude::*;

fn main() {
    let john_lee_hooker = Artist::fetch()
        .id("b0122194-c49a-46a1-ade7-84d1d76bd8e9")
        .with_release_groups()
        .execute()
        .unwrap();
}

And I changed the values for both primary_type_id and primary_type from here to Option<String>, but the error now when running the example changes to:

$ cargo run --example artist_release_groups
...
thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: Error(Json(Error("premature end of input", line: 1, column: 1483)))', examples/artist_release_groups.rs:9:10
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

and RUST_BACKTRACE=1 doesn't seem to return anything useful.

This error is weird IMO because jq correctly parses the json:

$ curl "https://musicbrainz.org/ws/2/artist/b0122194-c49a-46a1-ade7-84d1d76bd8e9?fmt=json&inc=release-groups" | jq

I've also cross-checked that this is the correct request url by accessing it through &self.0.path, and also writing the response text by accessing HTTP_CLIENT.get(&self.0.path).send()?.text() to a file and parsing it through jq works fine.

Not really sure what's wrong here or maybe if the error means something else.

[oknozor]

The fact that jq parse the result correctly doesn't mean serde will succeed doing it. The big difference here is that jq tries to deserialize the ouput result to any valid json, we are trying to deserialize it to a valid ReleaseGroup, Artist etc.

Few things might be going wrong here, in the json ouput I see isnis and ipis, I don't think we have those fields. This might not be a problem. I think serde skips unknown fields by default (you can check the serde doc).

What surprise me here is that end-area and begin-area are duplicated, I honestly don't know how serde would behave here.

To reproduce you might want to write a test with only serialization, checking the result of serde_json::from_str::<Artist>(input), this way you will be able to quickly modify the json input to find what's going wrong.

I am working today, but I might have some time to troubleshot this at noon. I will let you know asap.

[oknozor]

@ritiek , I just pushed a fix : type-id and primary-type-id were null in the json and we were parsing release date with the wrong deserializer (some date returned by the api are juste empty strings).

[oknozor]

I just deployed a new version to get the fix available.

[ritiek]

I see, good to know!

This makes me wonder whether or not musicbrainz api docs mention about what fields can be optional in their response based on the requested resource. I tried to search through the docs but couldn't find anything interesting though. Have we so far made some fields Option<T> by analyzing responses for different sample requests to musicbrainz api?