Skip to content

Conversation

therepanic
Copy link
Contributor

Since the GrantedAuthority#getAuthority contract itself implies that it can return null, we should mark it as @Nullable.

Closes: gh-17999

@therepanic
Copy link
Contributor Author

therepanic commented Oct 6, 2025

Currently, the build crashes in the docs module, and it's not entirely clear why. Can you explain please why the build crashes in the following class test?

ObtainingMoreAuthorizationTests. profileWhenMissingAuthorityConfigurationThenRedirectsToAuthorizationServer()
ObtainingMoreAuthorizationTests. profileWhenMissingAuthorityConfigurationThenRedirectsToAuthorizationServer()

Note: Now it is fixed

@therepanic therepanic force-pushed the gh-17999 branch 3 times, most recently from 54f53fc to e16ab55 Compare October 11, 2025 14:00
* precision).
*/
String getAuthority();
@Nullable String getAuthority();
Copy link
Contributor

@ronodhirSoumik ronodhirSoumik Oct 11, 2025

Choose a reason for hiding this comment

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

Question :: though this implementation supports null case, but it is supposed to be avoided (as per the documentation -> returning null should be avoided unless actually required. ) So dont we losing the feature/control with the overall changes?
@therepanic @rwinch

Copy link
Contributor

Choose a reason for hiding this comment

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

@ronodhirSoumik, good question. The documentation also says that null "should be returned" in cases where it cannot be expressed with sufficient precision. In other words, returning null is recommended when needed.

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, @therepanic! I've left some feedback inline.

@therepanic
Copy link
Contributor Author

Thanks for the review, @jzheaux.

I agree with everything. It was a bad idea to mark authority as @Nullable when there was a simpler way to do it. I implemented this change as a separate commit to make it easier to read the changes.

I don't think the build crashes because of the changes in the PR.

@jzheaux jzheaux self-assigned this Oct 20, 2025
@jzheaux jzheaux added in: core An issue in spring-security-core type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Oct 20, 2025
@jzheaux jzheaux added this to the 7.0.0 milestone Oct 20, 2025
@jzheaux jzheaux merged commit 0a2f55d into spring-projects:main Oct 20, 2025
6 checks passed
@jzheaux
Copy link
Contributor

jzheaux commented Oct 20, 2025

Thanks, @therepanic! This is now merged into main

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

Labels

in: core An issue in spring-security-core type: enhancement A general enhancement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

GrantedAuthority.getAuthority() should be marked @Nullable

4 participants