Skip to content

Refactor the rule loading mechanism to use ServiceLoader Pattern#69

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

Refactor the rule loading mechanism to use ServiceLoader Pattern#69
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 23, 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.

Caution

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 (see an example implementation):
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. A custom rule provider class that implement RuleProvider interface should be created (see an example implementation):
public class OSRuleProvider implements RuleProvider {  
    @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 (see an example implementation):
io.ballerina.stdlib.os.compiler.staticcodeanalyzer.OSRuleProvider
  1. The rules.json file is no longer needed and can be removed from the plugin resources.

Check List

@nureka-rodrigo
Copy link
Copy Markdown
Contributor Author

@keizer619 keizer619 requested a review from Copilot June 27, 2025 06:16
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

This PR refactors the rule loading mechanism to remove the deprecated JSON‑based approach and switch to Java’s ServiceLoader pattern for discovering rule providers. Key changes include the removal of rules.json files and associated JSON parsing code, a new loadRulesFromEnum method that uses ServiceLoader to load rules, and cleanup of obsolete compiler plugin files.

Reviewed Changes

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

File Description
test-compiler-plugins/* Removal of several plugin files (rules.json, analyzer implementations, README, etc.) related to the JSON‑based rule loading mechanism
scan-command/src/main/java/io/ballerina/scan/internal/ProjectAnalyzer.java Refactored external analyzer loading to use ServiceLoader, replacing old JSON parsing logic
Other files (e.g. IssueImpl, ReporterImpl, module-info.java, etc.) Adjustments to align with the new ServiceLoader pattern and removal of obsolete imports

Comment on lines +142 to +143
externalAnalyzers.put(pkgDependency.manifest().compilerPluginDescriptor().toString(),
new ArrayList<>());
Copy link

Copilot AI Jun 27, 2025

Choose a reason for hiding this comment

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

When pluginDesc is null, the current code uses pkgDependency.manifest().compilerPluginDescriptor().toString() as the key for externalAnalyzers. This key may be ambiguous or lead to unexpected behavior; consider using a more explicit fallback or skip adding an entry for dependencies without a valid descriptor.

Suggested change
externalAnalyzers.put(pkgDependency.manifest().compilerPluginDescriptor().toString(),
new ArrayList<>());
// Skip adding an entry for dependencies without a valid descriptor

Copilot uses AI. Check for mistakes.

try (URLClassLoader ucl = URLClassLoader.newInstance(jarUrls.toArray(URL[]::new),
this.getClass().getClassLoader())) {
ServiceLoader<RuleProvider> loader = ServiceLoader.load(RuleProvider.class, ucl);
Copy link

Copilot AI Jun 27, 2025

Choose a reason for hiding this comment

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

It may be useful to log or handle the case where no RuleProvider implementations are found, so that the absence of external rules does not go unnoticed during runtime.

Copilot uses AI. Check for mistakes.
@@ -0,0 +1,45 @@
package io.ballerina.examplestaticcodeanalyzer;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Usually we have multiple levels for package name like this io.ballerina.runtime.observability.metrics
better to rename this

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

These are test-related files that have existed since version 0.1.0. Should I still proceed with the renaming?

@sonarqubecloud
Copy link
Copy Markdown

@nureka-rodrigo nureka-rodrigo marked this pull request as draft September 4, 2025 06:36
@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud bot commented Sep 5, 2025

@nureka-rodrigo nureka-rodrigo marked this pull request as ready for review September 5, 2025 06:14
@nureka-rodrigo nureka-rodrigo closed this by deleting the head repository Nov 2, 2025
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