fix: Update validations for filter service (#8239)

- Refactor filter service for better readability and maintenance
- Add validations for the following:
   - If an invalid attribute is passed, a custom exception InvalidAttribute will be thrown.
   - If an invalid operator is passed, a custom exception InvalidOperator will be thrown.
   - If an invalid value (currently checking only null check), a custom exception InvalidValue will be thrown.

Fixes: https://linear.app/chatwoot/issue/CW-2702/activerecordstatementinvalid-pginvalidtextrepresentation-error-invalid
Fixes: https://linear.app/chatwoot/issue/CW-2703/activerecordstatementinvalid-pginvaliddatetimeformat-error-invalid
Fixes:  https://linear.app/chatwoot/issue/CW-2700/activerecordstatementinvalid-pgsyntaxerror-error-syntax-error-at-or


Co-authored-by: Sojan <sojan@pepalo.com>

app/controllers/api/v1/accounts/contacts_controller.rb

@@ -65,6 +65,10 @@ class Api::V1::Accounts::ContactsController < Api::V1::Accounts::BaseController
     contacts = result[:contacts]
     @contacts_count = result[:count]
     @contacts = fetch_contacts(contacts)
+  rescue CustomExceptions::CustomFilter::InvalidAttribute,
+         CustomExceptions::CustomFilter::InvalidOperator,
+         CustomExceptions::CustomFilter::InvalidValue => e
+    render_could_not_create_error(e.message)
   end
 
   def contactable_inboxes

app/controllers/api/v1/accounts/conversations_controller.rb

@@ -44,6 +44,10 @@ class Api::V1::Accounts::ConversationsController < Api::V1::Accounts::BaseContro
     result = ::Conversations::FilterService.new(params.permit!, current_user).perform
     @conversations = result[:conversations]
     @conversations_count = result[:count]
+  rescue CustomExceptions::CustomFilter::InvalidAttribute,
+         CustomExceptions::CustomFilter::InvalidOperator,
+         CustomExceptions::CustomFilter::InvalidValue => e
+    render_could_not_create_error(e.message)
   end
 
   def mute

app/helpers/filter_helper.rb

@@ -0,0 +1,86 @@
+module FilterHelper
+  def build_condition_query(model_filters, query_hash, current_index)
+    current_filter = model_filters[query_hash['attribute_key']]
+
+    # Throw InvalidOperator Error if the attribute is a standard attribute
+    # and the operator is not allowed in the config
+    if current_filter.present? && current_filter['filter_operators'].exclude?(query_hash[:filter_operator])
+      raise CustomExceptions::CustomFilter::InvalidOperator.new(
+        attribute_name: query_hash['attribute_key'],
+        allowed_keys: current_filter['filter_operators']
+      )
+    end
+
+    # Every other filter expects a value to be present
+    if %w[is_present is_not_present].exclude?(query_hash[:filter_operator]) && query_hash['values'].blank?
+      raise CustomExceptions::CustomFilter::InvalidValue.new(attribute_name: query_hash['attribute_key'])
+    end
+
+    condition_query = build_condition_query_string(current_filter, query_hash, current_index)
+    # The query becomes empty only when it doesn't match to any supported
+    # standard attribute or custom attribute defined in the account.
+    if condition_query.empty?
+      raise CustomExceptions::CustomFilter::InvalidAttribute.new(key: query_hash['attribute_key'],
+                                                                 allowed_keys: model_filters.keys)
+    end
+
+    condition_query
+  end
+
+  def build_condition_query_string(current_filter, query_hash, current_index)
+    filter_operator_value = filter_operation(query_hash, current_index)
+
+    return handle_nil_filter(query_hash, current_index) if current_filter.nil?
+
+    case current_filter['attribute_type']
+    when 'additional_attributes'
+      handle_additional_attributes(query_hash, filter_operator_value)
+    else
+      handle_standard_attributes(current_filter, query_hash, current_index, filter_operator_value)
+    end
+  end
+
+  def handle_nil_filter(query_hash, current_index)
+    attribute_type = "#{filter_config[:entity].downcase}_attribute"
+    custom_attribute_query(query_hash, attribute_type, current_index)
+  end
+
+  # TODO: Change the reliance on entity instead introduce datatype text_case_insensive
+  # Then we can remove the condition for Contact
+  def handle_additional_attributes(query_hash, filter_operator_value)
+    if filter_config[:entity] == 'Contact'
+      "LOWER(#{filter_config[:table_name]}.additional_attributes ->> '#{query_hash[:attribute_key]}') " \
+        "#{filter_operator_value} #{query_hash[:query_operator]}"
+    else
+      "#{filter_config[:table_name]}.additional_attributes ->> '#{query_hash[:attribute_key]}' " \
+        "#{filter_operator_value} #{query_hash[:query_operator]} "
+    end
+  end
+
+  def handle_standard_attributes(current_filter, query_hash, current_index, filter_operator_value)
+    case current_filter['data_type']
+    when 'date'
+      date_filter(current_filter, query_hash, filter_operator_value)
+    when 'labels'
+      tag_filter_query(query_hash, current_index)
+    else
+      default_filter(query_hash, filter_operator_value)
+    end
+  end
+
+  def date_filter(current_filter, query_hash, filter_operator_value)
+    "(#{filter_config[:table_name]}.#{query_hash[:attribute_key]})::#{current_filter['data_type']} " \
+      "#{filter_operator_value}#{current_filter['data_type']} #{query_hash[:query_operator]}"
+  end
+
+  # TODO: Change the reliance on entity instead introduce datatype text_case_insensive
+  # Then we can remove the condition for Contact
+  def default_filter(query_hash, filter_operator_value)
+    if filter_config[:entity] == 'Contact'
+      "LOWER(#{filter_config[:table_name]}.#{query_hash[:attribute_key]}) " \
+        "#{filter_operator_value} #{query_hash[:query_operator]}"
+    else
+      "#{filter_config[:table_name]}.#{query_hash[:attribute_key]} #{filter_operator_value} #{query_hash[:query_operator]}"
+    end
+  end
+end

app/services/automation_rules/condition_validation_service.rb

@@ -5,8 +5,8 @@ class AutomationRules::ConditionValidationService
     @rule = rule
     @account = rule.account
 
-    file = File.read('./lib/filters/filter_keys.json')
-    @filters = JSON.parse(file)
+    file = File.read('./lib/filters/filter_keys.yml')
+    @filters = YAML.safe_load(file)
 
     @conversation_filters = @filters['conversations']
     @contact_filters = @filters['contacts']

app/services/automation_rules/conditions_filter_service.rb

@@ -11,8 +11,9 @@ class AutomationRules::ConditionsFilterService < FilterService
     @account = conversation.account
 
     # setup filters from json file
-    file = File.read('./lib/filters/filter_keys.json')
-    @filters = JSON.parse(file)
+    file = File.read('./lib/filters/filter_keys.yml')
+    @filters = YAML.safe_load(file)
+
     @conversation_filters = @filters['conversations']
     @contact_filters = @filters['contacts']
     @message_filters = @filters['messages']

app/services/contacts/filter_service.rb

@@ -2,7 +2,7 @@ class Contacts::FilterService < FilterService
   ATTRIBUTE_MODEL = 'contact_attribute'.freeze
 
   def perform
-    @contacts = contact_query_builder
+    @contacts = query_builder(@filters['contacts'])
 
     {
       contacts: @contacts,
@@ -10,38 +10,6 @@ class Contacts::FilterService < FilterService
     }
   end
 
-  def contact_query_builder
-    contact_filters = @filters['contacts']
-
-    @params[:payload].each_with_index do |query_hash, current_index|
-      current_filter = contact_filters[query_hash['attribute_key']]
-      @query_string += contact_query_string(current_filter, query_hash, current_index)
-    end
-
-    base_relation.where(@query_string, @filter_values.with_indifferent_access)
-  end
-
-  def contact_query_string(current_filter, query_hash, current_index)
-    attribute_key = query_hash[:attribute_key]
-    query_operator = query_hash[:query_operator]
-    filter_operator_value = filter_operation(query_hash, current_index)
-
-    return custom_attribute_query(query_hash, 'contact_attribute', current_index) if current_filter.nil?
-
-    case current_filter['attribute_type']
-    when 'additional_attributes'
-      "  LOWER(contacts.additional_attributes ->> '#{attribute_key}') #{filter_operator_value} #{query_operator} "
-    when 'date_attributes'
-      " (contacts.#{attribute_key})::#{current_filter['data_type']} #{filter_operator_value}#{current_filter['data_type']} #{query_operator} "
-    when 'standard'
-      if attribute_key == 'labels'
-        " #{tag_filter_query('Contact', 'contacts', query_hash, current_index)} "
-      else
-        " LOWER(contacts.#{attribute_key}) #{filter_operator_value} #{query_operator} "
-      end
-    end
-  end
-
   def filter_values(query_hash)
     current_val = query_hash['values'][0]
     if query_hash['attribute_key'] == 'phone_number'
@@ -57,6 +25,13 @@ class Contacts::FilterService < FilterService
     Current.account.contacts
   end
 
+  def filter_config
+    {
+      entity: 'Contact',
+      table_name: 'contacts'
+    }
+  end
+
   private
 
   def equals_to_filter_string(filter_operator, current_index)

app/services/conversations/filter_service.rb

@@ -7,7 +7,7 @@ class Conversations::FilterService < FilterService
   end
 
   def perform
-    @conversations = conversation_query_builder
+    @conversations = query_builder(@filters['conversations'])
     mine_count, unassigned_count, all_count, = set_count_for_all_conversations
     assigned_count = all_count - unassigned_count
 
@@ -22,37 +22,6 @@ class Conversations::FilterService < FilterService
     }
   end
 
-  def conversation_query_builder
-    conversation_filters = @filters['conversations']
-    @params[:payload].each_with_index do |query_hash, current_index|
-      current_filter = conversation_filters[query_hash['attribute_key']]
-      @query_string += conversation_query_string(current_filter, query_hash, current_index)
-    end
-
-    base_relation.where(@query_string, @filter_values.with_indifferent_access)
-  end
-
-  def conversation_query_string(current_filter, query_hash, current_index)
-    attribute_key = query_hash[:attribute_key]
-    query_operator = query_hash[:query_operator]
-    filter_operator_value = filter_operation(query_hash, current_index)
-
-    return custom_attribute_query(query_hash, 'conversation_attribute', current_index) if current_filter.nil?
-
-    case current_filter['attribute_type']
-    when 'additional_attributes'
-      " conversations.additional_attributes ->> '#{attribute_key}' #{filter_operator_value} #{query_operator} "
-    when 'date_attributes'
-      " (conversations.#{attribute_key})::#{current_filter['data_type']} #{filter_operator_value}#{current_filter['data_type']} #{query_operator} "
-    when 'standard'
-      if attribute_key == 'labels'
-        " #{tag_filter_query('Conversation', 'conversations', query_hash, current_index)} "
-      else
-        " conversations.#{attribute_key} #{filter_operator_value} #{query_operator} "
-      end
-    end
-  end
-
   def base_relation
     @account.conversations.includes(
       :taggings, :inbox, { assignee: { avatar_attachment: [:blob] } }, { contact: { avatar_attachment: [:blob] } }, :team, :messages, :contact_inbox
@@ -63,6 +32,13 @@ class Conversations::FilterService < FilterService
     @params[:page] || 1
   end
 
+  def filter_config
+    {
+      entity: 'Conversation',
+      table_name: 'conversations'
+    }
+  end
+
   def conversations
     @conversations.sort_on_last_activity_at.page(current_page)
   end

app/services/filter_service.rb

@@ -1,6 +1,9 @@
 require 'json'
 
 class FilterService
+  include FilterHelper
+  include CustomExceptions::CustomFilter
+
   ATTRIBUTE_MODEL = 'conversation_attribute'.freeze
   ATTRIBUTE_TYPES = {
     date: 'date', text: 'text', number: 'numeric', link: 'text', list: 'text', checkbox: 'boolean'
@@ -9,8 +12,8 @@ class FilterService
   def initialize(params, user)
     @params = params
     @user = user
-    file = File.read('./lib/filters/filter_keys.json')
-    @filters = JSON.parse(file)
+    file = File.read('./lib/filters/filter_keys.yml')
+    @filters = YAML.safe_load(file)
     @query_string = ''
     @filter_values = {}
   end
@@ -106,7 +109,9 @@ class FilterService
     ]
   end
 
-  def tag_filter_query(model_name, table_name, query_hash, current_index)
+  def tag_filter_query(query_hash, current_index)
+    model_name = filter_config[:entity]
+    table_name = filter_config[:table_name]
     query_operator = query_hash[:query_operator]
     @filter_values["value_#{current_index}"] = filter_values(query_hash)
 
@@ -130,10 +135,8 @@ class FilterService
   def custom_attribute_query(query_hash, custom_attribute_type, current_index)
     @attribute_key = query_hash[:attribute_key]
     @custom_attribute_type = custom_attribute_type
-
     attribute_data_type
-
-    return ' ' if @custom_attribute.blank?
+    return '' if @custom_attribute.blank?
 
     build_custom_attr_query(query_hash, current_index)
   end
@@ -155,9 +158,9 @@ class FilterService
     table_name = attribute_model == 'conversation_attribute' ? 'conversations' : 'contacts'
 
     query = if attribute_data_type == 'text'
-              "  LOWER(#{table_name}.custom_attributes ->> '#{@attribute_key}')::#{attribute_data_type} #{filter_operator_value} #{query_operator} "
+              "LOWER(#{table_name}.custom_attributes ->> '#{@attribute_key}')::#{attribute_data_type} #{filter_operator_value} #{query_operator} "
             else
-              "  (#{table_name}.custom_attributes ->> '#{@attribute_key}')::#{attribute_data_type} #{filter_operator_value} #{query_operator} "
+              "(#{table_name}.custom_attributes ->> '#{@attribute_key}')::#{attribute_data_type} #{filter_operator_value} #{query_operator} "
             end
 
     query + not_in_custom_attr_query(table_name, query_hash, attribute_data_type)
@@ -194,4 +197,11 @@ class FilterService
 
     "NOT LIKE :value_#{current_index}"
   end
+
+  def query_builder(model_filters)
+    @params[:payload].each_with_index do |query_hash, current_index|
+      @query_string += " #{build_condition_query(model_filters, query_hash, current_index).strip}"
+    end
+    base_relation.where(@query_string, @filter_values.with_indifferent_access)
+  end
 end