fix MatchError in decode_datetimeoffset

[simonmcconnell]

I've been trying to get datetimeoffset columns working but kept getting this error when retrieving data:

iex(7)> Repo.all(DTO3)
[debug] QUERY ERROR source="dto3" db=16.0ms idle=890.0ms
SELECT d0.[id], d0.[dto3] FROM [dto3] AS d0 []
** (MatchError) no match of right hand side value: {:ok, ~U[2020-08-17 23:53:07.075200Z], 36000}
    (ecto_sql 3.4.5) lib/ecto/adapters/sql.ex:593: Ecto.Adapters.SQL.raise_sql_call_error/1
    (ecto_sql 3.4.5) lib/ecto/adapters/sql.ex:526: Ecto.Adapters.SQL.execute/5
    (ecto 3.4.6) lib/ecto/repo/queryable.ex:192: Ecto.Repo.Queryable.execute/4
    (ecto 3.4.6) lib/ecto/repo/queryable.ex:17: Ecto.Repo.Queryable.all/3

Not the most accurate stack trace. One must descend the ladder of functions to end up at the root cause, that {:ok, datetime, ^offset_min} = DateTime.from_iso8601("#{str}+#{h}:#{m}") is trying to match seconds (what from_iso860 returns) and minutes. It needs to be:

offset = offset_min * 60
{:ok, datetime, ^offset} = DateTime.from_iso8601("#{str}+#{h}:#{m}")`

[coveralls]

Coverage increased (+0.06%) to 73.191% when pulling aad99ca on simonmcconnell:patch-1 into 11e2f91 on livehelpnow:master.

[simonmcconnell]

It's confusing or a bug that you need to dump to the database using {{utc_y, utc_m, utc_d}, {utc_hr, utc_min, utc_sec, utc_usec}, offset_mins}} as the standard is to display it in the local time with the offset. I also just realised that I need to add the offset to the decoded datetime to get it to return the correct value.

offset = offset_min * 60
:ok, datetime, ^offset} = DateTime.from_iso8601("#{str}+#{h}:#{m}")

DateTime.add(datetime, offset)

Migration

defmodule Datetimeoffset.Repo.Migrations.CreateDto3Table do
  use Ecto.Migration

  def change do
    create table(:dto3) do
      add :dto3, :datetimeoffset
    end
  end
end

Schema

defmodule Datetimeoffset.DTO3 do
  use Ecto.Schema
  alias Types.DateTimeOffset

  schema "dto3" do
    field :dto3, DateTimeOffset
  end
end

Custom Type

defmodule Types.DateTimeOffset do
  use Ecto.Type
  use Timex

  require Logger

  @doc """
  type returned to Schema
  """
  def type, do: :datetimeoffset

  @doc """
  cast to DateTime to be used at runtime

  TODO: add other casts...
  """
  def cast(%DateTime{} = dt) do
    Logger.debug(fn ->
      "casting DateTime: #{inspect(dt)}"
    end)

    {:ok, dt}
  end

  @doc """
  handle loading data from the database
  ( db ) --> load()
  """
  def load({%DateTime{} = dt, offset_mins}) do
    Logger.debug(fn ->
      "loading tuple of DateTime and offset" <>
        "{#{inspect(dt)}, #{offset_mins}}"
    end)

    {:ok, dt}
  end

  def load(%DateTime{} = dt) do
    Logger.debug(fn ->
      "loading DateTime " <>
        "#{inspect(dt)}"
    end)

    {:ok, dt}
  end

  def load({{y, mth, d}, {h, min, s}, offset_mins}) do
    Logger.debug(fn ->
      "loading tuple format without microseconds supplied."
    end)

    load({{y, mth, d}, {h, min, s, 0}, offset_mins})
  end

  def load({{y, mth, d}, {h, min, s, us}, offset_mins}) do
    Logger.debug(fn ->
      "loading tuple format " <>
        "{{#{y}, #{mth}, #{d}}, {#{h}, #{min}, #{s}, #{us}}, #{offset_mins}}"
    end)

    secs = :calendar.datetime_to_gregorian_seconds({{y, mth, d}, {h, min, s}})
    tzname = Timezone.name_of(offset_mins * 60)

    case Timezone.resolve(tzname, secs) do
      {:error, _} ->
        :error

      %TimezoneInfo{} = tz ->
        dt = %DateTime{
          :year => y,
          :month => mth,
          :day => d,
          :hour => h,
          :minute => min,
          :second => s,
          :microsecond => Timex.DateTime.Helpers.construct_microseconds({us, -1}),
          :time_zone => tz.full_name,
          :zone_abbr => tz.abbreviation,
          :utc_offset => tz.offset_utc,
          :std_offset => tz.offset_std
        }

        {:ok, dt}

      %AmbiguousTimezoneInfo{after: a} ->
        dt = %DateTime{
          :year => y,
          :month => mth,
          :day => d,
          :hour => h,
          :minute => min,
          :second => s,
          :microsecond => Timex.DateTime.Helpers.construct_microseconds({us, -1}),
          :time_zone => a.full_name,
          :zone_abbr => a.abbreviation,
          :utc_offset => a.offset_utc,
          :std_offset => a.offset_std
        }

        {:ok, dt}
    end
  end

  def load(_) do
    :error
  end

  @doc """
  convert to native ecto representation
  dump() -> ( db )
  """
  def dump(%DateTime{utc_offset: utc_offset} = dt) do
    %DateTime{
      year: y,
      month: mth,
      day: d,
      hour: h,
      minute: min,
      second: s,
      microsecond: {us, _}
    } = Timex.to_datetime(dt)

    offset_mins = div(utc_offset, 60)

    Logger.debug(fn ->
      "dumping #{inspect(dt)} to database as " <>
        "{{#{y}, #{mth}, #{d}}, {#{h}, #{min}, #{s}, #{us}}, #{offset_mins}}"
    end)

    {:ok, {{y, mth, d}, {h, min, s, us}, offset_mins}}
  end
end

IEx

iex> now = Timex.local
iex> dto3 = %Datetimeoffset.DTO3{dto3: now}
iex> Datetimeoffset.Repo.insert(dto3)
iex> Datetimeoffset.Repo.all(Datetimeoffset.DTO3)

[mjaric]

Hi @simonmcconnell, I will check PR tomorrow, and if it is all good I will make new release.

Thanks!

[simonmcconnell]

I'll add some tests specific to datetimeoffset and a custom type as per uuid if you're happy with that? Will try get to it over the weekend.

[mjaric]

That would be awesome

[simonmcconnell]

What is the story with the datetime tuple representation being different to the others? It takes a {date, {hr, min, sec, microsecond} tuple where the others take a {hr, min, sec, fractional_second} tuple.

def encode_datetime({date, {h, m, s, us}}) do
    days = :calendar.date_to_gregorian_days(date) - @year_1900_days
    milliseconds = ((h * 60 + m) * 60 + s) * 1_000 + us / 1_000

    secs_300 = round(milliseconds / (10 / 3))

    {days, secs_300} =
      if secs_300 == 25_920_000 do
        {days + 1, 0}
      else
        {days, secs_300}
      end

    <<days::little-signed-32, secs_300::little-unsigned-32>>
  end

def encode_time({hour, min, sec, fsec}, scale) do
    # 10^scale fs in 1 sec
    fs_per_sec = trunc(:math.pow(10, scale))

    fsec =
      hour * 3600 * fs_per_sec + min * 60 * fs_per_sec + sec * fs_per_sec + fsec

    bin =
      cond do
        scale < 3 ->
          <<fsec::little-unsigned-24>>

        scale < 5 ->
          <<fsec::little-unsigned-32>>

        :else ->
          <<fsec::little-unsigned-40>>
      end

    {bin, scale}
  end

[mjaric]

datetime values are rounded to increments of .000, .003, or .007 seconds while time is not, it is 100 nanoseconds increments. datetime cannot have value e.g. 01/01/2020 23:59:59.999 it is actually preserved as 01/02/2020 00:00:00.000. `

datetime does not "scale" it is always of 3 decimal precision with above increments. time can have user defined precision (the scale arg) 0-7 digits in database and in elixir builtin types we can only handle max 6 digits

[simonmcconnell]

Sorry about the commit mess, I'm not well versed in git yet 🤭.

I believe I've fixed the encode/decode issue for datetimeoffsets that aren't in UTC. Added a bunch of unit and integration (i.e. database) tests.

Added a Tds.Types.DateTimeOffset with some instructions in the module docs on how to use it for timestamps() as UTC or local time, which requires TzData and Timex. Did not include the local timestamp code in this package as it would add two dependencies. It could be a separate package but it is just a single function module.

This type might need the cast(%{}) functions like timex_ecto to handle embeds...

[simonmcconnell]

@mjaric have you had a chance to go over this?

[mjaric]

Yes, except bug fix, I think we need to create new mix lib project for Time, DateTime, DateTimeOffset custom types that should support full precision and offsets. Second reason for such approach is that we are not solving fully issues that comes with non UTC problems. One could insert datetime value with offset that is in future, since we are not storing any info about offset in database, nor sql server does, we can't guaranty that the time will be correct when that future comes. Simply because government can decide to change daylight saving, and turn off that completely. Because of that, it would be better to store zone info instead of offset and save UTC time, since it will be always the same. For datetime values that are less than or equal to a moment we are persisting the value your implementation would be Ok and historical values cannot be affected by the above rule changes.

Another reason that third-party dependencies are not so well fit in this case is that this library is also dependency in ecto_sql, if we add timezoneDB then we are forcing it to use same version as we do, and that will not happen because maintainers agreed not to do that except if such library is agreed to be officially supported/used by all adapters. Timex is probably mostly used, but I think most people are storing future dates as UTC and historical values with NaiveDateTime + zone info or DateTimeOffset + zone info.

Now, I don't want your work to vanish just because I said this repo is not good place for this. There is potential especially because you added full precision that sql server can support and builtin elixir types lack of. It would be best to create new github repository and move custom types there and we can add a note in README referencing that repository for those who may concern. If possible, try not to use timex except for tests if you have to, that way you leave choice to lib user since you leave them possibility to pick implementation and how long they will stick to the version they are using. You are still opinionated how offsets should be handled but you are not forcing opinion which DB to use (maybe that guy want to use offsets stored in database)

So action plan is that you move custom type in separate repo and we can merge offset handling fix. Don't forget to put in the readme file reference to the new repo or hex, it is valuable info.

[simonmcconnell]

Ok, will do.

I think if someone wants to use time(7), datetime2(7), or datetimeoffset(7) we really need new time/datetime types that are functionally equivalent to the standard library's Time, NaiveDateTime and DateTime types except that they use fractional seconds rather than microseconds, in addition to a (possibly parameterized) Ecto type. I had a quick look at the standard library calendar module and I think it will get messy if one ever wanted to convert to a different calendar, as the TSQL one would use fractional seconds and the other microseconds. Having a precision of 7 is not a requirement of mine so I will proceed down the easy path for now.

[simonmcconnell]

bug fix moved to #114