Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes NoSuchMethodError on processing errors in pmd-compat6 #4749

Merged
merged 2 commits into from
Dec 7, 2023

Conversation

bergander
Copy link
Contributor

Describe the PR

When a processing error occurs while using pmd-compat6 (rc4) then a NoSuchMethodError is thrown:

[ERROR] Failed to execute goal org.apache.maven.plugins:maven-pmd-plugin:3.21.2:pmd (default-cli) on project flexpay-reflectdb: Execution default-cli of goal org.apache.maven.plugins:maven-pmd-plugin:3.21.2:pmd failed: An API incompatibility was encountered while executing org.apache.maven.plugins:maven-pmd-plugin:3.21.2:pmd: java.lang.NoSuchMethodError: 'java.lang.String net.sourceforge.pmd.Report$ProcessingError.getFile()'

This PR implements the missing getFile() method.

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)

@pmd-test
Copy link

1 Message
📖 No regression tested rules have been changed.

Generated by 🚫 Danger

Copy link
Member

@adangel adangel left a comment

Choose a reason for hiding this comment

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

Thanks for the PR.

Could you please add an integration test (see https://github.com/pmd/pmd/tree/master/pmd-compat6/src/it ), so that we have a reproducer and a verifier, that this fixes the problem and so that it is not reintroduced in the future.

I assume, this is about printing processing errors (here https://github.com/apache/maven-pmd-plugin/blob/9ce35a43fef8f50d84782eb8a960ceb832d386ec/src/main/java/org/apache/maven/plugins/pmd/exec/PmdExecutor.java#L265 ).
One simple way to provoke a processing error (and therefore to reproduce the problem) is by creating a custom ruleset with one custom XPath-based rule, which uses an invalid xpath expression.

pmd-compat6/src/main/java/net/sourceforge/pmd/Report.java Outdated Show resolved Hide resolved
@bergander bergander force-pushed the FixNoSuchMethodErrorInPmdCompat6 branch from ab75622 to 35ec564 Compare November 24, 2023 14:58
@bergander
Copy link
Contributor Author

A rule with an invalid XPath expression will only generate an error row in the log during initialization of the rule:

"Exception while initializing rule , the rule will not be run"

To produce a processing error, the rule needs to be initialized successfully and then throw an exception during execution. So instead I resorted to writing a rule in Java which just throws an exception. And to use it I had to create a test-jar and include it as a dependency to the pmd maven plugin.

Also, ProcessingError.getFile() is only called when using the pmd:pmd goal and not when using pmd:check, so that had to be added as well.

I'm not sure this is the best solution, so feel free to suggest a better approach!

Copy link
Member

@adangel adangel left a comment

Choose a reason for hiding this comment

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

Thanks for the update!

A rule with an invalid XPath expression will only generate an
error row in the log during initialization of the rule:

"Exception while initializing rule , the rule will not be run"

Oh, yes, my bad.

To produce a processing error, the rule needs to be initialized
successfully and then throw an exception during execution. So
instead I resorted to writing a rule in Java which just throws an
exception. And to use it I had to create a test-jar and include it
as a dependency to the pmd maven plugin.

Instead of creating a java based rule, we can still write a XPath rule using the error() function. This ends up as a processing error.
See my comment on the ruleset file.

So we don't need ExceptionThrowingRule and don't need to create a test-jar and this additional dependency.

Also, ProcessingError.getFile() is only called when using the
pmd:pmd goal and not when using pmd:check, so that had to be added
as well.

That's due to MPMD-236 - pmd:check is executing pmd:pmd, but doesn't apply the configuration from the specific execution. See my comment for the pom.xml.

Since we now have a processing error, we need to make sure, that we don't stop maven-pmd-plugin - therefore we need to "skipPmdError":

<skipPmdError>true</skipPmdError> <!-- we want to capture processing errors -->

Otherwise no report we be created and the verification will fail.

Also, we should verify, that the processing error is indeed ending up in the report (in target/pmd.xml). Please add the following lines in verify.bsh (at line 36):

if (!pmdXml.contains("<error filename=\"" + mainFile.getAbsolutePath()) || !pmdXml.contains(mainFile + "\" msg=\"PmdXPathException")) {
    throw new RuntimeException("Processing error has not been reported");
}

I'm not sure this is the best solution, so feel free to suggest a better approach!

I'd prefer to use the XPath-based approach, so that we don't need to create this extra test rule class.

Note: the failing CI build is unrelated to your change, it looks like intermittent NPE (#4757)

pmd-compat6/src/it/pmd-for-java/pom.xml Outdated Show resolved Hide resolved
pmd-compat6/src/it/pmd-for-java/exception_ruleset.xml Outdated Show resolved Hide resolved
@bergander bergander force-pushed the FixNoSuchMethodErrorInPmdCompat6 branch from 35ec564 to f73d75d Compare December 1, 2023 14:30
@adangel adangel added this to the 7.0.0 milestone Dec 7, 2023
Copy link
Member

@adangel adangel left a comment

Choose a reason for hiding this comment

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

Thanks!

adangel added a commit that referenced this pull request Dec 7, 2023
adangel added a commit that referenced this pull request Dec 7, 2023
Fixes NoSuchMethodError on processing errors in pmd-compat6 #4749
@adangel adangel merged commit dcdab9d into pmd:master Dec 7, 2023
3 checks passed
adangel added a commit that referenced this pull request Feb 2, 2024
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.

None yet

3 participants