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.

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

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

Merged
merged 8 commits into from
Aug 25, 2018
Merged

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

merged 8 commits into from
Aug 25, 2018

Conversation

raymond-lam
Copy link
Contributor

@raymond-lam raymond-lam commented Jan 4, 2017

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
Copy link
Collaborator

dasilvacontin commented Jan 5, 2017

@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
Copy link
Contributor Author

@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
Copy link
Collaborator

@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
Copy link
Contributor Author

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

@phillipj
Copy link
Collaborator

phillipj commented Jan 5, 2017

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
Copy link
Collaborator

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
Copy link
Collaborator

phillipj commented Jan 5, 2017

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
Copy link
Collaborator

dasilvacontin commented Jan 5, 2017

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
Copy link
Contributor Author

raymond-lam commented Jan 5, 2017

@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
Copy link
Collaborator

phillipj commented Jan 5, 2017

I think the added consistency is at least an improvement.

Absolutely 👍

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

@raymond-lam
Copy link
Contributor Author

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

@raymond-lam
Copy link
Contributor Author

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
Copy link
Collaborator

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
Copy link
Contributor Author

@dasilvacontin Oh, right.

@raymond-lam
Copy link
Contributor Author

@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
Copy link
Contributor Author

@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
Copy link
Collaborator

phillipj commented May 6, 2017

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@phillipj
Copy link
Collaborator

phillipj commented May 7, 2017

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

@dasilvacontin
Copy link
Collaborator

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
Copy link
Collaborator

Btw, sorry for the stalling @raymond-lam!

@dasilvacontin
Copy link
Collaborator

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
Copy link
Collaborator

dasilvacontin commented May 9, 2017

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
Copy link
Contributor Author

raymond-lam commented May 10, 2017

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
Copy link
Contributor Author

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

@raymond-lam
Copy link
Contributor Author

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

@phillipj
Copy link
Collaborator

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
Copy link
Contributor Author

raymond-lam commented Aug 12, 2017

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

@raymond-lam
Copy link
Contributor Author

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

@raymond-lam
Copy link
Contributor Author

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

@dasilvacontin
Copy link
Collaborator

dasilvacontin commented Apr 26, 2018 via email

@raymond-lam
Copy link
Contributor Author

@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
Copy link
Collaborator

dasilvacontin commented Apr 27, 2018 via email

@raymond-lam
Copy link
Contributor Author

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

@dasilvacontin
Copy link
Collaborator

dasilvacontin commented Jul 20, 2018 via email

@raymond-lam
Copy link
Contributor Author

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

@phillipj
Copy link
Collaborator

phillipj commented Jul 27, 2018

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!

@dasilvacontin dasilvacontin dismissed their stale review July 27, 2018 23:23

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

@dasilvacontin dasilvacontin removed their assignment Jul 27, 2018
@raymond-lam
Copy link
Contributor Author

@phillipj That sounds great — thanks!

@phillipj
Copy link
Collaborator

phillipj commented Aug 8, 2018

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

@phillipj
Copy link
Collaborator

phillipj commented Aug 8, 2018

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
Copy link
Collaborator

@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
Copy link
Collaborator

@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
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants