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

Fix PostgreSQL Cidr#change? to compare with address prefix #51633

Merged
merged 1 commit into from
Apr 24, 2024

Conversation

taketo1113
Copy link
Contributor

Motivation / Background

This Pull Request has been created because fixes #51582

Detail

This Pull Request changes to overrideActiveRecord::ConnectionAdapters::PostgreSQL::OID::Cidr#changed?.
It added to compare address prefix of IPAddr class.

Additional information

I’m proposing to fix IPAddr#== comparing address prefix for the cause of this problem.
ruby/ipaddr#69

Checklist

Before submitting the PR make sure the following are checked:

  • This Pull Request is related to one change. Unrelated changes 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.



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`
@yahonda yahonda merged commit 3e15614 into rails:main Apr 24, 2024
3 checks passed
yahonda added a commit to yahonda/rails that referenced this pull request Apr 24, 2024
Fix PostgreSQL `Cidr#change?` to compare with address prefix
@yahonda
Copy link
Member

yahonda commented Apr 24, 2024

Backported to the 7-1-stable branch via 887210f

@vipulnsward
Copy link
Member

@yahonda This needs a changelog as its a change in behavior?

@yahonda
Copy link
Member

yahonda commented Apr 30, 2024

Let me add a change log entry for this change. Thanks for the heads-up.

yahonda added a commit to yahonda/rails that referenced this pull request May 6, 2024
Follow up rails#51633

Co-authored-by: Taketo Takashima <t.taketo1113@gmail.com>
@yahonda
Copy link
Member

yahonda commented May 6, 2024

Opened #51742 to add a changelog entry for this change.

@chaadow
Copy link
Contributor

chaadow commented May 7, 2024

@yahonda @taketo1113 I'm on Rails 7-1 stable. and this makes my production app crash under certain circumstances.
image

when #save is called on an activerecord model. here is the stack trace:
image

@chaadow
Copy link
Contributor

chaadow commented May 7, 2024

Honestly I wish I could reproduce the bug (it's in production) but it's so mystical. all I can say is, it's related to this PR and it was done on a "ActiveRecord#save" call. on an unpersisted instance

(probably the reason why old_value.prefix is nil ?)

but what's mystical to me is why it goes through the changed? call here even though I have 0 CIDR columns

@chaadow
Copy link
Contributor

chaadow commented May 7, 2024

@taketo1113 @yahonda I finally managed to recreate the issue

It's due to the fact that inet inherits from CIDR.

this part helped me get less confused and better investigate and reproduce the issue

# frozen_string_literal: true

require "bundler/inline"

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

  # If you want to test against edge Rails replace the previous line with this:
  gem "rails", github: "rails/rails", branch: "main"

  gem "pg"
end

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

db_config = {
  adapter: 'postgresql',
  database: 'inet_cidr_test',
  host: 'localhost', # Change this to your PostgreSQL host
  port: 5432,
  username: 'postgres', # Change this to your PostgreSQL username
  password: 'postgres'  # Change this to your PostgreSQL password
}

ActiveRecord::Base.establish_connection(db_config.except(:database))
ActiveRecord::Base.connection.drop_database(db_config[:database]) rescue nil
ActiveRecord::Base.connection.create_database(db_config[:database])

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

ActiveRecord::Schema.define do
  create_table :users, force: true do |t|
    t.inet :ip_address
  end
end

class User < ActiveRecord::Base
end


class BugTest < Minitest::Test
  def test_ip_address_with_empty_string
    User.create!(ip_address: '')
  end
end

@carlosantoniodasilva
Copy link
Member

@chaadow if you haven't already, please open up a separate/new issue to help track this.

@taketo1113
Copy link
Contributor Author

taketo1113 commented May 7, 2024

I confirmed to reproduce issue and created PR (#51758)

yahonda added a commit to yahonda/rails that referenced this pull request May 13, 2024
Follow up rails#51633

Co-authored-by: Taketo Takashima <t.taketo1113@gmail.com>
rafaelfranca pushed a commit to yahonda/rails that referenced this pull request May 14, 2024
Follow up rails#51633

Co-authored-by: Taketo Takashima <t.taketo1113@gmail.com>
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.

[ActiveRecord] PostgreSQL Adapter skips update on CIDR column when only netmask is changed
6 participants