Skip to content

Consolidate shared code between JwtDecoders and ReactiveJwtDecoders #7263

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.

Sign up for GitHub

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

Jump to bottom
Closed
jzheaux opened this issue Aug 15, 2019 · 7 comments · Fixed by #7311
Closed

Consolidate shared code between JwtDecoders and ReactiveJwtDecoders #7263

jzheaux opened this issue Aug 15, 2019 · 7 comments · Fixed by #7311
Assignees
@jzheaux
Labels
in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) type: enhancement A general enhancement
Milestone
5.2.0.RC1

Comments

@jzheaux
Copy link
Contributor

jzheaux commented Aug 15, 2019

JwtDecoders and ReactiveJwtDecoders share code that could be moved into a package-private class.

A new package-private class called JwtDecoderProviderConfigurationUtils might be a good home for the shared code.
@jzheaux jzheaux added type: enhancement A general enhancement status: ideal-for-contribution An issue that we actively are looking for someone to help us with in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) labels Aug 15, 2019
@jzheaux jzheaux added this to the 5.2.0.RC1 milestone Aug 15, 2019
@ThomasVitale
Copy link
Contributor

Hi @jzheaux, I would like to work on this issue

@jzheaux
Copy link
Contributor Author

jzheaux commented Aug 16, 2019

It's yours, @ThomasVitale!

@jzheaux jzheaux removed the status: ideal-for-contribution An issue that we actively are looking for someone to help us with label Aug 16, 2019
@jzheaux jzheaux self-assigned this Aug 16, 2019
@ThomasVitale
Copy link
Contributor

ThomasVitale commented Aug 25, 2019 •
edited
Loading

Hi @jzheaux, I have submitted a first PR. When you have time, I would like to get your opinion about a few things please.

  • Refactoring. Is it ok? There is still something more that could be extracted from the 2 main classes, but it is also a matter of readability. I am a bit unsure there.
  • Javadoc. I moved some comments to the util class. Is it ok the amount of docs I kept in JwtDecoders and ReactiveJwtDecoders?
  • Unit tests. So far, I didn't change JwtDecodersTests and ReactiveJwtDecodersTests to do regression testing while refactoring. Now, do you think it makes sense to move some of the tests verifying the shared code to a separate test class? Or maybe should I just add new tests for the new class without touching the existing ones?

Thanks

@jzheaux
Copy link
Contributor Author

jzheaux commented Aug 26, 2019

Good questions, @ThomasVitale.

Yes, I think you have the right amount of consolidation.

Let's please change the JavaDocs on the public methods back to how they are on master. Also, it's fine to add JavaDocs to the utils class, but since it isn't public-facing, it's not quite as important.

Moving tests would be fine, but really since that class's methods are static, they are going to get tested anyway by ReactiveJwtDecoders and JwtDecoders, so I don't really see much point in moving tests or creating new ones. If you see coverage gaps that prevent you from being confident in your refactor, feel free to add tests.

@ThomasVitale
Copy link
Contributor

Thanks for your answer @jzheaux, I have updated the PR according to your input. I have changed back the JavaDocs on the public methods and not added more tests since all the methods in the util class are covered by the existing ones.

@jzheaux
Copy link
Contributor Author

jzheaux commented Aug 27, 2019

@ThomasVitale sounds great - will you please link the PR to this ticket? You can usually do that by adding a "Fixes gh-7263" at the bottom of the commit message.

@ThomasVitale ThomasVitale mentioned this issue Aug 27, 2019
Merged
@ThomasVitale
Copy link
Contributor

Done, thanks for your support @jzheaux

jzheaux pushed a commit that referenced this issue Aug 27, 2019
@ThomasVitale @jzheaux
Consolidate shared code between JwtDecoders and ReactiveJwtDecoders
505882c 
Extract duplicated code from JwtDecoders and ReactiveJwtDecoders into a
package-private class.

Fixes gh-7263
@ThomasVitale ThomasVitale mentioned this issue Oct 22, 2019
Closed
dadikovi pushed a commit to dadikovi/spring-security that referenced this issue May 2, 2020
@ThomasVitale @dadikovi
Deprecate OpenID 2.0 Support
84f45e4 
Add deprecation notice to all files in the spring-security-openid module

Fixes spring-projectsgh-7263
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@jzheaux jzheaux

Labels
in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) type: enhancement A general enhancement
Projects
None yet
Milestone
5.2.0.RC1
Development

Successfully merging a pull request may close this issue.

Consolidate shared code between JwtDecoders and ReactiveJwtDecoders
2 participants
@jzheaux @ThomasVitale