1.90 client with Elastic 1.7.5 - Incorrect roundtrip of DateTime & DateTimeOffsets

[niemyjski]

47:44.92714 T:EventRepository - PUT http://localhost:9200/test-events-201609/events/57ce0700f8a10338d0e43fef
{
  "id": "57ce0700f8a10338d0e43fef",
  "is_first_occurrence": false,
  "is_fixed": false,
  "is_hidden": false,
  "created_utc": "2016-09-06T21:47:44.420841Z",
  "date": "2016-09-05T19:00:00-05:00"
}

47:45.05973 T:EventRepository - GET http://localhost:9200/test-events-201609/events/57ce0700f8a10338d0e43fef

var ev = await _repository.AddAsync(new PersistentEvent { CreatedUtc = DateTime.UtcNow, Date = new DateTimeOffset(DateTime.UtcNow.Date, TimeSpan.Zero)});
var result = await _repository.GetByIdAsync(ev.Id);
Assert.Equal(ev.CreatedUtc, result.CreatedUtc);
Assert.Equal(ev.Date, result.Date);

As you can see from the screenshot and code above...

• The DateTime CreatedUtc property will lose the timezone info when deserialized from the server (this changes the DateTime.Kind property which is a big issue).
• The DateTimeOffset Date property loses offset information during serialization (when being sent to the server).

The asserts pass because the tick count is the same but if you implement GetHashCode or assert the Kind and Offset properties it will fail.

[niemyjski]

Here is the source code for the test (https://github.com/exceptionless/Exceptionless/blob/feature/repos/Source/Tests/Repositories/EventRepositoryTests.cs#L29)

[DaveVdE]

Never use DateTime for ISO8601. It always treats 'm as "local", no matter what the offset says.

In fact, I don't ever want to use DateTime for any purpose again.

[niemyjski]

@DaveVdE that's not really ever an acceptable answer, that's like saying don't use int. It's the core type for dates in .NET.

[DaveVdE]

I disagree. It was the core type for dates in .NET, until .NET 3.5.

But it's fundamentally broken, especially when it comes to representing local time and UTC (it always converts between the two using the system's current offset) and it doesn't handle ISO-8601 well.

In my opinion it should be deprecated.

But the question remains of how Elasticsearch.NET should handle serialisation then. Should it circumvent the problem by converting it to DateTimeOffset before serialisation? Or is that not the concern of the library?

[russcam]

Hey @niemyjski, I've opened PRs for both 1.x and 2.x that adds serialization tests for DateTime with all different DateTimeKind variants and also DateTimeOffset with three different offsets, including TimeSpan.Zero.

The 1.x tests make a comparison of what NEST does (which uses IsoDateTimeConverter for handling DateTime) compared to Json.NET's default behaviour, which handles DateTime internally, I've tried to summarise the key difference at the bottom. I haven't been able to reproduce your findings in any of the tests.

In all versions of 2.x released, we have used IsoDateTimeConverter to also serialize and deserialize DateTimeOffset, which is a difference from 1.x. Whilst using IsoDateTimeConverter is equally as valid as not using it, a key difference is that it always applies the local offset to the deserialized DateTimeOffset (adjusting clock time too), whereas the default Json.NET behaviour is to deserialize to a DateTimeOffset with an offset that reflects the one in the ISO8601 string. As an example:

var json = "2016-01-01T01:01:07.4553425+05:00";

using (var stream = new MemoryStream(Encoding.UTF8.GetBytes(nestJson)))
{
    var nestDateTimeOffset = client.Serializer.Deserialize<DateTimeOffset>(stream);
    nestDateTimeOffset .Ticks.Dump();
    nestDateTimeOffset .DateTime.Dump();
    nestDateTimeOffset .Offset.Dump();
}

var jsonNetDateTimeOffset = JsonConvert.DeserializeObject<DateTimeOffset>(json);
jsonNetDateTimeOffset .Ticks.Dump();
jsonNetDateTimeOffset .DateTime.Dump();
jsonNetDateTimeOffset .Offset.Dump();

for me here in Australia (+11:00 offset for the target date), this yields

NEST
-----
635872284674553425
01/01/2016 07:01:07
11:00:00

JSON.NET
-----------
635872068674553425
01/01/2016 01:01:07
05:00:00

There is another PR open to remove using IsoDateTimeConverter for handling DateTimeOffset for the reason highlighted here.

I had a brief look through your linked test; is it possible that DateTime is changed somewhere in the test infrastructure, maybe in TestSystemClock?

[niemyjski]

I'll try taking a look into this today. TestSystemClock just wraps the DateTime.Now instance and returns it.

[niemyjski]

Is there any chance we could setup a time to pair on this? I just spent quite a bit of time doing an integration test for this and comparing it to our setup and there appear to be no differences yet yours works and mine doesn't.

    [TestFixture]
    public class DefaultDateTimeHandlingIntegrationTests : IntegrationTests {
        protected Flight Flight;
        protected string LocalOffsetString;
        protected TimeSpan LocalOffset;

        [SetUp]
        public void SetUp() {
            var departureDateLocal = new DateTime(2013, 1, 21, 0, 0, 0, DateTimeKind.Local);
            LocalOffset = TimeZoneInfo.Local.GetUtcOffset(departureDateLocal);

            var departureDateUtc = new DateTime(2013, 1, 21, 0, 0, 0, DateTimeKind.Utc);

            Flight = new Flight {
                DepartureDate = new DateTime(2013, 1, 21, 0, 0, 0, DateTimeKind.Unspecified),
                DepartureDateUtc = departureDateUtc,
                DepartureDateLocal = departureDateLocal,
                DepartureDateUtcWithTicks = departureDateUtc.AddTicks(3456789),
                DepartureDateOffset = new DateTimeOffset(2013, 1, 21, 0, 0, 0, LocalOffset),
                DepartureDateOffsetZero = new DateTimeOffset(2013, 1, 21, 0, 0, 0, TimeSpan.Zero),
                DepartureDateOffsetNonLocal = new DateTimeOffset(2013, 1, 21, 0, 0, 0, TimeSpan.FromHours(-6.25)),
            };

            LocalOffsetString = string.Format("{0}:{1}",
                LocalOffset.Hours.ToString("+00;-00;"),
                LocalOffset.Minutes.ToString("00"));

            CreateFlightIndex(Client);
        }

        [Test]
        public void CanRoundTripFlight() {
            var response =  Client.Index(Flight, d => d.Type("flight").Index("flight"));
            response.IsValid.Should().Be(true);

            var getResponse = Client.Get<Flight>(d => d.Type(response.Type).Index(response.Index).Id(response.Id));
            getResponse.IsValid.Should().Be(true);
            getResponse.Found.Should().Be(true);

            var flight = getResponse.Source;
            flight.Should().Be(Flight);
            flight.DepartureDate.Kind.Should().Be(Flight.DepartureDate.Kind);
            flight.DepartureDateLocal.Kind.Should().Be(Flight.DepartureDateLocal.Kind);
            flight.DepartureDateUtc.Kind.Should().Be(Flight.DepartureDateUtc.Kind);
            flight.DepartureDateUtcWithTicks.Kind.Should().Be(Flight.DepartureDateUtcWithTicks.Kind);
            flight.DepartureDateOffset.Offset.Should().Be(Flight.DepartureDateOffset.Offset);
            flight.DepartureDateOffsetZero.Offset.Should().Be(Flight.DepartureDateOffsetZero.Offset);
            flight.DepartureDateOffsetNonLocal.Offset.Should().Be(Flight.DepartureDateOffsetNonLocal.Offset);
        }

        private static void CreateFlightIndex(IElasticClient client) {
            var createIndexResult = client.CreateIndex("flight", c => c
                .NumberOfReplicas(ElasticsearchConfiguration.NumberOfReplicas)
                .NumberOfShards(ElasticsearchConfiguration.NumberOfShards)
                .AddMapping<Flight>(m => m
                    .Dynamic()
                    .MapFromAttributes()
                    )
                );
            createIndexResult.IsValid.Should().BeTrue();
        }
    }

    public class Flight {
        public DateTime DepartureDate { get; set; }
        public DateTime DepartureDateUtc { get; set; }
        public DateTime DepartureDateLocal { get; set; }
        public DateTime DepartureDateUtcWithTicks { get; set; }
        public DateTimeOffset DepartureDateOffset { get; set; }
        public DateTimeOffset DepartureDateOffsetZero { get; set; }
        public DateTimeOffset DepartureDateOffsetNonLocal { get; set; }

        protected bool Equals(Flight other) {
            return DepartureDate.Ticks.Equals(other.DepartureDate.Ticks) &&
                   DepartureDateUtc.Ticks.Equals(other.DepartureDateUtc.Ticks) &&
                   DepartureDateLocal.Ticks.Equals(other.DepartureDateLocal.Ticks) &&
                   DepartureDateUtcWithTicks.Ticks.Equals(other.DepartureDateUtcWithTicks.Ticks) &&
                   DepartureDateOffset.Ticks.Equals(other.DepartureDateOffset.Ticks) &&
                   DepartureDateOffsetZero.Ticks.Equals(other.DepartureDateOffsetZero.Ticks) &&
                   DepartureDateOffsetNonLocal.Ticks.Equals(other.DepartureDateOffsetNonLocal.Ticks);
        }

        public override bool Equals(object obj) {
            if (ReferenceEquals(null, obj))
                return false;
            if (ReferenceEquals(this, obj))
                return true;
            if (obj.GetType() != this.GetType())
                return false;
            return Equals((Flight)obj);
        }

        public override int GetHashCode() {
            unchecked {
                var hashCode = DepartureDate.GetHashCode();
                hashCode = (hashCode * 397) ^ DepartureDateUtc.GetHashCode();
                hashCode = (hashCode * 397) ^ DepartureDateLocal.GetHashCode();
                hashCode = (hashCode * 397) ^ DepartureDateUtcWithTicks.GetHashCode();
                hashCode = (hashCode * 397) ^ DepartureDateOffset.GetHashCode();
                hashCode = (hashCode * 397) ^ DepartureDateOffsetZero.GetHashCode();
                hashCode = (hashCode * 397) ^ DepartureDateOffsetNonLocal.GetHashCode();
                return hashCode;
            }
        }
    }

[gmarz]

Closed via #2269

[niemyjski]

I need to uncomment our test in 5.x branch and try this but I'm still working on getting my mappings working