Zastai / MetaBrainz.MusicBrainz

Native .NET implementation of libmusicbrainz
MIT License
37 stars 9 forks source link

Cannot serialize back results to JSON with System.Text.Json #28

Closed guillaume86 closed 2 years ago

guillaume86 commented 2 years ago

Hi,

When I try to serialize results to JSON, I get this error:

System.NotSupportedException
  Message=This converter is for deserialization only. The unsupported member type is located on type 'MetaBrainz.MusicBrainz.PartialDate'. Path: $.LifeSpan.Begin.
  Source=System.Text.Json
  StackTrace:
   at System.Text.Json.ThrowHelper.ThrowNotSupportedException(WriteStack& state, NotSupportedException ex)
   at System.Text.Json.Serialization.JsonConverter`1.WriteCore(Utf8JsonWriter writer, T& value, JsonSerializerOptions options, WriteStack& state)
   at System.Text.Json.Serialization.JsonConverter`1.WriteCoreAsObject(Utf8JsonWriter writer, Object value, JsonSerializerOptions options, WriteStack& state)
   at System.Text.Json.JsonSerializer.WriteUsingSerializer[TValue](Utf8JsonWriter writer, TValue& value, JsonTypeInfo jsonTypeInfo)
   at System.Text.Json.JsonSerializer.WriteStringUsingSerializer[TValue](TValue& value, JsonTypeInfo jsonTypeInfo)
   at System.Text.Json.JsonSerializer.Serialize[TValue](TValue value, JsonSerializerOptions options)
   at Reactalog.TestsBase.Json(Object data) in C:\Users\guillaume.lecomte\source\repos\Reactalog\test\TestsBase.cs:line 26

The problems seems to come from the JsonConverter attribute here: https://github.com/Zastai/MetaBrainz.MusicBrainz/blob/main/MetaBrainz.MusicBrainz/PartialDate.cs#L14

If you could move it to https://github.com/Zastai/MetaBrainz.MusicBrainz/blob/main/MetaBrainz.MusicBrainz/Json/Converters.cs I think that would solve the issue.

Thanks for this library btw :)

Zastai commented 2 years ago

Ah you mean you are serializing a PartialDate to JSON for your own purpose and it's picking up my MB API oriented serializer?

Yes, I'll have a look - it's certainly not intentional to prevent doing your own (de)serialization.

guillaume86 commented 2 years ago

Yes that's it. Sorry for being a bit vague...

Thanks a lot!

Zastai commented 2 years ago

Can you try with the CI build from https://ci.appveyor.com/nuget/metabrainz-musicbrainz-d79tac5nmfsx (you'll want 4.0.0-pre.appveyor.80)?

guillaume86 commented 2 years ago

I think the feed need authentication, here's why I see:

<service xmlns:atom="http://www.w3.org/2005/Atom" xmlns:app="http://www.w3.org/2007/app" xmlns="http://www.w3.org/2007/app" xml:base="https://ci.appveyor.com/nuget/metabrainz-musicbrainz-d79tac5nmfsx">
<workspace>
<atom:title>Default</atom:title>
<collection href="Packages">
<atom:title>Packages</atom:title>
</collection>
</workspace>
</service>
guillaume86 commented 2 years ago

Nevermind the feed is working ok!

I just tried and the last build do fix my issue :)

Thanks a lot! Have a nice day.

Zastai commented 2 years ago

Not closing this until it's fixed in an actual release.

I'll want to validate the PR build some more before merging it, and I won't be releasing until I've removed the use of WebRequest. But that CI build will hopefully tide you over until then,

guillaume86 commented 2 years ago

Great I'll be notified when this issue is closed and will switch back to the official Nuget feed then.

Good luck for the switch to HttpClient ;).

Zastai commented 2 years ago

Well, when the issue is closed you can start taking builds from that feed other than specifically 4.0.0-pre.appveyor.80 (but even then, not all builds on that feed will be from the main branch). You can only switch to main NuGet once a release is cut (if you star the repo, I think GitHub will notify you of releases).