implications: refactor calculation of implied tags.

Refactor to use a recursive CTE to calculate implied tags in SQL, rather
than storing them in a descendant_names field. This avoids the
complexity of keeping the stored field up to date. It's also more
flexible, since it allows us to find both descendant tags (tags that
imply a given tag) as well as ancestor tags (tags that are implied by a
given tag).

Gemfile

@@ -44,6 +44,7 @@ gem 'puma'
 gem 'scenic'
 gem 'ipaddress'
 gem 'http'
+gem 'activerecord-hierarchical_query'
 
 # needed for looser jpeg header compat
 gem 'ruby-imagespec', :require => "image_spec", :git => "https://github.com/r888888888/ruby-imagespec.git", :branch => "exif-fixes"

Gemfile.lock

@@ -63,6 +63,9 @@ GEM
     activerecord (6.0.2.1)
       activemodel (= 6.0.2.1)
       activesupport (= 6.0.2.1)
+    activerecord-hierarchical_query (1.2.3)
+      activerecord (>= 5.0, < 6.1)
+      pg (>= 0.21, < 1.3)
     activestorage (6.0.2.1)
       actionpack (= 6.0.2.1)
       activejob (= 6.0.2.1)
@@ -402,6 +405,7 @@ PLATFORMS
 
 DEPENDENCIES
   activemodel-serializers-xml
+  activerecord-hierarchical_query
   addressable
   awesome_print
   aws-sdk-sqs (~> 1)
@@ -473,4 +477,4 @@ DEPENDENCIES
   whenever
 
 BUNDLED WITH
-   2.0.2
+   2.1.2

app/models/post.rb

@@ -643,7 +643,7 @@ class Post < ApplicationRecord
       normalized_tags = remove_invalid_tags(normalized_tags)
       normalized_tags = Tag.convert_cosplay_tags(normalized_tags)
       normalized_tags += Tag.create_for_list(TagImplication.automatic_tags_for(normalized_tags))
-      normalized_tags = TagImplication.with_descendants(normalized_tags)
+      normalized_tags += TagImplication.tags_implied_by(normalized_tags).map(&:name)
       normalized_tags = normalized_tags.compact.uniq.sort
       normalized_tags = Tag.create_for_list(normalized_tags)
       set_tag_string(normalized_tags.join(" "))

app/models/tag_implication.rb

@@ -1,12 +1,7 @@
 class TagImplication < TagRelationship
-  array_attribute :descendant_names
-
   has_many :child_implications, class_name: "TagImplication", primary_key: :consequent_name, foreign_key: :antecedent_name
   has_many :parent_implications, class_name: "TagImplication", primary_key: :antecedent_name, foreign_key: :consequent_name
 
-  before_save :update_descendant_names
-  after_save :update_descendant_names_for_parents
-  after_destroy :update_descendant_names_for_parents
   after_save :create_mod_action
   validates_uniqueness_of :antecedent_name, scope: [:consequent_name, :status], conditions: -> { active }
   validate :absence_of_circular_relation
@@ -19,11 +14,6 @@ class TagImplication < TagRelationship
     extend ActiveSupport::Concern
 
     module ClassMethods
-      # assumes names are normalized
-      def with_descendants(names)
-        (names + active.where(antecedent_name: names).flat_map(&:descendant_names)).uniq
-      end
-
       def automatic_tags_for(names)
         tags = []
         tags += names.grep(/\A(.+)_\(cosplay\)\z/i) { "char:#{TagAlias.to_aliased([$1]).first}" }
@@ -32,31 +22,28 @@ class TagImplication < TagRelationship
         tags.uniq
       end
     end
+  end
 
-    def descendants
+  concerning :HierarchyMethods do
-      [].tap do |all|
+    class_methods do
-        children = [consequent_name]
+      def ancestors_of(names)
-
+        join_recursive do |query|
-        until children.empty?
+          query.start_with(antecedent_name: names).connect_by(consequent_name: :antecedent_name)
-          all.concat(children)
-          children = TagImplication.active.where(antecedent_name: children).pluck(:consequent_name)
         end
-      end.sort.uniq
+      end
-    end
 
-    def update_descendant_names
+      def descendants_of(names)
-      self.descendant_names = descendants
+        join_recursive do |query|
-    end
+          query.start_with(consequent_name: names).connect_by(antecedent_name: :consequent_name)
+        end
+      end
 
-    def update_descendant_names!
+      def tags_implied_by(names)
-      update_descendant_names
+        Tag.where(name: active.ancestors_of(names).select(:consequent_name)).where.not(name: names)
-      update_attribute(:descendant_names, descendant_names)
+      end
-    end
 
-    def update_descendant_names_for_parents
+      def tags_implied_to(names)
-      parent_implications.each do |parent|
+        Tag.where(name: active.descendants_of(names).select(:antecedent_name))
-        parent.update_descendant_names!
-        parent.update_descendant_names_for_parents
       end
     end
   end
@@ -65,8 +52,9 @@ class TagImplication < TagRelationship
     def absence_of_circular_relation
       return if is_rejected?
 
-      # We don't want a -> b && b -> a chains
+      # We don't want a -> b -> a chains
-      if descendants.include?(antecedent_name)
+      implied_tags = TagImplication.tags_implied_by(consequent_name).map(&:name)
+      if implied_tags.include?(antecedent_name)
         errors[:base] << "Tag implication can not create a circular relation with another tag implication"
       end
     end
@@ -76,8 +64,9 @@ class TagImplication < TagRelationship
       return if is_rejected?
 
       # Find everything else the antecedent implies, not including the current implication.
-      implications = TagImplication.active.where("antecedent_name = ? and consequent_name != ?", antecedent_name, consequent_name)
+      implications = TagImplication.active.where("NOT (tag_implications.antecedent_name = ? AND tag_implications.consequent_name = ?)", antecedent_name, consequent_name)
-      implied_tags = implications.flat_map(&:descendant_names)
+      implied_tags = implications.tags_implied_by(antecedent_name).map(&:name)
+
       if implied_tags.include?(consequent_name)
         errors[:base] << "#{antecedent_name} already implies #{consequent_name} through another implication"
       end
@@ -122,7 +111,6 @@ class TagImplication < TagRelationship
         update(status: "processing")
         update_posts
         update(status: "active")
-        update_descendant_names_for_parents
         forum_updater.update(approval_message(approver), "APPROVED") if update_topic
       end
     rescue Exception => e

db/migrate/20200223042415_drop_descendant_names_from_tag_implications.rb

@@ -0,0 +1,5 @@
+class DropDescendantNamesFromTagImplications < ActiveRecord::Migration[6.0]
+  def change
+    remove_column :tag_implications, :descendant_names, "text[]", default: "{}", null: false
+  end
+end

db/structure.sql

@@ -2883,7 +2883,6 @@ CREATE TABLE public.tag_implications (
     id integer NOT NULL,
     antecedent_name character varying NOT NULL,
     consequent_name character varying NOT NULL,
-    descendant_names text[] DEFAULT '{}'::text[] NOT NULL,
     creator_id integer NOT NULL,
     forum_topic_id integer,
     status text DEFAULT 'pending'::text NOT NULL,
@@ -7352,6 +7351,7 @@ INSERT INTO "schema_migrations" (version) VALUES
 ('20200119184442'),
 ('20200119193110'),
 ('20200123184743'),
-('20200217044719');
+('20200217044719'),
+('20200223042415');
 
 

test/unit/tag_implication_test.rb

@@ -84,13 +84,6 @@ class TagImplicationTest < ActiveSupport::TestCase
       end
     end
 
-    should "ignore pending implications when building descendant names" do
-      ti2 = FactoryBot.build(:tag_implication, :antecedent_name => "b", :consequent_name => "c", :status => "pending")
-      ti2.save
-      ti1 = FactoryBot.create(:tag_implication, :antecedent_name => "a", :consequent_name => "b")
-      assert_equal(%w[b], ti1.descendant_names)
-    end
-
     should "populate the creator information" do
       ti = create(:tag_implication, antecedent_name: "aaa", consequent_name: "bbb", creator: CurrentUser.user)
       assert_equal(CurrentUser.user.id, ti.creator_id)
@@ -148,95 +141,6 @@ class TagImplicationTest < ActiveSupport::TestCase
       assert_includes(ti.errors[:base], "Consequent tag must not be aliased to another tag")
     end
 
-    should "calculate all its descendants" do
-      ti1 = FactoryBot.create(:tag_implication, :antecedent_name => "bbb", :consequent_name => "ccc")
-      assert_equal(%w[ccc], ti1.descendant_names)
-      ti2 = FactoryBot.create(:tag_implication, :antecedent_name => "aaa", :consequent_name => "bbb")
-      assert_equal(%w[bbb ccc], ti2.descendant_names)
-      ti1.reload
-      assert_equal(%w[ccc], ti1.descendant_names)
-    end
-
-    should "update its descendants on save" do
-      ti1 = FactoryBot.create(:tag_implication, :antecedent_name => "aaa", :consequent_name => "bbb", :status => "active")
-      ti2 = FactoryBot.create(:tag_implication, :antecedent_name => "ccc", :consequent_name => "ddd", :status => "active")
-      ti1.reload
-      ti2.reload
-      ti2.update(
-        :antecedent_name => "bbb"
-      )
-      ti1.reload
-      ti2.reload
-      assert_equal(%w[bbb ddd], ti1.descendant_names)
-      assert_equal(%w[ddd], ti2.descendant_names)
-    end
-
-    should "update the descendants for all of its parents on destroy" do
-      ti1 = FactoryBot.create(:tag_implication, :antecedent_name => "aaa", :consequent_name => "bbb")
-      ti2 = FactoryBot.create(:tag_implication, :antecedent_name => "xxx", :consequent_name => "bbb")
-      ti3 = FactoryBot.create(:tag_implication, :antecedent_name => "bbb", :consequent_name => "ccc")
-      ti4 = FactoryBot.create(:tag_implication, :antecedent_name => "ccc", :consequent_name => "ddd")
-      ti1.reload
-      ti2.reload
-      ti3.reload
-      ti4.reload
-      assert_equal(%w[bbb ccc ddd], ti1.descendant_names)
-      assert_equal(%w[bbb ccc ddd], ti2.descendant_names)
-      assert_equal(%w[ccc ddd], ti3.descendant_names)
-      assert_equal(%w[ddd], ti4.descendant_names)
-      ti3.destroy
-      ti1.reload
-      ti2.reload
-      ti4.reload
-      assert_equal(%w[bbb], ti1.descendant_names)
-      assert_equal(%w[bbb], ti2.descendant_names)
-      assert_equal(%w[ddd], ti4.descendant_names)
-    end
-
-    should "update the descendants for all of its parents on create" do
-      ti1 = FactoryBot.create(:tag_implication, :antecedent_name => "aaa", :consequent_name => "bbb")
-      ti1.reload
-      assert_equal("active", ti1.status)
-      assert_equal(%w[bbb], ti1.descendant_names)
-
-      ti2 = FactoryBot.create(:tag_implication, :antecedent_name => "bbb", :consequent_name => "ccc")
-      ti1.reload
-      ti2.reload
-      assert_equal("active", ti1.status)
-      assert_equal("active", ti2.status)
-      assert_equal(%w[bbb ccc], ti1.descendant_names)
-      assert_equal(%w[ccc], ti2.descendant_names)
-
-      ti3 = FactoryBot.create(:tag_implication, :antecedent_name => "ccc", :consequent_name => "ddd")
-      ti1.reload
-      ti2.reload
-      ti3.reload
-      assert_equal(%w[bbb ccc ddd], ti1.descendant_names)
-      assert_equal(%w[ccc ddd], ti2.descendant_names)
-
-      ti4 = FactoryBot.create(:tag_implication, :antecedent_name => "ccc", :consequent_name => "eee")
-      ti1.reload
-      ti2.reload
-      ti3.reload
-      ti4.reload
-      assert_equal(%w[bbb ccc ddd eee], ti1.descendant_names)
-      assert_equal(%w[ccc ddd eee], ti2.descendant_names)
-      assert_equal(%w[ddd], ti3.descendant_names)
-      assert_equal(%w[eee], ti4.descendant_names)
-
-      ti5 = FactoryBot.create(:tag_implication, :antecedent_name => "xxx", :consequent_name => "bbb")
-      ti1.reload
-      ti2.reload
-      ti3.reload
-      ti4.reload
-      ti5.reload
-      assert_equal(%w[bbb ccc ddd eee], ti1.descendant_names)
-      assert_equal(%w[ccc ddd eee], ti2.descendant_names)
-      assert_equal(%w[ddd], ti3.descendant_names)
-      assert_equal(%w[eee], ti4.descendant_names)
-      assert_equal(%w[bbb ccc ddd eee], ti5.descendant_names)
-    end
-
     should "update any affected post upon save" do
       p1 = FactoryBot.create(:post, :tag_string => "aaa bbb ccc")
       ti1 = FactoryBot.create(:tag_implication, :antecedent_name => "aaa", :consequent_name => "xxx")
@@ -249,6 +153,35 @@ class TagImplicationTest < ActiveSupport::TestCase
       assert_equal("aaa bbb ccc xxx yyy", p1.reload.tag_string)
     end
 
+    context "when calculating implied tags" do
+      should "include tags for all active implications" do
+        # a -> b -> c -> d; b -> b1; c -> c1
+        create(:tag_implication, antecedent_name: "a", consequent_name: "b", status: "active")
+        create(:tag_implication, antecedent_name: "b", consequent_name: "c", status: "active")
+        create(:tag_implication, antecedent_name: "c", consequent_name: "d", status: "active")
+        create(:tag_implication, antecedent_name: "b", consequent_name: "b1", status: "active")
+        create(:tag_implication, antecedent_name: "c", consequent_name: "c1", status: "active")
+
+        assert_equal(%w[b b1 c c1 d], TagImplication.tags_implied_by("a").map(&:name).sort)
+        assert_equal(%w[b1 c c1 d], TagImplication.tags_implied_by("b").map(&:name).sort)
+        assert_equal(%w[c1 d], TagImplication.tags_implied_by("c").map(&:name).sort)
+        assert_equal([], TagImplication.tags_implied_by("b1").map(&:name).sort)
+        assert_equal([], TagImplication.tags_implied_by("c1").map(&:name).sort)
+        assert_equal([], TagImplication.tags_implied_by("d").map(&:name).sort)
+      end
+
+      should "not include inactive implications" do
+        create(:tag_implication, antecedent_name: "a", consequent_name: "b", status: "active")
+        create(:tag_implication, antecedent_name: "b", consequent_name: "c", status: "pending")
+        create(:tag_implication, antecedent_name: "c", consequent_name: "d", status: "active")
+
+        assert_equal(["b"], TagImplication.tags_implied_by("a").map(&:name))
+        assert_equal([], TagImplication.tags_implied_by("b").map(&:name))
+        assert_equal(["d"], TagImplication.tags_implied_by("c").map(&:name))
+        assert_equal([], TagImplication.tags_implied_by("d").map(&:name))
+      end
+    end
+
     context "with an associated forum topic" do
       setup do
         @topic = FactoryBot.create(:forum_topic, :title => "Tag implication: aaa -> bbb")