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

Inconsistency between Time formats in ROM::Changeset::PipeRegistry and ROM::Plugins::Command::Timestamps #663

Open
Aerdayne opened this issue Jan 4, 2022 · 1 comment

Comments

@Aerdayne
Copy link

Aerdayne commented Jan 4, 2022

Describe the bug

When timestamps are added using PipeRegistry.add_timestamps (source) current time is NOT converted to UTC.

Command::Timestamps (source), on the other hand, converts the current time to UTC.

This leads to a situation in certain scenarios (for example, when Postgres is the database adapter - Sequel's DateTime type is mapped to Postgres' timestamp without time zone, hence the timezone offset is not being parsed) where equal timestamps differ since one of the timestamps gets converted to UTC beforehand.

I think that both APIs should at least have the same behavior, as in either they both convert to UTC before persisting the timestamps, or both preserve the application timezone.

To Reproduce

require 'rom'

rom = ROM.container(:sql, 'postgres://username:[email protected]/database') do |conf|
  conf.default.create_table(:users) do
    primary_key :id
    column :data, String, null: false

    column :created_at, DateTime, null: false # Postgres' "timestamp without time zone"
  end

  class Users < ROM::Relation[:sql]
    schema(infer: true)
  end

  conf.register_relation(Users)
end

class UserRepo < ROM::Repository[:users]
  commands :create, use: :timestamps, plugins_options: { timestamps: { timestamps: %i[created_at] } }

  def create_with_changeset(data)
    users
      .changeset(:create, { data: data })
      .map(:add_timestamps)
      .commit
  end

  def find(id)
    root.by_pk(id).one
  end
end

repo = UserRepo.new(rom)

with_command = repo.create(data: 'user #1')
with_changeset = repo.create_with_changeset('user #2')

Time.now # => 2022-01-04 12:51:44.463493 +0300
created_at_from_command = with_command.created_at # => 2022-01-04 09:51:44.451431 +0300
created_at_from_changeset = with_changeset.created_at # => 2022-01-04 12:51:44.460317 +0300

p created_at_from_changeset - created_at_from_command < 0.1 # => false

Expected behavior

created_at_from_command and created_at_from_changeset should be approximately the same, not have a 3-hour difference.

My environment

  • Ruby version: 3.0.1
  • ROM versions:
    rom-5.2.6
    rom-changeset-5.2.3
    rom-core-5.2.6
    rom-repository-5.2.2
    rom-sql-3.5.0

This is possible to avoid by replacing .map(:add_timestamps) with .map { |tuple| tuple.merge(created_at: Time.now.utc) } as a temporary workaround.

Should I submit a PR?

@solnic
Copy link
Member

solnic commented Jan 5, 2022

Thanks for reporting this. I'm afraid this needs to wait until rom 6.0 because it's one of those cases where fixing a bug may break people's code.

@solnic solnic added this to the 6.0.0 milestone Jan 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants