-
Notifications
You must be signed in to change notification settings - Fork 19
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement a JsonConverterFactory equivalent of the existing default converter attribute #99
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,32 @@ | ||
// Copyright 2023 The Noda Time Authors. All rights reserved. | ||
// Use of this source code is governed by the Apache License 2.0, | ||
// as found in the LICENSE.txt file. | ||
|
||
using System; | ||
using System.Text.Json; | ||
using System.Text.Json.Serialization; | ||
|
||
namespace NodaTime.Serialization.SystemTextJson; | ||
|
||
/// <summary> | ||
/// Provides JSON default converters for Noda Time types, as if using serializer | ||
/// options configured by <see cref="Extensions.ConfigureForNodaTime(JsonSerializerOptions, IDateTimeZoneProvider)"/> | ||
/// with a provider of <see cref="DateTimeZoneProviders.Tzdb"/>. | ||
/// </summary> | ||
/// <remarks> | ||
/// This attribute allows JSON conversion to be easily specified for properties without | ||
/// having to configure a specific options object. | ||
/// </remarks> | ||
public sealed class NodaTimeDefaultJsonConverterAttribute : JsonConverterAttribute | ||
{ | ||
/// <summary> | ||
/// Constructs an instance of the attribute. | ||
/// </summary> | ||
public NodaTimeDefaultJsonConverterAttribute() | ||
{ | ||
} | ||
|
||
/// <inheritdoc /> | ||
public override JsonConverter CreateConverter(Type typeToConvert) => | ||
NodaTimeDefaultJsonConverterFactory.GetConverter(typeToConvert); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,102 @@ | ||
// Copyright 2023 The Noda Time Authors. All rights reserved. | ||
// Use of this source code is governed by the Apache License 2.0, | ||
// as found in the LICENSE.txt file. | ||
|
||
using NodaTime.Serialization.SystemTextJson; | ||
using NodaTime.TimeZones; | ||
using NUnit.Framework; | ||
using System; | ||
using System.Collections.Generic; | ||
using System.Text; | ||
using System.Text.Json; | ||
using System.Text.Json.Serialization; | ||
|
||
namespace NodaTime.Serialization.Test.SystemTextJson; | ||
|
||
public partial class NodaTimeDefaultJsonConverterFactoryTest | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps give this a more-specific name. This isn't testing You might also link to e.g. https://learn.microsoft.com/en-us/dotnet/standard/serialization/system-text-json/source-generation in comments here to explain what's going on? Separately, let's add a test (which could be called There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've left the source generation test present in the same class (but renamed it to SourceGenerationCompatibility) and added more "normal" tests above. |
||
{ | ||
private static readonly List<Type> convertibleTypes = new() | ||
{ | ||
typeof(AnnualDate), | ||
typeof(AnnualDate?), | ||
typeof(DateInterval), | ||
typeof(DateTimeZone), | ||
typeof(Duration), | ||
typeof(Duration?), | ||
typeof(Instant), | ||
typeof(Instant?), | ||
typeof(Interval), | ||
typeof(Interval?), | ||
typeof(LocalDate), | ||
typeof(LocalDate?), | ||
typeof(LocalTime), | ||
typeof(LocalTime?), | ||
typeof(Offset), | ||
typeof(Offset?), | ||
typeof(OffsetDate), | ||
typeof(OffsetDate?), | ||
typeof(OffsetDateTime), | ||
typeof(OffsetDateTime?), | ||
typeof(OffsetTime), | ||
typeof(OffsetTime?), | ||
typeof(Period), | ||
// Note: YearMonth isn't supported yet | ||
typeof(ZonedDateTime), | ||
typeof(ZonedDateTime?), | ||
}; | ||
|
||
private static readonly List<Type> nonConvertibleTypes = new() | ||
{ | ||
typeof(int), | ||
typeof(int?), | ||
typeof(PeriodBuilder), | ||
typeof(ZoneInterval) | ||
}; | ||
|
||
[Test] | ||
[TestCaseSource(nameof(convertibleTypes))] | ||
public void ConvertibleTypes(Type type) | ||
{ | ||
var factory = new NodaTimeDefaultJsonConverterFactory(); | ||
Assert.IsTrue(factory.CanConvert(type)); | ||
var converter = factory.CreateConverter(type, default); | ||
Assert.NotNull(converter); | ||
// The converter doesn't "advertise" that it handles nullable value types, | ||
// unlike the Newtonsoft.Json version. | ||
var typeToCheckForCanConvert = Nullable.GetUnderlyingType(type) ?? type; | ||
Assert.IsTrue(converter.CanConvert(typeToCheckForCanConvert)); | ||
} | ||
|
||
[Test] | ||
[TestCaseSource(nameof(nonConvertibleTypes))] | ||
public void NonConvertibleTypes(Type type) | ||
{ | ||
var factory = new NodaTimeDefaultJsonConverterFactory(); | ||
Assert.IsFalse(factory.CanConvert(type)); | ||
var converter = factory.CreateConverter(type, default); | ||
Assert.IsNull(converter); | ||
} | ||
|
||
// See https://github.com/nodatime/nodatime.serialization/issues/97 and | ||
// https://learn.microsoft.com/en-us/dotnet/standard/serialization/system-text-json/source-generation | ||
[Test] | ||
public void SourceGenerationCompatibility() | ||
{ | ||
var sample = new SampleData { Foo = Instant.FromUtc(2023, 8, 6, 12, 40, 12) }; | ||
byte[] utf8Json = JsonSerializer.SerializeToUtf8Bytes(sample, SampleJsonContext.Default.SampleData); | ||
string actual = Encoding.UTF8.GetString(utf8Json); | ||
string expected = "{\"Foo\":\"2023-08-06T12:40:12Z\"}"; | ||
Assert.AreEqual(expected, actual); | ||
} | ||
|
||
public class SampleData | ||
{ | ||
[JsonConverter(typeof(NodaTimeDefaultJsonConverterFactory))] | ||
public Instant Foo { get; set; } | ||
} | ||
|
||
[JsonSerializable(typeof(SampleData))] | ||
public partial class SampleJsonContext : JsonSerializerContext | ||
{ | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider renaming
options
tounused
so that it's obvious that the parameter is being deliberately ignored?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would prefer not to do that, as it can cause confusion with named arguments. But I could fill in documentation directly instead of inheriting it, and stress that the parameter is unused?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, good point about named arguments.
I think it's okay to leave the documentation as-is: it's still correct, it's just that the options aren't relevant to any of the returned converters (I assume that is true, rather than it being a bug that we're not looking at them).
If
<inheritdoc/>
allows adding extra information (like{@inheritDoc}
in Java), you could add a sentence here, but I'm not sure it's necessary.