From 4e54eec4b70a7065eda5bc1103b2cebc1e28eeb7 Mon Sep 17 00:00:00 2001 From: Didier Lafforgue Date: Fri, 20 Sep 2024 19:27:49 +0200 Subject: [PATCH] improve the performance of the with_scope attributes parsing + use a different name of the simple version of the parser --- lib/locomotive/steam/liquid.rb | 2 +- .../liquid/tags/concerns/attributes_parser.rb | 142 +++++++++++ ...ributes.rb => simple_attributes_parser.rb} | 3 +- lib/locomotive/steam/liquid/tags/consume.rb | 2 +- .../steam/liquid/tags/editable/base.rb | 2 +- .../steam/liquid/tags/inherited_block.rb | 2 +- lib/locomotive/steam/liquid/tags/link_to.rb | 2 +- .../steam/liquid/tags/locale_switcher.rb | 3 +- .../steam/liquid/tags/model_form.rb | 2 +- lib/locomotive/steam/liquid/tags/paginate.rb | 2 +- lib/locomotive/steam/liquid/tags/path_to.rb | 2 +- .../steam/liquid/tags/redirect_to.rb | 2 +- lib/locomotive/steam/liquid/tags/section.rb | 2 +- .../steam/liquid/tags/with_scope.rb | 228 +----------------- spec/unit/liquid/tags/with_scope_spec.rb | 15 +- 15 files changed, 173 insertions(+), 238 deletions(-) create mode 100644 lib/locomotive/steam/liquid/tags/concerns/attributes_parser.rb rename lib/locomotive/steam/liquid/tags/concerns/{attributes.rb => simple_attributes_parser.rb} (96%) diff --git a/lib/locomotive/steam/liquid.rb b/lib/locomotive/steam/liquid.rb index 52f13b61..5d31bf51 100644 --- a/lib/locomotive/steam/liquid.rb +++ b/lib/locomotive/steam/liquid.rb @@ -9,6 +9,6 @@ require_relative 'liquid/drops/i18n_base' require_relative 'liquid/tags/hybrid' require_relative 'liquid/tags/concerns/section' -require_relative 'liquid/tags/concerns/attributes' +require_relative 'liquid/tags/concerns/simple_attributes_parser' require_relative 'liquid/tags/section' require_relative_all %w(. drops filters tags/concerns tags), 'liquid' diff --git a/lib/locomotive/steam/liquid/tags/concerns/attributes_parser.rb b/lib/locomotive/steam/liquid/tags/concerns/attributes_parser.rb new file mode 100644 index 00000000..2f570610 --- /dev/null +++ b/lib/locomotive/steam/liquid/tags/concerns/attributes_parser.rb @@ -0,0 +1,142 @@ +module Locomotive + module Steam + module Liquid + module Tags + module Concerns + + # The with_scope liquid tag lets the developer use a Ruby syntax to + # pass options which is difficult to implement with the Liquid parsing + # approach (see the SimpleAttributesParser for instance) + module AttributesParser + extend ActiveSupport::Concern + + def parse_markup(markup) + parser = self.class.current_parser + + # 'liquid_code.rb' is purely arbitrary + source_buffer = ::Parser::Source::Buffer.new('liquid_code.rb') + source_buffer.source = "{%s}" % markup + + ast = parser.parse(source_buffer) + AstProcessor.new.process(ast) + end + + class_methods do + def current_parser + (@current_parser ||= build_parser).tap do |parser| + parser.reset + end + end + + def build_parser + ::Parser::CurrentRuby.new.tap do |parser| + # Silent the error instead of logging them to STDERR (default behavior of the parser) + parser.diagnostics.consumer = ->(message) { true } + end + end + end + + class AstProcessor + include AST::Processor::Mixin + + def on_hash(node) + nodes = process_all(node) + nodes.inject({}) { |memo, sub_hash| memo.merge(sub_hash) } + end + + def on_pair(node) + key_expr, right_expr = *node + { process(key_expr) => process(right_expr) } + end + + def on_sym(node) + node.children.first.to_sym + end + + def on_array(node) + process_all(node) + end + + def on_int(node) + node.children.first.to_i + end + + def on_float(node) + node.children.first.to_f + end + + def on_str(node) + node.children.first.to_s + end + + def on_true(node) + true + end + + def on_false(node) + false + end + + def on_regexp(node) + regexp_expr, opts_expr = *node + Regexp.new(process(regexp_expr), process(opts_expr)) + end + + def on_regopt(node) + node.children ? node.children.join('') : nil + end + + def on_deep_send(node) + source_expr, name_expr = *node + + if source_expr.nil? + [name_expr.to_s] + elsif source_expr.type == :send + process(source_expr.updated(:deep_send, nil)) << name_expr.to_s + else + raise 'NOT IMPLEMENTED [DEEP_SEND]' # TODO + end + end + + def on_send(node) + source_expr, name_expr = *node + + if source_expr.nil? + ::Liquid::Expression.parse(name_expr.to_s) + elsif name_expr == :+ + process(source_expr) + elsif source_expr.type == :send + ::Liquid::Expression.parse( + (process(source_expr.updated(:deep_send, nil)) << name_expr.to_s).join('.') + ) + else + raise 'NOT IMPLEMENTED [SEND]' # TODO + end + end + + # HACK: override the default process implementation + def process(node) + return if node.nil? + + node = node.to_ast + + # Invoke a specific handler + on_handler = :"on_#{node.type}" + if respond_to? on_handler + new_node = send on_handler, node + else + new_node = handler_missing(node) + end + + # fix: the original method considered false as nil which is incorrect + node = new_node unless new_node.nil? + + node + end + end + end + end + end + end + end +end \ No newline at end of file diff --git a/lib/locomotive/steam/liquid/tags/concerns/attributes.rb b/lib/locomotive/steam/liquid/tags/concerns/simple_attributes_parser.rb similarity index 96% rename from lib/locomotive/steam/liquid/tags/concerns/attributes.rb rename to lib/locomotive/steam/liquid/tags/concerns/simple_attributes_parser.rb index f85b63a6..29e04937 100644 --- a/lib/locomotive/steam/liquid/tags/concerns/attributes.rb +++ b/lib/locomotive/steam/liquid/tags/concerns/simple_attributes_parser.rb @@ -7,8 +7,7 @@ module Concerns # Many of Liquid tags have attributes (like options) # This module makes sure we use the same reliable way to # extract and evaluate them. - - module Attributes + module SimpleAttributesParser attr_reader :attributes, :raw_attributes diff --git a/lib/locomotive/steam/liquid/tags/consume.rb b/lib/locomotive/steam/liquid/tags/consume.rb index 90fc1be7..7946ab63 100644 --- a/lib/locomotive/steam/liquid/tags/consume.rb +++ b/lib/locomotive/steam/liquid/tags/consume.rb @@ -15,7 +15,7 @@ module Tags # class Consume < ::Liquid::Block - include Concerns::Attributes + include Concerns::SimpleAttributesParser Syntax = /(#{::Liquid::VariableSignature}+)\s*from\s*(#{::Liquid::QuotedFragment}+),?(.+)?/o.freeze diff --git a/lib/locomotive/steam/liquid/tags/editable/base.rb b/lib/locomotive/steam/liquid/tags/editable/base.rb index 9f02a9d0..ce2e3aa5 100644 --- a/lib/locomotive/steam/liquid/tags/editable/base.rb +++ b/lib/locomotive/steam/liquid/tags/editable/base.rb @@ -5,7 +5,7 @@ module Tags module Editable class Base < ::Liquid::Block - include Concerns::Attributes + include Concerns::SimpleAttributesParser Syntax = /(#{::Liquid::QuotedFragment})(\s*,\s*#{::Liquid::Expression}+)?/o diff --git a/lib/locomotive/steam/liquid/tags/inherited_block.rb b/lib/locomotive/steam/liquid/tags/inherited_block.rb index b2476bfe..2209566a 100644 --- a/lib/locomotive/steam/liquid/tags/inherited_block.rb +++ b/lib/locomotive/steam/liquid/tags/inherited_block.rb @@ -15,7 +15,7 @@ module Tags # class InheritedBlock < ::Liquid::Block - include Concerns::Attributes + include Concerns::SimpleAttributesParser SYNTAX = /(#{::Liquid::QuotedFragment}+)/o diff --git a/lib/locomotive/steam/liquid/tags/link_to.rb b/lib/locomotive/steam/liquid/tags/link_to.rb index d07f5a34..f21d026a 100644 --- a/lib/locomotive/steam/liquid/tags/link_to.rb +++ b/lib/locomotive/steam/liquid/tags/link_to.rb @@ -4,7 +4,7 @@ module Liquid module Tags class LinkTo < Hybrid - include Concerns::Attributes + include Concerns::SimpleAttributesParser include Concerns::I18nPage include Concerns::Path diff --git a/lib/locomotive/steam/liquid/tags/locale_switcher.rb b/lib/locomotive/steam/liquid/tags/locale_switcher.rb index 66065285..e9fd46ec 100644 --- a/lib/locomotive/steam/liquid/tags/locale_switcher.rb +++ b/lib/locomotive/steam/liquid/tags/locale_switcher.rb @@ -19,10 +19,9 @@ module Tags # - "iso" is the default choice for label # - " | " is the default separating code # - class LocaleSwitcher < ::Liquid::Tag - include Concerns::Attributes + include Concerns::SimpleAttributesParser include Concerns::I18nPage attr_reader :attributes, :site, :page, :current_locale, :url_builder diff --git a/lib/locomotive/steam/liquid/tags/model_form.rb b/lib/locomotive/steam/liquid/tags/model_form.rb index 3ca90c34..3f5a939d 100644 --- a/lib/locomotive/steam/liquid/tags/model_form.rb +++ b/lib/locomotive/steam/liquid/tags/model_form.rb @@ -18,7 +18,7 @@ module Tags # class ModelForm < ::Liquid::Block - include Concerns::Attributes + include Concerns::SimpleAttributesParser Syntax = /(#{::Liquid::QuotedFragment})\s*,*(.*)?/o.freeze diff --git a/lib/locomotive/steam/liquid/tags/paginate.rb b/lib/locomotive/steam/liquid/tags/paginate.rb index 6ec5863d..e0bb4283 100644 --- a/lib/locomotive/steam/liquid/tags/paginate.rb +++ b/lib/locomotive/steam/liquid/tags/paginate.rb @@ -15,7 +15,7 @@ module Tags # class Paginate < ::Liquid::Block - include Concerns::Attributes + include Concerns::SimpleAttributesParser Syntax = /(#{::Liquid::QuotedFragment}+)\s+by\s+(#{::Liquid::QuotedFragment}+)/o diff --git a/lib/locomotive/steam/liquid/tags/path_to.rb b/lib/locomotive/steam/liquid/tags/path_to.rb index 03f71eff..16af0af3 100644 --- a/lib/locomotive/steam/liquid/tags/path_to.rb +++ b/lib/locomotive/steam/liquid/tags/path_to.rb @@ -4,7 +4,7 @@ module Liquid module Tags class PathTo < ::Liquid::Tag - include Concerns::Attributes + include Concerns::SimpleAttributesParser include Concerns::I18nPage include Concerns::Path diff --git a/lib/locomotive/steam/liquid/tags/redirect_to.rb b/lib/locomotive/steam/liquid/tags/redirect_to.rb index 2b1040e3..188530b9 100644 --- a/lib/locomotive/steam/liquid/tags/redirect_to.rb +++ b/lib/locomotive/steam/liquid/tags/redirect_to.rb @@ -5,7 +5,7 @@ module Tags class RedirectTo < ::Liquid::Tag - include Concerns::Attributes + include Concerns::SimpleAttributesParser include Concerns::I18nPage include Concerns::Path diff --git a/lib/locomotive/steam/liquid/tags/section.rb b/lib/locomotive/steam/liquid/tags/section.rb index c3c28da4..4c92f70e 100644 --- a/lib/locomotive/steam/liquid/tags/section.rb +++ b/lib/locomotive/steam/liquid/tags/section.rb @@ -5,7 +5,7 @@ module Tags class Section < ::Liquid::Include include Concerns::Section - include Concerns::Attributes + include Concerns::SimpleAttributesParser Syntax = /(#{::Liquid::QuotedString}|#{::Liquid::VariableSignature}+)\s*,*(.*)?/o.freeze diff --git a/lib/locomotive/steam/liquid/tags/with_scope.rb b/lib/locomotive/steam/liquid/tags/with_scope.rb index ec6d0a2a..82ac9a0f 100644 --- a/lib/locomotive/steam/liquid/tags/with_scope.rb +++ b/lib/locomotive/steam/liquid/tags/with_scope.rb @@ -13,129 +13,14 @@ module Tags # {% endfor %} # {% endwith_scope %} # - - # s(:hash, - # s(:pair, - # s(:sym, :key), - # s(:array, - # s(:int, 1), - # s(:int, 2), - # s(:int, 3)))) class WithScope < ::Liquid::Block - class HashProcessor - include AST::Processor::Mixin - - def on_hash(node) - nodes = process_all(node) - nodes.inject({}) { |memo, sub_hash| memo.merge(sub_hash) } - end - - def on_pair(node) - key_expr, right_expr = *node - { process(key_expr) => process(right_expr) } - end - - def on_sym(node) - node.children.first.to_sym - end - - def on_array(node) - process_all(node) - end - - def on_int(node) - node.children.first.to_i - end - - def on_float(node) - node.children.first.to_f - end - - def on_str(node) - node.children.first.to_s - end - - def on_true(node) - true - end - - def on_false(node) - false - end - - def on_regexp(node) - regexp_expr, opts_expr = *node - Regexp.new(process(regexp_expr), process(opts_expr)) - end - - def on_regopt(node) - node.children ? node.children.join('') : nil - end - - def on_deep_send(node) - source_expr, name_expr = *node - - if source_expr.nil? - [name_expr.to_s] - elsif source_expr.type == :send - process(source_expr.updated(:deep_send, nil)) << name_expr.to_s - else - raise 'NOT IMPLEMENTED [DEEP_SEND]' # TODO - end - end - - def on_send(node) - # pp node.location - # pp node.location.expression.source - - ::Liquid::Expression.parse(node.location.expression.source) - - # raise 'TODO' - - # source_expr, name_expr = *node - - # if source_expr.nil? - # ::Liquid::Expression.parse(name_expr.to_s) - # elsif source_expr.type == :send - # ::Liquid::Expression.parse( - # (process(source_expr.updated(:deep_send, nil)) << name_expr.to_s).join('.') - # ) - # else - # raise 'NOT IMPLEMENTED [SEND]' # TODO - # end - end - - # TODO: create our own mixin - def process(node) - return if node.nil? + include Concerns::AttributesParser - node = node.to_ast - - # Invoke a specific handler - on_handler = :"on_#{node.type}" - if respond_to? on_handler - new_node = send on_handler, node - else - new_node = handler_missing(node) - end - - node = new_node unless new_node.nil? - - node - end - end - - include Concerns::Attributes - - # Regexps and Arrays are allowed - ArrayFragment = /\[(\s*(#{::Liquid::QuotedFragment},\s*)*#{::Liquid::QuotedFragment}\s*)\]/o.freeze + # Regexps are allowed as strings RegexpFragment = /\/([^\/]+)\/([imx]+)?/o.freeze StrictRegexpFragment = /\A#{RegexpFragment}\z/o.freeze - # a slight different from the Shopify implementation because we allow stuff like `started_at.le` - TagAttributes = /([a-zA-Z_0-9\.]+)\s*\:\s*(#{ArrayFragment}|#{RegexpFragment}|#{::Liquid::QuotedFragment})/o.freeze - # SingleVariable = /(#{::Liquid::VariableSignature}+)/om.freeze SingleVariable = /\A\s*([a-zA-Z_0-9]+)\s*\z/om.freeze REGEX_OPTIONS = { @@ -148,53 +33,29 @@ def process(node) SYMBOL_OPERATORS_REGEXP = /(\w+\.(#{OPERATORS.join('|')})){1}\s*\:/o - attr_reader :attributes, :attributes_var_name, :ast + attr_reader :attributes, :attributes_var_name def initialize(tag_name, markup, options) - super # convert symbol operators into valid ruby code markup.gsub!(SYMBOL_OPERATORS_REGEXP, ':"\1" =>') - # pp markup - if markup =~ SingleVariable - # puts "HERE?" # alright, maybe we'vot got a single variable built # with the Action liquid tag instead? @attributes_var_name = Regexp.last_match(1) elsif markup.present? - # ast = Parser::CurrentRuby.parse("{%s}" % markup) - # pp ast - # @attributes = HashProcessor.new.process(ast) - # puts "-----" - # pp @attributes + # use our own Ruby parser @attributes = parse_markup(markup) end if attributes.blank? && attributes_var_name.blank? raise ::Liquid::SyntaxError.new("Syntax Error in 'with_scope' - Valid syntax: with_scope : , ..., : ") end - - # raise 'TODO' - - # # simple hash? - # parse_attributes(markup) { |value| parse_attribute(value) } - - # if attributes.empty? && markup =~ SingleVariable - # # alright, maybe we'vot got a single variable built - # # with the Action liquid tag instead? - # @attributes_var_name = Regexp.last_match(1) - # end - - # if attributes.empty? && attributes_var_name.blank? - # raise ::Liquid::SyntaxError.new("Syntax Error in 'with_scope' - Valid syntax: with_scope : , ..., : ") - # end end def render(context) - pp attributes if ENV['WITH_SCOPE_DEBUG'] context.stack do context['with_scope'] = self.evaluate_attributes(context) @@ -207,50 +68,6 @@ def render(context) protected - def parse_markup(markup) - # begin - parser = nil - ast = nil - source_buffer = nil - - puts "init:" - puts Benchmark.measure { parser = Parser::CurrentRuby.new } - # Silent the error instead of logging them to STDERR (default behavior of the parser) - - puts "consumer:" - puts Benchmark.measure { parser.diagnostics.consumer = ->(message) { true } } - - # 'with_scope.rb' is purely arbitrary - puts "source_buffer:" - puts Benchmark.measure { source_buffer = Parser::Source::Buffer.new('with_scope.rb') } - - source_buffer.source = "{%s}" % markup - - puts "ast: " - puts Benchmark.measure { ast = parser.parse(source_buffer) } - - foo = nil - puts "visit ast: " - puts Benchmark.measure { foo = HashProcessor.new.process(ast) } - foo - # rescue StandardError => e - # TODO: log something??? - # {} - # end - end - - # def parse_attribute(value) - # case value - # when StrictRegexpFragment - # # let the cast_value attribute create the Regexp (done during the rendering phase) - # value - # when ArrayFragment - # $1.split(',').map { |_value| parse_attribute(_value) } - # else - # ::Liquid::Expression.parse(value) - # end - # end - def evaluate_attributes(context) @attributes = context[attributes_var_name] || {} if attributes_var_name.present? @@ -258,65 +75,32 @@ def evaluate_attributes(context) # _slug instead of _permalink _key = key.to_s == '_permalink' ? '_slug' : key.to_s - # puts [_key, evaluate_attribute(context, value)] - memo.merge({ _key => evaluate_attribute(context, value) }) end end def evaluate_attribute(context, value) - # pp "evaluate_attribute = #{value}" case value when Array value.map { |v| evaluate_attribute(context, v) } when Hash Hash[value.map { |k, v| [k.to_s, evaluate_attribute(context, v)] }] + when StrictRegexpFragment + create_regexp($1, $2) when ::Liquid::VariableLookup evaluated_value = context.evaluate(value) evaluated_value.respond_to?(:_id) ? evaluated_value.send(:_source) : evaluate_attribute(context, evaluated_value) - when StrictRegexpFragment - create_regexp($1, $2) else value end end - # def evaluate_attributes(context) - # @attributes = context[attributes_var_name] || {} if attributes_var_name.present? - - # HashWithIndifferentAccess.new.tap do |hash| - # attributes.each do |key, value| - # # _slug instead of _permalink - # _key = key.to_s == '_permalink' ? '_slug' : key.to_s - - # # evaluate the value if possible before casting it - # _value = value.is_a?(::Liquid::VariableLookup) ? context.evaluate(value) : value - - # hash[_key] = cast_value(context, _value) - # end - # end - # end - - # def cast_value(context, value) - # case value - # when Array then value.map { |_value| cast_value(context, _value) } - # when StrictRegexpFragment then create_regexp($1, $2) - # else - # _value = context.evaluate(value) - # _value.respond_to?(:_id) ? _value.send(:_source) : _value - # end - # end - def create_regexp(value, unparsed_options) options = unparsed_options.blank? ? nil : unparsed_options.split('').uniq.inject(0) do |_options, letter| _options |= REGEX_OPTIONS[letter] end Regexp.new(value, options) end - - def tag_attributes_regexp - TagAttributes - end end ::Liquid::Template.register_tag('with_scope'.freeze, WithScope) diff --git a/spec/unit/liquid/tags/with_scope_spec.rb b/spec/unit/liquid/tags/with_scope_spec.rb index 1fbf7c2d..a1482522 100644 --- a/spec/unit/liquid/tags/with_scope_spec.rb +++ b/spec/unit/liquid/tags/with_scope_spec.rb @@ -52,9 +52,20 @@ end + describe 'don\'t decode numeric operations' do + let(:source) { "{% with_scope price: 41 + 1 %}{% assign conditions = with_scope %}{% endwith_scope %}" } + it { expect(conditions['price']).to eq 41 } + + context 'the operation calls a variable' do + let(:assigns) { { 'prices' => { 'low' => 41 } } } + let(:source) { "{% with_scope price: prices.low + 1 %}{% assign conditions = with_scope %}{% endwith_scope %}" } + it { expect(conditions['price']).to eq 41 } + end + end + describe 'decode basic options (boolean, integer, ...)' do - let(:source) { "{% with_scope active: true, price: 41 + 1, title: 'foo', hidden: false %}{% assign conditions = with_scope %}{% endwith_scope %}" } + let(:source) { "{% with_scope active: true, price: 42, title: 'foo', hidden: false %}{% assign conditions = with_scope %}{% endwith_scope %}" } it { expect(conditions['active']).to eq true } it { expect(conditions['price']).to eq 42 } @@ -138,7 +149,7 @@ describe 'decode criteria with gt and lt' do - let(:source) { "{% with_scope price.gt:42.0, price.lt:50, published_at.lte: '2019-09-10 00:00:00', published_at.gte: '2019/09/09 00:00:00' %}{% assign conditions = with_scope %}{% endwith_scope %}" } + let(:source) { "{% with_scope price.gt: 42.0, price.lt:50, published_at.lte: '2019-09-10 00:00:00', published_at.gte: '2019/09/09 00:00:00' %}{% assign conditions = with_scope %}{% endwith_scope %}" } it { expect(conditions['price.gt']).to eq 42.0 } it { expect(conditions['price.lt']).to eq 50 } it { expect(conditions['published_at.lte']).to eq '2019-09-10 00:00:00' }