Open
Conversation
Thisara-Welmilla
force-pushed
the
improve-sms-config
branch
from
58b32f5 to
677ad8e
Compare
![wso2-engineering[bot]](https://avatars.githubusercontent.com/in/1856828?s=60&v=4){kind=small}
Comment on lines
+117
to
+138
|
|
||
| public Authentication build() { | ||
|
|
||
| switch (authType) { | ||
| case BASIC: | ||
| resolvedAuthProperties.put(USERNAME_AUTH_PROP, getProperty(propertiesMap, USERNAME_AUTH_PROP)); | ||
| resolvedAuthProperties.put(PASSWORD_AUTH_PROP, getProperty(propertiesMap, PASSWORD_AUTH_PROP)); | ||
| break; | ||
| case BEARER: | ||
| resolvedAuthProperties.put( | ||
| ACCESS_TOKEN_AUTH_PROP, getProperty(propertiesMap, ACCESS_TOKEN_AUTH_PROP)); | ||
| break; | ||
| case CLIENT_CREDENTIAL: | ||
| resolvedAuthProperties.put(CLIENT_ID_AUTH_PROP, getProperty(propertiesMap, CLIENT_ID_AUTH_PROP)); | ||
| resolvedAuthProperties.put( | ||
| CLIENT_SECRET_AUTH_PROP, getProperty(propertiesMap, CLIENT_SECRET_AUTH_PROP)); | ||
| resolvedAuthProperties.put(SCOPE_AUTH_PROP, getProperty(propertiesMap, SCOPE_AUTH_PROP)); | ||
| break; | ||
| case API_KEY: | ||
| resolvedAuthProperties.put(HEADER_AUTH_PROP, getProperty(propertiesMap, HEADER_AUTH_PROP)); | ||
| resolvedAuthProperties.put(VALUE_AUTH_PROP, getProperty(propertiesMap, VALUE_AUTH_PROP)); | ||
| break; |
There was a problem hiding this comment.
Log Improvement Suggestion No: 1
Suggested change
| public Authentication build() { | |
| switch (authType) { | |
| case BASIC: | |
| resolvedAuthProperties.put(USERNAME_AUTH_PROP, getProperty(propertiesMap, USERNAME_AUTH_PROP)); | |
| resolvedAuthProperties.put(PASSWORD_AUTH_PROP, getProperty(propertiesMap, PASSWORD_AUTH_PROP)); | |
| break; | |
| case BEARER: | |
| resolvedAuthProperties.put( | |
| ACCESS_TOKEN_AUTH_PROP, getProperty(propertiesMap, ACCESS_TOKEN_AUTH_PROP)); | |
| break; | |
| case CLIENT_CREDENTIAL: | |
| resolvedAuthProperties.put(CLIENT_ID_AUTH_PROP, getProperty(propertiesMap, CLIENT_ID_AUTH_PROP)); | |
| resolvedAuthProperties.put( | |
| CLIENT_SECRET_AUTH_PROP, getProperty(propertiesMap, CLIENT_SECRET_AUTH_PROP)); | |
| resolvedAuthProperties.put(SCOPE_AUTH_PROP, getProperty(propertiesMap, SCOPE_AUTH_PROP)); | |
| break; | |
| case API_KEY: | |
| resolvedAuthProperties.put(HEADER_AUTH_PROP, getProperty(propertiesMap, HEADER_AUTH_PROP)); | |
| resolvedAuthProperties.put(VALUE_AUTH_PROP, getProperty(propertiesMap, VALUE_AUTH_PROP)); | |
| break; | |
| public Authentication build() { | |
| switch (authType) { | |
| case BASIC: | |
| resolvedAuthProperties.put(USERNAME_AUTH_PROP, getProperty(propertiesMap, USERNAME_AUTH_PROP)); | |
| resolvedAuthProperties.put(PASSWORD_AUTH_PROP, getProperty(propertiesMap, PASSWORD_AUTH_PROP)); | |
| log.info("Building authentication configuration with type: BASIC"); | |
| break; | |
| case BEARER: | |
| resolvedAuthProperties.put( | |
| ACCESS_TOKEN_AUTH_PROP, getProperty(propertiesMap, ACCESS_TOKEN_AUTH_PROP)); | |
| log.info("Building authentication configuration with type: BEARER"); | |
| break; | |
| case CLIENT_CREDENTIAL: | |
| resolvedAuthProperties.put(CLIENT_ID_AUTH_PROP, getProperty(propertiesMap, CLIENT_ID_AUTH_PROP)); | |
| resolvedAuthProperties.put( | |
| CLIENT_SECRET_AUTH_PROP, getProperty(propertiesMap, CLIENT_SECRET_AUTH_PROP)); | |
| resolvedAuthProperties.put(SCOPE_AUTH_PROP, getProperty(propertiesMap, SCOPE_AUTH_PROP)); | |
| log.info("Building authentication configuration with type: CLIENT_CREDENTIAL"); | |
| break; | |
| case API_KEY: | |
| resolvedAuthProperties.put(HEADER_AUTH_PROP, getProperty(propertiesMap, HEADER_AUTH_PROP)); | |
| resolvedAuthProperties.put(VALUE_AUTH_PROP, getProperty(propertiesMap, VALUE_AUTH_PROP)); | |
| log.info("Building authentication configuration with type: API_KEY"); | |
| break; | |
| case NONE: | |
| log.info("Building authentication configuration with type: NONE"); | |
| break; |
Comment on lines
+148
to
+160
| // Throw an error and properly handle in the API server level | ||
| private String getProperty(Map<String, String> actionEndpointProperties, String propName) { | ||
|
|
||
| if (actionEndpointProperties != null && actionEndpointProperties.containsKey(propName)) { | ||
| String propValue = actionEndpointProperties.get(propName); | ||
| if (StringUtils.isNotBlank(propValue)) { | ||
| return propValue; | ||
| } | ||
| throw new IllegalArgumentException(String.format("The Property %s cannot be blank.", propName)); | ||
| } | ||
|
|
||
| throw new NoSuchElementException(String.format("The property %s must be provided as an authentication " + | ||
| "property for the %s authentication type.", propName, authType.name())); |
There was a problem hiding this comment.
Log Improvement Suggestion No: 2
Suggested change
| // Throw an error and properly handle in the API server level | |
| private String getProperty(Map<String, String> actionEndpointProperties, String propName) { | |
| if (actionEndpointProperties != null && actionEndpointProperties.containsKey(propName)) { | |
| String propValue = actionEndpointProperties.get(propName); | |
| if (StringUtils.isNotBlank(propValue)) { | |
| return propValue; | |
| } | |
| throw new IllegalArgumentException(String.format("The Property %s cannot be blank.", propName)); | |
| } | |
| throw new NoSuchElementException(String.format("The property %s must be provided as an authentication " + | |
| "property for the %s authentication type.", propName, authType.name())); | |
| private String getProperty(Map<String, String> actionEndpointProperties, String propName) { | |
| if (actionEndpointProperties != null && actionEndpointProperties.containsKey(propName)) { | |
| String propValue = actionEndpointProperties.get(propName); | |
| if (StringUtils.isNotBlank(propValue)) { | |
| if (log.isDebugEnabled()) { | |
| log.debug("Retrieved property: " + propName + " for authentication type: " + authType.name()); | |
| } | |
| return propValue; | |
| } | |
| log.error("The Property " + propName + " cannot be blank."); | |
| throw new IllegalArgumentException(String.format("The Property %s cannot be blank.", propName)); | |
| } | |
| log.error("Missing required property: " + propName + " for authentication type: " + authType.name()); | |
| throw new NoSuchElementException(String.format("The property %s must be provided as an authentication " + | |
| "property for the %s authentication type.", propName, authType.name())); |
Comment on lines
+151
to
+153
| return tokenManager.getToken(); | ||
| } catch (Exception e) { | ||
| throw new IllegalArgumentException("Error while acquiring token for the SMS sender", e); |
There was a problem hiding this comment.
Log Improvement Suggestion No: 3
Suggested change
| return tokenManager.getToken(); | |
| } catch (Exception e) { | |
| throw new IllegalArgumentException("Error while acquiring token for the SMS sender", e); | |
| } catch (Exception e) { | |
| log.error("Error while acquiring token for the SMS sender: " + e.getMessage()); | |
| throw new IllegalArgumentException("Error while acquiring token for the SMS sender", e); |
Comment on lines
+234
to
+235
|
|
||
| SMSSenderDTO smsSenderDTO = new SMSSenderDTO(); |
There was a problem hiding this comment.
Log Improvement Suggestion No: 4
Suggested change
| SMSSenderDTO smsSenderDTO = new SMSSenderDTO(); | |
| SMSSenderDTO smsSenderDTO = new SMSSenderDTO(); | |
| log.debug("Building SMS sender configuration with name: " + this.name); | |
| if (StringUtils.isNotEmpty(authType)) { |
Comment on lines
+213
to
+216
| public static void addAuthenticationProperties(Map<String, String> mapToBeUpdated, Authentication authentication) { | ||
|
|
||
| if (authentication != null) { | ||
| mapToBeUpdated.put(AUTH_TYPE_PREFIX, authentication.getType().name()); |
There was a problem hiding this comment.
Log Improvement Suggestion No: 5
Suggested change
| public static void addAuthenticationProperties(Map<String, String> mapToBeUpdated, Authentication authentication) { | |
| if (authentication != null) { | |
| mapToBeUpdated.put(AUTH_TYPE_PREFIX, authentication.getType().name()); | |
| public static void addAuthenticationProperties(Map<String, String> mapToBeUpdated, Authentication authentication) { | |
| if (authentication != null) { | |
| log.debug("Adding authentication properties for type: " + authentication.getType().name()); | |
| mapToBeUpdated.put(AUTH_TYPE_PREFIX, authentication.getType().name()); |
Comment on lines
+509
to
+511
|
|
||
| try { | ||
| SMSSenderDTO smsSenderDTO = smsSenderBuilder.build(); |
There was a problem hiding this comment.
Log Improvement Suggestion No: 6
Suggested change
| try { | |
| SMSSenderDTO smsSenderDTO = smsSenderBuilder.build(); | |
| try { | |
| SMSSenderDTO smsSenderDTO = smsSenderBuilder.build(); | |
| if (log.isDebugEnabled()) { | |
| log.debug("Built SMS sender configuration for: " + smsSenderDTO.getName()); | |
| } | |
| internalAuthProp.keySet().forEach(internalAuthProperty -> |
Comment on lines
+46
to
+53
|
|
||
| public String getToken() throws TokenHandlerException { | ||
|
|
||
| // try with refresh, if any error new token | ||
| TokenRequestContext tokenRequestContext = buildTokenRequestContext(authentication); | ||
| tokenAcquirerService.setTokenRequestContext(tokenRequestContext); | ||
| TokenResponse tokenResponse = tokenAcquirerService.getNewAccessToken(); | ||
| return tokenResponse.getAccessToken(); |
There was a problem hiding this comment.
Log Improvement Suggestion No: 7
Suggested change
| public String getToken() throws TokenHandlerException { | |
| // try with refresh, if any error new token | |
| TokenRequestContext tokenRequestContext = buildTokenRequestContext(authentication); | |
| tokenAcquirerService.setTokenRequestContext(tokenRequestContext); | |
| TokenResponse tokenResponse = tokenAcquirerService.getNewAccessToken(); | |
| return tokenResponse.getAccessToken(); | |
| public String getToken() throws TokenHandlerException { | |
| log.info("Initiating token acquisition process"); | |
| // try with refresh, if any error new token | |
| TokenRequestContext tokenRequestContext = buildTokenRequestContext(authentication); | |
| tokenAcquirerService.setTokenRequestContext(tokenRequestContext); | |
| TokenResponse tokenResponse = tokenAcquirerService.getNewAccessToken(); | |
| if (log.isDebugEnabled()) { | |
| log.debug("Successfully acquired new access token"); | |
| } | |
| return tokenResponse.getAccessToken(); | |
| } |
Comment on lines
+57
to
+72
|
|
||
| private TokenRequestContext buildTokenRequestContext(Authentication authentication) throws TokenHandlerException { | ||
|
|
||
| // only for credentials grant type <= think where this check should go | ||
| // properly set the grant context details with switch | ||
| GrantContext grantContext = new GrantContext.Builder() | ||
| .grantType(GrantContext.GrantType.CLIENT_CREDENTIAL) | ||
| .properties(authentication.getProperties()) | ||
| .build(); | ||
|
|
||
| // get endpoint url from prop | ||
| TokenRequestContext.Builder builder = new TokenRequestContext.Builder() | ||
| .grantContext(grantContext) | ||
| .endpointUrl("https://customauth.free.beeceptor.com/todos"); | ||
|
|
||
| return builder.build(); |
There was a problem hiding this comment.
Log Improvement Suggestion No: 8
Suggested change
| private TokenRequestContext buildTokenRequestContext(Authentication authentication) throws TokenHandlerException { | |
| // only for credentials grant type <= think where this check should go | |
| // properly set the grant context details with switch | |
| GrantContext grantContext = new GrantContext.Builder() | |
| .grantType(GrantContext.GrantType.CLIENT_CREDENTIAL) | |
| .properties(authentication.getProperties()) | |
| .build(); | |
| // get endpoint url from prop | |
| TokenRequestContext.Builder builder = new TokenRequestContext.Builder() | |
| .grantContext(grantContext) | |
| .endpointUrl("https://customauth.free.beeceptor.com/todos"); | |
| return builder.build(); | |
| private TokenRequestContext buildTokenRequestContext(Authentication authentication) throws TokenHandlerException { | |
| if (log.isDebugEnabled()) { | |
| log.debug("Building token request context for grant type: CLIENT_CREDENTIAL"); | |
| } | |
| // only for credentials grant type <= think where this check should go | |
| // properly set the grant context details with switch | |
| GrantContext grantContext = new GrantContext.Builder() | |
| .grantType(GrantContext.GrantType.CLIENT_CREDENTIAL) | |
| .properties(authentication.getProperties()) | |
| .build(); | |
| // get endpoint url from prop | |
| TokenRequestContext.Builder builder = new TokenRequestContext.Builder() | |
| .grantContext(grantContext) | |
| .endpointUrl("https://customauth.free.beeceptor.com/todos"); | |
| return builder.build(); | |
| } |
There was a problem hiding this comment.
AI Agent Log Improvement Checklist
- The log-related comments and suggestions in this review were generated by an AI tool to assist with identifying potential improvements. Purpose of reviewing the code for log improvements is to improve the troubleshooting capabilities of our products.
- Please make sure to manually review and validate all suggestions before applying any changes. Not every code suggestion would make sense or add value to our purpose. Therefore, you have the freedom to decide which of the suggestions are helpful.
✅ Before merging this pull request:
- Review all AI-generated comments for accuracy and relevance.
- Complete and verify the table below. We need your feedback to measure the accuracy of these suggestions and the value they add. If you are rejecting a certain code suggestion, please mention the reason briefly in the suggestion for us to capture it.
Thisara-Welmilla
force-pushed
the
improve-sms-config
branch
from
677ad8e to
2169032
Compare
Thisara-Welmilla
force-pushed
the
improve-sms-config
branch
from
2169032 to
ea9d7d6
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proposed changes in this pull request
[List all changes you want to add here. If you fixed an issue, please
add a reference to that issue as well.]
When should this PR be merged
[Please describe any preconditions that need to be addressed before we
can merge this pull request.]
Follow up actions
[List any possible follow-up actions here; for instance, testing data
migrations, software that we need to install on staging and production
environments.]
Developer Checklist (Mandatory)
product-isissue to track any behavioral change or migration impact.Checklist (for reviewing)
General
Functionality
Code
Tests
Security
Documentation