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

Fix VariableNotExistException for variable knowingly set to null #295

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Fix VariableNotExistException for variable knowingly set to null #295

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
0d572d1
Differentiate between null and missing values
kohlschuetter Dec 27, 2023
9afde46
Add support for "sane" strict variable checks
kohlschuetter Dec 27, 2023
6f40bda
Also allow {% if obj[missing] %} checks in SANE strictVariablesMode
kohlschuetter Dec 27, 2023
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
15 changes: 10 additions & 5 deletions src/main/java/liqp/TemplateContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,13 @@
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
import java.util.concurrent.atomic.AtomicBoolean;

import liqp.RenderTransformer.ObjectAppender;
import liqp.exceptions.ExceededMaxIterationsException;

public class TemplateContext {
private static final AtomicBoolean FOUND_DUMMY = new AtomicBoolean(false);

public static final String REGISTRY_CYCLE = "cycle";
public static final String REGISTRY_IFCHANGED = "ifchanged";
Expand Down Expand Up @@ -99,20 +101,23 @@ public boolean containsKey(String key) {
}

public Object get(String key) {
return get(key, FOUND_DUMMY);
}

public Object get(String key, AtomicBoolean found) {
// First try to retrieve the key from the local context
Object value = this.variables.get(key);

if (value != null) {
return value;
if (this.variables.containsKey(key)) {
found.set(true);
return this.variables.get(key);
}

if (parent != null) {
// Not available locally, try the parent context
return parent.get(key);
return parent.get(key, found);
}

// Not available
found.set(false);
return null;
}

Expand Down
50 changes: 43 additions & 7 deletions src/main/java/liqp/TemplateParser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,30 @@ public enum ErrorMode {
LAX
}

/**
* Controls the "strict variables" checking strategy.
*
* @see <a href="https://github.com/Shopify/liquid/issues/1034">liquid issue 1034</a>
*/
public enum StrictVariablesMode {
/**
* No strict variable checking.
*/
OFF,

/**
* Strict variables, but allow checking for the existence of directly-nested variables without
* throwing an error (e.g., checking for "object.missing" is acceptable, but
* "object.nested.missing" isn't).
*/
SANE,

/**
* Strict variable checking.
*/
STRICT
}

public final Flavor flavor;
public final boolean stripSpacesAroundTags;
public final boolean stripSingleLine;
Expand All @@ -68,8 +92,13 @@ public enum ErrorMode {

/**
* The same as <code>template.render!({}, strict_variables: true)</code> in ruby
*
* @see #strictVariablesMode
*/
@Deprecated(forRemoval = true)
public final boolean strictVariables;
public final StrictVariablesMode strictVariablesMode;

/**
* This field doesn't have equivalent in ruby.
*/
Expand Down Expand Up @@ -138,7 +167,7 @@ public static class Builder {
private Boolean liquidStyleWhere;


private boolean strictVariables = false;
private StrictVariablesMode strictVariablesMode = StrictVariablesMode.OFF;
private boolean showExceptionsFromInclude;
private EvaluateMode evaluateMode = EvaluateMode.LAZY;
private Locale locale = DEFAULT_LOCALE;
Expand All @@ -165,7 +194,7 @@ public Builder(TemplateParser parser) {
this.insertions = new ArrayList<>(parser.insertions.values());
this.filters = new ArrayList<>(parser.filters.values());

this.strictVariables = parser.strictVariables;
this.strictVariablesMode = parser.strictVariablesMode;
this.evaluateMode = parser.evaluateMode;
this.locale = parser.locale;
this.renderTransformer = parser.renderTransformer;
Expand Down Expand Up @@ -248,9 +277,15 @@ public Builder withLiquidStyleInclude(boolean liquidStyleInclude) {
return this;
}

@SuppressWarnings("hiding")
public Builder withStrictVariables(boolean strictVariables) {
this.strictVariables = strictVariables;
this.strictVariablesMode = strictVariables ? StrictVariablesMode.STRICT
: StrictVariablesMode.OFF;
return this;
}

@SuppressWarnings("hiding")
public Builder withStrictVariables(StrictVariablesMode strictVariablesMode) {
this.strictVariablesMode = Objects.requireNonNull(strictVariablesMode);
return this;
}

Expand Down Expand Up @@ -404,12 +439,12 @@ public TemplateParser build() {
nameResolver = new LocalFSNameResolver(snippetsFolderName);
}

return new TemplateParser(strictVariables, showExceptionsFromInclude, evaluateMode, renderTransformer, locale, defaultTimeZone, environmentMapConfigurator, errorMode, fl, stripSpacesAroundTags, stripSingleLine, mapper,
return new TemplateParser(strictVariablesMode, showExceptionsFromInclude, evaluateMode, renderTransformer, locale, defaultTimeZone, environmentMapConfigurator, errorMode, fl, stripSpacesAroundTags, stripSingleLine, mapper,
allInsertions, finalFilters, evaluateInOutputTag, strictTypedExpressions, liquidStyleInclude, liquidStyleWhere, nameResolver, limitMaxIterations, limitMaxSizeRenderedString, limitMaxRenderTimeMillis, limitMaxTemplateSizeBytes);
}
}

TemplateParser(boolean strictVariables, boolean showExceptionsFromInclude, EvaluateMode evaluateMode,
TemplateParser(StrictVariablesMode strictVariablesMode, boolean showExceptionsFromInclude, EvaluateMode evaluateMode,
RenderTransformer renderTransformer, Locale locale, ZoneId defaultTimeZone,
Consumer<Map<String, Object>> environmentMapConfigurator, ErrorMode errorMode, Flavor flavor, boolean stripSpacesAroundTags, boolean stripSingleLine,
ObjectMapper mapper, Insertions insertions, Filters filters, boolean evaluateInOutputTag,
Expand All @@ -427,7 +462,8 @@ public TemplateParser build() {
this.liquidStyleInclude = liquidStyleInclude;
this.liquidStyleWhere = liquidStyleWhere;

this.strictVariables = strictVariables;
this.strictVariablesMode = strictVariablesMode;
this.strictVariables = strictVariablesMode != StrictVariablesMode.OFF;
this.showExceptionsFromInclude = showExceptionsFromInclude;
this.evaluateMode = evaluateMode;
this.renderTransformer = renderTransformer == null ? RenderTransformerDefaultImpl.INSTANCE
Expand Down
78 changes: 62 additions & 16 deletions src/main/java/liqp/nodes/LookupNode.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import java.util.Iterator;
import java.util.List;
import java.util.Map;
import java.util.concurrent.atomic.AtomicBoolean;

import liqp.TemplateContext;
import liqp.TemplateParser;
Expand All @@ -13,7 +14,6 @@
import liqp.parser.LiquidSupport;

public class LookupNode implements LNode {

private final String id;
private final List<Indexable> indexes;

Expand All @@ -28,7 +28,10 @@ public void add(Indexable indexable) {

@Override
public Object render(TemplateContext context) {
return render(context, false);
}

public Object render(TemplateContext context, boolean okIfMissing) {
Object value = null;

String realId;
Expand All @@ -38,8 +41,11 @@ public Object render(TemplateContext context) {
} else {
realId = id;
}

AtomicBoolean found = new AtomicBoolean(false);
if (context.containsKey(realId)) {
value = context.get(realId);
found.set(true);
}
if (value == null) {
Map<String, Object> environmentMap = context.getEnvironmentMap();
Expand All @@ -48,15 +54,47 @@ public Object render(TemplateContext context) {
}
}

for(Indexable index : indexes) {
value = index.get(value, context);
boolean foundAllButLastOne = okIfMissing;
for (Iterator<Indexable> it = indexes.iterator(); it.hasNext();) {
Indexable index = it.next();
if (it.hasNext()) {
value = index.get(value, context, found);
if (value == null) {
found.set(false);
foundAllButLastOne = false;
break;
}
} else {
// last item
value = index.get(value, context, found);
if (!found.get()) {
foundAllButLastOne = true;
}
}
}

if(value == null && context.getParser().strictVariables) {
RuntimeException e = new VariableNotExistException(getVariableName());
context.addError(e);
if (context.getErrorMode() == TemplateParser.ErrorMode.STRICT) {
throw e;
if (value == null && !found.get()) {
final boolean error;
switch (context.getParser().strictVariablesMode) {
case OFF:
error = false;
break;
case STRICT:
error = true;
break;
case SANE:
error = !foundAllButLastOne;
break;
default:
throw new UnsupportedOperationException();
}

if (error) {
RuntimeException e = new VariableNotExistException(getVariableName());
context.addError(e);
if (context.getErrorMode() == TemplateParser.ErrorMode.STRICT) {
throw e;
}
}
}

Expand All @@ -72,7 +110,7 @@ private String getVariableName() {
}

public interface Indexable {
Object get(Object value, TemplateContext context);
Object get(Object value, TemplateContext context, AtomicBoolean found);
}

public static class Hash implements Indexable {
Expand All @@ -84,8 +122,8 @@ public Hash(String hash) {
}

@Override
public Object get(Object value, TemplateContext context) {

public Object get(Object value, TemplateContext context, AtomicBoolean found) {
found.set(true);
if(value == null) {
return null;
}
Expand Down Expand Up @@ -141,10 +179,14 @@ else if(value.getClass().isArray()) {
} else {
map = (Map<?,?>) value;
}
if (!map.containsKey(hash)) {
found.set(false);
return null;
}
return map.get(hash);
}
else if(value instanceof TemplateContext) {
return ((TemplateContext)value).get(hash);
return ((TemplateContext)value).get(hash, found);
} else {
return null;
}
Expand All @@ -168,13 +210,17 @@ public Index(LNode expression, String text) {
}

@Override
public Object get(Object value, TemplateContext context) {

public Object get(Object value, TemplateContext context, AtomicBoolean found) {
found.set(true);
if(value == null) {
return null;
}

key = expression.render(context);
if (expression instanceof LookupNode) {
key = ((LookupNode)expression).render(context, true);
} else {
key = expression.render(context);
}

if (key instanceof Number) {
int index = ((Number)key).intValue();
Expand Down Expand Up @@ -238,7 +284,7 @@ else if (value instanceof List) {
}

String hash = String.valueOf(key);
return new Hash(hash).get(value, context);
return new Hash(hash).get(value, context, found);
}
}

Expand Down
63 changes: 63 additions & 0 deletions src/test/java/liqp/blocks/IfTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,18 @@

import static org.hamcrest.CoreMatchers.is;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.junit.Assert.assertThrows;

import java.util.Collections;

import org.antlr.v4.runtime.RecognitionException;
import org.junit.Test;

import liqp.Template;
import liqp.TemplateParser;
import liqp.TemplateParser.StrictVariablesMode;
import liqp.exceptions.LiquidException;
import liqp.exceptions.VariableNotExistException;

public class IfTest {

Expand Down Expand Up @@ -410,4 +415,62 @@ public void and_or_evaluation_orderTest() throws RecognitionException {
assertThat(TemplateParser.DEFAULT.parse("{% if true or false and false %}TRUE{% else %}FALSE{% endif %}").render(), is("TRUE"));
assertThat(TemplateParser.DEFAULT.parse("{% if true and false and false or true %}TRUE{% else %}FALSE{% endif %}").render(), is("FALSE"));
}

@Test
public void strictVariablesTest() throws RecognitionException {
assertThat(new TemplateParser.Builder().withStrictVariables(StrictVariablesMode.STRICT).build()
.parse("{% if obj.var %}true{% else %}false{% endif %}").render(Collections.singletonMap(
"obj", Collections.singletonMap("var", "val"))), is("true"));

assertThrows(VariableNotExistException.class, () -> {
new TemplateParser.Builder().withStrictVariables(StrictVariablesMode.STRICT).build().parse(
"{% if obj.missing %}true{% else %}false{% endif %}").render(Collections.singletonMap(
"obj", Collections.singletonMap("var", "val")));
});
assertThrows(VariableNotExistException.class, () -> {
new TemplateParser.Builder().withStrictVariables(StrictVariablesMode.STRICT).build().parse(
"{% if obj.nested.missing %}true{% else %}false{% endif %}").render(Collections.singletonMap(
"obj", Collections.singletonMap("var", "val")));
});
assertThrows(VariableNotExistException.class, () -> {
new TemplateParser.Builder().withStrictVariables(StrictVariablesMode.STRICT).build().parse(
"{% if obj[missing] %}true{% else %}false{% endif %}").render(Collections.singletonMap(
"obj", Collections.singletonMap("var", "val")));
});

assertThat(new TemplateParser.Builder().withStrictVariables(StrictVariablesMode.SANE).build()
.parse("{% if obj.missing %}true{% else %}false{% endif %}").render(Collections.singletonMap(
"obj", Collections.singletonMap("var", "val"))), is("false"));
assertThat(new TemplateParser.Builder().withStrictVariables(StrictVariablesMode.SANE).build()
.parse("{% if obj[missing] %}true{% else %}false{% endif %}").render(Collections.singletonMap(
"obj", Collections.singletonMap("var", "val"))), is("false"));
assertThrows(VariableNotExistException.class, () -> {
new TemplateParser.Builder().withStrictVariables(StrictVariablesMode.SANE).build().parse(
"{% if obj.nested.missing %}true{% else %}false{% endif %}").render(Collections
.singletonMap("obj", Collections.singletonMap("var", "val")));
});
assertThrows(VariableNotExistException.class, () -> {
new TemplateParser.Builder().withStrictVariables(StrictVariablesMode.SANE).build().parse(
"{% if obj[nested].missing %}true{% else %}false{% endif %}").render(Collections
.singletonMap("obj", Collections.singletonMap("var", "val")));
});
assertThrows(VariableNotExistException.class, () -> {
new TemplateParser.Builder().withStrictVariables(StrictVariablesMode.SANE).build().parse(
"{% if obj[nested][missing] %}true{% else %}false{% endif %}").render(Collections
.singletonMap("obj", Collections.singletonMap("var", "val")));
});

assertThat(new TemplateParser.Builder().withStrictVariables(StrictVariablesMode.OFF).build()
.parse("{% if obj.missing %}true{% else %}false{% endif %}").render(Collections.singletonMap(
"obj", Collections.singletonMap("var", "val"))), is("false"));
assertThat(new TemplateParser.Builder().withStrictVariables(StrictVariablesMode.OFF).build()
.parse("{% if obj.nested.missing %}true{% else %}false{% endif %}").render(Collections
.singletonMap("obj", Collections.singletonMap("var", "val"))), is("false"));
assertThat(new TemplateParser.Builder().withStrictVariables(StrictVariablesMode.OFF).build()
.parse("{% if obj[nested].missing %}true{% else %}false{% endif %}").render(Collections
.singletonMap("obj", Collections.singletonMap("var", "val"))), is("false"));
assertThat(new TemplateParser.Builder().withStrictVariables(StrictVariablesMode.OFF).build()
.parse("{% if obj[nested][missing] %}true{% else %}false{% endif %}").render(Collections
.singletonMap("obj", Collections.singletonMap("var", "val"))), is("false"));
}
}
Loading