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

[improve][build] Upgrade Spotbugs to a version with JDK 17 compatibility #19315

Merged
merged 2 commits into from Jan 24, 2023

Conversation

lhotari
Copy link
Member

@lhotari lhotari commented Jan 24, 2023

Motivation

Build log contains warning messages such as

(file:/home/runner/.m2/repository/com/github/spotbugs/spotbugs/4.2.2/spotbugs-4.2.2.jar)
     [java] WARNING: Please consider reporting this to the maintainers of edu.umd.cs.findbugs.ba.jsr305.TypeQualifierValue
     [java] WARNING: System::setSecurityManager will be removed in a future release
     [java] WARNING: A terminally deprecated method in java.lang.System has been called
     [java] WARNING: System::setSecurityManager has been called by edu.umd.cs.findbugs.ba.jsr305.TypeQualifierValue

This is fixed by spotbugs/spotbugs#1983 in 4.7.0

Modifications

Upgrade Spotbugs to 4.7.3.

Spotbugs 4.7.3 release notes:
https://github.com/spotbugs/spotbugs/releases/tag/4.7.3

Ignore violations in existing code that fail with new rules that are active in upgraded Spotbugs version.

Documentation

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

…ity with JDK 17

Log contains messages such as
(file:/home/runner/.m2/repository/com/github/spotbugs/spotbugs/4.2.2/spotbugs-4.2.2.jar)
     [java] WARNING: Please consider reporting this to the maintainers of edu.umd.cs.findbugs.ba.jsr305.TypeQualifierValue
     [java] WARNING: System::setSecurityManager will be removed in a future release
     [java] WARNING: A terminally deprecated method in java.lang.System has been called
     [java] WARNING: System::setSecurityManager has been called by edu.umd.cs.findbugs.ba.jsr305.TypeQualifierValue
This is fixed by spotbugs/spotbugs#1983 in 4.7.0

Spotbugs 4.7.3 release notes:
https://github.com/spotbugs/spotbugs/releases/tag/4.7.3
@lhotari lhotari added this to the 2.12.0 milestone Jan 24, 2023
@lhotari lhotari self-assigned this Jan 24, 2023
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Jan 24, 2023
@lhotari lhotari marked this pull request as draft January 24, 2023 07:20
findbugsExclude.xml file updated with this shell command:
mytemp=$(mktemp); { cat src/main/resources/findbugsExclude.xml | head -n -1; echo '  <!-- Ignore violations that were present when the rule was enabled -->'; xmlstarlet sel -t -e 'FindBugsFilter' -m '//BugInstance' -s A:T:L 'Class/@classname' -e 'Match' -e 'Class' -a 'name' -v 'Class/@classname' -b -b -e 'Method' -a 'name' -v 'Method/@name' -b -b -e 'Bug' -a 'pattern' -v '@type' target/spotbugsXml.xml  | xmlstarlet fo --omit-decl | tail -n +2 } > $mytemp; xmlstarlet fo --omit-decl $mytemp > src/main/resources/findbugsExclude.xml; rm $mytemp
@lhotari lhotari marked this pull request as ready for review January 24, 2023 11:22
@cbornet cbornet self-requested a review January 24, 2023 14:03
@nicoloboschi nicoloboschi merged commit b880b1d into apache:master Jan 24, 2023
@lhotari
Copy link
Member Author

lhotari commented Mar 31, 2023

Adding a comment here so that we have some documentation about handling EI_EXPOSE_REP Spotbugs errors.

This is the spotbugs description for EI_EXPOSE_REP

EI: May expose internal representation by returning reference to mutable object (EI_EXPOSE_REP)
Returning a reference to a mutable object value stored in one of the object's fields exposes the internal representation of the object. If instances are accessed by untrusted code, and unchecked changes to the mutable object would compromise security or other important properties, you will need to do something different. Returning a new copy of the object is better approach in many situations.

Newer versions of Spotbugs enable this rule by default. One possibility is to ignore the error.

There are 2 ways to ignore the error: with an annotation or with an XML file. Since there was a lot of locations where this is happening, I used the XML file in this PR. Having the rule is useful since we should be thinking of the implications of thread safety and so on when a mutable object is shared.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/build doc-not-needed Your PR changes do not impact docs ready-to-test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants