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

Escaping dot should be taken in deep property #402

Merged
merged 4 commits into from Mar 20, 2015

Conversation

@umireon
Copy link
Contributor

@umireon umireon commented Mar 17, 2015

According to getPathInfo.js, deep.property(name) is assumed to support escaping dot . by prepending backslash \. However, escaping dot in deep.property seems to work in the unexpected way. Here is the example:

> expect({"foo.bar": "baz"}).to.have.deep.property("foo.bar", "baz");
AssertionError: expected { 'foo.bar': 'baz' } to have a deep property 'foo.bar'
// fail expectedly

> expect({"foo.bar": "baz"}).to.have.deep.property("foo\\.bar", "baz");
AssertionError: expected { 'foo.bar': 'baz' } to have a deep property 'foo\\.bar'
// fail unexpectedly

> expect({"foo\\.bar": "baz"}).to.have.deep.property("foo\\.bar", "baz");
// success unexpectedly

The current implementation of deep.propertynever match {"foo\\.bar": "baz"} because backslash \ is not removed on processing escape. This PR fixes this problem. In addition, it includes the test case to ensure the problem fixed.

Noted that this PR obviously breaks some backward compatibility.

* removing backslash prior to "."
* test case: "foo\\.bar" names {"foo.bar": "baz"}
@keithamus
Copy link
Member

@keithamus keithamus commented Mar 17, 2015

Thanks for the PR @umireon!

While it does strictly break backwards compatibility - I'd be shocked if anyone was bitten by this - so I'm on the fence whether or not to merge it here or wait for 3.x.x. Having said that - as you mentioned, trying to escape results in wild behaviour so likely if people have seen this, they've seen bugged out behaviour - so I'd consider this a bug fix.

Irregardless, a few notes:

  • If we're escaping .s we should probably also escape []s. Would you be happy to implement [] escaping too?
This commit includes:
* getPathInfo handles escaping `.[]`
* Documentation added to `.property`
* Documentation added to `.getPathInfo`
* Test cases for `.property`
* Test cases for `.deep.property`
* Test cases for `.getPathInfo`
Note that the input of `.getPathInfo` assumed to match the following:
/^(?:(?:\\[.\[\]]|[^.\[\]])+|\[\d+\])(?:\.(?:\\[.\[\]]|[^.\[\]])+|\[\d+\])$/
@umireon
Copy link
Contributor Author

@umireon umireon commented Mar 18, 2015

Hi, @keithamus !
I've written some documentations and implemented escaping [].

In fact, the path strings which the current implementation of .deep.property accepts is very wide.
I assumed the input of .deep.property matches /^(?:(?:[.[]]|[^.[]])+|[\d+])(?:.(?:[.[]]|[^.[]])+|[\d+])$/ to keep the implementation clean.
Therefore, any path string which does not may match the expression results in an unexpected way.

I'm looking forward to hearing from you,

deep-path

var re = /(?:\\[.\[\]]|[^.\[\]])+|\[(\d+)\]/g
, parsed = []
, value;
while ((value = re.exec(path)) !== null) {

This comment has been minimized.

@umireon

umireon Mar 18, 2015
Author Contributor

I need to use while instead of Array#map to avoid matching /[(\d+)]/ twice.

@keithamus
Copy link
Member

@keithamus keithamus commented Mar 18, 2015

Well, .[] isnt officially supported - it was more a by-product of implementation, and I'm not sure its something we need to explicitly support (especially as its not familiar syntax for javascript). Would that not simplify parsing a lot?

@umireon
Copy link
Contributor Author

@umireon umireon commented Mar 19, 2015

I'm sorry if I didn't get your point, but you mean it would be nice to implement
escaping using shorter regular expression with modifying the string?

@keithamus
Copy link
Member

@keithamus keithamus commented Mar 19, 2015

Pretty much yes. The regexp seems a little complex - and unless I'm mistaken a fair chunk of the complexity comes from detecting . vs \\. vs .[] vs \\.[] vs .\\[], but in reality we just need to find ., [] and escape on \\. and \\[].

Please correct me if I'm wrong though.

@umireon
Copy link
Contributor Author

@umireon umireon commented Mar 19, 2015

Frankly speaking, I'm not confident to keep the behavior unchanged with parsing the regular language (= the simple syntax of .deep.property's name) by the mechanism more complex than regular expression.
Regular expression is so simple syntax that it can easily be proved to match the desired string.
Therefore, I unintentionally wrote the code whose everything is in one regular expression but it is not concern of this PR.
Finally, it's my fault to mangling the regular expression in this PR, very sorry 😢.

@@ -61,13 +62,13 @@ module.exports = function getPathInfo(path, obj) {
*/

function parsePath (path) {
var str = path.replace(/\[/g, '.[')
var str = path.replace(/([^\\])\[/g, '$1.[')

This comment has been minimized.

@umireon

umireon Mar 19, 2015
Author Contributor

Without (([^\\]), $1), a\\[results in accessing a.[.

, parts = str.match(/(\\\.|[^.]+?)+/g);
return parts.map(function (value) {
var re = /\[(\d+)\]$/
var re = /^\[(\d+)\]$/

This comment has been minimized.

@umireon

umireon Mar 19, 2015
Author Contributor

This ^ ensures test\\[1] parsed as the property test[1], not the index 1

@umireon
Copy link
Contributor Author

@umireon umireon commented Mar 19, 2015

Actually, ] don't have to be escaped.

@umireon
Copy link
Contributor Author

@umireon umireon commented Mar 19, 2015

The regexp seems a little complex - and unless I'm mistaken a fair chunk of the complexity comes from detecting . vs . vs .[] vs .[] vs .[], but in reality we just need to find ., [] and escape on . and [].

The expression was complex simply because three regular expressions and memory rewriting were combined.

@keithamus
Copy link
Member

@keithamus keithamus commented Mar 19, 2015

@umireon that now looks much clearer to me. Good job 😄

I'd like to pester you for one more bit:

  • Add documentation for .deep.property escaping in the deep flag docs (assertion.jsL68-80).

Once that's done I'm happy to wrap up this PR 😄

@umireon
Copy link
Contributor Author

@umireon umireon commented Mar 20, 2015

@keithamus I've made it done!

@keithamus
Copy link
Member

@keithamus keithamus commented Mar 20, 2015

Great work @umireon! 🎉

keithamus added a commit that referenced this pull request Mar 20, 2015
Escaping dot should be taken in deep property
@keithamus keithamus merged commit 32e890e into chaijs:master Mar 20, 2015
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@keithamus
Copy link
Member

@keithamus keithamus commented Mar 26, 2015

Linking #398

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

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.