[Java] Fix duplicate step defintion when using interfaces

[jkronegg]

🤔 What's changed?

Corrected wrong exception saying there is a duplicate step definition

⚡️ What's your motivation?

Fixes #2757

🏷️ What kind of change is this?

• 🐛 Bug fix (non-breaking change which fixes a defect)

♻️ Anything particular you want feedback on?

Tested in TDD mode (the additional test was first red, then green after the code correction).
Tested on a real project using the snapshot version.

📋 Checklist:

• I agree to respect and uphold the Cucumber Community Code of Conduct
• I've changed the behaviour of the code
  • I have added/updated tests to cover my changes.
• My change requires a change to the documentation.
  • I have updated the documentation accordingly.
• Users should know about my change
  • I have added an entry to the "Unreleased" section of the CHANGELOG, linking to this pull request.

[codecov]

Codecov Report

Merging #2759 (628b627) into main (b53204e) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##               main    #2759   +/-   ##
=========================================
  Coverage     84.98%   84.98%           
- Complexity     2724     2725    +1     
=========================================
  Files           331      331           
  Lines          9535     9537    +2     
  Branches        913      914    +1     
=========================================
+ Hits           8103     8105    +2     
  Misses         1108     1108           
  Partials        324      324           

Impacted Files	Coverage Δ
.../src/main/java/io/cucumber/java/MethodScanner.java	83.01% <100.00%> (+0.66%)	⬆️

[mpkorstanje]

Thanks for picking this up so quickly!

But I don't think this is the correct solution. Rather MethodScanner.scan should not have produced the duplicate definition.

[mpkorstanje]

It should also not be possible to write:

interface Example {
  
   @Given("a step")
   void step();

}

This should result in a InvalidMethodException. I think it would currently merely be ignored. But I'm not sure.

[jkronegg]

I chose to correct the issue in JavaBackend.loadGlue() to reduce the risk of side effects, but I'll move the correction to MethodScanner.scan.

Regarding you step definition in the interface, I don't understand the point. The unit test is only defining an interface and a class which implements this interface.

public interface StepsInterface {
    Number test();
}
public class StepsWithInterface implements StepsInterface {
    @Given("test")
    public Integer test() {
        return 1;
    }
}

When doing StepsWithInterface.class.getDeclaredMethods(), it will return two methods (both behing annotated by Given("test"):

• Number test()
• Integer test()

[mpkorstanje]

Mmh. Maybe I was a bit to quick with the second example. I'd have to look at the problem a bit more carefully I suppose. I'll do that as soon as I can find some time.

The general idea is that all step definition classes should be concrete and "effectively final".

If they're not it gets really confusing later on because people have trouble figuring out that their duplicate step definitions came from an abstract class or interface that is inherited by two other classes (novices think there is only one because they only wrote one).

Normally extending a class with step definitions causes a validation to fail. And I would have expected that to be the case for this method inherited from the interface too.

So in my mind there are two problems, the one reported and the one where the system (correctly, but only by accident) allows this method to pass validation. I would like to explore a bit around that.

[jkronegg]

The general idea is that all step definition classes should be concrete and "effectively final".

The StepsWithInterface class is concrete and "effectively final" (it's a minimalistic example which reproduces the issue, you don't need to use generics and abstract classes as suggested by @sanflg in the issue). If I write another class which implements the interface and define steps, this will lead to another step definition, without ambiguity, even for beginners.

Of course it's a bit "unusual" to implement an interface when you write a class which contains cucumber step definitions, but I don't see this approach as a problem. While I don't clearly see a situation where implementing an interface gives you an advantage, there is absolutely no issue to allow this in Cucumber.

BTW, I moved the check in the MethodScanner.scan.

[mpkorstanje]

In terms of how the MethodScanner should behave I agree with your description.

In terms of how I understand the implementation I would have behaved, there are a few more things I'd like to check. One being the example I gave. I'll try to find some time for that soon.

[mpkorstanje]

Note to self: I was puzzled why this method appears to be volatile while volatile is not a valid method modifier. The actual type is probably bridge which has the same bit mask value.

https://stackoverflow.com/questions/6651867/why-make-a-method-volatile-in-java

[jkronegg]

Good point (I'll be smarter when I get the "volatile method" question in a future job interview 😅).

Actually, I think we should use the "synthetic" flag rather than the "bridge" flag, because some synthetic methods generated by the compiler are not bridge methods. For example:

public class TestSyntheticAndBridge {
	@Retention(RetentionPolicy.RUNTIME)
	static @interface Annotation {
		
	}
	
	static interface I {
		Number get();
	}
	
	static class B implements I {
		Function<String, Integer> func = s -> s.length();
		
		@Annotation
		public Integer get() {
			return 1;
		}
    }
	public static final void main(String[] args) {
		Stream.of(B.class.getDeclaredMethods()).forEach(method -> 
			System.out.println(method+
					", synthetic="+method.isSynthetic()+
					", bridge="+method.isBridge()+
					", annotations="+method.getAnnotations().length));
	}
}

This displays:

public java.lang.Number TestSyntheticAndBridge$B.get(), synthetic=true, bridge=true, annotations=0
public java.lang.Integer TestSyntheticAndBridge$B.get(), synthetic=false, bridge=false, annotations=1
private static java.lang.Integer TestSyntheticAndBridge$B.lambda$0(java.lang.String), synthetic=true, bridge=false, annotations=0

The lambda method has has been generated for the field "func" and is synthethic but not bridge.
Note that here the method from the interface has no annotation (the test was done with Oracle JVM 10.0.1, while the tests in my previous posts were done using AzulZulu 17). I'll investigate further...

On OpenJdk Azul Zulu 17.0.4 (as well as with OpenJDK 1.8, and OpenJDK 7.0.211 when rewriting the code without streams and functions):

public java.lang.Number TestSyntheticAndBridge$B.get(), synthetic=true, bridge=true, annotations=1
public java.lang.Integer TestSyntheticAndBridge$B.get(), synthetic=false, bridge=false, annotations=1
private static java.lang.Integer TestSyntheticAndBridge$B.lambda$new$0(java.lang.String), synthetic=true, bridge=false, annotations=0

Notice the "annotations=1" on the method coming from the interface. So the root cause is a difference between the JVM implementations. This is maybe why @mpkorstanje could not reproduce the issue.

I found https://bugs.openjdk.org/browse/JDK-6695379 which apparently added this behavior as a feature (I'm not sure this is the cause because OpenJdk 7 has the same behavior and the issue has a fix version 8, but at least this is related).

Similar issue is related here: https://www.premium-minds.com/blog/bug-conspiracy/

[mpkorstanje]

Thanks for the explanation. That does cover most of my concerns and questions.

How do we know that there always is an non-synthetic annotated method?

For example, what happens in Kotlin?

[jkronegg]

How do we know that there always is an non-synthetic annotated method?

We may may have no non-synthetic method, but that should not be an issue (in this case the MethodScanner will process 0 methods).

For Kotlin, I don't have enough knowledge to say...

[mpkorstanje]

I did a really quick manual test. Kotlin is going to be fine. I'm not happy with doing this manually, ideally we had an example project for this but that's all the time I've got right now.

I've narrowed the solution to bridge methods only. The bug report was about a bridge method and the tests we've written are also for bridge methods. If other bugs show up involving synthetics that aren't bridge methods, we can test those individually.

[mpkorstanje]

@jkronegg I think this will do. But do have a look at my changes too.

[jkronegg]

@jkronegg I think this will do. But do have a look at my changes too.

That's fine for me.