From 542c673221e228aeacd4dbf6c2422ba078d5cf81 Mon Sep 17 00:00:00 2001 From: evazion Date: Tue, 4 Jul 2017 17:02:17 -0500 Subject: [PATCH 1/2] Fix #3200: Disallow creation of superfluous implications. Disallow transitive implications. If a -> b -> c already exists, don't allow a -> c. Caveat: if b -> c already exists, and we make a BUR for a -> b and a -> c, the BUR validates even though a -> c is redundant. It only fails when the BUR is approved. --- app/models/tag_implication.rb | 11 ++++++++++ test/unit/bulk_update_request_test.rb | 29 +++++++++++++++++++++++++++ test/unit/tag_implication_test.rb | 11 +++++++++- 3 files changed, 50 insertions(+), 1 deletion(-) diff --git a/app/models/tag_implication.rb b/app/models/tag_implication.rb index b3d228009..9d3882959 100644 --- a/app/models/tag_implication.rb +++ b/app/models/tag_implication.rb @@ -18,6 +18,7 @@ class TagImplication < ApplicationRecord validates :forum_topic, presence: { message: "must exist" }, if: lambda { forum_topic_id.present? } validates_uniqueness_of :antecedent_name, :scope => :consequent_name validate :absence_of_circular_relation + validate :absence_of_transitive_relation validate :antecedent_is_not_aliased validate :consequent_is_not_aliased validate :antecedent_and_consequent_are_different @@ -137,6 +138,16 @@ class TagImplication < ApplicationRecord end end + # If we already have a -> b -> c, don't allow a -> c. + def absence_of_transitive_relation + # Find everything else the antecedent implies, not including the current implication. + implications = TagImplication.active.where("antecedent_name = ? and consequent_name != ?", antecedent_name, consequent_name) + implied_tags = implications.flat_map(&:descendant_names_array) + if implied_tags.include?(consequent_name) + self.errors[:base] << "#{antecedent_name} already implies #{consequent_name} through another implication" + end + end + def antecedent_is_not_aliased # We don't want to implicate a -> b if a is already aliased to c if TagAlias.active.exists?(["antecedent_name = ?", antecedent_name]) diff --git a/test/unit/bulk_update_request_test.rb b/test/unit/bulk_update_request_test.rb index a77771850..654c91ab4 100644 --- a/test/unit/bulk_update_request_test.rb +++ b/test/unit/bulk_update_request_test.rb @@ -62,6 +62,35 @@ class BulkUpdateRequestTest < ActiveSupport::TestCase end end + context "for an implication that is redundant with an existing implication" do + should "not validate" do + FactoryGirl.create(:tag_implication, :antecedent_name => "a", :consequent_name => "b") + FactoryGirl.create(:tag_implication, :antecedent_name => "b", :consequent_name => "c") + bur = FactoryGirl.build(:bulk_update_request, :script => "imply a -> c") + bur.save + + assert_equal(["Error: a already implies c through another implication (create implication a -> c)"], bur.errors.full_messages) + end + end + + context "for an implication that is redundant with another implication in the same BUR" do + setup do + FactoryGirl.create(:tag_implication, :antecedent_name => "b", :consequent_name => "c") + @bur = FactoryGirl.build(:bulk_update_request, :script => "imply a -> b\nimply a -> c") + @bur.save + end + + should "not process" do + assert_no_difference("TagImplication.count") do + @bur.approve!(@admin) + end + end + + should_eventually "not validate" do + assert_equal(["Error: a already implies c through another implication (create implication a -> c)"], @bur.errors.full_messages) + end + end + context "with an associated forum topic" do setup do @topic = FactoryGirl.create(:forum_topic, :title => "[bulk] hoge") diff --git a/test/unit/tag_implication_test.rb b/test/unit/tag_implication_test.rb index 5ac1a5ef9..cbcfba0fc 100644 --- a/test/unit/tag_implication_test.rb +++ b/test/unit/tag_implication_test.rb @@ -66,12 +66,21 @@ class TagImplicationTest < ActiveSupport::TestCase assert_equal("Tag implication can not create a circular relation with another tag implication", ti2.errors.full_messages.join("")) end + should "not validate when a transitive relation is created" do + ti_ab = FactoryGirl.create(:tag_implication, :antecedent_name => "a", :consequent_name => "b") + ti_bc = FactoryGirl.create(:tag_implication, :antecedent_name => "b", :consequent_name => "c") + ti_ac = FactoryGirl.build(:tag_implication, :antecedent_name => "a", :consequent_name => "c") + ti_ac.save + + assert_equal("a already implies c through another implication", ti_ac.errors.full_messages.join("")) + end + should "not allow for duplicates" do ti1 = FactoryGirl.create(:tag_implication, :antecedent_name => "aaa", :consequent_name => "bbb") ti2 = FactoryGirl.build(:tag_implication, :antecedent_name => "aaa", :consequent_name => "bbb") ti2.save assert(ti2.errors.any?, "Tag implication should not have validated.") - assert_equal("Antecedent name has already been taken", ti2.errors.full_messages.join("")) + assert_includes(ti2.errors.full_messages, "Antecedent name has already been taken") end should "not validate if its consequent is aliased to another tag" do From aac1463fbf38f9d5cfd9ce74c635d231a34ec79e Mon Sep 17 00:00:00 2001 From: evazion Date: Tue, 4 Jul 2017 20:33:45 -0500 Subject: [PATCH 2/2] implications: count 'queued' implications as active. Bug: implications that were approved but that were still in the 'queued' state were not seen as active yet, which led to the transitivity validation passing because it didn't include queued implications. --- app/models/tag_implication.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/app/models/tag_implication.rb b/app/models/tag_implication.rb index 9d3882959..2e886b132 100644 --- a/app/models/tag_implication.rb +++ b/app/models/tag_implication.rb @@ -32,7 +32,7 @@ class TagImplication < ApplicationRecord module ClassMethods # assumes names are normalized def with_descendants(names) - (names + where("antecedent_name in (?) and status in (?)", names, ["active", "processing"]).map(&:descendant_names_array)).flatten.uniq + (names + active.where(antecedent_name: names).flat_map(&:descendant_names_array)).uniq end def automatic_tags_for(names) @@ -49,7 +49,7 @@ class TagImplication < ApplicationRecord until children.empty? all.concat(children) - children = TagImplication.where("antecedent_name IN (?) and status in (?)", children, ["active", "processing"]).map(&:consequent_name) + children = TagImplication.active.where(antecedent_name: children).pluck(:consequent_name) end end.sort.uniq end @@ -97,7 +97,7 @@ class TagImplication < ApplicationRecord end def active - where("status IN (?)", ["active", "processing"]) + where(status: %w[active processing queued]) end def search(params)