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.

Sign up for GitHub

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

Jump to bottom

More easily detect overridden methods & calls to super #1040

Open
wants to merge 20 commits into
base: main
Choose a base branch
from
Open

More easily detect overridden methods & calls to super #1040

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
380958a
Added Method#isOverridden
java-coding-prodigy Jan 6, 2023
e569b0e
Fixed the method and added a test
java-coding-prodigy Jan 7, 2023
9c68442
Merge branch 'main' into main
java-coding-prodigy Jan 7, 2023
9f58651
Added support for methods in grandparent class and used ArchUnit API …
java-coding-prodigy Jan 8, 2023
1903590
Merge branch 'main' of github.com:TNG/ArchUnit
java-coding-prodigy Jan 8, 2023
3e97e3f
Fixed issue with `isOverridden`
java-coding-prodigy Jan 8, 2023
3ea2f36
Added testing for overriden method in grandparent class
java-coding-prodigy Jan 8, 2023
c901839
Merge branch 'main' of github.com:java-coding-prodigy/ArchUnit
java-coding-prodigy Jan 8, 2023
9e74551
Undo accidental copyright change
java-coding-prodigy Jan 8, 2023
c9e63f6
Check for super methods in interfaces as well
java-coding-prodigy Jan 8, 2023
02eae70
Import fix
java-coding-prodigy Jan 8, 2023
e67cb29
Merge branch 'main' into main
java-coding-prodigy Feb 8, 2023
d7c3481
Got this so far
java-coding-prodigy May 17, 2023
2acc5d6
Got this so far
java-coding-prodigy May 17, 2023
9c8a0a2
Merge branch 'main' into main
java-coding-prodigy May 18, 2023
82c8bfa
Merge branch 'main' into main
java-coding-prodigy Jul 25, 2023
1767d88
Merge branch 'main' into main
java-coding-prodigy Oct 6, 2023
b8b74e6
Merge branch 'TNG:main' into main
java-coding-prodigy Oct 15, 2023
c13a74a
Finally passed the test.
java-coding-prodigy Nov 11, 2023
fa2bebc
Merge branch 'main' into main
java-coding-prodigy Nov 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 15 additions & 8 deletions archunit/src/main/java/com/tngtech/archunit/core/domain/JavaMethod.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2014-2022 TNG Technology Consulting GmbH
* Copyright 2014-2023 TNG Technology Consulting GmbH
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. 😅

*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand All @@ -15,20 +15,21 @@
*/
package com.tngtech.archunit.core.domain;

import java.lang.reflect.Method;
import java.util.List;
import java.util.Optional;
import java.util.Set;
import java.util.function.Function;
import java.util.function.Supplier;

import com.tngtech.archunit.PublicAPI;
import com.tngtech.archunit.base.ArchUnitException.InconsistentClassPathException;
import com.tngtech.archunit.base.MayResolveTypesViaReflection;
import com.tngtech.archunit.base.ResolvesTypesViaReflection;
import com.tngtech.archunit.base.Suppliers;
import com.tngtech.archunit.core.importer.DomainBuilders;

import java.lang.reflect.Method;
import java.util.Arrays;
import java.util.List;
import java.util.Optional;
import java.util.Set;
import java.util.function.Function;
import java.util.function.Supplier;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, I'll do so in JavaMethodTest as well

import static com.google.common.collect.Sets.union;
import static com.tngtech.archunit.PublicAPI.Usage.ACCESS;
import static com.tngtech.archunit.core.domain.Formatters.formatMethod;
Expand Down Expand Up @@ -125,6 +126,12 @@ public String getDescription() {
return "Method <" + getFullName() + ">";
}

@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));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. 😉

}

@ResolvesTypesViaReflection
@MayResolveTypesViaReflection(reason = "Just part of a bigger resolution process")
private class ReflectMethodSupplier implements Supplier<Method> {
Expand Down
39 changes: 39 additions & 0 deletions archunit/src/test/java/com/tngtech/archunit/core/domain/JavaMethodTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
package com.tngtech.archunit.core.domain;


import com.tngtech.archunit.core.importer.ClassFileImporter;
import org.junit.Test;

import static com.tngtech.archunit.testutil.Assertions.assertThat;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertNotEquals;

public class JavaMethodTest {
@Test
public void isOverriddenTest() {
class Base {
void method1() {
}

void method1(int x) {
}
}
class Child extends Base {
void method1() {
}

void method2() {
}
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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().

assertEquals(childMethod1.getName(), baseMethod1.getName());
Copy link
Member

@hankem hankem Jan 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

assertThat(childClass.getMethod("method1").isOverridden()).isTrue();
assertThat(childClass.getMethod("method1", int.class).isOverridden()).isFalse();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

assertThat(childClass.getMethod("method2").isOverridden()).isFalse();
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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();
    }