Null handling when iterating String arrays/lists

[softwaremaverick]

Given this example:

import java.io.StringReader;
import java.io.StringWriter;

import org.testng.annotations.Test;

import com.github.mustachejava.DefaultMustacheFactory;
import com.github.mustachejava.Mustache;
import com.github.mustachejava.MustacheFactory;


public class SimpleMustacheTest
{
private static class MyObject
{
	public String[] getNames()
	{
		return new String[] { "Fred", null };
	}
}


@Test
public void simpleTest()
{
	final String template = "{{#names}}{{.}}\n\n{{/names}}";

	final MustacheFactory mustacheFactory = new DefaultMustacheFactory();
	final Mustache mustache = mustacheFactory.compile(new StringReader(template), "test");

	final StringWriter writer = new StringWriter();
	mustache.execute(writer, new MyObject());

	System.out.println(writer.toString());
}
}

Why do I get this?

Fred
SimpleMustacheTest$MyObject@4891a775

Effectively it looks like it's saying that if a value cannot be extracted for an element then I'll take the parent scope. If I'm doing a {{.}} then that parent scope seeking shouldn't happen should it? It's not like anyone would be saying that if I don't have a value then get my parent instead would they?

If this is a design decision then does that mean we have to wrap all objects in true/false checks?

Currently the scenario in the real code I'm forcing the data to always return empty string if the value is a null to overcome this issue.

[spullara]

It is an odd case that you have nulls in your lists, this hasn't come up before, but I agree that {{.}} probably shouldn't go up the stack to the next object. This wasn't an explicit design decision.

[softwaremaverick]

I agree it is indeed an odd case but one that we are in. The amount of code required to ensure that all possible paths are null protected outweighs the amount of code for it to be protected against in mustache I'm sure. Assuming that someone else knows exactly where in the codebase to go to before I even start looking, could this please be fixed in the 0.8.x version as well as 0.9.x? We're not quite ready for java 8 just yet. It's a really useful library though by the way and fits well with what we needed it for.

…

On 24 January 2018 21:33:12 GMT, Sam Pullara ***@***.***> wrote: It is an odd case that you have nulls in your lists, this hasn't come up before, but I agree that {{.}} probably shouldn't go up the stack to the next object. This wasn't an explicit design decision. -- You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub: #202 (comment)

-- Sent from my Android device with K-9 Mail. Please excuse my brevity.

[softwaremaverick]

Another thought. If the scenario wasn't a list of strings but a list of objects and the template wanted to output a value field, even if that was null should it go to the patent context even then? I haven't tested that scenario it was just a thought. I'm just wondering whether the fix actually is to always stop with null values assuming that the null actually came from somewhere as opposed to being null because a field named value didn't exist in the child object.

…

On 24 January 2018 21:33:12 GMT, Sam Pullara ***@***.***> wrote: It is an odd case that you have nulls in your lists, this hasn't come up before, but I agree that {{.}} probably shouldn't go up the stack to the next object. This wasn't an explicit design decision. -- You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub: #202 (comment)

-- Sent from my Android device with K-9 Mail. Please excuse my brevity.

[spullara]

This issue may be more fundamental than I thought. Basically as it is creating the list of scopes it ignores scopes that are null. The . references the latest scope and since the value was null the iterator itself is the latest scope in this case. I'm investigating how much that assumption is strung through the code.

[spullara]

People are definitely using null as an indicator to look to parent scopes since that is the normal behavior. I might be able to special case for just '.' though because it is already a very special case.

[spullara]

I added a test that shows you how to construct a MustacheFactory that has the behavior that you need. This kind of change is one of the reasons that almost everything is overridable and I don't need to change it for everyone or add a configuration parameter for users to get whatever custom behavior they need. I don't think that you will run into any other problems using it like this since all of my tests pass locally when I tried to make the global change, just worried there may be some edge case out there that would break.

c762c26

Can you try this in your program and see if it gives you what you need?

[softwaremaverick]

Thanks for that @spullara. However, the code you provided above I suspect is for 0.9.x as I got an Override warning as the method wasn't being overridden.

Modifying my 0.8.12 (as that is what we currently use), I made the following using an empty string instead of a null.

final MustacheFactory mustacheFactory = new DefaultMustacheFactory()
{
	@Override
	public MustacheVisitor createMustacheVisitor()
	{
		return new DefaultMustacheVisitor(this)
		{
			@Override
			public void iterable(final TemplateContext templateContext, final String variable,
					final Mustache mustache)
			{
				this.list.add(new IterableCode(templateContext, this.df, mustache, variable)
				{
					@Override
					protected Object[] addScope(final Object[] scopes, final Object scope)
					{
						return super.addScope(scopes, scope == null ? StringUtils.EMPTY : scope);
					}
				});
			}
		};
}
};

I'm currently running a full build of tests to see if anything comes out of it, but the few tests I ran worked fine with it. I don't see any potential issues arising from using an empty string either.

I've tested this with both the {{.}} and specifying {{value}} to test whether reflection trying to find the value from a String would return a value (after modifying the string to return the word Test instead of empty string of course), but it didn't which was expected and so an empty string in that test was returned.

What do you think?

[spullara]

LGTM. Please close the issue when you are convinced it is working for you.

[softwaremaverick]

Thanks I will do. I would still urge you to put in a permanent fix for this on both the 0.8 and 0.9 branches as we've been using mustache for years but only just realised this issue. The consequences could have been potential security leak of information. Thanks again. Andrew

…

On 25 January 2018 17:44:32 GMT, Sam Pullara ***@***.***> wrote: LGTM. Please close the issue when you are convinced it is working for you. -- You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub: #202 (comment)

-- Sent from my Android device with K-9 Mail. Please excuse my brevity.

[spullara]

I need to convince myself that it doesn't break backwards compatibility with some wanted behavior.

[xjdsg]

@spullara I am using mustache 0.9.6 and I found one issue with overriding the addScope as below:
protected boolean addScope(List<Object> scopes, Object scope) { scopes.add(scope); return true; }
The issue happened when iterating List, and the first object of the List is null,
With the test case
{ "content": { "values": [ null, { "name": "Alice" }, { "name": "Bob" } ] } }
And
{{#content}} {{#values}} {{name}} {{/values}} {{/content}}
The result after compiling the mustache is empty string \n \n \n instead of \n Alice\n Bob\n
The root cause is when resolving the null value,
the scopes is [content, values, null]
it will create a wrapper with
LengthGuard(length 3) -> MapGuard (index 1) -> MapGuard(index 0)... finally wrap as MissingWrapper

When iterating the second value Alice, the scopes is
[content, values, name]
it will try to find the right wrapper in prevWrapper, which is Missing Wrapper now, and it will find all the things are matching, and finally get MissingWrapper, and can't find the right value Alice. The problem is it has missed the index 2 in the scopes.
using
protected Object[] addScope(final Object[] scopes, final Object scope) { return super.addScope(scopes, scope == null ? StringUtils.EMPTY : scope); }

would not have this problem, because it has replace the null value with string class. I could use it as a temporary solution.

But could you include the fix or considering this problem into the official build?

There are several solutions I could think of:

1. ReflectionObjectHandler.find() function, line 67, if the scope is null, still create a ClassGuard for it and then continue
2. As prevWrapper is used for improving the performance, I am wondering if it's valuable to put the MissingWrapper in it. Maybe we can move the MissingWrapper out of it.

[spullara]

Thanks for the report. My guess is that the first solution might work. Taking a loot at it.