Skip to content

Allow to set severity per signature #253

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

Merged

Conversation

kwin
Copy link
Contributor

@kwin kwin commented Jan 4, 2025

This closes #252 and #219

@kwin kwin force-pushed the feature/severity-per-signature branch 4 times, most recently from 2a210f3 to b66a66b Compare January 4, 2025 17:38
@kwin
Copy link
Contributor Author

kwin commented Jan 4, 2025

@uschindler I don't know enough about Gradle to add that functionality there as well. Is it acceptable to only allow that parametrisation from Maven for now?

@uschindler
Copy link
Member

I have not checked this at all. You also need to add the command line settings. Gradle is as simple as Maven. Just add getters and setters like in Maven or Ant.

@kwin kwin force-pushed the feature/severity-per-signature branch from b66a66b to 9bfc24c Compare January 6, 2025 18:30
@kwin kwin marked this pull request as ready for review January 6, 2025 18:32
@kwin kwin force-pushed the feature/severity-per-signature branch from 9bfc24c to 962dcdb Compare January 6, 2025 18:32
@kwin
Copy link
Contributor Author

kwin commented Mar 24, 2025

@uschindler Any chance to include this in a release?

@uschindler
Copy link
Member

Oh haven't seen that you updated it for Gradle.... Will need to review

Copy link
Member

@uschindler uschindler left a comment

Choose a reason for hiding this comment

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

This looks good. I like that we now have finally a SuppressionResult. All the methods returning String was a bit of desaster. I wanted to change this since long time, but it was too much work. But you took care of it. Thanks!
There is a small incompatibility with Java 7 (the current minimum version), I will fix it! (Map#getOrDefault() is missing).



private ViolationSeverity getSeverityForKey(String key) {
return severityPerSignature.getOrDefault(key, failOnViolation ? ViolationSeverity.ERROR : ViolationSeverity.WARNING);
Copy link
Member

Choose a reason for hiding this comment

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

getOrDefault does not exist in Java 7, which is still minimum version. Will fix this later.

@uschindler uschindler merged commit 9b4c93f into policeman-tools:main Mar 31, 2025
1 check passed
@uschindler
Copy link
Member

This is the compile failure with Java 7:

compile:
    [mkdir] Created dir: C:\Users\Uwe Schindler\Projects\lucene\forbidden-apis\forbidden-apis\build\main
     [copy] Copying 87 files to C:\Users\Uwe Schindler\Projects\lucene\forbidden-apis\forbidden-apis\build\main
    [javac] Compiling 29 source files to C:\Users\Uwe Schindler\Projects\lucene\forbidden-apis\forbidden-apis\build\main
    [javac] C:\Users\Uwe Schindler\Projects\lucene\forbidden-apis\forbidden-apis\src\main\java\de\thetaphi\forbiddenapis\Signatures.java:451: error: cannot find symbol
    [javac]     return severityPerSignature.getOrDefault(key, failOnViolation ? ViolationSeverity.ERROR : ViolationSeverity.WARNING);
    [javac]                                ^
    [javac]   symbol:   method getOrDefault(String,ViolationSeverity)
    [javac]   location: variable severityPerSignature of type Map<String,ViolationSeverity>
    [javac] 1 error

uschindler added a commit that referenced this pull request Mar 31, 2025
- Java 7 compatibility
- Clean up imports / indent
@uschindler
Copy link
Member

This commit fixes the Java 7 problem: 2a91113

I will do some further checks and adjust. This is a good first step. Thanks!

@uschindler uschindler added this to the 3.9 milestone Mar 31, 2025
@uschindler uschindler self-assigned this Mar 31, 2025
@uschindler
Copy link
Member

uschindler commented Apr 1, 2025

I have to fix the ant task because it only allows to set supressed signature one (via attribute), but to set multiple ones.

For it to work there needs to be a subelement for this, e.g. look at this: https://github.com/policeman-tools/forbidden-apis/blob/main/src/main/java/de/thetaphi/forbiddenapis/ant/BundledSignaturesType.java

Of course documentation needs update, too.

@uschindler
Copy link
Member

uschindler commented Apr 2, 2025

Hi @kwin,
can you look at PR #262. It fixes the problems with Ant configuration. Unfortunately, the Maven project config format is the worst for developing plugins like this as the types allowed in the maven config are just key/multi-values with fixed types only. Theres no real structure possible.

Ant was changed to add the severityOverride using subelements:

    <forbiddenapis classpathref="path.all" targetVersion="${jdk.version}">
      <fileset refid="main.classes"/>
      <bundledSignatures name="jdk-unsafe"/>
      java.util.Locale#ENGLISH @ We are speaking chinese here!
      java.lang.** @ You are crazy that you disallow all java.lang
      java.io.** @ You are crazy that you disallow all java.io
      <severityOverride severity="warning">java.util.Locale#ENGLISH</severityOverride>
      <severityOverride severity="debug">
        java.lang.**
        java.io.**
      </severityOverride>
    </forbiddenapis>

I will have a separate look at Gradle, too. Gradle is also more flexible, so it might be better to set the priority using a "map" like structure there, too. Gradle is not limited to simple setters, you can call all the methods provided by the task. So separating into the 2 lists of sigantures is not ideal.

If I do not have a better idea, I'd like to release 3.9 soon. People are waiting already.

Can you just have a quick look on the PR?

@uschindler
Copy link
Member

I marked the new API/setting with @Incubating in the Gradle task.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Allow to skip certain signatures from bundled signature files
2 participants