refactor: removed some code smells from few classes #5284
Conversation
dosubot
bot
added
the
size:XL
WalkthroughThe pull request includes significant updates across multiple classes in the Apollo project, primarily focusing on configuration management and service enhancements. Key changes involve refactoring constructors and method signatures, introducing new configuration classes, and improving the handling of property sources. Notable modifications include the removal of certain methods, the introduction of new methods for better functionality, and the restructuring of existing classes to enhance readability and maintainability. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Outside diff range and nitpick comments (26)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/ImportTaskService.java (2)
3-6: Enhance JavaDoc documentation for better clarity and completeness.The current documentation could be more comprehensive. Consider adding:
@param Tto document the generic type parameter@sincetag to track API introduction- Examples of typical usage
- Exception handling expectations
/** - * - * Used in ConfigsImportService class to remove duplication from importEntities method + * A functional interface that defines a single operation to be performed during entity import. + * This interface is primarily used in ConfigsImportService to streamline the importEntities method + * and reduce code duplication across different entity types. + * + * @param <T> the type of entity being imported + * @since 2.0.0 + * + * @see com.ctrip.framework.apollo.portal.service.ConfigsImportService */
7-10: Consider adding bounds to the generic type parameter and improving method naming.The interface implementation looks good, but consider these improvements:
- Add bounds to the generic type parameter if specific types are expected
- Consider a more specific method name than 'execute' (e.g., 'importEntity')
- Document expected exception handling behavior
@FunctionalInterface -public interface ImportTaskService<T> { - void execute(T entity); +public interface ImportTaskService<T> { + /** + * Performs the import operation for a single entity. + * + * @param entity the entity to be imported + * @throws IllegalArgumentException if the entity is invalid + */ + void execute(T entity); }apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/config/EmailConfig.java (4)
8-10: Enhance class documentationWhile the current documentation indicates the purpose, it could be more descriptive by including:
- The relationship with PortalConfig (part of the refactoring)
- The configuration properties it manages
- Usage examples or important notes
/** - * This class handled email related configs for portalConfig class + * Manages email-related configurations that were previously part of PortalConfig. + * This class is responsible for providing email settings such as SMTP configuration, + * templates, and general email functionality controls. + * + * @see PortalConfig + * @see RefreshableConfig */
14-16: Add null check for constructor parameterConsider adding parameter validation to prevent NullPointerException.
public EmailConfig(final PortalDBPropertySource portalDBPropertySource) { + Objects.requireNonNull(portalDBPropertySource, "portalDBPropertySource must not be null"); super(portalDBPropertySource); }
18-21: Document fallback behavior in emailSender()The fallback to
emailConfigUser()when sender is empty should be documented for clarity.+ /** + * Returns the email sender address. If not explicitly configured, + * falls back to the email config user as the sender. + * + * @return the configured sender address or the email config user + */ public String emailSender() {
1-54: Good separation of concernsThe extraction of email configuration into a dedicated class aligns well with the Single Responsibility Principle and the PR's objective of removing code smells. The use of
RefreshableConfigand Spring's dependency injection demonstrates good architectural practices.Consider adding integration tests to verify the interaction between this new configuration class and the original
PortalConfigclass.Would you like me to help create integration tests for this configuration class?
apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/config/ConfigTest.java (1)
Line range hint
29-29: Consider updating the test class name for clarity.Since this test class now tests
RefreshableConfiginstead ofPortalConfig, consider renaming it toRefreshableConfigTestor adding a comment to clarify its current purpose.-public class ConfigTest extends AbstractUnitTest{ +public class RefreshableConfigTest extends AbstractUnitTest{apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/config/EnvConfig.java (3)
12-14: Enhance class documentationThe current documentation is brief and could be more descriptive. Consider adding:
- The class's primary responsibilities
- Description of key configuration properties it manages
- Relationship with PortalConfig and why it was extracted
/** - * This class handled environment related configs for portalConfig class + * Manages environment-related configurations for the Apollo portal. + * + * This class is responsible for: + * - Managing supported environments (FAT, UAT, PRO, etc.) + * - Configuring environment-specific settings for email, webhooks, and publishing + * - Providing search result limitations per environment + * + * Originally extracted from PortalConfig to improve separation of concerns and reduce code complexity. */
22-31: Consider caching the supported environments listThe
portalSupportedEnvs()method creates a new list on each invocation. Since environment configurations don't change frequently, consider caching the result and refreshing it only when the underlying property changes.public class EnvConfig extends RefreshableConfig { + private volatile List<Env> cachedPortalSupportedEnvs; + public List<Env> portalSupportedEnvs() { + if (cachedPortalSupportedEnvs == null) { + synchronized (this) { + if (cachedPortalSupportedEnvs == null) { String[] configurations = getArrayProperty(PortalConfigConstants.APOLLO_PORTAL_ENVS, new String[]{"FAT", "UAT", "PRO"}); List<Env> envs = Lists.newLinkedList(); for (String env : configurations) { envs.add(Env.addEnvironment(env)); } + cachedPortalSupportedEnvs = envs; + } + } + } + return Lists.newArrayList(cachedPortalSupportedEnvs); }
33-35: Extract magic number to constantThe default value of 200 for maximum search results should be extracted to a constant for better maintainability.
+ private static final int DEFAULT_PER_ENV_MAX_RESULTS = 200; + public int getPerEnvSearchMaxResults() { - return getIntProperty(PortalConfigConstants.PER_ENV_MAX_RESULTS, 200); + return getIntProperty(PortalConfigConstants.PER_ENV_MAX_RESULTS, DEFAULT_PER_ENV_MAX_RESULTS); }apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/auth/ConsumerPermissionValidatorTest.java (1)
56-62: Consider splitting test cases for better readability.While the test cases are comprehensive, consider splitting them into separate test methods for better readability and maintainability:
- One test for successful permission checks
- One test for failed permission checks
Example refactor:
- public void testConsumerHasPermission() throws Exception { + public void testConsumerHasPermissionSuccessful() throws Exception { String someTargetId = "someTargetId"; String anotherTargetId = "anotherTargetId"; String somePermissionType = "somePermissionType"; String anotherPermissionType = "anotherPermissionType"; long someConsumerId = 1; long anotherConsumerId = 2; - long someConsumerWithNoPermission = 3; assertTrue(consumerPermissionValidator.consumerHasPermission(someConsumerId, somePermissionType, someTargetId)); assertTrue(consumerPermissionValidator.consumerHasPermission(someConsumerId, anotherPermissionType, anotherTargetId)); assertTrue(consumerPermissionValidator.consumerHasPermission(anotherConsumerId, somePermissionType, someTargetId)); assertTrue(consumerPermissionValidator.consumerHasPermission(anotherConsumerId, anotherPermissionType, anotherTargetId)); + } + @Test + @Sql(scripts = "/sql/permission/insert-test-roles.sql", executionPhase = Sql.ExecutionPhase.BEFORE_TEST_METHOD) + @Sql(scripts = "/sql/permission/insert-test-permissions.sql", executionPhase = Sql.ExecutionPhase.BEFORE_TEST_METHOD) + @Sql(scripts = "/sql/permission/insert-test-consumerroles.sql", executionPhase = Sql.ExecutionPhase.BEFORE_TEST_METHOD) + @Sql(scripts = "/sql/permission/insert-test-rolepermissions.sql", executionPhase = Sql.ExecutionPhase.BEFORE_TEST_METHOD) + @Sql(scripts = "/sql/cleanup.sql", executionPhase = Sql.ExecutionPhase.AFTER_TEST_METHOD) + public void testConsumerHasPermissionFailed() throws Exception { + String someTargetId = "someTargetId"; + String anotherTargetId = "anotherTargetId"; + String somePermissionType = "somePermissionType"; + String anotherPermissionType = "anotherPermissionType"; + long someConsumerWithNoPermission = 3; + assertFalse(consumerPermissionValidator.consumerHasPermission(someConsumerWithNoPermission, somePermissionType, someTargetId)); assertFalse(consumerPermissionValidator.consumerHasPermission(someConsumerWithNoPermission, anotherPermissionType, anotherTargetId)); }apollo-common/src/main/java/com/ctrip/framework/apollo/common/config/RefreshableConfig.java (3)
57-65: Consider enhancing constructor flexibilityThe current implementation has several potential improvements:
- The constructor accepts a single source but wraps it in a List, which might be limiting
- The private getter method
getPropertySources()appears redundant as it simply returns the fieldConsider these improvements:
- private List<RefreshablePropertySource> propertySources; + private final List<RefreshablePropertySource> propertySources; - public RefreshableConfig(RefreshablePropertySource propertySources) { - this.propertySources = Collections.singletonList(propertySources); + public RefreshableConfig(RefreshablePropertySource... propertySources) { + this.propertySources = Collections.unmodifiableList(Arrays.asList(propertySources)); } - private List<RefreshablePropertySource> getPropertySources() { - return propertySources; - }This would:
- Allow multiple property sources to be passed
- Make the list immutable
- Remove the redundant getter
Line range hint
70-74: Fix potential issues in setup methodThere are several concerns with the current implementation:
- The assignment
propertySources = getPropertySources()is redundant since the field is already initialized in the constructor- The null check could be moved to the constructor for earlier validation
- There's a potential race condition between @PostConstruct and other methods accessing propertySources
Consider this improvement:
- propertySources = getPropertySources(); - if (CollectionUtils.isEmpty(propertySources)) { - throw new IllegalStateException("Property sources can not be empty."); - } + // Property sources are validated in constructor, just proceed with setup //add property source to environment for (RefreshablePropertySource propertySource : propertySources) {And add validation to the constructor:
public RefreshableConfig(RefreshablePropertySource propertySources) { + if (propertySources == null) { + throw new IllegalArgumentException("Property source cannot be null"); + } this.propertySources = Collections.singletonList(propertySources); }
Line range hint
46-47: Consider making refresh interval configurable and manage executor lifecycle
- The refresh interval is hardcoded to 60 seconds. Consider making this configurable through environment properties.
- The executor service should be properly managed to prevent resource leaks.
Consider these improvements:
- private static final int CONFIG_REFRESH_INTERVAL = 60; + private static final String CONFIG_REFRESH_INTERVAL_KEY = "apollo.config.refresh.interval"; + private static final int DEFAULT_REFRESH_INTERVAL = 60; + private ScheduledExecutorService executorService; @PostConstruct public void setup() { - ScheduledExecutorService executorService = + executorService = Executors.newScheduledThreadPool(1, ApolloThreadFactory.create("ConfigRefresher", true)); + int refreshInterval = getIntProperty(CONFIG_REFRESH_INTERVAL_KEY, DEFAULT_REFRESH_INTERVAL); executorService.scheduleWithFixedDelay(... } + @PreDestroy + public void destroy() { + if (executorService != null) { + executorService.shutdown(); + try { + if (!executorService.awaitTermination(5, TimeUnit.SECONDS)) { + executorService.shutdownNow(); + } + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + } + } + }apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/service/config/ConfigServiceWithCache.java (1)
126-130: Consider enhancing the extracted method with validation and naming improvements.While the method extraction is a good improvement, consider these enhancements:
- Add parameter validation
- Use a more specific name for the boolean variable
- Consider a more concise return statement
Here's a suggested improvement:
private static boolean outdatedCacheCheck(ApolloNotificationMessages clientMessages, String messageKey, ConfigCacheEntry cacheEntry) { - boolean clientMessagesHasMessageKey = clientMessages != null && clientMessages.has(messageKey); - return clientMessagesHasMessageKey && - clientMessages.get(messageKey) > cacheEntry.getNotificationId(); + if (clientMessages == null || messageKey == null || cacheEntry == null) { + return false; + } + return clientMessages.has(messageKey) && + clientMessages.get(messageKey) > cacheEntry.getNotificationId(); }apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/ReleaseService.java (1)
222-235: Add parameter validation and documentation.Consider adding:
- Null parameter validation
- JavaDoc documentation explaining the method's purpose and parameters
- Comments explaining the gray release logic
+ /** + * Compares releases based on release history information. + * For gray releases with no previous release, compares the latest master release with the branch release. + * For all other cases, compares the previous release with the current release. + * + * @param env The environment where the releases exist + * @param releaseHistory The release history information containing release IDs and operation type + * @return A comparison result of the two releases + * @throws IllegalArgumentException if releaseHistory is null + */ public ReleaseCompareResult compareReleaseHistory(Env env, ReleaseHistoryBO releaseHistory) { + if (releaseHistory == null) { + throw new IllegalArgumentException("Release history cannot be null"); + } + + // For gray releases without a previous release, compare with the latest master release if (releaseHistory.getOperation() == ReleaseOperation.GRAY_RELEASE && releaseHistory.getPreviousReleaseId() == 0) { - // Load the latest master release and branch release + // Compare the latest master release with the current branch release to show changes ReleaseDTO masterLatestActiveRelease = loadLatestRelease( releaseHistory.getAppId(), env, releaseHistory.getClusterName(), releaseHistory.getNamespaceName()); ReleaseDTO branchLatestActiveRelease = findReleaseById(env, releaseHistory.getReleaseId()); return compare(masterLatestActiveRelease, branchLatestActiveRelease); } - // Compare based on previous release and current release + // For normal releases or gray releases with a previous release, compare the two releases directly return compare(env, releaseHistory.getPreviousReleaseId(), releaseHistory.getReleaseId()); }apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/ReleaseController.java (2)
101-102: Consider extracting common validation logic.This validation pattern is repeated across multiple methods. Consider extracting it into a private helper method to reduce duplication.
+ private void validateReleaseParams(String releasedBy, String releaseTitle) { + boolean doesContainEmpty = StringUtils.isContainEmpty(releasedBy, releaseTitle); + RequestPrecondition.checkArguments(!doesContainEmpty, + "Params(releaseTitle and releasedBy) can not be empty"); + if (userService.findByUserId(releasedBy) == null) { + throw BadRequestException.userNotExists(releasedBy); + } + }
182-188: Consider standardizing environment case conversion.The
env.toUpperCase()conversion is sometimes done in the calling method (line 153) and sometimes in this helper method (line 184). Consider standardizing this to prevent potential inconsistencies.- private void populateReleaseModel(NamespaceReleaseModel releaseModel, String appId, String env, String branchName, String namespaceName) { + private void populateReleaseModel(NamespaceReleaseModel releaseModel, String appId, String env, String branchName, String namespaceName) { releaseModel.setAppId(appId); - releaseModel.setEnv(env.toUpperCase()); + releaseModel.setEnv(env); // Assume env is already uppercase from Env.valueOf(env).toString() releaseModel.setClusterName(branchName); releaseModel.setNamespaceName(namespaceName); }apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/config/BizConfig.java (1)
Line range hint
1-248: Consider further modularization of configuration concernsWhile the constructor refactoring is a good step, the class still handles many different types of configurations (eureka, polling, caching, access control, etc.). Consider splitting these into more focused configuration classes to improve maintainability and follow the Single Responsibility Principle.
Potential groupings:
- Cache-related configs (namespace, access key, release message)
- Security-related configs (access control, tokens)
- Limits-related configs (item key/value lengths, namespace limits)
- Release-related configs (gray release, release history)
Would you like me to propose a detailed refactoring plan for this modularization?
apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/grayReleaseRule/GrayReleaseRulesHolder.java (1)
160-170: Good readability improvement, consider minor variable name refinementsThe refactoring improves readability by introducing explanatory variables. However, the variable names could be even more precise:
- String ruleKeyViaClientIp = assembleReversedGrayReleaseRuleKey(clientAppId, - namespaceName, clientIp); - String ruleKeyViaItemDTO = assembleReversedGrayReleaseRuleKey(clientAppId, namespaceName, GrayReleaseRuleItemDTO - .ALL_IP); - - boolean hasScheduleForThisKey = reversedGrayReleaseRuleCache.containsKey(ruleKeyViaClientIp); - boolean hasScheduleForThatKey = reversedGrayReleaseRuleCache.containsKey - (ruleKeyViaItemDTO); + String specificClientIpKey = assembleReversedGrayReleaseRuleKey(clientAppId, + namespaceName, clientIp); + String wildcardIpKey = assembleReversedGrayReleaseRuleKey(clientAppId, namespaceName, GrayReleaseRuleItemDTO + .ALL_IP); + + boolean hasRuleForSpecificIp = reversedGrayReleaseRuleCache.containsKey(specificClientIpKey); + boolean hasRuleForAllIps = reversedGrayReleaseRuleCache.containsKey + (wildcardIpKey); - return hasScheduleForThisKey || hasScheduleForThatKey; + return hasRuleForSpecificIp || hasRuleForAllIps;The suggested variable names better reflect their purpose:
specificClientIpKeyclearly indicates it's for a specific client IPwildcardIpKeyindicates it's for the ALL_IP casehasRuleForSpecificIpandhasRuleForAllIpsare more descriptive than the generic "schedule" terminologyapollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/config/SecurityConfig.java (4)
14-15: Typographical Error in Class DocumentationThe class comment contains a grammatical error and should be updated for clarity and correctness.
Suggested change:
- * This class handled security related configs for portalConfig class + * This class handles security-related configurations for the portal.
37-37: Avoid Unnecessary Dependency on Guava'sLists.newArrayListTo convert the
String[]to aList<String>, you can useArrays.asList(value), eliminating the dependency on Guava'sLists.newArrayList.Suggested change:
- return Lists.newArrayList(value); + return Arrays.asList(value);This simplifies the code and reduces external dependencies.
45-46: Centralize Configuration Constants inPortalConfigConstantsThe use of constants from
SystemRoleManagerServicefor configuration keys is not ideal. Consider moving these constants toPortalConfigConstantsto enhance modularity and maintainability.Suggested approach:
- Move
CREATE_APPLICATION_LIMIT_SWITCH_KEYandMANAGE_APP_MASTER_LIMIT_SWITCH_KEYtoPortalConfigConstants.- Update the methods to reference the new constants.
- return getBooleanProperty(SystemRoleManagerService.CREATE_APPLICATION_LIMIT_SWITCH_KEY, false); + return getBooleanProperty(PortalConfigConstants.CREATE_APPLICATION_LIMIT_SWITCH_KEY, false);And similarly for
MANAGE_APP_MASTER_LIMIT_SWITCH_KEY.Also applies to: 49-50
55-61: Optimize Environment Check with Stream APIIn the
isConfigViewMemberOnlymethod, using a loop to check for the environment can be replaced with a more efficient and readable approach using the Stream API.Suggested change:
- for (String memberOnlyEnv : configViewMemberOnlyEnvs) { - if (memberOnlyEnv.equalsIgnoreCase(env)) { - return true; - } - } - return false; + return Arrays.stream(configViewMemberOnlyEnvs) + .anyMatch(memberOnlyEnv -> memberOnlyEnv.equalsIgnoreCase(env));This enhances readability and may improve performance.
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/auth/ConsumerPermissionValidator.java (1)
46-53: Reduce Constructor Dependencies for Better MaintainabilityThe constructor now requires multiple repository dependencies, which increases coupling and can make testing more complex. Consider refactoring by creating a service or a composite object that encapsulates these repositories. This approach will enhance maintainability and simplify dependency management.
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/ConfigsImportService.java (1)
191-208: Consider usingCompletableFuturefor better asynchronous handlingUsing
parallelStream()in combination withCountDownLatchmay not be the most efficient approach for handling asynchronous tasks. TheCompletableFutureAPI can provide better control over asynchronous execution, improved error handling, and enhanced readability.Here is a possible refactoring using
CompletableFuture:-private <T> void importEntities(String entityName, List<T> entities, ImportTaskService<T> importTask) throws InterruptedException { LOGGER.info("Start to import {}. size = {}", entityName, entities.size()); long startTime = System.currentTimeMillis(); CountDownLatch latch = new CountDownLatch(entities.size()); entities.parallelStream().forEach(entity -> { try { importTask.execute(entity); } catch (Exception e) { LOGGER.error("Import {} error. entity = {}", entityName, entity, e); } finally { latch.countDown(); } }); latch.await(); LOGGER.info("Finish to import {}. duration = {}", entityName, System.currentTimeMillis() - startTime); } +private <T> void importEntities(String entityName, List<T> entities, ImportTaskService<T> importTask) { + LOGGER.info("Start to import {}. size = {}", entityName, entities.size()); + + long startTime = System.currentTimeMillis(); + + List<CompletableFuture<Void>> futures = entities.stream() + .map(entity -> CompletableFuture.runAsync(() -> { + try { + importTask.execute(entity); + } catch (Exception e) { + LOGGER.error("Import {} error. entity = {}", entityName, entity, e); + } + })) + .collect(Collectors.toList()); + + CompletableFuture.allOf(futures.toArray(new CompletableFuture[0])).join(); + LOGGER.info("Finish to import {}. duration = {}", entityName, System.currentTimeMillis() - startTime); +}This refactoring eliminates the need for
CountDownLatchand leverages theCompletableFutureAPI for asynchronous task execution and synchronization.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (22)
apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/config/BizConfig.java(1 hunks)apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/grayReleaseRule/GrayReleaseRulesHolder.java(1 hunks)apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/service/ReleaseHistoryService.java(0 hunks)apollo-build-sql-converter/pom.xml(1 hunks)apollo-common/src/main/java/com/ctrip/framework/apollo/common/config/RefreshableConfig.java(3 hunks)apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/service/config/ConfigServiceWithCache.java(2 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/auth/ConsumerPermissionValidator.java(2 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/ReleaseController.java(6 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/config/EmailConfig.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/config/EnvConfig.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/config/PortalConfig.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/config/PortalConfigConstants.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/config/SecurityConfig.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/config/ServerConfig.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/emailbuilder/ConfigPublishEmailBuilder.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/listener/ConfigPublishListener.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/ConfigsImportService.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/ImportTaskService.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/ReleaseService.java(2 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/spi/defaultimpl/DefaultRoleInitializationService.java(1 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/auth/ConsumerPermissionValidatorTest.java(3 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/config/ConfigTest.java(2 hunks)
💤 Files with no reviewable changes (1)
- apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/service/ReleaseHistoryService.java
✅ Files skipped from review due to trivial changes (4)
- apollo-build-sql-converter/pom.xml
- apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/config/PortalConfigConstants.java
- apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/listener/ConfigPublishListener.java
- apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/spi/defaultimpl/DefaultRoleInitializationService.java
🔇 Additional comments (22)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/ImportTaskService.java (1)
1-10: Verify the interface implementation and usage.
Let's confirm that this interface is being used effectively to reduce code duplication.
✅ Verification successful
Let me analyze the usage of ImportTaskService based on the results:
- The interface is used in ConfigsImportService.java for importing different types of entities:
importEntities("app", toImportApps, app -> importApp(app, importEnvs, operator));
importEntities("appnamespace", toImportAppNSs, appNS -> importAppNamespace(appNS, operator));
importEntities("cluster", toImportClusters, cluster -> importCluster(cluster, operator));
importEntities("namespace", toImportNSs, namespace -> importNamespaceFromText(...));- The generic importEntities method implementation shows proper usage of the functional interface:
private <T> void importEntities(String entityName, List<T> entities, ImportTaskService<T> importTask) {
// Parallel processing with error handling and logging
entities.parallelStream().forEach(entity -> {
try {
importTask.execute(entity);
} catch (Exception e) {
LOGGER.error("Import {} error. entity = {}", entityName, entity, e);
} finally {
latch.countDown();
}
});
}Interface is effectively used to reduce code duplication
The ImportTaskService interface successfully achieves its goal of removing duplication by:
- Providing a generic way to handle different entity imports
- Enabling parallel processing with consistent error handling
- Centralizing logging and timing logic
- Allowing lambda-based implementations for different entity types
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the implementation and usage of ImportTaskService
# Check for implementations and usage
echo "Checking for ImportTaskService usage..."
rg -l "ImportTaskService" --type java
# Look for the refactored importEntities method
echo "Checking for importEntities method implementation..."
ast-grep --pattern 'private <T> void importEntities($$$)'
# Check for potential duplicate code that could be refactored
echo "Checking for potential remaining duplicate code patterns..."
rg "import.*Entities|import.*Data" --type java -A 5
Length of output: 73921
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/config/EmailConfig.java (1)
51-53: LGTM! Good default for email enabled flag
The default value of false for isEmailEnabled() is a good safety practice, ensuring email functionality is explicitly enabled rather than enabled by default.
apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/config/ConfigTest.java (2)
20-20: LGTM! Import changes align with the refactoring.
The addition of RefreshableConfig import supports the configuration management changes while maintaining backward compatibility.
37-37: Verify test coverage for configuration-specific features.
While the change to RefreshableConfig aligns with the refactoring goals, we should ensure that PortalConfig-specific features are still adequately tested elsewhere.
✅ Verification successful
Change from PortalConfig to RefreshableConfig is safe and well-tested
The change is safe because:
PortalConfigextendsRefreshableConfig, making this a valid type change- The test class
ConfigTestverifies all essential configuration functionality:- Value retrieval with defaults
- Array property handling
- Boolean property handling
- Integer property handling
- The actual
PortalConfig-specific features are tested in other test files wherePortalConfigis properly mocked and injected (as seen in multiple test classes likeAuthUserPasswordCheckerTest,PortalMetaDomainServiceTest, etc.)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for other test files that might cover PortalConfig-specific features
# Look for test files related to PortalConfig and its new subcomponents
echo "Searching for related test files..."
fd -e java -E "**/target/**" . | rg -l "(?:PortalConfig|EnvConfig|ServerConfig|SecurityConfig|EmailConfig).*Test"
# Look for test methods that might have been affected
echo "Searching for related test methods..."
rg -A 5 "@Test.*(?:PortalConfig|EnvConfig|ServerConfig|SecurityConfig|EmailConfig)" --type java
Length of output: 387
Script:
#!/bin/bash
# Let's try a different approach to find test coverage
# First, let's find all test files in the portal module
echo "=== Test files in portal module ==="
fd "Test\.java$" apollo-portal/src/test
# Now let's look for any usage of RefreshableConfig in tests
echo -e "\n=== RefreshableConfig usage in tests ==="
rg "RefreshableConfig" --type java apollo-portal/src/test -C 3
# Let's also check for configuration-related test methods
echo -e "\n=== Configuration-related test methods ==="
rg "@Test.*config" --type java apollo-portal/src/test -i -C 3
# Finally, let's check the actual ConfigTest file content
echo -e "\n=== Content of ConfigTest.java ==="
cat apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/config/ConfigTest.java
Length of output: 8960
Script:
#!/bin/bash
# Let's check the PortalConfig implementation and its test coverage
# First, check the PortalConfig implementation
echo "=== PortalConfig implementation ==="
rg -t java "class PortalConfig" -A 5
# Check if there are other config test files
echo -e "\n=== Other config test files ==="
fd ".*Config.*Test\.java$" apollo-portal/src/test
# Check for test methods in ConfigServiceTest
echo -e "\n=== ConfigServiceTest content ==="
cat apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/service/ConfigServiceTest.java
# Look for PortalConfig usage across tests
echo -e "\n=== PortalConfig usage in tests ==="
rg "PortalConfig" --type java apollo-portal/src/test -C 3
Length of output: 35467
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/config/EnvConfig.java (1)
16-16: Verify PortalConfig integration
Let's verify that all environment-related configurations have been properly moved from PortalConfig and that it now correctly uses this new class.
✅ Verification successful
Integration with PortalConfig is properly implemented
The environment-related configurations have been correctly extracted into EnvConfig:
- EnvConfig is properly injected in PortalConfig's constructor
- All environment-related constants are now in PortalConfigConstants
- Environment-related methods are implemented in EnvConfig with proper reuse of code
- No residual environment configuration methods remain in PortalConfig
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining environment-related configurations in PortalConfig
rg -l "APOLLO_PORTAL_ENVS|EMAIL_SUPPORTED_ENVS|WEBHOOK_SUPPORTED_ENVS" --type java
# Verify that PortalConfig properly injects and uses EnvConfig
ast-grep --pattern 'class PortalConfig {
$$$
private EnvConfig $_ ;
$$$
}'
Length of output: 367
Script:
#!/bin/bash
# Check how EnvConfig is used in PortalConfig
rg -A 5 "EnvConfig" apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/config/PortalConfig.java
# Check the implementation of environment-related methods in EnvConfig
rg -A 5 "APOLLO_PORTAL_ENVS|EMAIL_SUPPORTED_ENVS|WEBHOOK_SUPPORTED_ENVS" apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/config/EnvConfig.java
# Check for any references to PortalConfig's environment methods
rg "getPortalSupportedEnvs|getSupportedEnvs|getEmailSupportedEnvs|getWebhookSupportedEnvs" --type java
Length of output: 1946
apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/auth/ConsumerPermissionValidatorTest.java (2)
17-17: LGTM! Package and class name changes improve code organization.
The move to the auth package and the class rename better reflect the single responsibility of permission validation.
Also applies to: 32-32
34-34: Verify test coverage with the new validator.
The switch from ConsumerRolePermissionService to ConsumerPermissionValidator looks good, but let's ensure all previous test scenarios are still covered.
✅ Verification successful
Test coverage is comprehensive and properly validates the new validator
The test coverage is complete with:
- Comprehensive permission validation scenarios testing both positive and negative cases
- Complete test data setup through SQL scripts covering all required entities (roles, permissions, consumer roles, and role permissions)
- Integration test validating the validator in a controller context
- Proper cleanup after tests
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining references to ConsumerRolePermissionService and verify test coverage
# Check for any lingering references to the old service
echo "Checking for lingering references to ConsumerRolePermissionService..."
rg "ConsumerRolePermissionService" -l
# Look for other test files that might need similar updates
echo "Looking for related test files that might need updates..."
fd -e java -t f "Test.java" | xargs rg "ConsumerRolePermissionService" -l
Length of output: 590
Script:
#!/bin/bash
# Let's analyze the test coverage by examining the test file content and the validator implementation
# First, check the implementation of ConsumerPermissionValidator
echo "=== ConsumerPermissionValidator Implementation ==="
rg -A 10 "class ConsumerPermissionValidator" apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/auth/ConsumerPermissionValidator.java
# Then check the test scenarios
echo -e "\n=== Test Scenarios ==="
rg -A 10 "class ConsumerPermissionValidatorTest" apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/auth/ConsumerPermissionValidatorTest.java
# Check test methods to ensure coverage
echo -e "\n=== Test Methods ==="
rg "^[[:space:]]*@Test" apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/auth/ConsumerPermissionValidatorTest.java -A 1
# Check if there are any test methods in the old service's test file for comparison
echo -e "\n=== Previous Test Methods (if any) ==="
fd -e java -t f "ConsumerRolePermissionServiceTest.java" --exec rg "^[[:space:]]*@Test" {} -A 1 || echo "No previous test file found"
Length of output: 1929
Script:
#!/bin/bash
# Let's examine the test file more thoroughly to see all test methods and SQL setup
# Check the full test file content
echo "=== Full Test File Content ==="
cat apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/auth/ConsumerPermissionValidatorTest.java
# Check the SQL setup file content
echo -e "\n=== SQL Setup Content ==="
cat apollo-portal/src/test/resources/sql/permission/insert-test-roles.sql
# Check for any other related test files
echo -e "\n=== Related Test Files ==="
fd -e java -t f "Test.java" | xargs rg "ConsumerPermissionValidator|ConsumerRolePermissionService" -l
Length of output: 4451
Script:
#!/bin/bash
# Let's check the other test files and SQL setup files to complete our analysis
# Check the other test file that uses the validator
echo "=== NamespaceControllerTest Content ==="
cat apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/NamespaceControllerTest.java
# Check remaining SQL setup files
echo -e "\n=== Permission SQL Files ==="
echo "=== insert-test-permissions.sql ==="
cat apollo-portal/src/test/resources/sql/permission/insert-test-permissions.sql
echo -e "\n=== insert-test-consumerroles.sql ==="
cat apollo-portal/src/test/resources/sql/permission/insert-test-consumerroles.sql
echo -e "\n=== insert-test-rolepermissions.sql ==="
cat apollo-portal/src/test/resources/sql/permission/insert-test-rolepermissions.sql
Length of output: 5688
apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/service/config/ConfigServiceWithCache.java (1)
115-117: LGTM! Good refactoring of the cache outdated check.
The extraction of the complex condition into a well-named method with a descriptive variable improves code readability and maintainability.
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/ReleaseService.java (2)
20-20: LGTM! New imports are properly organized and necessary.
The added imports for ReleaseOperation and ReleaseHistoryBO are required for the new functionality.
Also applies to: 23-23
222-235: LGTM! Well-structured implementation with good separation of concerns.
The method effectively handles both gray releases and normal releases by reusing existing comparison logic. The implementation:
- Correctly identifies gray releases without previous versions
- Properly loads the appropriate releases for comparison
- Maintains consistency by reusing the existing compare methods
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/ReleaseController.java (4)
78-79: LGTM! Good readability improvement.
The introduction of the doesContainEmpty variable makes the validation logic more readable and self-documenting.
130-130: LGTM! Good extraction of common logic.
The use of populateReleaseModel helper method effectively reduces code duplication and improves maintainability.
162-163: LGTM! Consistent validation approach.
The validation of the operator parameter follows the same pattern, improving code consistency.
155-156: Verify the publish method signature change.
The publish method is now called with an additional releasedBy parameter. Let's verify this change is consistent with the service implementation.
✅ Verification successful
Based on the search results, I can now provide the final response:
The publish method signature change is correct and consistent.
The portal's ReleaseService has two publish method overloads:
publish(NamespaceReleaseModel model)- for regular releasespublish(NamespaceGrayDelReleaseModel model, String releaseBy)- for gray deletion releases
The code change in ReleaseController.java correctly uses the second overload by passing both the model and releasedBy parameter for gray deletion releases.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the publish method signature in ReleaseService
ast-grep --pattern 'publish($model, $releasedBy)'
Length of output: 51
Script:
#!/bin/bash
# Search for the publish method in ReleaseService using ripgrep for better context
rg "publish.*\(" --type java -A 5 -B 5
Length of output: 68321
Script:
#!/bin/bash
# Search for the ReleaseService class implementation to verify the publish method signature
rg "class ReleaseService" -A 10 --type java
Length of output: 4245
Script:
#!/bin/bash
# Search for publish method in portal ReleaseService to verify the signature
rg "public.*publish.*\(" --type java apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/ReleaseService.java -A 5
Length of output: 739
apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/config/BizConfig.java (1)
76-76: Constructor refactoring improves code quality
The simplified constructor that delegates property source management to the parent class is a good improvement. It reduces code duplication and improves encapsulation.
Let's verify that this change doesn't impact property source access patterns:
✅ Verification successful
Let me analyze the results and generate one more verification to be completely certain:
The results from the first verification show:
- Only the parent class
RefreshableConfigand test files access the property source - No direct usage of
getRefreshablePropertySources() BizConfigis only instantiated in test code with proper property source
Let's verify the parent class implementation to ensure it properly handles the property source before making the final decision.
Property source handling is properly implemented in parent class
The change to delegate property source management to the parent class is safe and improves code quality. The verification shows:
- The parent class
RefreshableConfigproperly handles the property source:- Stores it securely in a list
- Adds it to the environment's property sources
- Manages periodic refresh through a scheduled executor
- Tests confirm the functionality remains intact:
BizConfigTestshows all configuration methods work correctlyBizDBPropertySourceTestverifies proper property source behavior
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any direct propertySource field access or getRefreshablePropertySources() calls
# that might be affected by this change
# Search for direct propertySource field access
rg -l "propertySource\." --type java
# Search for getRefreshablePropertySources method calls
rg -l "getRefreshablePropertySources\(\)" --type java
# Search for BizConfig usage patterns
ast-grep --pattern 'new BizConfig($_)'
Length of output: 5248
Script:
#!/bin/bash
# Let's examine the parent class and the property source handling implementation
# Check the parent class implementation
cat apollo-common/src/main/java/com/ctrip/framework/apollo/common/config/RefreshableConfig.java
# Check the test implementation for any potential issues
cat apollo-biz/src/test/java/com/ctrip/framework/apollo/biz/service/BizDBPropertySourceTest.java
cat apollo-biz/src/test/java/com/ctrip/framework/apollo/biz/config/BizConfigTest.java
Length of output: 15685
apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/grayReleaseRule/GrayReleaseRulesHolder.java (1)
160-170: Implementation is thread-safe and maintains good performance
The refactored code maintains thread safety by only reading from the concurrent reversedGrayReleaseRuleCache. The changes preserve the existing behavior while improving code clarity, with no impact on performance as it's still just checking cache keys.
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/config/SecurityConfig.java (1)
28-30: Validate Constructor Parameter for Nullability
To prevent potential NullPointerException, ensure that portalDBPropertySource is not null before passing it to the superclass constructor.
Would you like to add a null check or use Objects.requireNonNull(portalDBPropertySource)?
Example:
+ import java.util.Objects;
public SecurityConfig(final PortalDBPropertySource portalDBPropertySource) {
+ super(Objects.requireNonNull(portalDBPropertySource, "portalDBPropertySource must not be null"));
}apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/auth/ConsumerPermissionValidator.java (2)
94-95: Ensure System Permissions Are Properly Configured
In the hasCreateApplicationPermission method, the permission check uses SYSTEM_PERMISSION_TARGET_ID. Verify that this target ID is correctly set and that the necessary permissions are configured in the system to avoid unauthorized access or permission issues.
You can run the following script to check the system permissions:
✅ Verification successful
Based on the search results, I can now provide a final response:
System Permission Configuration is Properly Implemented
The SYSTEM_PERMISSION_TARGET_ID is correctly defined as "SystemRole" and is properly initialized through DefaultRoleInitializationService. The service automatically creates the required CREATE_APPLICATION permission if it doesn't exist, ensuring the permission check in ConsumerPermissionValidator will work as intended.
Key findings:
SYSTEM_PERMISSION_TARGET_IDis a constant defined inSystemRoleManagerService- Permission is automatically initialized by
DefaultRoleInitializationService - The permission is created with target ID "SystemRole" and type CREATE_APPLICATION
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that SYSTEM_PERMISSION_TARGET_ID is properly configured.
# Expected: At least one permission should exist for `SYSTEM_PERMISSION_TARGET_ID` with type `CREATE_APPLICATION`.
# Check for permissions with the specified target ID and permission type
gh api /repos/apolloconfig/apollo/permissions --jq '.[] | select(.permissionType=="CREATE_APPLICATION" and .targetId=="SYSTEM_PERMISSION_TARGET_ID")'
Length of output: 265
Script:
#!/bin/bash
# Let's first check how SYSTEM_PERMISSION_TARGET_ID is defined
rg "SYSTEM_PERMISSION_TARGET_ID" -A 2 -B 2
# Also check the ConsumerPermissionValidator class implementation
ast-grep --pattern 'class ConsumerPermissionValidator {
$$$
}'
Length of output: 4823
Script:
#!/bin/bash
# Let's check the initialization and permission creation logic
ast-grep --pattern 'createPermission($targetId, $permType, $_) {
$$$
}'
# Also check the consumer permission validation logic
ast-grep --pattern 'consumerHasPermission($consumerId, $permType, $targetId) {
$$$
}'
Length of output: 165
61-63: Redundant Permission Checks in hasModifyNamespacePermission
In the method hasModifyNamespacePermission, the permission check for MODIFY_NAMESPACE is performed twice with different target IDs (RoleUtils.buildNamespaceTargetId(appId, namespaceName) and RoleUtils.buildNamespaceTargetId(appId, namespaceName, env)). Ensure that both target IDs are necessary. If they represent different permissions, consider adding comments to clarify their purposes. If not, consider removing redundancy.
If you suspect redundancy, you can verify the permissions associated with the target IDs:
✅ Verification successful
Both Permission Checks are Necessary - Not Redundant
The two permission checks in hasModifyNamespacePermission serve different purposes and are not redundant:
RoleUtils.buildNamespaceTargetId(appId, namespaceName)- Checks app-level namespace modification permissionRoleUtils.buildNamespaceTargetId(appId, namespaceName, env)- Checks environment-specific namespace modification permission
This is evidenced by:
- The implementation in
RoleUtilsshows these generate different target IDs - The
DefaultRoleInitializationServicecreates separate roles and permissions for both app-level and environment-specific namespace modifications - Similar pattern is used consistently across the codebase for other permission checks (e.g., release permissions)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify if the two namespace target IDs correspond to different permissions.
# Expected: Each target ID should have distinct permission entries if both are necessary.
# Fetch permissions for the first target ID
echo "Permissions for target ID with appId and namespaceName:"
gh api /repos/apolloconfig/apollo/permissions --jq '.[] | select(.target_id=="'"$(RoleUtils.buildNamespaceTargetId "$appId" "$namespaceName")"'")'
# Fetch permissions for the second target ID
echo "Permissions for target ID with appId, namespaceName, and env:"
gh api /repos/apolloconfig/apollo/permissions --jq '.[] | select(.target_id=="'"$(RoleUtils.buildNamespaceTargetId "$appId" "$namespaceName" "$env")"'")'
Length of output: 983
Script:
#!/bin/bash
# Let's analyze the implementation of RoleUtils and the permission checks
# First, let's find the RoleUtils implementation
ast-grep --pattern 'class RoleUtils {
$$$
buildNamespaceTargetId($$$) {
$$$
}
$$$
}'
# Let's also check the ConsumerPermissionValidator implementation for context
ast-grep --pattern 'class ConsumerPermissionValidator {
$$$
consumerHasPermission($$$) {
$$$
}
$$$
}'
# Let's search for any tests that verify these permission checks
rg -l "hasModifyNamespacePermission.*test" -g "*.java"
Length of output: 271
Script:
#!/bin/bash
# Let's try a different approach to find the relevant code
# Search for RoleUtils class implementation
rg -A 5 "class RoleUtils" --type java
# Search for buildNamespaceTargetId method
rg -A 5 "buildNamespaceTargetId" --type java
# Search for the full ConsumerPermissionValidator class to understand the context
rg -A 20 "class ConsumerPermissionValidator" --type java
# Look for usages of MODIFY_NAMESPACE permission type
rg "MODIFY_NAMESPACE" --type java
Length of output: 24524
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/config/PortalConfig.java (1)
51-200: Methods delegation is appropriate
All the methods properly delegate to their respective configuration classes (emailConfig, serverConfig, securityConfig, envConfig). This enhances modularity and improves code readability.
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/emailbuilder/ConfigPublishEmailBuilder.java (1)
182-182: Ensure the new method call maintains previous logic
By replacing the custom method getReleaseCompareResult(env, releaseHistory) with a direct call to releaseService.compareReleaseHistory(env, releaseHistory), please verify that all the specific logic previously handled—especially regarding gray releases and previous release IDs—is fully incorporated in the compareReleaseHistory method to prevent any functional regressions in the email diff rendering.
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/ConfigsImportService.java (1)
176-189: Great refactoring to improve code readability and maintainability
The introduction of the importEntities method effectively abstracts the repetitive import logic for different entity types. This reduces code duplication in the doImport method and enhances overall maintainability and readability of the code.
| 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, ""); | ||
| } |
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.
- Empty SMTP configuration values (host, user, password) should have meaningful defaults or proper validation
- Email template paths should have default templates in the classpath or validate for empty values
- Add JavaDoc to document the expected format and impact of these configurations
🔗 Analysis chain
Add documentation and consider non-empty defaults
The configuration methods should:
- Have JavaDoc explaining their purpose and default values
- Consider meaningful defaults instead of empty strings
- Verify the implications of empty configuration values in the system
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 executed
The 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
| private Set<Env> getSupportedEnvs(String key) { | ||
| String[] configurations = getArrayProperty(key, null); | ||
| Set<Env> result = Sets.newHashSet(); | ||
| if (configurations != null) { | ||
| for (String env : configurations) { | ||
| result.add(Env.valueOf(env)); | ||
| } | ||
| } | ||
| return result; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add environment name validation
The getSupportedEnvs method should validate environment names before creating Env objects to prevent runtime exceptions.
private Set<Env> getSupportedEnvs(String key) {
String[] configurations = getArrayProperty(key, null);
Set<Env> result = Sets.newHashSet();
if (configurations != null) {
for (String env : configurations) {
+ try {
result.add(Env.valueOf(env));
+ } catch (IllegalArgumentException e) {
+ logger.warn("Invalid environment name: {}. Skipping...", env);
+ }
}
}
return result;
}Committable suggestion skipped: line range outside the PR's diff.
| public Set<Env> publishTipsSupportedEnvs() { | ||
| String[] configurations = getArrayProperty(PortalConfigConstants.NAMESPACE_PUBLISH_TIPS_SUPPORTED_ENVS_KEY, null); | ||
|
|
||
| Set<Env> result = Sets.newHashSet(); | ||
| if (configurations == null || configurations.length == 0) { | ||
| return result; | ||
| } | ||
|
|
||
| for (String env : configurations) { | ||
| result.add(Env.valueOf(env)); | ||
| } | ||
|
|
||
| return result; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Eliminate code duplication with getSupportedEnvs
The publishTipsSupportedEnvs method contains logic that's very similar to the private getSupportedEnvs method. Consider reusing the existing helper method to reduce code duplication.
public Set<Env> publishTipsSupportedEnvs() {
- String[] configurations = getArrayProperty(PortalConfigConstants.NAMESPACE_PUBLISH_TIPS_SUPPORTED_ENVS_KEY, null);
-
- Set<Env> result = Sets.newHashSet();
- if (configurations == null || configurations.length == 0) {
- return result;
- }
-
- for (String env : configurations) {
- result.add(Env.valueOf(env));
- }
-
- return result;
+ return getSupportedEnvs(PortalConfigConstants.NAMESPACE_PUBLISH_TIPS_SUPPORTED_ENVS_KEY);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public Set<Env> publishTipsSupportedEnvs() { | |
| String[] configurations = getArrayProperty(PortalConfigConstants.NAMESPACE_PUBLISH_TIPS_SUPPORTED_ENVS_KEY, null); | |
| Set<Env> result = Sets.newHashSet(); | |
| if (configurations == null || configurations.length == 0) { | |
| return result; | |
| } | |
| for (String env : configurations) { | |
| result.add(Env.valueOf(env)); | |
| } | |
| return result; | |
| } | |
| public Set<Env> publishTipsSupportedEnvs() { | |
| return getSupportedEnvs(PortalConfigConstants.NAMESPACE_PUBLISH_TIPS_SUPPORTED_ENVS_KEY); | |
| } |
| @@ -28,6 +28,7 @@ | |||
| import org.springframework.core.env.ConfigurableEnvironment; | |||
| import org.springframework.util.CollectionUtils; | |||
|
|
|||
| import java.util.Collections; | |||
There was a problem hiding this comment.
💡 Codebase verification
Significant breaking changes detected in RefreshableConfig
The verification reveals that this is indeed a breaking change with significant impact:
- 5 classes directly extend RefreshableConfig:
- apollo-portal: EmailConfig, EnvConfig, SecurityConfig, ServerConfig
- apollo-biz: BizConfig
The changes are breaking because:
- The abstract method
getRefreshablePropertySources()has been removed - A new constructor requiring
RefreshablePropertySourceis now mandatory - The class is no longer abstract, changing its contract completely
All extending classes will need to be modified to:
- Remove their implementation of
getRefreshablePropertySources() - Add constructor to pass the required
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:
#!/bin/bash
# Search for classes extending RefreshableConfig to assess impact
ast-grep --pattern 'class $_ extends RefreshableConfig {
$$$
}'
Length of output: 67
Script:
#!/bin/bash
# Let's try a different approach using ripgrep to find references to RefreshableConfig
rg "extends\s+RefreshableConfig" -l
Length of output: 501
Script:
#!/bin/bash
# Also search for any usage of RefreshableConfig class
rg "RefreshableConfig" -A 3
Length of output: 8017
Script:
#!/bin/bash
# Check the file history to understand the context of changes
git diff HEAD~1 apollo-common/src/main/java/com/ctrip/framework/apollo/common/config/RefreshableConfig.java
Length of output: 2144
| public boolean consumerHasPermission(long consumerId, String permissionType, String targetId) { | ||
| Permission permission = | ||
| permissionRepository.findTopByPermissionTypeAndTargetId(permissionType, targetId); | ||
| if (permission == null) { | ||
| return false; | ||
| } | ||
|
|
||
| List<ConsumerRole> consumerRoles = consumerRoleRepository.findByConsumerId(consumerId); | ||
| if (CollectionUtils.isEmpty(consumerRoles)) { | ||
| return false; | ||
| } | ||
|
|
||
| Set<Long> roleIds = | ||
| consumerRoles.stream().map(ConsumerRole::getRoleId).collect(Collectors.toSet()); | ||
| List<RolePermission> rolePermissions = rolePermissionRepository.findByRoleIdIn(roleIds); | ||
| if (CollectionUtils.isEmpty(rolePermissions)) { | ||
| return false; | ||
| } | ||
|
|
||
| for (RolePermission rolePermission : rolePermissions) { | ||
| if (rolePermission.getPermissionId() == permission.getId()) { | ||
| return true; | ||
| } | ||
| } | ||
|
|
||
| return false; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize Database Queries in consumerHasPermission Method
The consumerHasPermission method performs multiple sequential database queries, which may impact performance. You can optimize these queries by fetching the necessary data in a single query or reducing the number of database calls.
Consider refactoring the method as follows:
public boolean consumerHasPermission(long consumerId, String permissionType, String targetId) {
Permission permission = permissionRepository.findTopByPermissionTypeAndTargetId(permissionType, targetId);
if (permission == null) {
return false;
}
Set<Long> roleIds = consumerRoleRepository.findByConsumerId(consumerId)
.stream()
.map(ConsumerRole::getRoleId)
.collect(Collectors.toSet());
if (CollectionUtils.isEmpty(roleIds)) {
return false;
}
int count = rolePermissionRepository.countByRoleIdInAndPermissionId(roleIds, permission.getId());
return count > 0;
}This refactoring reduces the number of database queries by:
- Combining the retrieval of
RolePermissioninto a single count query. - Eliminating unnecessary iterations over collections.
| this.emailConfig = new EmailConfig(portalDBPropertySource); | ||
| this.securityConfig = new SecurityConfig(portalDBPropertySource); | ||
| this.envConfig = new EnvConfig(portalDBPropertySource); | ||
| this.serverConfig = new ServerConfig(portalDBPropertySource); |
There was a problem hiding this comment.
Avoid manual instantiation of dependencies
Manually creating instances of EmailConfig, ServerConfig, SecurityConfig, and EnvConfig bypasses Spring's dependency injection and can lead to issues with configuration and proxying.
Ensure that these dependencies are managed by Spring by removing manual instantiation and allowing Spring to inject them. This aligns with the suggested refactoring in the previous comment.
| @Autowired | ||
| private final EmailConfig emailConfig; | ||
| @Autowired | ||
| private final ServerConfig serverConfig; | ||
| @Autowired | ||
| private final SecurityConfig securityConfig; | ||
| @Autowired | ||
| private final EnvConfig envConfig; |
There was a problem hiding this comment.
Inconsistent dependency injection approach detected
The class PortalConfig is using both field injection with @Autowired and manual instantiation in the constructor for the same dependencies (EmailConfig, ServerConfig, SecurityConfig, and EnvConfig). This can lead to unexpected behavior and makes the code less maintainable.
Consider refactoring to use constructor injection exclusively. Remove the @Autowired annotations from the fields and annotate the constructor with @Autowired to allow Spring to inject the dependencies. Here's how you can modify the code:
-import org.springframework.beans.factory.annotation.Autowired;
...
-public class PortalConfig {
-
- @Autowired
- private final EmailConfig emailConfig;
- @Autowired
- private final ServerConfig serverConfig;
- @Autowired
- private final SecurityConfig securityConfig;
- @Autowired
- private final EnvConfig envConfig;
-
- public PortalConfig(final PortalDBPropertySource portalDBPropertySource) {
- this.emailConfig = new EmailConfig(portalDBPropertySource);
- this.securityConfig = new SecurityConfig(portalDBPropertySource);
- this.envConfig = new EnvConfig(portalDBPropertySource);
- this.serverConfig = new ServerConfig(portalDBPropertySource);
- }
+import org.springframework.beans.factory.annotation.Autowired;
...
+public class PortalConfig {
+
+ private final EmailConfig emailConfig;
+ private final ServerConfig serverConfig;
+ private final SecurityConfig securityConfig;
+ private final EnvConfig envConfig;
+
+ @Autowired
+ public PortalConfig(EmailConfig emailConfig,
+ ServerConfig serverConfig,
+ SecurityConfig securityConfig,
+ EnvConfig envConfig) {
+ this.emailConfig = emailConfig;
+ this.serverConfig = serverConfig;
+ this.securityConfig = securityConfig;
+ this.envConfig = envConfig;
+ }Committable suggestion skipped: line range outside the PR's diff.
| public List<Organization> organizations() { | ||
|
|
||
| String organizations = getValue(PortalConfigConstants.DEFAULT_ORGANIZATION_KEY); | ||
| return organizations == null ? Collections.emptyList() : GSON.fromJson(organizations, ORGANIZATION); |
There was a problem hiding this comment.
Add error handling for JSON parsing in 'organizations()' method
The method organizations() does not handle exceptions that may occur during JSON parsing. If the JSON string is malformed, it can lead to a runtime exception and potentially crash the application. Consider adding exception handling to make the application more robust.
Apply this diff to add exception handling:
public List<Organization> organizations() {
String organizations = getValue(PortalConfigConstants.DEFAULT_ORGANIZATION_KEY);
if (organizations == null) {
return Collections.emptyList();
}
+ try {
return GSON.fromJson(organizations, ORGANIZATION);
+ } catch (Exception e) {
+ logger.error("Failed to parse organizations JSON", e);
+ return Collections.emptyList();
+ }
}Committable suggestion skipped: line range outside the PR's diff.
| if (Strings.isNullOrEmpty(superAdminConfig)) { | ||
| return Collections.emptyList(); | ||
| } | ||
| return splitter.splitToList(superAdminConfig); |
There was a problem hiding this comment.
Undefined variable 'splitter' in 'superAdmins()' method
The variable splitter is not declared in the class, which will cause a compilation error when splitter.splitToList(superAdminConfig) is called.
To fix this issue, declare splitter as a static final field:
+import com.google.common.base.Splitter;
...
+private static final Splitter SPLITTER = Splitter.on(",").omitEmptyStrings().trimResults();
public List<String> superAdmins() {
String superAdminConfig = getValue(PortalConfigConstants.SUPER_ADMIN, "");
if (Strings.isNullOrEmpty(superAdminConfig)) {
return Collections.emptyList();
}
- return splitter.splitToList(superAdminConfig);
+ return SPLITTER.splitToList(superAdminConfig);
}Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
CHANGES.md (2)
8-8: Enhance the changelog entry with more specific details.While the current entry correctly references the PR, it could be more informative by detailing the specific refactoring changes made. Consider expanding it to:
-* [Refactor: removed few of the code smells and refactored the code to improve readability](https://github.com/apolloconfig/apollo/pull/5284) +* [Refactor: Enhanced configuration management by separating PortalConfig into specialized config classes (EnvConfig, ServerConfig, SecurityConfig, EmailConfig), introduced ImportTaskService interface, and improved RefreshableConfig's property source handling](https://github.com/apolloconfig/apollo/pull/5284)This provides better context for users and maintainers about the specific improvements made.
8-8: Consider adding migration notes for breaking changes.The refactoring changes might affect users upgrading to this version. Consider adding migration notes under a new "Breaking Changes" or "Migration Guide" section:
+Breaking Changes +------------------ +* Configuration Management + * PortalConfig has been split into specialized config classes (EnvConfig, ServerConfig, SecurityConfig, EmailConfig) + * RefreshableConfig's property source handling has been modified - subclasses no longer need to override certain methods + Changes by VersionThis helps users understand and prepare for potential impacts when upgrading.
|
Thanks for your contribution. Given the size of this pull request, I’m curious if the refactoring was done using a tool or through a manual review of each line. Also, I’d like to understand the criteria for identifying code smells; for instance, I believe the changes to |
|
This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 14 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
What's the purpose of this PR
I identified a few code smells such as Imperative abstraction, duplicate code, feature envy and long statement, and removed them by applying refactoring techniques such as move method, extract class, pull-up method and decompose conditional. I have made sure that existing code and test cases do not break. I haven't added new features so there was no need to add extra tests.
Which issue(s) this PR fixes:
N/A
Brief changelog
ImportTaskServicethat is used inConfigsImportServiceclass to remove code duplicationRefreshableConfigclass now handles the operation of returning property sources. Subtypes of this class no longer have to override the methodFollow this checklist to help us incorporate your contribution quickly and easily:
mvn clean testto make sure this pull request doesn't break anything.CHANGESlog.Summary by CodeRabbit
Release Notes for Apollo Version 2.4.0
New Features
Bug Fixes
Refactor
Documentation