Support for System.Text.Json

[DSilence]

Given that .net core 3.0 is around the corner I think it would be great to add support for System.Text.Json serializer. I've been playing around with it for some time and am willing to submit a PR.

[jskeet]

Go ahead :) It should definitely be in a new package rather than part of the existing ones.

[Newmski]

Is this something that will be implemented now that .NET Core 3.0 has been released?

[jskeet]

@Newmski: There's already an implementation in a separate branch. When I have time, I'll review it again and merge it into the master branch, then look at doing a beta release.

[jskeet]

One interesting question is whether it should target NodaTime 2.x or NodaTime 3.x. Or whether we should have two separate packages, one for each. Or two major versions of the same package, so that serialization package version x.y targets NodaTime x.0.

[alyssa-dahlberg]

Came to see if this was being worked on and I could contribute, will keep an eye out for a beta to test.

[0x15e]

I actually spent most of yesterday porting the JsonNet code over to System.Text.Json (they're very similar - it's almost like MS directly patterned System.Text.Json after JsonNet) and was about to start on the tests today when I noticed that there's already a branch and aside from a couple of naming differences and details I hadn't seen yet (like this one), my code looks very similar.

Re: @jskeet's comment about which NodaTime should be targeted, I was targeting the current stable release (2.4.7). That seems the most prudent way to go since 3.x isn't "official" yet. Is there some discussion I could read to learn more about the major differences / breaking changes in 3.x?

What's the status on the existing SystemText code? It looks like it's sat untouched for about two months now. Does that mean it's considered stable or just put on the back burner? I have an immediate need for that functionality and my employer encourages contribution to the projects we use so I'd be happy to do some polishing / testing on it.

[jskeet]

Biggest changes for 3.0:

• Only targeting netstandard2.0
• Dropping binary serialization

Status of other branch: waiting on me having time to re-review and then merge it, basically (and then release it, of course - which takes time in itself). I can try to get to that this weekend, but I can't make any promises.

[0x15e]

I just checked it out and aside from needing to update the System.Text.Json package reference from 4.6.0-preview to 4.6.0-release, everything looks good (tests included). I can work with this for the time being and will keep an eye on the project for updates. If I run into any problems I'll post an issue (and hopefully a PR).

Thanks!

[jskeet]

Hmm... we still have an issue as per #48 - the tests fail due to + being escaped to \u002b everywhere. It looks like this is a known issue with System.Text.Json, but I haven't been able to tell whether there's a workaround available yet. I don't think we want to ship it with + escaped everywhere. Any thoughts @0x15e and @DSilence?

[0x15e]

I have a couple of thoughts on this.

First, this causes the tests to fail, but does it cause a round-trip serialization / deserialization to fail? And would it cause problems serializing with, e.g., System.Text.Json but deserializing with JsonNet? Would JavaScript have a problem with unescaping the value?

Second, would it be a problem for the ConfigureforNodaTime extension to set the JsonSerializerOptions.Encoder property to JavaScriptEncoder.UnsafeRelaxedJsonEscaping the same way you do in the tests? Possibly as an option? I agree that it's less than ideal but not exactly unreasonable in my opinion.

[jskeet]

No, it shouldn't cause actual serialization/deserialization to fail - the documents should be equivalent for any valid parser.

I don't think ConfigureForNodaTime should start messing with the Encoder property though. The same encoder would be used for other types, which might not be safe - and indeed we want stricter encoding for other things, I suspect, just not plus.

I'm very tempted to change the tests in terms of what's expected, and just live with it being a bit ugly.

[0x15e]

I think from a tests perspective, it's probably more "correct" here to change the test to match the default encoder's output, even if it's ugly.

That said, do you think it would be worth the effort to implement a JavaScriptEncoder that has a special case for plus but delegates everything else to JavaScriptEncoder.Default? That way if someone ran into an edge case where they really needed that plus to be unescaped, there would be one available without them having to roll their own. Then again, they could probably just use UnsafeRelaxed in that case and not have to have a NodaTime-specific one. Now that I've said it out loud, it does feel a bit hacky to me.

This is the code responsible for escaping that plus but that's definitely not the only difference. The base allowed character set for the default encoder is basic latin but unsafe uses UnicodeRanges.All so I could definitely see why someone wouldn't want to allow all of that by default.

[DSilence]

I think the issue with escapes was fixed in #49, but even though it was merged I cannot find the changes in the current branch state. Could it have been lost for some reason? I've used JsonEncodedText.Encode(text, JavaScriptEncoder.UnsafeRelaxedJsonEscaping) to create a string without escaped characters on a Converter level, without messing up the global settings.

[jskeet]

I think there were more things being escaped before. I'll see if I can dig out the reflog to see.

[0x15e]

From what I saw, using UnsafeRelaxed will disable escaping on a whole lot of extra characters. Even just considering the base character set difference between it and the default encoder, things like homoglyph attacks would potentially become easier using UnsafeRelaxed.

I suspect it's called "unsafe" for a good reason.

[DSilence]

Fair points, but given that nodatime controls the resulting string generation itself via IPattern, I don't see a scenario in which using the encoder that allows special characters can be exploited.

With that said, I do agree that it may be worth it to give more control to the user, as it may be desired to keep the encoding on by default globally. The same rule applies to the Json.Net and it's StringEscapeHandling, which will override the string generation globally. The only difference is that Json.Net by default escapes only newline character, and none of the escape modes escapes '+' symbol, so it doesn't affect nodatime that much.

[jskeet]

Yup, I plan on looking through all this code more carefully and avoiding anything about unsafe encoding. I think I'm happy enough to accept the ugliness of the + encoding at this point.

Any objections?

[jskeet]

I've just released NodaTime.Serialization.SystemTextJson version 1.0.0-beta01. I haven't written any documentation for it yet, but I believe you just need to call the ConfigureForNodaTime extension method on JsonSerializerOptions.

Will leave this issue open until I've got some docs up.

[0x15e]

That's correct for doing one-off configuration. To do it globally in, e.g., a .NET Core 3.0 app, you do that in your ConfigureServices method, in IMvcBuilder.AddJsonOptions:

services
    .AddControllers()
    .AddJsonOptions(options => options
        .JsonSerializerOptions
        .ConfigureForNodaTime(DateTimeZoneProviders.Tzdb))
// ... other config
    .SetCompatibilityVersion(CompatibilityVersion.Latest);

Unfortunately it does take some wrangling if you want to inject your IDateTimeZoneProvider but honestly I couldn't think of a solution that wouldn't unnecessarily bloat the serialization package itself, and that's not exactly a unique problem to NodaTime.Serialization.SystemTextJson anyway.

[jskeet]

Great :) I'm hoping to get the documentation updated this week.

[jskeet]

We now have documentation - it's brief, but it's there...

• https://nodatime.org/unstable/userguide/serialization
• https://nodatime.org/serialization/api/NodaTime.Serialization.SystemTextJson.html