Support arbitrary getters in templates via reflection

[michaelhixson]

When I first tried this library, I naively expected templates to be able to invoke getter methods on my objects. Like this:

class:

public class Foo {
  String message;
  Foo(this.message);
}

template:

{{#foo}}{{message}}{{/foo}}

rendering code:

var foo = new Foo("hello");
print(template.renderString({"foo": foo}));
// Want this to print "hello", but it throws an error.

mustache4dart and mustache.java both support this. I have a rough working implementation of this feature locally, for this library. It is pretty convenient, but it performs much worse (about 1/3 as fast) than the alternative:

var foo = new Foo("hello");
print(template.renderString({"foo": { "message": foo.message }}));

But I think the ease of use is worth the performance penalty. It's not so clear in this simple example, but I think the "transform everything to Maps" approach would become very ugly the more you have to use it, and the more complicated your objects become. Imagine rendering a list of Foos, and each one has a Bar, and each Bar has a list of Baz, and you want to render properties of all of them in the template. If reflection is supported you get that for free. Without reflection, you have to marshall all that data into Maps yourself.

Would you be interested in including this feature? (If so, I can clean up my code, test it more thoroughly, and share it here.)

[xxgreg]

I agree. I'm interested in adding this code.

However one problem is dart2js reflection support. Ideally there would be two libraries, both within the mustache package. One library would allow non-reflection based library for use in the browser. I've got a few ideas of how to separate this so that the code can be shared for both libraries.

Please share your patch - I'll merge it, and then we can discuss the non-reflection use case.

I'll get your other patch merged soon too.

Cheers ;)

[michaelhixson]

Ok, here is a commit that adds this feature.

https://github.com/michaelhixson/mustache/commit/f22aecb0639296d6dae4ccad98a27cd68fde5176

Please ignore the List/Iterable changes in this commit; I undid my other patch accidentally.

[xxgreg]

Nice work.

I'm worried that this bit could actually lead to wrong results if there is a mix of objects and Maps in the stack. This code will always choose the first map, even if there is an object with that named property that is closer to the top of the stack.

One other way to optimise this would be to cache the names of the properties rather than looking them up each time.

+    // Optimistically assume all names are map keys at first,
+    // since reflection is more expensive than map lookups.
+    // This strategy should avoid penalizing users who do only
+    // use maps and avoid relying on reflection.
+
+    var object = _stack
+        .reversed
+        .firstWhere(
+            (v) => v is Map && v.containsKey(parts[0]),
+            orElse: () => null);

Should this code distinguish between an object which has a named property that has a value of null with an object which has no such named property? I think it probably should. But am not certain about this.

+    if (object == null) {
+      object = _stack
+          .reversed
+          .firstWhere(
+              (v) => (object = _getNamedProperty(v, parts[0])) != null,
+              orElse: () => null);

The fix would be to change _getNamedProperty() to return a NO_SUCH_PROPERTY sentinel value instead of null in this case. (Or to make a _hasNamedProperty() method, but that is probably slower).

Btw - if you didn't see already you can run the mustache spec tests with: dart test/mustache_spec_test.dart This isn't a dart unit test but it runs the mustache spec tests and prints output.

I had a weird bug when running the test cases, I had to add some extra brackets on these lines. I think I'm one SDK version behind, this could have been fixed already.

+++ b/lib/template.dart
@@ -171,9 +171,9 @@ class _Template implements Template {
                        return null;
                }
                var invocation = null;
-               if (field is VariableMirror || field is MethodMirror && field.isGetter) {
+               if ((field is VariableMirror) || (field is MethodMirror) && field.isGetter) {
                        invocation = instance.getField(field.simpleName);
-               } else if (field is MethodMirror && field.parameters.length == 0) {
+               } else if ((field is MethodMirror) && field.parameters.length == 0) {
                        invocation = instance.invoke(field.simpleName, []);
                }

[michaelhixson]

Ok, I made another attempt. https://github.com/michaelhixson/mustache/commit/1e90f8b3352a0fad01ab3f25ff31e2a1fe8c54cc

I'm worried that this bit could actually lead to wrong results if there is a mix of objects and Maps in the stack. This code will always choose the first map, even if there is an object with that named property that is closer to the top of the stack.

This should be fixed now. It always tries, for each value in the stack:

1. Map value, if the key is contained in the map
2. Reflection, if the value has a property with the given name

One other way to optimise this would be to cache the names of the properties rather than looking them up each time.

I find the mirrors API a bit confusing and I wasn't sure what to cache. I agree this would be worth investigating though.

Should this code distinguish between an object which has a named property that has a value of null with an object which has no such named property? I think it probably should. But am not certain about this.

Agreed. I changed it so null is a valid value (as in a map lookup where the map does contain the key, and its value is null). It's treated like the empty string. Is that all right?

The fix would be to change _getNamedProperty() to return a NO_SUCH_PROPERTY sentinel value instead of null in this case. (Or to make a _hasNamedProperty() method, but that is probably slower).

This is in there now. The _NO_SUCH_PROPERTY sentinel value is handled like null was previously - it causes an error to be thrown unless it's in lenient mode.

Btw - if you didn't see already you can run the mustache spec tests with: dart test/mustache_spec_test.dart This isn't a dart unit test but it runs the mustache spec tests and prints output.

I've checked that no additional tests failed this time.

I had a weird bug when running the test cases, I had to add some extra brackets on these lines. I think I'm one SDK version behind, this could have been fixed already.

Weird. The Dart editor was ok with it, but Dart from the command line was not. I've added in extra parentheses.

However one problem is dart2js reflection support. Ideally there would be two libraries, both within the mustache package. One library would allow non-reflection based library for use in the browser. I've got a few ideas of how to separate this so that the code can be shared for both libraries.

Does dart2js just not support reflection at all? That's too bad. I've never used dart2js so I'm not sure I'll be of much help here.

[xxgreg]

Good work. The code is more concise now too.

I don't think there's any need to worry about reflection performance and caching for now. If you pass only maps to render() then none of the reflection code will run. I have some ideas on improving the performance - but that can be done as a separate change.

Perhaps you should check what happens with accessing lists via reflection. Do you want to use reflection for looking up list properties? I think the code may do this at the moment. This is probably the right way to handle this.

For example: data: {'list': [1, 2, 3]} template: {{list.length}}

Or this: data: {'list': [1, 2, 3]} template: {{list.first}}

Lenient/strict mode looks good.

I think dart2js reflection is a work in progress. It may have some nasty code size implications when used because it will make it very hard for dart2js to optimise code.

[michaelhixson]

Perhaps you should check what happens with accessing lists via reflection. Do you want to use reflection for looking up list properties? I think the code may do this at the moment. This is probably the right way to handle this.

Yes, this was intentional. Checking list.isEmpty can be especially useful in a template. For maps, the key has priority over the property.

var view = {'foo': { 'bar': 'baz', 'length': '1 million' }};
var template = '{{#foo}}{{bar}} {{length}} {{isEmpty}}{{/foo}}';
var output = mustache.parse(template).renderString(view);
print(output); // "baz 1 million false"

[xxgreg]

I've merged the changes. A couple of unit tests are failing. It looks like it's due to handling of null in strict mode (which is the default). In strict mode passing a null value should result in a FormatException being thrown. I'm open to discussion whether this is the correct behaviour or not.

https://drone.io/github.com/xxgreg/mustache/13

[michaelhixson]

If null values are invalid, is there any need for the sentinel NO_SUCH_PROPERTY value? They'd be treated the same, right?

I don't have much of an opinion here. I guess I took the NO_SUCH_PROPERTY suggestion to mean you wanted null to be valid (treated as an empty string), but I probably just misunderstood.

Also, my bad for not running your unit tests. I was only trying the spec tests.

[xxgreg]

No worries about the unit tests. I forget to run them sometimes too - but then drone.io sends me angry emails ;)

About NO_SUCH_PROPERTY good point - I obviously hadn't though that through. Sorry about the confusion.

My original idea was to have two modes. Strict mode and lenient mode. This was based on a Java mustache library - I think. In lenient mode nulls are treated as empty string. In strict mode nulls are an error. The idea with strict mode is to catch all mistakes in the template as soon as possible.

Do that sound like a good idea to you? I'm not sure which behaviour is better. I could imagine using null could actually be useful.

If you're busy - I'm happy changing the tests to reflect the currently implemented behaviour. I can then open another ticket about strict mode, and we can stall this conversation for a better time.

If you're not too busy keep on reading.

In this case it's probably useful to have the block omitted:

class Foo {
   var bar = null;
}

parse('{{#foo}}{{bar}}{{/foo}}')
   .renderString({'foo': null});

In this case there should be an error:

parse('{{#foo}}{{notAMember}}{{/foo}}')
   .renderString({'foo': new Foo()});

Not sure whether an error is useful or not in this case:

parse('{{#foo}}{{bar}}{{/foo}}')
   .renderString({'foo': new Foo()});

[michaelhixson]

Here would be my preference for your three examples:

1. Lenient mode: ok. Strict mode: ok.
2. Lenient mode: ok. Strict mode: error.
3. Lenient mode: ok. Strict mode: ok.

I think that is what I tried to implement, too, but I might have messed up.

[xxgreg]

I feel like it would be useful for end users if no. 3 in strict mode would generate an error. But I'm not super fussed about this. Shall I change the tests to reflect the current behaviour? By current - I mean what you've already implemented.

[michaelhixson]

I do think that {{nullProperty}} should print the empty string, lenient or not. I don't see it as a win to force those people to do {{#nullableProperty}}{{.}}{{/nullableProperty}} instead.

(However, {{nullProperty.anything}} should throw an error.)

The {{notAMember}} failure seems like it will have very few "false positives". By that I mean, if that error is thrown, it is very likely that the programmer made a mistake elsewhere, like a typo. I think a {{nullProperty}} failure would have many more false positives (maybe even a majority), meaning the programmer knew (or didn't care) that it was null, and then was surprised by the way this library reacted.

TL;DR yes, I think the tests should be changed.

[xxgreg]

Cool. I'll do that and publish it on pub.

On Thu, Jun 20, 2013 at 2:11 PM, michaelhixson notifications@github.comwrote:

I do think that {{nullProperty}} should print the empty string, lenient or not. I don't see it as a win to force those people to do {{#nullableProperty}}{{.}}{{/nullableProperty}} instead.

(However, {{nullProperty.anything}} should throw an error.)

The {{notAMember}} failure seems like it will have very few "false positives". By that I mean, if that error is thrown, it is very likely that the programmer made a mistake elsewhere, like a typo. I think a {{nullProperty}} failure would have many more false positives (maybe even a majority), meaning the programmer knew (or didn't care) that it was null, and then was surprised by the way this library reacted.

TL;DR yes, I think the tests should be changed.

— Reply to this email directly or view it on GitHubhttps://github.com/xxgreg/mustache/issues/3#issuecomment-19727030 .