ppy
Fix exception when trying to deserialize generic `Beatmap<T>` from json
I ran into this recently when working on a custom ruleset. Trying to deserialize a Beatmap<T> from json where T is not HitObject will throw the following error because TypedListConverter<HitObject> returns a List<HitObject> regardless of the property's actual type.
Newtonsoft.Json.JsonSerializationException : Error setting value to 'HitObjects' on 'osu.Game.Beatmaps.Beatmap`1[osu.Game.Rulesets.Osu.Objects.OsuHitObject]'.
----> System.InvalidCastException : Unable to cast object of type 'System.Collections.Generic.List`1[osu.Game.Rulesets.Objects.HitObject]' to type 'System.Collections.Generic.List`1[osu.Game.Rulesets.Osu.Objects.OsuHitObject]'.
...
Changes the TypedListConverter to resolve the generic list type from the type provided by the serializer, so it will always deserialize as the exact type specified by the property.
Here's my alternative to this, thoughts? Uses IList (handles List<T>, T[], BindableList<T>, etc) and effectively acts as populating the list rather than creating it, which also means the list has to be created at the usage site in turn.
I haven't tested it but I want to avoid using reflection as much as possible.
diff --git a/osu.Game/IO/Serialization/Converters/TypedListConverter.cs b/osu.Game/IO/Serialization/Converters/TypedListConverter.cs
index 19ef6b8fe6..2bd6f5ab8e 100644
--- a/osu.Game/IO/Serialization/Converters/TypedListConverter.cs
+++ b/osu.Game/IO/Serialization/Converters/TypedListConverter.cs
@@ -1,9 +1,8 @@
// Copyright (c) ppy Pty Ltd <[email protected]>. Licensed under the MIT Licence.
// See the LICENCE file in the repository root for full licence text.
-#nullable disable
-
using System;
+using System.Collections;
using System.Collections.Generic;
using Newtonsoft.Json;
using Newtonsoft.Json.Linq;
@@ -12,12 +11,12 @@
namespace osu.Game.IO.Serialization.Converters
{
/// <summary>
- /// A type of <see cref="JsonConverter"/> that serializes an <see cref="IReadOnlyList{T}"/> alongside
+ /// A type of <see cref="JsonConverter"/> that serializes a list alongside
/// a lookup table for the types contained. The lookup table is used in deserialization to
/// reconstruct the objects with their original types.
/// </summary>
- /// <typeparam name="T">The type of objects contained in the <see cref="IReadOnlyList{T}"/> this attribute is attached to.</typeparam>
- public class TypedListConverter<T> : JsonConverter<IReadOnlyList<T>>
+ /// <typeparam name="TExpected">The expected type of objects contained in the list this attribute is attached to.</typeparam>
+ public class TypedListConverter<TExpected> : JsonConverter
{
private readonly bool requiresTypeVersion;
@@ -39,35 +38,39 @@ public TypedListConverter(bool requiresTypeVersion)
this.requiresTypeVersion = requiresTypeVersion;
}
- public override IReadOnlyList<T> ReadJson(JsonReader reader, Type objectType, IReadOnlyList<T> existingValue, bool hasExistingValue, JsonSerializer serializer)
+ public override bool CanConvert(Type objectType) => objectType.IsAssignableTo(typeof(IList));
+
+ public override object ReadJson(JsonReader reader, Type objectType, object? existingValue, JsonSerializer serializer)
{
- var list = new List<T>();
+ if (existingValue == null)
+ throw new JsonException($"{nameof(TypedListConverter<TExpected>)} requires an already-constructed list.");
- var obj = JObject.Load(reader);
+ IList list = (IList)existingValue;
+ var obj = JObject.Load(reader);
if (obj["$lookup_table"] == null)
return list;
- var lookupTable = serializer.Deserialize<List<string>>(obj["$lookup_table"].CreateReader());
+ var lookupTable = serializer.Deserialize<List<string>>(obj["$lookup_table"]!.CreateReader());
if (lookupTable == null)
return list;
if (obj["$items"] == null)
return list;
- foreach (var tok in obj["$items"])
+ foreach (var tok in obj["$items"]!)
{
var itemReader = tok.CreateReader();
if (tok["$type"] == null)
throw new JsonException("Expected $type token.");
- // Prevent instantiation of types that do not inherit the type targetted by this converter
- Type type = Type.GetType(lookupTable[(int)tok["$type"]]).AsNonNull();
- if (!type.IsAssignableTo(typeof(T)))
+ // Prevent instantiation of types that do not inherit the type targeted by this converter
+ Type type = Type.GetType(lookupTable[(int)tok["$type"]!]).AsNonNull();
+ if (!type.IsAssignableTo(typeof(TExpected)))
continue;
- var instance = (T)Activator.CreateInstance(type)!;
+ var instance = (TExpected)Activator.CreateInstance(type)!;
serializer.Populate(itemReader, instance);
list.Add(instance);
@@ -76,12 +79,17 @@ public override IReadOnlyList<T> ReadJson(JsonReader reader, Type objectType, IR
return list;
}
- public override void WriteJson(JsonWriter writer, IReadOnlyList<T> value, JsonSerializer serializer)
+ public override void WriteJson(JsonWriter writer, object? value, JsonSerializer serializer)
{
+ if (value == null)
+ throw new JsonException($"{nameof(TypedListConverter<TExpected>)} requires an already-constructed list.");
+
+ IList list = (IList)value;
+
var lookupTable = new List<string>();
var objects = new List<JObject>();
- foreach (var item in value)
+ foreach (object? item in list)
{
var type = item.GetType();
var assemblyName = type.Assembly.GetName();
handles
List<T>,T[],BindableList<T>, etc
I don't think this actually works for T[], while you can cast it to IList it will throw when you try to add values to it.
Right... Interesting you can list.Clear() (this would be required anyway), so indexing wouldn't be too hard.
Of course it should be mentioned that this entire thing is dodgy as all hell. It was a spur of the moment creation that I regret... I wonder if there's a way it can be specialised for hitobjects/this specific usage, or if a better path is to look into removing the generic param from Beatmap<> altogether (I think the team generally agrees it's caused more pain than not...?).
For example another path may be something along the lines:
class Beatmap<T>
{
[JsonIgnore]
public List<T> HitObjects { get; set; } = new List<T>();
}
class Beatmap : Beatmap<HitObject>
{
[HitObjectListConverter<HitObject>]
private IEnumerable<HitObject> hitObjects
{
get => HitObjects;
set => HitObjects = value.ToArray();
}
}
public class OsuBeatmap : Beatmap<OsuHitObject>
{
[HitObjectListConverter<OsuHitObject>]
private IEnumerable<OsuHitObject> hitObjects
{
get => HitObjects;
set => HitObjects = value.ToArray();
}
}
That is - move the conversion inside the concrete types.
Moving the conversion into the ruleset-specific beatmaps sounds like it should work but not being able to enforce it's existence at type level might cause issues since there's nothing forcing (custom) rulesets to implement it, especially from rulesets that already existed before that change.
@minetoblend is this blocking anything specific? i'm on board with getting your version in if it helps some kind of ruleset / editor issue, but it's not 100% clear what your goal of json encoding here is.
is this blocking anything specific?
It was originally an issue when I was still under the assumption that json was gonna be used for custom-ruleset beatmap storage and was working on a prototype for this. But after this conversation on discord I don't think there's a need for this anymore aside from being able to use json for tests (since class MyBeatmap : Beatmap<MyHitObject> would throw).
If the plan is to to avoid json for this when saving/loading beatmaps for custom rulesets gets added down the road then I strongly prefer that, as newtonsoft json does not fare very well against other serialization libraries I've worked with.
Fwiw I do have a prototype for a version of the beatmap encoder that largely follows the current .osu format but delegates some parts to the rulesets, so I can share that whenever there's time and resources to look into that topic.