Deprecate environment fallback for Mustache variable resolution

[dsyer]

MustacheEnvironmentCollector has some strange logic in it (which I probably wrote). It looks for the "native" collector via super.createFetcher() but then uses it exclusively if it is not null. But a not-null fetcher is not the same as a fetcher of not-null values. What it should do is inject the fetcher into the PropertyVariableFetcher and try to use it before falling back to the Environment.

There's a complication though, to do with the fact that JMustache natively resolves variables with period separators as bean.property (i.e. "bean" is an object). We wouldn't want to break that, but we don't want to start returning random stuff from the Environment that happens to match bean.property. It might turn out that this concern is already handled by JMustache (it depends on the order it presents the variable names to the fetcher - does it try bean before bean.property or the other way round?). Need a test.

[philwebb]

Closing in favor of PR #21060

[wilkinsona]

Unfortunately, it's flawed to assume that a null response from a fetcher indicates that it could not fetch a value. The model could have an accessor that explicitly returns null. No fallback attempt should be made at this point and the compiler's nullValue, if configured, should be used to determine the result. This faulty assumption resulted in a regression. I'm going to revert #21060 and re-open this issue so that we can revisit the fix.

[wilkinsona]

The more I think about this, the less I am sure we should change the behaviour here. If the context contains a null value for a given name, I'm not sure that it should fall back to the environment at all. Falling back prevents the context from overriding a value in the environment with null and from making use of the compiler's nullValue support.

[dsyer]

Can we have a debate about this please? The (reasonable) expectation from users is that they can put placeholders in templates for values in the Environment. If null is not the right signal that this is what is intended, then maybe something else is needed? Like an explicit env value in the model or something.

[wilkinsona]

The (reasonable) expectation from users is that they can put placeholders in templates for values in the Environment

I agree that's a reasonable expectation, and it's one that, AIUI, is already supported. As long as the model doesn't contain a value (including null) for a placeholder, the environment will be queried. If the user's put a null value in the model, that needs to be honoured so that the model can override the environment and also so that Mustache's nullValue support can be used. Given that the user controls the model, if they don't want the value that it contains to be used, can they not omit the value from the model?

[wilkinsona]

Looking again, the current tests are wrong and falling back to the environment doesn't always work, irrespective of whether a name is mapped to null in the model. The tests currently pass new Object() as the model. If the model is replaced with Collections.emptyMap() they fail.

[wilkinsona]

Some of the tests in the reverted PR seem to me to be wrong as well. They assert that the model takes priority for "normal" keys but that the environment takes priority for compound keys. For example, if a is available from the model and the environment the model will win but if b.c is available from the model and the environment, the environment will win. The fallback behaviour needs to be consistent irrespective of the nature of the key. I think the model should win in both cases.

[wilkinsona]

I've reworked the tests to illustrate some of the current inconsistencies with how object- and map-based models are handled. There's one test that fails with both models and 7 that fail with only one type of model.

Mustache's handling of compound keys complicates things. When it's in standards mode, resolving nested.name will just look for nested.name in the model. If it's not found we can fall back to the environment and look for the nested.name property. When it's not in standards mode (the default), resolving nested.name will first check for nested.name. If nothing's found, it'll then drill down. It does this by check for nested and, if it's found, checking for name on the result of the check for nested. If it's not found after drilling own, we can fall back to the environment and look for the nested.name property.

The problem with the above is that the collector needs to know if a compound key will result in a single call (standards mode) or multiple calls (the default), falling back to the environment when the first and only call finds nothing or once the key contains no . separators respectively. There doesn't appear to be any link between a collector and its compiler so it doesn't appear to be possible to make the required distinction.

[wilkinsona]

We might be able to have the collector make an assumption about the mode that Mustache is using. When we auto-configure Mustache the compiler doesn't have standards mode enabled so the collector that we auto-configure could know this. If the user replaces the compiler with one with standards mode enabled, they could configure a collector with the same mode.

[eis]

Looking again, the current tests are wrong and falling back to the environment doesn't always work, irrespective of whether a name is mapped to null in the model. The tests currently pass new Object() as the model. If the model is replaced with Collections.emptyMap() they fail.

Does this mean the current state of things is that values from environment are not supported, if any kind of model map exists? Currently in my app I'm not able to pass environment variables to be used by jmustache, and I'm not sure if that's how it is intended, this issue discussed here, or a problem with my configuration.

[wilkinsona]

That sounds like the issue discussed here.

[wilkinsona]

Mustache doesn't behave as I had hoped above. When it's not in standards mode and a.b is being resolved the collector is called three times to create a fetcher with three different names:

1. a.b
2. a
3. b

Once the third call hasn't found anything, we need to fall back to the environment and get the a.b property. I had previously thought that we would be able to associate these three calls with each other so that we know that once the call with b hasn't found anything we can get a.b from the environment. That doesn't appear to be possible as the Collector implementation is effectively stateless.

As far as I can tell, it isn't possible to consistently fall back to the environment when using Mustache in non-standards mode.

[wilkinsona]

I think we're going to need to find another way to skin this cat. The most straightforward would be to recommend that users consider the environment when creating the model that's passed to Mustache if that's the behaviour that a they want. When creating a Map-based model they can put values retrieved from the Environment in the map. When creating a POJO-based model they can implement getters that return values retrieved from the Environment. This approach also has the benefit that it breaks the coupling between the names in the model for the view and the names of properties in the environment.

[wilkinsona]

@dsyer Unless you can think of something that I've missed, we're going to remove the environment fallback in 2.5 as it doesn't seem possible to make it work in all scenarios.

[dsyer]

We'd have to duplicate some of the logic in the Mustache Template in order to eagerly evaluate nested paths. Fortunately that logic isn't particularly complicated. This works for the tests I found in your branch:

public class MustacheEnvironmentCollector extends DefaultCollector implements EnvironmentAware {

	private ConfigurableEnvironment environment;

	private final VariableFetcher propertyFetcher = new PropertyVariableFetcher();

	@Override
	public void setEnvironment(Environment environment) {
		this.environment = (ConfigurableEnvironment) environment;
	}

	@Override
	public VariableFetcher createFetcher(Object ctx, String name) {
		VariableFetcher fetcher = super.createFetcher(ctx, name);
		Object value = null;
		if (fetcher != null) {
			try {
				value = fetcher.get(ctx, name);
			}
			catch (Exception e) {
			}
			if (value != null && !Template.NO_FETCHER_FOUND.equals(value)) {
				return fetcher;
			}
		}
		if (this.environment.containsProperty(name)) {
			Object compound = getCompoundValue(ctx, name);
			// There's a TODO in the tests related to this null check
			if (compound == null || Template.NO_FETCHER_FOUND.equals(compound)) {
				return this.propertyFetcher;
			}
		}
		return fetcher;
	}

	private Object getCompoundValue(Object ctx, String name) {
		if (!name.contains(".")) {
			return Template.NO_FETCHER_FOUND;
		}
		String[] comps = name.split("\\.");
		Object value = null;
		int index = 0;
		while (index < comps.length && !Template.NO_FETCHER_FOUND.equals(value)) {
			VariableFetcher fetcher = super.createFetcher(ctx, comps[index]);
			if (fetcher == null) {
				value = Template.NO_FETCHER_FOUND;
				break;
			}
			try {
				value = fetcher.get(ctx, comps[index]);
			}
			catch (Exception e) {
				value = Template.NO_FETCHER_FOUND;
				break;
			}
			ctx = value;
			index++;
		}
		return value;
	}

	private class PropertyVariableFetcher implements VariableFetcher {

		@Override
		public Object get(Object ctx, String name) {
			return MustacheEnvironmentCollector.this.environment.getProperty(name);
		}

	}

}

[wilkinsona]

Thanks, Dave. I didn't give much consideration to any approaches that would require duplicating lots of Mustache's logic. My concern is that our copy of the logic will get out of sync with Mustache's. I think I'd rather drop the feature than introduce that fragility, but let's see what the rest of the team thinks.

[dsyer]

FWIW that code is super stable. And all we really did was borrow it to suit our purpose, so even if it changed in Mustache, it would arguably still suit our purpose, so there would be no reason to change it, unless they changed the way VariableFetcher works which seems unlikely given how stable it is.

[wilkinsona]

We discussed this on Friday and decided that we're going to deprecate support for the environment fallback. Beyond it not working correctly in all scenarios, we think that it should be something that users opt into by populating the model with values read from the environment. This will make Mustache's behaviour consistent with the other template engines that we auto-configure where, for example, FreeMarker's freemarker.core.Environment doesn't fall back to our Environment.

[wilkinsona]

As far as I can see, the current fallback behaviour isn't documented so there's not much to document here, in the reference documentation at least. I think it's still worth mentioning the change in the upgrade section of the release notes though.