Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix sorting of DeferredLazyReference values that are prefixed before deferred block of code (such as an if tag block) #1193

Merged
merged 2 commits into from
Aug 23, 2024

Conversation

jasmith-hs
Copy link
Contributor

@jasmith-hs jasmith-hs commented Aug 23, 2024

Found that there's a bug with how #894 is supposed to work now.

We are checking for instanceof LazyReference, when we should be checking for instanceof DeferredLazyReference here, as that is how the values are always stored on the context.

There wasn't a corresponding test for this behaviour, as the logic in

public static Map<String, String> handleDeferredTokenAndReconstructReferences(
handles a most of the cases for reconstructing references in the proper order.

But that is for when the deferral happens on the same scope level. Since we need to make sure we reconstruct the value on the correct scope. When it's on a higher-level scope, we wait before reconstructing, and that reconstruction happens at the EagerExecutionResult#getPrefixToPreserveState() in the if tag when determining the PrefixToPreserveState for itself and its children. We need to make sure the {% set %} tags that prefix the {% if %} tag are ordered properly, and that is done here by having the DeferredLazyReference values come after any direct values (additionally sorting if there's recursion among the lazy references too)

To elaborate on why we need to wait until we're on the right scope level, we wouldn't want to have a reconstructed result like:

{% if deferred %}
{% set foo = ['a', 1] %}{% set bar = foo %}
{% do bar.append(2) %}
{% endif %}
{% do bar.append(3) %}
{{ foo ~ 'and' ~ bar }}

Because then foo and bar wouldn't exist if the if block doesn't get entered, which is wrong. That whole logic isn't changed in this PR, this PR just makes it so that the prefix before the {% if deferred %} is {% set foo = ['a', 1] %}{% set bar = foo %} instead of {% set bar = foo %}{% set foo = ['a', 1] %}

DeferredLazyReference and not LazyReference.
This was the intended functionality to make sure that the order of
reconstructed values was such that {% set foo = [] %}{% set bar = foo
%}, rather than {% set bar = foo %}{% set foo = [] %} would happen
@jasmith-hs jasmith-hs merged commit 444a9f4 into master Aug 23, 2024
5 checks passed
@jasmith-hs jasmith-hs deleted the deferred-lazy-ref-fix branch August 23, 2024 21:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant