diff --git a/lib/twig/extension/core.rb b/lib/twig/extension/core.rb index 2fc9b5f..44c8854 100644 --- a/lib/twig/extension/core.rb +++ b/lib/twig/extension/core.rb @@ -5,6 +5,10 @@ module Extension class Core < Base DEFAULT_TRIM_CHARS = " \t\n\r\0\x0B" + # Returned by subscript_key when nothing matched. A sentinel rather than + # nil, because nil is a perfectly good hash key and a perfectly good value. + KEY_NOT_FOUND = Object.new.freeze + class << self include ActiveSupport::NumberHelper end @@ -860,25 +864,64 @@ def self.matches(regexp, string) raise Error::Runtime, "Invalid regular expression passed to matches: #{e.message}" end + # The key or index that +attribute+ matches on a subscriptable object, or + # KEY_NOT_FOUND when it matches none. + # + # Returning the key rather than the value is the point: it lets the caller + # subscript exactly once. Fetching first and testing the result instead — + # as `object[attribute] || object[attribute.to_s]` did — can't tell a key + # holding nil or false from a key that isn't there, so { 'billable' => + # false } came back as nil. + # + # @return [Object] the matching key, or KEY_NOT_FOUND + def self.subscript_key(object, attribute) + if object.is_a?(Array) + return KEY_NOT_FOUND unless attribute.is_a?(Integer) + + # Ruby indexes from the end with a negative, so -1 is the last element + # while anything below -length is as absent as anything above length. + in_bounds = attribute >= -object.length && attribute < object.length + + return in_bounds ? attribute : KEY_NOT_FOUND + end + + return KEY_NOT_FOUND unless object.respond_to?(:key?) + return attribute if object.key?(attribute) + + # A template writes h.foo, h['foo'] and h[:foo] for the same Ruby hash, + # so both forms of the name count as a match. + symbol = attribute.to_sym if attribute.respond_to?(:to_sym) + return symbol if !symbol.nil? && object.key?(symbol) + + string = attribute.to_s if attribute.respond_to?(:to_s) + return string if !string.nil? && object.key?(string) + + KEY_NOT_FOUND + end + + # Names the keys that do exist rather than dumping the object, which for + # anything the size of a request's parameters buries the one absent key + # under everything else. + # + # @return [String] + def self.key_not_found_message(object, attribute) + keys = object.respond_to?(:keys) ? " with keys \"#{object.keys.join(', ')}\"" : '' + + "Key \"#{attribute}\" for sequence/mapping#{keys} does not exist." + end + # @param [Environment] environment def self.get_attribute( environment, source, object, attribute, type, arguments: {}, defined_test: false, ignore_strict_check: false, lineno: -1, & ) if type == Template::ARRAY_CALL || object.respond_to?(:[]) - if object.respond_to?(:[]) && ( - (object.is_a?(Array) && attribute.is_a?(Integer) && attribute < object.length) || - ( - object.respond_to?(:key?) && ( - object.key?(attribute) || - (attribute.respond_to?(:to_sym) && object.key?(attribute.to_sym)) || - (attribute.respond_to?(:to_s) && object.key?(attribute.to_s)) - ) - ) - ) + key = object.respond_to?(:[]) ? subscript_key(object, attribute) : KEY_NOT_FOUND + + unless key.equal?(KEY_NOT_FOUND) return true if defined_test - return object[attribute] || (attribute.is_a?(String) ? object[attribute.to_sym] : object[attribute.to_s]) + return object[key] end if type == Template::ARRAY_CALL @@ -890,7 +933,11 @@ def self.get_attribute( return end - raise Error::Runtime, "Can't find key #{attribute} in #{object.inspect}." + raise Error::Runtime.new( + key_not_found_message(object, attribute), + lineno, + source + ) end end @@ -925,19 +972,22 @@ def self.get_attribute( kwargs = kwargs.transform_keys(&:to_sym) + # public_send, not send: respond_to? above already excludes private and + # protected methods, but an object with a loose respond_to_missing? + # can report true for one. Templates only reach the public interface. if positional.length.positive? && kwargs.empty? - object.send(attribute, *positional, &) + object.public_send(attribute, *positional, &) elsif positional.empty? && kwargs.length.positive? - object.send(attribute, **kwargs, &) + object.public_send(attribute, **kwargs, &) elsif positional.length.positive? && kwargs.length.positive? - object.send(attribute, *positional, **kwargs, &) + object.public_send(attribute, *positional, **kwargs, &) else - case object - when Hash, Array - object[attribute] - else - object.send(attribute, &) - end + # Reached only when the object responds to the attribute and the + # key/index lookup above did not match, so subscripting a Hash or + # Array here could only ever miss — it raised TypeError on arrays + # ({{ list.any? }}) and returned nil on hashes, silently rendering + # nothing. Collections dispatch methods like any other object. + object.public_send(attribute, &) end # Constant could be nil but we should return if we find it elsif (constant = get_constant(object, attribute.to_s)) && constant[0] == :found @@ -952,8 +1002,7 @@ def self.get_attribute( message = if object.nil? "Impossible to access an attribute (\"#{attribute}\") on a null variable." elsif object.respond_to?(:[]) && !object.is_a?(String) - keys = object.respond_to?(:keys) ? "with keys \"#{object.keys.join(', ')}\"" : '' - "Key \"#{attribute}\" for sequence/mapping #{keys} does not exist." + key_not_found_message(object, attribute) else "Impossible to access an attribute (\"#{attribute}\") on a #{object.class} " \ "variable (\"#{object}\")." diff --git a/lib/twig/version.rb b/lib/twig/version.rb new file mode 100644 index 0000000..4c7b495 --- /dev/null +++ b/lib/twig/version.rb @@ -0,0 +1,5 @@ +# frozen_string_literal: true + +module Twig + VERSION = '0.0.9' +end diff --git a/spec/lib/twig/extension/core_spec.rb b/spec/lib/twig/extension/core_spec.rb index df31e59..3173a5a 100644 --- a/spec/lib/twig/extension/core_spec.rb +++ b/spec/lib/twig/extension/core_spec.rb @@ -482,6 +482,10 @@ def each describe '#get_attribute' do let(:loader) { Twig::Loader::Hash.new({}) } let(:environment) { Twig::Environment.new(loader) } + let(:strict) { Twig::Environment.new(loader, { strict_variables: true }) } + # A missing key raises with the source attached, so the double has to answer + # what Error::Base asks of it. + let(:source) { instance_double(Twig::Source, path: nil, name: nil) } it 'does not use bracket access if doing a method call' do klass = Class.new do @@ -518,5 +522,145 @@ def foo ) ).to eq(3.14) end + + def attribute_of(object, attribute, env = environment) + described_class.get_attribute(env, source, object, attribute, Twig::Template::METHOD_CALL) + end + + def subscript_of(object, attribute, env = environment) + described_class.get_attribute(env, source, object, attribute, Twig::Template::ARRAY_CALL) + end + + context 'when the key is missing' do + it 'names the keys that exist rather than dumping the object' do + expect { subscript_of({ 'a' => 'x', 'b' => 'y' }, 'c', strict) }. + to raise_error(Twig::Error::Runtime, %r{Key "c" for sequence/mapping with keys "a, b" does not exist}) + end + + it 'reports the same way for bracket and dot access' do + object = { 'a' => 'x' } + bracket = begin + subscript_of(object, 'c', strict) + rescue Twig::Error::Runtime => e + e.message + end + dot = begin + attribute_of(object, 'c', strict) + rescue Twig::Error::Runtime => e + e.message + end + + expect(bracket).to eq(dot) + end + + it 'does not double the space when the object has no keys' do + expect { subscript_of(%w[a b], 5, strict) }. + to raise_error(Twig::Error::Runtime, %r{sequence/mapping does not exist}) + end + end + + context 'with a key holding a falsy value' do + it 'returns false for a string key reached by a string name' do + expect(subscript_of({ 'flag' => false }, 'flag')).to be(false) + expect(attribute_of({ 'flag' => false }, 'flag')).to be(false) + end + + it 'returns false for a symbol key reached by a symbol name' do + expect(subscript_of({ flag: false }, :flag)).to be(false) + end + + it 'returns false across the string/symbol divide' do + expect(subscript_of({ flag: false }, 'flag')).to be(false) + expect(subscript_of({ 'flag' => false }, :flag)).to be(false) + end + + it 'returns nil for a key that genuinely holds nil' do + expect(subscript_of({ 'flag' => nil }, 'flag')).to be_nil + end + + it 'prefers the key that matches over the other form of the name' do + # Both keys exist; the one the template asked for wins, even though it + # holds nil and the other holds something truthy. + expect(subscript_of({ 'a' => nil, a: 1 }, 'a')).to be_nil + expect(subscript_of({ 'a' => 1, a: nil }, :a)).to be_nil + end + + it 'reports a falsy value as defined' do + expect( + described_class.get_attribute( + environment, + instance_double(Twig::Source), + { 'flag' => false }, + 'flag', + Twig::Template::ARRAY_CALL, + defined_test: true + ) + ).to be(true) + end + end + + context 'with an array index' do + it 'reads from the end with a negative index' do + expect(subscript_of(%w[a b c], -1)).to eq('c') + expect(subscript_of(%w[a b c], -3)).to eq('a') + end + + it 'treats an out-of-range negative index as missing, not as a TypeError' do + expect { subscript_of(%w[a b c], -99, strict) }. + to raise_error(Twig::Error::Runtime, /-99/) + end + + it 'treats an out-of-range positive index as missing' do + expect { subscript_of(%w[a b c], 99, strict) }. + to raise_error(Twig::Error::Runtime, /99/) + end + + it 'returns nil for an out-of-range index when not strict' do + expect(subscript_of(%w[a b c], -99)).to be_nil + expect(subscript_of(%w[a b c], 99)).to be_nil + end + end + + context 'with a collection' do + it 'calls methods on an array rather than subscripting with the name' do + expect(attribute_of(%w[a b], :any?)).to be(true) + expect(attribute_of(%w[a b], :size)).to eq(2) + expect(attribute_of([], :any?)).to be(false) + end + + it 'calls methods on a hash rather than returning nil' do + expect(attribute_of({ 'a' => 1 }, :any?)).to be(true) + expect(attribute_of({}, :any?)).to be(false) + end + + it 'still prefers a real key over a method of the same name' do + expect(attribute_of({ 'size' => 'from key' }, 'size')).to eq('from key') + expect(attribute_of({ 'first' => 'Ada' }, 'first')).to eq('Ada') + end + + it 'still prefers a real index over a method name' do + expect(attribute_of(%w[a b], 0)).to eq('a') + end + end + + context 'with a method the object only claims to respond to' do + let(:liar) do + Class.new do + def respond_to_missing?(name, include_all = false) + name.to_sym == :hidden || super + end + + private + + def hidden + 'private' + end + end.new + end + + it 'does not reach private methods' do + expect { attribute_of(liar, :hidden) }.to raise_error(NoMethodError) + end + end end end diff --git a/twig-ruby.gemspec b/twig-ruby.gemspec index 3788f76..5bba5a6 100644 --- a/twig-ruby.gemspec +++ b/twig-ruby.gemspec @@ -1,8 +1,13 @@ # frozen_string_literal: true +# require_relative rather than requiring the gem: this needs the version and +# nothing else, and evaluating the whole library to read one constant would pull +# ActiveSupport in at packaging time. +require_relative 'lib/twig/version' + Gem::Specification.new do |s| s.name = 'twig_ruby' - s.version = '0.0.8' + s.version = Twig::VERSION s.summary = 'Twig Templating for Ruby' s.description = '' s.authors = ['Craig Blanchette', 'Fabian Potencier']