From 91d74693d3623a144000bc27acc2c410cd50007d Mon Sep 17 00:00:00 2001 From: Radhames Brito Date: Wed, 3 May 2017 07:06:01 -0400 Subject: [PATCH 01/10] Added test to non unique create retry --- lib/acts_as_taggable_on/tag.rb | 2 +- spec/acts_as_taggable_on/tag_spec.rb | 11 +++++++++++ 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/lib/acts_as_taggable_on/tag.rb b/lib/acts_as_taggable_on/tag.rb index 05f2a6773..870ee4cb0 100644 --- a/lib/acts_as_taggable_on/tag.rb +++ b/lib/acts_as_taggable_on/tag.rb @@ -78,7 +78,7 @@ def self.find_or_create_all_with_like_by_name(*list) existing_tag = existing_tags.find { |tag| comparable_name(tag.name) == comparable_tag_name } existing_tag || create(name: tag_name) rescue ActiveRecord::RecordNotUnique - if (tries -= 1).positive? + unless (tries -= 1).negative? ActiveRecord::Base.connection.execute 'ROLLBACK' retry end diff --git a/spec/acts_as_taggable_on/tag_spec.rb b/spec/acts_as_taggable_on/tag_spec.rb index 392e0f929..e74332e14 100644 --- a/spec/acts_as_taggable_on/tag_spec.rb +++ b/spec/acts_as_taggable_on/tag_spec.rb @@ -167,6 +167,17 @@ it 'should return an empty array if no tags are specified' do expect(ActsAsTaggableOn::Tag.find_or_create_all_with_like_by_name([])).to be_empty end + + context 'retry 3 times on not unique exception' do + it 'performs 3 tries before raising the exception' do + allow(ActsAsTaggableOn::Tag).to receive(:named_any).and_raise(ActiveRecord::RecordNotUnique) # trigger error inside block + expect(ActiveRecord::Base.connection).to receive(:execute).with('ROLLBACK').exactly(3).times + + expect { + ActsAsTaggableOn::Tag.find_or_create_all_with_like_by_name('AWESOME', 'awesome') + }.to raise_error ActsAsTaggableOn::DuplicateTagError + end + end end it 'should require a name' do From 375afcfbcf514a6d2bc930bf1800ccbbdc286475 Mon Sep 17 00:00:00 2001 From: Radhames Brito Date: Wed, 3 May 2017 10:02:52 -0400 Subject: [PATCH 02/10] add message to exceptions for preview AR versions --- spec/acts_as_taggable_on/tag_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/acts_as_taggable_on/tag_spec.rb b/spec/acts_as_taggable_on/tag_spec.rb index e74332e14..469e6bf79 100644 --- a/spec/acts_as_taggable_on/tag_spec.rb +++ b/spec/acts_as_taggable_on/tag_spec.rb @@ -170,7 +170,7 @@ context 'retry 3 times on not unique exception' do it 'performs 3 tries before raising the exception' do - allow(ActsAsTaggableOn::Tag).to receive(:named_any).and_raise(ActiveRecord::RecordNotUnique) # trigger error inside block + allow(ActsAsTaggableOn::Tag).to receive(:named_any).and_raise(ActiveRecord::RecordNotUnique.new('error')) # trigger error inside block expect(ActiveRecord::Base.connection).to receive(:execute).with('ROLLBACK').exactly(3).times expect { From adb67e0e9d9d88df99b1fdf1c9e8e9f027b5b3d8 Mon Sep 17 00:00:00 2001 From: Radhames Brito Date: Tue, 9 May 2017 08:10:23 -0400 Subject: [PATCH 03/10] Rewrite Core#tagged_with. --- lib/acts-as-taggable-on.rb | 1 + lib/acts_as_taggable_on/taggable/core.rb | 142 +----------------- .../taggable/tagged_with_query.rb | 16 ++ .../tagged_with_query/all_tags_query.rb | 108 +++++++++++++ .../tagged_with_query/any_tags_query.rb | 64 ++++++++ .../tagged_with_query/exclude_tags_query.rb | 82 ++++++++++ .../taggable/tagged_with_query/query_base.rb | 40 +++++ lib/acts_as_taggable_on/tagging.rb | 5 +- spec/acts_as_taggable_on/taggable_spec.rb | 4 +- 9 files changed, 320 insertions(+), 142 deletions(-) create mode 100644 lib/acts_as_taggable_on/taggable/tagged_with_query.rb create mode 100644 lib/acts_as_taggable_on/taggable/tagged_with_query/all_tags_query.rb create mode 100644 lib/acts_as_taggable_on/taggable/tagged_with_query/any_tags_query.rb create mode 100644 lib/acts_as_taggable_on/taggable/tagged_with_query/exclude_tags_query.rb create mode 100644 lib/acts_as_taggable_on/taggable/tagged_with_query/query_base.rb diff --git a/lib/acts-as-taggable-on.rb b/lib/acts-as-taggable-on.rb index 8e92d0cc8..da52925d5 100644 --- a/lib/acts-as-taggable-on.rb +++ b/lib/acts-as-taggable-on.rb @@ -2,6 +2,7 @@ require 'active_record/version' require 'active_support/core_ext/module' + begin require 'rails/engine' require 'acts_as_taggable_on/engine' diff --git a/lib/acts_as_taggable_on/taggable/core.rb b/lib/acts_as_taggable_on/taggable/core.rb index 6f02663bf..d6f25d7b3 100644 --- a/lib/acts_as_taggable_on/taggable/core.rb +++ b/lib/acts_as_taggable_on/taggable/core.rb @@ -1,3 +1,5 @@ +require_relative 'tagged_with_query' + module ActsAsTaggableOn::Taggable module Core def self.included(base) @@ -88,145 +90,7 @@ def tagged_with(tags, options = {}) return empty_result if tag_list.empty? - joins = [] - conditions = [] - having = [] - select_clause = [] - order_by = [] - - context = options.delete(:on) - owned_by = options.delete(:owned_by) - alias_base_name = undecorated_table_name.gsub('.', '_') - # FIXME use ActiveRecord's connection quote_column_name - quote = ActsAsTaggableOn::Utils.using_postgresql? ? '"' : '' - - if options.delete(:exclude) - if options.delete(:wild) - tags_conditions = tag_list.map { |t| sanitize_sql(["#{ActsAsTaggableOn::Tag.table_name}.name #{ActsAsTaggableOn::Utils.like_operator} ? ESCAPE '!'", "%#{ActsAsTaggableOn::Utils.escape_like(t)}%"]) }.join(' OR ') - else - tags_conditions = tag_list.map { |t| sanitize_sql(["#{ActsAsTaggableOn::Tag.table_name}.name #{ActsAsTaggableOn::Utils.like_operator} ?", t]) }.join(' OR ') - end - - conditions << "#{table_name}.#{primary_key} NOT IN (SELECT #{ActsAsTaggableOn::Tagging.table_name}.taggable_id FROM #{ActsAsTaggableOn::Tagging.table_name} JOIN #{ActsAsTaggableOn::Tag.table_name} ON #{ActsAsTaggableOn::Tagging.table_name}.tag_id = #{ActsAsTaggableOn::Tag.table_name}.#{ActsAsTaggableOn::Tag.primary_key} AND (#{tags_conditions}) WHERE #{ActsAsTaggableOn::Tagging.table_name}.taggable_type = #{self.connection.quote(base_class.name)})" - - if owned_by - joins << "JOIN #{ActsAsTaggableOn::Tagging.table_name}" + - " ON #{ActsAsTaggableOn::Tagging.table_name}.taggable_id = #{quote}#{table_name}#{quote}.#{primary_key}" + - " AND #{ActsAsTaggableOn::Tagging.table_name}.taggable_type = #{self.connection.quote(base_class.name)}" + - " AND #{ActsAsTaggableOn::Tagging.table_name}.tagger_id = #{self.connection.quote(owned_by.id)}" + - " AND #{ActsAsTaggableOn::Tagging.table_name}.tagger_type = #{self.connection.quote(owned_by.class.base_class.to_s)}" - - joins << " AND " + sanitize_sql(["#{ActsAsTaggableOn::Tagging.table_name}.created_at >= ?", options.delete(:start_at)]) if options[:start_at] - joins << " AND " + sanitize_sql(["#{ActsAsTaggableOn::Tagging.table_name}.created_at <= ?", options.delete(:end_at)]) if options[:end_at] - end - - elsif options.delete(:any) - # get tags, drop out if nothing returned (we need at least one) - tags = if options.delete(:wild) - ActsAsTaggableOn::Tag.named_like_any(tag_list) - else - ActsAsTaggableOn::Tag.named_any(tag_list) - end - - return empty_result if tags.length == 0 - - # setup taggings alias so we can chain, ex: items_locations_taggings_awesome_cool_123 - # avoid ambiguous column name - taggings_context = context ? "_#{context}" : '' - - taggings_alias = adjust_taggings_alias( - "#{alias_base_name[0..4]}#{taggings_context[0..6]}_taggings_#{ActsAsTaggableOn::Utils.sha_prefix(tags.map(&:name).join('_'))}" - ) - - tagging_cond = "#{ActsAsTaggableOn::Tagging.table_name} #{taggings_alias}" + - " WHERE #{taggings_alias}.taggable_id = #{quote}#{table_name}#{quote}.#{primary_key}" + - " AND #{taggings_alias}.taggable_type = #{self.connection.quote(base_class.name)}" - - tagging_cond << " AND " + sanitize_sql(["#{taggings_alias}.created_at >= ?", options.delete(:start_at)]) if options[:start_at] - tagging_cond << " AND " + sanitize_sql(["#{taggings_alias}.created_at <= ?", options.delete(:end_at)]) if options[:end_at] - - tagging_cond << " AND " + sanitize_sql(["#{taggings_alias}.context = ?", context.to_s]) if context - - # don't need to sanitize sql, map all ids and join with OR logic - tag_ids = tags.map { |t| self.connection.quote(t.id) }.join(', ') - tagging_cond << " AND #{taggings_alias}.tag_id in (#{tag_ids})" - select_clause << " #{table_name}.*" unless context and tag_types.one? - - if owned_by - tagging_cond << ' AND ' + - sanitize_sql([ - "#{taggings_alias}.tagger_id = ? AND #{taggings_alias}.tagger_type = ?", - owned_by.id, - owned_by.class.base_class.to_s - ]) - end - - conditions << "EXISTS (SELECT 1 FROM #{tagging_cond})" - if options.delete(:order_by_matching_tag_count) - order_by << "(SELECT count(*) FROM #{tagging_cond}) desc" - end - else - tags = ActsAsTaggableOn::Tag.named_any(tag_list) - - return empty_result unless tags.length == tag_list.length - - tags.each do |tag| - taggings_alias = adjust_taggings_alias("#{alias_base_name[0..11]}_taggings_#{ActsAsTaggableOn::Utils.sha_prefix(tag.name)}") - tagging_join = "JOIN #{ActsAsTaggableOn::Tagging.table_name} #{taggings_alias}" \ - " ON #{taggings_alias}.taggable_id = #{quote}#{table_name}#{quote}.#{primary_key}" + - " AND #{taggings_alias}.taggable_type = #{self.connection.quote(base_class.name)}" + - " AND #{taggings_alias}.tag_id = #{self.connection.quote(tag.id)}" - - tagging_join << " AND " + sanitize_sql(["#{taggings_alias}.created_at >= ?", options.delete(:start_at)]) if options[:start_at] - tagging_join << " AND " + sanitize_sql(["#{taggings_alias}.created_at <= ?", options.delete(:end_at)]) if options[:end_at] - - tagging_join << " AND " + sanitize_sql(["#{taggings_alias}.context = ?", context.to_s]) if context - - if owned_by - tagging_join << ' AND ' + - sanitize_sql([ - "#{taggings_alias}.tagger_id = ? AND #{taggings_alias}.tagger_type = ?", - owned_by.id, - owned_by.class.base_class.to_s - ]) - end - - joins << tagging_join - end - end - - group ||= [] # Rails interprets this as a no-op in the group() call below - if options.delete(:order_by_matching_tag_count) - select_clause << "#{table_name}.*, COUNT(#{taggings_alias}.tag_id) AS #{taggings_alias}_count" - group_columns = ActsAsTaggableOn::Utils.using_postgresql? ? grouped_column_names_for(self) : "#{table_name}.#{primary_key}" - group = group_columns - order_by << "#{taggings_alias}_count DESC" - - elsif options.delete(:match_all) - taggings_alias, _ = adjust_taggings_alias("#{alias_base_name}_taggings_group"), "#{alias_base_name}_tags_group" - joins << "LEFT OUTER JOIN #{ActsAsTaggableOn::Tagging.table_name} #{taggings_alias}" \ - " ON #{taggings_alias}.taggable_id = #{quote}#{table_name}#{quote}.#{primary_key}" \ - " AND #{taggings_alias}.taggable_type = #{self.connection.quote(base_class.name)}" - - joins << " AND " + sanitize_sql(["#{taggings_alias}.context = ?", context.to_s]) if context - joins << " AND " + sanitize_sql(["#{ActsAsTaggableOn::Tagging.table_name}.created_at >= ?", options.delete(:start_at)]) if options[:start_at] - joins << " AND " + sanitize_sql(["#{ActsAsTaggableOn::Tagging.table_name}.created_at <= ?", options.delete(:end_at)]) if options[:end_at] - - group_columns = ActsAsTaggableOn::Utils.using_postgresql? ? grouped_column_names_for(self) : "#{table_name}.#{primary_key}" - group = group_columns - having = "COUNT(#{taggings_alias}.taggable_id) = #{tags.size}" - end - - order_by << options[:order] if options[:order].present? - - query = self - query = self.select(select_clause.join(',')) unless select_clause.empty? - query.joins(joins.join(' ')) - .where(conditions.join(' AND ')) - .group(group) - .having(having) - .order(order_by.join(', ')) - .readonly(false) + ::ActsAsTaggableOn::Taggable::TaggedWithQuery.build(self, ActsAsTaggableOn::Tag, ActsAsTaggableOn::Tagging, tag_list, options) end def is_taggable? diff --git a/lib/acts_as_taggable_on/taggable/tagged_with_query.rb b/lib/acts_as_taggable_on/taggable/tagged_with_query.rb new file mode 100644 index 000000000..a2b2613c6 --- /dev/null +++ b/lib/acts_as_taggable_on/taggable/tagged_with_query.rb @@ -0,0 +1,16 @@ +require_relative 'tagged_with_query/query_base' +require_relative 'tagged_with_query/exclude_tags_query' +require_relative 'tagged_with_query/any_tags_query' +require_relative 'tagged_with_query/all_tags_query' + +module ActsAsTaggableOn::Taggable::TaggedWithQuery + def self.build(taggable_model, tag_model, tagging_model, tag_list, options) + if options[:exclude].present? + ExcludeTagsQuery.new(taggable_model, tag_model, tagging_model, tag_list, options).build + elsif options[:any].present? + AnyTagsQuery.new(taggable_model, tag_model, tagging_model, tag_list, options).build + else + AllTagsQuery.new(taggable_model, tag_model, tagging_model, tag_list, options).build + end + end +end diff --git a/lib/acts_as_taggable_on/taggable/tagged_with_query/all_tags_query.rb b/lib/acts_as_taggable_on/taggable/tagged_with_query/all_tags_query.rb new file mode 100644 index 000000000..79b6406f7 --- /dev/null +++ b/lib/acts_as_taggable_on/taggable/tagged_with_query/all_tags_query.rb @@ -0,0 +1,108 @@ +module ActsAsTaggableOn::Taggable::TaggedWithQuery + class AllTagsQuery < QueryBase + def build + taggable_model.joins(each_tag_in_list) + .group(by_taggable) + .having(tags_that_matches_count) + .order(order_conditions) + .readonly(false) + end + + private + + def each_tag_in_list + arel_join = taggable_arel_table + + tag_list.each do |tag| + tagging_alias = tagging_arel_table.alias("#{tag}_taggings_#{SecureRandom.hex(10)}") + arel_join = arel_join + .join(tagging_alias) + .on(on_conditions(tag, tagging_alias)) + end + + if options[:match_all].present? + arel_join = arel_join + .join(tagging_arel_table, Arel::Nodes::OuterJoin) + .on( + match_all_on_conditions + ) + end + + return arel_join.join_sources + end + + def on_conditions(tag, tagging_alias) + on_condition = tagging_alias[:taggable_id].eq(taggable_arel_table[taggable_model.primary_key]) + .and(tagging_alias[:taggable_type].eq(taggable_model.base_class.name)) + .and( + tagging_alias[:tag_id].in( + tag_arel_table.project(tag_arel_table[:id]).where(tag_match_type(tag)) + ) + ) + + if options[:start_at].present? + on_condition = on_condition.and(tagging_alias[:created_at].gteq(options[:start_at])) + end + + if options[:end_at].present? + on_condition = on_condition.and(tagging_alias[:created_at].lteq(options[:end_at])) + end + + if options[:on].present? + on_condition = on_condition.and(tagging_alias[:context].lteq(options[:on])) + end + + if (owner = options[:owned_by]).present? + owner_table = owner.class.base_class.arel_table + + on_condition = on_condition.and(tagging_alias[:tagger_id].eq(owner.id)) + .and(tagging_alias[:tagger_type].eq(owner.class.base_class.to_s)) + end + + on_condition + end + + def match_all_on_conditions + on_condition = tagging_arel_table[:taggable_id].eq(taggable_arel_table[taggable_model.primary_key]) + .and(tagging_arel_table[:taggable_type].eq(taggable_model.base_class.name)) + + if options[:start_at].present? + on_condition = on_condition.and(tagging_arel_table[:created_at].gteq(options[:start_at])) + end + + if options[:end_at].present? + on_condition = on_condition.and(tagging_arel_table[:created_at].lteq(options[:end_at])) + end + + if options[:on].present? + on_condition = on_condition.and(tagging_arel_table[:context].lteq(options[:on])) + end + + on_condition + end + + def by_taggable + return [] unless options[:match_all].present? + + taggable_arel_table[taggable_model.primary_key] + end + + def tags_that_matches_count + return [] unless options[:match_all].present? + + taggable_model.find_by_sql(tag_arel_table.project(Arel.star.count).where(tags_match_type).to_sql) + + tagging_arel_table[:taggable_id].count.eq( + tag_arel_table.project(Arel.star.count).where(tags_match_type) + ) + end + + def order_conditions + order_by = [] + order_by << tagging_arel_table.project(tagging_arel_table[Arel.star].count.as('taggings_count')).order('taggings_count DESC').to_sql if options[:order_by_matching_tag_count].present? && options[:match_all].blank? + + order_by << options[:order] if options[:order].present? + order_by.join(', ') + end + end +end diff --git a/lib/acts_as_taggable_on/taggable/tagged_with_query/any_tags_query.rb b/lib/acts_as_taggable_on/taggable/tagged_with_query/any_tags_query.rb new file mode 100644 index 000000000..44fb5bf28 --- /dev/null +++ b/lib/acts_as_taggable_on/taggable/tagged_with_query/any_tags_query.rb @@ -0,0 +1,64 @@ +module ActsAsTaggableOn::Taggable::TaggedWithQuery + class AnyTagsQuery < QueryBase + def build + taggable_model.select(all_fields) + .where(model_has_at_least_one_tag) + .order(order_conditions) + .readonly(false) + end + + private + + def all_fields + taggable_arel_table[Arel.star] + end + + def model_has_at_least_one_tag + tagging_alias = tagging_arel_table.alias("#{tag_list.join('_')}_#{SecureRandom.hex(10)}") + + + tagging_arel_table.project(Arel.star).where(at_least_one_tag).exists + end + + def at_least_one_tag + exists_contition = tagging_arel_table[:taggable_id].eq(taggable_arel_table[taggable_model.primary_key]) + .and(tagging_arel_table[:taggable_type].eq(taggable_model.base_class.name)) + .and( + tagging_arel_table[:tag_id].in( + tag_arel_table.project(tag_arel_table[:id]).where(tags_match_type) + ) + ) + + if options[:start_at].present? + exists_contition = exists_contition.and(tagging_arel_table[:created_at].gteq(options[:start_at])) + end + + if options[:end_at].present? + exists_contition = exists_contition.and(tagging_arel_table[:created_at].lteq(options[:end_at])) + end + + if options[:on].present? + exists_contition = exists_contition.and(tagging_arel_table[:context].lteq(options[:on])) + end + + if (owner = options[:owned_by]).present? + owner_table = owner.class.base_class.arel_table + + exists_contition = exists_contition.and(tagging_arel_table[:tagger_id].eq(owner.id)) + .and(tagging_arel_table[:tagger_type].eq(owner.class.base_class.to_s)) + end + + exists_contition + end + + def order_conditions + order_by = [] + if options[:order_by_matching_tag_count].present? + order_by << "(SELECT count(*) FROM #{tagging_model.table_name} WHERE #{at_least_one_tag.to_sql}) desc" + end + + order_by << options[:order] if options[:order].present? + order_by.join(', ') + end + end +end diff --git a/lib/acts_as_taggable_on/taggable/tagged_with_query/exclude_tags_query.rb b/lib/acts_as_taggable_on/taggable/tagged_with_query/exclude_tags_query.rb new file mode 100644 index 000000000..26a67fb78 --- /dev/null +++ b/lib/acts_as_taggable_on/taggable/tagged_with_query/exclude_tags_query.rb @@ -0,0 +1,82 @@ +module ActsAsTaggableOn::Taggable::TaggedWithQuery + class ExcludeTagsQuery < QueryBase + def build + taggable_model.joins(owning_to_tagger) + .where(tags_not_in_list) + .having(tags_that_matches_count) + .readonly(false) + end + + private + + def tags_not_in_list + return taggable_arel_table[:id].not_in( + tagging_arel_table + .project(tagging_arel_table[:taggable_id]) + .join(tag_arel_table) + .on( + tagging_arel_table[:tag_id].eq(tag_arel_table[:id]) + .and(tagging_arel_table[:taggable_type].eq(taggable_model.base_class.name)) + .and(tags_match_type) + ) + ) + + # FIXME: missing time scope, this is also missing in the original implementation + end + + + def owning_to_tagger + return [] unless options[:owned_by].present? + + owner = options[:owned_by] + + arel_join = taggable_arel_table + .join(tagging_arel_table) + .on( + tagging_arel_table[:tagger_id].eq(owner.id) + .and(tagging_arel_table[:tagger_type].eq(owner.class.base_class.to_s)) + .and(tagging_arel_table[:taggable_id].eq(taggable_arel_table[taggable_model.primary_key])) + .and(tagging_arel_table[:taggable_type].eq(taggable_model.base_class.name)) + ) + + if options[:match_all].present? + arel_join = arel_join + .join(tagging_arel_table, Arel::Nodes::OuterJoin) + .on( + match_all_on_conditions + ) + end + + return arel_join.join_sources + end + + def match_all_on_conditions + on_condition = tagging_arel_table[:taggable_id].eq(taggable_arel_table[taggable_model.primary_key]) + .and(tagging_arel_table[:taggable_type].eq(taggable_model.base_class.name)) + + if options[:start_at].present? + on_condition = on_condition.and(tagging_arel_table[:created_at].gteq(options[:start_at])) + end + + if options[:end_at].present? + on_condition = on_condition.and(tagging_arel_table[:created_at].lteq(options[:end_at])) + end + + if options[:on].present? + on_condition = on_condition.and(tagging_arel_table[:context].lteq(options[:on])) + end + + on_condition + end + + def tags_that_matches_count + return [] unless options[:match_all].present? + + taggable_model.find_by_sql(tag_arel_table.project(Arel.star.count).where(tags_match_type).to_sql) + + tagging_arel_table[:taggable_id].count.eq( + tag_arel_table.project(Arel.star.count).where(tags_match_type) + ) + end + end +end diff --git a/lib/acts_as_taggable_on/taggable/tagged_with_query/query_base.rb b/lib/acts_as_taggable_on/taggable/tagged_with_query/query_base.rb new file mode 100644 index 000000000..6d09457d5 --- /dev/null +++ b/lib/acts_as_taggable_on/taggable/tagged_with_query/query_base.rb @@ -0,0 +1,40 @@ +module ActsAsTaggableOn::Taggable::TaggedWithQuery + class QueryBase + def initialize(taggable_model, tag_model, tagging_model, tag_list, options) + @taggable_model = taggable_model + @tag_model = tag_model + @tagging_model = tagging_model + @tag_list = tag_list + @options = options + end + + private + + attr_reader :taggable_model, :tag_model, :tagging_model, :tag_list, :options + + def taggable_arel_table + @taggable_arel_table ||= taggable_model.arel_table + end + + def tag_arel_table + @tag_arel_table ||= tag_model.arel_table + end + + def tagging_arel_table + @tagging_arel_table ||=tagging_model.arel_table + end + + def tag_match_type(tag) + match_type = options[:wild].present? ? 'matches' : 'eq' + + tag_arel_table[:name].lower.public_send(match_type, tag.downcase) + end + + def tags_match_type + match_type = options[:wild].present? ? 'matches_any' : 'eq_any' + + tags = options[:wild].present? ? tag_list.map { |tag| "%#{tag.downcase}%"} : tag_list.map(&:downcase) + tag_arel_table[:name].lower.public_send(match_type, tags) + end + end +end diff --git a/lib/acts_as_taggable_on/tagging.rb b/lib/acts_as_taggable_on/tagging.rb index fcfda57f9..783c45825 100644 --- a/lib/acts_as_taggable_on/tagging.rb +++ b/lib/acts_as_taggable_on/tagging.rb @@ -1,5 +1,6 @@ module ActsAsTaggableOn class Tagging < ::ActiveRecord::Base #:nodoc: + DEFAULT_CONTEXT = 'tags' belongs_to :tag, class_name: '::ActsAsTaggableOn::Tag', counter_cache: ActsAsTaggableOn.tags_counter belongs_to :taggable, polymorphic: true @@ -8,8 +9,8 @@ class Tagging < ::ActiveRecord::Base #:nodoc: scope :owned_by, ->(owner) { where(tagger: owner) } scope :not_owned, -> { where(tagger_id: nil, tagger_type: nil) } - scope :by_contexts, ->(contexts) { where(context: (contexts || 'tags')) } - scope :by_context, ->(context = 'tags') { by_contexts(context.to_s) } + scope :by_contexts, ->(contexts) { where(context: (contexts || DEFAULT_CONTEXT)) } + scope :by_context, ->(context = DEFAULT_CONTEXT) { by_contexts(context.to_s) } validates_presence_of :context validates_presence_of :tag_id diff --git a/spec/acts_as_taggable_on/taggable_spec.rb b/spec/acts_as_taggable_on/taggable_spec.rb index 414b6d2f1..cb71e241e 100644 --- a/spec/acts_as_taggable_on/taggable_spec.rb +++ b/spec/acts_as_taggable_on/taggable_spec.rb @@ -247,7 +247,7 @@ expect(TaggableModel.tagged_with("ruby", :start_at => today, :end_at => tomorrow).count).to eq(1) end - it "shouldn't be able to find a tag outside date range" do + it "shouldn't be able to find a tag outside date range" do @taggable.skill_list = "ruby" @taggable.save @@ -260,6 +260,7 @@ @taggable.save expect(TaggableModel.tagged_with('ruby').first).to eq(@taggable) + expect(TaggableModel.tagged_with('ruby, css').first).to eq(@taggable) expect(TaggableModel.tagged_with('bob', on: :skills).first).to_not eq(@taggable) expect(TaggableModel.tagged_with('bob', on: :tags).first).to eq(@taggable) @@ -458,6 +459,7 @@ frank = TaggableModel.create(name: 'Frank', tag_list: 'weaker, depressed, inefficient', skill_list: 'ruby, rails, css') steve = TaggableModel.create(name: 'Steve', tag_list: 'fitter, happier, more productive', skill_list: 'c++, java, ruby') + expect(TaggableModel.tagged_with(%w(ruby java), any: true, order_by_matching_tag_count: true, order: 'taggable_models.name').to_a).to eq([steve, bob, frank]) expect(TaggableModel.tagged_with(%w(c++ fitter), any: true, order_by_matching_tag_count: true, order: 'taggable_models.name').to_a).to eq([steve, bob]) expect(TaggableModel.tagged_with(%w(depressed css), any: true, order_by_matching_tag_count: true, order: 'taggable_models.name').to_a).to eq([frank, bob]) From 5a2d5911aacc68845506ff47c604905c60d485a7 Mon Sep 17 00:00:00 2001 From: Radhames Brito Date: Mon, 15 May 2017 14:59:53 -0400 Subject: [PATCH 04/10] removing extra lines and fixing test --- lib/acts-as-taggable-on.rb | 1 - spec/acts_as_taggable_on/tag_spec.rb | 2 +- spec/acts_as_taggable_on/taggable_spec.rb | 2 -- 3 files changed, 1 insertion(+), 4 deletions(-) diff --git a/lib/acts-as-taggable-on.rb b/lib/acts-as-taggable-on.rb index da52925d5..8e92d0cc8 100644 --- a/lib/acts-as-taggable-on.rb +++ b/lib/acts-as-taggable-on.rb @@ -2,7 +2,6 @@ require 'active_record/version' require 'active_support/core_ext/module' - begin require 'rails/engine' require 'acts_as_taggable_on/engine' diff --git a/spec/acts_as_taggable_on/tag_spec.rb b/spec/acts_as_taggable_on/tag_spec.rb index 469e6bf79..0aeb75cfd 100644 --- a/spec/acts_as_taggable_on/tag_spec.rb +++ b/spec/acts_as_taggable_on/tag_spec.rb @@ -171,7 +171,7 @@ context 'retry 3 times on not unique exception' do it 'performs 3 tries before raising the exception' do allow(ActsAsTaggableOn::Tag).to receive(:named_any).and_raise(ActiveRecord::RecordNotUnique.new('error')) # trigger error inside block - expect(ActiveRecord::Base.connection).to receive(:execute).with('ROLLBACK').exactly(3).times + expect(ActiveRecord::Base.connection).to receive(:execute).with('ROLLBACK').exactly(4).times # one extra rollback from database cleaner expect { ActsAsTaggableOn::Tag.find_or_create_all_with_like_by_name('AWESOME', 'awesome') diff --git a/spec/acts_as_taggable_on/taggable_spec.rb b/spec/acts_as_taggable_on/taggable_spec.rb index cb71e241e..a2835e0ca 100644 --- a/spec/acts_as_taggable_on/taggable_spec.rb +++ b/spec/acts_as_taggable_on/taggable_spec.rb @@ -260,7 +260,6 @@ @taggable.save expect(TaggableModel.tagged_with('ruby').first).to eq(@taggable) - expect(TaggableModel.tagged_with('ruby, css').first).to eq(@taggable) expect(TaggableModel.tagged_with('bob', on: :skills).first).to_not eq(@taggable) expect(TaggableModel.tagged_with('bob', on: :tags).first).to eq(@taggable) @@ -459,7 +458,6 @@ frank = TaggableModel.create(name: 'Frank', tag_list: 'weaker, depressed, inefficient', skill_list: 'ruby, rails, css') steve = TaggableModel.create(name: 'Steve', tag_list: 'fitter, happier, more productive', skill_list: 'c++, java, ruby') - expect(TaggableModel.tagged_with(%w(ruby java), any: true, order_by_matching_tag_count: true, order: 'taggable_models.name').to_a).to eq([steve, bob, frank]) expect(TaggableModel.tagged_with(%w(c++ fitter), any: true, order_by_matching_tag_count: true, order: 'taggable_models.name').to_a).to eq([steve, bob]) expect(TaggableModel.tagged_with(%w(depressed css), any: true, order_by_matching_tag_count: true, order: 'taggable_models.name').to_a).to eq([frank, bob]) From c34ae84e72b45ae3a2882d9a1b10a012e87edc7e Mon Sep 17 00:00:00 2001 From: Radhames Brito Date: Mon, 15 May 2017 15:09:58 -0400 Subject: [PATCH 05/10] changing DBcleaner strategy --- spec/acts_as_taggable_on/tag_spec.rb | 2 +- spec/support/database_cleaner.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/acts_as_taggable_on/tag_spec.rb b/spec/acts_as_taggable_on/tag_spec.rb index 0aeb75cfd..469e6bf79 100644 --- a/spec/acts_as_taggable_on/tag_spec.rb +++ b/spec/acts_as_taggable_on/tag_spec.rb @@ -171,7 +171,7 @@ context 'retry 3 times on not unique exception' do it 'performs 3 tries before raising the exception' do allow(ActsAsTaggableOn::Tag).to receive(:named_any).and_raise(ActiveRecord::RecordNotUnique.new('error')) # trigger error inside block - expect(ActiveRecord::Base.connection).to receive(:execute).with('ROLLBACK').exactly(4).times # one extra rollback from database cleaner + expect(ActiveRecord::Base.connection).to receive(:execute).with('ROLLBACK').exactly(3).times expect { ActsAsTaggableOn::Tag.find_or_create_all_with_like_by_name('AWESOME', 'awesome') diff --git a/spec/support/database_cleaner.rb b/spec/support/database_cleaner.rb index 3e4008c6f..ab9972e53 100644 --- a/spec/support/database_cleaner.rb +++ b/spec/support/database_cleaner.rb @@ -2,7 +2,7 @@ config.before(:suite) do DatabaseCleaner.clean_with(:truncation) - DatabaseCleaner.strategy = :transaction + DatabaseCleaner.strategy = :deletion DatabaseCleaner.clean end From 987738367bd1b69adb0c510446b8d1a42df53ddc Mon Sep 17 00:00:00 2001 From: Radhames Brito Date: Mon, 15 May 2017 15:22:25 -0400 Subject: [PATCH 06/10] removing retry changes --- lib/acts_as_taggable_on/tag.rb | 2 +- spec/acts_as_taggable_on/tag_spec.rb | 11 ----------- 2 files changed, 1 insertion(+), 12 deletions(-) diff --git a/lib/acts_as_taggable_on/tag.rb b/lib/acts_as_taggable_on/tag.rb index 870ee4cb0..05f2a6773 100644 --- a/lib/acts_as_taggable_on/tag.rb +++ b/lib/acts_as_taggable_on/tag.rb @@ -78,7 +78,7 @@ def self.find_or_create_all_with_like_by_name(*list) existing_tag = existing_tags.find { |tag| comparable_name(tag.name) == comparable_tag_name } existing_tag || create(name: tag_name) rescue ActiveRecord::RecordNotUnique - unless (tries -= 1).negative? + if (tries -= 1).positive? ActiveRecord::Base.connection.execute 'ROLLBACK' retry end diff --git a/spec/acts_as_taggable_on/tag_spec.rb b/spec/acts_as_taggable_on/tag_spec.rb index 469e6bf79..392e0f929 100644 --- a/spec/acts_as_taggable_on/tag_spec.rb +++ b/spec/acts_as_taggable_on/tag_spec.rb @@ -167,17 +167,6 @@ it 'should return an empty array if no tags are specified' do expect(ActsAsTaggableOn::Tag.find_or_create_all_with_like_by_name([])).to be_empty end - - context 'retry 3 times on not unique exception' do - it 'performs 3 tries before raising the exception' do - allow(ActsAsTaggableOn::Tag).to receive(:named_any).and_raise(ActiveRecord::RecordNotUnique.new('error')) # trigger error inside block - expect(ActiveRecord::Base.connection).to receive(:execute).with('ROLLBACK').exactly(3).times - - expect { - ActsAsTaggableOn::Tag.find_or_create_all_with_like_by_name('AWESOME', 'awesome') - }.to raise_error ActsAsTaggableOn::DuplicateTagError - end - end end it 'should require a name' do From 818c13c65c74d5ca5e20d9e243dc5402bc2a29d8 Mon Sep 17 00:00:00 2001 From: Radhames Brito Date: Mon, 15 May 2017 15:23:28 -0400 Subject: [PATCH 07/10] removing DBcleaner changes --- spec/support/database_cleaner.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/support/database_cleaner.rb b/spec/support/database_cleaner.rb index ab9972e53..3e4008c6f 100644 --- a/spec/support/database_cleaner.rb +++ b/spec/support/database_cleaner.rb @@ -2,7 +2,7 @@ config.before(:suite) do DatabaseCleaner.clean_with(:truncation) - DatabaseCleaner.strategy = :deletion + DatabaseCleaner.strategy = :transaction DatabaseCleaner.clean end From 41b36d1dc2dfecb733bf4f9d5e52c84d6c05571c Mon Sep 17 00:00:00 2001 From: Radhames Brito Date: Mon, 15 May 2017 17:15:46 -0400 Subject: [PATCH 08/10] fix matching and encoding errors --- .../tagged_with_query/all_tags_query.rb | 7 ++++- .../tagged_with_query/any_tags_query.rb | 13 +++++++++- .../taggable/tagged_with_query/query_base.rb | 26 ++++++++++++++----- 3 files changed, 38 insertions(+), 8 deletions(-) diff --git a/lib/acts_as_taggable_on/taggable/tagged_with_query/all_tags_query.rb b/lib/acts_as_taggable_on/taggable/tagged_with_query/all_tags_query.rb index 79b6406f7..ba5674c1a 100644 --- a/lib/acts_as_taggable_on/taggable/tagged_with_query/all_tags_query.rb +++ b/lib/acts_as_taggable_on/taggable/tagged_with_query/all_tags_query.rb @@ -14,7 +14,7 @@ def each_tag_in_list arel_join = taggable_arel_table tag_list.each do |tag| - tagging_alias = tagging_arel_table.alias("#{tag}_taggings_#{SecureRandom.hex(10)}") + tagging_alias = tagging_arel_table.alias(tagging_alias(tag)) arel_join = arel_join .join(tagging_alias) .on(on_conditions(tag, tagging_alias)) @@ -104,5 +104,10 @@ def order_conditions order_by << options[:order] if options[:order].present? order_by.join(', ') end + + def tagging_alias(tag) + alias_base_name = taggable_model.base_class.name.downcase + adjust_taggings_alias("#{alias_base_name[0..11]}_taggings_#{ActsAsTaggableOn::Utils.sha_prefix(tag)}") + end end end diff --git a/lib/acts_as_taggable_on/taggable/tagged_with_query/any_tags_query.rb b/lib/acts_as_taggable_on/taggable/tagged_with_query/any_tags_query.rb index 44fb5bf28..88d5de4c1 100644 --- a/lib/acts_as_taggable_on/taggable/tagged_with_query/any_tags_query.rb +++ b/lib/acts_as_taggable_on/taggable/tagged_with_query/any_tags_query.rb @@ -14,7 +14,7 @@ def all_fields end def model_has_at_least_one_tag - tagging_alias = tagging_arel_table.alias("#{tag_list.join('_')}_#{SecureRandom.hex(10)}") + tagging_alias = tagging_arel_table.alias(alias_name(tag_list)) tagging_arel_table.project(Arel.star).where(at_least_one_tag).exists @@ -60,5 +60,16 @@ def order_conditions order_by << options[:order] if options[:order].present? order_by.join(', ') end + + def alias_name(tag_list) + alias_base_name = taggable_model.base_class.name.downcase + taggings_context = options[:on] ? "_#{options[:on]}" : '' + + taggings_alias = adjust_taggings_alias( + "#{alias_base_name[0..4]}#{taggings_context[0..6]}_taggings_#{ActsAsTaggableOn::Utils.sha_prefix(tag_list.join('_'))}" + ) + + taggings_alias + end end end diff --git a/lib/acts_as_taggable_on/taggable/tagged_with_query/query_base.rb b/lib/acts_as_taggable_on/taggable/tagged_with_query/query_base.rb index 6d09457d5..6b697394d 100644 --- a/lib/acts_as_taggable_on/taggable/tagged_with_query/query_base.rb +++ b/lib/acts_as_taggable_on/taggable/tagged_with_query/query_base.rb @@ -25,16 +25,30 @@ def tagging_arel_table end def tag_match_type(tag) - match_type = options[:wild].present? ? 'matches' : 'eq' - - tag_arel_table[:name].lower.public_send(match_type, tag.downcase) + if options[:wild].present? + tag_arel_table[:name].matches("%#{escaped_tag(tag)}%", "!", ActsAsTaggableOn.strict_case_match) + else + tag_arel_table[:name].matches(escaped_tag(tag), "!", ActsAsTaggableOn.strict_case_match) + end end def tags_match_type - match_type = options[:wild].present? ? 'matches_any' : 'eq_any' + if options[:wild].present? + tag_arel_table[:name].matches_any(tag_list.map{|tag| "%#{escaped_tag(tag)}%"}, "!", ActsAsTaggableOn.strict_case_match) + else + tag_arel_table[:name].matches_any(tag_list.map{|tag| "#{escaped_tag(tag)}"}, "!", ActsAsTaggableOn.strict_case_match) + end + end + + def escaped_tag(tag) + tag.gsub(/[!%_]/) { |x| '!' + x } + end - tags = options[:wild].present? ? tag_list.map { |tag| "%#{tag.downcase}%"} : tag_list.map(&:downcase) - tag_arel_table[:name].lower.public_send(match_type, tags) + def adjust_taggings_alias(taggings_alias) + if taggings_alias.size > 75 + taggings_alias = 'taggings_alias_' + Digest::SHA1.hexdigest(taggings_alias) + end + taggings_alias end end end From 68a9df19e6f73e64cee938ae4e4126fef7364d52 Mon Sep 17 00:00:00 2001 From: Radhames Brito Date: Mon, 15 May 2017 17:31:44 -0400 Subject: [PATCH 09/10] fix arel 6.0 case sensitibity --- .../taggable/tagged_with_query/query_base.rb | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/lib/acts_as_taggable_on/taggable/tagged_with_query/query_base.rb b/lib/acts_as_taggable_on/taggable/tagged_with_query/query_base.rb index 6b697394d..011ae51e8 100644 --- a/lib/acts_as_taggable_on/taggable/tagged_with_query/query_base.rb +++ b/lib/acts_as_taggable_on/taggable/tagged_with_query/query_base.rb @@ -25,22 +25,29 @@ def tagging_arel_table end def tag_match_type(tag) + matches_attribute = tag_arel_table[:name] + matches_attribute = matches_attribute.lower unless ActsAsTaggableOn.strict_case_match + if options[:wild].present? - tag_arel_table[:name].matches("%#{escaped_tag(tag)}%", "!", ActsAsTaggableOn.strict_case_match) + tag_arel_table[:name].matches("%#{escaped_tag(tag)}%", "!") else - tag_arel_table[:name].matches(escaped_tag(tag), "!", ActsAsTaggableOn.strict_case_match) + tag_arel_table[:name].matches(escaped_tag(tag), "!") end end def tags_match_type + matches_attribute = tag_arel_table[:name] + matches_attribute = matches_attribute.lower unless ActsAsTaggableOn.strict_case_match + if options[:wild].present? - tag_arel_table[:name].matches_any(tag_list.map{|tag| "%#{escaped_tag(tag)}%"}, "!", ActsAsTaggableOn.strict_case_match) + matches_attribute.matches_any(tag_list.map{|tag| "%#{escaped_tag(tag)}%"}, "!") else - tag_arel_table[:name].matches_any(tag_list.map{|tag| "#{escaped_tag(tag)}"}, "!", ActsAsTaggableOn.strict_case_match) + matches_attribute.matches_any(tag_list.map{|tag| "#{escaped_tag(tag)}"}, "!") end end def escaped_tag(tag) + tag = tag.downcase unless ActsAsTaggableOn.strict_case_match tag.gsub(/[!%_]/) { |x| '!' + x } end From 0f68d64ec174bef5629798f00f01e48d6aa354ba Mon Sep 17 00:00:00 2001 From: Radhames Brito Date: Mon, 15 May 2017 17:39:14 -0400 Subject: [PATCH 10/10] remove unused method --- lib/acts_as_taggable_on/taggable/core.rb | 7 ------- 1 file changed, 7 deletions(-) diff --git a/lib/acts_as_taggable_on/taggable/core.rb b/lib/acts_as_taggable_on/taggable/core.rb index d6f25d7b3..734d82aa1 100644 --- a/lib/acts_as_taggable_on/taggable/core.rb +++ b/lib/acts_as_taggable_on/taggable/core.rb @@ -97,13 +97,6 @@ def is_taggable? true end - def adjust_taggings_alias(taggings_alias) - if taggings_alias.size > 75 - taggings_alias = 'taggings_alias_' + Digest::SHA1.hexdigest(taggings_alias) - end - taggings_alias - end - def taggable_mixin @taggable_mixin ||= Module.new end