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

Make null values loop-able. #1140

Merged
merged 3 commits into from
Jan 3, 2024
Merged

Make null values loop-able. #1140

merged 3 commits into from
Jan 3, 2024

Conversation

hs-lsong
Copy link
Collaborator

When the loop collection contains null, the loop var will be searching the outer scope of the value instead of just use the null value. For example,

context.put("number", -1);
context.put("the_list", Lists.newArrayList(1L, 2L, null, null, null));
String template = "{% for number in the_list %} {{ number }} {% endfor %}";

When rendering the above for tag, because the 3rd and later values are null, the loop var is number, and it will find the value of it from outside of the forTag scope. It will result in the following

Expected :" 1  2  null  null  null "
Actual   :" 1  2  -1  -1  -1 "

With this special NullValue instance, the loopVar value will stay inside the loop.

Copy link
Contributor

@boulter boulter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps this should be put in a feature because I could see someone relying on this "default" functionality, intentionally or not.

Copy link
Contributor

@jasmith-hs jasmith-hs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We use https://github.com/HubSpot/jinjava/blob/0c47c7f14e6d02b378dc1c625319e6ed9adb090f/src/main/java/com/hubspot/jinjava/LegacyOverrides.java to override the legacy Jinjava behaviour so this could be a good place for this to reside
withNullableLoopValues()

* we treat it as the there is not key "a" in the map.
*/
public final class NullValue {
public static final NullValue INSTANCE = new NullValue();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just having this have the String value of null wouldn't be sufficient.
Likely you'd want to unwrap the NullValue -> null inside of com.hubspot.jinjava.util.ScopeMap#get(Object)

Creating some construct such as:

record ScopeMap.ScopeValue(Object value) {}

And always unwrapping those values when accessing them inside of a ScopeMap seems like it would do the trick

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you elaborate why we need to unwrap the NullValue -> null? I don't think we need anywhere to use the null value.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

null != "null"
Here's a quick example:

{% set foo = [0, 1, None, 2] %}
{% for i in foo %}
{% if i %}
 i is {{ i }}
{% endif %}
{% endfor %}

The expected jinja output is:

 i is 1
 i is 2

If treating null as "null", the output would be:

i is 1
i is null
i is 2

@hs-lsong hs-lsong merged commit f623369 into master Jan 3, 2024
4 checks passed
@hs-lsong hs-lsong deleted the forloop-with-null branch January 3, 2024 14:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants