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.

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

Bug: Timespan and DateTime do not maintain their values when serialised / deserialised #138

Closed
2 tasks done
kevinoneill opened this issue Nov 4, 2024 · 2 comments · Fixed by #140
Closed
2 tasks done
Labels
bug Something isn't working

Comments

@kevinoneill
Copy link

Describe the bug

While testing some of my own serialisation I found issues with the round trip serialisation of DateTime and Timespan.

I've included my xUnit/FSCheck property tests for reference.

I have fixes for the issues (included below) and I was going to add the tests and fixes as a pull request but I don't seem to be able to build the project locally (libsurreal_memory.dylib is missing from SurrealDb.Embedded.InMemory

Steps to reproduce

The following xUnit/FSCheck tests will fail.

  [Property(DisplayName = "When given a time span it should serialize and deserialize correctly")]
  public bool TimeSpanSerialization(TimeSpan value) {
    return Deserialize(Serialize(value)) == value;

    TimeSpan Deserialize(ReadOnlySpan<byte> source) => Cbor.Deserialize<TimeSpan>(source, new CborOptions().WithWeRaceOptions());

    ReadOnlySpan<byte> Serialize(TimeSpan source) {
      var buffer = new ArrayBufferWriter<byte>();

      Cbor.Serialize(source, buffer, new CborOptions().WithWeRaceOptions());
      return buffer.WrittenMemory.Span;
    }
  }
  [Property(DisplayName = "When given a date time it should serialize and deserialize correctly")]
public bool DateTimeSerialization(DateTime seed) {
  var utc = seed.ToUniversalTime();

  var result = Deserialize(Serialize(utc));

  return result.Equals(utc);

  DateTime Deserialize(ReadOnlySpan<byte> source) =>
    Cbor.Deserialize<DateTime>(source, new CborOptions().WithWeRaceOptions());

  ReadOnlySpan<byte> Serialize(DateTime source) {
    var buffer = new ArrayBufferWriter<byte>();

    Cbor.Serialize(source, buffer, new CborOptions().WithWeRaceOptions());
    return buffer.WrittenMemory.Span;
  }

Expected behaviour

The values should round trip correctly.

The following implementations resolve the issue.

internal class DateTimeConverter : CborConverterBase<DateTime> {
  // ReSharper disable once InconsistentNaming
  private const int CBOR_ARRAY_SIZE = 2;

  public override DateTime Read(ref CborReader reader) {
    reader.ReadBeginArray();

    var size = reader.ReadSize();
    if (size != CBOR_ARRAY_SIZE) {
      throw new CborException("Expected a CBOR array with 2 elements");
    }

    var seconds = reader.ReadInt64();
    var nanos = reader.ReadInt32();

    return DateTime.UnixEpoch.AddTicks(
      (seconds * TimeSpan.TicksPerSecond) +
      (nanos / 100)
    );
  }

  public override void Write(ref CborWriter writer, DateTime value) {
    var utc = value.Kind == DateTimeKind.Utc ? value : value.ToUniversalTime();

    var ticks = utc.Ticks - DateTime.UnixEpoch.Ticks;

    var seconds = ticks / TimeSpan.TicksPerSecond;
    var remaining = ticks % TimeSpan.TicksPerSecond;
    var nanos = (int)remaining * 100;

    writer.WriteSemanticTag(CborTagConstants.TAG_CUSTOM_DATETIME);
    writer.WriteBeginArray(CBOR_ARRAY_SIZE);

    writer.WriteInt64(seconds);
    writer.WriteInt32(nanos);

    writer.WriteEndArray(CBOR_ARRAY_SIZE);
  }
}
internal sealed class TimeSpanConverter : CborConverterBase<TimeSpan> {
  public override TimeSpan Read(ref CborReader reader) {
    reader.ReadBeginArray();

    var size = reader.ReadSize();
    if (size > 2) {
      throw new CborException("Expected a CBOR array with at most 2 elements");
    }

    var seconds = size >= 1 ? reader.ReadInt64() : 0;
    var nanos = size >= 2 ? reader.ReadInt32() : 0;

    return TimeSpan.FromTicks(
      (seconds * TimeSpan.TicksPerSecond) +
      (nanos / 100)
    );
  }

  public override void Write(ref CborWriter writer, TimeSpan value) {
    writer.WriteSemanticTag(CborTagConstants.TAG_CUSTOM_DURATION);

    var seconds = value.Ticks / TimeSpan.TicksPerSecond;
    var remaining = value.Ticks % TimeSpan.TicksPerSecond;
    var nanos = (int)remaining * 100;

    var writeNanos = nanos != 0;
    var writeSeconds = writeNanos || seconds != 0;

    var size = (writeSeconds ? 1 : 0) + (writeNanos ? 1 : 0);

    writer.WriteBeginArray(size);

    if (writeSeconds) {
      writer.WriteInt64(seconds);
    }

    if (writeNanos) {
      writer.WriteInt32(nanos);
    }

    writer.WriteEndArray(size);
  }
}

SurrealDB version

v2.0.4

Package version(s)

0.6.0

Contact Details

No response

Is there an existing issue for this?

  • I have searched the existing issues

Code of Conduct

  • I agree to follow this project's Code of Conduct
@kevinoneill kevinoneill added the bug Something isn't working label Nov 4, 2024
@kevinoneill
Copy link
Author

Note that the with werace options simply loads the serialisers which I copied into my project as they are not exposed externally.

@Odonno
Copy link
Contributor

Odonno commented Nov 5, 2024

Hello @kevinoneill

I notice the main difference with your version is how you convert nanoseconds using Ticks. Is it where the issue lies?
I also noticed the usage of the UTC datetime for the DateTimeConverter.

If it is, I will write proper tests that should fail with the current implementation in order to apply the fix you suggested.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants