Skip to content
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.

Sign up for GitHub

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

Jump to bottom

NodaTimeDefaultJsonConverter doesn't seem to work with Interval #135

Open
kaydensigh opened this issue Feb 12, 2025 · 2 comments · May be fixed by #136
Open

NodaTimeDefaultJsonConverter doesn't seem to work with Interval #135

kaydensigh opened this issue Feb 12, 2025 · 2 comments · May be fixed by #136

Comments

@kaydensigh
Copy link

Hi, I can't seem to get NodaTimeDefaultJsonConverter to work with Interval.

I added kaydensigh@ab0aef9 to NodaTimeDefaultJsonConverterAttributeTest.RoundTrip and the test fails with:

Message: 
  Assert.That(actual, Is.EqualTo(expected))
  Expected: 2023-05-29T14:11:23Z/2025-01-02T03:04:05Z
  But was:  1970-01-01T00:00:00Z/1970-01-01T00:00:00Z

Stack Trace: 
ClassicAssert.AreEqual(Object expected, Object actual)
NodaTimeDefaultJsonConverterAttributeTest.Roundtrip() line 32
1)    at NUnit.Framework.Legacy.ClassicAssert.AreEqual(Object expected, Object actual)
NodaTimeDefaultJsonConverterAttributeTest.Roundtrip() line 32

Is this expected?
@jskeet
Copy link
Member

jskeet commented Feb 12, 2025

Will look into this over the weekend. It looks like it's expected to work, given that an interval converter is specified in https://github.com/nodatime/nodatime.serialization/blob/main/src/NodaTime.Serialization.SystemTextJson/NodaTimeDefaultJsonConverterFactory.cs

@jskeet
Copy link
Member

jskeet commented Feb 15, 2025 •
edited
Loading

Okay, I think I've worked out what's going on - ish. It does go into the NodaIntervalConverter, but then when we call options.WriteType with each of the instants, the JsonSerializerOptions don't have any converter registered for Instant, so it writes an empty string.

Basically I'll need to add a field to NodaIntervalConverter and NodaDateIntervalConverter so they each have a converter to use when reading/writing. It's not immediately clear to me what should happen if the caller has provided a JsonSerializerOptions which already has an InstantConverter though - will see what happens with other properties. (I don't know whether the defaulting is only applied as a fallback.)

jskeet added a commit to jskeet/nodatime.serialization that referenced this issue Feb 15, 2025
@jskeet
Use explicit subconverters for [NodaTimeDefaultJsonConverter] for Int…
e1f2cf4 
…erval and DateInterval

Fixes nodatime#135
@jskeet jskeet linked a pull request Feb 15, 2025 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@jskeet @kaydensigh