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.

Sign up for GitHub

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
12f3d60
Make null values loop-able.
hs-lsong Dec 22, 2023
f5c3682
Explicit test and result.
hs-lsong Dec 27, 2023
ccb1721
Make this a legacy override instead of feature.
hs-lsong Jan 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions src/main/java/com/hubspot/jinjava/LegacyOverrides.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ public class LegacyOverrides {
.withParseWhitespaceControlStrictly(true)
.withAllowAdjacentTextNodes(true)
.withUseTrimmingForNotesAndExpressions(true)
.withKeepNullableLoopValues(true)
.build();
private final boolean evaluateMapKeys;
private final boolean iterateOverMapKeys;
Expand All @@ -27,6 +28,7 @@ public class LegacyOverrides {
private final boolean parseWhitespaceControlStrictly;
private final boolean allowAdjacentTextNodes;
private final boolean useTrimmingForNotesAndExpressions;
private final boolean keepNullableLoopValues;

private LegacyOverrides(Builder builder) {
evaluateMapKeys = builder.evaluateMapKeys;
Expand All @@ -38,6 +40,7 @@ private LegacyOverrides(Builder builder) {
parseWhitespaceControlStrictly = builder.parseWhitespaceControlStrictly;
allowAdjacentTextNodes = builder.allowAdjacentTextNodes;
useTrimmingForNotesAndExpressions = builder.useTrimmingForNotesAndExpressions;
keepNullableLoopValues = builder.keepNullableLoopValues;
}

public static Builder newBuilder() {
Expand Down Expand Up @@ -80,6 +83,10 @@ public boolean isUseTrimmingForNotesAndExpressions() {
return useTrimmingForNotesAndExpressions;
}

public boolean isKeepNullableLoopValues() {
return keepNullableLoopValues;
}

public static class Builder {
private boolean evaluateMapKeys = false;
private boolean iterateOverMapKeys = false;
Expand All @@ -90,6 +97,7 @@ public static class Builder {
private boolean parseWhitespaceControlStrictly = false;
private boolean allowAdjacentTextNodes = false;
private boolean useTrimmingForNotesAndExpressions = false;
private boolean keepNullableLoopValues = false;

private Builder() {}

Expand Down Expand Up @@ -168,5 +176,10 @@ public Builder withUseTrimmingForNotesAndExpressions(
this.useTrimmingForNotesAndExpressions = useTrimmingForNotesAndExpressions;
return this;
}

public Builder withKeepNullableLoopValues(boolean keepNullableLoopValues) {
this.keepNullableLoopValues = keepNullableLoopValues;
return this;
}
}
}
21 changes: 21 additions & 0 deletions src/main/java/com/hubspot/jinjava/interpret/NullValue.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
package com.hubspot.jinjava.interpret;

/**
* Marker object of a `null` value. A null value in the map is usually considered
* the key does not exist. For example map = {"a": null}, if map.get("a") == null,
* 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


private NullValue() {}

public static NullValue instance() {
return INSTANCE;
}

@Override
public String toString() {
return "null";
}
}
11 changes: 10 additions & 1 deletion src/main/java/com/hubspot/jinjava/lib/tag/ForTag.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
import com.hubspot.jinjava.interpret.InterpretException;
import com.hubspot.jinjava.interpret.JinjavaInterpreter;
import com.hubspot.jinjava.interpret.JinjavaInterpreter.InterpreterScopeClosable;
import com.hubspot.jinjava.interpret.NullValue;
import com.hubspot.jinjava.interpret.OutputTooBigException;
import com.hubspot.jinjava.interpret.TemplateError;
import com.hubspot.jinjava.interpret.TemplateSyntaxException;
Expand Down Expand Up @@ -200,7 +201,15 @@ public String renderForCollection(

// set item variables
if (loopVars.size() == 1) {
interpreter.getContext().put(loopVars.get(0), val);
if (
val == null &&
interpreter.getContext().get(loopVars.get(0)) != null &&
interpreter.getConfig().getLegacyOverrides().isKeepNullableLoopValues()
) {
interpreter.getContext().put(loopVars.get(0), NullValue.INSTANCE);
} else {
interpreter.getContext().put(loopVars.get(0), val);
}
} else {
for (int loopVarIndex = 0; loopVarIndex < loopVars.size(); loopVarIndex++) {
String loopVar = loopVars.get(loopVarIndex);
Expand Down
6 changes: 5 additions & 1 deletion src/test/java/com/hubspot/jinjava/BaseJinjavaTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,11 @@ public void baseSetup() {
JinjavaConfig
.newBuilder()
.withLegacyOverrides(
LegacyOverrides.newBuilder().withUsePyishObjectMapper(true).build()
LegacyOverrides
.newBuilder()
.withUsePyishObjectMapper(true)
.withKeepNullableLoopValues(true)
.build()
)
.build()
);
Expand Down
13 changes: 13 additions & 0 deletions src/test/java/com/hubspot/jinjava/lib/tag/ForTagTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -377,6 +377,19 @@ public void itAllowsCheckingOfWithinForLoop() throws NoSuchMethodException {
assertThat(rendered.getOutput()).isEqualTo("false true true false");
}

@Test
public void forLoopWithNullValues() {
context.put("number", -1);
context.put("the_list", Lists.newArrayList(1L, 2L, null, null, null));
String template = "{% for number in the_list %} {{ number }} {% endfor %}";
TagNode tagNode = (TagNode) new TreeParser(interpreter, template)
.buildTree()
.getChildren()
.getFirst();
String result = tag.interpret(tagNode, interpreter);
assertThat(result).isEqualTo(" 1 2 null null null ");
}

public static boolean inForLoop() {
JinjavaInterpreter interpreter = JinjavaInterpreter.getCurrent();
return interpreter.getContext().isInForLoop();
Expand Down
6 changes: 5 additions & 1 deletion src/test/java/com/hubspot/jinjava/lib/tag/eager/EagerForTagTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,11 @@ public void eagerSetup() {
.withMaxOutputSize(MAX_OUTPUT_SIZE)
.withExecutionMode(EagerExecutionMode.instance())
.withLegacyOverrides(
LegacyOverrides.newBuilder().withUsePyishObjectMapper(true).build()
LegacyOverrides
.newBuilder()
.withUsePyishObjectMapper(true)
.withKeepNullableLoopValues(true)
.build()
)
.build()
);
Expand Down