Skip to content

Commit

Permalink
cucumber-expressions: Prefer Java type hint over parameter type
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@mpkorstanje
mpkorstanje committed Aug 2, 2019
1 parent 0e63c88 commit f90958d
Show file tree
Hide file tree
Showing 5 changed files with 126 additions and 25 deletions.
3 changes: 1 addition & 2 deletions ...sions/java/src/main/java/io/cucumber/cucumberexpressions/BuiltInParameterTransformer.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
import java.lang.reflect.Type;
import java.math.BigDecimal;
import java.math.BigInteger;
import java.text.NumberFormat;
import java.util.Locale;

import static java.util.Objects.requireNonNull;
Expand Down Expand Up @@ -35,7 +34,7 @@ public Object transform(String fromValue, Type toValueType) {
return new BigInteger(fromValue);
}

if (BigDecimal.class.equals(toValueClass)) {
if (BigDecimal.class.equals(toValueClass) || Number.class.equals(toValueClass)) {
return numberParser.parseBigDecimal(fromValue);
}

Expand Down
68 changes: 56 additions & 12 deletions cucumber-expressions/java/src/main/java/io/cucumber/cucumberexpressions/ParameterType.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ public final class ParameterType<T> implements Comparable<ParameterType<?>> {
private final boolean useForSnippets;
private final CaptureGroupTransformer<T> transformer;
private final boolean anonymous;
private final boolean useRegexpMatchAsStrongTypeHint;

static void checkParameterTypeName(String name) {
String unescapedTypeName = UNESCAPE_PATTERN.matcher(name).replaceAll("$2");
Expand All @@ -37,9 +38,10 @@ static ParameterType<Object> createAnonymousParameterType(String regexp) {
public Object transform(String[] arg) {
throw new UnsupportedOperationException("Anonymous transform must be deanonymized before use");
}
}, false, true, true);
}, false, true, false, true);
}

@SuppressWarnings("unchecked")
public static <E extends Enum> ParameterType<E> fromEnum(final Class<E> enumClass) {
Enum[] enumConstants = enumClass.getEnumConstants();
StringBuilder regexpBuilder = new StringBuilder();
Expand All @@ -51,16 +53,11 @@ public static <E extends Enum> ParameterType<E> fromEnum(final Class<E> enumClas
enumClass.getSimpleName(),
regexpBuilder.toString(),
enumClass,
new Transformer<E>() {
@Override
public E transform(String arg) {
return (E) Enum.valueOf(enumClass, arg);
}
}
(String arg) -> (E) Enum.valueOf(enumClass, arg)
);
}

private ParameterType(String name, List<String> regexps, Type type, CaptureGroupTransformer<T> transformer, boolean useForSnippets, boolean preferForRegexpMatch, boolean anonymous) {
private ParameterType(String name, List<String> regexps, Type type, CaptureGroupTransformer<T> transformer, boolean useForSnippets, boolean preferForRegexpMatch, boolean useRegexpMatchAsStrongTypeHint, boolean anonymous) {
if (regexps == null) throw new NullPointerException("regexps cannot be null");
if (type == null) throw new NullPointerException("type cannot be null");
if (transformer == null) throw new NullPointerException("transformer cannot be null");
Expand All @@ -72,10 +69,31 @@ private ParameterType(String name, List<String> regexps, Type type, CaptureGroup
this.useForSnippets = useForSnippets;
this.preferForRegexpMatch = preferForRegexpMatch;
this.anonymous = anonymous;
this.useRegexpMatchAsStrongTypeHint = useRegexpMatchAsStrongTypeHint;
}

public ParameterType(String name, List<String> regexps, Type type, CaptureGroupTransformer<T> transformer, boolean useForSnippets, boolean preferForRegexpMatch, boolean useRegexpMatchAsStrongTypeHint) {
this(name, regexps, type, transformer, useForSnippets, preferForRegexpMatch, useRegexpMatchAsStrongTypeHint, false);
}

public ParameterType(String name, List<String> regexps, Type type, CaptureGroupTransformer<T> transformer, boolean useForSnippets, boolean preferForRegexpMatch) {
this(name, regexps, type, transformer, useForSnippets, preferForRegexpMatch, false);
// Unless explicitly set useRegexpMatchAsStrongTypeHint is true.
//
// 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.
this(name, regexps, type, transformer, useForSnippets, preferForRegexpMatch, true);
}

public ParameterType(String name, List<String> regexps, Class<T> type, CaptureGroupTransformer<T> transformer, boolean useForSnippets, boolean preferForRegexpMatch) {
Expand All @@ -94,14 +112,26 @@ public ParameterType(String name, String regexp, Class<T> type, CaptureGroupTran
this(name, singletonList(regexp), type, transformer, true, false);
}

public ParameterType(String name, List<String> regexps, Type type, Transformer<T> transformer, boolean useForSnippets, boolean preferForRegexpMatch, boolean useRegexpMatchAsStrongTypeHint) {
this(name, regexps, type, new TransformerAdaptor<>(transformer), useForSnippets, preferForRegexpMatch, useRegexpMatchAsStrongTypeHint);
}

public ParameterType(String name, List<String> regexps, Type type, Transformer<T> transformer, boolean useForSnippets, boolean preferForRegexpMatch) {
this(name, regexps, type, new TransformerAdaptor<>(transformer), useForSnippets, preferForRegexpMatch);
}

public ParameterType(String name, List<String> regexps, Class<T> type, Transformer<T> transformer, boolean useForSnippets, boolean preferForRegexpMatch, boolean useRegexpMatchAsStrongTypeHint) {
this(name, regexps, (Type) type, transformer, useForSnippets, preferForRegexpMatch, useRegexpMatchAsStrongTypeHint);
}

public ParameterType(String name, List<String> regexps, Class<T> type, Transformer<T> transformer, boolean useForSnippets, boolean preferForRegexpMatch) {
this(name, regexps, (Type) type, transformer, useForSnippets, preferForRegexpMatch);
}

public ParameterType(String name, String regexp, Class<T> type, Transformer<T> transformer, boolean useForSnippets, boolean preferForRegexpMatch, boolean useRegexpMatchAsStrongTypeHint) {
this(name, singletonList(regexp), type, transformer, useForSnippets, preferForRegexpMatch, useRegexpMatchAsStrongTypeHint);
}

public ParameterType(String name, String regexp, Class<T> type, Transformer<T> transformer, boolean useForSnippets, boolean preferForRegexpMatch) {
this(name, singletonList(regexp), type, transformer, useForSnippets, preferForRegexpMatch);
}
Expand Down Expand Up @@ -152,7 +182,7 @@ public boolean preferForRegexpMatch() {
}

/**
* Indicates whether or not this is a parameter type that should be used for generating
* Indicates whether or not this is a parameter type should be used for generating
* {@link GeneratedExpression}s from text. Typically, parameter types with greedy regexps
* should return false.
*
Expand All @@ -166,8 +196,22 @@ boolean isAnonymous() {
return anonymous;
}

/**
* Indicates whether or not this parameter provides a strong type hint when considering a
* regular expression match. If so, the type hint provided by the method arguments will be
* ignored. If not, when both type hints are in agreement, this parameter types transformer
* will be used. Otherwise parameter transformation for a regular expression match will be
* handled by {@link ParameterTypeRegistry#getDefaultParameterTransformer()}.
*
* @return true if this parameter type provides a type hint when considering a regular
* expression match
*/
public boolean useRegexpMatchAsStrongTypeHint() {
return useRegexpMatchAsStrongTypeHint;
}

ParameterType<Object> deAnonymize(Type type, Transformer<Object> transformer) {
return new ParameterType<>("anonymous", regexps, type, new TransformerAdaptor<>(transformer), useForSnippets, preferForRegexpMatch, anonymous);
return new ParameterType<>("anonymous", regexps, type, new TransformerAdaptor<>(transformer), useForSnippets, preferForRegexpMatch, useRegexpMatchAsStrongTypeHint, anonymous);
}

T transform(List<String> groupValues) {
Expand Down Expand Up @@ -204,7 +248,7 @@ public int compareTo(ParameterType<?> o) {
}

public int weight() {
if(this.type.equals(Integer.class) || this.type.equals(Integer.TYPE)) {
if (this.type.equals(Integer.class) || this.type.equals(Integer.TYPE)) {
return 1000;
}
return 0;
Expand Down
20 changes: 10 additions & 10 deletions ...expressions/java/src/main/java/io/cucumber/cucumberexpressions/ParameterTypeRegistry.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,55 +48,55 @@ private ParameterTypeRegistry(ParameterByTypeTransformer defaultParameterTransfo
public BigInteger transform(String arg) throws Throwable {
return (BigInteger) internalParameterTransformer.transform(arg, BigInteger.class);
}
}, false, false));
}, false, false, false));
defineParameterType(new ParameterType<>("bigdecimal", FLOAT_REGEXPS, BigDecimal.class, new Transformer<BigDecimal>() {
@Override
public BigDecimal transform(String arg) throws Throwable {
return (BigDecimal) internalParameterTransformer.transform(arg, BigDecimal.class);
}
}, false, false));
}, false, false, false));
defineParameterType(new ParameterType<>("byte", INTEGER_REGEXPS, Byte.class, new Transformer<Byte>() {
@Override
public Byte transform(String arg) throws Throwable {
return (Byte) internalParameterTransformer.transform(arg, Byte.class);
}
}, false, false));
}, false, false, false));
defineParameterType(new ParameterType<>("short", INTEGER_REGEXPS, Short.class, new Transformer<Short>() {
@Override
public Short transform(String arg) throws Throwable {
return (Short) internalParameterTransformer.transform(arg, Short.class);
}
}, false, false));
}, false, false, false));
defineParameterType(new ParameterType<>("int", INTEGER_REGEXPS, Integer.class, new Transformer<Integer>() {
@Override
public Integer transform(String arg) throws Throwable {
return (Integer) internalParameterTransformer.transform(arg, Integer.class);
}
}, true, true));
}, true, true, false));
defineParameterType(new ParameterType<>("long", INTEGER_REGEXPS, Long.class, new Transformer<Long>() {
@Override
public Long transform(String arg) throws Throwable {
return (Long) internalParameterTransformer.transform(arg, Long.class);
}
}, false, false));
}, false, false, false));
defineParameterType(new ParameterType<>("float", FLOAT_REGEXPS, Float.class, new Transformer<Float>() {
@Override
public Float transform(String arg) throws Throwable {
return (Float) internalParameterTransformer.transform(arg, Float.class);
}
}, false, false));
}, false, false, false));
defineParameterType(new ParameterType<>("double", FLOAT_REGEXPS, Double.class, new Transformer<Double>() {
@Override
public Double transform(String arg) throws Throwable {
return (Double) internalParameterTransformer.transform(arg, Double.class);
}
}, true, true));
}, true, true, false));
defineParameterType(new ParameterType<>("word", WORD_REGEXPS, String.class, new Transformer<String>() {
@Override
public String transform(String arg) throws Throwable {
return (String) internalParameterTransformer.transform(arg, String.class);
}
}, false, false));
}, false, false, false));
defineParameterType(new ParameterType<>("string", STRING_REGEXPS, String.class, new Transformer<String>() {
@Override
public String transform(String arg) throws Throwable {
Expand All @@ -105,7 +105,7 @@ public String transform(String arg) throws Throwable {
.replaceAll("\\\\'", "'"),
String.class);
}
}, true, false));
}, true, false, false));

defineParameterType(createAnonymousParameterType(ANONYMOUS_REGEX));
}
Expand Down
14 changes: 13 additions & 1 deletion ...ber-expressions/java/src/main/java/io/cucumber/cucumberexpressions/RegularExpression.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,10 +36,22 @@ public List<Argument<?>> match(String text, Type... typeHints) {
int typeHintIndex = 0;
for (GroupBuilder groupBuilder : treeRegexp.getGroupBuilder().getChildren()) {
final String parameterTypeRegexp = groupBuilder.getSource();
final Type typeHint = typeHintIndex < typeHints.length ? typeHints[typeHintIndex++] : String.class;
boolean hasTypeHint = typeHintIndex < typeHints.length;
final Type typeHint = hasTypeHint ? typeHints[typeHintIndex++] : String.class;

ParameterType<?> parameterType = parameterTypeRegistry.lookupByRegexp(parameterTypeRegexp, expressionRegexp, text);

// 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.
if (parameterType != null && hasTypeHint && !parameterType.useRegexpMatchAsStrongTypeHint()) {
if (!parameterType.getType().equals(typeHint)) {
parameterType = null;
}
}

if (parameterType == null) {
parameterType = createAnonymousParameterType(parameterTypeRegexp);
}
Expand Down
46 changes: 46 additions & 0 deletions ...expressions/java/src/test/java/io/cucumber/cucumberexpressions/RegularExpressionTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,19 @@ public void matches_positive_int() {
assertEquals(singletonList(22), match);
}

@Test
public void matches_positive_int_with_hint() {
List<?> match = match(compile("(\\d+)"), "22", Integer.class);
assertEquals(singletonList(22), match);
}

@Test
public void matches_positive_int_with_conflicting_type_hint() {
List<?> match = match(compile("(\\d+)"), "22", String.class);
assertEquals(singletonList("22"), match);
}


@Test
public void matches_nested_capture_group_without_match() {
List<?> match = match(compile("^a user( named \"([^\"]*)\")?$"), "a user");
Expand Down Expand Up @@ -127,6 +140,39 @@ public String transform(String s) {
assertEquals(asList("\" AND QUOTE \""), match);
}

@Test
public void ignores_type_hint_when_parameter_type_has_strong_type_hint() {
parameterTypeRegistry.defineParameterType(new ParameterType<>(
"test",
"one|two|three",
Integer.class,
new Transformer<Integer>() {
@Override
public Integer transform(String s) {
return 42;
}
}, false, false, true
));
assertEquals(asList(42), match(compile("(one|two|three)"), "one", String.class));
}


@Test
public void follows_type_hint_when_parameter_type_does_not_have_strong_type_hint() {
parameterTypeRegistry.defineParameterType(new ParameterType<>(
"test",
"one|two|three",
Integer.class,
new Transformer<Integer>() {
@Override
public Integer transform(String s) {
return 42;
}
}, false, false, false
));
assertEquals(asList("one"), match(compile("(one|two|three)"), "one", String.class));
}

@Test
public void matches_anonymous_parameter_type_with_hint() {
assertEquals(singletonList(0.22f), match(compile("(.*)"), "0.22", Float.class));
Expand Down

0 comments on commit f90958d

Please sign in to comment.