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(
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I'm not familiar with the class resolution yet, but this looks to me as if the package is re-created with just the current class and the package-info class.
If you look e.g. at the package testexamples, you have these classes (and more):

  • SomeClass
  • OtherClass
  • package-info

assuming we are now in the JavaClass representing SomeClass, I expect that getPackage().getClasses() contains all three of these classes, but I don't see how it could return anything else then Set.of(SomeClass, package-info), as this statement replaces the package and just passes these 2 classes into the new factory.
I don't see any call in JavaPackage.from checking the package of the class, which could contain the information.

It is also not clear to me how the dependencies and subpackages are build if all the other classes in teh package are missing.

Wouldn't it be much cleaner if we could resolve the package-info class when initially building the JavaPackage?
Why should each JavaClass in that package be responsible for resolving the package-info?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

because initially building JavaPackage is only something that happens if the package-info class is in the initial list of files passed to ClassfileImporter. when class dependencies are resolved, if the other class lives in another (non-imported) package, only the package name is known. this method that is added allows us to resolve the rest of the package info for this package, even if the entire package was not included in the import.

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"))
.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",
"com.tngtech.archunit.core.domain.packageexamples.annotated.PackageLevelAnnotation",
"com.tngtech.archunit.core.domain.packageexamples.annotated.WithinAnnotatedPackage"
);

assertThatThrownBy(nonAnnotatedPackage::getPackageInfo)
.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 @@ -59,6 +65,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 @@ -620,6 +627,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

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

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

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

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

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


@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

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

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