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

Timezone timestamp decoder behavior mismatch with hash vs. keyword arguments on v1.5.0 #524

Closed
bravehager opened this issue Apr 24, 2023 · 1 comment · Fixed by #525
Closed

Comments

@bravehager
Copy link

Description

The behavior of timestamp convenience classes such as PG::TextDecoder::TimestampUtc is different when using a hash vs. keyword arguments.

With a hash argument, we use Hash#merge, ensuring that flags are always set correctly even if they are passed via the constructor. With keyword arguments, it's possible to accidentally overwrite the value of flags such that they are not set correctly (see rails/rails#48055).

# Before
class TimestampUtc < Timestamp
  def initialize(params={})
    super(params.merge(flags: PG::Coder::TIMESTAMP_DB_UTC | PG::Coder::TIMESTAMP_APP_UTC))
  end
end

# After
class TimestampUtc < Timestamp
  def initialize(**kwargs)
    super(flags: PG::Coder::TIMESTAMP_DB_UTC | PG::Coder::TIMESTAMP_APP_UTC, **kwargs)
  end
end
@larskanis
Copy link
Collaborator

Sorry for the inconvenience! The order is wrong. It should be:

super(**kwargs, flags: PG::Coder::TIMESTAMP_DB_UTC | PG::Coder::TIMESTAMP_APP_UTC)

I'll fix this in pg and release pg-1.5.1 today. But the change as in rails/rails#48048 is right and expected.

larskanis added a commit to larskanis/ruby-pg that referenced this issue Apr 24, 2023
It is unexpected, since it makes the coder behave contrary to the class name.
Moreover it causes a regression in rails:
  rails/rails#48049

Fixes ged#524
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants