aaronrenner / phx_gen_auth

An authentication system generator for Phoenix 1.5 applications.
772 stars 55 forks source link

Add support to timestamps with UTC #105

Closed sebasortiz-dev closed 3 years ago

sebasortiz-dev commented 3 years ago

In the current generator all timestamps are ecto default naive_datetime. I would be useful if another parameter, similar a --binary-id is added to support DateTime and :utc_datetime_usec as type for timestamps.

josevalim commented 3 years ago

@aaronrenner what do you think? If you think it is a good idea, we can probably accept a pull request that adds this to Phoenix directly (since the generators have been merged).

aaronrenner commented 3 years ago

@sebasortiz-dev Thanks for bringing this up! I think this is a good idea, but should be solved at the phoenix level first so all code generators can benefit.

I think this would be similar to the --binary-id flag on phx.new where a --timestamp-type utc_datetime_usec sets a config value like this.

      config :your_app, :generators,
        timestamp_type: :utc_datetime_usec

Then when schemas are generated the generator would add @timestamp_opts to the schema if it's configured to something other than :naive_datetime.

defmodule YourApp.Blog.Post do

  @timestamps_opts type: :utc_datetime

  schema "posts" do
    #...
    timestamps
  end
end

I'd vote for figuring out a standard way to change this in Phoenix instead of changing it in only a specific generator.

@josevalim Thoughts?

rafaelfess commented 3 years ago

I've always wondered why UTC is not the default for timestamps.

josevalim commented 3 years ago

@aaronrenner I like your solution!

@rafaelfesi originally there was not a lot you could do with DateTime in Elixir, NaiveDateTime were more powerful, so they were picked as default. In practice, given that Ecto requires the database to be UTC, they end-up being pretty much the same. Then you can upgrade to DateTime only when you effectively want to work with timezones.

sebasortiz-dev commented 3 years ago

@josevalim @aaronrenner Awesome, thanks for taking the time to discuss this. I will try to make a PR with the solution you proposed. Will link the issue / PR from the Phoenix project.

kevinlang commented 3 years ago

I'm also interested in this. One question I have for your proposed solution, @aaronrenner :

AFAIK for the migration will also need to be updated to support the case where one of the _usec variants is provided. Do you think this should be done by setting the migration_timestamps for the entire project to have the corresponding value? Or should we update the migration generators to look at the timestamp_type configuration and then manually specify the :type when we generate the corresponding timestamps() part of the migration?

I'm slightly leaning towards the "look at timestamp_type approach", so that both the schema and the migration are controlled by the same configuration that can be changed between generator invocations if desired. But it also feels weird to not use a configuration that seems meant for this purpose :man_shrugging: .

@sebasortiz-dev , let me know if you still wanted to tackle this, if not I may try to see if I can come up with a suitable implementation.

aaronrenner commented 3 years ago

@sebasortiz-dev If this is still a request, would you mind opening it under the phoenix repo? Since phx.gen.auth has been released with phoenix 1.6, this repo is being archived. Thanks!