fix: restrict the use of surrounding parentheses only to IN operator

Author: romain-gilliotte

app/services/forest_liana/filters_parser.rb

@@ -109,16 +109,7 @@ def parse_condition_without_smart_field(condition)
       parsed_value = parse_value(operator, value)
       field_and_operator = "#{parsed_field} #{parsed_operator}"
 
-      # parenthesis around the parsed_value are required to make the `IN` operator work
-      # and have no side effects on other requests
-      if Rails::VERSION::MAJOR < 5
-        "#{field_and_operator} (#{ActiveRecord::Base.sanitize(parsed_value)})"
-      # NOTICE: sanitize method as been removed in Rails 5.1 and sanitize_sql introduced in Rails 5.2.
-      elsif Rails::VERSION::MAJOR == 5 && Rails::VERSION::MINOR == 1
-        "#{field_and_operator} (#{ActiveRecord::Base.connection.quote(parsed_value)})"
-      else
-        ActiveRecord::Base.sanitize_sql(["#{field_and_operator} (?)", parsed_value])
-      end
+      sanitize_condition(field_and_operator, operator, parsed_value)
     end
 
     def parse_aggregation_operator(aggregator_operator)
@@ -296,5 +287,26 @@ def ensure_valid_condition(condition)
         raise ForestLiana::Errors::HTTP422Error.new('Invalid condition format')
       end
     end
+
+    private
+
+    def prepare_value_for_operator(operator, value)
+      # parenthesis around the parsed_value are required to make the `IN` operator work
+      operator == 'in' ? "(#{value})" : value
+    end
+
+    def sanitize_condition(field_and_operator, operator, parsed_value)
+      if Rails::VERSION::MAJOR < 5
+        condition_value = prepare_value_for_operator(operator, ActiveRecord::Base.sanitize(parsed_value))
+        "#{field_and_operator} #{condition_value}"
+        # NOTICE: sanitize method as been removed in Rails 5.1 and sanitize_sql introduced in Rails 5.2.
+      elsif Rails::VERSION::MAJOR == 5 && Rails::VERSION::MINOR == 1
+        condition_value = prepare_value_for_operator(operator, ActiveRecord::Base.connection.quote(parsed_value))
+        "#{field_and_operator} #{condition_value}"
+      else
+        condition_value = prepare_value_for_operator(operator, '?')
+        ActiveRecord::Base.sanitize_sql(["#{field_and_operator} #{condition_value}", parsed_value])
+      end
+    end
   end
 end

spec/services/forest_liana/filters_parser_spec.rb

@@ -11,6 +11,7 @@ module ForestLiana
     let(:date_condition_1) { { 'field' => 'created_at', 'operator' => 'before', 'value' => 2.hours.ago } }
     let(:date_condition_2) { { 'field' => 'created_at', 'operator' => 'today' } }
     let(:date_condition_3) { { 'field' => 'created_at', 'operator' => 'previous_x_days', 'value' => 2 } }
+    let(:presence_condition) { { 'field' => 'name', 'operator' => 'present' } }
 
     before {
       island = Island.create!(name: "L'île de la muerta")
@@ -274,14 +275,39 @@ module ForestLiana
         let(:filters) { { 'aggregator' => 'or', 'conditions' => [simple_condition_2, simple_condition_3] } }
         it { expect(resource.where(query).count).to eq 2 }
       end
+
+      context "'name ends_with \"3\"' 'or' 'name is not null'" do
+        let(:filters) { { 'aggregator' => 'or', 'conditions' => [simple_condition_2, presence_condition] } }
+        it { expect(resource.where(query).count).to eq 3 }
+      end
     end
 
     describe 'parse_condition' do
       let(:condition) { simple_condition_2 }
       let(:result) { filter_parser.parse_condition(condition) }
 
       context 'on valid condition' do
-        it { expect(result).to eq "\"trees\".\"name\" LIKE ('%3')" }
+        context 'when the condition uses the contains operator' do
+          it { expect(result).to eq "\"trees\".\"name\" LIKE '%3'" }
+        end
+
+        context 'when the condition uses the blank operator' do
+          let(:condition) { { 'field' => 'name', 'operator' => 'blank' } }
+
+          it { expect(result).to eq "\"trees\".\"name\" IS NULL" }
+        end
+
+        context 'when the condition uses the presence operator' do
+          let(:condition) { presence_condition }
+
+          it { expect(result).to eq "\"trees\".\"name\" IS NOT NULL" }
+        end
+
+        context 'when the condition uses the in operator' do
+          let(:condition) { { 'field' => 'name', 'operator' => 'in', 'value' => ['Tree n1', 'Tree n3'] } }
+
+          it { expect(result).to eq "\"trees\".\"name\" IN ('Tree n1','Tree n3')" }
+        end
       end
 
       context 'on belongs_to condition' do

spec/services/forest_liana/resource_updater_spec.rb

@@ -84,7 +84,7 @@ module ForestLiana
           subject.perform
 
           expect(subject.record).to be nil
-          expect(subject.errors[0][:detail]).to eq 'Couldn\'t find User with \'id\'=1 [WHERE (("users"."id" > (2)))]'
+          expect(subject.errors[0][:detail]).to eq 'Couldn\'t find User with \'id\'=1 [WHERE (("users"."id" > 2))]'
         end
       end