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

Regression in PostgreSQL HStore dirty check when key order changes #27502

Closed
cupakromer opened this issue Dec 29, 2016 · 4 comments
Closed

Regression in PostgreSQL HStore dirty check when key order changes #27502

cupakromer opened this issue Dec 29, 2016 · 4 comments

Comments

@cupakromer
Copy link
Contributor

cupakromer commented Dec 29, 2016

Steps to reproduce

This passes on Rails 4.2.7.1 but fails on Rails 5.0.1:

begin
  require "bundler/inline"
rescue LoadError => e
  $stderr.puts "Bundler version 1.10 or later is required. Please update your Bundler"
  raise e
end

gemfile(true) do
  source "https://rubygems.org"
  # Activate the gem you are reporting the issue against.
  gem "activerecord", "5.0.1"       # Fails
  #gem "activerecord", "4.2.7.1"    # Passes
  gem "pg"
end

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

# PostgreSQL Specific Issue
ActiveRecord::Base.establish_connection(adapter: 'postgresql', database: 'hstore_dirty')
ActiveRecord::Base.logger = Logger.new(STDOUT)

ActiveRecord::Schema.define do
  enable_extension 'hstore' unless extension_enabled?('hstore')
  create_table :posts, force: true do |t|
    t.hstore 'settings'
  end
end

class Post < ActiveRecord::Base
end

class BugTest < Minitest::Test
  def setup
    @settings = { "alongkey" => "anything", "key" => "value" }
    @post = Post.create!(settings: @settings)
  end

  def test_hstore_dirty_from_user_no_change
    # @post.settings is: {"alongkey"=>"anything", "key"=>"value"}
    assert_equal @settings, @post.settings
    @post.settings = @settings
    refute @post.changed?
  end

  def test_hstore_dirty_from_user_no_equality_change
    # @post.settings is: {"alongkey"=>"anything", "key"=>"value"}
    assert_equal @settings, @post.settings
    @post.settings = { "key" => "value", "alongkey" => "anything" }
    assert_equal @settings, @post.settings
    refute @post.changed?
  end

  def test_hstore_dirty_from_database
    @post.reload
    # The value of @post.settings is dependent on how the database decides to
    # return the HStore. For my system on PostgreSQL 9.5.4 it seems to place
    # shorter keys before longer ones.
    #
    # At this point @post.settings is: {"key"=>"value", "alongkey"=>"anything"}
    assert_equal @settings, @post.settings
    @post.settings = @settings
    refute @post.changed?
  end
end

Expected behavior

The record has no changes and should not be saved or marked as dirty.

Actual behavior

The record thinks the HStore has changed. This is causing it to be saved resulting in a cascade of effects which should be avoided (e.g. updates the updated_at timestamp, causing touch association updates, busting caches and ETags).

System configuration

Rails version: 5.0.1

Ruby version: 2.3.3

PostgreSQL version: 9.5.4

@maclover7
Copy link
Contributor

Thank you for reporting @cupakromer! Investigating...

@maclover7
Copy link
Contributor

Looks like test_hstore_dirty_from_user_no_equality_change and test_hstore_dirty_from_database both started failing via 8e633e5. Investigating a solution.

cc @sgrif

@cupakromer
Copy link
Contributor Author

From what I could tell the root of the problem seemed to be when it did the changed in place check. When the attribute is FromDatabase the value before type cast is a string, which obviously is positional, so that check fails thus it thinks something changed. Given that I thought maybe if it had a hash value instead it would fix things. Unfortunately, I wasn't able to work out how the values were setup in the adapter code so that didn't go anywhere.

Tangentially related to this new use of attributes for changes. I noticed that both the changes and previous_changes hashes are no longer mutable (but they are for ActiveModel). Still trying to work through how to handle that.

@maclover7
Copy link
Contributor

Opened #27517 to try and solve this.

maclover7 added a commit to maclover7/rails that referenced this issue Jan 3, 2017
Per the regression commit below, the commit changes the behavior of
`#changed?`to consult the `#changed_in_place?` method on `Type::Value` classes.
Per this change, `PostgreSQL::OID::Hstore` needs to override this method
in order to compare the deserialized forms of the two arguments. In
Ruby, two hashes are considered equal even if their key order is
different. This commit helps to bring that behavior to `Hstore` values.

Fixes regression introduced by 8e633e5

Fixes rails#27502
maclover7 added a commit to maclover7/rails that referenced this issue Jan 3, 2017
Per the regression commit below, the commit changes the behavior of
`#changed?`to consult the `#changed_in_place?` method on `Type::Value` classes.
Per this change, `PostgreSQL::OID::Hstore` needs to override this method
in order to compare the deserialized forms of the two arguments. In
Ruby, two hashes are considered equal even if their key order is
different. This commit helps to bring that behavior to `Hstore` values.

Fixes regression introduced by 8e633e5

Fixes rails#27502
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