ActiveRecord Hstore bug: can't update a key in the hash #6127

Closed
joevandyk opened this Issue May 2, 2012 · 27 comments

Comments

Projects
None yet
@joevandyk
Contributor

joevandyk commented May 2, 2012

joevandyk@f2318b2 shows a failing test.

  def test_updating_key                                                                             
    x = Hstore.create! :tags => { "key1" => "old value 1", "key2" => "old value 2" }                
    x.reload                                                                                        
    assert_equal "old value 1", x.tags["key1"]                                                      

    # Nothing gets saved/updated here.
    x.tags["key1"] = "new"                                                                          
    x.save!                                                                                         

    assert_equal "new", x.reload.tags["key1"]                                                       
    assert_equal "old value 2",   x.reload.tags["key2"]                                             
  end
  1) Failure:
test_updating_key(PostgresqlHstoreTest) [cases/adapters/postgresql/hstore_test.rb:55]:
Expected: "new"
  Actual: "old value 1"
@kennyj

This comment has been minimized.

Show comment
Hide comment
@kennyj

kennyj May 22, 2012

Contributor

I can reproduce this issue.
It seems that any query weren't executed when executing save!
I guess:
x.tags method returns a Hash object, and we can't track dirty flag.

Contributor

kennyj commented May 22, 2012

I can reproduce this issue.
It seems that any query weren't executed when executing save!
I guess:
x.tags method returns a Hash object, and we can't track dirty flag.

@gertig

This comment has been minimized.

Show comment
Hide comment
@gertig

gertig Sep 26, 2012

I can also reproduce this issue.

gertig commented Sep 26, 2012

I can also reproduce this issue.

@al2o3cr

This comment has been minimized.

Show comment
Hide comment
@al2o3cr

al2o3cr Sep 27, 2012

Contributor

A quick workaround would be to call (in your example) x.tags_will_change! before setting the new value.

Historically, this has always been a problem with serialize columns, and AR's approach in that case is to always save serialized columns. HStore columns might need the same treatment.

Contributor

al2o3cr commented Sep 27, 2012

A quick workaround would be to call (in your example) x.tags_will_change! before setting the new value.

Historically, this has always been a problem with serialize columns, and AR's approach in that case is to always save serialized columns. HStore columns might need the same treatment.

@tenderlove

This comment has been minimized.

Show comment
Hide comment
@tenderlove

tenderlove Sep 27, 2012

Member

@al2o3cr ya, I've been looking at this. I'd like to treat the hstore columns as serialized columns, but it's going to take a little refactoring.

Member

tenderlove commented Sep 27, 2012

@al2o3cr ya, I've been looking at this. I'd like to treat the hstore columns as serialized columns, but it's going to take a little refactoring.

@parndt

This comment has been minimized.

Show comment
Hide comment
@parndt

parndt Dec 4, 2012

Contributor

@tenderlove has there been any refactoring to date that would have helped this?

Contributor

parndt commented Dec 4, 2012

@tenderlove has there been any refactoring to date that would have helped this?

@neerajdotname

This comment has been minimized.

Show comment
Hide comment
@neerajdotname

neerajdotname Apr 25, 2013

Member

Here is another version and this one does not break the API. It is based on feedback given by @pixeltrix

https://github.com/neerajdotname/rails/compare/6127b

I'll clean up the code a bit , add tests and will delete some code that is not needed regarding hstore etc if there is any interest in moving hstore to be on top of serialization.

Member

neerajdotname commented Apr 25, 2013

Here is another version and this one does not break the API. It is based on feedback given by @pixeltrix

https://github.com/neerajdotname/rails/compare/6127b

I'll clean up the code a bit , add tests and will delete some code that is not needed regarding hstore etc if there is any interest in moving hstore to be on top of serialization.

@jaykenney

This comment has been minimized.

Show comment
Hide comment
@jaykenney

jaykenney May 11, 2013

@neerjdotname Forgive me if this seems naive, but wouldn't it make more sense to specify using hstore in the database.yml (serialize using hstore), what would happen if that model was using sqlite instead?

@neerjdotname Forgive me if this seems naive, but wouldn't it make more sense to specify using hstore in the database.yml (serialize using hstore), what would happen if that model was using sqlite instead?

@neerajdotname

This comment has been minimized.

Show comment
Hide comment
@neerajdotname

neerajdotname May 11, 2013

Member

@bloodycelt sqlite does not support hstore. So if you are using sqlite you should not add this line in your model .

serialize :preferences, ActiveRecord::Coders::HstoreColumn.new
Member

neerajdotname commented May 11, 2013

@bloodycelt sqlite does not support hstore. So if you are using sqlite you should not add this line in your model .

serialize :preferences, ActiveRecord::Coders::HstoreColumn.new
@prathamesh-sonpatki

This comment has been minimized.

Show comment
Hide comment
@prathamesh-sonpatki

prathamesh-sonpatki Aug 15, 2013

Member

Bump. Any updates on this?

Bump. Any updates on this?

@robin850

This comment has been minimized.

Show comment
Hide comment
@robin850

robin850 Oct 13, 2013

Member

Isn't it related to #12395 ?

Member

robin850 commented Oct 13, 2013

Isn't it related to #12395 ?

@peterc

This comment has been minimized.

Show comment
Hide comment
@peterc

peterc Dec 19, 2013

Contributor

Pushed back another point release?

Contributor

peterc commented Dec 19, 2013

Pushed back another point release?

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Dec 19, 2013

Member

Yes. 😞

Member

rafaelfranca commented Dec 19, 2013

Yes. 😞

@wtn

This comment has been minimized.

Show comment
Hide comment
@wtn

wtn Jan 26, 2014

For a quick fix, I added #attr_name_will_change! before each change to hstore attributes in my application.

wtn commented Jan 26, 2014

For a quick fix, I added #attr_name_will_change! before each change to hstore attributes in my application.

@senny senny added the PostgreSQL label Mar 26, 2014

@etagwerker

This comment has been minimized.

Show comment
Hide comment
@etagwerker

etagwerker Mar 28, 2014

Bump. I just reproduced this bug in 4.0.4

I tried using #attr_name_will_change! before each change but it's not working for me.

Bump. I just reproduced this bug in 4.0.4

I tried using #attr_name_will_change! before each change but it's not working for me.

@gdeglin

This comment has been minimized.

Show comment
Hide comment
@gdeglin

gdeglin Apr 4, 2014

Contributor

Yikes, this is a nasty one for anyone using hstore. Just got bit by this...

Example of problematic code

def example
  player = Player.last
  player.tags["foo"] = "bar"
  puts "Initial: #{player.tags['foo']}"       # "bar"
  player.save
  puts "Reload: #{player.reload.tags['foo']}" # null (???)
  player.tags["foo"] = "bar"
  player.tags_will_change! # Workaround...
  player.save
  puts "Now?: #{player.reload.tags['foo']}" # "bar"
end
example

Output:

Initial: bar
Reload: 
Now?: bar

Version info:

ruby 2.1.0p0 (2013-12-25 revision 44422) [x86_64-darwin13.0]
Rails 4.0.4

Column info for "tags":

 tags               | hstore                      | not null default ''::hstore
Contributor

gdeglin commented Apr 4, 2014

Yikes, this is a nasty one for anyone using hstore. Just got bit by this...

Example of problematic code

def example
  player = Player.last
  player.tags["foo"] = "bar"
  puts "Initial: #{player.tags['foo']}"       # "bar"
  player.save
  puts "Reload: #{player.reload.tags['foo']}" # null (???)
  player.tags["foo"] = "bar"
  player.tags_will_change! # Workaround...
  player.save
  puts "Now?: #{player.reload.tags['foo']}" # "bar"
end
example

Output:

Initial: bar
Reload: 
Now?: bar

Version info:

ruby 2.1.0p0 (2013-12-25 revision 44422) [x86_64-darwin13.0]
Rails 4.0.4

Column info for "tags":

 tags               | hstore                      | not null default ''::hstore
@al2o3cr

This comment has been minimized.

Show comment
Hide comment
@al2o3cr

al2o3cr Apr 4, 2014

Contributor

@gdeglin - I'd recommend one change to the above: player.tags_will_change! should be before the assignment. That's why it's "will" and not "did" change :)

Contributor

al2o3cr commented Apr 4, 2014

@gdeglin - I'd recommend one change to the above: player.tags_will_change! should be before the assignment. That's why it's "will" and not "did" change :)

@fabiokr

This comment has been minimized.

Show comment
Hide comment
@fabiokr

fabiokr Apr 4, 2014

Contributor

This is a monkey patch I have been using to make that a default for hstores and arrays:

module Ext::ActiveRecord::Dirty
  private

  # Private: Overwrites the default implementation to include arrays and hstores
  # in the list of attributes that should always be saved.
  #
  # Without this, arrays and stores don't get their values updated in
  # the database.
  #
  # Relates to this issue: https://github.com/rails/rails/issues/6127
  def keys_for_partial_write
    super | self.class.columns.select do |c|
      c.try(:array) || c.type == :hstore
    end.map(&:name)
  end
end

ActiveSupport.on_load :active_record do
  include Ext::ActiveRecord::Dirty
end
Contributor

fabiokr commented Apr 4, 2014

This is a monkey patch I have been using to make that a default for hstores and arrays:

module Ext::ActiveRecord::Dirty
  private

  # Private: Overwrites the default implementation to include arrays and hstores
  # in the list of attributes that should always be saved.
  #
  # Without this, arrays and stores don't get their values updated in
  # the database.
  #
  # Relates to this issue: https://github.com/rails/rails/issues/6127
  def keys_for_partial_write
    super | self.class.columns.select do |c|
      c.try(:array) || c.type == :hstore
    end.map(&:name)
  end
end

ActiveSupport.on_load :active_record do
  include Ext::ActiveRecord::Dirty
end
@kylase

This comment has been minimized.

Show comment
Hide comment
@kylase

kylase Apr 5, 2014

@gdeglin I have just tested with @al2o3cr suggestion and you should execute tags_will_change! first then set tags.

kylase commented Apr 5, 2014

@gdeglin I have just tested with @al2o3cr suggestion and you should execute tags_will_change! first then set tags.

@chadwtaylor

This comment has been minimized.

Show comment
Hide comment
@chadwtaylor

chadwtaylor Apr 26, 2014

+1 This is an issue for me too!

+1 This is an issue for me too!

@senny

This comment has been minimized.

Show comment
Hide comment
@senny

senny May 12, 2014

Member

This is the current expected behavior. You need to call XYZ_will_change! when modifying hstore or json attributes in place. It is related to the discussion on serialized attributes (they are always saved even when they don't change). A fix will benefit both scenarios.

@joevandyk thank you for reporting.

Member

senny commented May 12, 2014

This is the current expected behavior. You need to call XYZ_will_change! when modifying hstore or json attributes in place. It is related to the discussion on serialized attributes (they are always saved even when they don't change). A fix will benefit both scenarios.

@joevandyk thank you for reporting.

@senny senny closed this May 12, 2014

@saurabhnanda

This comment has been minimized.

Show comment
Hide comment
@saurabhnanda

saurabhnanda May 27, 2014

Just faced this. What is the final call:
(a) Fix serialized columns -- should be saved only when changed, OR
(b) Make sure Hstore column is always saved, whether changed or not?

Just faced this. What is the final call:
(a) Fix serialized columns -- should be saved only when changed, OR
(b) Make sure Hstore column is always saved, whether changed or not?

@saurabhnanda

This comment has been minimized.

Show comment
Hide comment
@saurabhnanda

saurabhnanda May 27, 2014

Btw, in Postgres, hstore allows you to change the column partially, eg.

UPDATE mytable SET properties = (properties - 'deleted_key') || hstore('added_key', 'added_value')

Btw, in Postgres, hstore allows you to change the column partially, eg.

UPDATE mytable SET properties = (properties - 'deleted_key') || hstore('added_key', 'added_value')

@senny

This comment has been minimized.

Show comment
Hide comment
@senny

senny May 27, 2014

Member

@saurabhnanda we keep the current behavior until we have a working solution for serialized attributes.

Member

senny commented May 27, 2014

@saurabhnanda we keep the current behavior until we have a working solution for serialized attributes.

@chibicode

This comment has been minimized.

Show comment
Hide comment
@chibicode

chibicode Jun 3, 2014

Came here from Stack Overflow: http://stackoverflow.com/questions/20251296/how-can-i-update-a-data-records-value-with-ruby-on-rails-4-0-1-postgresql-hstor

I'm fine with the current behavior but I wish this was documented better. Does Rails 4 have a documentation page on Hstore related stuff?

Came here from Stack Overflow: http://stackoverflow.com/questions/20251296/how-can-i-update-a-data-records-value-with-ruby-on-rails-4-0-1-postgresql-hstor

I'm fine with the current behavior but I wish this was documented better. Does Rails 4 have a documentation page on Hstore related stuff?

@senny

This comment has been minimized.

Show comment
Hide comment
@senny

senny Jun 4, 2014

Member

@chibicode I'm working on a PostgreSQL specific guide. See my current draft it does go into the details and will be updated in the future.

We are also working on resolving the situation with serialized attributes. Once we finished that, we can use that same behavior for json and hstore columns.

Member

senny commented Jun 4, 2014

@chibicode I'm working on a PostgreSQL specific guide. See my current draft it does go into the details and will be updated in the future.

We are also working on resolving the situation with serialized attributes. Once we finished that, we can use that same behavior for json and hstore columns.

@chibicode

This comment has been minimized.

Show comment
Hide comment
@chibicode

chibicode Jun 4, 2014

@senny wow, that's very useful. Thank you!

@senny wow, that's very useful. Thank you!

@tilsammans

This comment has been minimized.

Show comment
Hide comment
@tilsammans

tilsammans Jun 22, 2014

Contributor

For what it's worth, this behavior has been fixed as of #15674 and will be part of 4.2.

Contributor

tilsammans commented Jun 22, 2014

For what it's worth, this behavior has been fixed as of #15674 and will be part of 4.2.

@rails rails locked and limited conversation to collaborators Jun 22, 2014

joallard added a commit to joallard/trasto that referenced this issue Jan 24, 2015

joallard added a commit to joallard/trasto that referenced this issue Jan 25, 2015

joallard added a commit to joallard/trasto that referenced this issue Jan 25, 2015

joallard added a commit to joallard/trasto that referenced this issue Jan 25, 2015

joallard added a commit to joallard/trasto that referenced this issue Apr 3, 2015

ramontayag added a commit to G5/storext that referenced this issue Apr 9, 2015

Fix issue when deleting keys.
If Rails < 4.2, then handle it ourselves by duping the hash. If Rails >= 4.2,
then rely on the Rails fix:

- rails/rails#15674
- rails/rails#6127
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.