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

Remove dependency on conmmons-codec by using java.util.Base64 #11319

Merged
merged 1 commit into from Jun 9, 2022
Merged

Remove dependency on conmmons-codec by using java.util.Base64 #11319

merged 1 commit into from Jun 9, 2022

Conversation

j3graham
Copy link
Contributor

@j3graham j3graham commented Jun 1, 2022

see gh-11318

First commit removes test usages of Base64 and some other minor commons-codec functions.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jun 1, 2022
@rwinch rwinch requested a review from jzheaux June 1, 2022 19:49
@rwinch
Copy link
Member

rwinch commented Jun 1, 2022

Thanks for the PR @j3graham To start, I think another change might be to remove commons-codec occurences from *.gradle files so the dependency is no longer added to the classpath.

rg commons-codec
messaging/spring-security-messaging.gradle
18:	testImplementation 'commons-codec:commons-codec'

build.gradle
92:			components.withModule("commons-codec:commons-codec") { selection ->
95:					selection.reject("commons-codec updates break saml tests");

web/spring-security-web.gradle
23:	testImplementation 'commons-codec:commons-codec'

dependencies/spring-security-dependencies.gradle
26:		api "commons-codec:commons-codec:1.15"

I'll leave it to @jzheaux to review this further.

@j3graham
Copy link
Contributor Author

j3graham commented Jun 1, 2022

Dependencies removed.

Copy link
Contributor

@jzheaux jzheaux left a comment

Choose a reason for hiding this comment

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

Thanks, @j3graham! I've left some feedback inline.

@jzheaux jzheaux self-assigned this Jun 1, 2022
@jzheaux jzheaux added type: enhancement A general enhancement in: saml2 An issue in SAML2 modules and removed status: waiting-for-triage An issue we've not yet triaged labels Jun 1, 2022
@jzheaux jzheaux added this to the 6.0.0-M6 milestone Jun 1, 2022
@jzheaux
Copy link
Contributor

jzheaux commented Jun 1, 2022

Also, would you please consider rebasing this PR on 5.8.x so that the enhancement can get out to folks a little sooner?

@j3graham
Copy link
Contributor Author

j3graham commented Jun 1, 2022

Will be happy to rebase it when you're happy with this version. Interestingly the minor StandardCharsets change you called out requires Java 10, so will need to leave that out.

@j3graham
Copy link
Contributor Author

j3graham commented Jun 2, 2022

New PR for 5.8.x: #11322

javax->jakarta changes were a bit awkward.

Perhaps we can abandon this one in favour of the 5.8.x one and let the merge to main happen.

@j3graham
Copy link
Contributor Author

j3graham commented Jun 7, 2022

@jzheaux Should I fix up this PR, or will the one from 5.8 get merged independently?

@jzheaux
Copy link
Contributor

jzheaux commented Jun 8, 2022

Because of javax->jakarta, the merge isn't clean so it needs to be done manually. Since you've done that work already, I figure we can finish this PR, too. That said, if you are out of time, I can take care of it by cherry-picking the other and changing the package names.

Up to you -- if you close the PR, I'll do the cherry-picking. If not, we can merge this one as well once it's ready.

@j3graham
Copy link
Contributor Author

j3graham commented Jun 8, 2022

I cherry-picked the change and fixed the jakarta stuff.

@jzheaux jzheaux merged commit f3c96fa into spring-projects:main Jun 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: saml2 An issue in SAML2 modules type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants