Skip to content

Refactor the rule loading mechanism to use ServiceLoader Pattern#67

Closed
nureka-rodrigo wants to merge 3 commits intoballerina-platform:mainfrom
nureka-rodrigo:test
Closed

Refactor the rule loading mechanism to use ServiceLoader Pattern#67
nureka-rodrigo wants to merge 3 commits intoballerina-platform:mainfrom
nureka-rodrigo:test

Conversation

@nureka-rodrigo
Copy link
Copy Markdown
Contributor

@nureka-rodrigo nureka-rodrigo commented Jun 3, 2025

Purpose

Fixes #66

Approach

Replaced the JSON-based rule loading mechanism with Java's ServiceLoader pattern. Before the rules were loaded by parsing a rules.json file from the plugin JAR using Gson. Rules are now loaded using ServiceLoader<RuleProvider>, where external plugins implement the RuleProvider interface.

Limitation

  1. The scan tool is no longer backward compatible with old compiler plugins. Existing plugins will need to be updated to work with this new approach.

Migration Guide for Compiler Plugin Authors

With this change, external compiler plugins need to be updated to use the new ServiceLoader-based approach:

  1. Rules should now be defined as enum classes that implement the Rule interface:
public enum OSRule implements Rule {
    AVOID_UNSANITIZED_CMD_ARGS(1, "Avoid constructing system command arguments from user " +
            "input without proper sanitization", VULNERABILITY),
    AVOID_UNSANITIZED_ENV_VARS(2, "Environment variables should not be defined from untrusted input",
            VULNERABILITY);

    private final int id;
    private final String description;
    private final RuleKind kind;

    OSRule(int id, String description, RuleKind kind) {
        this.id = id;
        this.description = description;
        this.kind = kind;
    }

    @Override
    public String id() {
        return "";
    }

    @Override
    public int numericId() {
        return id;
    }

    @Override
    public String description() {
        return description;
    }

    @Override
    public RuleKind kind() {
        return kind;
    }
}
  1. The compiler plugin code analyzer should implement the RuleProvider with a public default constructor:
public class OSStaticCodeAnalyzer extends CodeAnalyzer implements RuleProvider {
    private Reporter reporter;

    public OSStaticCodeAnalyzer() {
        // Default constructor for service loading
    }

    public OSStaticCodeAnalyzer(Reporter reporter) {
        this.reporter = reporter;
    }

    @Override
    public void init(CodeAnalysisContext analysisContext) {
        analysisContext.addSyntaxNodeAnalysisTask(new OSCommandInjectionAnalyzer(reporter), List.of(FUNCTION_CALL));
    }

    @Override
    public Iterable<Rule> getRules() {
        return List.of(OSRule.values());
    }
}
  1. The Service should be registered in META-INF/services/io.ballerina.scan.RuleProvider in the compiler plugin JAR:
io.ballerina.stdlib.os.compiler.staticcodeanalyzer.OSStaticCodeAnalyzer
  1. The rules.json file is no longer needed and can be removed from the plugin resources.

Check List

@nureka-rodrigo nureka-rodrigo requested a review from azinneera as a code owner June 3, 2025 18:53
@keizer619 keizer619 requested a review from Copilot June 4, 2025 02:01

This comment was marked as outdated.

@nureka-rodrigo nureka-rodrigo marked this pull request as draft June 4, 2025 03:05
@nureka-rodrigo nureka-rodrigo marked this pull request as ready for review June 4, 2025 04:55
@nureka-rodrigo nureka-rodrigo requested a review from Copilot June 4, 2025 04:55

This comment was marked as outdated.

@nureka-rodrigo nureka-rodrigo marked this pull request as draft June 4, 2025 05:14
@nureka-rodrigo nureka-rodrigo requested a review from Copilot June 4, 2025 05:21

This comment was marked as outdated.

Comment thread scan-command/src/main/java/io/ballerina/scan/internal/ProjectAnalyzer.java Outdated
Comment thread scan-command/src/main/java/io/ballerina/scan/internal/ReporterImpl.java Outdated
Comment thread scan-command/src/main/java/io/ballerina/scan/internal/ProjectAnalyzer.java Outdated
Comment thread scan-command/src/main/java/io/ballerina/scan/internal/ReporterImpl.java Outdated
@nureka-rodrigo nureka-rodrigo changed the title Fix classloader compatibility issue in reportIssue method using proxy pattern Fix classloader compatibility and refactor rule loading mechanism Jun 6, 2025
@nureka-rodrigo nureka-rodrigo requested a review from Copilot June 6, 2025 04:14

This comment was marked as outdated.

@nureka-rodrigo nureka-rodrigo requested a review from Copilot June 6, 2025 04:23

This comment was marked as outdated.

@nureka-rodrigo nureka-rodrigo changed the title Fix classloader compatibility and refactor rule loading mechanism Refactor the rule loading mechanism to use ServiceLoader Pattern Jun 10, 2025
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Refactors the rule loading mechanism to use Java’s ServiceLoader instead of parsing a rules.json file.

  • Introduces a RuleProvider SPI and updates module-info.java to uses it
  • Removes Gson-based JSON loading and adds loadRulesFromEnum to pull rules via ServiceLoader
  • Updates ProjectAnalyzer to collect and wrap external rules dynamically

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
scan-command/src/main/java/module-info.java Declares uses io.ballerina.scan.RuleProvider for service loading
ProjectAnalyzer.java Replaces JSON parsing with ServiceLoader<RuleProvider> logic
scan-command/src/main/java/io/ballerina/scan/RuleProvider.java Adds the RuleProvider service interface
Comments suppressed due to low confidence (4)

scan-command/src/main/java/io/ballerina/scan/internal/ProjectAnalyzer.java:138

  • Using Optional.toString() as the map key yields an unclear key (e.g. "Optional.empty"). Consider using a consistent plugin identifier, such as pluginDesc.plugin().getClassName() or org + "/" + name.
externalAnalyzers.put(pkgDependency.manifest().compilerPluginDescriptor().toString(), new ArrayList<>());

scan-command/src/main/java/io/ballerina/scan/internal/ProjectAnalyzer.java:157

  • [nitpick] Method name loadRulesFromEnum is misleading since rules are loaded via ServiceLoader, not strictly from enums. Consider renaming to loadRulesFromProviders or loadRulesWithServiceLoader.
private List<Rule> loadRulesFromEnum(CompilerPluginDescriptor pluginDesc, String org, String name)

scan-command/src/main/java/io/ballerina/scan/internal/ProjectAnalyzer.java:126

  • The new ServiceLoader path in getExternalAnalyzers lacks direct unit tests. Consider adding tests that validate rule discovery from dummy RuleProvider implementations.
Map<String, List<Rule>> getExternalAnalyzers() {

scan-command/src/main/java/io/ballerina/scan/RuleProvider.java:28

  • The interface Javadoc should mention the requirement for a public no-argument constructor on implementations to support ServiceLoader instantiation.
public interface RuleProvider {

Comment thread scan-command/src/main/java/io/ballerina/scan/internal/ProjectAnalyzer.java Outdated
Comment thread scan-command/src/main/java/io/ballerina/scan/internal/ProjectAnalyzer.java Outdated
@nureka-rodrigo nureka-rodrigo marked this pull request as ready for review June 11, 2025 07:24
@sonarqubecloud
Copy link
Copy Markdown

@nureka-rodrigo nureka-rodrigo marked this pull request as draft June 16, 2025 05:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Refactor scan tool to retrieve rule metadata directly from Rule instance instead of rules.json

3 participants