timestamps() generates nullable fields

[ppraisethesun]

Readme shows:

...
  inserted_at: NaiveDateTime.t(),
  updated_at: NaiveDateTime.t()
...

typed_schema "people" do
  ...
  timestamps(type: :naive_datetime_usec)
end

However for me it generates the following spec:

  inserted_at: NaiveDateTime.t() | nil,
  updated_at: NaiveDateTime.t() | nil

[bamorim]

This is intentional, as you might have something that was not inserted yet.
The difference between inserted and not inserted is something that we could maybe consider again.

[bamorim]

But I think setting it as non null would be better.

[bamorim]

@ppraisethesun sorry for the slow resolution time, but after more thought I think the null should be there for the non-inserted cases.

I was thinking about a few ideas regarding that (and other nullability issues regarding inserted/non-inserted):

• Update the docs to state that it should be nullable (for the non inserted values)
• Set the types to non nullable if we do:

  typed_schema "people" do
    timestamps(type: :naive_datetime_usec, null: false)
  end

• Maybe we could generate another type inserted() that would make sure that all generated values are there, but I'm not sure if it is worth, but might be interesting exploring this idea

Any thoughts on that as well @dvic?

[dvic]

I have thought about this lately (inserted/non-inserted) and came to the conclusion that the only "proper" way to do this is to have multiple variants:

If I have

defmodule People do
  @primary_key {:id, Ecto.UUID, autogenerate: true}

  typed_schema "people" do
    field :name, :string, null: false
    field :age, :integer
    timestamps(type: :naive_datetime_usec)
  end
end

t ::
  # type for inserted
  %__MODULE__{
    id: Ecto.UUID.t(),
    # assuming name is required in the DB
    name: String.t(),
    age: integer() | nil,
    # once inserted, inserted_at and updated_at are both non-null
    inserted_at: NaiveDateTime.t(),
    updated_at: NaiveDateTime.t()
  }
  # type for non-inserted, everything is nullable,
  # because we can have trigger and default values
  # for everything (in theory) in the db
  | %__MODULE__{
      id: Ecto.UUID.t() | nil,
      name: String.t() | nil,
      age: integer() | nil,
      inserted_at: NaiveDateTime.t() | nil,
      updated_at: NaiveDateTime.t() | nil
    }

This might be too loose and opinionated but I think from the perspective of Dialyzer and how databases are used 99% of the time this should work.

[bamorim]

The problem is that this wouldn't help much as no simple check would narrow the type down to the inserted version.

[dvic]

The problem is that this wouldn't help much as no simple check would narrow the type down to the inserted version.

What do you mean with "narrow the type down"? This is the only thing we can express in dialyzer typespecs right?

[bamorim]

Type narrowing is the process by which a type checker can "narrow" the possible types. So for example, if we do:

@type result() :: %{success: true, data: [String.t()]} | %{success: false, data: nil}

If do

@spec do_something(result()) :: :ok
def do_something(result) do
  if result.success do
    # Here dialyzer "narrowed" result to be of type %{success: true, data: [String.t()]}
  else
    # Here dialyzer "narrowed" result to be of type %{success: false, data: nil}
  end
  
  :ok
end

The problem is that the disjunction we have there is exactly the same as just the second argument (we are just adding | nil to the nullable fields). We don't have any discriminant.

For non-embedded schemas, however, we could maybe take advantage of the Ecto.Schema.Metadata state so there could be a benefit by checking schema.__meta__.state == :loaded.

However, this is starting to get into area of diminishing returns, because this is not code people would idiomatically do, therefore I don't see much benefit in having the disjunction there as it adds more complexity and I can't see how that improves the experience of the user.

[dvic]

Yeah it's a bit of a subjective thing and depends on how people use it, probably the easiest is to have an option config-wide that determines this behaviour (also for primary keys)