Add ignoreMethodAnnotatedBy property to ParameterNumberCheck

[tfij]

Check name: ParameterNumberCheck

https://checkstyle.org/checks/sizes/parameternumber.html#ParameterNumber

Problem

https://www.baeldung.com/jackson-annotations#1-jsoncreator
https://fasterxml.github.io/jackson-annotations/javadoc/2.7/com/fasterxml/jackson/annotation/JsonCreator.html
If I implement external API, I have no power over the number of parameters, e.g. when creating a DTO class for REST API and deserializing JSON to an object by constructor annotated by @JsonCreator

class MyDto {
  private final String a1;
  private final String a2;
  private final String a3;
  private final String a4;
  private final String a5;
  private final String a6;
  private final String a7;
  private final String a8;
  private final String a9;

  @JsonCreator
  MyDto(String a1, String a2, String a3, String a4, String a5, String a6, String a7, String a8, String a9) {
    this.a1 = a1;
    this.a2 = a2;
    this.a3 = a3;
    this.a4 = a4;
    this.a5 = a5;
    this.a6 = a6;
    this.a7 = a7;
    this.a8 = a8;
    this.a9 = a9;
  }
}

Solution

I'd like to configure check to ignore this constructor. In general, ignore all methods annotated by @JsonCreator, e.g.

 <module name="ParameterNumber">
   <property name="max" value="5"/>
   <property name="ignoreMethodAnnotatedBy" value="JsonCreator"/>
 </module>

The PR with the proposal is here #11339

Check already has property ignoreOverriddenMethods so new property will make it more general.

[bgalek]

Nice!

[nrmancuso]

Supression of constructors with this annotation can be achieved via SuppresionXpathSingleFilter:

➜  java cat Test.java 
public class Test {
    private final String a1;
    private final String a2;
    private final String a3;
    private final String a4;
    private final String a5;
    private final String a6;
    private final String a7;
    private final String a8;
    private final String a9;

    @JsonCreator
    Test(String a1, String a2, String a3, String a4, String a5, String a6, String a7, String a8, String a9) {
        this.a1 = a1;
        this.a2 = a2;
        this.a3 = a3;
        this.a4 = a4;
        this.a5 = a5;
        this.a6 = a6;
        this.a7 = a7;
        this.a8 = a8;
        this.a9 = a9;
    }
}

@interface JsonCreator {}

➜  java javac Test.java

➜  java cat config.xml 
<?xml version="1.0"?>
<!DOCTYPE module PUBLIC
        "-//Puppy Crawl//DTD Check Configuration 1.3//EN"
        "http://www.puppycrawl.com/dtds/configuration_1_3.dtd">

<module name="Checker">
    <property name="charset" value="UTF-8"/>
    <module name="TreeWalker">
        <module name="ParameterNumber">
            <property name="max" value="5"/>
        </module>
        <module name="SuppressionXpathSingleFilter">
            <property name="checks" value="ParameterNumber"/>
            <property name="query"
                      value="//CTOR_DEF/IDENT[preceding-sibling::MODIFIERS/ANNOTATION/IDENT[@text='JsonCreator']]"/>
        </module>

    </module>
</module>

➜  java java -jar checkstyle-9.3-all.jar -c config.xml Test.java
Starting audit...
Audit done.

@tfij we typically try to avoid adding properties to checks when we can handle suppression of special cases with some filter. If you prefer to use a suppression file, you can use the same XPath with https://checkstyle.sourceforge.io/config_filters.html#SuppressionXpathFilter.

XPath can also be extended by the | operator to cover method definitions:

➜  java cat Test.java
public class Test {
    private final String a1;
    private final String a2;
    private final String a3;
    private final String a4;
    private final String a5;
    private final String a6;
    private final String a7;
    private final String a8;
    private final String a9;

    @JsonCreator
    Test(String a1, String a2, String a3, String a4, String a5, String a6, String a7, String a8, String a9) {
        this.a1 = a1;
        this.a2 = a2;
        this.a3 = a3;
        this.a4 = a4;
        this.a5 = a5;
        this.a6 = a6;
        this.a7 = a7;
        this.a8 = a8;
        this.a9 = a9;
    }

    @JsonCreator
    void someMethod(int a, int b, int c, int d, int e, int f) {}
}

@interface JsonCreator {}

➜  java javac Test.java

➜  java cat config.xml 
<?xml version="1.0"?>
<!DOCTYPE module PUBLIC
        "-//Puppy Crawl//DTD Check Configuration 1.3//EN"
        "http://www.puppycrawl.com/dtds/configuration_1_3.dtd">

<module name="Checker">
    <property name="charset" value="UTF-8"/>
    <module name="TreeWalker">
        <module name="ParameterNumber">
            <property name="max" value="5"/>
        </module>
        <module name="SuppressionXpathSingleFilter">
            <property name="checks" value="ParameterNumber"/>
            <property name="query"
                      value="//CTOR_DEF/IDENT[preceding-sibling::MODIFIERS/ANNOTATION/IDENT[@text='JsonCreator']]
                            | //METHOD_DEF/IDENT[preceding-sibling::MODIFIERS/ANNOTATION/IDENT[@text='JsonCreator']]"/>
        </module>

    </module>
</module>

➜  java java -jar checkstyle-9.3-all.jar -c config.xml Test.java
Starting audit...
Audit done.

[romani]

@nmancus1 , @rnveach , @strkkk , @pbludov ,
lets think/vote/re-consider on our decision to avoid extra suppresion/ignore properties. We use to make this decision as we had no time to provide updates to all user requests, so Xpath is definitely workaround for user, but it is not very good for configurations as it imply two modules that are hard to keep together and manage later on.

in the same time, updating Checks to have such properties might also be not that good, as potentially all Checks might need something like this. And it is magic of certain library that not everyone is using. But in the same time instrumenting code with magic annotations is definitely a trend.

[nrmancuso]

As for me, I prefer to keep checks simple, and only add properties if they cannot be covered by some reasonable/ reliable XPath or other filter. So,

avoid extra suppresion/ignore properties

would be my vote.

As experience has shown, checks with lots of properties become harder to maintain; not just from a simple code standpoint, but also the logic of how the properties interact with each other.

Just as a related note - I think that we need to keep a "repository" of sorts (or entire website page) for all of the XPath related modules that we create, to help users more readily find what they need (with examples).

[romani]

usually we resolved such cases, by example in documentation (xdoc) on how to handle frequent edge cases, so googling usually helps to find solution.

[tfij]

I agree with @romani

it imply two modules that are hard to keep together and manage later on

How about making it possible to set suppress property for any check configuration? In this case

<module name="ParameterNumber">
    <property name="max" value="5"/>
    <suppress-check query="//CTOR_DEF/IDENT[preceding-sibling::MODIFIERS/ANNOTATION/IDENT[@text='JsonCreator']]
                            | //METHOD_DEF/IDENT[preceding-sibling::MODIFIERS/ANNOTATION/IDENT[@text='JsonCreator']]" />
</module>

or something like this.

It will solve a problem with two modules maintained separately.

It is a generic solution, consistent for all checks and can reuse some existing code.

[romani]

we did not make it like this few years ago, as implementation of Xpath was not mature enough, we still have items to finish to make it for all Checks. May be we can start sooner, somebody need to do deep thinking on this.

Even we will have this option, it is not user friendly approach and can help users for edge cases and workaround some defects. But we still need to think on most common ways to suppress and allow them to be done by Check properties. This border of xpath to Check special property is not defined by us and not clear for us.

[pbludov]

I am against complicating the check. Such property will fix this particular issue, but it won't work for another.
Suppressions are more flexible.

[rnveach]

I think I might like the idea of adding suppressions to the check itself, locking the 2 together ( #11340 (comment) ). The only downside to that is there would need to be some type of double checking that this new suppression is only suppressing the violations of this check and not accidentally picking up some other check by user configuration error.

I am probably more extreme in that we have ignoreOverriddenMethods which is already looking at annotations. Why don't we just deprecate that property and add a new one that extends it to all annotation and then the user can add @Override or any other annotation. (ignoreOverriddenMethods is default to false, so we would continue that default in the new property). To me this makes more sense as it allows, like this issue wants, a more broader usage.

Otherwise, I don't think I can get behind having the 2 options live together in the same check.

[romani]

Deprecation is long for us and painful process for users. Better to keep two options and let them work by same code inside.

Xpath is too brutal to user, it tooks awhile for users to understand how to do this, and how to make xpath expression that is required. @tfij, imagine we did not give you xpath expression, how much time you will spend to make it ?

But in the same time Lombok like libraries become very popular way of codding now. So suppression by annotation might be well used approach.

[tfij]

Agree, xpath is very mysterious. Mainly because I don't know what XML structure I'm scanning.
I imagine that if user hadn't found a ready or similar solution in the documentation or on stackoverflow, it would take hours to work out this by themself.

[nrmancuso]

After reading through the discussion here, and thinking on this some more, I have reconsidered my general position on check properties. If a new check property that is a “feature” is proposed, I stay with my original position. But - if new property is some simple suppression (i.e. checking for a particular annotation) , we should consider it. Most of the time, this will just require some Boolean method used to guard the log method.

We do not want to make Checkstyle difficult to configure or inflexible, this will only frustrate users.

[romani]

I more care about users experience, implementation pain is limited to few people during PR and eventually forgotten. We use/reconfigure checkstyle more than we spend time to code it.

Xpath suppression was good approach to resolve problem on user side quickly, temporary hack. And it might continue to serve this purpose.

But custom ignore properties are a way more easy to use.

[romani]

Ignore by annotation will be most used ignore/suppression approach. As annotation imply some third-party features/magic that happening in runtime or compilation time,
Checkstyle is not working at that time, so we have to skip validation in some cases.

We did not have time to implement ignore properties in Checks, that is why we invested more in xpath to let users has workaround sooner, but if implementation is provided by community, I think it is good to accept it. Especially if we see that such ignore approach can be widely reused.

Just example of property to ignore: 0b3a260 that is named "exclude" but meaning is the same as ignore/suppress.

[nrmancuso]

Let's make a decision here, in regards to this particular issue. If we want to continue discussion about adding check properties in general, let's create a Github discussion and continue there.

After thinking on @romani 's comments above, I am for approval of this issue.

[romani]

One more example of Lombok usage that cause unwanted violation #11415

[tfij]

Hi, is there anything I can do to help make the decision?
I hope it will be the approval of this proposal :)

[romani]

@tfij, can I ask you to help :
We need to review all Checks that has same property and list all of them, and we need to choose a good name from existing or just new.

For now I think we should name it like: ignoreAnnothedBy or something similar. We should avoid usage of "method" or similar words to let it be used for any Check target.

Can we also analyze what Checks are highly likely to have such property in future? And what Check unlikely to change behavior by annotated target.

[romani]

one more example how annotation make false positive for static analisys - #11481
pmd using property ignoredAnnotations

[tfij]

@romani I can handle it next week.

One question, by

review all Checks that has same property and list all of them

Do you mean some kind of grid of all checks? or some subset of checks? What format do you expect?

[romani]

All Checks, any readable format, to see what we have already

[romani]

 <module name="ParameterNumber">
   <property name="max" value="5"/>
   <property name="ignoreAnnotatedBy" value="JsonCreator"/>
 </module>

sounds like ignore parameter that is annotated by, not a method. But check is actually targeting method, so it is not confusion.

Sounds like we have to come back to originnal:

 <module name="ParameterNumber">
   <property name="max" value="5"/>
   <property name="ignoreMethodAnnotatedBy" value="JsonCreator"/>
 </module>

to be very explicit where annotation is expected. Ideally user should not read documentation to understand how to configure Check.

@rnveach , please approve.

[tfij]

I'm OK with ignoreMethodAnnotatedBy.
However, you have to make a decision :)

[romani]

@rnveach , @nrmancuso , please review and approve.

@tfij, if no response will be there before 1 january 2023, please mention me here, I will approve issue.

[nrmancuso]

I would prefer this new property to be called ignoreAnnotatedBy for two reasons:

1. ParameterNumber also checks constructors (which is sort of a special method)
2. As we extend this concept to other checks, property name can be consistent and users don’t have to be concerned with details

We could also devise some alternative general naming such as ignoreTargetAnnotatedBy or something similar.

[romani]

Ok, let's move forward with ignoreAnnotatedBy.

[romani]

Everyone is welcome to finish this PR #11339, just reuse code and move this PR across finish line.

@sktpy, do you have time to help us with this issue ?

[sktpy]

Yes, I am on it.

[sktpy]

Label needs to be added (new feature ig)
https://github.com/checkstyle/checkstyle/actions/runs/8361654518/job/22890247968#step:4:130

Also, I am not sure how the release works, but I think this was supposed to be released with 10.15.0 #14553 (comment) but the bot has added it to 10.14.3 milestone.

[nrmancuso]

@sktpy thanks for noticing this, we have added the label. As far as the milestone goes, I think that is automatically recalculated based on what labels were on the issues at the time of release.

[romani]

yes, but maintainers has classify issue after fix/merge and update version by github action

https://github.com/checkstyle/checkstyle/actions/runs/8369880545/job/22916356794#step:4:130

Generation ends with 1 errors.
Error: Validation of release number failed. Release number is a patch(10.14.3), but release notes contain 'new' or 'breaking compatability' labels. Please correct release number by running https://github.com/checkstyle/checkstyle/actions/workflows/bump-version-and-update-milestone.yml . The offending issue(s): #11340
Error: Process completed with exit code 254.

[romani]

done https://github.com/checkstyle/checkstyle/actions/runs/8375045697