RegularExpression constructor is not filtering non-capturing groups

[aslakhellesoy]

From @Jordyderuijter on May 23, 2017 11:4

Hi there,

It seems like there is a bug in the RegularExpression class in the cucumber-expressions dependency.
The regex in this constructor looks for capturing groups when processing steps using the regex:

/\(([^(]+)\)/g

This regex finds everything between braces (a.k.a. capturing groups), but it also accepts non-capturing groups. This causes the constructor to generate parameter types for the non capturing groups (instead of the capturing groups only).

These parameter types are used in the buildArguments function, but are only applied to capturing groups, since the given step regex filters out the non-capturing groups correctly.

I would expect the RegularExpression constructor to filter out these capturing groups correctly.

The step and step definitions I used to reproduce the issue are:

Step:
I can cancel the 1st slide upload

Step definition:
(\S+) ?(can|cannot)? (?:delete|cancel) the (\d+)(?:st|nd|rd|th) (attachment|slide) ?(?:upload)?

In this case 7 parameter types are being generated by the constructor, while it should only create 4, since 3 of them are non-capturing groups. As a result, the parameter type that belongs to (\d+) is being applied to (attachment|slide), treating it as an integer. This results into a NaN value that is being passed to my step definition.

Version info:

• Cucumberjs: 2.1.0
• Node: v6.9.1
• Npm: 3.10.8

Copied from original issue: cucumber/cucumber-js#837

[aslakhellesoy]

Dupe of #131

[aslakhellesoy]

My bad - this is not a dupe of #131 so I'm reopening it.

We now have 3 proposed fixes for this issue:

• cucumber-expressions: #131 Cucumber Expressions: Non Capture Group #179 - has unresolved code review comments
• cucumber-expressions: make CAPTURE_GROUP_PATTERN aware of non-… #216 - fixes the bug with a more elaborate regexp-parsing regexp than Fix for Cucumber expressions #211 ignore non capturing groups #220, but for js only
• Fix for Cucumber expressions #211 ignore non capturing groups #220 - implements a fix for java/javascript/ruby

I'm going to improve #220 with the technique from #216 and merge that.

I realise this implementation doesn't handle nested groups, as explained in comments in #216, but we'll handle that in a different issue.

[aslakhellesoy]

Fixed by #220

[aslakhellesoy]

I'm about to make a change that would disallow optional capture groups such as (can|cannot)?. Groups will have to be non-capturing ((?:can|cannot)?) or required/non-optional ((can|cannot)).

So the example above would have to be one of the following:

(\S+) ?(can|cannot) (?:delete|cancel) the (\d+)(?:st|nd|rd|th) (attachment|slide) ?(?:upload)?

or:

(\S+) ?(?:can|cannot)? (?:delete|cancel) the (\d+)(?:st|nd|rd|th) (attachment|slide) ?(?:upload)?

Can you see any issues with this @Jordyderuijter?

[Jordyderuijter]

This would be a breaking change, but I think it can easily be solved by using a format like this:
(can|cannot|)

In this case you can still use a capturing group without making it optional.

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.