byroot / activerecord-typedstore

ActiveRecord::Store but with type definition
MIT License
437 stars 57 forks source link

typed_store attributes is marked as changed when inspecting instance #58

Closed adis-io closed 1 year ago

adis-io commented 6 years ago

Steps to reproduce

  1. save snippet below, name it like json_typed_store_bug.rb
  2. ruby json_typed_store_bug.rb
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'

  gem 'rails', '4.2.7.1'
  gem 'pg', '0.19.0'
  gem 'factory_bot_rails'
  gem 'activerecord-typedstore'
end

require 'active_record'
require 'action_controller/railtie'

ActiveRecord::Base.establish_connection(
  adapter: 'postgresql', database: 'ovkovckm',
  host: 'baasu.db.elephantsql.com', username: 'ovkovckm',
  password: '866lKeR6FSUgG_fEfo1e13XpYu0zZ-Iz'
)
ActiveRecord::Base.logger = Logger.new(STDOUT)

ActiveRecord::Schema.define do
  create_table :users, force: true do |t|
    t.string :username
    t.json :extra_attrs, default: {}, null: false
    t.timestamps
  end
end

class User < ActiveRecord::Base
  typed_store :extra_attrs do |s|
    s.string :first_name
    s.string :last_name
  end
end

FactoryBot.define do
  factory :user do
    username { 'username' }
  end
end

require 'minitest/autorun'

class TypedStoreTest < Minitest::Test
  def test_primary_categories
    FactoryBot.create(:user)

    user = User.first
    assert_equal({}, user.changed_attributes)

    user.inspect

    assert_equal({}, user.changed_attributes)
  end
end

Expected behaviour

tests passes

Actual behaviour

tests fails

adis-io commented 6 years ago

I didn't know how to reproduce without user interaction. I will appreciate if somebody will give a hint.

rafaelfranca commented 6 years ago

Try changing biding.pry with user.inspect

adis-io commented 6 years ago

Thank you, @rafaelfranca

So when I type user it inspects object?

rafaelfranca commented 6 years ago

Yes

adis-io commented 6 years ago
describe 'dirty tracking' do
  it 'is not dirty when freshly initialized' do
    model = described_class.new
    model.inspect
    expect(model.changed?).to be_falsey
  end
end

Test case not passing by now for models which use typed_store.

I think model should not be marked as dirty when you inspect it.

What do you think, guys? @rafaelfranca @byroot

gaffneyc commented 5 years ago

Just ran into a version of this where a freshly loaded model is reporting as changed.

class Dummy < ActiveRecord::Base
  typed_store :fields, coder: ::ActiveRecord::TypedStore::IdentityCoder do |s|
    s.string :email
  end
end

d = Dummy.last
d.changed? # => true

before = d.attribute_in_database(:fields)
after = d.fields
before == after # => true
edward commented 4 years ago

@gaffneyc am also running into a similar issue, though in my case, the ActiveRecord instance is only marked as dirty once I access any typed_store field.

I believe the problem stems from how ActiveModel::Attribute works:

d.send(:mutations_from_database).send(:attributes)["email"].send(:original_value_for_database) will return the serialized version of the field, which will always be different from the “apparent”, deserialized version of that field, which ActiveModel recognizes as changed_in_place? == true

I’ll investigate if we can sidestep this somehow.

gilbert commented 4 years ago

This behavior is conflicting with activerecord's locking. To extend @gaffneyc's example, if you try to run this:

d = Dummy.last
d.with_lock do
  # Doesn't matter what's in here
end

then ActiveRecord will throw an error:

RuntimeError: Locking a record with unpersisted changes is not supported. Use `save` to persist the changes, or `reload` to discard them explicitly.
mountriv99 commented 3 years ago

+1

bartekadams commented 3 years ago

+1