Merge branch '216618-remove-liquid' into 'master'

Remove the liquid gem and use gsub instead

See merge request gitlab-org/gitlab!31482

Gemfile

@@ -481,8 +481,6 @@ gem 'countries', '~> 3.0'
 
 gem 'retriable', '~> 3.1.2'
 
-gem 'liquid', '~> 4.0'
-
 # LRU cache
 gem 'lru_redux'
 

Gemfile.lock

@@ -602,7 +602,6 @@ GEM
       xml-simple
     licensee (8.9.2)
       rugged (~> 0.24)
-    liquid (4.0.3)
     listen (3.1.5)
       rb-fsevent (~> 0.9, >= 0.9.4)
       rb-inotify (~> 0.9, >= 0.9.7)
@@ -1289,7 +1288,6 @@ DEPENDENCIES
   letter_opener_web (~> 1.3.4)
   license_finder (~> 5.4)
   licensee (~> 8.9)
-  liquid (~> 4.0)
   lockbox (~> 0.3.3)
   lograge (~> 0.5)
   loofah (~> 2.2)

app/services/prometheus/proxy_variable_substitution_service.rb

@@ -4,6 +4,16 @@ module Prometheus
   class ProxyVariableSubstitutionService < BaseService
     include Stepable
 
+    VARIABLE_INTERPOLATION_REGEX = /
+      {{                  # Variable needs to be wrapped in these chars.
+        \s*               # Allow whitespace before and after the variable name.
+          (?<variable>    # Named capture.
+            \w+           # Match one or more word characters.
+          )
+        \s*
+      }}
+    /x.freeze
+
     steps :validate_variables,
       :add_params_to_result,
       :substitute_params,
@@ -49,12 +59,9 @@ module Prometheus
     def substitute_liquid_variables(result)
       return success(result) unless query(result)
 
-      result[:params][:query] =
-        TemplateEngines::LiquidService.new(query(result)).render(full_context)
+      result[:params][:query] = gsub(query(result), full_context)
 
       success(result)
-    rescue TemplateEngines::LiquidService::RenderError => e
-      error(e.message)
     end
 
     def substitute_ruby_variables(result)
@@ -75,12 +82,24 @@ module Prometheus
       error(_('Malformed string'))
     end
 
+    def gsub(string, context)
+      # Search for variables of the form `{{variable}}` in the string and replace
+      # them with their value.
+      string.gsub(VARIABLE_INTERPOLATION_REGEX) do |match|
+        # Replace with the value of the variable, or if there is no such variable,
+        # replace the invalid variable with itself. So,
+        # `up{instance="{{invalid_variable}}"}` will remain
+        # `up{instance="{{invalid_variable}}"}` after substitution.
+        context.fetch($~[:variable], match)
+      end
+    end
+
     def predefined_context
       @predefined_context ||= Gitlab::Prometheus::QueryVariables.call(@environment)
     end
 
     def full_context
-      @full_context ||= predefined_context.reverse_merge(variables_hash)
+      @full_context ||= predefined_context.stringify_keys.reverse_merge(variables_hash)
     end
 
     def variables

app/services/template_engines/liquid_service.rb

-# frozen_string_literal: true
-
-module TemplateEngines
-  class LiquidService < BaseService
-    RenderError = Class.new(StandardError)
-
-    DEFAULT_RENDER_SCORE_LIMIT = 1_000
-
-    def initialize(string)
-      @template = Liquid::Template.parse(string)
-    end
-
-    def render(context, render_score_limit: DEFAULT_RENDER_SCORE_LIMIT)
-      set_limits(render_score_limit)
-
-      @template.render!(context.stringify_keys)
-    rescue Liquid::MemoryError => e
-      handle_exception(e, string: @string, context: context)
-
-      raise RenderError, _('Memory limit exceeded while rendering template')
-    rescue Liquid::Error => e
-      handle_exception(e, string: @string, context: context)
-
-      raise RenderError, _('Error rendering query')
-    end
-
-    private
-
-    def set_limits(render_score_limit)
-      @template.resource_limits.render_score_limit = render_score_limit
-
-      # We can also set assign_score_limit and render_length_limit if required.
-
-      # render_score_limit limits the number of nodes (string, variable, block, tags)
-      #   that are allowed in the template.
-      # render_length_limit seems to limit the sum of the bytesize of all node blocks.
-      # assign_score_limit seems to limit the sum of the bytesize of all capture blocks.
-    end
-
-    def handle_exception(exception, extra = {})
-      log_error(exception.message)
-      Gitlab::ErrorTracking.track_exception(exception, {
-        template_string: extra[:string],
-        variables: extra[:context]
-      })
-    end
-  end
-end

changelogs/unreleased/216618-remove-liquid.yml

+---
+title: Use gsub instead of the Liquid gem for variable substitution in the Prometheus
+  proxy API
+merge_request: 31482
+author:
+type: changed

doc/user/project/integrations/prometheus.md

@@ -194,7 +194,7 @@ Variables for Prometheus queries must be lowercase.
 
 There are 2 methods to specify a variable in a query or dashboard:
 
-1. Variables can be specified using the [Liquid template format](https://shopify.dev/docs/liquid/reference/basics), for example `{{ci_environment_slug}}` ([added](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/20793) in GitLab 12.6).
+1. Variables can be specified using double curly braces, such as `{{ci_environment_slug}}` ([added](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/20793) in GitLab 12.7).
 1. You can also enclose it in quotation marks with curly braces with a leading percent, for example `"%{ci_environment_slug}"`. This method is deprecated though and support will be [removed in the next major release](https://gitlab.com/gitlab-org/gitlab/issues/37990).
 
 #### Query Variables from URL

locale/gitlab.pot

@@ -8519,9 +8519,6 @@ msgstr ""
 msgid "Error rendering markdown preview"
 msgstr ""
 
-msgid "Error rendering query"
-msgstr ""
-
 msgid "Error saving label update."
 msgstr ""
 
@@ -12998,9 +12995,6 @@ msgstr ""
 msgid "Memory Usage"
 msgstr ""
 
-msgid "Memory limit exceeded while rendering template"
-msgstr ""
-
 msgid "Merge"
 msgstr ""
 

spec/services/prometheus/proxy_variable_substitution_service_spec.rb

@@ -142,7 +142,7 @@ describe Prometheus::ProxyVariableSubstitutionService do
         end
 
         it_behaves_like 'success' do
-          let(:expected_query) { 'up{pod_name=""}' }
+          let(:expected_query) { 'up{pod_name="{{pod_name}}"}' }
         end
       end
 
@@ -161,28 +161,6 @@ describe Prometheus::ProxyVariableSubstitutionService do
       end
     end
 
-    context 'with liquid tags and ruby format variables' do
-      let(:params_keys) do
-        {
-          query: 'up{ {% if true %}env1="%{ci_environment_slug}",' \
-            'env2="{{ci_environment_slug}}"{% endif %} }'
-        }
-      end
-
-      # The following spec will fail and should be changed to a 'success' spec
-      # once we remove support for the Ruby interpolation format.
-      # https://gitlab.com/gitlab-org/gitlab/issues/37990
-      #
-      # Liquid tags `{% %}` cannot be used currently because the Ruby `%`
-      # operator raises an error when it encounters a Liquid `{% %}` tag in the
-      # string.
-      #
-      # Once we remove support for the Ruby format, users can start using
-      # Liquid tags.
-
-      it_behaves_like 'error', 'Malformed string'
-    end
-
     context 'ruby template rendering' do
       let(:params_keys) do
         { query: 'up{env=%{ci_environment_slug},%{environment_filter}}' }
@@ -271,17 +249,79 @@ describe Prometheus::ProxyVariableSubstitutionService do
       end
     end
 
-    context 'when liquid template rendering raises error' do
-      before do
-        liquid_service = instance_double(TemplateEngines::LiquidService)
+    context 'gsub variable substitution tolerance for weirdness' do
+      context 'with whitespace around variable' do
+        let(:params_keys) do
+          {
+            query: 'up{' \
+                "env1={{ ci_environment_slug}}," \
+                "env2={{ci_environment_slug }}," \
+                "{{  environment_filter }}" \
+              '}'
+          }
+        end
 
-        allow(TemplateEngines::LiquidService).to receive(:new).and_return(liquid_service)
-        allow(liquid_service).to receive(:render).and_raise(
-          TemplateEngines::LiquidService::RenderError, 'error message'
-        )
+        it_behaves_like 'success' do
+          let(:expected_query) do
+            'up{' \
+              "env1=#{environment.slug}," \
+              "env2=#{environment.slug}," \
+              "container_name!=\"POD\",environment=\"#{environment.slug}\"" \
+            '}'
+          end
+        end
+      end
+
+      context 'with empty variables' do
+        let(:params_keys) do
+          { query: "up{env1={{}},env2={{  }}}" }
+        end
+
+        it_behaves_like 'success' do
+          let(:expected_query) { "up{env1={{}},env2={{  }}}" }
+        end
       end
 
-      it_behaves_like 'error', 'error message'
+      context 'with multiple occurrences of variable in string' do
+        let(:params_keys) do
+          { query: "up{env1={{ci_environment_slug}},env2={{ci_environment_slug}}}" }
+        end
+
+        it_behaves_like 'success' do
+          let(:expected_query) { "up{env1=#{environment.slug},env2=#{environment.slug}}" }
+        end
+      end
+
+      context 'with multiple variables in string' do
+        let(:params_keys) do
+          { query: "up{env={{ci_environment_slug}},{{environment_filter}}}" }
+        end
+
+        it_behaves_like 'success' do
+          let(:expected_query) do
+            "up{env=#{environment.slug}," \
+            "container_name!=\"POD\",environment=\"#{environment.slug}\"}"
+          end
+        end
+      end
+
+      context 'with unknown variables in string' do
+        let(:params_keys) { { query: "up{env={{env_slug}}}" } }
+
+        it_behaves_like 'success' do
+          let(:expected_query) { "up{env={{env_slug}}}" }
+        end
+      end
+
+      context 'with unknown and known variables in string' do
+        let(:params_keys) do
+          { query: "up{env={{ci_environment_slug}},other_env={{env_slug}}}" }
+        end
+
+        it_behaves_like 'success' do
+          let(:expected_query) { "up{env=#{environment.slug},other_env={{env_slug}}}" }
+        end
+      end
     end
   end
 end

spec/services/template_engines/liquid_service_spec.rb

-# frozen_string_literal: true
-
-require 'spec_helper'
-
-describe TemplateEngines::LiquidService do
-  describe '#render' do
-    let(:template) { 'up{env={{ci_environment_slug}}}' }
-    let(:result) { subject }
-
-    let_it_be(:slug) { 'env_slug' }
-
-    let_it_be(:context) do
-      {
-        ci_environment_slug: slug,
-        environment_filter: "container_name!=\"POD\",environment=\"#{slug}\""
-      }
-    end
-
-    subject { described_class.new(template).render(context) }
-
-    it 'with symbol keys in context it substitutes variables' do
-      expect(result).to include("up{env=#{slug}")
-    end
-
-    context 'with multiple occurrences of variable in template' do
-      let(:template) do
-        'up{env1={{ci_environment_slug}},env2={{ci_environment_slug}}}'
-      end
-
-      it 'substitutes variables' do
-        expect(result).to eq("up{env1=#{slug},env2=#{slug}}")
-      end
-    end
-
-    context 'with multiple variables in template' do
-      let(:template) do
-        'up{env={{ci_environment_slug}},' \
-        '{{environment_filter}}}'
-      end
-
-      it 'substitutes all variables' do
-        expect(result).to eq(
-          "up{env=#{slug}," \
-          "container_name!=\"POD\",environment=\"#{slug}\"}"
-        )
-      end
-    end
-
-    context 'with unknown variables in template' do
-      let(:template) { 'up{env={{env_slug}}}' }
-
-      it 'does not substitute unknown variables' do
-        expect(result).to eq("up{env=}")
-      end
-    end
-
-    context 'with extra variables in context' do
-      let(:template) { 'up{env={{ci_environment_slug}}}' }
-
-      it 'substitutes variables' do
-        # If context has only 1 key, there is no need for this spec.
-        expect(context.count).to be > 1
-        expect(result).to eq("up{env=#{slug}}")
-      end
-    end
-
-    context 'with unknown and known variables in template' do
-      let(:template) { 'up{env={{ci_environment_slug}},other_env={{env_slug}}}' }
-
-      it 'substitutes known variables' do
-        expect(result).to eq("up{env=#{slug},other_env=}")
-      end
-    end
-
-    context 'Liquid errors' do
-      shared_examples 'raises RenderError' do |message|
-        it do
-          expect { result }.to raise_error(described_class::RenderError, message)
-        end
-      end
-
-      context 'when liquid raises error' do
-        let(:template) { 'up{env={{ci_environment_slug}}' }
-        let(:liquid_template) { Liquid::Template.new }
-
-        before do
-          allow(Liquid::Template).to receive(:parse).with(template).and_return(liquid_template)
-          allow(liquid_template).to receive(:render!).and_raise(exception, message)
-        end
-
-        context 'raises Liquid::MemoryError' do
-          let(:exception) { Liquid::MemoryError }
-          let(:message) { 'Liquid error: Memory limits exceeded' }
-
-          it_behaves_like 'raises RenderError', 'Memory limit exceeded while rendering template'
-        end
-
-        context 'raises Liquid::Error' do
-          let(:exception) { Liquid::Error }
-          let(:message) { 'Liquid error: Generic error message' }
-
-          it_behaves_like 'raises RenderError', 'Error rendering query'
-        end
-      end
-
-      context 'with template that is expensive to render' do
-        let(:template) do
-          '{% assign loop_count     = 1000 %}'\
-          '{% assign padStr  = "0" %}'\
-          '{% assign number_to_pad = "1" %}'\
-          '{% assign strLength = number_to_pad | size %}'\
-          '{% assign padLength = loop_count | minus: strLength %}'\
-          '{% if padLength > 0 %}'\
-          '  {% assign padded = number_to_pad %}'\
-          '  {% for position in (1..padLength) %}'\
-          '    {% assign padded = padded | prepend: padStr %}'\
-          '  {% endfor %}'\
-          '  {{ padded }}'\
-          '{% endif %}'
-        end
-
-        it_behaves_like 'raises RenderError', 'Memory limit exceeded while rendering template'
-      end
-    end
-  end
-end