From e6b16e8fe5e8bf0058433debbba0151fd5e4964e Mon Sep 17 00:00:00 2001 From: r888888888 Date: Thu, 28 Jan 2016 17:39:01 -0800 Subject: [PATCH] better validation for bulk update requests --- app/logical/alias_and_implication_importer.rb | 29 +++++++++++++++++++ app/models/bulk_update_request.rb | 13 ++++++++- app/models/tag_alias.rb | 21 +++++++++++++- app/models/tag_implication.rb | 15 ++++++++++ config/danbooru_default_config.rb | 5 ++++ test/unit/bulk_update_request_test.rb | 16 ++++++++++ test/unit/tag_alias_test.rb | 2 +- 7 files changed, 98 insertions(+), 3 deletions(-) diff --git a/app/logical/alias_and_implication_importer.rb b/app/logical/alias_and_implication_importer.rb index 8f365301b..c29a5ca72 100644 --- a/app/logical/alias_and_implication_importer.rb +++ b/app/logical/alias_and_implication_importer.rb @@ -12,6 +12,11 @@ class AliasAndImplicationImporter parse(tokens) end + def validate! + tokens = AliasAndImplicationImporter.tokenize(text) + validate(tokens) + end + def rename_aliased_pages? @rename_aliased_pages == "1" end @@ -45,6 +50,30 @@ class AliasAndImplicationImporter end end + def validate(tokens) + tokens.map do |token| + case token[0] + when :create_alias + tag_alias = TagAlias.new(:forum_topic_id => forum_id, :status => "pending", :antecedent_name => token[1], :consequent_name => token[2]) + unless tag_alias.valid? + raise "Error: #{tag_alias.errors.full_messages.join("; ")} (create alias #{tag_alias.antecedent_name} -> #{tag_alias.consequent_name})" + end + + when :create_implication + tag_implication = TagImplication.new(:forum_topic_id => forum_id, :status => "pending", :antecedent_name => token[1], :consequent_name => token[2]) + unless tag_implication.valid? + raise "Error: #{tag_implication.errors.full_messages.join("; ")} (create implication #{tag_implication.antecedent_name} -> #{tag_implication.consequent_name})" + end + + when :remove_alias, :remove_implication, :mass_update + # okay + + else + raise "Unknown token: #{token[0]}" + end + end + end + private def parse(tokens) diff --git a/app/models/bulk_update_request.rb b/app/models/bulk_update_request.rb index a5e4f9dad..2bd7055b4 100644 --- a/app/models/bulk_update_request.rb +++ b/app/models/bulk_update_request.rb @@ -10,6 +10,7 @@ class BulkUpdateRequest < ActiveRecord::Base validates_inclusion_of :status, :in => %w(pending approved rejected) validate :script_formatted_correctly validate :forum_topic_id_not_invalid + validate :validate_script attr_accessible :user_id, :forum_topic_id, :script, :title, :reason attr_accessible :status, :as => [:admin] before_validation :initialize_attributes, :on => :create @@ -130,7 +131,6 @@ class BulkUpdateRequest < ActiveRecord::Base end end - def update_forum_topic_for_approve if forum_topic forum_topic.posts.create( @@ -150,4 +150,15 @@ class BulkUpdateRequest < ActiveRecord::Base def normalize_text self.script = script.downcase end + + def validate_script + begin + AliasAndImplicationImporter.new(script, forum_topic_id, "1").validate! + rescue RuntimeError => e + self.errors[:base] = e.message + return false + end + + errors.empty? + end end diff --git a/app/models/tag_alias.rb b/app/models/tag_alias.rb index 3ef1319bf..c09a0e82e 100644 --- a/app/models/tag_alias.rb +++ b/app/models/tag_alias.rb @@ -8,6 +8,8 @@ class TagAlias < ActiveRecord::Base validates_uniqueness_of :antecedent_name validate :absence_of_transitive_relation validate :antecedent_and_consequent_are_different + # validate :consequent_has_wiki_page + # validate :mininum_antecedent_count belongs_to :creator, :class_name => "User" belongs_to :forum_topic attr_accessible :antecedent_name, :consequent_name, :forum_topic_id, :status @@ -139,7 +141,7 @@ class TagAlias < ActiveRecord::Base # We don't want a -> b && b -> c chains if the b -> c alias was created first. # If the a -> b alias was created first, the new one will be allowed and the old one will be moved automatically instead. if self.class.active.exists?(["antecedent_name = ?", consequent_name]) - self.errors[:base] << "Tag alias can not create a transitive relation with another tag alias" + self.errors[:base] << "A tag alias for #{consequent_name} already exists" false end end @@ -290,6 +292,23 @@ class TagAlias < ActiveRecord::Base destroy end + def consequent_has_wiki_page + return if !Danbooru.config.strict_tag_requirements + + unless WikiPage.titled(consequent_name).exists? + self.errors[:base] = "The #{consequent_name} tag needs a corresponding wiki page" + return false + end + end + + def mininum_antecedent_count + return if !Danbooru.config.strict_tag_requirements + + unless Post.fast_count(antecedent_name) >= 50 + self.errors[:base] = "The #{antecedent_name} tag must have at least 50 posts for an alias to be created" + end + end + def self.update_cached_post_counts_for_all TagAlias.without_timeout do execute_sql("UPDATE tag_aliases SET post_count = tags.post_count FROM tags WHERE tags.name = tag_aliases.consequent_name") diff --git a/app/models/tag_implication.rb b/app/models/tag_implication.rb index 988e800f0..92ebc7780 100644 --- a/app/models/tag_implication.rb +++ b/app/models/tag_implication.rb @@ -12,6 +12,7 @@ class TagImplication < ActiveRecord::Base validate :antecedent_is_not_aliased validate :consequent_is_not_aliased validate :antecedent_and_consequent_are_different + # validate :wiki_pages_present attr_accessible :antecedent_name, :consequent_name, :descendant_names, :forum_topic_id, :status, :forum_topic module DescendantMethods @@ -256,6 +257,20 @@ class TagImplication < ActiveRecord::Base end end + def wiki_pages_present + return if !Danbooru.config.strict_tag_requirements + + unless WikiPage.titled(consequent_name).exists? + self.errors[:base] = "The #{consequent_name} tag needs a corresponding wiki page" + return false + end + + unless WikiPage.titled(antecedent_name).exists? + self.errors[:base] = "The #{antecedent_name} tag needs a corresponding wiki page" + return false + end + end + def reject! update_forum_topic_for_reject destroy diff --git a/config/danbooru_default_config.rb b/config/danbooru_default_config.rb index 5e991ff54..dc913f0c6 100644 --- a/config/danbooru_default_config.rb +++ b/config/danbooru_default_config.rb @@ -348,6 +348,11 @@ module Danbooru "zDMSATq0W3hmA5p3rKTgD" end + # impose additional requirements to create tag aliases and implications + def strict_tag_requirements + true + end + # For downloads, if the host matches any of these IPs, block it def banned_ip_for_download?(ip_addr) raise ArgumentError unless ip_addr.is_a?(IPAddr) diff --git a/test/unit/bulk_update_request_test.rb b/test/unit/bulk_update_request_test.rb index 7f81ebc93..eb622bee0 100644 --- a/test/unit/bulk_update_request_test.rb +++ b/test/unit/bulk_update_request_test.rb @@ -4,10 +4,12 @@ class BulkUpdateRequestTest < ActiveSupport::TestCase context "creation" do setup do CurrentUser.user = FactoryGirl.create(:user) + CurrentUser.ip_addr = "127.0.0.1" end teardown do CurrentUser.user = nil + CurrentUser.ip_addr = nil end should "create a forum topic" do @@ -16,6 +18,20 @@ class BulkUpdateRequestTest < ActiveSupport::TestCase end end + context "that has an invalid alias" do + setup do + @alias1 = FactoryGirl.create(:tag_alias) + @req = FactoryGirl.build(:bulk_update_request, :script => "create alias bbb -> aaa") + end + + should "not validate" do + assert_difference("TagAlias.count", 0) do + @req.save + end + assert_equal(["Error: A tag alias for aaa already exists (create alias bbb -> aaa)"], @req.errors.full_messages) + end + end + context "with an associated forum topic" do setup do @admin = FactoryGirl.create(:admin_user) diff --git a/test/unit/tag_alias_test.rb b/test/unit/tag_alias_test.rb index 90277330d..7eb6c36bd 100644 --- a/test/unit/tag_alias_test.rb +++ b/test/unit/tag_alias_test.rb @@ -60,7 +60,7 @@ class TagAliasTest < ActiveSupport::TestCase ta2 = FactoryGirl.build(:tag_alias, :antecedent_name => "aaa", :consequent_name => "bbb") ta2.save assert(ta2.errors.any?, "Tag alias should be invalid") - assert_equal("Tag alias can not create a transitive relation with another tag alias", ta2.errors.full_messages.join) + assert_equal("A tag alias for bbb already exists", ta2.errors.full_messages.join) end end