Merge pull request #3201 from evazion/fix-transitive-implications

Fix #3200: Disallow creation of superfluous implications

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
@@ -31,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)
@@ -48,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
@@ -96,7 +97,7 @@ class TagImplication < ApplicationRecord
     end
 
     def active
-      where("status IN (?)", ["active", "processing"])
+      where(status: %w[active processing queued])
     end
 
     def search(params)
@@ -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])

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")

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