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

[ActiveRecord] PostgreSQL Adapter skips update on CIDR column when only netmask is changed #51582

Closed
dari-us opened this issue Apr 17, 2024 · 0 comments · Fixed by #51633
Closed

Comments

@dari-us
Copy link

dari-us commented Apr 17, 2024

Steps to reproduce

I cannot provide an executable test case of the exact problem since PostgreSQL cannot run in-memory easily, but I can demonstrate the underlying problem.
When I have a CIDR column in a PostgreSQL database with value 1.1.1.0/24 and I change it to 1.1.1.0/25, it will not be updated on save.

# frozen_string_literal: true

require "bundler/inline"

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

  gem "rails"
end

require "active_record"
require "active_record/connection_adapters/postgresql/oid"
require "minitest/autorun"
require "logger"

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

class BugTest < Minitest::Test
  def test_netmask_change
    type = ActiveRecord::ConnectionAdapters::PostgreSQL::OID::Cidr.new()
    old_value = type.cast_value("1.1.1.0/24")

    new_str = "1.1.1.0/25"
    new_value = type.cast_value(new_str)

    assert type.changed?(old_value, new_value, new_str)
  end
end

Expected behavior

When old_value is in any way different from new_value, then changed? should return true.

Actual behavior

old_value (1.1.1.0/24) is treated as the same as new_value (1.1.1.0/25) which is obviously not the case.

Further notes

The cause of this problem is, that IPAddr#== ignores the netmask for its comparison.
To solve the problem, it is necessary to overwrite ActiveRecord::ConnectionAdapters::PostgreSQL::OID::Cidr#changed? so it does not use IPAddr#== anymore. IPAddr#eql? is suitable for this, but may be overkill. There is an open issue addressing that IPAddr#== does not work as expected ruby/ipaddr#21

It is possible to monkey-patch ActiveRecord::ConnectionAdapters::PostgreSQL::OID::Cidr to work as expected:

class ActiveRecord::ConnectionAdapters::PostgreSQL::OID::Cidr
  def changed?(old_value, new_value, _new_value_before_type_cast)
    return old_value.nil? != new_value.nil? || (!old_value.nil? && !old_value.eql?(new_value))
  end
end

System configuration

Rails version: 7.1.3.2

Ruby version: 3.3.0

taketo1113 added a commit to taketo1113/rails that referenced this issue Apr 22, 2024
taketo1113 added a commit to taketo1113/rails that referenced this issue Apr 23, 2024
taketo1113 added a commit to taketo1113/rails that referenced this issue Apr 23, 2024


Update activerecord/lib/active_record/connection_adapters/postgresql/oid/cidr.rb

Co-authored-by: Yasuo Honda <yasuo.honda@gmail.com>

Updated to keep the argument order
taketo1113 added a commit to taketo1113/rails that referenced this issue Apr 23, 2024


Update activerecord/lib/active_record/connection_adapters/postgresql/oid/cidr.rb

Co-authored-by: Yasuo Honda <yasuo.honda@gmail.com>

Updated to keep the argument order

Added note to remove `changed?` method when `IPAddr#==` compares `IPAddr#prefix`
fractaledmind pushed a commit to fractaledmind/rails that referenced this issue May 13, 2024


Update activerecord/lib/active_record/connection_adapters/postgresql/oid/cidr.rb

Co-authored-by: Yasuo Honda <yasuo.honda@gmail.com>

Updated to keep the argument order

Added note to remove `changed?` method when `IPAddr#==` compares `IPAddr#prefix`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants