Skip to content
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

bugfix: Enhance 401 Error Handling by Refreshing Token in acquireClusterMetaData Method #6923

Open
wants to merge 6 commits into
base: 2.x
Choose a base branch
from

Conversation

o-jimin
Copy link

@o-jimin o-jimin commented Oct 14, 2024

…thod

  • I have registered the PR changes.

Ⅰ. Describe what this PR did

This pull request modifies the acquireClusterMetaData method in the RaftRegistryServiceImpl class to improve the handling of 401 Unauthorized errors. When a 401 error occurs during metadata retrieval, the code now attempts to refresh the authentication token and retry the request with the new token. This enhancement ensures that the application can recover from token expiration without manual intervention, thus improving user experience and system reliability.

Ⅱ. Does this pull request fix one issue?

fixes #6919

@funky-eyes funky-eyes changed the title Enhance 401 Error Handling by Refreshing Token in acquireClusterMetaData Method bug: Enhance 401 Error Handling by Refreshing Token in acquireClusterMetaData Method Oct 15, 2024
@funky-eyes funky-eyes added type: bug Category issues or prs related to bug. first-time contributor first-time contributor module/discovery discovery module labels Oct 15, 2024
@funky-eyes funky-eyes added this to the 2.3.0 milestone Oct 15, 2024
@funky-eyes funky-eyes changed the title bug: Enhance 401 Error Handling by Refreshing Token in acquireClusterMetaData Method bugfix: Enhance 401 Error Handling by Refreshing Token in acquireClusterMetaData Method Oct 15, 2024
@funky-eyes
Copy link
Contributor

https://github.com/apache/incubator-seata/blob/2.x/changes/zh-cn/2.x.md
https://github.com/apache/incubator-seata/blob/2.x/changes/en-us/2.x.md
请在上述md文件中登记pr信息和作者信息
Please register pr information and author information in the above md file

Copy link

codecov bot commented Oct 15, 2024

Codecov Report

Attention: Patch coverage is 0% with 6 lines in your changes missing coverage. Please review.

Project coverage is 52.56%. Comparing base (1e26c91) to head (c9b3546).

Files with missing lines Patch % Lines
...scovery/registry/raft/RaftRegistryServiceImpl.java 0.00% 6 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##                2.x    #6923      +/-   ##
============================================
+ Coverage     51.80%   52.56%   +0.75%     
- Complexity     6462     6547      +85     
============================================
  Files          1122     1122              
  Lines         39857    39860       +3     
  Branches       4668     4668              
============================================
+ Hits          20649    20952     +303     
+ Misses        17248    16910     -338     
- Partials       1960     1998      +38     
Files with missing lines Coverage Δ
...scovery/registry/raft/RaftRegistryServiceImpl.java 17.46% <0.00%> (-0.24%) ⬇️

... and 33 files with indirect coverage changes

@o-jimin
Copy link
Author

o-jimin commented Oct 15, 2024

yes I have written it

throw new AuthenticationFailedException("Authentication failed! you should configure the correct username and password.");
}
} else if (statusCode == HttpStatus.SC_UNAUTHORIZED) {
refreshToken(tcAddress);
Copy link
Contributor

Choose a reason for hiding this comment

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

why remove
(StringUtils.isNotBlank(USERNAME) && StringUtils.isNotBlank(PASSWORD)

Copy link
Author

Choose a reason for hiding this comment

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

I thought it wasn't necessary, but having validation might be good, so I made the changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

I thought it wasn't necessary, but having validation might be good, so I made the changes.

I think refreshToken(tcAddress); should be placed after the condition check; otherwise, it will waste a call when the account password is empty

Copy link
Author

Choose a reason for hiding this comment

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

I initially placed it outside the condition because refreshToken checks the account, but your point makes sense, so I moved it. Thanks for the feedback!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
first-time contributor first-time contributor module/discovery discovery module type: bug Category issues or prs related to bug.
Projects
None yet
2 participants