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

Code cleanup #3174

Merged
merged 15 commits into from
Dec 9, 2024
Merged

Code cleanup #3174

merged 15 commits into from
Dec 9, 2024

Conversation

duanemay
Copy link
Member

@duanemay duanemay commented Dec 2, 2024

Mostly automated with Openrewrite rules, and some manual updates.

Rules Run: CodeStyle, Common Static Analysis Issue remediation, Java 17

@duanemay duanemay changed the title Rewrite 202411 Code cleanup Dec 2, 2024
Copy link
Member

@strehle strehle left a comment

Choose a reason for hiding this comment

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

Copy link
Member

@strehle strehle left a comment

Choose a reason for hiding this comment

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

Please resolve with develop

@strehle
Copy link
Member

strehle commented Dec 6, 2024

@duanemay So far I am through with, thanks for the cleanup. I have 2 open clarifications

  1. The test coverage is low because some objects from IdP removal ( see Remove SAML IDP feature #2638 , reason of major upgrade to 77.x ) still there. Suggest to remove these objects with an extra PR, WDYT ?

  2. Guava. Should we remove Guava from usage at all ? Because right now you removed the direct dependency only but then we get Guava from the transient dependency from opensaml 4.x . We can exclude it in general but then also some other classes still use Util functions from Guava. Problem now: transient dependency adds Guava 28.x which has some CVEs. With that package we run into a problem with our validation

@strehle
Copy link
Member

strehle commented Dec 6, 2024

@duanemay So far I am through with, thanks for the cleanup. I have 2 open clarifications

  1. The test coverage is low because some objects from IdP removal ( see Remove SAML IDP feature #2638 , reason of major upgrade to 77.x ) still there. Suggest to remove these objects with an extra PR, WDYT ?

Tested without guava and yes,
net.shibboleth.utilities.java.support.xml.BasicParserPool.setBuilderFeatures
needs it, so we cannot omit but then it is better we define the version of used guava and use the lastest. Otherwise we get an outdated version if we rely on transitive deps.

@duanemay duanemay merged commit 23d08bb into develop Dec 9, 2024
21 of 22 checks passed
@duanemay duanemay deleted the rewrite-202411 branch December 9, 2024 14:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

2 participants