Resolve #589, and possibly fix other manifestations of this bug

[raymond-lam]

Resolve #589
Because value is used as an intermediate variable as well as the final return value of lookup, there is a possibility that value is returned even if the hasProperty test fails. In the reported issue, the string's length is stored in value, even though lookupHit is false (because a string has a length property but is not an Object). The outer loop breaks, and value is returned, incorrectly as the string's length. lookup should never return a value where the lookup is not hit, so don't use value as an intermediate variable -- only assign to it when we know that the hasProperty test passes.

[dasilvacontin]

@raymond-lam Thanks! Looks good!

I think it would be nice to release this as a major version. I fear that some people might have bumped into this non-intended behavior and taken it as a feature, therefore we would be breaking their app/render.

Thoughts? /cc @phillipj

[raymond-lam]

@dasilvacontin Thanks!

You might be right. So what would that mean -- would we wait before merging this PR, or simply that the next release would be a major one?

/cc @phillipj

[dasilvacontin]

@raymond-lam Ideally, it would be nice to check if there are other fixes to review and merge before bumping the version. I don't have time to check stuff, I just somehow bumped into this PR. Maybe @phillipj, or someone (?). (Personally busy til Jan 21th)

Do you have any hurry in getting this merged?

[raymond-lam]

@dasilvacontin no hurry -- just curious as to what the process was. Thanks!

[phillipj]

I would expect to be able to output Array.length. That still seems to work with these changes, would you mind adding that as part of dot_notation test?

diff --git a/test/_files/dot_notation.mustache b/test/_files/dot_notation.mustache
index 2640514..8b6b2e9 100644
--- a/test/_files/dot_notation.mustache
+++ b/test/_files/dot_notation.mustache
@@ -8,3 +8,4 @@
 <p>Zero: {{truthy.zero}}</p>
 <p>False: {{truthy.notTrue}}</p>
 <p>length of string should not be rendered: {{price.currency.name.length}}</p>
+<p>length of an array should be rendered: {{authors.length}}</p>
diff --git a/test/_files/dot_notation.txt b/test/_files/dot_notation.txt
index eeb346d..98d2e6a 100644
--- a/test/_files/dot_notation.txt
+++ b/test/_files/dot_notation.txt
@@ -8,3 +8,4 @@
 <p>Zero: 0</p>
 <p>False: false</p>
 <p>length of string should not be rendered: </p>
+<p>length of an array should be rendered: 2</p>

Completely agree doing a major version bump, better safe than sorry. No other upcoming breaking changes as I know of, so this would be good to go as soon as it lands IMO.

And thanks for fixing this 👍

[dasilvacontin]

I would expect to be able to output Array.length.

@phillipj I think we should check the specs 😅 . afaik, Mustache templates should be language agnostic. .length is not language agnostic (eg PHP does not have it).

[phillipj]

Hah, that's a very good point. Pretty sure I used it a lot when reusing templates between java & JS tho. That brings the philosophical question of whether or not we should prevent Array.length from being resolvable or not at all.

I'm leaning towards letting it be resolvable. Not everyone has the need to use templates across different languages. And as mentioned, I'm pretty sure it's completely valid between java and JS. Us deliberately hiding something that's obviously useful in many scenarios feels strange, doesn't it?

On second thought the exact same could be said about not being able to resolve String.length in templates..

[dasilvacontin]

I feel there should be a standard way of printing the length of a String, Array, or Object, and of iterating an Object (hey, maybe even iterating a String), without having to write your own lambda, or doing complicated stuff. It would still be logic-less.

Maybe other Mustache implementations have come up with a PRAGMA since Mustache spec development is halted.

[raymond-lam]

@phillipj @dasilvacontin As far as this particular PR goes, the accessibility of Array.length is not affected -- with or without this PR, Array.length is indeed accessible.

As to the question of whether String.length should be accessible -- without this PR, the accessibility of String.length is inconsistent. See the original issue #589 for an example of when it is accessible and when it is not. Strictly speaking, this PR makes the accessibility of String.length (and other properties of boxed primitives) consistent, and it so happens that because hasProperty's conditions, it would be consistently inaccessible. I think the added consistency is at least an improvement.

If we wanted to change the accessibility of properties like String.length, I think we should leave these changes in (because we want consistency regardless) and alter the conditions of hasProperty (for example, by taking out the typeof obj === 'object' constraint).

This brings up another case, though perhaps a less worthy one:

var aFunction = function() {};
aFunction.foo = 'bar';
console.log(Mustache.render('{{a.foo}}', { a: aFunction }));

How should this render?

Given all of this, how should we proceed with this PR? (Should I still add the tests that @phillipj suggested?)

[phillipj]

I think the added consistency is at least an improvement.

Absolutely 👍

Maybe the always wise @bobthecow has some thoughts on this one?

[raymond-lam]

Whoops -- fixed typo in the aFunction code snippet above. Now it should make a lot more sense.

[raymond-lam]

Also another point -- if we decided to make properties of boxed primitives (and functions for that matter) consistently accessible (which obviously would require revisions to this PR), then that would make this PR not a breaking change and we wouldn't have to do a major version bump, right?

[dasilvacontin]

Hmm. Making properties available where previously weren't would stop property lookup to previous contexts. That would probably need to go to a major as well.

[raymond-lam]

@dasilvacontin Oh, right.

[raymond-lam]

@dasilvacontin @phillipj @bobthecow

Hi everyone,
Were there any further thoughts on this pull request? Specifically, did we decide whether or not I should add the unit test @phillipj mentioned before it gets merged in?

Thanks,
Ray

[raymond-lam]

@dasilvacontin @phillipj @bobthecow

Hi everyone,
It's been a few months now... have there any further thoughts on this pull request? Specifically, did we decide whether or not I should add the unit test @phillipj mentioned before it gets merged in?

Thanks,
Ray

[phillipj]

Sorry this stalled, really sorry but thanks for your patience!

As I've stated before, I'm all for consistency which this PR seems to improve.

Personally from a user's perspective, find it less than ideal that String.length cannot be rendered. I've done some testing with an old java & JS project where mustache were used, and have found that String.length was actually possible to render with mustache.js pre v2.0.

Since it sounds like these changes has to be released in a new major version anyways, I'd rather see String.length renderable. Though exposing any property from something that is strictly not typeof obj === 'object' might cause other bad effects that I'm not aware yet.

Bottom line is I'd prefer the simplicity of being able to render what is available, over mustache spec compliance for the sake of strictness.

@dasilvacontin you agree?

[raymond-lam]

Though exposing any property from something that is strictly not typeof obj === 'object' might cause other bad effects that I'm not aware yet.

One such bad effect might be that the bug_length_property render test would end up failing.

[raymond-lam]

(BTW I added the Array.length test that @phillipj mentioned above)

[phillipj]

Thanks 👍 Whether or not we end up exposing String.length or not, having the expected Array.length result documented is a good bonus.

[dasilvacontin]

Okay, let me state my thoughts in case I'm wrong:

• right now this would be a major because it prevents string length rendering, which is currently possible, and a quite likely used 'feature'
• It's ugly that you can render an array's length but you can't render an string's length, which is most likely caused by that typeof obj === 'object' constraint @raymond-lam pointed out.

So, if we removed that constrain and made string's length able to be rendered, we could release this as a patch. We would need to be sure we would not be breaking people. Otherwise, major for safety. Removing the constraint and checking which tests fail would be a start.

[dasilvacontin]

Btw, sorry for the stalling @raymond-lam!

[dasilvacontin]

I agree with your comments @phillipj. Actually, due to #618 (comment), making string's length renderable would need to be a major anyways. So nevermind what I've said about the patch version.

[dasilvacontin]

Apparently in can only be used with Object types. Maybe do:

(typeof obj === 'object' ? (propName in obj) : obj.hasOwnProperty(propName))

Feel free to revert/delete my commit. I can't work on it right now.

[raymond-lam]

I've fixed the issue from @dasilvacontin's change where one cannot use in for primitives, but as I mentioned to @phillipj, this breaks the 'bug_length_property' test, which I think is a good test which describes expected behavior and probably shouldn't be broken.

So, I've pushed another commit, which mostly undoes the changes by @dasilvacontin but makes String.length rendering possible through dot notation, while still passing the 'bug_length_property' test. I'm not sure how I feel about this fix, but I guess one could rationalize that if you're using dot notation to access a property, you really mean to access that property.

[raymond-lam]

Also just added a test to address the original issue as reported specifically.

[raymond-lam]

Hi @dasilvacontin, @phillipj,
Were there any other thoughts on this PR?

[phillipj]

LGTM 👍 Thanks for a good discussion and being very patience.

I'll let this be open for a few days in case @dasilvacontin has any other thoughts, or merge it within the end of this week -- I'm creating a reminder for my self so this won't stall again.

[raymond-lam]

@dasilvacontin BTW, if you have the time, would love to hear your thoughts on #643 while we are here.

[raymond-lam]

@dasilvacontin Just wanted to bump this thread so that we didn't forget about this PR.

[raymond-lam]

@dasilvacontin @phillipj just wanted to bump this PR, as it's been quite some time now. Thanks!

[dasilvacontin]

It’s been on my mind like every week 😅. I think mid-May might be good.

…

On 26 Apr 2018, at 14:58, Raymond Lam ***@***.***> wrote: @dasilvacontin @phillipj just wanted to bump this PR, as it's been quite some time now. Thanks! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[raymond-lam]

@dasilvacontin Great, thanks! FYI I will be traveling for much of the summer, so I may not be able to respond to any comments until later in the summer.

[dasilvacontin]

No worries, enjoy the trip(s)! :)

…

On 27 Apr 2018, at 05:20, Raymond Lam ***@***.***> wrote: @dasilvacontin Great, thanks! FYI I will be traveling for much of the summer, so I may not be able to respond to any comments until later in the summer. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[raymond-lam]

@dasilvacontin I'm back from traveling -- just wanted to check in on this :)

[dasilvacontin]

Same old :(. Maybe the PR should move on without my review, I haven’t been a maintainer in the project for a long while already. David

…

Le 20 juil. 2018 à 20:50, Raymond Lam ***@***.***> a écrit : @dasilvacontin I'm back from traveling -- just wanted to check in on this :) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[raymond-lam]

@dasilvacontin No problem. Who do you recommend to look at it then? @phillipj ?

[phillipj]

As before, I'm very positive to these changes. Greatly appreciate both the tests and the added thorough comments.

For the sake of being extra cautious and signal to users of mustache.js that changes has landed that might cause different output compared to before, I'd like to see these changes be part of a new major version.

I haven't published a new version before, but I'll give it a go within a week or two. Just came back from vacation and need some warmup time before giving the release script a go 😄 Again, thanks for your extreme patience!

I don't have the resources to commit to maintaining anymore.

[raymond-lam]

@phillipj That sounds great — thanks!

[phillipj]

Short update to inform I'm getting to this PR eventually, v2.3.1 got published yesterday.

[phillipj]

I don't have the resources to commit to maintaining anymore.

Thanks for sharing and being explicit about it 👍 mustache.js wouldn't be the same without your hard work, much appreciated!

[dasilvacontin]

@phillipj You're welcome! Neither without your hardwork either!

Sorry for not having "given up" earlier, I was hoping for a comeback :). Will mention mustache.js if I ever talk at a JS event again! (I'm not so much in the community anymore since most talks are about front-end frameworks/libraries and not interesting in them atm)

[phillipj]

@raymond-lam as there might seem to be a custom tags regression in the newly released v2.3.1, I'm holding this off until we've got that fixed. Don't want to add more changes to the mix while digging into what's causing that regression.

[raymond-lam]

@phillipj Excellent! Thank you for all your help! Glad we got this thing merged in.