Browse Source

Maintenance: Introduced custom Rubocop `Zammad/ExistsConditions` to check for `.find_by` in conditions and replace it with `.exists?` to increase performance and reduce memory cost.

Thorsten Eckel 4 years ago
parent
commit
f5841a2fe3

+ 120 - 0
.rubocop/cop/zammad/exists_condition.rb

@@ -0,0 +1,120 @@
+module RuboCop
+  module Cop
+    module Zammad
+      # This cop is used to identify usages of `find_by` in conditions and
+      # changes them to use `exists?` instead.
+      #
+      # @example
+      #   # bad
+      #   if User.find_by(name: 'Rubocop')
+      #   if !User.find_by(name: 'Rubocop')
+      #   unless User.find_by(name: 'Rubocop')
+      #   if find_by(name: 'Rubocop')
+      #   if !find_by(name: 'Rubocop')
+      #   unless find_by(name: 'Rubocop')
+      #
+      #   # good
+      #   if User.exists?(name: 'Rubocop')
+      #   if !User.exists?(name: 'Rubocop')
+      #   unless User.exists?(name: 'Rubocop')
+      #   if exists?(name: 'Rubocop')
+      #   if !exists?(name: 'Rubocop')
+      #   unless exists?(name: 'Rubocop')
+      class ExistsCondition < Cop
+
+        def_node_matcher :find_by_condition?, <<-PATTERN
+          {
+            $(send $_ :find_by ...)
+            (send $(send $_ :find_by ...) :!)
+          }
+        PATTERN
+
+        MSG = 'Use `%<prefer>s` instead of `%<current>s`.'.freeze
+
+        def on_if(node)
+          check_for_find_by(node)
+        end
+
+        def on_while(node)
+          check_for_find_by(node)
+        end
+
+        def on_while_post(node)
+          check_for_find_by(node)
+        end
+
+        def on_until(node)
+          check_for_find_by(node)
+        end
+
+        def on_until_post(node)
+          check_for_find_by(node)
+        end
+
+        def autocorrect(node)
+          lambda do |corrector|
+            corrector.replace(node.loc.selector, 'exists?')
+          end
+        end
+
+        private
+
+        def check_for_find_by(node)
+          cond = condition(node)
+          handle_node(cond)
+        end
+
+        def check_node(node)
+          return unless node
+
+          if keyword_bang?(node)
+            receiver, = *node
+
+            handle_node(receiver)
+          elsif node.operator_keyword?
+            node.each_child_node { |op| handle_node(op) }
+          elsif node.begin_type? && node.children.one?
+            handle_node(node.children.first)
+          end
+        end
+
+        def keyword_bang?(node)
+          node.respond_to?(:keyword_bang?) && node.keyword_bang?
+        end
+
+        def handle_node(node)
+          if node.send_type?
+            check_offense(*find_by_condition?(node)) # rubocop:disable Rails/DynamicFindBy
+          elsif %i[and or begin].include?(node.type)
+            check_node(node)
+          end
+        end
+
+        def condition(node)
+          if node.send_type?
+            node.receiver
+          else
+            node.condition
+          end
+        end
+
+        def check_offense(method_call = nil, receiver = nil)
+          return if method_call.nil?
+
+          add_offense(method_call,
+                      message: format(MSG,
+                                      prefer:  replacement(receiver),
+                                      current: current(receiver)))
+        end
+
+        def current(node)
+          node.respond_to?(:source) ? "#{node.source}.find_by" : 'find_by'
+        end
+
+        def replacement(node)
+          node.respond_to?(:source) ? "#{node.source}.exists?" : 'exists?'
+        end
+      end
+    end
+  end
+end

+ 1 - 0
.rubocop/default.yml

@@ -5,6 +5,7 @@ require:
   - rubocop-performance
   - rubocop-rails
   - rubocop-rspec
+  - ./rubocop_zammad.rb
 
 inherit_from:
   - todo.yml

+ 1 - 0
.rubocop/rubocop_zammad.rb

@@ -0,0 +1 @@
+require_relative 'cop/zammad/exists_condition'

+ 1 - 1
app/controllers/channels_email_controller.rb

@@ -31,7 +31,7 @@ class ChannelsEmailController < ApplicationController
 
       email_address_ids.push email_address.id
       assets = email_address.assets(assets)
-      if !email_address.channel_id || !email_address.active || !Channel.find_by(id: email_address.channel_id)
+      if !email_address.channel_id || !email_address.active || !Channel.exists?(id: email_address.channel_id)
         not_used_email_address_ids.push email_address.id
       end
     end

+ 2 - 2
app/models/calendar.rb

@@ -360,7 +360,7 @@ returns
 
   # check if min one is set to default true
   def min_one_check
-    if !Calendar.find_by(default: true)
+    if !Calendar.exists?(default: true)
       first = Calendar.order(:created_at, :id).limit(1).first
       return true if !first
 
@@ -376,7 +376,7 @@ returns
         sla.save!
         next
       end
-      if !Calendar.find_by(id: sla.calendar_id)
+      if !Calendar.exists?(id: sla.calendar_id)
         sla.calendar_id = default_calendar.id
         sla.save!
       end

+ 1 - 1
app/models/channel/driver/sms/twilio.rb

@@ -33,7 +33,7 @@ class Channel::Driver::Sms::Twilio
     Rails.logger.info "Receiving SMS frim recipient #{attr[:From]}"
 
     # prevent already created articles
-    if Ticket::Article.find_by(message_id: attr[:SmsMessageSid])
+    if Ticket::Article.exists?(message_id: attr[:SmsMessageSid])
       return ['application/xml; charset=UTF-8;', Twilio::TwiML::MessagingResponse.new.to_s]
     end
 

+ 3 - 3
app/models/channel/driver/twitter.rb

@@ -186,7 +186,7 @@ returns
         end
 
         next if @client.locale_sender?(tweet) && own_tweet_already_imported?(tweet)
-        next if Ticket::Article.find_by(message_id: tweet.id)
+        next if Ticket::Article.exists?(message_id: tweet.id)
         break if @client.tweet_limit_reached(tweet)
 
         @client.to_group(tweet, search[:group_id], @channel)
@@ -202,7 +202,7 @@ returns
     event_time = Time.zone.now
     sleep 4
     12.times do |loop_count|
-      if Ticket::Article.find_by(message_id: tweet.id)
+      if Ticket::Article.exists?(message_id: tweet.id)
         Rails.logger.debug { "Own tweet already imported, skipping tweet #{tweet.id}" }
         return true
       end
@@ -215,7 +215,7 @@ returns
       sleep sleep_time
     end
 
-    if Ticket::Article.find_by(message_id: tweet.id)
+    if Ticket::Article.exists?(message_id: tweet.id)
       Rails.logger.debug { "Own tweet already imported, skipping tweet #{tweet.id}" }
       return true
     end

+ 1 - 1
app/models/channel/filter/identify_sender.rb

@@ -35,7 +35,7 @@ module Channel::Filter::IdentifySender
             items.each do |item|
 
               # skip if recipient is system email
-              next if EmailAddress.find_by(email: item.address.downcase)
+              next if EmailAddress.exists?(email: item.address.downcase)
 
               customer_user = user_create(
                 login:     item.address,

+ 1 - 1
app/models/channel/filter/sender_is_system_address.rb

@@ -19,7 +19,7 @@ module Channel::Filter::SenderIsSystemAddress
 
       items = mail[form].addrs
       items.each do |item|
-        next if !EmailAddress.find_by(email: item.address.downcase)
+        next if !EmailAddress.exists?(email: item.address.downcase)
 
         mail['x-zammad-ticket-create-article-sender'.to_sym] = 'Agent'
         mail['x-zammad-article-sender'.to_sym] = 'Agent'

+ 1 - 1
app/models/email_address.rb

@@ -63,7 +63,7 @@ check and if channel not exists reset configured channels for email addresses
   def check_if_channel_exists_set_inactive
 
     # set to active if channel exists
-    if channel_id && Channel.find_by(id: channel_id)
+    if channel_id && Channel.exists?(id: channel_id)
       self.active = true
       return true
     end

Some files were not shown because too many files changed in this diff