diff --git a/Gemfile b/Gemfile index 9a86e1b40..bb53c1da3 100644 --- a/Gemfile +++ b/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" diff --git a/Gemfile.lock b/Gemfile.lock index 7b01d6da3..05ae21fd9 100644 --- a/Gemfile.lock +++ b/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 diff --git a/app/models/post.rb b/app/models/post.rb index 6bc60b270..1370802f2 100644 --- a/app/models/post.rb +++ b/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(" ")) diff --git a/app/models/tag_implication.rb b/app/models/tag_implication.rb index 05d26cd04..d65cc5fcf 100644 --- a/app/models/tag_implication.rb +++ b/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 - [].tap do |all| - children = [consequent_name] - - until children.empty? - all.concat(children) - children = TagImplication.active.where(antecedent_name: children).pluck(:consequent_name) + concerning :HierarchyMethods do + class_methods do + def ancestors_of(names) + join_recursive do |query| + query.start_with(antecedent_name: names).connect_by(consequent_name: :antecedent_name) end - end.sort.uniq - end + end - def update_descendant_names - self.descendant_names = descendants - end + def descendants_of(names) + join_recursive do |query| + query.start_with(consequent_name: names).connect_by(antecedent_name: :consequent_name) + end + end - def update_descendant_names! - update_descendant_names - update_attribute(:descendant_names, descendant_names) - end + def tags_implied_by(names) + Tag.where(name: active.ancestors_of(names).select(:consequent_name)).where.not(name: names) + end - def update_descendant_names_for_parents - parent_implications.each do |parent| - parent.update_descendant_names! - parent.update_descendant_names_for_parents + def tags_implied_to(names) + Tag.where(name: active.descendants_of(names).select(:antecedent_name)) 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 - if descendants.include?(antecedent_name) + # We don't want a -> b -> a chains + 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) - implied_tags = implications.flat_map(&:descendant_names) + implications = TagImplication.active.where("NOT (tag_implications.antecedent_name = ? AND tag_implications.consequent_name = ?)", antecedent_name, consequent_name) + 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 diff --git a/db/migrate/20200223042415_drop_descendant_names_from_tag_implications.rb b/db/migrate/20200223042415_drop_descendant_names_from_tag_implications.rb new file mode 100644 index 000000000..f8a5a070a --- /dev/null +++ b/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 diff --git a/db/structure.sql b/db/structure.sql index 9a3b78e96..abe06aaa6 100644 --- a/db/structure.sql +++ b/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'); diff --git a/test/unit/tag_implication_test.rb b/test/unit/tag_implication_test.rb index a27799ce0..6e907ef08 100644 --- a/test/unit/tag_implication_test.rb +++ b/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")