Skip to content

MINOR: Refactor return statement and log info#10012

Merged
chia7712 merged 1 commit intoapache:trunkfrom
tang7526:feature/RefactorOAuthBearerUnsecuredValidatorCallbackHandler
Feb 9, 2021
Merged

MINOR: Refactor return statement and log info#10012
chia7712 merged 1 commit intoapache:trunkfrom
tang7526:feature/RefactorOAuthBearerUnsecuredValidatorCallbackHandler

Conversation

@tang7526
Copy link
Copy Markdown
Contributor

@tang7526 tang7526 commented Feb 1, 2021

  • Invoke method only conditionally.
  • No need to call "toString()" method as formatting and string conversion is done by the Formatter.

before

log.info("Successfully validated token with principal {}: {}", unsecuredJwt.principalName(), unsecuredJwt.claims().toString());

after

if (log.isInfoEnabled()) {
    log.info("Successfully validated token with principal {}: {}", unsecuredJwt.principalName(), unsecuredJwt.claims());
}
  • Immediately return the expression instead of assigning it to the temporary variable.

before

String principalClaimName = principalClaimNameValue != null && !principalClaimNameValue.trim().isEmpty()
        ? principalClaimNameValue.trim()
        : "sub";
return principalClaimName;

after

return principalClaimNameValue != null && !principalClaimNameValue.trim().isEmpty()
        ? principalClaimNameValue.trim()
        : "sub";

Committer Checklist (excluded from commit message)

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation (including upgrade notes)

@tang7526 tang7526 force-pushed the feature/RefactorOAuthBearerUnsecuredValidatorCallbackHandler branch from a1012b2 to d3a9122 Compare February 2, 2021 04:57
Copy link
Copy Markdown
Member

@chia7712 chia7712 left a comment

Choose a reason for hiding this comment

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

@tang7526 Thanks for your patch. Left some minor comments.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is this condition check necessary? It is rare in code base.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

OK, I think you are right. We do not need condition check in here. I already revert it.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

How about adding a helper method to check both "null" and "empty"?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done.

@tang7526 tang7526 force-pushed the feature/RefactorOAuthBearerUnsecuredValidatorCallbackHandler branch from d3a9122 to 511ec9a Compare February 3, 2021 16:16
Comment on lines 26 to 32
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

For me the isNotBlank() adds no value. These could just be added to Utils, rather than adding the new class. Also: Add javadoc.

Copy link
Copy Markdown
Contributor Author

@tang7526 tang7526 Feb 4, 2021

Choose a reason for hiding this comment

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

@tombentley Thanks for your opinion. I have removed isNotBlank() and added javadoc.

@tang7526 tang7526 closed this Feb 4, 2021
@tang7526 tang7526 force-pushed the feature/RefactorOAuthBearerUnsecuredValidatorCallbackHandler branch from 511ec9a to eb524ec Compare February 4, 2021 12:26
@tang7526 tang7526 reopened this Feb 4, 2021
Copy link
Copy Markdown
Member

@chia7712 chia7712 left a comment

Choose a reason for hiding this comment

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

@tang7526 Thanks for your patch. LGTM

@chia7712
Copy link
Copy Markdown
Member

chia7712 commented Feb 8, 2021

The test failure is related to #10024

@tang7526 Could you merge trunk to trigger QA again?

@tang7526 tang7526 force-pushed the feature/RefactorOAuthBearerUnsecuredValidatorCallbackHandler branch from 02584d4 to d278363 Compare February 8, 2021 15:19
@tang7526 tang7526 closed this Feb 8, 2021
@tang7526 tang7526 reopened this Feb 8, 2021
@tang7526
Copy link
Copy Markdown
Contributor Author

tang7526 commented Feb 8, 2021

The test failure is related to #10024

@tang7526 Could you merge trunk to trigger QA again?

@chia7712 Done

* @param str a string to be checked
* @return true if the string is null, empty or whitespace only; otherwise, return false.
*/
public static boolean isBlank(String str) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

How about applying this helper method to code base?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

How about applying this helper method to code base?
@chia7712 OK. I will do it on next PR.

@chia7712 chia7712 merged commit 1bfce16 into apache:trunk Feb 9, 2021
@tang7526 tang7526 deleted the feature/RefactorOAuthBearerUnsecuredValidatorCallbackHandler branch February 9, 2021 07:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants