Fix #5525: [core] Add Sarif Level Property #5573

julees7 · 2025-03-05T20:04:34Z

Describe the PR

Adding and filling the field for the severeness of a rule in the sarif report according to the defined priorities in the pmd ruleset.

Related issues

adding the suggested feature from ticket [core] Add rule priority as level to Sarif report #5525
Fixes [core] Add rule priority as level to Sarif report #5525

Ready?

Added unit tests for fixed bug/feature
Passing all unit tests
Complete build ./mvnw clean verify passes (checked automatically by github actions)
Added (in-code) documentation (if needed)

adangel

Thanks for your PR!

Could you please also update the example on https://docs.pmd-code.org/latest/pmd_userdocs_report_formats.html#sarif ? The file to change is https://github.com/pmd/pmd/blob/main/docs/report-examples/pmd-report.sarif.json

Then I think, we should additionally add the property "defaultConfiguration" to "runs[0].tool.driver.rules[x]". There we can specify the default log level per rule. See https://docs.oasis-open.org/sarif/sarif/v2.1.0/errata01/os/sarif-v2.1.0-errata01-os-complete.html#_Toc141791100 and https://docs.oasis-open.org/sarif/sarif/v2.1.0/errata01/os/sarif-v2.1.0-errata01-os-complete.html#_Toc141791105 .

In theory, when we have such a default configuration, we wouldn't need the level property on each result, as the specification defines precisely how to derive the value (https://docs.oasis-open.org/sarif/sarif/v2.1.0/errata01/os/sarif-v2.1.0-errata01-os-complete.html#_Toc141790898).
However, we could still keep it, as it makes it easier for the tools (e.g. VS Code's SARIF Viewer recognizes the defaultConfiguration, while IntelliJ's build-in Service-Side Analysis tool window doesn't seem to).

pmd-core/src/main/java/net/sourceforge/pmd/renderers/internal/sarif/SarifLog.java

pmd-core/src/main/java/net/sourceforge/pmd/renderers/internal/sarif/SarifLogBuilder.java

julees7 · 2025-03-22T12:05:35Z

@adangel So I added the ReportingConfiguration as a Class to be used for the attribute "defaultConfiguration", but I need more information on what this should be filled with or where ^^'

adangel · 2025-03-25T19:20:33Z

@adangel So I added the ReportingConfiguration as a Class to be used for the attribute "defaultConfiguration", but I need more information on what this should be filled with or where ^^'

Let's start with "where": the defaultConfiguration property is part of the reportingDescriptor object - which is created when the SarifLogBuilder processes a rule violation at

pmd/pmd-core/src/main/java/net/sourceforge/pmd/renderers/internal/sarif/SarifLogBuilder.java

Lines 44 to 46 in 901d976

    
           public SarifLogBuilder add(RuleViolation violation) { 
        
               final ReportingDescriptor ruleDescriptor = getReportingDescriptor(violation); 
        
               int ruleIndex = rules.indexOf(ruleDescriptor);

That means, you need to create a new defaultConfiguration object here

pmd/pmd-core/src/main/java/net/sourceforge/pmd/renderers/internal/sarif/SarifLogBuilder.java

Lines 170 to 179 in 901d976

    
           private ReportingDescriptor getReportingDescriptor(RuleViolation rv) { 
        
               return ReportingDescriptor.builder() 
        
                   .id(rv.getRule().getName()) 
        
                   .shortDescription(new MultiformatMessage(rv.getDescription())) 
        
                   .fullDescription(new MultiformatMessage(rv.getRule().getDescription())) 
        
                   .helpUri(rv.getRule().getExternalInfoUrl()) 
        
                   .help(new MultiformatMessage(rv.getRule().getDescription())) 
        
                   .properties(getRuleProperties(rv)) 
        
                   .build(); 
        
           }

However, before you can add it to the reportingDescriptor builder, you need to add the property "defaultConfiguration" to the ReportingDescriptor class along with a setter in the builder:

pmd/pmd-core/src/main/java/net/sourceforge/pmd/renderers/internal/sarif/SarifLog.java

Line 2067 in 08fb81a

public static class ReportingDescriptor {

and

pmd/pmd-core/src/main/java/net/sourceforge/pmd/renderers/internal/sarif/SarifLog.java

Line 2119 in 08fb81a

public static class ReportingDescriptorBuilder {

The "what" is not so much different than what you already did: In getReportingDescriptor() you have access to the rule's priority setting via rv.getRule().getPriority() that you need to map to "error", "warning", "note".

pmd-core/src/main/java/net/sourceforge/pmd/renderers/internal/sarif/SarifLog.java

Pankraz76

Benefits of Fully Qualified Imports in Java
we should consider using https://checkstyle.sourceforge.io/checks/whitespace/emptylineseparator.html

pmd-core/src/main/java/net/sourceforge/pmd/renderers/internal/sarif/SarifLog.java

bdovaz · 2025-04-14T06:49:18Z

How is this PR going? I am interested in this change as well.

Thanks.

github-actions · 2025-04-14T18:35:24Z

Documentation Preview

Compared to main:
This changeset changes 0 violations,
introduces 0 new violations, 0 new errors and 0 new configuration errors,
removes 0 violations, 0 errors and 0 configuration errors.

Regression Tester Report

adangel

Thanks for the PR!

I'm going to fix one bug before I merge this: The enabled property of the defaultConfiguration should either be true or not specified at all. Currently it is false, which is irritating, as the report definitely contains a result for this rule but the rule seems to be disabled...

pmd-core/src/test/resources/net/sourceforge/pmd/renderers/sarif/expected.sarif.json

docs/report-examples/pmd-report.sarif.json

pmd-core/src/main/java/net/sourceforge/pmd/renderers/internal/sarif/SarifLogBuilder.java

pmd-core/src/main/java/net/sourceforge/pmd/renderers/internal/sarif/SarifLog.java

Merge pull request #5573 from julees7:main

#5525 adding sarif level property

c8d786d

adangel changed the title ~~[core] Adding Sarif Level Property #5525~~ Fix #5525: [core] Add Sarif Level Property Mar 6, 2025

adangel requested changes Mar 6, 2025

View reviewed changes

pmd-core/src/main/java/net/sourceforge/pmd/renderers/internal/sarif/SarifLog.java Outdated Show resolved Hide resolved

pmd-core/src/main/java/net/sourceforge/pmd/renderers/internal/sarif/SarifLogBuilder.java Show resolved Hide resolved

bdovaz mentioned this pull request Mar 6, 2025

Retrieve SARIF errors and warnings correctly oxsecurity/megalinter#4837

Merged

#5525 adding ReportingConfiguration

9873b70

adangel reviewed Mar 25, 2025

View reviewed changes

pmd-core/src/main/java/net/sourceforge/pmd/renderers/internal/sarif/SarifLog.java Outdated Show resolved Hide resolved

adangel reviewed Mar 25, 2025

View reviewed changes

pmd-core/src/main/java/net/sourceforge/pmd/renderers/internal/sarif/SarifLog.java Outdated Show resolved Hide resolved

Pankraz76 reviewed Apr 1, 2025

View reviewed changes

Pankraz76 mentioned this pull request Apr 1, 2025

[core] Cleanup SarifLog.java #5662

Closed

3 tasks

julees7 added 2 commits April 14, 2025 14:57

#5525 clean up requested by @Pankraz76

94049d4

#5525 filled defaultConfig

1d83b26

#5525 fixed failing tests

ad0d8d7

julees7 force-pushed the main branch from 8dc41fb to ad0d8d7 Compare April 15, 2025 12:38

julees7 requested a review from adangel April 16, 2025 19:52

adangel added this to the 7.13.0 milestone Apr 17, 2025

adangel self-assigned this Apr 17, 2025

adangel added 3 commits April 18, 2025 10:22

[core] Sarif: fix default value for ReportConfiguration.enabled

18ce6dd

[doc] Add defaultConfiguration to example sarif report

f9c3bcb

[core] SarifLogBuilder - use RulePriority

89195d1

adangel approved these changes Apr 18, 2025

View reviewed changes

adangel added a commit that referenced this pull request Apr 18, 2025

[doc] Update release notes (#5525, #5573)

88ac8d5

adangel merged commit 89195d1 into pmd:main Apr 18, 2025
8 checks passed

adangel added a commit that referenced this pull request Apr 18, 2025

Fix #5525: [core] Add Sarif Level Property (#5573)

b8d9724

Merge pull request #5573 from julees7:main

Uh oh!

Fix #5525: [core] Add Sarif Level Property #5573

Fix #5525: [core] Add Sarif Level Property #5573

Uh oh!

Conversation

julees7 commented Mar 5, 2025 • edited by adangel Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Describe the PR

Related issues

Ready?

Uh oh!

adangel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

julees7 commented Mar 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adangel commented Mar 25, 2025

Uh oh!

Uh oh!

Uh oh!

Pankraz76 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bdovaz commented Apr 14, 2025

Uh oh!

github-actions bot commented Apr 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adangel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

julees7 commented Mar 5, 2025 •

edited by adangel

Loading

julees7 commented Mar 22, 2025 •

edited

Loading

github-actions bot commented Apr 14, 2025 •

edited

Loading