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

Cucumber forces Integer type on a \d+ capture group #658

Closed
yktoo opened this issue Jul 19, 2019 · 10 comments · Fixed by #659
Closed

Cucumber forces Integer type on a \d+ capture group #658

yktoo opened this issue Jul 19, 2019 · 10 comments · Fixed by #659
Labels

Comments

@yktoo
Copy link

yktoo commented Jul 19, 2019

Summary

The compatibility with previous version(s) is broken.

A step definition that uses (\d+) capture group forces the method have an Integer (or int) argument. Previously a String would do fine.

Example code:

@When("^I type (\\d+) into input field$")
public void register(String value) {
...

Expected Behavior

The typed value is converted into a String, just like it was previously (tested with info.cukes.cucumber-java v1.2.5)

Current Behavior

An exception is thrown:

cucumber.runtime.CucumberException: Failed to invoke nl.ns.cucumber.LoketdienstSteps.register(String) in file:[...path...], caused by java.lang.IllegalArgumentException: argument type mismatch
	at cucumber.runtime.Utils$1.call(Utils.java:29)
	at cucumber.runtime.Timeout.timeout(Timeout.java:16)
	at cucumber.runtime.Utils.invoke(Utils.java:20)
	at cucumber.runtime.java.JavaStepDefinition.execute(JavaStepDefinition.java:57)
	at cucumber.runner.PickleStepDefinitionMatch.runStep(PickleStepDefinitionMatch.java:50)
	at cucumber.runner.TestStep.executeStep(TestStep.java:65)
	at cucumber.runner.TestStep.run(TestStep.java:50)
	at cucumber.runner.PickleStepTestStep.run(PickleStepTestStep.java:43)
	at cucumber.runner.TestCase.run(TestCase.java:46)
	at cucumber.runner.Runner.runPickle(Runner.java:50)
	at cucumber.runtime.Runtime$1.run(Runtime.java:104)
	at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:511)
	at java.util.concurrent.FutureTask.run(FutureTask.java:266)
	at cucumber.runtime.Runtime$SameThreadExecutorService.execute(Runtime.java:258)
	at java.util.concurrent.AbstractExecutorService.submit(AbstractExecutorService.java:112)
	at cucumber.runtime.Runtime.run(Runtime.java:101)
	at io.cucumber.core.cli.Main.run(Main.java:43)
	at io.cucumber.core.cli.Main.main(Main.java:14)
Caused by: java.lang.IllegalArgumentException: argument type mismatch
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at cucumber.runtime.Utils$1.call(Utils.java:26)
	... 17 more

Context & Motivation

This is particularly bad since it would try to convert whatever is given, for example, providing 2222222222 (which is too big for an Integer) results in:

cucumber.runtime.CucumberException: Could not convert arguments for step [...] defined at '...'.
The details are in the stacktrace below.
	at cucumber.runner.PickleStepDefinitionMatch.couldNotConvertArguments(PickleStepDefinitionMatch.java:69)
	at cucumber.runner.PickleStepDefinitionMatch.runStep(PickleStepDefinitionMatch.java:46)
	at cucumber.runner.TestStep.executeStep(TestStep.java:65)
	at cucumber.runner.TestStep.run(TestStep.java:50)
	at cucumber.runner.PickleStepTestStep.run(PickleStepTestStep.java:43)
	at cucumber.runner.TestCase.run(TestCase.java:46)
	at cucumber.runner.Runner.runPickle(Runner.java:50)
	at cucumber.runtime.Runtime$1.run(Runtime.java:104)
	at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:511)
	at java.util.concurrent.FutureTask.run(FutureTask.java:266)
	at cucumber.runtime.Runtime$SameThreadExecutorService.execute(Runtime.java:258)
	at java.util.concurrent.AbstractExecutorService.submit(AbstractExecutorService.java:112)
	at cucumber.runtime.Runtime.run(Runtime.java:101)
	at io.cucumber.core.cli.Main.run(Main.java:43)
	at io.cucumber.core.cli.Main.main(Main.java:14)
Caused by: io.cucumber.cucumberexpressions.CucumberExpressionException: ParameterType {int} failed to transform [2222222222] to class java.lang.Integer
	at io.cucumber.cucumberexpressions.ParameterType.transform(ParameterType.java:190)
	at io.cucumber.cucumberexpressions.Argument.getValue(Argument.java:64)
	at io.cucumber.stepexpression.ExpressionArgument.getValue(ExpressionArgument.java:15)
	at cucumber.runner.PickleStepDefinitionMatch.runStep(PickleStepDefinitionMatch.java:41)
	... 13 more
Caused by: java.lang.NumberFormatException: For input string: "2222222222"
	at java.lang.NumberFormatException.forInputString(NumberFormatException.java:65)
	at java.lang.Integer.parseInt(Integer.java:583)
	at java.lang.Integer.valueOf(Integer.java:740)
	at java.lang.Integer.decode(Integer.java:1197)
	at io.cucumber.cucumberexpressions.BuiltInParameterTransformer.transform(BuiltInParameterTransformer.java:51)
	at io.cucumber.cucumberexpressions.ParameterTypeRegistry$5.transform(ParameterTypeRegistry.java:70)
	at io.cucumber.cucumberexpressions.ParameterTypeRegistry$5.transform(ParameterTypeRegistry.java:67)
	at io.cucumber.cucumberexpressions.ParameterType$TransformerAdaptor.transform(ParameterType.java:215)
	at io.cucumber.cucumberexpressions.ParameterType.transform(ParameterType.java:186)
	... 16 more

Your Environment

  • Cucumber 4.6.0
  • Java 8
@yktoo yktoo changed the title Cucumber forces Integer type on a Cucumber forces Integer type on a \d+ capture group Jul 19, 2019
@mlvandijk
Copy link
Member

Not sure this is a bug or a feature. "\d, \D: ANY ONE digit/non-digit character. Digits are [0-9]" so it imho it really should be a digit (int) and not a String.

Also note there have been some breaking changes on major versions. Please check the migration guides here.

@yktoo
Copy link
Author

yktoo commented Jul 19, 2019

imho it really should be a digit (int) and not a String.

Alright, but how do you handle the input like the 2222222222 above, which doesn't fit into an Integer?

@aslakhellesoy
Copy link
Contributor

aslakhellesoy commented Jul 19, 2019

@yktoo how does this work?

@When("^I type (\\d+) into input field$")
public void register(java.math.BigInteger value) {

@yktoo
Copy link
Author

yktoo commented Jul 19, 2019

Nope.

cucumber.runtime.CucumberException: Failed to invoke ....register(BigInteger) in file:[...], caused by java.lang.IllegalArgumentException: argument type mismatch
	at cucumber.runtime.Utils$1.call(Utils.java:29)
	at cucumber.runtime.Timeout.timeout(Timeout.java:16)
	at cucumber.runtime.Utils.invoke(Utils.java:20)
...

@mpkorstanje
Copy link
Contributor

mpkorstanje commented Jul 19, 2019

@aslakhellesoy in the ParameterTypeRegistry the BigDecimal parameter has set the useForRegexMatch to false because it's the same pattern for integer. For Java I think we might have to parse everything to number and use the method parameter type to work out which type of number.

Prior to v3 we used to throw everything at XStream and most of the time we'd get something usable back. Cucumber expressions does have all those smart type conversions build in.

@yktoo

Can't try this out but if you use the Cucumber Expression instead it should be at bit easier:

@When("I type {word} into input field")

But since you're entering strings you may well do

@When("I type {string} into input field")

which will match I type "22222" into the input field and typographically marks the bounds of the input.

@yktoo
Copy link
Author

yktoo commented Jul 19, 2019

@mpkorstanje That is indeed my workaround to use {string} and Cucumber Expression for the time being. This lacks, however, the input validation bit.

I have two other humble thoughts on the matter:

  1. It must be possible to coerce any type into a String (simply using toString()) disregarding the capture group pattern. I can't really see why it's forbidden.
  2. The way of inferring the Integer type from \d+ is a bit controversial and I don't really understand the rationale behind it. For instance, ([\d]+) (which is essentially equivalent) works just fine supplying a String argument.

@mpkorstanje
Copy link
Contributor

mpkorstanje commented Jul 19, 2019

I don't really understand the rationale behind it.

Mostly because of an imperfect migration.

Cucumber used XStream to handle all the type conversions. We had to stop using XStream because it fails to work modern Java versions. The next best replacement was then serendipitously created cucumber-expression library. However the library was mostly designed for to solve problems encountered by cucumber implementations in weakly typed languages. Ruby, Javascript, ect only had to deal with Number rather then Integer, Long, BigInteger, ect and these assumptions were carried over in the java implementation. So we now have a library that tries to guess the types based on the regex you use instead of the other way around.

Now a while ago we did add typeHints to the CucumberExpression and RegularExpression implementation. Currently it is only used by the anonymous parameter type but I think it can be used allow for a bit more lenient argument conversion of numbers/strings.

@aslakhellesoy
Copy link
Contributor

I think this will work:

@When("I type {bigint} into input field")
public void register(java.math.BigInteger value) {

My motivation for creating the Cucumber-Expressions library was to provide a simpler alternative to RegExp which many people seem to struggle with. It then made sense to move the RegExp logic into that library so we have a consistent way to transform capture groups (or cukexp output parameters) to other types.

Another motivation was to introduce the concept of parameter types. I think {currency} conveys more info than [A-Z]{3}, and the fact that snippets will suggest known types also improves user experience.

So for me, improved UX was the primary motivation and it coincied with the need to get rid of XStream.

Because \d+ is used so frequently I thought the user experience would be better if it were automatically converted to an int in the dynamic language implementations of the library (Ruby, JavaScript).

While we've done our best to keep the RegExp support backwards compatible, there are some edge cases where the behaviour has changed or is buggy.

This will be a fun one to fix!

@mpkorstanje
Copy link
Contributor

mpkorstanje commented Jul 19, 2019

I think #659 is a good solution but I'm going to let it percolate a bit. The short version:

If we have a matching regex and it's of the right type, use that. If not treat it as an anonymous parameter type.

There is a flag to override this behavior. This flag is off for all basic types. But will be on for all user defined parameter types. This solution assumes that if you are using regex and you define a parameter type you really want that type.

I'm not sure if this assumption is correct.

This will be a fun one to fix!

Yeah.

mpkorstanje added a commit that referenced this issue Jul 19, 2019
@mpkorstanje mpkorstanje reopened this Jul 19, 2019
@mpkorstanje
Copy link
Contributor

Pushed the wrong branch.

mpkorstanje added a commit that referenced this issue Aug 2, 2019
When there is a conflict between the type hint from the regular expression
and the method prefer the the parameter type associated with the regular
expression. This ensures we will use the internal/user registered parameter
transformer rather then the default.

Unless the parameter type indicates it is the stronger type hint.

Reasoning:
1. Pure cucumber expression users will not notice this in either scenario.
2. Pure regular expression users will benefit because
   `BuiltInParameterTransformer` can now seamlessly transform any captured
   values. (For all built in types useRegexpMatchAsStrongTypeHint is
   explicitly set to false.)
2. Regular expression users that define a default transformer have little
   need to define parameter types. The default transformer should be
   sufficiently powerful to meet their needs and will often allow users to
   add custom creation methods e.g. Jacksons @JsonFactory.
3. Users who mix regular and cucumber expressions may run into conflicts
   when a registered cucumber expression and unregistered happens to
   collide. However this was the situation before this flag was added.
4. Regular expression users who define custom parameter types do so with
   the expectation that the parameter will be matched. Subverting this
   expectation when the method signature does not match may result in a
   parameter transformer that is unable to convert to the desired type.
   Leaving the user puzzled as to why his transform was ignored.

Fixes: #658
mpkorstanje added a commit that referenced this issue Aug 11, 2019
* cucumber-expressions: Prefer Java type hint over parameter type

When there is a conflict between the type hint from the regular expression
and the method prefer the the parameter type associated with the regular
expression. This ensures we will use the internal/user registered parameter
transformer rather then the default.

Unless the parameter type indicates it is the stronger type hint.

Reasoning:
1. Pure cucumber expression users will not notice this in either scenario.
2. Pure regular expression users will benefit because
   `BuiltInParameterTransformer` can now seamlessly transform any captured
   values. (For all built in types useRegexpMatchAsStrongTypeHint is
   explicitly set to false.)
2. Regular expression users that define a default transformer have little
   need to define parameter types. The default transformer should be
   sufficiently powerful to meet their needs and will often allow users to
   add custom creation methods e.g. Jacksons @JsonFactory.
3. Users who mix regular and cucumber expressions may run into conflicts
   when a registered cucumber expression and unregistered happens to
   collide. However this was the situation before this flag was added.
4. Regular expression users who define custom parameter types do so with
   the expectation that the parameter will be matched. Subverting this
   expectation when the method signature does not match may result in a
   parameter transformer that is unable to convert to the desired type.
   Leaving the user puzzled as to why his transform was ignored.

Fixes: #658

* cucumber-expressions: Prefer Go type hint over parameter type

When there is a conflict between the type hint from the regular expression
and the method prefer the the parameter type associated with the regular
expression. This ensures we will use the internal/user registered parameter
transformer rather then the default.

Unless the parameter type indicates it is the stronger type hint.

Reasoning:
1. Pure cucumber expression users will not notice this in either scenario.
2. Pure regular expression users will benefit because
   `BuiltInParameterTransformer` can now seamlessly transform any captured
   values. (For all built in types `useRegexpMatchAsStrongTypeHint` is
   explicitly set to false.)
2. Regular expression users that define a default transformer have little
   need to define parameter types. The default transformer should be
   sufficiently powerful to meet their needs and will often allow users to
   add custom creation methods e.g. Jacksons @JsonFactory.
3. Users who mix regular and cucumber expressions may run into conflicts
   when a registered cucumber expression and unregistered happens to
   collide. However this was the situation before this flag was added.
4. Regular expression users who define custom parameter types do so with
   the expectation that the parameter will be matched. Subverting this
   expectation when the method signature does not match may result in a
   parameter transformer that is unable to convert to the desired type.
   Leaving the user puzzled as to why his transform was ignored.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants