fix: Newtonsoft Json converter for nullable types #346
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Thanks for the PR! Which tests are failing due to time zones? I thought I'd fixed those, but I wasn't 100% sure that I got all of them. I've updated the Wiki page on what to do with different snapshots: https://github.com/SteveDunn/Vogen/wiki/Tests Thanks again, I'll take a look to see if I spot this weirdness! |
Tests like these are failing: Basically all related to RoundtripKind. I've updated the snapshots. |
|
@SteveDunn I wonder right now why there are separate implementations in the templates for all the standard primitive types and then an "Any" type. The way I see it the main differences should be between Value and Reference types. I wasn't able at a cursory glance at least to find any significant differences between the implementations for the different value types. Since I needed to differentiate between Value AnyTypes and Reference AnyTypes I wonder whether this isn't just a distinction that should be made in general. This would massively reduce the number of templates needed and likewise would reduce the test surface quite a bit as now the only real tests that would need to be performed are ValueTypes vs ReferenceTypes and maybe Records as well but there'd be no real need to test against all different basic ValueTypes if the implementation was identical. |
|
I wrote 'Cli version of Vogen' this because Rider has a real pain with Intellisense, and I got tired of restarting rider due to compiler errors because of source generators. If its helpful to add to Vogen, i have no problem with that, i just need to clean up the code a little. I think SourceGenerators are much much better than CLI, but I just got so tried of the "the If its helpful, here is a list of the Newtonsoft implementations by type. I simply generated one of each type, then copied out the Newtonsoft code. So this the generation result of each type namespace Aeonic.CodeGen.Vogen.Infra.Stores;
using SharedCommon.Primitives;
public class NewtonsoftJsonConverterStore
{
// ReSharper disable once InconsistentNaming
public readonly ICodeGenTemplate Attribute = CodeGenTemplate.With(
@"[global::Newtonsoft.Json.JsonConverter(typeof({{strongValueObjectName}}NewtonsoftJsonConverter))]");
private const string StringTemplate = @"
class {{strongValueObjectName}}NewtonsoftJsonConverter : Newtonsoft.Json.JsonConverter
{
public override bool CanConvert(System.Type objectType)
{
return objectType == typeof({{strongValueObjectName}});
}
public override void WriteJson(Newtonsoft.Json.JsonWriter writer, object value, Newtonsoft.Json.JsonSerializer serializer)
{
var id = ({{strongValueObjectName}})value;
serializer.Serialize(writer, id.Value);
}
public override object ReadJson(Newtonsoft.Json.JsonReader reader, System.Type objectType, object existingValue, Newtonsoft.Json.JsonSerializer serializer)
{
return {{strongValueObjectName}}.Deserialize(serializer.Deserialize<global::System.String>(reader));
}
}
";
private const string BooleanTemplate = @"
class {{strongValueObjectName}}NewtonsoftJsonConverter : Newtonsoft.Json.JsonConverter
{
public override bool CanConvert(System.Type objectType)
{
return objectType == typeof({{strongValueObjectName}});
}
public override void WriteJson(Newtonsoft.Json.JsonWriter writer, object value, Newtonsoft.Json.JsonSerializer serializer)
{
var id = ({{strongValueObjectName}})value;
serializer.Serialize(writer, id.Value);
}
public override object ReadJson(Newtonsoft.Json.JsonReader reader, System.Type objectType, object existingValue, Newtonsoft.Json.JsonSerializer serializer)
{
var result = serializer.Deserialize<global::System.Boolean?>(reader);
return result.HasValue ? {{strongValueObjectName}}.Deserialize(result.Value) : null;
}
}";
private const string DateTimeTemplate = @"
class {{strongValueObjectName}}NewtonsoftJsonConverter : Newtonsoft.Json.JsonConverter
{
public override bool CanConvert(System.Type objectType)
{
return objectType == typeof({{strongValueObjectName}});
}
public override void WriteJson(Newtonsoft.Json.JsonWriter writer, object value, Newtonsoft.Json.JsonSerializer serializer)
{
var id = ({{strongValueObjectName}})value;
serializer.Serialize(writer, id.Value);
}
public override object ReadJson(Newtonsoft.Json.JsonReader reader, System.Type objectType, object existingValue, Newtonsoft.Json.JsonSerializer serializer)
{
var dt = serializer.Deserialize<global::System.DateTime?>(reader);
return dt.HasValue ? {{strongValueObjectName}}.Deserialize(dt.Value) : null;
}
}";
private const string DateOnlyTemplate = @"
class {{strongValueObjectName}}NewtonsoftJsonConverter : Newtonsoft.Json.JsonConverter
{
public override bool CanConvert(System.Type objectType)
{
return objectType == typeof({{strongValueObjectName}});
}
public override void WriteJson(Newtonsoft.Json.JsonWriter writer, object value, Newtonsoft.Json.JsonSerializer serializer)
{
var id = ({{strongValueObjectName}})value;
serializer.Serialize(writer, id.Value);
}
public override object ReadJson(Newtonsoft.Json.JsonReader reader, System.Type objectType, object existingValue, Newtonsoft.Json.JsonSerializer serializer)
{
var dt = serializer.Deserialize<global::System.DateOnly?>(reader);
return dt.HasValue ? {{strongValueObjectName}}.Deserialize(dt.Value) : null;
}
}";
private const string TimeOnlyTemplate = @"
class {{strongValueObjectName}}NewtonsoftJsonConverter : Newtonsoft.Json.JsonConverter
{
public override bool CanConvert(System.Type objectType)
{
return objectType == typeof({{strongValueObjectName}});
}
public override void WriteJson(Newtonsoft.Json.JsonWriter writer, object value, Newtonsoft.Json.JsonSerializer serializer)
{
var id = ({{strongValueObjectName}})value;
serializer.Serialize(writer, id.Value);
}
public override object ReadJson(Newtonsoft.Json.JsonReader reader, System.Type objectType, object existingValue, Newtonsoft.Json.JsonSerializer serializer)
{
var dt = serializer.Deserialize<global::System.TimeOnly?>(reader);
return dt.HasValue ? {{strongValueObjectName}}.Deserialize(dt.Value) : null;
}
}";
private const string DateTimeOffsetTemplate = @"
class {{strongValueObjectName}}NewtonsoftJsonConverter : Newtonsoft.Json.JsonConverter
{
public override bool CanConvert(System.Type objectType)
{
return objectType == typeof({{strongValueObjectName}});
}
public override void WriteJson(Newtonsoft.Json.JsonWriter writer, object value, Newtonsoft.Json.JsonSerializer serializer)
{
var id = ({{strongValueObjectName}})value;
serializer.Serialize(writer, id.Value);
}
public override object ReadJson(Newtonsoft.Json.JsonReader reader, System.Type objectType, object existingValue, Newtonsoft.Json.JsonSerializer serializer)
{
var dt = serializer.Deserialize<global::System.DateTimeOffset?>(reader);
return dt.HasValue ? {{strongValueObjectName}}.Deserialize(dt.Value) : null;
}
}
";
private const string IntTemplate = @"
class {{strongValueObjectName}}NewtonsoftJsonConverter : Newtonsoft.Json.JsonConverter
{
public override bool CanConvert(System.Type objectType)
{
return objectType == typeof({{strongValueObjectName}});
}
public override void WriteJson(Newtonsoft.Json.JsonWriter writer, object value, Newtonsoft.Json.JsonSerializer serializer)
{
var id = ({{strongValueObjectName}})value;
serializer.Serialize(writer, id.Value);
}
public override object ReadJson(Newtonsoft.Json.JsonReader reader, System.Type objectType, object existingValue, Newtonsoft.Json.JsonSerializer serializer)
{
var result = serializer.Deserialize<global::System.Int32?>(reader);
return result.HasValue ? {{strongValueObjectName}}.Deserialize(result.Value) : null;
}
}";
private const string FloatSingleTemplate = @"
class {{strongValueObjectName}}NewtonsoftJsonConverter : Newtonsoft.Json.JsonConverter
{
public override bool CanConvert(System.Type objectType)
{
return objectType == typeof({{strongValueObjectName}});
}
public override void WriteJson(Newtonsoft.Json.JsonWriter writer, object value, Newtonsoft.Json.JsonSerializer serializer)
{
var id = ({{strongValueObjectName}})value;
serializer.Serialize(writer, id.Value);
}
public override object ReadJson(Newtonsoft.Json.JsonReader reader, System.Type objectType, object existingValue, Newtonsoft.Json.JsonSerializer serializer)
{
var result = serializer.Deserialize<global::System.Single?>(reader);
return result.HasValue ? {{strongValueObjectName}}.Deserialize(result.Value) : null;
}
}";
private const string DoubleTemplate = @"
class {{strongValueObjectName}}NewtonsoftJsonConverter : Newtonsoft.Json.JsonConverter
{
public override bool CanConvert(System.Type objectType)
{
return objectType == typeof({{strongValueObjectName}});
}
public override void WriteJson(Newtonsoft.Json.JsonWriter writer, object value, Newtonsoft.Json.JsonSerializer serializer)
{
var id = ({{strongValueObjectName}})value;
serializer.Serialize(writer, id.Value);
}
public override object ReadJson(Newtonsoft.Json.JsonReader reader, System.Type objectType, object existingValue, Newtonsoft.Json.JsonSerializer serializer)
{
var result = serializer.Deserialize<global::System.Double?>(reader);
return result.HasValue ? {{strongValueObjectName}}.Deserialize(result.Value) : null;
}
}
";
private const string DecimalTemplate = @"
class {{strongValueObjectName}}NewtonsoftJsonConverter : Newtonsoft.Json.JsonConverter
{
public override bool CanConvert(System.Type objectType)
{
return objectType == typeof({{strongValueObjectName}});
}
public override void WriteJson(Newtonsoft.Json.JsonWriter writer, object value, Newtonsoft.Json.JsonSerializer serializer)
{
var id = ({{strongValueObjectName}})value;
serializer.Serialize(writer, id.Value);
}
public override object ReadJson(Newtonsoft.Json.JsonReader reader, System.Type objectType, object existingValue, Newtonsoft.Json.JsonSerializer serializer)
{
var result = serializer.Deserialize<global::System.Decimal?>(reader);
return result.HasValue ? {{strongValueObjectName}}.Deserialize(result.Value) : null;
}
}
";
private const string LongTemplate = @"
class {{strongValueObjectName}}NewtonsoftJsonConverter : Newtonsoft.Json.JsonConverter
{
public override bool CanConvert(System.Type objectType)
{
return objectType == typeof({{strongValueObjectName}});
}
public override void WriteJson(Newtonsoft.Json.JsonWriter writer, object value, Newtonsoft.Json.JsonSerializer serializer)
{
var id = ({{strongValueObjectName}})value;
serializer.Serialize(writer, id.Value);
}
public override object ReadJson(Newtonsoft.Json.JsonReader reader, System.Type objectType, object existingValue, Newtonsoft.Json.JsonSerializer serializer)
{
var result = serializer.Deserialize<global::System.Int64?>(reader);
return result.HasValue ? {{strongValueObjectName}}.Deserialize(result.Value) : null;
}
}
";
private const string ShortTemplate = @"
class {{strongValueObjectName}}NewtonsoftJsonConverter : Newtonsoft.Json.JsonConverter
{
public override bool CanConvert(System.Type objectType)
{
return objectType == typeof({{strongValueObjectName}});
}
public override void WriteJson(Newtonsoft.Json.JsonWriter writer, object value, Newtonsoft.Json.JsonSerializer serializer)
{
var id = ({{strongValueObjectName}})value;
serializer.Serialize(writer, id.Value);
}
public override object ReadJson(Newtonsoft.Json.JsonReader reader, System.Type objectType, object existingValue, Newtonsoft.Json.JsonSerializer serializer)
{
var result = serializer.Deserialize<global::System.Int16?>(reader);
return result.HasValue ? {{strongValueObjectName}}.Deserialize(result.Value) : null;
}
}";
private const string GuidTemplate = @"
class {{strongValueObjectName}}NewtonsoftJsonConverter : Newtonsoft.Json.JsonConverter
{
public override bool CanConvert(System.Type objectType)
{
return objectType == typeof({{strongValueObjectName}});
}
public override void WriteJson(Newtonsoft.Json.JsonWriter writer, object value, Newtonsoft.Json.JsonSerializer serializer)
{
var id = ({{strongValueObjectName}})value;
serializer.Serialize(writer, id.Value);
}
public override object ReadJson(Newtonsoft.Json.JsonReader reader, System.Type objectType, object existingValue, Newtonsoft.Json.JsonSerializer serializer)
{
var guid = serializer.Deserialize<System.Guid?>(reader);
return guid.HasValue ? {{strongValueObjectName}}.Deserialize(guid.Value) : null;
}
}";
public NewtonsoftJsonConverterStore()
{
this.Store.Add(PrimitiveTypeEnum.AsString, CodeGenTemplate.With(StringTemplate));
this.Store.Add(PrimitiveTypeEnum.AsBoolean, CodeGenTemplate.With(BooleanTemplate));
this.Store.Add(PrimitiveTypeEnum.AsDateTime, CodeGenTemplate.With(DateTimeTemplate));
this.Store.Add(PrimitiveTypeEnum.AsDateTimeOffset, CodeGenTemplate.With(DateTimeOffsetTemplate));
this.Store.Add(PrimitiveTypeEnum.AsDateOnly, CodeGenTemplate.With(DateOnlyTemplate));
this.Store.Add(PrimitiveTypeEnum.AsTimeOnly, CodeGenTemplate.With(TimeOnlyTemplate));
this.Store.Add(PrimitiveTypeEnum.AsInt, CodeGenTemplate.With(IntTemplate));
this.Store.Add(PrimitiveTypeEnum.AsFloat, CodeGenTemplate.With(FloatSingleTemplate));
this.Store.Add(PrimitiveTypeEnum.AsDouble, CodeGenTemplate.With(DoubleTemplate));
this.Store.Add(PrimitiveTypeEnum.AsDecimal, CodeGenTemplate.With(DecimalTemplate));
this.Store.Add(PrimitiveTypeEnum.AsGuid, CodeGenTemplate.With(GuidTemplate));
this.Store.Add(PrimitiveTypeEnum.AsShort, CodeGenTemplate.With(ShortTemplate));
}
private Dictionary<PrimitiveTypeEnum, ICodeGenTemplate> Store { get; } = new();
public ICodeGenTemplate Get(PrimitiveTypeEnum primitiveType)
{
return this.Store.First(_ => _.Key == primitiveType)
.Value;
}
} |
|
@SteveDunn So the template tests failing is kind of expected, as I did not retain the normal template, it could be added back just as a placeholder but that seems weird. Again the question is should we just use two template files to begin with one for value one for reference types? I don't know why the snapshot tests are failing. I've ran the tests locally with -p:THOROUGH=true and commited the result. |
@Blackclaws - the templates are different for each type because each type can be coerced from a different range of types. For instance, here's the difference between the Dapper template for
Similarly for
Looking at a non-Dapper template:
This has similar logic to the Dapper templates. The |
|
@Blackclaws - having a look at the template, I think I see what you mean now! The The difference between
So, perhaps the string implementation needs this null check too? |
SteveDunn
reviewed
Jan 28, 2023
src/Vogen/Templates/AnyOtherType/AnyOtherType_NewtonsoftJsonConverterReferenceType.cs
Show resolved
Hide resolved
Thank you @jeffward01 - so you use those instead of getting Vogen to generate them? I'll open a bug report in Rider today and share the link. There are similar problems in ReSharper too. Plus, aside from source generation, Rider isn't very happy with the Vogen solution. I'm assuming it's down to the thousands of snapshot files (even though I have them as 'do not analyze') |
|
Thanks for the PR @Blackclaws , much appreciated! LGTM! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is an attempt at fixing the behavior of the Newtonsoft.Json converter when confronted with serializing nullable fields of ValueTypes of reference types.
Its still failing the Snapshot tests as I have no clue how to update all the snapshots and haven't been able to find any documentation regarding this.
Its also failing some other tests for reasons I don't really understand as they are also failing in a clean pull of the main branch ( for example all tests dealing with date time offset fail because they expect the offset to timezone to be 0:00 while my local timezone is +1:00 (these seem to be a bit brittle).
Other tests that fail are indeed Newtonsoft.Json tests, however those test failures are a bit weird as they fail in the serialization step which now serializes some value objects to a null string. However I didn't touch that code at all and this result also seems to hold for a clean pull of the main branch. Its a bit weird, we'll see how the CI test runners handle this one.
I might add that I'm developing on Linux and the tooling around the project seems to mostly assume a Windows environment.
Also the tests are missing a test for non ValueType user types as ValueObject types.
Fixes #344