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

Fix bug when iterating through object with empty keys #49

Closed
wants to merge 1 commit into from

Conversation

untitaker
Copy link
Contributor

No description provided.

@untitaker
Copy link
Contributor Author

I didn't yet figure out where the bug is, would appreciate hints.

@sunng87
Copy link
Owner

sunng87 commented Nov 11, 2015

Probably the issue is when we try to build a json path with empty string:

let new_path = format!("{}/{}/{}", path, param, k);

Then we got path/param/ and failed to get the value from json object. I'm not sure if this is permitted in only implementation.

@untitaker
Copy link
Contributor Author

I believe I have found a bugfix, could you check?

On Tue, Nov 10, 2015 at 07:02:19PM -0800, Ning Sun wrote:

Probably the issue is when we try to build a json path with empty string:

let new_path = format!("{}/{}/{}", path, param, k);

Then we got path/param/ and failed to get the value from json object. I'm not sure if this is permitted in only implementation.


Reply to this email directly or view it on GitHub:
#49 (comment)

@sunng87
Copy link
Owner

sunng87 commented Nov 11, 2015

Thanks for the fix. We need think about if removing "" will cause some other issues for accessing json data structure.

@untitaker
Copy link
Contributor Author

Perhaps you'll have problems if you (incorrectly) use /ident instead of ./ident, but I don't really see anything else. Note that I don't really understand the handlebars impl though.

@untitaker untitaker changed the title Testcase for empty-keys bug with #each Fix bug when iterating through object with empty keys Nov 11, 2015
sunng87 added a commit that referenced this pull request Nov 12, 2015
@sunng87
Copy link
Owner

sunng87 commented Nov 12, 2015

I should have found a better fix and cherry-picked your test case. Thanks for your fix.

@sunng87 sunng87 closed this Nov 12, 2015
@untitaker
Copy link
Contributor Author

Yup, that does seem like a better solution. I wonder though what my fix would've broken? I've tried a lot of things and they all continued to work...

On 12 November 2015 16:53:45 CET, Ning Sun notifications@github.com wrote:

I should have found a better fix and cherry-picked your test case.
Thanks for your fix.


Reply to this email directly or view it on GitHub:
#49 (comment)

Sent from my phone. Please excuse my brevity.

@sunng87
Copy link
Owner

sunng87 commented Nov 13, 2015

Your fix should be correct for most common case. But it possibly breaks some incorrect usage like /a/b or /a//b. I hope we can keep compatibility for these cases so we better choose a safer fix.

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.

2 participants