refactor: removed some code smells from few classes #5284
There are no files selected for viewing
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,54 @@ | ||
| package com.ctrip.framework.apollo.portal.component.config; | ||
|
|
||
| import com.ctrip.framework.apollo.common.config.RefreshableConfig; | ||
| import com.ctrip.framework.apollo.portal.service.PortalDBPropertySource; | ||
| import com.google.common.base.Strings; | ||
| import org.springframework.stereotype.Component; | ||
|
|
||
| /** | ||
| * This class handled email related configs for portalConfig class | ||
| */ | ||
| @Component | ||
| public class EmailConfig extends RefreshableConfig { | ||
|
|
||
| public EmailConfig(final PortalDBPropertySource portalDBPropertySource) { | ||
| super(portalDBPropertySource); | ||
| } | ||
|
|
||
| public String emailSender() { | ||
| String value = getValue(PortalConfigConstants.EMAIL_SENDER, ""); | ||
| return Strings.isNullOrEmpty(value) ? emailConfigUser() : value; | ||
| } | ||
|
|
||
| public String emailConfigHost() { | ||
| return getValue(PortalConfigConstants.EMAIL_CONFIG_HOST, ""); | ||
| } | ||
|
|
||
| public String emailConfigPassword() { | ||
| return getValue(PortalConfigConstants.EMAIL_CONFIG_PASSWORD, ""); | ||
| } | ||
|
|
||
| public String emailTemplateFramework() { | ||
| return getValue(PortalConfigConstants.EMAIL_TEMPLATE_FRAMEWORK, ""); | ||
| } | ||
|
|
||
| public String emailReleaseDiffModuleTemplate() { | ||
| return getValue(PortalConfigConstants.EMAIL_RELEASE_MODULE_DIFF, ""); | ||
| } | ||
|
|
||
| public String emailRollbackDiffModuleTemplate() { | ||
| return getValue(PortalConfigConstants.EMAIL_ROLLBACK_MODULE_DIFF, ""); | ||
| } | ||
|
|
||
| public String emailGrayRulesModuleTemplate() { | ||
| return getValue(PortalConfigConstants.EMAIL_RELEASE_MODULE_RULES, ""); | ||
| } | ||
|
|
||
| public String emailConfigUser() { | ||
| return getValue(PortalConfigConstants.EMAIL_CONFIG_USER, ""); | ||
| } | ||
|
Comment on lines
+23
to
+49
There was a problem hiding this comment. 💡 Codebase verification Based on the gathered information, I can now provide the final response: Empty email configuration values could lead to SMTP connection failures The DefaultEmailService uses these configurations for SMTP server connection, and empty values for host, user, and password would cause email delivery failures. The email templates are used for building notification emails for release, rollback, and gray deployment events.
🔗 Analysis chainAdd documentation and consider non-empty defaults The configuration methods should:
Let me gather more information about the email configuration constants and their usage in the system. Let me check one more thing about the email service implementation to understand how it handles these configurations. 🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Search for usages of these methods to understand the impact of empty values
rg -t java "emailConfigHost|emailConfigPassword|emailTemplateFramework|emailReleaseDiffModuleTemplate|emailRollbackDiffModuleTemplate|emailGrayRulesModuleTemplate|emailConfigUser" -A 2
Length of output: 11363 Script: #!/bin/bash
# Search for the constants definitions and any validation/checks on empty values
rg -t java "EMAIL_CONFIG_HOST|EMAIL_CONFIG_PASSWORD|EMAIL_TEMPLATE_FRAMEWORK|EMAIL_RELEASE_MODULE_DIFF|EMAIL_ROLLBACK_MODULE_DIFF|EMAIL_RELEASE_MODULE_RULES|EMAIL_CONFIG_USER" -A 2
# Check DefaultEmailService implementation for empty value handling
ast-grep --pattern 'class DefaultEmailService {
$$$
}'
Length of output: 4668 Script: #!/bin/bash
# Check DefaultEmailService for SMTP configuration and error handling
rg -t java -A 10 "class DefaultEmailService"
# Check if there are any default email templates in resources
fd -t f -e html -e ftl -e vm "email"
Length of output: 1513 |
||
|
|
||
| public boolean isEmailEnabled() { | ||
| return getBooleanProperty(PortalConfigConstants.EMAIL_ENABLED, false); | ||
| } | ||
| } | ||
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codebase verification
Significant breaking changes detected in RefreshableConfig
The verification reveals that this is indeed a breaking change with significant impact:
The changes are breaking because:
getRefreshablePropertySources()has been removedRefreshablePropertySourceis now mandatoryAll extending classes will need to be modified to:
getRefreshablePropertySources()RefreshablePropertySource🔗 Analysis chain
Verify impact of removing abstract modifier
The class has been changed from abstract to concrete, which is a breaking change. This could affect existing subclasses that previously implemented abstract methods.
Also applies to: 40-40
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
Length of output: 67
Script:
Length of output: 501
Script:
Length of output: 8017
Script:
Length of output: 2144