From b660aeefd7f938589348a41b0193ae393767637e Mon Sep 17 00:00:00 2001 From: evazion Date: Tue, 13 Nov 2018 16:25:45 -0600 Subject: [PATCH 1/5] application record: add array_attribute method. Add `array_attribute` method that defines helper methods for converting array attributes to or from strings. --- app/controllers/pools_controller.rb | 2 +- app/models/application_record.rb | 31 +++++++++++++++++++++++++++++ app/models/pool.rb | 16 +++------------ app/views/pools/edit.html.erb | 2 +- app/views/pools/new.html.erb | 2 +- 5 files changed, 37 insertions(+), 16 deletions(-) diff --git a/app/controllers/pools_controller.rb b/app/controllers/pools_controller.rb index 610c22c7e..2f183e4dd 100644 --- a/app/controllers/pools_controller.rb +++ b/app/controllers/pools_controller.rb @@ -94,7 +94,7 @@ class PoolsController < ApplicationController private def pool_params - permitted_params = %i[name description category is_active post_ids] + permitted_params = %i[name description category is_active post_ids post_ids_string] params.require(:pool).permit(*permitted_params, post_ids: []) end end diff --git a/app/models/application_record.rb b/app/models/application_record.rb index 42dced560..e80fe1ccd 100644 --- a/app/models/application_record.rb +++ b/app/models/application_record.rb @@ -269,6 +269,37 @@ class ApplicationRecord < ActiveRecord::Base end end + concerning :AttributeMethods do + class_methods do + # Defines `_string`, `_string=`, and `=` + # methods for converting an array attribute to or from a string. + # + # The `=` setter parses strings into an array using the + # `parse` regex. The resulting strings can be converted to another type + # with the `cast` option. + def array_attribute(name, parse: /[^[:space:]]+/, cast: :itself) + define_method "#{name}_string" do + send(name).join(" ") + end + + define_method "#{name}_string=" do |value| + raise ArgumentError, "#{name} must be a String" unless value.respond_to?(:to_str) + send("#{name}=", value) + end + + define_method "#{name}=" do |value| + if value.respond_to?(:to_str) + super value.to_str.scan(parse).map(&cast) + elsif value.respond_to?(:to_a) + super value.to_a + else + raise ArgumentError, "#{name} must be a String or an Array" + end + end + end + end + end + def warnings @warnings ||= ActiveModel::Errors.new(self) end diff --git a/app/models/pool.rb b/app/models/pool.rb index 115088842..6df6a08cc 100644 --- a/app/models/pool.rb +++ b/app/models/pool.rb @@ -1,13 +1,15 @@ class Pool < ApplicationRecord class RevertError < Exception ; end + array_attribute :post_ids, parse: /\d+/, cast: :to_i + belongs_to_creator + validates_uniqueness_of :name, case_sensitive: false, if: :name_changed? validate :validate_name, if: :name_changed? validates_inclusion_of :category, :in => %w(series collection) validate :updater_can_change_category validate :updater_can_remove_posts validate :updater_can_edit_deleted - belongs_to_creator before_validation :normalize_post_ids before_validation :normalize_name after_save :update_category_pseudo_tags_for_posts_async @@ -153,18 +155,6 @@ class Pool < ApplicationRecord self.post_ids = post_ids.uniq if is_collection? end - # allow assigning a string to post_ids so it can be assigned from the text - # field in the pool edit form (PUT /pools/1?post_ids=1+2+3). - def post_ids=(value) - if value.respond_to?(:to_str) - super value.to_str.scan(/\d+/).map(&:to_i) - elsif value.respond_to?(:to_a) - super value.to_a - else - raise ArgumentError, "post_ids must be a String or an Array" - end - end - def revert_to!(version) if id != version.pool_id raise RevertError.new("You cannot revert to a previous version of another pool.") diff --git a/app/views/pools/edit.html.erb b/app/views/pools/edit.html.erb index 460a0ba5c..53318d5bc 100644 --- a/app/views/pools/edit.html.erb +++ b/app/views/pools/edit.html.erb @@ -8,7 +8,7 @@ <%= f.input :name, :as => :string, :input_html => { :value => @pool.pretty_name } %> <%= dtext_field "pool", "description" %> <%= dtext_preview_button "pool", "description" %> - <%= f.input :post_ids, as: :text, label: "Posts", input_html: { value: @pool.post_ids.join(" ") } %> + <%= f.input :post_ids_string, as: :text, label: "Posts" %> <%= f.input :category, :collection => ["series", "collection"], :include_blank => false %> <%= f.input :is_active %> <%= f.button :submit %> diff --git a/app/views/pools/new.html.erb b/app/views/pools/new.html.erb index d3d28221f..b1156a0f4 100644 --- a/app/views/pools/new.html.erb +++ b/app/views/pools/new.html.erb @@ -7,7 +7,7 @@ <%= simple_form_for(@pool) do |f| %> <%= f.input :name, :as => :string, :required => true %> <%= dtext_field "pool", "description" %> - <%= f.input :post_ids, as: :text, label: "Posts", input_html: { value: @pool.post_ids.join(" ") } %> + <%= f.input :post_ids_string, as: :text, label: "Posts" %> <%= f.input :category, :collection => ["series", "collection"], :include_blank => true, :selected => "", :required => true %> <%= f.input :is_active %> <%= f.button :submit, "Submit" %> From 308a5021b49aa2f57d8ac0e14c747a9490a824d2 Mon Sep 17 00:00:00 2001 From: evazion Date: Tue, 13 Nov 2018 18:08:20 -0600 Subject: [PATCH 2/5] wiki pages: convert other_names to array (#3987). --- app/controllers/wiki_pages_controller.rb | 19 ++++--------- app/helpers/wiki_pages_helper.rb | 2 +- app/logical/sources/strategies/base.rb | 2 +- app/models/wiki_page.rb | 28 ++++++++----------- app/models/wiki_page_version.rb | 5 +--- app/views/wiki_pages/_form.html.erb | 2 +- ..._change_strings_to_arrays_on_wiki_pages.rb | 26 +++++++++++++++++ db/structure.sql | 19 ++++--------- test/unit/wiki_page_test.rb | 2 +- 9 files changed, 55 insertions(+), 50 deletions(-) create mode 100644 db/migrate/20181113174914_change_strings_to_arrays_on_wiki_pages.rb diff --git a/app/controllers/wiki_pages_controller.rb b/app/controllers/wiki_pages_controller.rb index 5c3eab980..41b7a2b59 100644 --- a/app/controllers/wiki_pages_controller.rb +++ b/app/controllers/wiki_pages_controller.rb @@ -5,7 +5,7 @@ class WikiPagesController < ApplicationController before_action :normalize_search_params, :only => [:index] def new - @wiki_page = WikiPage.new(wiki_page_create_params) + @wiki_page = WikiPage.new(wiki_page_params(:create)) respond_with(@wiki_page) end @@ -54,13 +54,13 @@ class WikiPagesController < ApplicationController end def create - @wiki_page = WikiPage.create(wiki_page_create_params) + @wiki_page = WikiPage.create(wiki_page_params(:create)) respond_with(@wiki_page) end def update @wiki_page = WikiPage.find(params[:id]) - @wiki_page.update(wiki_page_update_params) + @wiki_page.update(wiki_page_params(:update)) respond_with(@wiki_page) end @@ -98,18 +98,11 @@ class WikiPagesController < ApplicationController end end - def wiki_page_create_params - permitted_params = %i[title body other_names skip_secondary_validations] + def wiki_page_params(context) + permitted_params = %i[body other_names other_names_string skip_secondary_validations] permitted_params += %i[is_locked is_deleted] if CurrentUser.is_builder? + permitted_params += %i[title] if context == :create || CurrentUser.is_builder? params.fetch(:wiki_page, {}).permit(permitted_params) end - - def wiki_page_update_params - permitted_params = %i[body other_names skip_secondary_validations] - permitted_params += %i[title is_locked is_deleted] if CurrentUser.is_builder? - - params.fetch(:wiki_page, {}).permit(permitted_params) - end - end diff --git a/app/helpers/wiki_pages_helper.rb b/app/helpers/wiki_pages_helper.rb index b41b9aa66..daa3b2d40 100644 --- a/app/helpers/wiki_pages_helper.rb +++ b/app/helpers/wiki_pages_helper.rb @@ -18,7 +18,7 @@ module WikiPagesHelper end def wiki_page_other_names_list(wiki_page) - names_html = wiki_page.other_names_array.map{|name| link_to(name, "http://www.pixiv.net/search.php?s_mode=s_tag_full&word=#{u(name)}", :class => "wiki-other-name")} + names_html = wiki_page.other_names.map{|name| link_to(name, "http://www.pixiv.net/search.php?s_mode=s_tag_full&word=#{u(name)}", :class => "wiki-other-name")} names_html.join(" ").html_safe end end diff --git a/app/logical/sources/strategies/base.rb b/app/logical/sources/strategies/base.rb index 7e891aeed..01d883d74 100644 --- a/app/logical/sources/strategies/base.rb +++ b/app/logical/sources/strategies/base.rb @@ -187,7 +187,7 @@ module Sources def translate_tag(untranslated_tag) return [] if untranslated_tag.blank? - translated_tag_names = WikiPage.active.other_names_equal(untranslated_tag).uniq.pluck(:title) + translated_tag_names = WikiPage.active.other_names_include(untranslated_tag).uniq.pluck(:title) translated_tag_names = TagAlias.to_aliased(translated_tag_names) translated_tags = Tag.where(name: translated_tag_names) diff --git a/app/models/wiki_page.rb b/app/models/wiki_page.rb index e3d840fc4..6a9c9cd59 100644 --- a/app/models/wiki_page.rb +++ b/app/models/wiki_page.rb @@ -4,14 +4,16 @@ class WikiPage < ApplicationRecord before_save :normalize_title before_save :normalize_other_names after_save :create_version - belongs_to_creator - belongs_to_updater validates_uniqueness_of :title, :case_sensitive => false validates_presence_of :title validates_presence_of :body, :unless => -> { is_deleted? || other_names.present? } validate :validate_rename validate :validate_not_locked + attr_accessor :skip_secondary_validations + array_attribute :other_names + belongs_to_creator + belongs_to_updater has_one :tag, :foreign_key => "name", :primary_key => "title" has_one :artist, -> {where(:is_active => true)}, :foreign_key => "name", :primary_key => "title" has_many :versions, -> {order("wiki_page_versions.id ASC")}, :class_name => "WikiPageVersion", :dependent => :destroy @@ -29,17 +31,16 @@ class WikiPage < ApplicationRecord order("updated_at DESC").limit(25) end - def other_names_equal(name) - query_sql = name.unicode_normalize(:nfkc).to_escaped_for_tsquery - where("other_names_index @@ to_tsquery('danbooru', E?)", query_sql) + def other_names_include(name) + where("wiki_pages.other_names @> ARRAY[?]", name.unicode_normalize(:nfkc)) end def other_names_match(name) if name =~ /\*/ - subquery = WikiPage.from("unnest(string_to_array(other_names, ' ')) AS other_name").where("other_name ILIKE ?", name.to_escaped_for_sql_like) + subquery = WikiPage.from("unnest(other_names) AS other_name").where("other_name ILIKE ?", name.to_escaped_for_sql_like) where(id: subquery) else - other_names_equal(name) + other_names_include(name) end end @@ -73,9 +74,9 @@ class WikiPage < ApplicationRecord end if params[:other_names_present].to_s.truthy? - q = q.where("other_names is not null and other_names != ''") + q = q.where("other_names is not null and other_names != '{}'") elsif params[:other_names_present].to_s.falsy? - q = q.where("other_names is null or other_names = ''") + q = q.where("other_names is null or other_names = '{}'") end q = q.attribute_matches(:is_locked, params[:is_locked]) @@ -97,7 +98,7 @@ class WikiPage < ApplicationRecord module ApiMethods def hidden_attributes - super + [:body_index, :other_names_index] + super + [:body_index] end def method_attributes @@ -145,8 +146,7 @@ class WikiPage < ApplicationRecord end def normalize_other_names - normalized_other_names = other_names.to_s.unicode_normalize(:nfkc).scan(/[^[:space:]]+/) - self.other_names = normalized_other_names.uniq.join(" ") + self.other_names = other_names.map { |name| name.unicode_normalize(:nfkc) }.uniq end def skip_secondary_validations=(value) @@ -224,8 +224,4 @@ class WikiPage < ApplicationRecord def visible? artist.blank? || !artist.is_banned? || CurrentUser.is_builder? end - - def other_names_array - other_names.to_s.split(/[[:space:]]+/) - end end diff --git a/app/models/wiki_page_version.rb b/app/models/wiki_page_version.rb index 3db9ca94e..1f39a5467 100644 --- a/app/models/wiki_page_version.rb +++ b/app/models/wiki_page_version.rb @@ -1,4 +1,5 @@ class WikiPageVersion < ApplicationRecord + array_attribute :other_names belongs_to :wiki_page belongs_to_updater belongs_to :artist, optional: true @@ -45,8 +46,4 @@ class WikiPageVersion < ApplicationRecord def category_name Tag.category_for(title) end - - def other_names_array - other_names.to_s.split(/[[:space:]]+/) - end end diff --git a/app/views/wiki_pages/_form.html.erb b/app/views/wiki_pages/_form.html.erb index 737f3c9b7..48ebbd2db 100644 --- a/app/views/wiki_pages/_form.html.erb +++ b/app/views/wiki_pages/_form.html.erb @@ -10,7 +10,7 @@

<%= @wiki_page.pretty_title %>

<% end %> - <%= f.input :other_names, :as => :text, :label => "Other names (view help)".html_safe, :hint => "Names used for this tag on other sites such as Pixiv. Separate with spaces." %> + <%= f.input :other_names_string, as: :text, label: "Other names (#{link_to "help", wiki_pages_path(title: "help:translated_tags")})".html_safe, hint: "Names used for this tag on other sites such as Pixiv. Separate with spaces." %> <%= dtext_field "wiki_page", "body" %> diff --git a/db/migrate/20181113174914_change_strings_to_arrays_on_wiki_pages.rb b/db/migrate/20181113174914_change_strings_to_arrays_on_wiki_pages.rb new file mode 100644 index 000000000..9537993ca --- /dev/null +++ b/db/migrate/20181113174914_change_strings_to_arrays_on_wiki_pages.rb @@ -0,0 +1,26 @@ +class ChangeStringsToArraysOnWikiPages < ActiveRecord::Migration[5.2] + def up + WikiPage.without_timeout do + change_column :wiki_pages, :other_names, "text[]", using: "string_to_array(other_names, ' ')::text[]", default: "{}" + change_column :wiki_page_versions, :other_names, "text[]", using: "string_to_array(other_names, ' ')::text[]", default: "{}" + + remove_column :wiki_pages, :other_names_index + execute "DROP TRIGGER trigger_wiki_pages_on_update_for_other_names ON wiki_pages" + + add_index :wiki_pages, :other_names, using: :gin + end + end + + def down + WikiPage.without_timeout do + remove_index :wiki_pages, :other_names + + add_column :wiki_pages, :other_names_index, :tsvector + add_index :wiki_pages, :other_names_index, using: :gin + execute "CREATE TRIGGER trigger_wiki_pages_on_update_for_other_names BEFORE INSERT OR UPDATE ON wiki_pages FOR EACH ROW EXECUTE PROCEDURE tsvector_update_trigger('other_names_index', 'public.danbooru', 'other_names')" + + change_column :wiki_pages, :other_names, "text", using: "array_to_string(other_names, ' ')", default: nil + change_column :wiki_page_versions, :other_names, "text", using: "array_to_string(other_names, ' ')", default: nil + end + end +end diff --git a/db/structure.sql b/db/structure.sql index 7cdd77c0c..9227e739f 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -3363,7 +3363,7 @@ CREATE TABLE public.wiki_page_versions ( is_locked boolean NOT NULL, created_at timestamp without time zone, updated_at timestamp without time zone, - other_names text, + other_names text[] DEFAULT '{}'::text[], is_deleted boolean DEFAULT false NOT NULL ); @@ -3402,8 +3402,7 @@ CREATE TABLE public.wiki_pages ( created_at timestamp without time zone, updated_at timestamp without time zone, updater_id integer, - other_names text, - other_names_index tsvector, + other_names text[] DEFAULT '{}'::text[], is_deleted boolean DEFAULT false NOT NULL ); @@ -7312,10 +7311,10 @@ CREATE INDEX index_wiki_pages_on_body_index_index ON public.wiki_pages USING gin -- --- Name: index_wiki_pages_on_other_names_index; Type: INDEX; Schema: public; Owner: - +-- Name: index_wiki_pages_on_other_names; Type: INDEX; Schema: public; Owner: - -- -CREATE INDEX index_wiki_pages_on_other_names_index ON public.wiki_pages USING gin (other_names_index); +CREATE INDEX index_wiki_pages_on_other_names ON public.wiki_pages USING gin (other_names); -- @@ -7402,13 +7401,6 @@ CREATE TRIGGER trigger_posts_on_tag_index_update BEFORE INSERT OR UPDATE ON publ CREATE TRIGGER trigger_wiki_pages_on_update BEFORE INSERT OR UPDATE ON public.wiki_pages FOR EACH ROW EXECUTE PROCEDURE tsvector_update_trigger('body_index', 'public.danbooru', 'body', 'title'); --- --- Name: wiki_pages trigger_wiki_pages_on_update_for_other_names; Type: TRIGGER; Schema: public; Owner: - --- - -CREATE TRIGGER trigger_wiki_pages_on_update_for_other_names BEFORE INSERT OR UPDATE ON public.wiki_pages FOR EACH ROW EXECUTE PROCEDURE tsvector_update_trigger('other_names_index', 'public.danbooru', 'other_names'); - - -- -- PostgreSQL database dump complete -- @@ -7579,6 +7571,7 @@ INSERT INTO "schema_migrations" (version) VALUES ('20180913184128'), ('20180916002448'), ('20181108162204'), -('20181108205842'); +('20181108205842'), +('20181113174914'); diff --git a/test/unit/wiki_page_test.rb b/test/unit/wiki_page_test.rb index e37dac810..bcf1e5575 100644 --- a/test/unit/wiki_page_test.rb +++ b/test/unit/wiki_page_test.rb @@ -63,7 +63,7 @@ class WikiPageTest < ActiveSupport::TestCase should "normalize its other names" do @wiki_page.update(:other_names => "foo*bar baz baz 加賀（艦これ）") - assert(%w[foo*bar baz 加賀(艦これ)], @wiki_page.other_names_array) + assert_equal(%w[foo*bar baz 加賀(艦これ)], @wiki_page.other_names) end should "search by title" do From fe2698a011335ba874e3d4a983b3f69e3faefcf4 Mon Sep 17 00:00:00 2001 From: evazion Date: Wed, 14 Nov 2018 12:46:35 -0600 Subject: [PATCH 3/5] tag implications: convert descendant_names to array (#3987). --- app/models/tag_implication.rb | 14 ++--- ...dant_names_to_array_on_tag_implications.rb | 13 +++++ db/structure.sql | 5 +- test/unit/tag_implication_test.rb | 57 +++++++++---------- 4 files changed, 49 insertions(+), 40 deletions(-) create mode 100644 db/migrate/20181114180205_change_descendant_names_to_array_on_tag_implications.rb diff --git a/app/models/tag_implication.rb b/app/models/tag_implication.rb index 3a9dd506f..3f4c0f117 100644 --- a/app/models/tag_implication.rb +++ b/app/models/tag_implication.rb @@ -1,6 +1,8 @@ class TagImplication < TagRelationship extend Memoist + array_attribute :descendant_names + before_save :update_descendant_names after_save :update_descendant_names_for_parents after_destroy :update_descendant_names_for_parents @@ -22,7 +24,7 @@ class TagImplication < TagRelationship module ClassMethods # assumes names are normalized def with_descendants(names) - (names + active.where(antecedent_name: names).flat_map(&:descendant_names_array)).uniq + (names + active.where(antecedent_name: names).flat_map(&:descendant_names)).uniq end def automatic_tags_for(names) @@ -44,12 +46,8 @@ class TagImplication < TagRelationship end memoize :descendants - def descendant_names_array - descendant_names.split(/ /) - end - def update_descendant_names - self.descendant_names = descendants.join(" ") + self.descendant_names = descendants end def update_descendant_names! @@ -88,7 +86,7 @@ class TagImplication < TagRelationship 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) + implied_tags = implications.flat_map(&:descendant_names) if implied_tags.include?(consequent_name) self.errors[:base] << "#{antecedent_name} already implies #{consequent_name} through another implication" end @@ -170,7 +168,7 @@ class TagImplication < TagRelationship def update_posts Post.without_timeout do Post.raw_tag_match(antecedent_name).where("true /* TagImplication#update_posts */").find_each do |post| - fixed_tags = "#{post.tag_string} #{descendant_names}".strip + fixed_tags = "#{post.tag_string} #{descendant_names_string}".strip CurrentUser.scoped(creator, creator_ip_addr) do post.update_attributes( :tag_string => fixed_tags diff --git a/db/migrate/20181114180205_change_descendant_names_to_array_on_tag_implications.rb b/db/migrate/20181114180205_change_descendant_names_to_array_on_tag_implications.rb new file mode 100644 index 000000000..2719ac46c --- /dev/null +++ b/db/migrate/20181114180205_change_descendant_names_to_array_on_tag_implications.rb @@ -0,0 +1,13 @@ +class ChangeDescendantNamesToArrayOnTagImplications < ActiveRecord::Migration[5.2] + def up + TagImplication.without_timeout do + change_column :tag_implications, :descendant_names, "text[]", using: "string_to_array(descendant_names, ' ')::text[]", default: "{}" + end + end + + def down + TagImplication.without_timeout do + change_column :tag_implications, :descendant_names, "text", using: "array_to_string(descendant_names, ' ')", default: nil + end + end +end diff --git a/db/structure.sql b/db/structure.sql index 9227e739f..31a94f6d5 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -3012,7 +3012,7 @@ CREATE TABLE public.tag_implications ( id integer NOT NULL, antecedent_name character varying NOT NULL, consequent_name character varying NOT NULL, - descendant_names text NOT NULL, + descendant_names text[] DEFAULT '{}'::text[] NOT NULL, creator_id integer NOT NULL, creator_ip_addr inet NOT NULL, forum_topic_id integer, @@ -7572,6 +7572,7 @@ INSERT INTO "schema_migrations" (version) VALUES ('20180916002448'), ('20181108162204'), ('20181108205842'), -('20181113174914'); +('20181113174914'), +('20181114180205'); diff --git a/test/unit/tag_implication_test.rb b/test/unit/tag_implication_test.rb index b8ad95380..cdc243de9 100644 --- a/test/unit/tag_implication_test.rb +++ b/test/unit/tag_implication_test.rb @@ -46,7 +46,7 @@ class TagImplicationTest < ActiveSupport::TestCase 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("b", ti1.descendant_names) + assert_equal(%w[b], ti1.descendant_names) end should "populate the creator information" do @@ -89,14 +89,11 @@ class TagImplicationTest < ActiveSupport::TestCase should "calculate all its descendants" do ti1 = FactoryBot.create(:tag_implication, :antecedent_name => "bbb", :consequent_name => "ccc") - assert_equal("ccc", ti1.descendant_names) - assert_equal(["ccc"], ti1.descendant_names_array) + assert_equal(%w[ccc], ti1.descendant_names) ti2 = FactoryBot.create(:tag_implication, :antecedent_name => "aaa", :consequent_name => "bbb") - assert_equal("bbb ccc", ti2.descendant_names) - assert_equal(["bbb", "ccc"], ti2.descendant_names_array) + assert_equal(%w[bbb ccc], ti2.descendant_names) ti1.reload - assert_equal("ccc", ti1.descendant_names) - assert_equal(["ccc"], ti1.descendant_names_array) + assert_equal(%w[ccc], ti1.descendant_names) end should "update its descendants on save" do @@ -109,8 +106,8 @@ class TagImplicationTest < ActiveSupport::TestCase ) ti1.reload ti2.reload - assert_equal("bbb ddd", ti1.descendant_names) - assert_equal("ddd", ti2.descendant_names) + 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 @@ -122,49 +119,49 @@ class TagImplicationTest < ActiveSupport::TestCase ti2.reload ti3.reload ti4.reload - assert_equal("bbb ccc ddd", ti1.descendant_names) - assert_equal("bbb ccc ddd", ti2.descendant_names) - assert_equal("ccc ddd", ti3.descendant_names) - assert_equal("ddd", ti4.descendant_names) + 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("bbb", ti1.descendant_names) - assert_equal("bbb", ti2.descendant_names) - assert_equal("ddd", ti4.descendant_names) + 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("bbb", ti1.descendant_names) + 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("bbb ccc", ti1.descendant_names) - assert_equal("ccc", ti2.descendant_names) + 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("bbb ccc ddd", ti1.descendant_names) - assert_equal("ccc ddd", ti2.descendant_names) + 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("bbb ccc ddd eee", ti1.descendant_names) - assert_equal("ccc ddd eee", ti2.descendant_names) - assert_equal("ddd", ti3.descendant_names) - assert_equal("eee", ti4.descendant_names) + 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 @@ -172,11 +169,11 @@ class TagImplicationTest < ActiveSupport::TestCase ti3.reload ti4.reload ti5.reload - assert_equal("bbb ccc ddd eee", ti1.descendant_names) - assert_equal("ccc ddd eee", ti2.descendant_names) - assert_equal("ddd", ti3.descendant_names) - assert_equal("eee", ti4.descendant_names) - assert_equal("bbb ccc ddd eee", ti5.descendant_names) + 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 From 741462ae6828da03752fee5e01b118ffeb360be2 Mon Sep 17 00:00:00 2001 From: evazion Date: Wed, 14 Nov 2018 14:22:06 -0600 Subject: [PATCH 4/5] artist versions: convert other_names, url_string to arrays (#3987). --- app/models/artist.rb | 12 ++++++------ app/models/artist_version.rb | 19 +++++++------------ ...ge_strings_to_arrays_on_artist_versions.rb | 19 +++++++++++++++++++ db/structure.sql | 7 ++++--- test/unit/artist_test.rb | 12 ++++++------ 5 files changed, 42 insertions(+), 27 deletions(-) create mode 100644 db/migrate/20181114185032_change_strings_to_arrays_on_artist_versions.rb diff --git a/app/models/artist.rb b/app/models/artist.rb index af2b08ce8..327049170 100644 --- a/app/models/artist.rb +++ b/app/models/artist.rb @@ -174,11 +174,11 @@ class Artist < ApplicationRecord end def url_array - urls.map(&:to_s) + urls.map(&:to_s).sort end def url_string - url_array.sort.join("\n") + url_array.join("\n") end def url_string=(string) @@ -275,7 +275,7 @@ class Artist < ApplicationRecord :name => name, :updater_id => CurrentUser.id, :updater_ip_addr => CurrentUser.ip_addr, - :url_string => url_string, + :urls => url_array, :is_active => is_active, :is_banned => is_banned, :other_names => other_names, @@ -287,7 +287,7 @@ class Artist < ApplicationRecord prev = versions.last prev.update_attributes( :name => name, - :url_string => url_string, + :urls => url_array, :is_active => is_active, :is_banned => is_banned, :other_names => other_names, @@ -306,9 +306,9 @@ class Artist < ApplicationRecord end self.name = version.name - self.url_string = version.url_string + self.url_string = version.urls.join("\n") self.is_active = version.is_active - self.other_names = version.other_names + self.other_names = version.other_names.join(" ") self.group_name = version.group_name save end diff --git a/app/models/artist_version.rb b/app/models/artist_version.rb index 3bfa6c7f1..029b20546 100644 --- a/app/models/artist_version.rb +++ b/app/models/artist_version.rb @@ -1,4 +1,7 @@ class ArtistVersion < ApplicationRecord + array_attribute :urls + array_attribute :other_names + belongs_to_updater belongs_to :artist delegate :visible?, :to => :artist @@ -47,18 +50,10 @@ class ArtistVersion < ApplicationRecord extend SearchMethods - def url_array - url_string.to_s.split(/[[:space:]]+/) - end - - def other_names_array - other_names.to_s.split(/[[:space:]]+/) - end - def urls_diff(version) latest_urls = artist.url_array || [] - new_urls = url_array - old_urls = version.present? ? version.url_array : [] + new_urls = urls + old_urls = version.present? ? version.urls : [] added_urls = new_urls - old_urls removed_urls = old_urls - new_urls @@ -74,8 +69,8 @@ class ArtistVersion < ApplicationRecord def other_names_diff(version) latest_names = artist.other_names_array || [] - new_names = other_names_array - old_names = version.present? ? version.other_names_array : [] + new_names = other_names + old_names = version.present? ? version.other_names : [] added_names = new_names - old_names removed_names = old_names - new_names diff --git a/db/migrate/20181114185032_change_strings_to_arrays_on_artist_versions.rb b/db/migrate/20181114185032_change_strings_to_arrays_on_artist_versions.rb new file mode 100644 index 000000000..c0a696a8e --- /dev/null +++ b/db/migrate/20181114185032_change_strings_to_arrays_on_artist_versions.rb @@ -0,0 +1,19 @@ +class ChangeStringsToArraysOnArtistVersions < ActiveRecord::Migration[5.2] + def up + ArtistVersion.without_timeout do + change_column :artist_versions, :other_names, "text[]", using: "array_remove(regexp_split_to_array(other_names, '\\s+'), '')", default: "{}" + + change_column :artist_versions, :url_string, "text[]", using: "array_remove(regexp_split_to_array(url_string, '\\s+'), '')", default: "{}" + rename_column :artist_versions, :url_string, :urls + end + end + + def down + ArtistVersion.without_timeout do + rename_column :artist_versions, :urls, :url_string + change_column :artist_versions, :url_string, "text", using: "array_to_string(url_string, '\\n')", default: nil + + change_column :artist_versions, :other_names, "text", using: "array_to_string(other_names, ' ')", default: nil + end + end +end diff --git a/db/structure.sql b/db/structure.sql index 31a94f6d5..97d96fbaf 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -726,9 +726,9 @@ CREATE TABLE public.artist_versions ( updater_id integer NOT NULL, updater_ip_addr inet NOT NULL, is_active boolean DEFAULT true NOT NULL, - other_names text, + other_names text[] DEFAULT '{}'::text[], group_name character varying, - url_string text, + urls text[] DEFAULT '{}'::text[], is_banned boolean DEFAULT false NOT NULL, created_at timestamp without time zone, updated_at timestamp without time zone @@ -7573,6 +7573,7 @@ INSERT INTO "schema_migrations" (version) VALUES ('20181108162204'), ('20181108205842'), ('20181113174914'), -('20181114180205'); +('20181114180205'), +('20181114185032'); diff --git a/test/unit/artist_test.rb b/test/unit/artist_test.rb index e7559022d..bd40c45ae 100644 --- a/test/unit/artist_test.rb +++ b/test/unit/artist_test.rb @@ -475,7 +475,7 @@ class ArtistTest < ActiveSupport::TestCase end first_version = ArtistVersion.first - assert_equal("yyy", first_version.other_names) + assert_equal(%w[yyy], first_version.other_names) artist.revert_to!(first_version) artist.reload assert_equal("yyy", artist.other_names) @@ -506,35 +506,35 @@ class ArtistTest < ActiveSupport::TestCase should "create a new version when an url is added" do assert_difference("ArtistVersion.count") do @artist.update(:url_string => "http://foo.com http://bar.com") - assert_equal(%w[http://bar.com http://foo.com], @artist.versions.last.url_array) + assert_equal(%w[http://bar.com http://foo.com], @artist.versions.last.urls) end end should "create a new version when an url is removed" do assert_difference("ArtistVersion.count") do @artist.update(:url_string => "") - assert_equal(%w[], @artist.versions.last.url_array) + assert_equal(%w[], @artist.versions.last.urls) end end should "create a new version when an url is marked inactive" do assert_difference("ArtistVersion.count") do @artist.update(:url_string => "-http://foo.com") - assert_equal(%w[-http://foo.com], @artist.versions.last.url_array) + assert_equal(%w[-http://foo.com], @artist.versions.last.urls) end end should "not create a new version when nothing has changed" do assert_no_difference("ArtistVersion.count") do @artist.save - assert_equal(%w[http://foo.com], @artist.versions.last.url_array) + assert_equal(%w[http://foo.com], @artist.versions.last.urls) end end should "not save invalid urls" do assert_no_difference("ArtistVersion.count") do @artist.update(:url_string => "http://foo.com www.example.com") - assert_equal(%w[http://foo.com], @artist.versions.last.url_array) + assert_equal(%w[http://foo.com], @artist.versions.last.urls) end end end From 41ff05c12196598f903e1c30d5d6b59911edb4ad Mon Sep 17 00:00:00 2001 From: evazion Date: Wed, 14 Nov 2018 15:54:51 -0600 Subject: [PATCH 5/5] artists: convert other_names to array (#3987). --- app/controllers/artists_controller.rb | 2 +- .../src/styles/specific/artists.scss | 2 +- app/models/artist.rb | 40 +++++++++---------- app/models/artist_version.rb | 2 +- app/views/artists/_form.html.erb | 2 +- app/views/artists/_summary.html.erb | 2 +- app/views/artists/index.html.erb | 2 +- ..._change_other_names_to_array_on_artists.rb | 23 +++++++++++ db/structure.sql | 24 +++-------- test/unit/artist_test.rb | 14 +++---- 10 files changed, 60 insertions(+), 53 deletions(-) create mode 100644 db/migrate/20181114202744_change_other_names_to_array_on_artists.rb diff --git a/app/controllers/artists_controller.rb b/app/controllers/artists_controller.rb index 21deaee0d..b06226798 100644 --- a/app/controllers/artists_controller.rb +++ b/app/controllers/artists_controller.rb @@ -105,7 +105,7 @@ private end def artist_params - permitted_params = %i[name other_names other_names_comma group_name url_string notes] + permitted_params = %i[name other_names other_names_string group_name url_string notes] permitted_params << :is_active if CurrentUser.is_builder? params.fetch(:artist, {}).permit(permitted_params) diff --git a/app/javascript/src/styles/specific/artists.scss b/app/javascript/src/styles/specific/artists.scss index 0d82f9466..1149ef5ce 100644 --- a/app/javascript/src/styles/specific/artists.scss +++ b/app/javascript/src/styles/specific/artists.scss @@ -14,7 +14,7 @@ div#c-artists, div#excerpt { textarea { height: 10em; - &#artist_other_names_comma { + &#artist_other_names_string { height: 3em; } } diff --git a/app/models/artist.rb b/app/models/artist.rb index 327049170..509d09c1b 100644 --- a/app/models/artist.rb +++ b/app/models/artist.rb @@ -3,8 +3,10 @@ class Artist < ApplicationRecord class RevertError < Exception ; end attr_accessor :url_string_changed + array_attribute :other_names before_validation :normalize_name + before_validation :normalize_other_names after_save :create_version after_save :categorize_tag after_save :update_wiki @@ -239,16 +241,8 @@ class Artist < ApplicationRecord name.tr("_", " ") end - def other_names_array - other_names.try(:split, /[[:space:]]+/) - end - - def other_names_comma - other_names_array.try(:join, ", ") - end - - def other_names_comma=(string) - self.other_names = string.split(/,/).map {|x| Artist.normalize_name(x)}.join(" ") + def normalize_other_names + self.other_names = other_names.map { |x| Artist.normalize_name(x) } end end @@ -308,7 +302,7 @@ class Artist < ApplicationRecord self.name = version.name self.url_string = version.urls.join("\n") self.is_active = version.is_active - self.other_names = version.other_names.join(" ") + self.other_names = version.other_names self.group_name = version.group_name save end @@ -466,13 +460,21 @@ class Artist < ApplicationRecord where(name: normalize_name(name)) end + def any_other_name_matches(regex) + where(id: Artist.from("unnest(other_names) AS other_name").where("other_name ~ ?", regex)) + end + + def any_other_name_like(name) + where(id: Artist.from("unnest(other_names) AS other_name").where("other_name LIKE ?", name.to_escaped_for_sql_like)) + end + def any_name_matches(query) if query =~ %r!\A/(.*)/\z! - where_regex(:name, $1).or(where_regex(:other_names, $1)).or(where_regex(:group_name, $1)) + where_regex(:name, $1).or(any_other_name_matches($1)).or(where_regex(:group_name, $1)) else normalized_name = normalize_name(query) normalized_name = "*#{normalized_name}*" unless normalized_name.include?("*") - where_like(:name, normalized_name).or(where_like(:other_names, normalized_name)).or(where_like(:group_name, normalized_name)) + where_like(:name, normalized_name).or(any_other_name_like(normalized_name)).or(where_like(:group_name, normalized_name)) end end @@ -492,9 +494,12 @@ class Artist < ApplicationRecord q = super q = q.search_text_attribute(:name, params) - q = q.search_text_attribute(:other_names, params) q = q.search_text_attribute(:group_name, params) + if params[:any_other_name_like] + q = q.any_other_name_like(params[:any_other_name_like]) + end + if params[:any_name_matches].present? q = q.any_name_matches(params[:any_name_matches]) end @@ -536,12 +541,6 @@ class Artist < ApplicationRecord end end - module ApiMethods - def hidden_attributes - super + [:other_names_index] - end - end - include UrlMethods include NameMethods include GroupMethods @@ -551,7 +550,6 @@ class Artist < ApplicationRecord include TagMethods include BanMethods extend SearchMethods - include ApiMethods def status if is_banned? && is_active? diff --git a/app/models/artist_version.rb b/app/models/artist_version.rb index 029b20546..2d116114a 100644 --- a/app/models/artist_version.rb +++ b/app/models/artist_version.rb @@ -68,7 +68,7 @@ class ArtistVersion < ApplicationRecord end def other_names_diff(version) - latest_names = artist.other_names_array || [] + latest_names = artist.other_names || [] new_names = other_names old_names = version.present? ? version.other_names : [] diff --git a/app/views/artists/_form.html.erb b/app/views/artists/_form.html.erb index 8dadd1fbe..7ae80753f 100644 --- a/app/views/artists/_form.html.erb +++ b/app/views/artists/_form.html.erb @@ -14,7 +14,7 @@

<%= @artist.name %>

<% end %> - <%= f.input :other_names_comma, :hint => "Separate with commas", :as => :text, :label => "Other names" %> + <%= f.input :other_names_string, :hint => "Separate with spaces", :as => :text, :label => "Other names" %> <%= f.input :group_name %> <%= f.input :url_string, :label => "URLs", :as => :text, :input_html => {:size => "50x5", :value => params.dig(:artist, :url_string) || @artist.urls.join("\n")}, :hint => "You can prefix a URL with - to mark it as dead." %> diff --git a/app/views/artists/_summary.html.erb b/app/views/artists/_summary.html.erb index e9eb006d3..9c50f7561 100644 --- a/app/views/artists/_summary.html.erb +++ b/app/views/artists/_summary.html.erb @@ -7,7 +7,7 @@
  • Tag Alias <%= artist.tag_alias_name %>
    • <% end %> <% if artist.other_names.present? %> -
  • Other Names <%= link_to_artists(artist.other_names.split(/ /)) %>
    • +
  • Other Names <%= link_to_artists(artist.other_names) %>
    • <% end %> <% if artist.group_name.present? %>
  • Group <%= link_to_artist(artist.group_name) %>
    • diff --git a/app/views/artists/index.html.erb b/app/views/artists/index.html.erb index 66c74fa14..2b8eca9ba 100644 --- a/app/views/artists/index.html.erb +++ b/app/views/artists/index.html.erb @@ -19,7 +19,7 @@ (group: <%= link_to(artist.group_name, artist_path(artist)) %>) <% end %> - <%= artist.other_names %> + <%= artist.other_names_string %> <%= artist.status %> <% end %> <% end %> diff --git a/db/migrate/20181114202744_change_other_names_to_array_on_artists.rb b/db/migrate/20181114202744_change_other_names_to_array_on_artists.rb new file mode 100644 index 000000000..a56183c4f --- /dev/null +++ b/db/migrate/20181114202744_change_other_names_to_array_on_artists.rb @@ -0,0 +1,23 @@ +class ChangeOtherNamesToArrayOnArtists < ActiveRecord::Migration[5.2] + def up + Artist.without_timeout do + remove_index :artists, name: "index_artists_on_other_names_trgm" + change_column :artists, :other_names, "text[]", using: "array_remove(regexp_split_to_array(other_names, '\\s+'), '')", default: "{}" + add_index :artists, :other_names, using: :gin + + remove_column :artists, :other_names_index + execute "DROP TRIGGER trigger_artists_on_update ON artists" + end + end + + def down + Artist.without_timeout do + remove_index :artists, :other_names + change_column :artists, :other_names, "text", using: "array_to_string(other_names, ' ')", default: nil + add_index :artists, :other_names, name: "index_artists_on_other_names_trgm", using: :gin, opclass: :gin_trgm_ops + + add_column :artists, :other_names_index, :tsvector + execute "CREATE TRIGGER trigger_artists_on_update BEFORE INSERT OR UPDATE ON artists FOR EACH ROW EXECUTE PROCEDURE tsvector_update_trigger('other_names_index', 'public.danbooru', 'other_names')" + end + end +end diff --git a/db/structure.sql b/db/structure.sql index 97d96fbaf..81291c3c1 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -765,8 +765,7 @@ CREATE TABLE public.artists ( creator_id integer NOT NULL, is_active boolean DEFAULT true NOT NULL, is_banned boolean DEFAULT false NOT NULL, - other_names text, - other_names_index tsvector, + other_names text[] DEFAULT '{}'::text[], group_name character varying, created_at timestamp without time zone, updated_at timestamp without time zone @@ -5078,17 +5077,10 @@ CREATE INDEX index_artists_on_name_trgm ON public.artists USING gin (name public -- --- Name: index_artists_on_other_names_index; Type: INDEX; Schema: public; Owner: - +-- Name: index_artists_on_other_names; Type: INDEX; Schema: public; Owner: - -- -CREATE INDEX index_artists_on_other_names_index ON public.artists USING gin (other_names_index); - - --- --- Name: index_artists_on_other_names_trgm; Type: INDEX; Schema: public; Owner: - --- - -CREATE INDEX index_artists_on_other_names_trgm ON public.artists USING gin (other_names public.gin_trgm_ops); +CREATE INDEX index_artists_on_other_names ON public.artists USING gin (other_names); -- @@ -7345,13 +7337,6 @@ CREATE INDEX index_wiki_pages_on_updated_at ON public.wiki_pages USING btree (up CREATE TRIGGER insert_favorites_trigger BEFORE INSERT ON public.favorites FOR EACH ROW EXECUTE PROCEDURE public.favorites_insert_trigger(); --- --- Name: artists trigger_artists_on_update; Type: TRIGGER; Schema: public; Owner: - --- - -CREATE TRIGGER trigger_artists_on_update BEFORE INSERT OR UPDATE ON public.artists FOR EACH ROW EXECUTE PROCEDURE tsvector_update_trigger('other_names_index', 'public.danbooru', 'other_names'); - - -- -- Name: comments trigger_comments_on_update; Type: TRIGGER; Schema: public; Owner: - -- @@ -7574,6 +7559,7 @@ INSERT INTO "schema_migrations" (version) VALUES ('20181108205842'), ('20181113174914'), ('20181114180205'), -('20181114185032'); +('20181114185032'), +('20181114202744'); diff --git a/test/unit/artist_test.rb b/test/unit/artist_test.rb index bd40c45ae..76117910d 100644 --- a/test/unit/artist_test.rb +++ b/test/unit/artist_test.rb @@ -407,8 +407,8 @@ class ArtistTest < ActiveSupport::TestCase end should "normalize its other names" do - artist = FactoryBot.create(:artist, :name => "a1", :other_names_comma => "aaa, bbb, ccc ddd") - assert_equal("aaa, bbb, ccc_ddd", artist.other_names_comma) + artist = FactoryBot.create(:artist, :name => "a1", :other_names => "aaa bbb ccc_ddd") + assert_equal("aaa bbb ccc_ddd", artist.other_names_string) end should "search on its name should return results" do @@ -421,11 +421,11 @@ class ArtistTest < ActiveSupport::TestCase end should "search on other names should return matches" do - artist = FactoryBot.create(:artist, :name => "artist", :other_names_comma => "aaa, ccc ddd") + artist = FactoryBot.create(:artist, :name => "artist", :other_names_string => "aaa ccc_ddd") - assert_nil(Artist.search(other_names_like: "*artist*").first) - assert_not_nil(Artist.search(other_names_like: "*aaa*").first) - assert_not_nil(Artist.search(other_names_like: "*ccc_ddd*").first) + assert_nil(Artist.search(any_other_name_like: "*artist*").first) + assert_not_nil(Artist.search(any_other_name_like: "*aaa*").first) + assert_not_nil(Artist.search(any_other_name_like: "*ccc_ddd*").first) assert_not_nil(Artist.search(name: "artist").first) assert_not_nil(Artist.search(:any_name_matches => "aaa").first) assert_not_nil(Artist.search(:any_name_matches => "/a/").first) @@ -478,7 +478,7 @@ class ArtistTest < ActiveSupport::TestCase assert_equal(%w[yyy], first_version.other_names) artist.revert_to!(first_version) artist.reload - assert_equal("yyy", artist.other_names) + assert_equal(%w[yyy], artist.other_names) end should "update the category of the tag when created" do