feat: Add support for variables in checkstyle config file

[SaptarshiSarkar12]

What problem are you trying to solve?

Currently, there is no support for accepting values of the variables defined in the checkstyle configuration file. This causes a warning from OpenRewrite that it has failed to parse the checkstyle configuration file. This problem has been observed in strimzi/strimzi-kafka-operator#9422 hence, this issue has been raised.

Describe the solution you'd like

The checkstyle variables can be input through maven/gradle plugin configurations like for maven 👇

<configuration>
    <checkstyleVariables>
        ........
        <checkstyleVariable>importControlFile=${pom.baseDir}/.checkstyle/import-control.xml<checkstyleVariable>
        ........
    </checkstyleVariables>
</configuration>

Similar case will be for gradle.
We can then replace the variable defined in that file with the expected value (By using some Java methods and File Operations like creating a copy of the original file) and send the edited checkstyle config file to the next steps. It is not going to show any warning anymore and it will be able to parse the file.

Are you interested in contributing this feature to OpenRewrite?

Yes, I would love to work on this issue. I would require some help from the maintainers to let me know if any changes needs to be made in the solution I have thought.

[timtebeek]

Hi @SaptarshiSarkar12 ; Good seeing you here and thanks for the offer to help! I think the best way to get started is to add a unit test, such that we can work out exactly what's needed from there. I imagine that we'll have to change Maven and Gradle properties separately in each of the plugins, so I'll move this issue to our Maven plugin, as that's where we first saw this issue.

[timtebeek]

We load the checkstyle configuration in this method currently:

rewrite-maven-plugin/src/main/java/org/openrewrite/maven/ConfigurableRewriteMojo.java

Lines 227 to 260 in 3094325

	protected List<NamedStyles> loadStyles(MavenProject project, Environment env) {
	List<NamedStyles> styles = env.activateStyles(getActiveStyles());
	try {
	Plugin checkstylePlugin = project.getPlugin("org.apache.maven.plugins:maven-checkstyle-plugin");
	if (checkstyleConfigFile != null && !checkstyleConfigFile.isEmpty()) {
	styles.add(CheckstyleConfigLoader.loadCheckstyleConfig(Paths.get(checkstyleConfigFile), emptyMap()));
	} else if (checkstyleDetectionEnabled && checkstylePlugin != null) {
	Object checkstyleConfRaw = checkstylePlugin.getConfiguration();
	if (checkstyleConfRaw instanceof Xpp3Dom) {
	Xpp3Dom xmlCheckstyleConf = (Xpp3Dom) checkstyleConfRaw;
	Xpp3Dom xmlConfigLocation = xmlCheckstyleConf.getChild("configLocation");
	if (xmlConfigLocation == null) {
	// When no config location is specified, the maven-checkstyle-plugin falls back on sun_checks.xml
	try (InputStream is = Checker.class.getResourceAsStream("/sun_checks.xml")) {
	if (is != null) {
	styles.add(CheckstyleConfigLoader.loadCheckstyleConfig(is, emptyMap()));
	}
	}
	} else {
	// resolve location against plugin location (could be in parent pom)
	Path configPath = Paths.get(checkstylePlugin.getLocation("").getSource().getLocation())
	.resolveSibling(xmlConfigLocation.getValue());
	if (configPath.toFile().exists()) {
	styles.add(CheckstyleConfigLoader.loadCheckstyleConfig(configPath, emptyMap()));
	}
	}
	}
	}
	} catch (Exception e) {
	getLog().warn("Unable to parse checkstyle configuration. Checkstyle will not inform rewrite execution.", e);
	}
	return styles;
	}

Notice how that switches between different CheckstyleConfigLoader.loadCheckstyleConfig methods based on the type (InputStream or Path). There's a third loadCheckstyleConfig method that takes a String that might be better suited here, if we are to first replace any properties with values.

Perhaps the best way to start is with a unit test on the loading of styles, such that we can see that we correctly replace the values.

[SaptarshiSarkar12]

Hi @SaptarshiSarkar12 ; Good seeing you here and thanks for the offer to help! I think the best way to get started is to add a unit test, such that we can work out exactly what's needed from there. I imagine that we'll have to change Maven and Gradle properties separately in each of the plugins, so I'll move this issue to our Maven plugin, as that's where we first saw this issue.

Thank you @timtebeek for all your efforts 😄. I love contributing to Open-Source projects.
@timtebeek So, do we need to create a test class? If yes, in which tests directory?
Yes, we need to make those changes separately.

[SaptarshiSarkar12]

We load the checkstyle configuration in this method currently:

rewrite-maven-plugin/src/main/java/org/openrewrite/maven/ConfigurableRewriteMojo.java

Lines 227 to 260 in 3094325

	protected List<NamedStyles> loadStyles(MavenProject project, Environment env) {
	List<NamedStyles> styles = env.activateStyles(getActiveStyles());
	try {
	Plugin checkstylePlugin = project.getPlugin("org.apache.maven.plugins:maven-checkstyle-plugin");
	if (checkstyleConfigFile != null && !checkstyleConfigFile.isEmpty()) {
	styles.add(CheckstyleConfigLoader.loadCheckstyleConfig(Paths.get(checkstyleConfigFile), emptyMap()));
	} else if (checkstyleDetectionEnabled && checkstylePlugin != null) {
	Object checkstyleConfRaw = checkstylePlugin.getConfiguration();
	if (checkstyleConfRaw instanceof Xpp3Dom) {
	Xpp3Dom xmlCheckstyleConf = (Xpp3Dom) checkstyleConfRaw;
	Xpp3Dom xmlConfigLocation = xmlCheckstyleConf.getChild("configLocation");
	if (xmlConfigLocation == null) {
	// When no config location is specified, the maven-checkstyle-plugin falls back on sun_checks.xml
	try (InputStream is = Checker.class.getResourceAsStream("/sun_checks.xml")) {
	if (is != null) {
	styles.add(CheckstyleConfigLoader.loadCheckstyleConfig(is, emptyMap()));
	}
	}
	} else {
	// resolve location against plugin location (could be in parent pom)
	Path configPath = Paths.get(checkstylePlugin.getLocation("").getSource().getLocation())
	.resolveSibling(xmlConfigLocation.getValue());
	if (configPath.toFile().exists()) {
	styles.add(CheckstyleConfigLoader.loadCheckstyleConfig(configPath, emptyMap()));
	}
	}
	}
	}
	} catch (Exception e) {
	getLog().warn("Unable to parse checkstyle configuration. Checkstyle will not inform rewrite execution.", e);
	}
	return styles;
	}

Notice how that switches between different CheckstyleConfigLoader.loadCheckstyleConfig methods based on the type (InputStream or Path). There's a third loadCheckstyleConfig method that takes a String that might be better suited here, if we are to first replace any properties with values.

Perhaps the best way to start is with a unit test on the loading of styles, such that we can see that we correctly replace the values.

Yeah, right. We can change the String and replace the values just before it is loaded. Also, adding a test will help, I agree.

[timtebeek]

Probably best to start with a test similar to the tests we have already in https://github.com/openrewrite/rewrite-maven-plugin/tree/main/src/test ; then configure checkstyle in a new project there, with variables in the xml, and see that it does not throw an exception. Best to start a draft pull request early, such that we can both track progress and commit as needed.

[SaptarshiSarkar12]

@timtebeek Okay. Can you please tell me on what basis are the tests created? I mean does it check the console outputs by the plugin or something else? I found the test for console outputs from

https://github.com/openrewrite/rewrite-maven-plugin/blob/main/src/test/java/org/openrewrite/maven/RewriteDryRunIT.java#L48-L55

and

https://github.com/openrewrite/rewrite-maven-plugin/blob/main/src/test/java/org/openrewrite/maven/DiscoverNoActiveRecipeIT.java#L31-L41

[tobli]

The checkstyle maven plugin has quite a few options, like inline config, config in files, and config in jar files of plugin dependencies. I imagine re-implementing all of them would be quite complex. Have you considered either:

• first executing the plugin, since it will resolve the final config and write it to target/checkstyle-checker.xml (and resolve any properties file as well). This will run the style check (failures can be suppressed with failOnViolation=false), which may not be desirable.
• adding a dependency on either the plugin jar (more code), or just Plexus to get the ResourceManager that does the heavy lifting?

[timtebeek]

Thanks for the helpful suggestions @tobli ! at 97KB the checkstyle plugin itself isn't even that big to include, although we wouldn't want any of it's goals to show up. It's total size addition will depend on what dependencies it brings in. Definitely something we can explore.

@SaptarshiSarkar12 right now it seems we mostly verify that tasks successfully ran, and what output was produced in terms of log lines and files. I think the best way to start is to add a test that has a checkstyle config that we need to resolve and work from there, taking into account the hints above.

[SaptarshiSarkar12]

@timtebeek Okay. Got the idea. Thank you!