Cleanup #17801

ngocnhan-tran1996 · 2025-08-24T07:12:39Z

This PR includes

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

Signed-off-by: Tran Ngoc Nhan <[email protected]>

ronodhirSoumik · 2025-08-26T17:57:04Z

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

-		if (result.length() == 0) {
+		if (result.isEmpty()) {
 			return null;
 		}


String result = (query == null) ? "" : artifactPattern.matcher(query).replaceFirst(""); if (result.isEmpty()) { return null; }

Fair point, @ronodhirSoumik, though the Spring checkstyle requires a negation in ternaries. So, this is probably the result of having run checkstyleTest on the PR.

ronodhirSoumik · 2025-08-26T18:06:02Z

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

-	 * @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.


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

ronodhirSoumik · 2025-08-26T18:13:36Z

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

 			return role;
 		}
-		if (defaultRolePrefix == null || defaultRolePrefix.length() == 0) {
+		if (defaultRolePrefix == null || defaultRolePrefix.isEmpty()) {


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; }

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

Signed-off-by: Tran Ngoc Nhan <[email protected]>

jzheaux

Thanks for all the great cleanup, @ngocnhan-tran1996. I've left some feedback inline.

jzheaux · 2025-10-20T22:20:57Z

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

 	@Override
 	public boolean equals(Object obj) {
-		if (this == obj) {
+		if (super.equals(obj)) {


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

jzheaux · 2025-10-20T22:21:51Z

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

 		}
-		if (!super.equals(obj) || !(obj instanceof DefaultServiceAuthenticationDetails)) {
-			return false;
+		if (obj instanceof DefaultServiceAuthenticationDetails that) {


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

jzheaux · 2025-10-20T22:23:51Z

...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) {


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

jzheaux · 2025-10-20T22:24:34Z

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

-		if (result.length() == 0) {
+		if (result.isEmpty()) {
 			return null;
 		}


Fair point, @ronodhirSoumik, though the Spring checkstyle requires a negation in ternaries. So, this is probably the result of having run checkstyleTest on the PR.

jzheaux · 2025-10-20T22:25:27Z

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

-	 * @param defaultRolePrefix
-	 * @param role
-	 * @return
+	 * @param defaultRolePrefix the default prefix to add to roles.


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_}

jzheaux · 2025-10-20T22:29:11Z

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

+import org.springframework.util.StringUtils;
+
+/**
+ * Implementation of PasswordEncoder.


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 */

jzheaux · 2025-10-20T22:31:17Z

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

 	public final boolean upgradeEncoding(@Nullable String encodedPassword) {
-		if (encodedPassword == null || encodedPassword.length() == 0) {
-			return false;
+		if (StringUtils.hasLength(encodedPassword)) {


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.

jzheaux · 2025-10-20T22:32:31Z

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

 		this.strippedServletPath = strip(request.getServletPath());
 		String pathInfo = strip(request.getPathInfo());
-		if (pathInfo != null && pathInfo.length() == 0) {
+		if (pathInfo != null && pathInfo.isEmpty()) {


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

Signed-off-by: Tran Ngoc Nhan <[email protected]>

ngocnhan-tran1996 added 5 commits August 24, 2025 13:00

Use String#isEmpty

23fa294

Signed-off-by: Tran Ngoc Nhan <[email protected]>

Update javadoc

a96dbd8

Signed-off-by: Tran Ngoc Nhan <[email protected]>

Remove redundant check and exception

82ac99f

Signed-off-by: Tran Ngoc Nhan <[email protected]>

Polish some methods

8bf49d2

Signed-off-by: Tran Ngoc Nhan <[email protected]>

Add author

7c436a2

Signed-off-by: Tran Ngoc Nhan <[email protected]>

spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Aug 24, 2025

ronodhirSoumik reviewed Aug 26, 2025

View reviewed changes

ngocnhan-tran1996 added 2 commits August 27, 2025 01:27

Update javadoc and apply StringUtils#hasLength

e08de06

Signed-off-by: Tran Ngoc Nhan <[email protected]>

Fix checkstyle

f77e0b9

Signed-off-by: Tran Ngoc Nhan <[email protected]>

ngocnhan-tran1996 force-pushed the cleanup branch from ec9b7b9 to f77e0b9 Compare August 26, 2025 19:33

jzheaux requested changes Oct 20, 2025

View reviewed changes

jzheaux self-assigned this Oct 20, 2025

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

ngocnhan-tran1996 added 4 commits October 21, 2025 18:50

Resolve conflict

8f09d93

Signed-off-by: Tran Ngoc Nhan <[email protected]>

Resolve feedback

dbf9528

Signed-off-by: Tran Ngoc Nhan <[email protected]>

Fix checkstyle

72c3bad

Signed-off-by: Tran Ngoc Nhan <[email protected]>

Revert code

99f372c

Signed-off-by: Tran Ngoc Nhan <[email protected]>

ngocnhan-tran1996 requested a review from jzheaux October 23, 2025 02:39

Cleanup #17801

Are you sure you want to change the base?

Cleanup #17801

Uh oh!

Conversation

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

Uh oh!

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

jzheaux left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ngocnhan-tran1996 commented Aug 24, 2025 •

edited

Loading

ronodhirSoumik Aug 26, 2025 •

edited

Loading

ronodhirSoumik Aug 26, 2025 •

edited

Loading