Cleanup #17801

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

Jump to bottom

Open

ngocnhan-tran1996 wants to merge 11 commits into spring-projects:main

from ngocnhan-tran1996:cleanup

Open

Cleanup #17801

ngocnhan-tran1996 wants to merge 11 commits into spring-projects:main from ngocnhan-tran1996:cleanup

+57 −57

Conversation

@ngocnhan-tran1996

Copy link

Contributor

@ngocnhan-tran1996 ngocnhan-tran1996 commented Aug 24, 2025 •

edited

Loading

This PR includes

Use StringUtils#hasLength
Remove redundant check and exception
Update javadoc
Polish some methods

ngocnhan-tran1996 added 5 commits

August 24, 2025 13:00

@ngocnhan-tran1996


 Use String#isEmpty

23fa294

Signed-off-by: Tran Ngoc Nhan <ngocnhan.tran1996@gmail.com>

@ngocnhan-tran1996


 Update javadoc

a96dbd8

Signed-off-by: Tran Ngoc Nhan <ngocnhan.tran1996@gmail.com>

@ngocnhan-tran1996


 Remove redundant check and exception

82ac99f

Signed-off-by: Tran Ngoc Nhan <ngocnhan.tran1996@gmail.com>

@ngocnhan-tran1996


 Polish some methods

8bf49d2

Signed-off-by: Tran Ngoc Nhan <ngocnhan.tran1996@gmail.com>

@ngocnhan-tran1996


 Add author

7c436a2

Signed-off-by: Tran Ngoc Nhan <ngocnhan.tran1996@gmail.com>

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage label

Aug 24, 2025

ronodhirSoumik

ronodhirSoumik reviewed

Aug 26, 2025

View reviewed changes

...org/springframework/security/cas/web/authentication/DefaultServiceAuthenticationDetails.java Show resolved Hide resolved

ronodhirSoumik

ronodhirSoumik reviewed

Aug 26, 2025

View reviewed changes

...org/springframework/security/cas/web/authentication/DefaultServiceAuthenticationDetails.java Outdated

* @return

* @param artifactParameterName the artifactParameterName that is removed from the

* current URL. The result becomes the service url. Cannot be null and cannot be an

* empty String.

Copy link

Contributor

@ronodhirSoumik ronodhirSoumik Aug 26, 2025

Choose a reason for hiding this comment

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

Cannot be null and cannot be an empty String -> Cannot be null or empty String

ronodhirSoumik

ronodhirSoumik reviewed

Aug 26, 2025

View reviewed changes

core/src/main/java/org/springframework/security/access/expression/SecurityExpressionRoot.java Outdated

return role;

}

if (defaultRolePrefix == null || defaultRolePrefix.length() == 0) {

if (defaultRolePrefix == null || defaultRolePrefix.isEmpty()) {

Copy link

Contributor

@ronodhirSoumik ronodhirSoumik Aug 26, 2025 •

edited

Loading

Choose a reason for hiding this comment

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

As there are several uses of str == null || str.isEmpty() I think a common util function will be helpful

boolean isEmpty(String str) {
	return str == null || str.length == 0;
}

Copy link

Contributor Author

@ngocnhan-tran1996 ngocnhan-tran1996 Aug 26, 2025

Choose a reason for hiding this comment

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

I think we can use !org.springframework.util.StringUtils#hasLength

ngocnhan-tran1996 added 2 commits

August 27, 2025 01:27

@ngocnhan-tran1996


 Update javadoc and apply StringUtils#hasLength

e08de06

Signed-off-by: Tran Ngoc Nhan <ngocnhan.tran1996@gmail.com>

@ngocnhan-tran1996


 Fix checkstyle

f77e0b9

Signed-off-by: Tran Ngoc Nhan <ngocnhan.tran1996@gmail.com>

@ngocnhan-tran1996 ngocnhan-tran1996 force-pushed the cleanup branch from ec9b7b9 to f77e0b9 Compare

August 26, 2025 19:33

jzheaux

jzheaux requested changes

Oct 20, 2025

View reviewed changes

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 for all the great cleanup, @ngocnhan-tran1996. I've left some feedback inline.

...org/springframework/security/cas/web/authentication/DefaultServiceAuthenticationDetails.java Outdated

@Override

public boolean equals(Object obj) {

if (this == obj) {

if (super.equals(obj)) {

Copy link

Contributor

@jzheaux jzheaux Oct 20, 2025

Choose a reason for hiding this comment

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

Let's please stay with this == obj as this is a bit more obvious what kind of shortcut we are doing.

...org/springframework/security/cas/web/authentication/DefaultServiceAuthenticationDetails.java Outdated

}

if (!super.equals(obj) || !(objinstanceof DefaultServiceAuthenticationDetails)) {

return false;

if (objinstanceof DefaultServiceAuthenticationDetailsthat) {

Copy link

Contributor

@jzheaux jzheaux Oct 20, 2025

Choose a reason for hiding this comment

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

Thanks for this change. In Spring Security, it's preferred to exit early for simple cases. Would you please flip the logic here to:

if (!(obj instanceof DefaultServiceAuthenticationDetails that)) {
 return false;
}
// ... continue

...org/springframework/security/cas/web/authentication/DefaultServiceAuthenticationDetails.java

private String getQueryString(final HttpServletRequest request, final Pattern artifactPattern) {

final String query = request.getQueryString();

if (query == null) {

Copy link

Contributor

@jzheaux jzheaux Oct 20, 2025

Choose a reason for hiding this comment

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

Let's please keep checking for null here since that allows us to exit early without needing to instantiate a variable.

...org/springframework/security/cas/web/authentication/DefaultServiceAuthenticationDetails.java Show resolved Hide resolved

core/src/main/java/org/springframework/security/access/expression/SecurityExpressionRoot.java Outdated

* @param defaultRolePrefix

* @param role

* @return

* @param defaultRolePrefix the default prefix to add to roles.

Copy link

Contributor

@jzheaux jzheaux Oct 20, 2025

Choose a reason for hiding this comment

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

Let's please give an example here since "role prefix" could be a bit esoteric. For example:

the default prefix to add to roles, for example {@code ROLE_}

...ain/java/org/springframework/security/crypto/password/AbstractValidatingPasswordEncoder.java Outdated

import org.springframework.util.StringUtils;

/**

* Implementation of PasswordEncoder.

Copy link

Contributor

@jzheaux jzheaux Oct 20, 2025

Choose a reason for hiding this comment

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

I wouldn't quite say that this is an implementation as much as it is an abstract class that allows subclasses to except the password to be non-{@code null}.

Perhaps change this to:

/**
 * An abstract {@link PasswordEncoder} that implementers can use for expecting the
 * password to be non-{@code null}. Each common password API method is accompanied
 * with an abstract method with a {@code NonNull} prefix. By implementing this, the concrete
 * class is specifying what to do with the password when it is non-{@code null}, allowing this
 * class to handle the {@code null} case.
 * 
 * @author Rob Winch
 * @since 7.0
 */

...ain/java/org/springframework/security/crypto/password/AbstractValidatingPasswordEncoder.java Outdated

public final boolean upgradeEncoding(@Nullable String encodedPassword) {

if (encodedPassword == null || encodedPassword.length() == 0) {

return false;

if (StringUtils.hasLength(encodedPassword)) {

Copy link

Contributor

@jzheaux jzheaux Oct 20, 2025

Choose a reason for hiding this comment

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

Please favor the simpler exits coming first:

if (!StringUtils.hasLength(encodedPassword)) {
 return false;
}
return upgradeEncodingNonNull(encodedPassword);

This also has the added benefit of making the overall change smaller.

web/src/main/java/org/springframework/security/web/firewall/RequestWrapper.java Outdated

this.strippedServletPath = strip(request.getServletPath());

String pathInfo = strip(request.getPathInfo());

if (pathInfo != null && pathInfo.length() == 0) {

if (pathInfo != null && pathInfo.isEmpty()) {

Copy link

Contributor

@jzheaux jzheaux Oct 20, 2025

Choose a reason for hiding this comment

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

Would !StringUtils.hasLength(pathInfo)) work here as well?

@jzheaux jzheaux self-assigned this

Oct 20, 2025

@jzheaux jzheaux added in: core type: enhancement and removed status: waiting-for-triage labels

Oct 20, 2025

ngocnhan-tran1996 added 4 commits

October 21, 2025 18:50

@ngocnhan-tran1996


 Resolve conflict

8f09d93

Signed-off-by: Tran Ngoc Nhan <ngocnhan.tran1996@gmail.com>

@ngocnhan-tran1996


 Resolve feedback

dbf9528

Signed-off-by: Tran Ngoc Nhan <ngocnhan.tran1996@gmail.com>

@ngocnhan-tran1996


 Fix checkstyle

72c3bad

Signed-off-by: Tran Ngoc Nhan <ngocnhan.tran1996@gmail.com>

@ngocnhan-tran1996


 Revert code

99f372c

Signed-off-by: Tran Ngoc Nhan <ngocnhan.tran1996@gmail.com>

@ngocnhan-tran1996 ngocnhan-tran1996 requested a review from jzheaux

October 23, 2025 02:39

Labels

in: core type: enhancement

4 participants

@ngocnhan-tran1996 @jzheaux @ronodhirSoumik @spring-projects-issues

Uh oh!

Cleanup #17801

Are you sure you want to change the base?

Cleanup #17801

Conversation

@ngocnhan-tran1996 ngocnhan-tran1996 commented Aug 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

@ronodhirSoumik ronodhirSoumik Aug 26, 2025

Choose a reason for hiding this comment

Uh oh!

@ronodhirSoumik ronodhirSoumik Aug 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

@ngocnhan-tran1996 ngocnhan-tran1996 Aug 26, 2025

Choose a reason for hiding this comment

Uh oh!

@jzheaux jzheaux left a comment

Choose a reason for hiding this comment

Uh oh!

@jzheaux jzheaux Oct 20, 2025

Choose a reason for hiding this comment

Uh oh!

@jzheaux jzheaux Oct 20, 2025

Choose a reason for hiding this comment

Uh oh!

@jzheaux jzheaux Oct 20, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

@jzheaux jzheaux Oct 20, 2025

Choose a reason for hiding this comment

Uh oh!

@jzheaux jzheaux Oct 20, 2025

Choose a reason for hiding this comment

Uh oh!

@jzheaux jzheaux Oct 20, 2025

Choose a reason for hiding this comment

Uh oh!

@jzheaux jzheaux Oct 20, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

@ngocnhan-tran1996 ngocnhan-tran1996 commented Aug 24, 2025 •

edited

Loading

@ronodhirSoumik ronodhirSoumik Aug 26, 2025 •

edited

Loading