From 0a1f0893647cef1658a8ac3a96da4e2759576a5a Mon Sep 17 00:00:00 2001 From: Geremia Taglialatela Date: Fri, 14 Oct 2022 23:22:47 +0200 Subject: [PATCH 1/4] Add failing spec for historical relations Ref: #181 --- spec/chrono_model/time_machine/history_spec.rb | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/spec/chrono_model/time_machine/history_spec.rb b/spec/chrono_model/time_machine/history_spec.rb index 1c12371a..6700d3c3 100644 --- a/spec/chrono_model/time_machine/history_spec.rb +++ b/spec/chrono_model/time_machine/history_spec.rb @@ -54,6 +54,18 @@ it { expect($t.foo.history.except(:order).select('max(id) as foo, min(id) as bar').group('id').first.attributes.keys).to match_array %w[id foo bar] } end + context 'when finding historical elements by hid' do + subject(:historical_foo) { Foo::History.find(last_foo.hid) } + + let(:last_foo) { Foo.history.last } + + it { is_expected.to eq last_foo } + + it 'preserves associations' do + expect(historical_foo.bars).to eq last_foo.bars + end + end + context '.sorted' do describe 'orders by recorded_at, hid' do it { expect($t.foo.history.sorted.to_sql).to match(/order by .+"recorded_at" ASC, .+"hid" ASC/i) } From d6baa7a8968685e1551ac5ce1d64e7186bdba79d Mon Sep 17 00:00:00 2001 From: Geremia Taglialatela Date: Sat, 15 Oct 2022 00:16:08 +0200 Subject: [PATCH 2/4] Overload read_attribute method Starting from Rails 6.0, read_attribute will use the memoized `primary_key` if it detects that the attribute name is `id`. Since the `primary key` may have been changed to `hid` in this case because of `find` overload, the new behavior may break relations where `id` is still the correct attribute to read for foreign key Fix: #181 --- lib/chrono_model/time_machine/history_model.rb | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/lib/chrono_model/time_machine/history_model.rb b/lib/chrono_model/time_machine/history_model.rb index b5845e5a..c969c207 100644 --- a/lib/chrono_model/time_machine/history_model.rb +++ b/lib/chrono_model/time_machine/history_model.rb @@ -221,6 +221,20 @@ def recorded_at ChronoModel::Conversions.string_to_utc_time attributes_before_type_cast['recorded_at'] end + # Starting from Rails 6.0, `.read_attribute` will use the memoized + # `primary_key` if it detects that the attribute name is `id`. + # + # Since the `primary key` may have been changed to `hid` because of + # `.find` overload, the new behavior may break relations where `id` is + # still the correct attribute to read + # + # Ref: ifad/chronomodel#181 + def read_attribute(attr_name, &block) + return super unless attr_name.to_s == 'id' && @primary_key.to_s == 'hid' + + _read_attribute('id', &block) + end + private def with_hid_pkey From 0f76b07fb2dc2cf8b35c6315c6f993667e924f08 Mon Sep 17 00:00:00 2001 From: Danilo Grieco Date: Thu, 10 Nov 2022 10:13:52 +0100 Subject: [PATCH 3/4] Added tests on custom foreign key --- .../chrono_model/time_machine/history_spec.rb | 6 +++++ spec/support/time_machine/structure.rb | 22 +++++++++++++++++++ 2 files changed, 28 insertions(+) diff --git a/spec/chrono_model/time_machine/history_spec.rb b/spec/chrono_model/time_machine/history_spec.rb index 6700d3c3..8157e015 100644 --- a/spec/chrono_model/time_machine/history_spec.rb +++ b/spec/chrono_model/time_machine/history_spec.rb @@ -58,12 +58,18 @@ subject(:historical_foo) { Foo::History.find(last_foo.hid) } let(:last_foo) { Foo.history.last } + let!(:tar1) { Tar.create(foo: last_foo)} it { is_expected.to eq last_foo } it 'preserves associations' do expect(historical_foo.bars).to eq last_foo.bars end + + it 'preserves associations that are using a different fkid' do + expect(historical_foo.tars).not_to be_empty + expect(historical_foo.tars).to eq(last_foo.tars) + end end context '.sorted' do diff --git a/spec/support/time_machine/structure.rb b/spec/support/time_machine/structure.rb index 376ec74f..7913f549 100644 --- a/spec/support/time_machine/structure.rb +++ b/spec/support/time_machine/structure.rb @@ -46,6 +46,18 @@ module ChronoTest::TimeMachine t.string :name t.integer :fooity t.references :goo + t.string :refee_foo + end + + adapter.create_table 'tars', temporal: true do |t| + t.string :name + t.string :foo_refering + end + + adapter.change_table 'tars', temporal: true do + def up + add_reference :foos, column: :foo_refering, primary_key: :refee_foo + end end adapter.create_table 'moos', temporal: true do |t| @@ -80,6 +92,15 @@ class ::Baz < ActiveRecord::Base has_timeline with: :bar end + class ::Tar < ActiveRecord::Base + include ChronoModel::TimeGate + + belongs_to :foo, foreign_key: :foo_refering, primary_key: :refee_foo, class_name: 'Foo' + + has_timeline with: :foo + end + + class ::Boo < ActiveRecord::Base include ChronoModel::TimeMachine @@ -90,6 +111,7 @@ class ::Foo < ActiveRecord::Base include ChronoModel::TimeMachine has_many :bars + has_many :tars, foreign_key: :foo_refering, primary_key: :refee_foo has_many :sub_bars, through: :bars has_many :sub_sub_bars, through: :sub_bars From 6f9aba27ae7f22570bde99cb46e9bc3f61ebfb12 Mon Sep 17 00:00:00 2001 From: Geremia Taglialatela Date: Sun, 27 Nov 2022 10:58:58 +0100 Subject: [PATCH 4/4] Do not use `let` if the object is not referenced --- spec/chrono_model/time_machine/history_spec.rb | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/spec/chrono_model/time_machine/history_spec.rb b/spec/chrono_model/time_machine/history_spec.rb index 8157e015..4ceb43a9 100644 --- a/spec/chrono_model/time_machine/history_spec.rb +++ b/spec/chrono_model/time_machine/history_spec.rb @@ -58,7 +58,10 @@ subject(:historical_foo) { Foo::History.find(last_foo.hid) } let(:last_foo) { Foo.history.last } - let!(:tar1) { Tar.create(foo: last_foo)} + + before do + Tar.create(foo: last_foo) + end it { is_expected.to eq last_foo }