Issue #9889: Prevent usage of 'getFileContents()' method in code by haanhvu · Pull Request #9909 · checkstyle/checkstyle

haanhvu · 2021-04-21T09:44:22Z

romani · 2021-04-22T13:13:40Z

Wercker Failure:

[INFO] There are 2 errors reported by Checkstyle 8.42-SNAPSHOT with ../../../config/checkstyle_checks.xml ruleset.
[ERROR] src/main/java/com/github/sevntu/checkstyle/checks/coding/TernaryPerExpressionCountCheck.java:[239] (extension) noUsageOfGetFileContents()Method: The usage of getFileContents() method is not allowed
[ERROR] src/main/java/com/github/sevntu/checkstyle/checks/coding/TernaryPerExpressionCountCheck.java:[256] (extension) noUsageOfGetFileContents()Method: The usage of getFileContents() method is not allowed
[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE

We need to set similar suppression in sevntu project.
Please send PR to update https://github.com/sevntu-checkstyle/sevntu.checkstyle/blob/master/sevntu-checks/suppressions.xml

config/checkstyle_checks.xml

haanhvu · 2021-04-25T15:58:46Z

Wercker Failure:

[INFO] There are 2 errors reported by Checkstyle 8.42-SNAPSHOT with ../../../config/checkstyle_checks.xml ruleset.
[ERROR] src/main/java/com/github/sevntu/checkstyle/checks/coding/TernaryPerExpressionCountCheck.java:[239] (extension) noUsageOfGetFileContents()Method: The usage of getFileContents() method is not allowed
[ERROR] src/main/java/com/github/sevntu/checkstyle/checks/coding/TernaryPerExpressionCountCheck.java:[256] (extension) noUsageOfGetFileContents()Method: The usage of getFileContents() method is not allowed
[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE

We need to set similar suppression in sevntu project.
Please send PR to update https://github.com/sevntu-checkstyle/sevntu.checkstyle/blob/master/sevntu-checks/suppressions.xml

PR sent

rnveach · 2021-04-25T23:06:18Z

config/checkstyle_checks.xml

    </module>
+    <module name="RegexpSinglelineJava">
+      <property name="id" value="noUsageOfGetFileContents()Method"/>
+      <property name="format" value="getFileContents()"/>


Since this is regexp, don't we have to sanitize the () since we want to match text? IE: \(\)

rnveach · 2021-04-25T23:08:13Z

config/suppressions.xml

+  <suppress id="noUsageOfGetFileContents()Method" files="SuppressionCommentFilter.java"/>
+  <suppress id="noUsageOfGetFileContents()Method" files="SuppressWithNearbyCommentFilter.java"/>
+  <suppress id="noUsageOfGetFileContents()Method" files="XpathQueryGenerator.java"/>
+  <suppress id="noUsageOfGetFileContents()Method" files="AbstractFileSetCheck.java"/>


This is just a comment.

This is an example of a benefit using java check over reg exp.

src/main/java/com/puppycrawl/tools/checkstyle/api/AbstractFileSetCheck.java
137: protected final FileContents getFileContents() {

This is a method declaration and not an usage of the method.

Very good point.
Looks like we need xpath Check for this, as we need prevent usage of such method only classes that extend AbstractCheck that uses for all java Checks.

This is just a comment.

This is an example of a benefit using java check over reg exp.

src/main/java/com/puppycrawl/tools/checkstyle/api/AbstractFileSetCheck.java
137: protected final FileContents getFileContents() {

This is a method declaration and not an usage of the method.

Ok I dived deeper into the code and see what you mean here. From my understanding, XPath check is the strictest and the most secure one for this case. I vote for it. Do you all agree?

Either java check or xpath is fine.

Either java check or xpath is fine.

ok seems like @romani agrees on xPath too.

Should I close this PR or amend it the XPath way?

Go ahead and amend it. Just keep it rebased on the latest master.

here is example of method detection by Xpath - https://github.com/checkstyle/checkstyle/pull/9939/files#diff-3a0a81e04acf0df0c9c8adefc1cae87e39da88c3141a7c131bcaf7a8ab12c62cR421

but in your case you need to make sure that classes are AbstractCheck to show violation, all other usages should be allowed.

@romani hey AbstractCheck has some abstract subclasses (AbstractSuperCheck, AbstractJavadocCheck, AbstractClassCouplingCheck,...)

We need to forbid the usage of getFileContents() in any classes that extend AbstractCheck and any classes that extend its abstract subclasses. Or otherwise stated, any classes that extend superclasses identified with texts "Abstract" and "Check".

Do I understand correctly?

@haanhvu Please work on creating the xpath to match method calls we want to prevent.
Once that is working, we'll look at the areas it is flagging.

haanhvu · 2021-04-30T07:06:35Z

@romani @rnveach I amended the PR the XPath way. Please review.

Before suppression:

[ERROR] [checkstyle] [ERROR] C:\Users\Admin\Documents\Github\checkstyle\src\main\java\com\puppycrawl\tools\checkstyle\TreeWalker.java:54:1: Avoid using getFileContents() method in                                             AbstractCheck subclasses. [noUsageOfGetFileContents()Method]
[ERROR] [checkstyle] [ERROR] C:\Users\Admin\Documents\Github\checkstyle\src\main\java\com\puppycrawl\tools\checkstyle\checks\AvoidEscapedUnicodeCharactersCheck.java:166:1: Avoid using getFileContents() method in                                             AbstractCheck subclasses. [noUsageOfGetFileContents()Method]
[ERROR] [checkstyle] [ERROR] C:\Users\Admin\Documents\Github\checkstyle\src\main\java\com\puppycrawl\tools\checkstyle\checks\OuterTypeFilenameCheck.java:85:1: Avoid using getFileContents() method in                                             AbstractCheck subclasses. [noUsageOfGetFileContents()Method]
[ERROR] [checkstyle] [ERROR] C:\Users\Admin\Documents\Github\checkstyle\src\main\java\com\puppycrawl\tools\checkstyle\checks\annotation\MissingOverrideCheck.java:169:1: Avoid using getFileContents() method in                                             AbstractCheck subclasses. [noUsageOfGetFileContents()Method]
[ERROR] [checkstyle] [ERROR] C:\Users\Admin\Documents\Github\checkstyle\src\main\java\com\puppycrawl\tools\checkstyle\checks\annotation\PackageAnnotationCheck.java:79:1: Avoid using getFileContents() method in                                             AbstractCheck subclasses. [noUsageOfGetFileContents()Method]
[ERROR] [checkstyle] [ERROR] C:\Users\Admin\Documents\Github\checkstyle\src\main\java\com\puppycrawl\tools\checkstyle\checks\coding\FallThroughCheck.java:189:1: Avoid using getFileContents() method in                                             AbstractCheck subclasses. [noUsageOfGetFileContents()Method]
[ERROR] [checkstyle] [ERROR] C:\Users\Admin\Documents\Github\checkstyle\src\main\java\com\puppycrawl\tools\checkstyle\checks\coding\PackageDeclarationCheck.java:103:1: Avoid using getFileContents() method in                                             AbstractCheck subclasses. [noUsageOfGetFileContents()Method]
[ERROR] [checkstyle] [ERROR] C:\Users\Admin\Documents\Github\checkstyle\src\main\java\com\puppycrawl\tools\checkstyle\checks\imports\ImportControlCheck.java:449:1: Avoid using getFileContents() method in                                             AbstractCheck subclasses. [noUsageOfGetFileContents()Method]
[ERROR] [checkstyle] [ERROR] C:\Users\Admin\Documents\Github\checkstyle\src\main\java\com\puppycrawl\tools\checkstyle\checks\imports\UnusedImportsCheck.java:116:1: Avoid using getFileContents() method in                                             AbstractCheck subclasses. [noUsageOfGetFileContents()Method]
[ERROR] [checkstyle] [ERROR] C:\Users\Admin\Documents\Github\checkstyle\src\main\java\com\puppycrawl\tools\checkstyle\checks\javadoc\JavadocMethodCheck.java:289:1: Avoid using getFileContents() method in                                             AbstractCheck subclasses. [noUsageOfGetFileContents()Method]
[ERROR] [checkstyle] [ERROR] C:\Users\Admin\Documents\Github\checkstyle\src\main\java\com\puppycrawl\tools\checkstyle\checks\javadoc\JavadocStyleCheck.java:307:1: Avoid using getFileContents() method in                                             AbstractCheck subclasses. [noUsageOfGetFileContents()Method]
[ERROR] [checkstyle] [ERROR] C:\Users\Admin\Documents\Github\checkstyle\src\main\java\com\puppycrawl\tools\checkstyle\checks\javadoc\JavadocTypeCheck.java:203:1: Avoid using getFileContents() method in                                             AbstractCheck subclasses. [noUsageOfGetFileContents()Method]
[ERROR] [checkstyle] [ERROR] C:\Users\Admin\Documents\Github\checkstyle\src\main\java\com\puppycrawl\tools\checkstyle\checks\javadoc\JavadocVariableCheck.java:176:1: Avoid using getFileContents() method in                                             AbstractCheck subclasses. [noUsageOfGetFileContents()Method]
[ERROR] [checkstyle] [ERROR] C:\Users\Admin\Documents\Github\checkstyle\src\main\java\com\puppycrawl\tools\checkstyle\checks\javadoc\MissingJavadocMethodCheck.java:265:1: Avoid using getFileContents() method in                                             AbstractCheck subclasses. [noUsageOfGetFileContents()Method]
[ERROR] [checkstyle] [ERROR] C:\Users\Admin\Documents\Github\checkstyle\src\main\java\com\puppycrawl\tools\checkstyle\checks\javadoc\MissingJavadocPackageCheck.java:73:1: Avoid using getFileContents() method in                                             AbstractCheck subclasses. [noUsageOfGetFileContents()Method]
[ERROR] [checkstyle] [ERROR] C:\Users\Admin\Documents\Github\checkstyle\src\main\java\com\puppycrawl\tools\checkstyle\checks\javadoc\MissingJavadocTypeCheck.java:163:1: Avoid using getFileContents() method in                                             AbstractCheck subclasses. [noUsageOfGetFileContents()Method]
[ERROR] [checkstyle] [ERROR] C:\Users\Admin\Documents\Github\checkstyle\src\main\java\com\puppycrawl\tools\checkstyle\checks\javadoc\WriteTagCheck.java:185:1: Avoid using getFileContents() method in                                             AbstractCheck subclasses. [noUsageOfGetFileContents()Method]
[ERROR] [checkstyle] [ERROR] C:\Users\Admin\Documents\Github\checkstyle\src\main\java\com\puppycrawl\tools\checkstyle\checks\regexp\RegexpCheck.java:473:1: Avoid using getFileContents() method in                                             AbstractCheck subclasses. [noUsageOfGetFileContents()Method]
[ERROR] [checkstyle] [ERROR] C:\Users\Admin\Documents\Github\checkstyle\src\main\java\com\puppycrawl\tools\checkstyle\checks\regexp\RegexpSinglelineJavaCheck.java:209:1: Avoid using getFileContents() method in                                             AbstractCheck subclasses. [noUsageOfGetFileContents()Method]
[ERROR] [checkstyle] [ERROR] C:\Users\Admin\Documents\Github\checkstyle\src\main\java\com\puppycrawl\tools\checkstyle\checks\sizes\MethodLengthCheck.java:103:1: Avoid using getFileContents() method in                                             AbstractCheck subclasses. [noUsageOfGetFileContents()Method]
[ERROR] [checkstyle] [ERROR] C:\Users\Admin\Documents\Github\checkstyle\src\main\java\com\puppycrawl\tools\checkstyle\checks\whitespace\EmptyLineSeparatorCheck.java:328:1: Avoid using getFileContents() method in                                             AbstractCheck subclasses. [noUsageOfGetFileContents()Method]
[ERROR] [checkstyle] [ERROR] C:\Users\Admin\Documents\Github\checkstyle\src\main\java\com\puppycrawl\tools\checkstyle\meta\JavadocMetadataScraper.java:51:1: Avoid using getFileContents() method in                                             AbstractCheck subclasses. [noUsageOfGetFileContents()Method]
[ERROR] [checkstyle] [ERROR] C:\Users\Admin\Documents\Github\checkstyle\src\test\java\com\puppycrawl\tools\checkstyle\CheckerTest.java:1703:5: Avoid using getFileContents() method in                                             AbstractCheck subclasses. [noUsageOfGetFileContents()Method]

After suppression:
BUILD SUCCESS

config/checkstyle_checks.xml

romani

Items:

src/main/java/com/puppycrawl/tools/checkstyle/api/AbstractCheck.java

rnveach · 2021-07-27T22:54:51Z

@haanhvu Are you going to continue this PR and fix the CI failure?

haanhvu · 2021-07-28T03:49:13Z

@haanhvu Are you going to continue this PR and fix the CI failure?

Yes, I will and I want to. Sorry it's been crazy weeks here. Can you show me more about the context of the failure and what I can do to fix it?

rnveach · 2021-08-03T18:14:51Z

The error printing in the CI is a checkstyle error in another repo.

pipeline/source/.ci-temp/methods-distance/methods-distance/src/main/java/com/github/sevntu/checkstyle/module/MethodCallDependencyCheckstyleModule.java:39:1: Avoid using getFileContents method in AbstractCheck subclasses. [noUsageOfGetFileContentsMethod]

https://github.com/sevntu-checkstyle/methods-distance
You will need to send a PR and correct the error.

I also recommend rebasing on the latest master.

rnveach · 2021-08-25T20:52:47Z

@haanhvu ping

haanhvu · 2021-08-31T12:28:46Z

@haanhvu ping

yeah I'm working on it

nrmancuso · 2021-11-29T16:26:19Z

@haanhvu any progress on this PR?

haanhvu · 2021-11-29T16:28:15Z

@nmancus1 I'm finishing it this week. Sorry for the delay.

romani · 2021-12-18T22:26:45Z

@haanhvu , please rebase on latest code, medthod-distance PR is merged

rnveach · 2021-12-31T20:10:41Z

Rebased.

rnveach · 2022-01-08T19:34:23Z

@romani
Sonar fails because of the marked deprecated method in this PR.

TC fails from the usage of the deprecated methods. How do we suppress these in TC?

romani · 2022-01-08T22:30:29Z

there is not way to suppress this in inspection configuration

so it mean that we need to suppress it by javadoc tags with comment until ...
I placed suppression to all, I hope we will address them at #11166
Sonar issues should be marked as "Wontfix until #11166"

… code

haanhvu force-pushed the issue#9889 branch 2 times, most recently from 29dd515 to 75dee4e Compare April 22, 2021 07:08

rnveach reviewed Apr 23, 2021

View reviewed changes

config/checkstyle_checks.xml Outdated Show resolved Hide resolved

haanhvu mentioned this pull request Apr 25, 2021

config: set suppression for noUsageOfGetFileContents()Method sevntu-checkstyle/sevntu.checkstyle#839

Merged

haanhvu force-pushed the issue#9889 branch from 75dee4e to 6aedc6b Compare April 25, 2021 10:32

haanhvu force-pushed the issue#9889 branch 2 times, most recently from b5b74ed to 61f57dc Compare April 25, 2021 16:22

romani approved these changes Apr 25, 2021

View reviewed changes

romani assigned rnveach Apr 25, 2021

rnveach suggested changes Apr 25, 2021

View reviewed changes

haanhvu marked this pull request as draft April 28, 2021 07:36

haanhvu force-pushed the issue#9889 branch from a1bc1e0 to 13df984 Compare April 28, 2021 07:42

haanhvu closed this Apr 28, 2021

haanhvu force-pushed the issue#9889 branch from 13df984 to 62f0040 Compare April 28, 2021 07:43

haanhvu reopened this Apr 28, 2021

haanhvu force-pushed the issue#9889 branch 5 times, most recently from ff3fe56 to 682128f Compare April 30, 2021 05:31

haanhvu marked this pull request as ready for review April 30, 2021 06:56

haanhvu requested review from rnveach and romani April 30, 2021 06:57

haanhvu force-pushed the issue#9889 branch from 682128f to 17d39e6 Compare May 3, 2021 14:34

rnveach previously requested changes May 5, 2021

View reviewed changes

config/checkstyle_checks.xml Outdated Show resolved Hide resolved

rnveach assigned romani and unassigned rnveach May 6, 2021

rnveach added the awaiting changes label Jun 2, 2021

haanhvu force-pushed the issue#9889 branch 2 times, most recently from 4d32bd1 to 6fed1de Compare June 11, 2021 13:47

haanhvu marked this pull request as draft June 11, 2021 14:08

haanhvu force-pushed the issue#9889 branch 4 times, most recently from c02ef62 to a1e7309 Compare June 13, 2021 11:58

romani requested changes Jun 13, 2021

View reviewed changes

src/main/java/com/puppycrawl/tools/checkstyle/api/AbstractCheck.java Show resolved Hide resolved

haanhvu force-pushed the issue#9889 branch 2 times, most recently from 99557f3 to a5dd751 Compare June 16, 2021 09:16

haanhvu force-pushed the issue#9889 branch from a5dd751 to d69b029 Compare August 31, 2021 12:46

haanhvu mentioned this pull request Dec 5, 2021

minor: set suppression for noUsageOfGetFileContentsMethod sevntu-checkstyle/methods-distance#64

Merged

haanhvu marked this pull request as ready for review December 5, 2021 11:17

rnveach force-pushed the issue#9889 branch from d69b029 to 6765366 Compare December 31, 2021 20:10

Issue checkstyle#9889: Prevent usage of 'getFileContents()' method in…

1439170

… code

romani force-pushed the issue#9889 branch from 6765366 to 1439170 Compare January 8, 2022 23:04

rnveach approved these changes Jan 8, 2022

View reviewed changes

romani merged commit 769f4f7 into checkstyle:master Jan 9, 2022

Uh oh!

Conversation

haanhvu commented Apr 21, 2021

Uh oh!

romani commented Apr 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

haanhvu commented Apr 25, 2021

Uh oh!

rnveach Apr 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rnveach Apr 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

romani Apr 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

haanhvu Apr 26, 2021

Choose a reason for hiding this comment

Uh oh!

rnveach Apr 26, 2021

Choose a reason for hiding this comment

Uh oh!

haanhvu Apr 26, 2021

Choose a reason for hiding this comment

Uh oh!

rnveach Apr 26, 2021

Choose a reason for hiding this comment

Uh oh!

romani Apr 27, 2021

Choose a reason for hiding this comment

Uh oh!

haanhvu Apr 28, 2021

Choose a reason for hiding this comment

Uh oh!

rnveach Apr 29, 2021

Choose a reason for hiding this comment

Uh oh!

haanhvu commented Apr 30, 2021

Uh oh!

Uh oh!

romani left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rnveach commented Jul 27, 2021

Uh oh!

haanhvu commented Jul 28, 2021

Uh oh!

rnveach commented Aug 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rnveach commented Aug 25, 2021

Uh oh!

haanhvu commented Aug 31, 2021

Uh oh!

nrmancuso commented Nov 29, 2021

Uh oh!

haanhvu commented Nov 29, 2021

Uh oh!

romani commented Dec 18, 2021

Uh oh!

rnveach commented Dec 31, 2021

Uh oh!

rnveach commented Jan 8, 2022

Uh oh!

romani commented Jan 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

romani commented Apr 22, 2021 •

edited

Loading

rnveach Apr 25, 2021 •

edited

Loading

rnveach Apr 25, 2021 •

edited

Loading

romani Apr 26, 2021 •

edited

Loading

rnveach commented Aug 3, 2021 •

edited

Loading

romani commented Jan 8, 2022 •

edited

Loading