Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,10 @@ public static void completeClassHierarchy(JavaClass javaClass, ImportContext imp
javaClass.completeClassHierarchyFrom(importContext);
}

public static void completePackage(JavaClass javaClass, ImportContext importContext) {
javaClass.completePackageInfoFrom(importContext);
}

public static void completeEnclosingDeclaration(JavaClass javaClass, ImportContext importContext) {
javaClass.completeEnclosingDeclarationFrom(importContext);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
package com.tngtech.archunit.core.domain;

import java.lang.annotation.Annotation;
import java.util.Arrays;
import java.util.Collections;
import java.util.HashSet;
import java.util.List;
Expand Down Expand Up @@ -1429,6 +1430,13 @@ void completeClassHierarchyFrom(ImportContext context) {
completionProcess.markClassHierarchyComplete();
}

void completePackageInfoFrom(ImportContext context) {
this.javaPackage = JavaPackage.from(Arrays.asList(
Comment thread
StefanGraeber marked this conversation as resolved.
this,
context.resolveClass(this.getPackageName() + ".package-info")));
completionProcess.markPackageComplete();
}

private void completeSuperclassFrom(ImportContext context) {
Optional<JavaClass> rawSuperclass = context.createSuperclass(this);
if (rawSuperclass.isPresent()) {
Expand Down Expand Up @@ -1663,6 +1671,10 @@ public void markAnnotationsComplete() {
@Override
public void markDependenciesComplete() {
}

@Override
public void markPackageComplete() {
}
};

abstract boolean hasFinished();
Expand All @@ -1683,6 +1695,8 @@ public void markDependenciesComplete() {

public abstract void markDependenciesComplete();

public abstract void markPackageComplete();

static CompletionProcess start() {
return new FullCompletionProcess();
}
Expand All @@ -1701,6 +1715,7 @@ private static class FullCompletionProcess extends CompletionProcess {
private boolean membersComplete = false;
private boolean annotationsComplete = false;
private boolean dependenciesComplete = false;
private boolean packageComplete = false;

@Override
boolean hasFinished() {
Expand All @@ -1711,7 +1726,8 @@ boolean hasFinished() {
&& genericInterfacesComplete
&& membersComplete
&& annotationsComplete
&& dependenciesComplete;
&& dependenciesComplete
&& packageComplete;
}

@Override
Expand Down Expand Up @@ -1753,6 +1769,11 @@ public void markAnnotationsComplete() {
public void markDependenciesComplete() {
this.dependenciesComplete = true;
}

@Override
public void markPackageComplete() {
this.packageComplete = true;
}
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,15 @@ public void onNewClass(String className, Optional<String> superclassName, List<S
}
importRecord.addInterfaces(ownerName, interfaceNames);
dependencyResolutionProcess.registerSupertypes(interfaceNames);
}
if(!className.endsWith(".package-info")) {
if(className.contains(".")) {
String packageName = className.substring(0, className.lastIndexOf("."));
dependencyResolutionProcess.registerPackageInfo(packageName + ".package-info");
} else {
dependencyResolutionProcess.registerPackageInfo("package-info");
}
}
}

@Override
public void onDeclaredTypeParameters(JavaClassTypeParametersBuilder typeParametersBuilder) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@
import static com.tngtech.archunit.core.domain.DomainObjectCreationContext.completeGenericInterfaces;
import static com.tngtech.archunit.core.domain.DomainObjectCreationContext.completeGenericSuperclass;
import static com.tngtech.archunit.core.domain.DomainObjectCreationContext.completeMembers;
import static com.tngtech.archunit.core.domain.DomainObjectCreationContext.completePackage;
import static com.tngtech.archunit.core.domain.DomainObjectCreationContext.completeTypeParameters;
import static com.tngtech.archunit.core.domain.DomainObjectCreationContext.createInstanceofCheck;
import static com.tngtech.archunit.core.domain.DomainObjectCreationContext.createJavaClasses;
Expand Down Expand Up @@ -119,6 +120,7 @@ private void completeClasses() {
completeGenericInterfaces(javaClass, this);
completeMembers(javaClass, this);
completeAnnotations(javaClass, this);
completePackage(javaClass, this);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,11 @@ class DependencyResolutionProcess {
private final int maxRunsForGenericSignatureTypes = getConfiguredIterations(
MAX_ITERATIONS_FOR_GENERIC_SIGNATURE_TYPES_PROPERTY_NAME, MAX_ITERATIONS_FOR_GENERIC_SIGNATURE_TYPES_DEFAULT_VALUE);

static final String MAX_ITERATIONS_FOR_PACKAGE_INFO_PROPERTY_NAME = "maxIterationsForPackageInfo";
static final int MAX_ITERATIONS_FOR_PACKAGE_INFO_DEFAULT_VALUE = -1;
private final int maxRunsForPackageInfo = getConfiguredIterations(
MAX_ITERATIONS_FOR_PACKAGE_INFO_PROPERTY_NAME, MAX_ITERATIONS_FOR_PACKAGE_INFO_DEFAULT_VALUE);

private Set<String> currentTypeNames = new HashSet<>();
private int runNumber = 1;
private boolean shouldContinue;
Expand Down Expand Up @@ -116,6 +121,12 @@ void registerGenericSignatureType(String typeName) {
}
}

void registerPackageInfo(String typeName) {
if (runNumberHasNotExceeded(maxRunsForPackageInfo)) {
currentTypeNames.add(typeName);
}
}

void resolve(ImportedClasses classes) {
logConfiguration();
do {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import java.util.Map;
import java.util.Optional;
import java.util.function.Function;
import java.util.stream.Collectors;

import com.google.common.collect.HashMultimap;
import com.google.common.collect.ImmutableSet;
Expand Down Expand Up @@ -121,7 +122,12 @@ public static <T> MetricsComponents<T> from(Collection<T> elements, Function<? s
public static MetricsComponents<JavaClass> fromPackages(Collection<JavaPackage> packages) {
ImmutableSet.Builder<MetricsComponent<JavaClass>> components = ImmutableSet.builder();
for (JavaPackage javaPackage : packages) {
components.add(MetricsComponent.of(javaPackage.getName(), javaPackage.getClassesInPackageTree()));
components.add(MetricsComponent.of(
javaPackage.getName(),
javaPackage.getClassesInPackageTree().stream()
.filter(jc -> !jc.getSimpleName().equals("package-info"))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sorry for opening a new frontier, but I needed a fresh review from the start after a month...

this is a behavior change in case of having a package-info file.

please look at MetricsComponentsTest.creates_components_from_Java_packages and make explicit that the package-info is not imported (you need to create one in the test scenario, probably best in case 1 for top-level and subpackage.

adding the package-info might also require changes to the the other test methods. it would be interesting to see whether creates_components_from_Java_classes stick with 3 components.

I hope this doesn't break too much for our users becasue the old behavior is unexpected for me and I haven't seen MetricsCompment before

.collect(Collectors.toList())
));
}
return MetricsComponents.of(components.build());
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@
import com.tngtech.archunit.base.ArchUnitException.InvalidSyntaxUsageException;
import com.tngtech.archunit.base.DescribedPredicate;
import com.tngtech.archunit.base.HasDescription;
import com.tngtech.archunit.core.domain.packageexamples.annotated.PackageLevelAnnotation;
import com.tngtech.archunit.core.domain.packageexamples.annotated.WithinAnnotatedPackage;
import com.tngtech.archunit.core.domain.testobjects.AAccessingB;
import com.tngtech.archunit.core.domain.testobjects.AExtendingSuperAImplementingInterfaceForA;
import com.tngtech.archunit.core.domain.testobjects.AReferencingB;
Expand Down Expand Up @@ -1593,6 +1595,13 @@ public void function_getPackage() {
assertThat(JavaClass.Functions.GET_PACKAGE_NAME.apply(javaClass))
.as("result of GET_PACKAGE_NAME(clazz)")
.isEqualTo(javaClass.getPackageName());

JavaClass javaClassInAnnotatedPackage = importClassWithContext(WithinAnnotatedPackage.class);

assertThat(JavaClass.Functions.GET_PACKAGE.apply(javaClassInAnnotatedPackage)
.isAnnotatedWith(PackageLevelAnnotation.class))
.as("package info is available")
.isTrue();
}

@Test
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
import java.util.List;
import java.util.concurrent.BlockingQueue;
import java.util.function.Predicate;
import java.util.stream.Collectors;

import com.tngtech.archunit.base.DescribedPredicate;
import com.tngtech.archunit.core.domain.packageexamples.annotated.PackageLevelAnnotation;
Expand Down Expand Up @@ -423,6 +424,15 @@ public void test_getPackageInfo() {
JavaPackage nonAnnotatedPackage = importPackage("packageexamples");

assertThat(annotatedPackage.getPackageInfo()).isNotNull();
assertThat(annotatedPackage.getClasses())
.hasSize(3);
assertThat(annotatedPackage.getClasses().stream().map(JavaClass::getFullName).collect(Collectors.toList()))
.as("all classes and package-info are loaded as JavaClass")
.containsExactlyInAnyOrder(
"com.tngtech.archunit.core.domain.packageexamples.annotated.package-info",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

should we really get the package-info in the classes?
I wouldn't expect that as user and don't really like having it as the package info is not really a class.

I just wanted to write that you can't get it via reflection as Class.forName("com.tngtech.archunit.core.domain.packageexamples.annotated.package-info") but you actually get it as an interface... there is always sth to learn about java...

What are your thoughts on including the package-info class in the classes of the package?
I have the feeling that it was like this before, but I don't like it.

What about a missing package-info file? I just checked that it is NOT included in that case. please extend the
test, I write a separate comment f0fca6f#r3332408963 for it

related/caused by to f0fca6f#r3136501904

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I also just checked how the libraries Reflections and ClassGraph handle this:
Reflections includes it if you look for all subclasses of Object and disable the filterr preventing obejct sublcasses like this:

        Reflections reflections = new Reflections(
                new ConfigurationBuilder()
                        .forPackages("com.tngtech.archunit.core.domain.packageexamples.annotated")
                        .setScanners(Scanners.SubTypes.filterResultsBy(c -> true)) // disable the Object filter
        );
        Set<Class<?>> subTypesOfObject = reflections.getSubTypesOf(Object.class);

which is quite hacky.
ClassGraph doesn't include it at all but provides special methods for this

"com.tngtech.archunit.core.domain.packageexamples.annotated.PackageLevelAnnotation",
"com.tngtech.archunit.core.domain.packageexamples.annotated.WithinAnnotatedPackage"
);

assertThatThrownBy(nonAnnotatedPackage::getPackageInfo)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

created by f0fca6f#r3332389020

I'd prefer to exchange the tested package by one with some classes in it (e.g. packageexamples.first) and asserting that those 2 classes but not the package-info are returned from getClasses()

.isInstanceOf(IllegalArgumentException.class)
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
package com.tngtech.archunit.core.domain.packageexamples.annotated;

public class WithinAnnotatedPackage {
}
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import java.util.ArrayList;
import java.util.HashMap;
import java.util.List;
import java.util.Optional;
import java.util.Map;
import java.util.function.Supplier;

Expand All @@ -22,17 +23,20 @@
import com.tngtech.archunit.core.domain.JavaConstructorCall;
import com.tngtech.archunit.core.domain.JavaConstructorReference;
import com.tngtech.archunit.core.domain.JavaEnumConstant;
import com.tngtech.archunit.core.domain.JavaField;
import com.tngtech.archunit.core.domain.JavaFieldAccess;
import com.tngtech.archunit.core.domain.JavaMethod;
import com.tngtech.archunit.core.domain.JavaMethodCall;
import com.tngtech.archunit.core.domain.JavaMethodReference;
import com.tngtech.archunit.core.domain.JavaPackage;
import com.tngtech.archunit.core.domain.JavaParameterizedType;
import com.tngtech.archunit.core.domain.JavaType;
import com.tngtech.archunit.core.domain.JavaWildcardType;
import com.tngtech.archunit.core.domain.ReferencedClassObject;
import com.tngtech.archunit.core.domain.ThrowsDeclaration;
import com.tngtech.archunit.core.domain.properties.HasAnnotations;
import com.tngtech.archunit.core.importer.DependencyResolutionProcessTestUtils.ImporterWithAdjustedResolutionRuns;
import com.tngtech.archunit.core.importer.testexamples.OtherClass;
import com.tngtech.archunit.core.importer.testexamples.SomeAnnotation;
import com.tngtech.archunit.core.importer.testexamples.annotatedclassimport.ClassWithUnimportedAnnotation;
import com.tngtech.archunit.core.importer.testexamples.annotatedparameters.ClassWithMethodWithAnnotatedParameters;
Expand All @@ -44,9 +48,11 @@
import com.tngtech.archunit.core.importer.testexamples.annotationresolution.SomeAnnotationWithAnnotationParameter;
import com.tngtech.archunit.core.importer.testexamples.annotationresolution.SomeAnnotationWithClassParameter;
import com.tngtech.archunit.core.importer.testexamples.classhierarchyresolution.Child;
import com.tngtech.archunit.core.importer.testexamples.packageinforesolution.ClassThatReferencesOtherClass;
import com.tngtech.java.junit.dataprovider.DataProvider;
import com.tngtech.java.junit.dataprovider.DataProviderRunner;
import com.tngtech.java.junit.dataprovider.UseDataProvider;
import org.assertj.core.api.Assertions;
import org.junit.Test;
import org.junit.runner.RunWith;

Expand All @@ -60,6 +66,7 @@
import static com.tngtech.archunit.core.importer.DependencyResolutionProcess.MAX_ITERATIONS_FOR_GENERIC_SIGNATURE_TYPES_PROPERTY_NAME;
import static com.tngtech.archunit.core.importer.DependencyResolutionProcess.MAX_ITERATIONS_FOR_MEMBER_TYPES_PROPERTY_NAME;
import static com.tngtech.archunit.core.importer.DependencyResolutionProcess.MAX_ITERATIONS_FOR_SUPERTYPES_PROPERTY_NAME;
import static com.tngtech.archunit.core.importer.DependencyResolutionProcess.MAX_ITERATIONS_FOR_PACKAGE_INFO_PROPERTY_NAME;
import static com.tngtech.archunit.core.importer.testexamples.SomeEnum.OTHER_VALUE;
import static com.tngtech.archunit.core.importer.testexamples.SomeEnum.SOME_VALUE;
import static com.tngtech.archunit.core.importer.testexamples.annotatedparameters.ClassWithMethodWithAnnotatedParameters.methodWithOneAnnotatedParameterWithTwoAnnotations;
Expand Down Expand Up @@ -621,6 +628,33 @@ class Innermost {
assertThatType(outermost).matches(Outermost.class);
}

@Test
public void automatically_resolves_packages() {
JavaClass otherClass = new ClassFileImporter()
.importClass(OtherClass.class);

JavaPackage checkedClassPackageInfo = otherClass.getPackage();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The JavaPackage has the methods tryGetPackageInfo and getPackageInfo.
please include in the test they work as well.
as one calls the other, I would prefer to tests sth like assertThat(checkedClassPackageInfo.getPackageInfo()).isPresent(), even thought this is kind of redundant with the getAnnotations method which also relies on that field.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Looks like these exist in JavaPackageTest:

    @Test
    public void test_getPackageInfo() {
        JavaPackage annotatedPackage = importPackage("packageexamples.annotated");
        JavaPackage nonAnnotatedPackage = importPackage("packageexamples");

        assertThat(annotatedPackage.getPackageInfo()).isNotNull();

        assertThatThrownBy(nonAnnotatedPackage::getPackageInfo)
                .isInstanceOf(IllegalArgumentException.class)
                .hasMessageContaining(nonAnnotatedPackage.getDescription() + " does not contain a package-info.java");
    }

    @Test
    public void test_tryGetPackageInfo() {
        JavaPackage annotatedPackage = importPackage("packageexamples.annotated");
        JavaPackage nonAnnotatedPackage = importPackage("packageexamples");

        assertThat(annotatedPackage.tryGetPackageInfo()).isPresent();
        assertThat(nonAnnotatedPackage.tryGetPackageInfo()).isEmpty();
    }

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

yes, but since you re-create the JavaPackage, it might be worth checking that the re-created package behaves as expected in these methods

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

still open, please add an assertion like assertThat(checkedClassPackageInfo.getPackageInfo()).isPresent() to make explicit that the package-info is loaded.
Asserting it before the annotations will likely give better error messages if it ever breaks

assertThat(checkedClassPackageInfo.getAnnotations()).hasSize(1);
assertThat(checkedClassPackageInfo.isAnnotatedWith(SomeAnnotation.class)).isTrue();
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

When running checkedClassPackageInfo.getClasses(), this set now includes the package-info, which it didn't before.
I don't think that that class should be included in that Set as it is not a real class and shall instead be accessed via JavaPackage.getPackageInfo() which returns the JavaClass representing package-info as HasAnnotations. This change in type seems deliberate to me as this special class doesn't have all features of a JavaClass.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is already the normal behavior in scanning a package. Ill add this assertion to JavaPackageTest.test_getPackageInfo() to show this:

        assertThat(annotatedPackage.getClasses().stream().map(JavaClass::getFullName).collect(Collectors.toList()))
                .as("all classes and package-info are loaded as JavaClass")
                .containsExactlyInAnyOrder(
                        "com.tngtech.archunit.core.domain.packageexamples.annotated.package-info",
                        "com.tngtech.archunit.core.domain.packageexamples.annotated.PackageLevelAnnotation",
                        "com.tngtech.archunit.core.domain.packageexamples.annotated.WithinAnnotatedPackage"
                );

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

how about packages without a package-info file?
IIRC you added somewhere a stub of it as JavaClass. If I don't have the package-info as explicit file in the package, I find it even weirder to have it returned as PackageInfo.
That is was like that before might not have been desired.
As you know, I'm new to the ArchUnit dev side (so far I only used it), but I find it surprising that this pseudo class is present at all in the result of getClasses() and even more surprising that it is now present even though I haven't even written that class.

I would prefer to never have package-info in this result

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

related to f0fca6f#r3332389020


@Test
public void automatically_resolves_dependency_packages() {
JavaClass checkedClass = ImporterWithAdjustedResolutionRuns
.disableAllIterationsExcept(MAX_ITERATIONS_FOR_PACKAGE_INFO_PROPERTY_NAME, MAX_ITERATIONS_FOR_MEMBER_TYPES_PROPERTY_NAME)
.importClass(ClassThatReferencesOtherClass.class);

Optional<JavaField> firstField = checkedClass.getAllFields().stream().findFirst();
assertThat(firstField).isPresent();
JavaClass otherClass = firstField.get().getRawType();
assertThat(otherClass).isFullyImported(true);

JavaPackage otherClassPackageInfo = otherClass.getPackage();

Assertions.assertThat(otherClassPackageInfo.getAnnotations()).hasSize(1);
Assertions.assertThat(otherClassPackageInfo.isAnnotatedWith(SomeAnnotation.class)).isTrue();
}

@DataProvider
public static Object[][] data_automatically_resolves_generic_type_parameter_bounds() {
@SuppressWarnings("unused")
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
package com.tngtech.archunit.core.importer.testexamples.packageinforesolution;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't understand this new package name.
is it important that it is subpackage of the package in which OtherClass is defined or shall just be any other package?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It is important that this class is in a different package than OtherClass. other than that, this package could be anything

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I already suspected that, but "package in for resolution" doesn't really tell me anything.
maybe sth like 'otherclassrefrencefromanotherpackage`?
damn is that long :D
and not really better than yours... maybe think a bit about a better name, but if you don't find one, it's also fine for me


import com.tngtech.archunit.core.importer.testexamples.OtherClass;

public class ClassThatReferencesOtherClass {
OtherClass otherClass;
}
Loading