Skip to content

Use substring instead of replaceFirst in OAuth2AuthorizationConsent #1223

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
wants to merge 3 commits into from
Closed

Use substring instead of replaceFirst in OAuth2AuthorizationConsent #1223

wants to merge 3 commits into from

Conversation

heartape
Copy link
Contributor

Close #1222

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label May 22, 2023
jgrandja
jgrandja requested changes May 26, 2023
View reviewed changes
Copy link
Collaborator

@jgrandja jgrandja 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 the PR @heartape. Please see review comments.

CONTRIBUTING.adoc Outdated
@@ -67,7 +67,7 @@ Use git rebase –interactive, git add –patch and other tools to "squash" mult

== Format commit messages

. Keep the subject line to 50 characters or less if possible.
. Keep the subject line to 50 characters or fewer if possible.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please revert this as it's not related to the proposed replaceFirst() change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jgrandja I will submit a new PR for this section. Do you think this is a good idea?

Copy link
Collaborator

Choose a reason for hiding this comment

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

The use of "less" is proper grammar so we will leave as-is

...y/oauth2/server/authorization/oidc/web/authentication/OidcLogoutAuthenticationConverter.java Outdated
@@ -65,12 +65,6 @@ public Authentication convert(HttpServletRequest request) {
principal = ANONYMOUS_AUTHENTICATION;
}

String sessionId = null;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please revert this as it's not related to the proposed replaceFirst() change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

...ava/org/springframework/security/oauth2/server/authorization/OAuth2AuthorizationConsent.java
@@ -93,7 +93,7 @@ public Set<String> getScopes() {
Set<String> authorities = new HashSet<>();
for (GrantedAuthority authority : getAuthorities()) {
if (authority.getAuthority().startsWith(AUTHORITIES_SCOPE_PREFIX)) {
authorities.add(authority.getAuthority().replaceFirst(AUTHORITIES_SCOPE_PREFIX, ""));
authorities.add(authority.getAuthority().substring(AUTHORITIES_SCOPE_PREFIX.length()));
Copy link
Collaborator

Choose a reason for hiding this comment

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

I conducted a simple test and achieved a 40 times performance improvement.

How did you conduct the test?

Copy link
Contributor Author

@heartape heartape May 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This is all the testing information：

public static void main(String[] args) {
    String scope = "SCOPE_";
    String email = "SCOPE_email";
    int count = 10000000;

    long start1 = System.currentTimeMillis();
    for (int i = 0; i < count; i++) {
        String s = email.replaceFirst(scope, "");
    }
    long end1 = System.currentTimeMillis();
    System.out.println("replaceFirst : " + (end1 - start1));

    long start2 = System.currentTimeMillis();
    for (int i = 0; i < count; i++) {
        String s = email.substring(scope.length());
    }
    long end2 = System.currentTimeMillis();
    System.out.println("substring : " + (end2 - start2));
    System.out.println("count : " + count);
}
windows(8c16t 32G) idea 
java version "17.0.3.1" 2022-04-22 LTS
Java(TM) SE Runtime Environment (build 17.0.3.1+2-LTS-6)
Java HotSpot(TM) 64-Bit Server VM (build 17.0.3.1+2-LTS-6, mixed mode, sharing)

result:
replaceFirst : 2383
substring : 69
count : 10000000

###############################################################

centos7 vm(2c 2G)  command line 
java version "17.0.6" 2023-01-17 LTS
Java(TM) SE Runtime Environment (build 17.0.6+9-LTS-190)
Java HotSpot(TM) 64-Bit Server VM (build 17.0.6+9-LTS-190, mixed mode, sharing)

result:
replaceFirst : 2856
substring : 107
count : 10000000

@jgrandja jgrandja added status: waiting-for-feedback We need additional information before we can continue and removed status: waiting-for-triage An issue we've not yet triaged labels May 26, 2023
@jgrandja jgrandja changed the title Using substring instead of replaceFirst Use substring instead of replaceFirst in OAuth2AuthorizationConsent May 29, 2023
@jgrandja jgrandja self-assigned this May 29, 2023
@jgrandja jgrandja added status: duplicate A duplicate of another issue type: enhancement A general enhancement and removed status: waiting-for-feedback We need additional information before we can continue labels May 29, 2023
@jgrandja jgrandja added this to the 1.1.1 milestone May 29, 2023
@jgrandja
Copy link
Collaborator

Thanks for the update @heartape. This is now merged.

@jgrandja jgrandja closed this May 29, 2023
@heartape heartape deleted the gh-1222 branch June 5, 2023 05:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jgrandja jgrandja jgrandja requested changes

Assignees

@jgrandja jgrandja

Labels
status: duplicate A duplicate of another issue type: enhancement A general enhancement
Projects
None yet
Milestone
1.1.1
Development

Successfully merging this pull request may close these issues.

Use substring instead of replaceFirst in OAuth2AuthorizationConsent
3 participants
@heartape @jgrandja @spring-projects-issues