diff --git a/CHANGELOG.md b/CHANGELOG.md index 86b656ef..24b2870e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,6 +26,9 @@ recommendations of [keepachangelog.com](http://keepachangelog.com/). Fix error calling private method in rails 4.0 - [#938](https://github.com/airblade/paper_trail/pull/938) - Fix bug where non-standard foreign key names broke belongs_to associations +- [#940](https://github.com/airblade/paper_trail/pull/940) - When destroying + versions to stay under version_limit, don't rely on the database to + implicitly return the versions in the right order ## 6.0.2 (2016-12-13) diff --git a/lib/paper_trail/version_concern.rb b/lib/paper_trail/version_concern.rb index e3510c81..7718eb37 100644 --- a/lib/paper_trail/version_concern.rb +++ b/lib/paper_trail/version_concern.rb @@ -312,7 +312,8 @@ module PaperTrail def enforce_version_limit! limit = PaperTrail.config.version_limit return unless limit.is_a? Numeric - previous_versions = sibling_versions.not_creates + previous_versions = sibling_versions.not_creates. + order(self.class.timestamp_sort_order("asc")) return unless previous_versions.size > limit excess_versions = previous_versions - previous_versions.last(limit) excess_versions.map(&:destroy) diff --git a/spec/paper_trail/version_limit_spec.rb b/spec/paper_trail/version_limit_spec.rb new file mode 100644 index 00000000..b0bce620 --- /dev/null +++ b/spec/paper_trail/version_limit_spec.rb @@ -0,0 +1,58 @@ +require "rails_helper" + +module PaperTrail + RSpec.describe Cleaner, versioning: true do + before do + @last_limit = PaperTrail.config.version_limit + end + after do + PaperTrail.config.version_limit = @last_limit + end + + it "cleans up old versions" do + PaperTrail.config.version_limit = 10 + widget = Widget.create + + 100.times do |i| + widget.update_attributes(name: "Name #{i}") + expect(Widget.find(widget.id).versions.count).to be <= 11 + # 11 versions = 10 updates + 1 create. + end + end + + it "deletes oldest versions, when the database returns them in a different order" do + epoch = DateTime.new(2017, 1, 1) + + widget = Timecop.freeze(epoch) { Widget.create } + + # Sometimes a database will returns records in a different order than + # they were inserted. That's hard to get the database to do, so we'll + # just create them out-of-order: + (1..5).to_a.shuffle.each do |n| + Timecop.freeze(epoch + n.hours) do + PaperTrail::Version.create!( + item: widget, + event: "update", + object: { "id" => widget.id, "name" => "Name #{n}" }.to_yaml + ) + end + end + expect(Widget.find(widget.id).versions.count).to eq(6) # 1 create + 5 updates + + # Now, we've recreated the scenario where we can accidentally clean up + # the wrong versions. Re-enable the version_limit, and trigger the + # clean-up: + PaperTrail.config.version_limit = 3 + widget.versions.last.send(:enforce_version_limit!) + + # Verify that we have fewer versions: + expect(widget.reload.versions.count).to eq(4) # 1 create + 4 updates + + # Exclude the create, because the create will return nil for `#reify`. + last_names = widget.versions.not_creates.map(&:reify).map(&:name) + expect(last_names).to eq(["Name 3", "Name 4", "Name 5"]) + # No matter what order the version records are returned it, we should + # always keep the most-recent changes. + end + end +end