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.

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

ArchUnit doesn't scan src/main/java when using Java Modules (Java 11) #497

Closed
adem-oz opened this issue Dec 28, 2020 · 7 comments · Fixed by #506
Closed

ArchUnit doesn't scan src/main/java when using Java Modules (Java 11) #497

adem-oz opened this issue Dec 28, 2020 · 7 comments · Fixed by #506
Assignees

Comments

@adem-oz
Copy link

adem-oz commented Dec 28, 2020

We have an issue similar to the #321

ArchUnit doesn't scan the src/main/java when we have a Java Module (Java 11).

I will attach a demo.zip file which contains the demo.

Scenario to reproduice:
There is 3 projects in the demo.zip file.
mycompany-parent: basic maven parent project which contains the versions of the libraries used in the demo.
mycompany-project1: maven project which is not a Java module (doesn't contain module-info.java)
mycompany-project2: maven project which is a Java module.

mvn install the projects: mycompany-parent, mycompany-project1 and mycompany-project2.

The mycompany-project2 contains the archunit test which is scanning src/main/java but it cannot find the file "SecondService.java".

demo.zip

What is strange is if I change the package to scan "be.toto" inside "Project2ArchitectureTest.java". It finds the file inside that package but when I put it back to "be.mycompany" it doesn't find the files inside that package.

When I delete the module-info.java, it also find successfully the files inside package "be.mycompany".

Thanks for your help :)

@codecholeric
Copy link
Collaborator

Thanks for reporting this! 👍 I think this is a different issue though, if I understand it correctly. And I think for your issue the example project could also be reduced even more? At least I don't see why you need project1 at all, since there is no cross dependency at the moment and your problem also is unrelated to classes in project1? (i.e. the test is within project2 and so are the classes you're missing in your test?)

I think it might be related to the module path, i.e. you probably only need project2 to reproduce it, as long as you use Java Modules. However I'm a little puzzled why the loader behaves differently for package be.mycompany.project2 and be.toto, that seems to make no sense 🤔

I'll try to look into it more and then come up with a fix! (and understand why be.mycompany.project2 and be.toto behave differently, I'm really curious about that 😉)

@adem-oz
Copy link
Author

adem-oz commented Jan 8, 2021

With pleasure :)

Indeed, the project1 is not necessary to reproduce the issue. It was part of another test scenario that I didn't mention to not confuse you :)

Second test scenario:
add project1 dependency in project2 pom.xml
run the test inside project2

I saw that the file inside project1 was detected when scanning occurs on the jars (scan on package be.mycompany).

The project1 doesn't have a module-info. So it's not in the module path normally. Then I suspected that there is a problem when mixing project which has a module-info and project that doesn't have it.

@codecholeric
Copy link
Collaborator

I think I got a step further to at least understand why ArchUnit behaves the observed way at the moment (i.e. if you scan be.mycompany no violation is reported, but if you scan be.toto it finds the violation).
As far as I see the modulepath scanning doesn't work reliably for custom modules 🤔 The difference for the two packages is, that target/test-classes contains a package be.mycompany, but not be.toto.

I think what happens is the following:

  • Java 9 modulepath scanning misses the custom module
  • but it tries to find ClassLoader.getResources(packageAsResource) as a backup
  • for be.toto this works, because it will find the location in target/classes
  • for be.mycompany this doesn't work, because --patch-module will add target/test-classes and ClassLoader.getResources("/be/mycompany") will then ONLY return target/test-classes, since it contains that folder and --patch-module overrides the resource location and hides the original one (which I find odd, but I guess the JDK devs don't -> http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-July/054228.html)

I guess the solution will be to fix step one and correctly scan custom modules, so the backup through ClassLoader.getResources(..) does not need to catch this. I'll add some test project to ArchUnit to prevent regression in the future after I've fixed this...

@codecholeric
Copy link
Collaborator

I think I've implemented a fix in #506. No warranties that it will work for all sort of combinations of JVM module parameters afterwards 😉, but the example project you provided seems to have worked like expected with it.
If you want you can test this fix by adding the following repository to your POM:

<repositories>
  <repository>
    <id>sonatype-snapshots</id>
    <url>https://oss.sonatype.org/content/repositories/snapshots</url>
  </repository>
</repositories>

Then upgrade the ArchUnit version to 0.16.0-SNAPSHOT to pull in a SNAPSHOT version that contains the fix.
I would be very interested to hear, if the fix feels good to you and solves all your problems! (or vice versa, if there is another issue left to fix)

@adem-oz
Copy link
Author

adem-oz commented Jan 22, 2021

Indeed, it works well in my projects as well :)

I guess it will be part of the next release. When is the next Release planned?

@codecholeric
Copy link
Collaborator

Kewl 🎉 I will release this quickly then, basically merge #503 (where I'm currently waiting on a review), then release 👍

codecholeric added a commit that referenced this issue Jan 25, 2021
In a modular Java project, where the main code is supplied from the modulepath instead of the classpath, the class scanning was unreliable. In fact the behavior was sporadic at first sight. To elaborate, consider the following structure:

```
src
|-- main
|    |-- java
|         |-- module-info.java
|         |-- com/root/First.java
|         |-- com/otherroot/Second.java
|-- test
      |-- java
           |-- com/root/ArchUnitTest.java
```

In this scenario `com.otherroot.Second` would be found by ArchUnit's scanning, but surprisingly `com.root.First` would not be found. The reason for this behavior was an internal fallback to `ClassLoader.getResources(..)` to locate the class (e.g. `/com/otherroot/Second.class`), but this only worked, if the respective resource path had not been overwritten via `--patch-module`. Surprisingly `--patch-module` causes the original resource path to be hidden (compare http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-July/054228.html).
Thus in this example case `ArchUnitTest` could find `com.otherroot.Second`, as long as only `com/root` was patched. I.e. Maven would add `--patch-module example_module=com/root` to patch the test class into the module. But `ClassLoader.getResources(..)` would then not find `/com/root/First.class` anymore, since `target/classes/com/root` had been overwritten with `target/test-classes/com/root`. On the other hand `ClassLoader.getResources(..)` would still look into `target/classes/com/otherroot`, since that path had not been patched.

To fix this behavior I have tried to eliminate the root cause, namely to make the fallback to `ClassLoader.getResources(..)` unnecessary for classes on the module path. So far in the first step only the configured classpath and the system modulepath have been read in `ModuleLocationResolver`. I have extended this to also parse the configured modulepath (through the system property `jdk.module.path`) and scan the content of these modules analogously to the system modules.

It provided quite challenging to add a test for this behavior, since ArchUnit itself is not written as Java Modules and Gradle's support for Java Modules has proven to still lack some usability (i.e. I could not find a way without tinkering around with compiler and JVM args to get this working in a modular way, but this also might be because of the somewhat suboptimal ArchUnit build). To make the setup easier I have added the `org.javamodularity.moduleplugin` (compare https://github.com/java9-modularity/gradle-modules-plugin) to a separate Gradle module, which has the only purpose to try to scan a class from the module path that has been overridden by a `--patch-module` (since the test resides in the same package).
However, this test only has any significance if run with Gradle, since IntelliJ seems to simply dump all classes on the classpath to run tests, no matter if the code base is written as Java Modules. Thus I've added a simple assertion to the test to make sure that the respective `main` class is actually not present on the classpath, since the test would then just always be successful, even if modulepath scanning would be broken. This assertion is a little brittle, because it uses a lot of Reflection, but I wanted to reuse ArchUnit's main code to resolve the classpath and I didn't want to make these internal parts visible for external use, so I decided this is still the best solution, considering all tradeoffs.

In any case modulepath scanning probably still doesn't work in all scenarios now (e.g. if `--patch-module` and other options are supplied), but it should be good enough for most standard use cases. And since this issue has only been brought up over 3 years after ArchUnit + Java 9 have been out there together, I would guess that exotic combinations (like "I want to use `--patch-module` to patch in test code that I want to scan with ArchUnit" or similar) are not that common. Besides that it is always possible to just dump all classes on the classpath together to execute the ArchUnit test and avoid the modulepath issues altogether (like Gradle or IntelliJ seem to do by default anyway to execute unit tests).

Resolves: #497
@adem-oz
Copy link
Author

adem-oz commented Jan 25, 2021

thank you :-)

codecholeric added a commit that referenced this issue Feb 21, 2021
In a modular Java project, where the main code is supplied from the modulepath instead of the classpath, the class scanning was unreliable. In fact the behavior was sporadic at first sight. To elaborate, consider the following structure:

```
src
|-- main
|    |-- java
|         |-- module-info.java
|         |-- com/root/First.java
|         |-- com/otherroot/Second.java
|-- test
      |-- java
           |-- com/root/ArchUnitTest.java
```

In this scenario `com.otherroot.Second` would be found by ArchUnit's scanning, but surprisingly `com.root.First` would not be found. The reason for this behavior was an internal fallback to `ClassLoader.getResources(..)` to locate the class (e.g. `/com/otherroot/Second.class`), but this only worked, if the respective resource path had not been overwritten via `--patch-module`. Surprisingly `--patch-module` causes the original resource path to be hidden (compare http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-July/054228.html).
Thus in this example case `ArchUnitTest` could find `com.otherroot.Second`, as long as only `com/root` was patched. I.e. Maven would add `--patch-module example_module=com/root` to patch the test class into the module. But `ClassLoader.getResources(..)` would then not find `/com/root/First.class` anymore, since `target/classes/com/root` had been overwritten with `target/test-classes/com/root`. On the other hand `ClassLoader.getResources(..)` would still look into `target/classes/com/otherroot`, since that path had not been patched.

To fix this behavior I have tried to eliminate the root cause, namely to make the fallback to `ClassLoader.getResources(..)` unnecessary for classes on the module path. So far in the first step only the configured classpath and the system modulepath have been read in `ModuleLocationResolver`. I have extended this to also parse the configured modulepath (through the system property `jdk.module.path`) and scan the content of these modules analogously to the system modules.

It provided quite challenging to add a test for this behavior, since ArchUnit itself is not written as Java Modules and Gradle's support for Java Modules has proven to still lack some usability (i.e. I could not find a way without tinkering around with compiler and JVM args to get this working in a modular way, but this also might be because of the somewhat suboptimal ArchUnit build). To make the setup easier I have added the `org.javamodularity.moduleplugin` (compare https://github.com/java9-modularity/gradle-modules-plugin) to a separate Gradle module, which has the only purpose to try to scan a class from the module path that has been overridden by a `--patch-module` (since the test resides in the same package).
However, this test only has any significance if run with Gradle, since IntelliJ seems to simply dump all classes on the classpath to run tests, no matter if the code base is written as Java Modules. Thus I've added a simple assertion to the test to make sure that the respective `main` class is actually not present on the classpath, since the test would then just always be successful, even if modulepath scanning would be broken. This assertion is a little brittle, because it uses a lot of Reflection, but I wanted to reuse ArchUnit's main code to resolve the classpath and I didn't want to make these internal parts visible for external use, so I decided this is still the best solution, considering all tradeoffs.

In any case modulepath scanning probably still doesn't work in all scenarios now (e.g. if `--patch-module` and other options are supplied), but it should be good enough for most standard use cases. And since this issue has only been brought up over 3 years after ArchUnit + Java 9 have been out there together, I would guess that exotic combinations (like "I want to use `--patch-module` to patch in test code that I want to scan with ArchUnit" or similar) are not that common. Besides that it is always possible to just dump all classes on the classpath together to execute the ArchUnit test and avoid the modulepath issues altogether (like Gradle or IntelliJ seem to do by default anyway to execute unit tests).

Resolves: #497
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants