Consolidate contextualization of inputs via the context #1537

tjoyal · 2022-03-07T17:49:40Z

This PR is all over the place, I am sorry for that. I found it hard to separate isolated deliverables as there is a lot of inter-dependency at play.

This PR adds Context#contextualize which acts as a central hub to handle logic regarding transformation of variable into liquid aware representation.

This abstraction handles Proc resolutions when needed, the call to to_liquid and assignment of context via context=.

Changes

Context:

Pushed contextualization from find_variable to lookup_and_evaluate. No behavior change specific to this class, this will allow for VariableLookup#evaluate to re-use said logic.

VariableLookup:

Main path can delegate the responsibility of contextualization to Context#lookup_and_evaluate, allowing some of the logic duplication to be removed.
The "command methods" have to deal with manual contextualization have been updated to the unified code path. Context#contextualize logic properly handles Proc which solves for the newly added tests who would have been failing otherwise. We could look at moving this send call to Context but I opted to limit the changes in the current PR.

StandardFilters:

There is a bug fix and breaking change due to the removal of respond_to?(:to_liquid). The previous code would have left a possibly unexpected object exposed to the template which could have lead to unforeseen privilege given to the template. Procs were notably not being converted.

Behavior of unhandled Procs

Cases when Proc are not handled sometimes lead to exceptions, but other times they could lead to exposure of internal code structure (eg.: rendered as strings in the template such as"#<Proc:0xXXXXXX /Users/tjoyal/projects/liquid/test/integration/standard_filter_test.rb:485>").

North star

The question of supporting Proc behavior is to be asked.

I think most of that “lazy evaluation” can be achieved with an implementation over Liquid::Drop wrapping the underlying implementation which needs to be delayed.

I’ll need to do some git archeology to learn more as to when it was introduced and if it still have merit to be maintained going forward.

tjoyal · 2022-03-07T17:55:18Z

History.md

@@ -4,6 +4,7 @@

 ### Fixes
 * StandardFilter: Fix missing @context on iterations (#1525) [Thierry Joyal]
+* Consolidate contextualization of inputs via the context (#XXX) [Thierry Joyal]


Due to the potential "breaking change" in StandardFilters and VariableLookup where Proc are now properly handled, we might to look at this to be similar to #1527 flagged as deprecation for 6.0.

I do not think anyone expected to have Proc to be exposed directly to templates.

For standard filters specifically, that respond_to?(:to_liquid) might have left non-liquid aware object in by potential design error. I could see use cases leveraging this unexpectedly. They have an easy path forward to migrate, but a deprecation warning might be of interest here.

"contextualization" is a new concept, which makes this change log entry very unclear about what is being fixed. That seems like just a symptom of the fact that this PR combines multiple changes, a new feature (added method), behaviour fixes and some refactoring (which doesn't need to be in the changelog).

tjoyal · 2022-03-07T17:58:02Z

lib/liquid/context.rb

@@ -207,9 +202,9 @@ def lookup_and_evaluate(obj, key, raise_on_not_found: true)
      value = obj[key]

      if value.is_a?(Proc) && obj.respond_to?(:[]=)
-        obj[key] = value.arity == 0 ? value.call : value.call(self)
+        obj[key] = contextualize(value)


I am not changing the memoization in this PR as it let to too large of a change with even more unrelated code alterations.

I feel we should look at:

Exploring memoization everywhere (eg.: for code path like StandardFilters to benefit from it)

Not only memoize if value.is_a?(Proc). There are some test failures appearing when trying to remove the condition, can be iterated further in subsequent work if required.

tjoyal · 2022-03-07T17:58:37Z

lib/liquid/context.rb

@@ -207,9 +202,9 @@ def lookup_and_evaluate(obj, key, raise_on_not_found: true)
      value = obj[key]

      if value.is_a?(Proc) && obj.respond_to?(:[]=)
-        obj[key] = value.arity == 0 ? value.call : value.call(self)
+        obj[key] = contextualize(value)


This would have been clearer, but achieve the same behaviour with more code. No strong opinion on my end.

Suggested change

obj[key] = contextualize(value)

value = contextualize(value)

obj[key] = value

value

tjoyal · 2022-03-07T18:25:37Z

test/integration/standard_filter_test.rb

+    drop  = TestDrop.new(value: "123")
+    p     = proc { drop }
+    templ = '{{ procs | uniq }}'
+    assert_template_result("TestDrop(value:123)", templ, "procs" => [p])


These tests are to cover the new behavior for Proc over filters where historically they would have raised on missing method to_liquid

tjoyal · 2022-03-07T18:26:35Z

test/integration/tags/standard_tag_test.rb

+    assert_template_result("test", "{{ array.first }}", assigns)
+
+    assigns = { "hash" => { a: proc { "test" } } }
+    assert_template_result("test", "{{ hash.first.last }}", assigns)


These tests are to cover the new behavior for Proc where historically they would have returned the Proc as string (eg.: "#<Proc:0xXXXXXX /Users/tjoyal/projects/liquid/test/integration/standard_filter_test.rb:485>")

tjoyal · 2022-03-07T18:27:13Z

test/integration/tags/standard_tag_test.rb

-    assigns = { "hash" => { a: 1, b: 2, c: 3, d: 4 } }
-    assert_template_result('hash has 4 elements', "hash has {{ hash.size }} elements", assigns)
+  def test_command_methods_with_proc
+    skip("Liquid-C does not properly resolve Procs in with command methods") if ENV['LIQUID_C'] == '1'


Liquid-C does not pass this test. Skipping for now as I think we need to iterate further on Proc handling without blocking iterative work.

tjoyal · 2022-03-07T18:34:00Z

lib/liquid/standardfilters.rb

@@ -582,8 +582,7 @@ def empty?

      def each
        @input.each do |e|
-          e = e.respond_to?(:to_liquid) ? e.to_liquid : e
-          e.context = @context if e.respond_to?(:context=)
+          e = @context.contextualize(e)


As noted in the description, this change no longer exclude e.respond_to?(:to_liquid)

I found it hard to separate isolated deliverables as there is a lot of inter-dependency at play.

Seems like making the to_liquid call unconditional would be pretty easy to split into its own PR or at least its own commit.

Combining refactoring and behaviour changes in the same PR can end up making the behaviour changes quite subtle. Instead, one PR could be based on another. E.g. the behaviour change PR could be done first, then the contextualize change can refactor it. Alternatively, the refactor PR could just replace the code that doesn't result in a behaviour change, then the behaviour change PR can use a new help method from the refactor PR.

I can have a closer look. Making it unconditional was making a few tests of StandardFilters (specifically around map) no longer pass since there is Proc handling in said method and Proc does not respond to_liquid.

If we wanted to make the call unconditional in isolation, I will have to either:

Move support for Proc here (similar to the current PR, extending support for all use cases)

Drop support for Proc in StandardFilters#map (potentially what we will want to do down the road?)

I think going with 1 is a good tradeoff for now?

Let's assume that a Proc type is the only non-liquid value that needs to be handled here. In that case, we can use a if e.is_a?(Proc) check instead of e.respond_to?(:to_liquid). We can then preserve backwards compatibility with a deprecation warning for a Proc. Doing that here would also give a desirable deprecation warning for the e = e.call if e.is_a?(Proc) line in StandardFilters#map.

This deprecation should be communicated as deprecating support for having a Proc item in an enumerable.

On the other hand, I think we should continue supporting a Proc value in liquid Hash objects and should make sure to memoize the result of evaluating the Proc. This means that the property lookup in StandardFilters#map should memoize the result of calling any Proc value.

There are other filters that support looking properties, which should be able to just use context.lookup_and_evaluate(item, property, raise_on_not_found: false). Basically, that should be used as the helper method to consolidate Proc Hash lookup support.

That helper method could be used in the map filter implementation as well, although we still may want to keep the r.is_a?(Proc) ? r.call : r line to unconditionally evaluate the Proc even when memoization isn't supported, for backwards compatibility.

dylanahsmith · 2022-03-07T20:59:00Z

lib/liquid/context.rb

+      if object.is_a?(Proc)
+        object = object.arity == 0 ? object.call : object.call(self)
+      end


I don't think we should add support for Procs in more places. It's purpose is to make the evaluation of something relatively expensive lazy so that it isn't done when it isn't needed. Therefore, doing it in places where it isn't memoized introduces a performance trap. We should be deprecating any place where we are already doing that, instead of centralizing it so it is easy to do elsewhere.

lookup_and_evaluate is a better place to handle this concern for now. That is a safe API, because it ensures the Proc result is memoized. Although, it could also be improved by giving a deprecation warning if value.is_a?(Proc) is true but not obj.respond_to?(:[]=).

Longer term, it would be better to reduce support for Proc to just find_variable.

dylanahsmith · 2022-03-07T21:36:13Z

lib/liquid/standardfilters.rb

@@ -582,8 +582,7 @@ def empty?

      def each
        @input.each do |e|
-          e = e.respond_to?(:to_liquid) ? e.to_liquid : e
-          e.context = @context if e.respond_to?(:context=)
+          e = @context.contextualize(e)


I found it hard to separate isolated deliverables as there is a lot of inter-dependency at play.

Seems like making the to_liquid call unconditional would be pretty easy to split into its own PR or at least its own commit.

Combining refactoring and behaviour changes in the same PR can end up making the behaviour changes quite subtle. Instead, one PR could be based on another. E.g. the behaviour change PR could be done first, then the contextualize change can refactor it. Alternatively, the refactor PR could just replace the code that doesn't result in a behaviour change, then the behaviour change PR can use a new help method from the refactor PR.

dylanahsmith · 2022-03-07T21:52:59Z

lib/liquid/context.rb

+    # Convert input objects into liquid aware representations
+    # Procs will be resolved
+    # Assigns the context (self) through context=
+    def contextualize(object)


This method name makes it seem like its primary purpose is to add a liquid context to an object. I had proposed to_liquid as the method name in #1205 (without the Proc conversion) because I want to get to the point where liquid conversion and assigning the context are done together. That way context.to_liquid(object) can be a thin wrapper around object.to_liquid(context) after a deprecation cycle to combine these.

I get from the name that is sets context, but it's not clear that it converts it to a "liquid aware representations". Maybe that means the method is doing too much?

Could we instead move the "contextualization" to Drop#invoke_drop: add a context arg to invoke_drop, and wrap procs in drops? Or is there are use case outside of drops & procs for this?

dylanahsmith · 2022-03-07T22:04:04Z

History.md

@@ -4,6 +4,7 @@

 ### Fixes
 * StandardFilter: Fix missing @context on iterations (#1525) [Thierry Joyal]
+* Consolidate contextualization of inputs via the context (#XXX) [Thierry Joyal]


"contextualization" is a new concept, which makes this change log entry very unclear about what is being fixed. That seems like just a symptom of the fact that this PR combines multiple changes, a new feature (added method), behaviour fixes and some refactoring (which doesn't need to be in the changelog).

dylanahsmith · 2022-03-07T22:11:52Z

Behavior of unhandled Procs

Cases when Proc are not handled sometimes lead to exceptions, but other times they could lead to exposure of internal code structure

Where a Proc isn't supported, I would expect to_liquid to at least be called on the Proc, which should lead to an exception. Otherwise, this missing to_liquid call is the bug in the liquid feature, not the missing Proc outside of lookup_and_evaluate.

macournoyer

Is context= only useful for drops?

Ideally I think it would be better to allow passing the context to drops in Drop#invoke_drop(method_or_key, context), and get rid of context=.

I assume we're only keeping context= around because too many filters & drops (outside of Liquid) depend on it?

macournoyer · 2022-03-08T15:05:45Z

lib/liquid/context.rb

+    # Convert input objects into liquid aware representations
+    # Procs will be resolved
+    # Assigns the context (self) through context=
+    def contextualize(object)


I get from the name that is sets context, but it's not clear that it converts it to a "liquid aware representations". Maybe that means the method is doing too much?

Could we instead move the "contextualization" to Drop#invoke_drop: add a context arg to invoke_drop, and wrap procs in drops? Or is there are use case outside of drops & procs for this?

dylanahsmith · 2022-03-08T15:24:48Z

test/integration/standard_filter_test.rb

@@ -888,7 +920,7 @@ def test_all_filters_never_raise_non_liquid_exception
      { foo: "bar" },
      [{ "foo" => "bar" }, { "foo" => 123 }, { "foo" => nil }, { "foo" => true }, { "foo" => ["foo", "bar"] }],
      { 1 => "bar" },
-      ["foo", 123, nil, true, false, Drop, ["foo"], { foo: "bar" }],
+      ["foo", 123, nil, true, false, ["foo"], { foo: "bar" }],


Perhaps the intention was to test against a drop instance here, which should be supported

Suggested change

["foo", 123, nil, true, false, ["foo"], { foo: "bar" }],

["foo", 123, nil, true, false, Drop.new, ["foo"], { foo: "bar" }],

dylanahsmith · 2022-03-08T16:04:04Z

Ideally I think it would be better to allow passing the context to drops in Drop#invoke_drop(method_or_key, context), and get rid of context=.

Would that assign the context every time the drop is invoked? I suppose that would avoid having to incur this overhead for other types, although having that overhead on property access on drops still seems undesirable.

Also, we can't just get rid of context= without a deprecation cycle, since that could break liquid extensions that use it. It would also mean that the context would be set more lazily, which makes it less obvious to me whether that could result in other breaking changes.

Instead, I want to get to the point where to_liquid takes a context, so that not only would context setting overhead be isolated to drops, but it would also be isolated to initialization of the drops and avoid having drop instances that are in an invalid state (i.e. more obviously correct code).

dylanahsmith · 2022-03-08T16:10:53Z

Is context= only useful for drops?

Liquid seems to use duck typing for interactions with Liquid::Drop, rather than checks like .is_a?(Liquid::Drop). I'm not sure if this flexibility is actually used to have something like a Liquid::Drop that doesn't actually inherit from that class. However, I would expect that typically it is only a Liquid::Drop derived class that would use context=.

macournoyer · 2022-03-08T21:09:13Z

Instead, I want to get to the point where to_liquid takes a context, so that not only would context setting overhead be isolated to drops, but it would also be isolated to initialization of the drops and avoid having drop instances that are in an invalid state (i.e. more obviously correct code).

Ah that'd really good! I totally agree this is much better: to_liquid(context).

Consolidate contextualization of inputs via the context

1bf77d7

tjoyal commented Mar 7, 2022

View reviewed changes

tjoyal marked this pull request as ready for review March 7, 2022 18:34

tjoyal requested review from dylanahsmith and macournoyer and removed request for dylanahsmith March 7, 2022 18:34

dylanahsmith suggested changes Mar 7, 2022

View reviewed changes

macournoyer reviewed Mar 8, 2022

View reviewed changes

dylanahsmith reviewed Mar 8, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Consolidate contextualization of inputs via the context #1537

Consolidate contextualization of inputs via the context #1537

tjoyal commented Mar 7, 2022 •

edited

tjoyal Mar 7, 2022

dylanahsmith Mar 7, 2022

tjoyal Mar 7, 2022

tjoyal Mar 7, 2022

tjoyal Mar 7, 2022

tjoyal Mar 7, 2022

tjoyal Mar 7, 2022

tjoyal Mar 7, 2022

dylanahsmith Mar 7, 2022

tjoyal Mar 7, 2022

dylanahsmith Mar 8, 2022

dylanahsmith Mar 8, 2022

dylanahsmith Mar 7, 2022

dylanahsmith Mar 7, 2022

dylanahsmith Mar 7, 2022

macournoyer Mar 8, 2022

dylanahsmith Mar 7, 2022

dylanahsmith commented Mar 7, 2022

Behavior of unhandled Procs

macournoyer left a comment

macournoyer Mar 8, 2022

dylanahsmith Mar 8, 2022

dylanahsmith commented Mar 8, 2022

dylanahsmith commented Mar 8, 2022

macournoyer commented Mar 8, 2022

	["foo", 123, nil, true, false, ["foo"], { foo: "bar" }],
	["foo", 123, nil, true, false, Drop.new, ["foo"], { foo: "bar" }],

Consolidate contextualization of inputs via the context #1537

Are you sure you want to change the base?

Consolidate contextualization of inputs via the context #1537

Conversation

tjoyal commented Mar 7, 2022 • edited

Changes

Behavior of unhandled Procs

North star

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dylanahsmith commented Mar 7, 2022

Behavior of unhandled Procs

macournoyer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dylanahsmith commented Mar 8, 2022

dylanahsmith commented Mar 8, 2022

macournoyer commented Mar 8, 2022

tjoyal commented Mar 7, 2022 •

edited