More easily detect overridden methods & calls to super #1040

java-coding-prodigy · 2023-01-06T09:01:55Z

Fixes #982

Currently added a JavaMethod#isOverridden method for now.

hankem · 2023-01-06T10:09:07Z

archunit/src/main/java/com/tngtech/archunit/core/domain/JavaMethod.java

+    @PublicAPI(usage = ACCESS)
+    public boolean isOverridden() {
+        Method method = methodSupplier.get();
+        return Arrays.asList(method.getDeclaringClass().getSuperclass().getDeclaredMethods()).contains(method);


It would be nice to avoid the (expensive) reflection API, but more importantly: Does this even work for you? Please provide a unit test that proves the correctnes of your code.

I'd start with something like this:

public class JavaMethodTest { @Test public void isOverridden() { class Base { void method1() {} void method1(int x) {} } class Child extends Base { void method1() {} void method2() {} } JavaClass childClass = new ClassFileImporter().importClass(Child.class); assertThat(childClass.getMethod("method1").isOverridden()).isTrue(); assertThat(childClass.getMethod("method1", int.class).isOverridden()).isFalse(); assertThat(childClass.getMethod("method2").isOverridden()).isFalse(); }

Your implementation however gives me childClass.getMethod("method1").isOverridden() == false.

java-coding-prodigy · 2023-01-07T11:04:18Z

I couldn't figure out a direct way to get the inherited methods of the super class I tried to use .getMethods() but that only returns the public methods.
If we have something like a class extending a class which also extends another class, and the lowest class has a method that the highest class has, but the middle class does not, then .isOverridden() returns false.
I don't know whether our API or the reflection API has an easy way to fix this or not

hankem

You may have seen that the PR's DCO check fails. Can you please squash your changes and sign it off according to the DCO?

It's also preferable to rebase to the current main instead of merging it in order to create a simple linear history.

hankem · 2023-01-07T16:24:44Z

archunit/src/test/java/com/tngtech/archunit/core/domain/JavaMethodTest.java

+        JavaClass baseClass = new ClassFileImporter().importClass(Base.class);
+        JavaMethod childMethod1 = childClass.getMethod("method1");
+        JavaMethod baseMethod1 = baseClass.getMethod("method1");
+        assertNotEquals(childMethod1, baseMethod1);


This assertion doesn't test what you probably want it to, as the following would also pass:

JavaClass childClass1 = new ClassFileImporter().importClass(Child.class); JavaClass childClass2 = new ClassFileImporter().importClass(Child.class); JavaMethod childMethod1 = childClass1.getMethod("method1"); JavaMethod childMethod2 = childClass2.getMethod("method1"); assertNotEquals(childMethod1, childMethod2);

even though both JavaMethods refer to the same method Child#method1().

hankem · 2023-01-07T16:25:52Z

archunit/src/test/java/com/tngtech/archunit/core/domain/JavaMethodTest.java

+        JavaMethod childMethod1 = childClass.getMethod("method1");
+        JavaMethod baseMethod1 = baseClass.getMethod("method1");
+        assertNotEquals(childMethod1, baseMethod1);
+        assertEquals(childMethod1.getName(), baseMethod1.getName());


I don't think that such an assertion is needed in the isOverriddenTest.

Oops, I used that assertion while I was debugging why my check failed and forgot to remove it, will do so in the next commit.

hankem · 2023-01-07T16:32:08Z

archunit/src/test/java/com/tngtech/archunit/core/domain/JavaMethodTest.java

+        assertNotEquals(childMethod1, baseMethod1);
+        assertEquals(childMethod1.getName(), baseMethod1.getName());
+        assertThat(childClass.getMethod("method1").isOverridden()).isTrue();
+        assertThat(childClass.getMethod("method1", int.class).isOverridden()).isFalse();


Does this work for you? Sorry, I myself had suggested this line in a previous comment (off the top of my head 🙄), but it turns out that we cannot even get the method Base#method1(int) from childClass. When actually executing your test, childClass.getMethod("method1", int.class) gives me:

java.lang.IllegalArgumentException: No code unit with name 'method1' and parameters [int] in codeUnits [JavaMethod{com.tngtech.archunit.core.domain.JavaMethodTest$1Child.method2()}, JavaMethod{com.tngtech.archunit.core.domain.JavaMethodTest$1Child.method1()}] of class com.tngtech.archunit.core.domain.JavaMethodTest$1Child

hankem · 2023-01-07T16:51:32Z

archunit/src/main/java/com/tngtech/archunit/core/domain/JavaMethod.java

+    @PublicAPI(usage = ACCESS)
+    public boolean isOverridden() {
+        Class<?>[] reflectedParameters = reflect(getRawParameterTypes());
+        return Arrays.stream(reflect().getDeclaringClass().getSuperclass().getDeclaredMethods()).anyMatch(superMethod -> superMethod.getName().equals(getName()) && Arrays.equals(superMethod.getParameterTypes(), reflectedParameters));


Do you agree that isOverridden() should support the following test cases?

@Test public void isOverridden_considers_class_hierarchy() { class GrandParent { void method() {} } class Parent extends GrandParent { } class Child extends Parent { void method() {} } JavaClass childClass = new ClassFileImporter().importClass(Child.class); assertThat(childClass.getMethod("method").isOverridden()).isTrue(); } interface Grandma { } interface Grandpa { void method(); } interface Parent extends Grandma, Grandpa { } @Test public void isOverridden_considers_interface_hierarchy() { class Child implements Parent { public void method() {} } JavaClass childClass = new ClassFileImporter().importClass(Child.class); assertThat(childClass.getMethod("method").isOverridden()).isTrue(); }

As you mentioned, your current implementation doesn't.

I couldn't figure out a direct way to get the inherited methods of the super class I tried to use .getMethods() but that only returns the public methods.
If we have something like a class extending a class which also extends another class, and the lowest class has a method that the highest class has, but the middle class does not, then .isOverridden() returns false.
I don't know whether our API or the reflection API has an easy way to fix this or not

Have you seen the OverridesSuperClassMethod predicate in #982? The implementation within ArchUnit's domain model (i.e. without using reflection) is actually almost there already. 😉

…instead of reflection

I was checking if the parameters are equal by using `getParameters().equals()` which did not work as `JavaParameter#equals` checks for equality of the owner(in this case, super method) as well. `JavaType#equals` only checks for equality of the actual parameter type regardless of source, and hence works.

hankem

Nice; I think you're getting close! 👍

About the DCO: I would do

git rebase -i origin/main
replace pick with f for all commits after the first one and do
git commit --amend -s to create the sign-off, followed by
git push -f

hankem · 2023-01-08T09:20:20Z

archunit/src/main/java/com/tngtech/archunit/core/domain/JavaMethod.java

+import java.util.Set;
+import java.util.function.Function;
+import java.util.function.Supplier;
+


Can we please adhere to the import order specified in CONTRIBUTING.md, i.e. keep the java.* imports at the top?

Cool, I'll do so in JavaMethodTest as well

hankem · 2023-01-08T09:25:30Z

archunit/src/main/java/com/tngtech/archunit/core/domain/JavaMethod.java

@@ -125,6 +125,14 @@ public String getDescription() {
        return "Method <" + getFullName() + ">";
    }

+    @PublicAPI(usage = ACCESS)
+    public boolean isOverridden() {
+        return getOwner().getAllRawSuperclasses().stream()


If we want to conisder overridden methods from interfaces as well, we can replace this stream with

Streams.concat(getOwner().getAllRawSuperclasses().stream(), getOwner().getAllRawInterfaces().stream())

hankem · 2023-01-08T09:30:17Z

archunit/src/main/java/com/tngtech/archunit/core/domain/JavaMethod.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2014-2022 TNG Technology Consulting GmbH
+ * Copyright 2014-2023 TNG Technology Consulting GmbH


I guess we should isolate this "Happy new year, code base!" change into a separate commit (that also fixes all java files, which happens automatically when building the project).

Yeah for some reason my IntelliJ or some other plugin automatically made this change for all the classes with this documentation, I forgot to undo the changes on this file itself but did them for all other classes. 😅

hankem

Could you please sign-off your commits (most easily done after rebasing & squashing all into one), as suggested before in #1040 (review)?
Without the DCO check passing, GitHub won't let us merge the PR.

hankem · 2023-01-15T15:34:02Z

archunit/src/main/java/com/tngtech/archunit/core/domain/JavaMethod.java

@@ -125,6 +126,14 @@ public String getDescription() {
        return "Method <" + getFullName() + ">";
    }

+    @PublicAPI(usage = ACCESS)
+    public boolean isOverridden() {
+        return Stream.concat(getOwner().getAllRawSuperclasses().stream(), getOwner().getAllRawInterfaces().stream())


Can we pleae add a small test to cover the feature that overriden methods from interfaces are detected?

hankem · 2023-01-23T08:17:47Z

archunit/src/test/java/com/tngtech/archunit/core/domain/JavaMethodTest.java

+        assertThat(grandChildClass.getMethod("method3").isOverridden()).isFalse();
+        //TODO add testing for methods with generic parameters
+    }
+}


We should also have a test for the use case of #1047 .

I believe that is already in place, currently we check if the parameters and name of the method are equal, and that implies compatibility of return type.
If that is not the case, then the user code will not compile in the first place

I thought that we should document that this works (which it curently does, but we also want to avoid any future regression):

@Test public void more_specific_return_type_is_supported() { class Parent { CharSequence method() { return "base"; } } class Child extends Parent { @Override String method() { return "overridden"; } } JavaClass childClass = new ClassFileImporter().importClass(Child.class); JavaMethod method = childClass.getMethod("method"); assertThat(method.isOverridden()).isTrue(); }

hankem · 2023-02-09T08:03:15Z

archunit/src/test/java/com/tngtech/archunit/core/domain/JavaMethodTest.java

+        assertThat(grandChildClass.getMethod("method1", int.class).isOverridden()).isTrue();
+        assertThat(grandChildClass.getMethod("method2").isOverridden()).isTrue();
+        assertThat(grandChildClass.getMethod("method3").isOverridden()).isFalse();
+        //TODO add testing for methods with generic parameters


It seems that we indeed don't recognize all overriden generic methods:

@Test public void overridden_generic_methods_are_supported() { class Parent<T extends Number> { void method(T t) { } } class Child extends Parent<Integer> { @Override void method(Integer t) { } } JavaClass childClass = new ClassFileImporter().importClass(Child.class); JavaMethod method = childClass.getMethod("method", Integer.class); assertThat(method.isOverridden()).isTrue(); // Expecting value to be true but was false }

Hmmm, this might complicate the code now.
I'll see what I can do.

java-coding-prodigy · 2023-05-17T14:21:29Z

So far I think I know what we can do to fix the issue.
We need to get the supperclasses and interfaces of the method's class but they cannot be the raw ones as they don't have type arguments.
So looks like we can keep on calling a .supperclass() again and again unti we reach java.lang.Object as well as use .interfaces
However this solves the current problem at hand, it may not be bug-free.

Also ignore JavaMethodTest.java that is just me playing around with the objects while debugging.

codecholeric · 2023-10-08T15:39:57Z

@java-coding-prodigy are you still working on this? Do you need any support?

java-coding-prodigy · 2023-10-15T08:44:30Z

@java-coding-prodigy are you still working on this? Do you need any support?

I was working on it some time ago and I could continue now. However I had run into some issues earlier.
I'll see what I can do.

java-coding-prodigy · 2023-11-11T15:39:10Z

Fixed the generic issue, however code is too ugly and slow, we should try to enhance it.
I have only done this for classes, but I presume it would be similar for interfaces too.

java-coding-prodigy · 2023-11-11T15:56:57Z

I'm thinking of adding a JavaMethod#overridenFrom(JavaType and make isOverriden call that method. What do you think @hankem ?

Added Method#isOverridden

380958a

hankem requested changes Jan 6, 2023

View reviewed changes

Fixed the method and added a test

e569b0e

Merge branch 'main' into main

9c68442

hankem requested changes Jan 7, 2023

View reviewed changes

java-coding-prodigy added 5 commits January 8, 2023 13:54

Added support for methods in grandparent class and used ArchUnit API …

9f58651

…instead of reflection

Merge branch 'main' of github.com:TNG/ArchUnit

1903590

Added testing for overriden method in grandparent class

3ea2f36

Merge branch 'main' of github.com:java-coding-prodigy/ArchUnit

c901839

hankem reviewed Jan 8, 2023

View reviewed changes

java-coding-prodigy added 3 commits January 8, 2023 20:09

Undo accidental copyright change

9e74551

Check for super methods in interfaces as well

c9e63f6

Import fix

02eae70

java-coding-prodigy requested a review from hankem January 10, 2023 07:37

hankem reviewed Jan 15, 2023

View reviewed changes

hankem reviewed Jan 23, 2023

View reviewed changes

Merge branch 'main' into main

e67cb29

hankem reviewed Feb 9, 2023

View reviewed changes

hankem mentioned this pull request Apr 19, 2023

How do I execute the number check per JavaClass? #1103

Open

java-coding-prodigy added 2 commits May 17, 2023 19:42

Got this so far

d7c3481

Got this so far

2acc5d6

java-coding-prodigy added 3 commits May 18, 2023 16:16

Merge branch 'main' into main

9c8a0a2

Merge branch 'main' into main

82c8bfa

Merge branch 'main' into main

1767d88

Merge branch 'TNG:main' into main

b8b74e6

java-coding-prodigy and others added 2 commits November 11, 2023 21:02

Finally passed the test.

c13a74a

Merge branch 'main' into main

fa2bebc

java-coding-prodigy requested a review from hankem November 12, 2023 04:56

rweisleder mentioned this pull request Mar 23, 2024

Add possibility to check for overriding methods to domain and lang #1200

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

More easily detect overridden methods & calls to super #1040

More easily detect overridden methods & calls to super #1040

java-coding-prodigy commented Jan 6, 2023

hankem Jan 6, 2023 •

edited

Loading

java-coding-prodigy commented Jan 7, 2023

hankem left a comment

hankem Jan 7, 2023

hankem Jan 7, 2023 •

edited

Loading

java-coding-prodigy Jan 8, 2023

hankem Jan 7, 2023

hankem Jan 7, 2023

hankem left a comment

hankem Jan 8, 2023

java-coding-prodigy Jan 8, 2023

hankem Jan 8, 2023

hankem Jan 8, 2023

java-coding-prodigy Jan 8, 2023

hankem left a comment

hankem Jan 15, 2023

hankem Jan 23, 2023

java-coding-prodigy Feb 8, 2023

hankem Feb 9, 2023

hankem Feb 9, 2023

java-coding-prodigy Feb 12, 2023

java-coding-prodigy commented May 17, 2023

codecholeric commented Oct 8, 2023

java-coding-prodigy commented Oct 15, 2023

java-coding-prodigy commented Nov 11, 2023

java-coding-prodigy commented Nov 11, 2023

More easily detect overridden methods & calls to super #1040

Are you sure you want to change the base?

More easily detect overridden methods & calls to super #1040

Conversation

java-coding-prodigy commented Jan 6, 2023

hankem Jan 6, 2023 • edited Loading

Choose a reason for hiding this comment

java-coding-prodigy commented Jan 7, 2023

hankem left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hankem Jan 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hankem left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hankem left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

java-coding-prodigy commented May 17, 2023

codecholeric commented Oct 8, 2023

java-coding-prodigy commented Oct 15, 2023

java-coding-prodigy commented Nov 11, 2023

java-coding-prodigy commented Nov 11, 2023

hankem Jan 6, 2023 •

edited

Loading

hankem Jan 7, 2023 •

edited

Loading