Restricting based on modules

Question

Restricting based on modules

Closed this issue 3 years ago · 7 comments

A few nights ago I wasted a few hours debugging what turned out to be a few unwanted imports snuck into my code by well-meaning committers. One of my fellow maintainers recommended your project, which looks fantastic. I've mostly got things configured as I want them but I have a few implementation questions and possibly a related feature request.

Use case / TLDR:

I want to use this enforcer rule to only permit packages aligned with the modules I require.

Easier said than done. 😁

Motivation:

I support JDK8, and plan to until EOL, so my main development branch is JDK8 based
Some users want a module descriptor to support JDK11+, so I have a separate branch, with a few minor tweaks. I keep this up to date by cherry-picking into it, but it doesn't see the same testing with CI except when I copy stuff over
When I run into testing issues, they're nearly always related to java modules somehow. In the latest case, I had Junit 4 code that "worked" due to a transitive dependency, but was unwanted in my Junit 5 setup.
I could try to restrict expected problem areas (JUnit/Hamcrest) but other transitive issues have cropped up (Apache Commons, etc.) and it can end up being a game of whack-a-mole.
What I really want is a "exclude everything but what I allow"
- This is currently supported (see my example below) but on the package level
- I can read the module descriptors for my dependencies and add all their exported packages, except...
- The package level is unwieldy for some modules, specifically java.base.

Here's what I currently have which is mostly what I want:

<RestrictImports>
    <includeTestCode>true</includeTestCode>
    <!-- Disallow all imports except explicitly allowed -->
    <reason>Disallow transitive dependencies</reason>
    <bannedImport>**</bannedImport>
    <allowedImports>
        <!-- Allow oshi itself :-) -->
        <allowedImport>oshi.**</allowedImport>
        <!-- Allow core Java usages -->
        <allowedImport>java.**</allowedImport>
        <!-- Allow known dependencies -->
        <allowedImport>com.sun.jna.**</allowedImport>
        <allowedImport>org.slf4j.**</allowedImport>
        <allowedImport>org.junit.jupiter.api.**</allowedImport>
    </allowedImports>
    <!-- No restrictions on oshi-demo -->
    <exclusion>oshi.demo.**</exclusion>
</RestrictImports>

Two problems here:

I would really like to restrict to the java.base module. Your README includes an example of excluding JUL, whose implementations are in the java.logging module, which I don't include in my modular descriptor. I tried just listing by packages, but there are a ludicrous number of packages in java.base.
- While I understand that it may be impractical/impossible to restrict based on modules (one would have to read the modular descriptor and parse out all the requires, then find those modules and parse out the exports, and that's not even possible for projects like my own and JNA who publish separate JDK8 and JDK11 artifacts), I do think that the JDK, or the java.base module in particular, is special enough that it might deserve its own special treatment in your code. (I'd be happy to try to contribute something along these lines if you think it's a good idea.)
The documentation isn't clear in which order these rules are applied.
- Is it always restrict-then-allow? Can I allow broadly and then restrict?
- Can I restrict **, allow foo.**, and then restrict foo.bar.** in that order?
  - Specifically I've been lazy and just allowed java.** but I'd like to exclude JUL...
- How do multiple <RestrictImports> sections interact with each other? Both must apply? Either must apply? If I add a new section restricting JUL to my above configuration would it achieve my goal?

A third "problem" -- looks like I need to duplicate this entire setup for static imports? Is there no easy switch to use the same config for both?

Thanks for a great project. Looking forward to tweaking it "just right".

Answer 1 · 2021-11-27T18:24:28.000Z

Thx for your kind feedback, I really appreciate that. Also I'm happy to hear that someone uses this thing beyond the simple stuff and tries to implement something really interesting with it.
I'm short on time right now so I haven't really digested your whole text and I'm not sure whether I understand your requirements correctly. To be honest, I haven't really thought about this plugin in the context of JPMS at all. Let's tackle the easy questions first:

"Is it always restrict-then-allow? Can I allow broadly and then restrict?"
As stated here allowedImports always take precedence over bannedImports, so in these terms, it is always restrict-then-allow.
"Can I restrict , allow foo., and then restrict foo.bar.** in that order? "
This might be possible with Rule Groups. You could have a rule with basePackage=** which retricts everything, then another rule with basePackage=foo.* and another group with basePackage=foo.bar.**. When examining a single source file for banned imports, only the rules in the group with the most specific <basePackage> are applied.
In our example, when examining the class foo.Whatever, the base packages of all 3 groups would match, but only the rules in the group with basePackage=foo.bar.* would be applied as that is the most specific pattern among the 3 (Note that specificty of rule groups will get a minor do-over in the 2.0.0 release (see #49)).
You might run into problems with the middle group though, because I'm currently not sure whether it is allowed to have a rule that has no <bannedImport> defined. You could try though.
"How do multiple <RestrictImports> sections interact with each other?
This is simple: they just don't. Each <RestrictImports> section is handled completely independent from any other (each one becomes an independent instance of RestrictImports). I don't think it would be even technically possible that they interact as the rule class is instantiated and managed by the enforcer-plugin itself.
A third "problem" -- looks like I need to duplicate this entire setup for static imports?
Yes, you are right. But I think I grab this up as a feature because it seems pretty reasonable to ban the same imports, whether they are static or not (created #53)

Btw., great time to come up with new ideas because I've been working on version 2 anyways, so I'm more open to new and potentially breaking changes.

Answer 2 · 2021-11-27T18:42:31.000Z

Great, thanks for the pointer to rule groups. I'll dig into those a bit more, and offer any more lessons/feedback.

As mentioned, I think it's impossible to really restrict by modules, but would you accept a PR (if I get around to it) that would permit a syntax such as: <allowedModule>java.base</allowedModule> that would essentially allow all the packages in that module?

Answer 3 · 2021-11-29T00:13:45.000Z

Having thought a bit more about the modules, I think the best approach would to be allow the user to specify the paths to text files containing the module-info.java module descriptors, e.g.,

<allowedModuleExports>${basedir}/path-to-file</allowedModuleExports>

So for the java.base example, one would just copy the module-info.java file linked in the previous comment to the project filesystem somewhere and give it a friendly name like java.base.txt. Repeat as necessary for the modular dependencies of the project (requires and requires transitive). Then you could simply iterate through the file and find all "exports foo.bar;" ~~or "exports transitive foo.bar.baz;"~~ lines and add those packages to the allowed list.

Answer 4 · 2021-11-29T18:28:07.000Z

Isn't it possible, by looking at your own module-info.java, to decide your problem? I think you want to restrict your code base to only require java.base and forbid to require for example java.logging. So maybe, instead of taking the detour to decide this on package level, we could decide it by parsing a project's own module descriptors? I might be missing something here, I still haven't put much thought into it. But at first glance this seems like a reasonable approach for modular applications.

Answer 5 · 2021-11-29T20:15:52.000Z

Isn't it possible, by looking at your own module-info.java, to decide your problem?

If I had my own module-info.java (available to the plugin) this wouldn't be needed at all as the JDK9+ compiler would handle it for me! 😀

Before I move on, I'll note this is a much lower priority now, as I've figured out how to configure Checkstyle to handle this case. (I could also do the same package list with your plugin.) Here's the config file for main and for test (trimmed down). So I won't mind if you close this issue as "too hard".

Here's the logic behind why I want a plugin like this.

I support jdk8 and that project has no module-info.java
- that means in my JDK8 build, committers can access classes anywhere on the dependency tree of my project or its depencencies
- I also publish a JDK11 build with a module descriptor that restricts what packages can be used
  - The JDK11 compiler happily tells me when I've done something naughty
- my JDK8 build is the one I have set up with CI for pull requests, where I'm trying to catch these errors before the stack up and waste my time catching them later in the jdk11 build.
  - I usually cherry-pick several commits at a time into the JDK11 branch, and don't want to wait until this point to find the problems
So for the CI for JDK8 I don't have a module-info.java (it's in the source tree of a branch)
Even if I do/did have it, the sequence to find "legal" packages is:
1. Go through my module-info.java and find every requires and requires transitive
2. Find the corresponding module descriptor for that dependency (for one of my dependencies it's a separate jar than my JDK8 build)
3. In that module descriptor, get a list of all the unqualified exports statements -- these are the packages I want to allow

So it's reasonably easy to hunt down those module descriptors manually and globally edit exports foo.bar; to pkg allow="foo.bar". I've done that for checkstyle (see link above) and could do it with your plugin (or just use wildcards, which I've done). That's less "maintainable" than simply dropping a plain copy of the module-info for the dependencies into my files somewhere.

And as you can see from the checkstyle example, it's not bad listing all the packages for most projects, but for java.base there are dozens of packages. I'm OK sticking those into an xml config file but don't want all that spam in my pom.

Anyway... as I said, I've got checkstyle addressing this, and my slightly more broad application with this plugin (shown above) will work fine, particularly with the "import" simplification. So I'm ok with closing this request... but it was a fun exercise to think through.

Answer 6 · 2021-11-30T07:04:47.000Z

Ok, there was a missing bit that I didn't quite understand from you usecase, but now I got it. You want to achieve module compatibility without having modules, because you support both JDK8 and 11.
I think yours is a valid use case but its still a bit exotic to just start implementing it in the plugin. So good to know that you found a work around.

I've already thought about making this plugin plugable as well or provide the possibility to externalize the configuration from the pom (mostly to make it possible to easily share plugin configurations). Both still seems like a bit of overkill for now, so I'd like to keep the feature set as compact and simple as possible.

This discussion is a good starting point to actually think about this plugin in the context of JPMS modules (though they are an exotic topic on their own :))

Answer 7 · 2021-12-01T07:48:52.000Z

I think yours is a valid use case but its still a bit exotic to just start implementing it in the plugin.

TBH, with already having the shortcoming of only catching the import statements and not a fully qualified class name, it's best to keep this simple and streamlined. It can catch the "obvious" but the fact that I have checkstyle doing the harder/more detailed work is good enough for my CI setup, but I still like this enforcer rule (slightly more lenient) as it provides faster feedback (on mvn test, rather than requiring a separate mvn checkstyle:check) and it doesn't hurt to implement both.

The one takeaway I might consider from this discussion that's still somewhat valid is the idea of listing the package restrictions in a config xml separate from the pom. But that's just a nice-to-have preference thing.