Skip to content

Commit

Permalink
Fix belongs_to association validation for virtual attributes
Browse files Browse the repository at this point in the history
Previously, `belongs_to` associations in `ActiveType::Object` did not properly
validate presence when `ActiveRecord::Base.belongs_to_required_by_default = true`
and `ActiveRecord.belongs_to_required_validates_foreign_key = false` (Rails default).

```
require "bundler/inline"

gemfile do
  source "https://rubygems.org"
  gem "activerecord"
  gem "sqlite3"
  gem "active_type"
end

require "active_record"
require "sqlite3"
require "active_type"

ActiveRecord::Base.belongs_to_required_by_default = true
ActiveRecord.belongs_to_required_validates_foreign_key = false

ActiveRecord::Base.establish_connection(adapter: "sqlite3", database: ":memory:")

ActiveRecord::Schema.define do
  create_table :shops do |t|
  end

  create_table :items do |t|
    t.references :shop, null: false, foreign_key: true
  end
end

class Shop < ActiveRecord::Base
end

class Item < ActiveRecord::Base
  belongs_to :shop
end

class X < ActiveType::Object
  attribute :shop_id, :integer
  belongs_to :shop
end

p Item.new.valid?              #=> false
p Item.new(shop_id: -1).valid? #=> false
p X.new.valid?                 #=> false
p X.new(shop_id: -1).valid?    #=> true
```

This is because the `validates_presence_of` validation uses
`ActiveType::Object#attribute_changed?` and it returns always `false` for
virtual attributes.

```
p X.new(shop_id: 1).attribute_changed?(:shop_id) #=> false
```

https://github.com/rails/rails/blob/v8.0.1/activerecord/lib/active_record/associations/builder/belongs_to.rb#L135

This commit implements `ActiveType::VirtualAttributes#attribute_changed?` and fixes the problem.

The same applies to `ActiveType::Record`.
  • Loading branch information
kyanagi committed Jan 20, 2025
1 parent cbb28ed commit f1687ed
Show file tree
Hide file tree
Showing 5 changed files with 204 additions and 12 deletions.
9 changes: 9 additions & 0 deletions lib/active_type/virtual_attributes.rb
Original file line number Diff line number Diff line change
Expand Up @@ -239,6 +239,15 @@ def changes
super.merge(changes)
end

def attribute_changed?(attr, **)
attr = attr.to_s
if virtual_attributes.key?(attr)
virtual_attributes_were[attr] != send(attr)
else
super
end
end

if ActiveRecord::VERSION::MAJOR >= 4
def changes_applied
super
Expand Down
66 changes: 60 additions & 6 deletions spec/active_type/object_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -91,14 +91,46 @@ def after_rollback_callback
class Child < ActiveRecord::Base
end

class ObjectWithBelongsTo < Object
class ObjectWithRequiredBelongsTo < Object

attribute :child_id, :integer

belongs_to :child
belongs_to :child, optional: false

end

class ObjectWithOptionalBelongsTo < Object

attribute :child_id, :integer

belongs_to :child, optional: true

end

if ActiveRecord::VERSION::STRING >= '7.1.0'
ActiveRecord.belongs_to_required_validates_foreign_key = !ActiveRecord.belongs_to_required_validates_foreign_key

class ObjectWithRequiredBelongsToFlippedValidatesForeignKey < Object
BELONGS_TO_REQUIRED_VALIDATES_FOREIGN_KEY = ActiveRecord.belongs_to_required_validates_foreign_key

attribute :child_id, :integer

belongs_to :child, optional: false

end

class ObjectWithOptionalBelongsToFlippedValidatesForeignKey < Object
BELONGS_TO_REQUIRED_VALIDATES_FOREIGN_KEY = ActiveRecord.belongs_to_required_validates_foreign_key

attribute :child_id, :integer

belongs_to :child, optional: true

end

ActiveRecord.belongs_to_required_validates_foreign_key = !ActiveRecord.belongs_to_required_validates_foreign_key
end

class ObjectWithUnsupportedTypes < Object
attribute :virtual_array, :array
attribute :virtual_hash, :hash
Expand Down Expand Up @@ -327,7 +359,7 @@ class ObjectWithUnsupportedTypes < Object
it 'has 1 error_on' do
expect(subject.error_on(:virtual_string).size).to eq(1)
end

it 'validates the presence of boolean values' do
subject.virtual_boolean = false
expect(subject.error_on(:virtual_boolean).size).to eq(1)
Expand Down Expand Up @@ -369,10 +401,32 @@ class ObjectWithUnsupportedTypes < Object
it_should_behave_like 'a class supporting dirty tracking for virtual attributes', ActiveType::Object
end

describe '#belongs_to' do
subject { ObjectSpec::ObjectWithBelongsTo.new }
describe '#belongs_to, optional: false' do
subject { ObjectSpec::ObjectWithRequiredBelongsTo.new }

it_should_behave_like 'a required belongs_to association', :child, ObjectSpec::Child
end

describe '#belongs_to, optional: true' do
subject { ObjectSpec::ObjectWithOptionalBelongsTo.new }

it_should_behave_like 'a belongs_to association', :child, ObjectSpec::Child
it_should_behave_like 'an optional belongs_to association', :child, ObjectSpec::Child
end

if ActiveRecord::VERSION::STRING >= '7.1.0'
v = ObjectSpec::ObjectWithRequiredBelongsToFlippedValidatesForeignKey::BELONGS_TO_REQUIRED_VALIDATES_FOREIGN_KEY
describe "#belongs_to, optional: false, belongs_to_required_validates_foreign_key: #{v}" do
subject { ObjectSpec::ObjectWithRequiredBelongsToFlippedValidatesForeignKey.new }

it_should_behave_like 'a required belongs_to association', :child, ObjectSpec::Child
end

v = ObjectSpec::ObjectWithOptionalBelongsToFlippedValidatesForeignKey::BELONGS_TO_REQUIRED_VALIDATES_FOREIGN_KEY
describe "#belongs_to, optional: true, belongs_to_required_validates_foreign_key: #{v}" do
subject { ObjectSpec::ObjectWithOptionalBelongsToFlippedValidatesForeignKey.new }

it_should_behave_like 'an optional belongs_to association', :child, ObjectSpec::Child
end
end

describe '#save' do
Expand Down
75 changes: 70 additions & 5 deletions spec/active_type/record_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -52,13 +52,45 @@ class OtherRecord < ActiveType::Record
class Child < ActiveRecord::Base
end

class RecordWithBelongsTo < Record
class RecordWithRequiredBelongsTo < Record

attribute :child_id, :integer

belongs_to :child
belongs_to :child, optional: false

end

class RecordWithOptionalBelongsTo < Record

attribute :child_id, :integer

belongs_to :child, optional: true

end

if ActiveRecord::VERSION::STRING >= '7.1.0'
ActiveRecord.belongs_to_required_validates_foreign_key = !ActiveRecord.belongs_to_required_validates_foreign_key

class RecordWithRequiredBelongsToFlippedValidatesForeignKey < Record
BELONGS_TO_REQUIRED_VALIDATES_FOREIGN_KEY = ActiveRecord.belongs_to_required_validates_foreign_key

attribute :child_id, :integer

belongs_to :child, optional: false

end

class RecordWithOptionalBelongsToFlippedValidatesForeignKey < Record
BELONGS_TO_REQUIRED_VALIDATES_FOREIGN_KEY = ActiveRecord.belongs_to_required_validates_foreign_key

attribute :child_id, :integer

belongs_to :child, optional: true

end

ActiveRecord.belongs_to_required_validates_foreign_key = !ActiveRecord.belongs_to_required_validates_foreign_key
end
end


Expand Down Expand Up @@ -239,6 +271,17 @@ class RecordWithBelongsTo < Record
it_should_behave_like 'a class supporting dirty tracking for virtual attributes', RecordSpec::Record
end

describe '#attribute_changed?' do
it "returns true for persisted attributes if changed" do
subject.persisted_string = "persisted string"
expect(subject.attribute_changed?(:persisted_string)).to eq(true)
end

it "returns true for persisted attributes if not changed" do
expect(subject.attribute_changed?(:persisted_string)).to eq(false)
end
end

describe 'persistence' do

it 'persists to the database' do
Expand All @@ -259,10 +302,32 @@ class RecordWithBelongsTo < Record
end
end

describe '#belongs_to' do
subject { RecordSpec::RecordWithBelongsTo.new }
describe '#belongs_to, optional: false' do
subject { RecordSpec::RecordWithRequiredBelongsTo.new }

it_should_behave_like 'a required belongs_to association', :child, RecordSpec::Child
end

it_should_behave_like 'a belongs_to association', :child, RecordSpec::Child
describe '#belongs_to, optional: true' do
subject { RecordSpec::RecordWithOptionalBelongsTo.new }

it_should_behave_like 'an optional belongs_to association', :child, RecordSpec::Child
end

if ActiveRecord::VERSION::STRING >= '7.1.0'
v = RecordSpec::RecordWithRequiredBelongsToFlippedValidatesForeignKey::BELONGS_TO_REQUIRED_VALIDATES_FOREIGN_KEY
describe "#belongs_to, optional: false, belongs_to_required_validates_foreign_key: #{v}" do
subject { RecordSpec::RecordWithRequiredBelongsToFlippedValidatesForeignKey.new }

it_should_behave_like 'a required belongs_to association', :child, RecordSpec::Child
end

v = RecordSpec::RecordWithOptionalBelongsToFlippedValidatesForeignKey::BELONGS_TO_REQUIRED_VALIDATES_FOREIGN_KEY
describe "#belongs_to, optional: true, belongs_to_required_validates_foreign_key: #{v}" do
subject { RecordSpec::RecordWithOptionalBelongsToFlippedValidatesForeignKey.new }

it_should_behave_like 'an optional belongs_to association', :child, RecordSpec::Child
end
end

it 'can access virtual attributes after .find' do
Expand Down
42 changes: 41 additions & 1 deletion spec/shared_examples/belongs_to.rb
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
shared_examples_for 'a belongs_to association' do |association, klass|
shared_examples_for 'a required belongs_to association' do |association, klass|

let(:record) { klass.create }

Expand All @@ -14,4 +14,44 @@
expect(subject.send("#{association}")).to eq(record)
end

it 'is invalid if the associated record is not found' do
subject.send("#{association}_id=", -1)

expect(subject).to be_invalid
end

it 'is invalid if the assigned id is nil' do
subject.send("#{association}_id=", nil)

expect(subject).to be_invalid
end
end

shared_examples_for 'an optional belongs_to association' do |association, klass|

let(:record) { klass.create }

it 'sets the id when assigning a record' do
subject.send("#{association}=", record)

expect(subject.send("#{association}_id")).to eq(record.id)
end

it 'sets the record when assigning an id' do
subject.send("#{association}_id=", record.id)

expect(subject.send("#{association}")).to eq(record)
end

it 'is valid even if the associated record is not found' do
subject.send("#{association}_id=", -1)

expect(subject).to be_valid
end

it 'is valid even if the assigned id is nil' do
subject.send("#{association}_id=", nil)

expect(subject).to be_valid
end
end
24 changes: 24 additions & 0 deletions spec/shared_examples/dirty_tracking.rb
Original file line number Diff line number Diff line change
Expand Up @@ -103,4 +103,28 @@
end

end

describe '#attribute_changed?' do
it 'returns true if specified attribute is not nil' do
subject.virtual_attribute = 'foo'
expect(subject.attribute_changed?(:virtual_attribute)).to eq(true)
end

it 'returns false if specified attribute is nil' do
subject.virtual_attribute = nil
expect(subject.attribute_changed?(:virtual_attribute)).to eq(false)
end

context 'after applying changes' do
it 'returns false' do
subject.virtual_attribute = 'foo'
subject.changes_applied
expect(subject.attribute_changed?(:virtual_attribute)).to eq(false)
end
end

it 'returns false if specified attribute does not exist' do
expect(subject.attribute_changed?(:not_exist)).to eq(false)
end
end
end

0 comments on commit f1687ed

Please sign in to comment.