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

Improve interval overflow protection #50163

Merged
merged 1 commit into from
Dec 11, 2023

Conversation

EduardoGHdez
Copy link
Contributor

@EduardoGHdez EduardoGHdez commented Nov 24, 2023

Sometimes, operations on Times returns just float numbers of seconds, so, we
need to handle that.
Example: Time.current - (Time.current + 1.hour) # => -3600.000001776 (Float)

When this happens, it's possible to endup with a pretty large number of seconds, making the ISO 8601 formatted duration to trigger an interval field value out range error.

PostgreSQL 15 has already improved overflow detection when casting values to interval

However, to further reduce the likelihood of such issues and ensure better-formatted duration types, it now implements the construction of ActiveSupport::Duration during the serialization step.

How to reproduce (at least in versions prior to PG 15+):

require "bundler/inline"

gemfile(true) do
  source "https://rubygems.org"

  git_source(:github) { |repo| "https://github.com/#{repo}.git" }

  gem "rails", github: "rails/rails", branch: "main"
  gem "pg"
end

require "active_record"
require "minitest/autorun"
require "logger"

ActiveRecord::Base.establish_connection(
  adapter: 'postgresql',
  database: 'postgres',
  username: 'postgres',
  host: 'localhost'
)

ActiveRecord::Base.logger = Logger.new(STDOUT)

ActiveRecord::Schema.define do
  create_table :projects, force: true do |t|
    t.interval :duration
  end
end

class Project < ActiveRecord::Base; end

class IntervalBugTest < Minitest::Test
  def test_duration
    project = Project.create(duration: 70.years)
    assert_equal 70.years, project.duration
  end

  def test_duration_error
    duration = 70.years.ago - Time.now()

    assert_raises ActiveRecord::StatementInvalid do
      Project.create(duration: duration)
    end
  end
end

Checklist

Before submitting the PR make sure the following are checked:

  • This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
  • Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
  • Tests are added or updated if you fix a bug or add a feature.
  • CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

> Sometimes, operations on Times returns just float numbers of seconds, so, we
need to handle that.
> Example: Time.current - (Time.current + 1.hour) # => -3600.000001776 (Float)

When this happens, it's possible to endup with a pretty large number of seconds,
making the ISO 8601 formatted duration to trigger an `interval field value out
range error`.

PostgreSQL 15 has already improved overflow detection when casting values to
interval

However, to further reduce the likelihood of such issues and ensure
better-formatted duration types, it now implements the construction of
`ActiveSupport::Duration` during the serialization step.

How to reproduce (at least in versions prior to PG 15+):

``` ruby

require "bundler/inline"

gemfile(true) do
  source "https://rubygems.org"

  git_source(:github) { |repo| "https://github.com/#{repo}.git" }

  gem "rails", github: "rails/rails", branch: "main"
  gem "pg"
end

require "active_record"
require "minitest/autorun"
require "logger"

ActiveRecord::Base.establish_connection(
  adapter: 'postgresql',
  database: 'postgres',
  username: 'postgres',
  host: 'localhost'
)

ActiveRecord::Base.logger = Logger.new(STDOUT)

ActiveRecord::Schema.define do
  create_table :projects, force: true do |t|
    t.interval :duration
  end
end

class Project < ActiveRecord::Base; end

class IntervalBugTest < Minitest::Test
  def test_duration
    project = Project.create(duration: 70.years)
    assert_equal 70.years, project.duration
  end

  def test_duration_error
    duration = 70.years.ago - Time.now()

    assert_raises ActiveRecord::StatementInvalid do
      Project.create(duration: duration)
    end
  end
end
```
@EduardoGHdez EduardoGHdez changed the title Improve postgres interval overflow protection Improve interval overflow protection Nov 24, 2023
@@ -44,6 +44,13 @@ def test_time_values
assert_equal (-21.day), @first_time.scaled_time_interval
end

def test_update_large_time_in_seconds
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was just curious and anyone else wondering this test fails prior to this patch with:

Error:
PostgresqlDataTypeTest#test_update_large_time_in_seconds:
ActiveRecord::StatementInvalid: PG::IntervalFieldOverflow: ERROR:  interval field value out of range: "PT2208986640.0S"
CONTEXT:  unnamed portal parameter $1 = '...'

@byroot byroot merged commit f5c6aa3 into rails:main Dec 11, 2023
4 checks passed
@EduardoGHdez EduardoGHdez deleted the interval-overflow-protection branch December 11, 2023 22:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants