Refactor : Improve Code Maintainability, Readability, and Encapsulation #5364
Conversation
…pose Conditional
- Extracted complex conditional logic into separate helper methods:
- isClientLabelValid() for checking if the client label is not null or empty.
- isGrayReleaseRulePresent() for verifying the presence of a gray release rule.
- Improved readability, maintainability, and testability by simplifying conditionals.
…ility and maintainability
- Extracted validateNamespace to handle namespace validation
- Extracted validateItemLimit to enforce item limit constraints
- Extracted processItemChanges to manage item creation, updates, and deletions
- Extracted auditChanges to handle audit logging separately
- Extracted commitChanges to manage commit operations
This refactoring enhances code modularity, simplifies testing, and improves overall maintainability.
…SIZE in cleanReleaseHistory method
- Extracted magic number 100 into a named constant BATCH_SIZE for better readability and maintainability.
- Improved code clarity by making the batch size configurable in one place.
…ope and ApolloAuditScopeManager for Unidirectional Association
- Replaced bidirectional association between ApolloAuditScope and ApolloAuditScopeManager with unidirectional association.
- Simplified the ApolloAuditScope class by removing the dependency on the manager and directly managing the scope within the class.
- Updated the ApolloAuditScopeManager class to manage scope without maintaining a circular relationship.
- Refactored the test cases to align with the new design, removing unnecessary mocks and ensuring proper scope management.
- Enhanced clarity and maintainability of both the classes and their tests by eliminating unnecessary dependencies and simplifying interactions.
…rsionUtil
- Moved version retrieval logic from ApolloServer to a new utility class, VersionUtil
- Ensured encapsulation by making VERSION private and final in VersionUtil
- Improved maintainability by centralizing version retrieval logic
- Prevented direct instantiation of VersionUtil to enforce utility class behavior
This refactor enhances separation of concerns and improves reusability.
…class
- Moved findUsers method to UserSearchService to remove duplication
- Updated OidcLocalUserServiceImpl and SpringSecurityUserService to use the new service
- Improved code maintainability and reusability
WalkthroughThis change set refactors multiple modules. The audit components were streamlined by removing unnecessary dependencies and adjusting scope handling. Business logic in the gray release and item set services has been modularized into helper functions. Constant values are now encapsulated for easier maintenance. In the common module, version retrieval was updated and a new utility was introduced. The portal modules now delegate user search functionality to a dedicated service, eliminating internal duplication. Test cases have been updated to align with these refactorings. Changes
Sequence Diagram(s)sequenceDiagram
participant Manager as AuditScopeManager
participant Scope as ApolloAuditScope
participant PrevScope as PreviousScope
Manager->>Scope: Triggers deactivate/close
Scope-->>PrevScope: If exists, update last span ID
Manager->>Manager: Set currentScope to PrevScope
sequenceDiagram
participant Service as ItemSetService
participant Validator as Validation Modules
participant Processor as ProcessItemChanges
participant Auditor as AuditChanges
participant Committer as CommitChanges
Service->>Validator: validateNamespace() & validateItemLimit()
Service->>Processor: processItemChanges()
Service->>Auditor: auditChanges()
Service->>Committer: commitChanges()
sequenceDiagram
participant UserService as PortalUserService
participant SearchService as UserSearchService
participant Repository as UserRepository
UserService->>SearchService: searchUsers(keyword, includeInactive)
SearchService->>Repository: Query for matching users
Repository-->>SearchService: Return user data
SearchService-->>UserService: Return aggregated, unique user list
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🔇 Additional comments (7)
✨ Finishing Touches
🪧 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 (
|
dosubot
bot
added
the
size:L
|
CLA Assistant Lite bot All contributors have signed 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: 2
🧹 Nitpick comments (5)
apollo-audit/apollo-audit-impl/src/main/java/com/ctrip/framework/apollo/audit/context/ApolloAuditScopeManager.java (1)
33-37: Improved exception handling in deactivate methodThe
deactivatemethod has been simplified to remove unnecessary exception handling and now includes a null check before callingclose()on the scope, which is a good defensive programming practice.However, the
IOExceptionimport on line 19 is no longer used and should be removed.-import java.io.IOException;apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/service/ReleaseHistoryService.java (1)
207-208: Consider unifying the hardcoded limit with the newBATCH_SIZE.
Currently, the repository method is namedfindFirst100By..., while you’ve introduced aBATCH_SIZEconstant. If you decide to adjust the constant value in the future, the method name and logic will mismatch.- List<ReleaseHistory> findFirst100ByAppIdAndClusterName... + // Consider a custom query method or parameterizing the limit to match BATCH_SIZE.apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/util/UserSearchService.java (1)
1-53: Good extraction of user search logic to reduce duplicationThe creation of this dedicated service aligns well with the PR objectives to improve maintainability and reduce duplication by centralizing the previously duplicated user search functionality.
However, there are a few areas for improvement:
The string concatenation in queries like
"%" + keyword + "%"could potentially lead to SQL injection if the repository layer doesn't properly sanitize inputs. Consider using parameterized queries or additional input validation.When
includeInactiveUsersis true and keyword is empty, it returns all users without pagination, which could cause performance issues with large datasets.The hardcoded limit of 20 in
findFirst20ByEnabledshould be extracted as a configurable constant.public class UserSearchService { + private static final int DEFAULT_USER_LIMIT = 20; private final UserRepository userRepository; public UserSearchService(UserRepository userRepository) { this.userRepository = userRepository; } public List<UserPO> findUsers(String keyword, boolean includeInactiveUsers) { Map<Long, UserPO> users = new HashMap<>(); List<UserPO> byUsername; List<UserPO> byUserDisplayName; if (includeInactiveUsers) { if (StringUtils.isEmpty(keyword)) { return (List<UserPO>) userRepository.findAll(); } + // Consider using more secure query methods if available byUsername = userRepository.findByUsernameLike("%" + keyword + "%"); byUserDisplayName = userRepository.findByUserDisplayNameLike("%" + keyword + "%"); } else { if (StringUtils.isEmpty(keyword)) { - return userRepository.findFirst20ByEnabled(1); + return userRepository.findFirstByEnabled(DEFAULT_USER_LIMIT, 1); } byUsername = userRepository.findByUsernameLikeAndEnabled("%" + keyword + "%", 1); byUserDisplayName = userRepository.findByUserDisplayNameLikeAndEnabled("%" + keyword + "%", 1); }apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/spi/springsecurity/SpringSecurityUserService.java (1)
61-61: Consider using dependency injection instead of direct instantiationThe UserSearchService is directly instantiated in the constructor rather than being injected as a dependency. This approach makes unit testing more difficult and tightly couples the classes.
public SpringSecurityUserService( PasswordEncoder passwordEncoder, UserRepository userRepository, - AuthorityRepository authorityRepository) { + AuthorityRepository authorityRepository, + UserSearchService userSearchService) { this.passwordEncoder = passwordEncoder; this.userRepository = userRepository; this.authorityRepository = authorityRepository; - this.userSearchService = new UserSearchService(userRepository); + this.userSearchService = userSearchService; }apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/spi/oidc/OidcLocalUserServiceImpl.java (1)
65-65: Consider using dependency injection instead of direct instantiationSimilar to the issue in SpringSecurityUserService, the UserSearchService is directly instantiated rather than being injected. This approach makes unit testing more difficult and tightly couples the classes.
public OidcLocalUserServiceImpl( JdbcUserDetailsManager userDetailsManager, - UserRepository userRepository) { + UserRepository userRepository, + UserSearchService userSearchService) { this.userDetailsManager = userDetailsManager; this.userRepository = userRepository; - this.userSearchService = new UserSearchService(userRepository); + this.userSearchService = userSearchService; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
apollo-audit/apollo-audit-impl/src/main/java/com/ctrip/framework/apollo/audit/context/ApolloAuditScope.java(2 hunks)apollo-audit/apollo-audit-impl/src/main/java/com/ctrip/framework/apollo/audit/context/ApolloAuditScopeManager.java(1 hunks)apollo-audit/apollo-audit-impl/src/test/java/com/ctrip/framework/apollo/audit/component/ApolloAuditLogApiJpaImplTest.java(1 hunks)apollo-audit/apollo-audit-impl/src/test/java/com/ctrip/framework/apollo/audit/component/ApolloAuditScopeManagerTest.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/ItemSetService.java(1 hunks)apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/service/ReleaseHistoryService.java(4 hunks)apollo-common/src/main/java/com/ctrip/framework/apollo/common/constants/ApolloServer.java(1 hunks)apollo-common/src/main/java/com/ctrip/framework/apollo/common/constants/VersionUtil.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/spi/oidc/OidcLocalUserServiceImpl.java(3 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/spi/springsecurity/SpringSecurityUserService.java(3 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/util/UserSearchService.java(1 hunks)
🧰 Additional context used
🧬 Code Definitions (5)
apollo-common/src/main/java/com/ctrip/framework/apollo/common/constants/ApolloServer.java (1)
apollo-common/src/main/java/com/ctrip/framework/apollo/common/constants/VersionUtil.java (1)
VersionUtil(6-15)
apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/grayReleaseRule/GrayReleaseRulesHolder.java (1)
apollo-common/src/main/java/com/ctrip/framework/apollo/common/dto/GrayReleaseRuleItemDTO.java (1)
GrayReleaseRuleItemDTO(28-85)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/spi/oidc/OidcLocalUserServiceImpl.java (1)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/util/UserSearchService.java (1)
UserSearchService(13-53)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/spi/springsecurity/SpringSecurityUserService.java (1)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/util/UserSearchService.java (1)
UserSearchService(13-53)
apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/service/ItemSetService.java (1)
apollo-common/src/main/java/com/ctrip/framework/apollo/common/exception/NotFoundException.java (1)
NotFoundException(21-73)
🔇 Additional comments (27)
apollo-common/src/main/java/com/ctrip/framework/apollo/common/constants/VersionUtil.java (1)
1-15: Well-structured utility class for version information retrievalThis new utility class appropriately encapsulates version retrieval logic that was previously in ApolloServer. The implementation follows good practices:
- Private constructor to prevent instantiation
- Clear documentation
- Single responsibility focused on version information
- Null-safe implementation with fallback to "unknown"
This extraction aligns well with the PR objective of improving encapsulation.
apollo-common/src/main/java/com/ctrip/framework/apollo/common/constants/ApolloServer.java (2)
19-20: Import statement correctly addedThe import for the new VersionUtil class is properly positioned.
25-25: Version retrieval successfully delegated to utility classThe VERSION constant now properly delegates to the VersionUtil class for retrieving the version information. The modifier ordering has been improved from "public final static" to "public static final" which follows Java convention better.
This change fulfills the PR objective of moving version retrieval logic to a dedicated utility class and improves encapsulation.
apollo-audit/apollo-audit-impl/src/main/java/com/ctrip/framework/apollo/audit/context/ApolloAuditScope.java (2)
24-27: Constructor simplification improves class encapsulationThe constructor has been simplified by removing the dependency on
ApolloAuditScopeManager, which successfully reduces coupling between these classes. This change aligns with the PR objective to resolve cyclic dependencies by converting a bidirectional relationship into a unidirectional one.
34-36: Comment clarification for responsibility shiftThe updated comment clearly indicates that closing span management is now handled externally, which is consistent with the architectural changes to reduce cyclic dependencies. The empty implementation of the
close()method now correctly reflects that the responsibility has been moved elsewhere.apollo-audit/apollo-audit-impl/src/test/java/com/ctrip/framework/apollo/audit/component/ApolloAuditScopeManagerTest.java (1)
43-52: Test refactoring aligns with architectural changesThe test has been appropriately refactored to reflect the architectural changes in the
ApolloAuditScopeandApolloAuditScopeManagerclasses. Creating theApolloAuditScopedirectly and then setting it on the manager instance demonstrates the new unidirectional relationship between these classes.The verification steps are properly updated to ensure the scope has been correctly set and that the active span remains as expected.
apollo-audit/apollo-audit-impl/src/test/java/com/ctrip/framework/apollo/audit/component/ApolloAuditLogApiJpaImplTest.java (1)
101-126: Improved test clarity with step-by-step commentsThe test has been well-refactored to align with the architectural changes while maintaining the same functional behavior. The addition of detailed comments for each step of the process significantly improves the test's readability and maintainability.
The direct instantiation of
ApolloAuditScopewithout theApolloAuditScopeManagerproperly reflects the new unidirectional relationship between these classes. Storing the return value inreturnedScopeand using it for assertions enhances code clarity.apollo-audit/apollo-audit-impl/src/main/java/com/ctrip/framework/apollo/audit/context/ApolloAuditScopeManager.java (2)
25-25: Explicit constructor definitionThe constructor is now explicitly defined, which improves code readability and makes the implementation more intentional.
27-31: Simplified scope activation logicThe
activatemethod has been refactored to directly create a newApolloAuditScopewithout referencing a parent scope, which aligns with the broader goal of converting the bidirectional relationship to a unidirectional one. The comment clearly explains this design decision.apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/service/ReleaseHistoryService.java (4)
68-69: Great improvement introducing a constant for maintainability.
Defining aBATCH_SIZEconstant avoids hardcoded values, making the batch-processing loop more configurable and easier to maintain.
194-194: Helpful clarifying comment.
This additional comment succinctly conveys the intent of skipping cleanup when the retention limit remains at the default value.
210-211: Method reference usage is concise and correct.
Collecting release IDs into a set here is a clean and efficient approach, ensuring no duplicates.
221-221: Logic is coherent and consistent withBATCH_SIZE.
This termination check elegantly signals whether more records remain. Ensure that all references to a batch size align with one constant to prevent confusion if future changes are needed.apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/grayReleaseRule/GrayReleaseRulesHolder.java (4)
173-174: Clearer segmentation of label-based logic.
This refactoring separates the label check from the IP check, improving readability and maintainability.
178-179: No functional changes.
The extra brace and blank line do not affect functionality.
180-182: Encapsulated validity check for the client label.
Isolating theisClientLabelValidmethod heightens clarity, making the caller’s logic more readable.
184-186: Well-structured method to check for existing gray release rules.
This targeted method supports better separation of concerns.apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/service/ItemSetService.java (6)
66-77: Refactored updateSet flow enhances clarity.
By calling dedicated private methods for validation, processing, auditing, and committing, you’ve made theupdateSetmethod more readable and maintainable.
79-85: Proper segregation of namespace validation.
The newvalidateNamespacemethod offers a clear single-responsibility approach to namespace fetching and error handling.
87-104: Cleanly handling item limit logic.
Placing this in a separate method clarifies the constraints and their enforcement. This improves maintainability if future changes to item-limit policy are required.
106-117: Separation of creation, update, and deletion logic.
Extracting the operations intoprocessItemChangespromotes a well-structured flow and makes the code more readable.
119-129: Centralized auditing.
Encapsulating audits inauditChangeshelps ensure consistent audit handling across all item modifications.
131-137: Commit logic neatly encapsulated.
SeparatingcommitChangesfrom the rest ensures the final commit is performed only after all validations and changes, improving maintainability and reducing complexity in the main flow.apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/spi/springsecurity/SpringSecurityUserService.java (2)
31-31: Good import for the new serviceThis import correctly includes the newly created UserSearchService.
52-52: Add appropriate field for UserSearchServiceAdding a dedicated field for the UserSearchService is appropriate for maintaining the service reference.
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/spi/oidc/OidcLocalUserServiceImpl.java (2)
31-31: Good import for the new serviceThis import correctly includes the newly created UserSearchService.
58-58: Add appropriate field for UserSearchServiceAdding a dedicated field for the UserSearchService is appropriate for maintaining the service reference.
| @@ -102,45 +107,14 @@ public void changeEnabled(UserPO user) { | |||
| @Override | |||
| public List<UserInfo> searchUsers(String keyword, int offset, int limit, | |||
| boolean includeInactiveUsers) { | |||
| List<UserPO> users = this.findUsers(keyword, includeInactiveUsers); | |||
| List<UserPO> users = this.userSearchService.findUsers(keyword, includeInactiveUsers); | |||
There was a problem hiding this comment.
Unused pagination parameters in the delegated method call
The searchUsers method accepts offset and limit parameters, but these are not utilized when calling userSearchService.findUsers(). This might affect pagination functionality.
@Override
public List<UserInfo> searchUsers(String keyword, int offset, int limit,
boolean includeInactiveUsers) {
- List<UserPO> users = this.userSearchService.findUsers(keyword, includeInactiveUsers);
+ List<UserPO> users = this.userSearchService.findUsers(keyword, includeInactiveUsers);
+ // Apply pagination if needed
+ if (offset >= 0 && limit > 0 && users.size() > offset) {
+ int toIndex = Math.min(offset + limit, users.size());
+ users = users.subList(offset, toIndex);
+ }
if (CollectionUtils.isEmpty(users)) {
return Collections.emptyList();
}
return users.stream().map(UserPO::toUserInfo)
.collect(Collectors.toList());
}📝 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.
| List<UserPO> users = this.userSearchService.findUsers(keyword, includeInactiveUsers); | |
| @Override | |
| public List<UserInfo> searchUsers(String keyword, int offset, int limit, | |
| boolean includeInactiveUsers) { | |
| List<UserPO> users = this.userSearchService.findUsers(keyword, includeInactiveUsers); | |
| // Apply pagination if needed | |
| if (offset >= 0 && limit > 0 && users.size() > offset) { | |
| int toIndex = Math.min(offset + limit, users.size()); | |
| users = users.subList(offset, toIndex); | |
| } | |
| if (CollectionUtils.isEmpty(users)) { | |
| return Collections.emptyList(); | |
| } | |
| return users.stream().map(UserPO::toUserInfo) | |
| .collect(Collectors.toList()); | |
| } |
| @@ -89,46 +94,14 @@ public void updateUserInfo(UserInfo newUserInfo) { | |||
| @Override | |||
| public List<UserInfo> searchUsers(String keyword, int offset, int limit, | |||
| boolean includeInactiveUsers) { | |||
| List<UserPO> users = this.findUsers(keyword, includeInactiveUsers); | |||
| List<UserPO> users = this.userSearchService.findUsers(keyword, includeInactiveUsers); | |||
There was a problem hiding this comment.
Unused pagination parameters in the delegated method call
The searchUsers method accepts offset and limit parameters, but these are not utilized when calling userSearchService.findUsers(). This might affect pagination functionality.
@Override
public List<UserInfo> searchUsers(String keyword, int offset, int limit,
boolean includeInactiveUsers) {
- List<UserPO> users = this.userSearchService.findUsers(keyword, includeInactiveUsers);
+ List<UserPO> users = this.userSearchService.findUsers(keyword, includeInactiveUsers);
+ // Apply pagination if needed
+ if (offset >= 0 && limit > 0 && users.size() > offset) {
+ int toIndex = Math.min(offset + limit, users.size());
+ users = users.subList(offset, toIndex);
+ }
if (CollectionUtils.isEmpty(users)) {
return Collections.emptyList();
}
return users.stream().map(UserPO::toUserInfo)
.collect(Collectors.toList());
}📝 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.
| List<UserPO> users = this.userSearchService.findUsers(keyword, includeInactiveUsers); | |
| @Override | |
| public List<UserInfo> searchUsers(String keyword, int offset, int limit, | |
| boolean includeInactiveUsers) { | |
| List<UserPO> users = this.userSearchService.findUsers(keyword, includeInactiveUsers); | |
| // Apply pagination if needed | |
| if (offset >= 0 && limit > 0 && users.size() > offset) { | |
| int toIndex = Math.min(offset + limit, users.size()); | |
| users = users.subList(offset, toIndex); | |
| } | |
| if (CollectionUtils.isEmpty(users)) { | |
| return Collections.emptyList(); | |
| } | |
| return users.stream().map(UserPO::toUserInfo) | |
| .collect(Collectors.toList()); | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/util/UserSearchService.java (3)
42-54: Consider adding pagination support consistentlyThe method handles empty keywords differently based on
includeInactiveUsers:
- When
trueand empty keyword: returns all users (potentially thousands)- When
falseand empty keyword: returns only first 20 enabled usersThis inconsistency could lead to performance issues with large user bases. Consider implementing consistent pagination across all cases.
- public List<UserPO> findUsers(String keyword, boolean includeInactiveUsers) { + public List<UserPO> findUsers(String keyword, boolean includeInactiveUsers, int offset, int limit) { // ... existing code if (includeInactiveUsers) { if (StringUtils.isEmpty(keyword)) { - return (List<UserPO>) userRepository.findAll(); + return userRepository.findAll(PageRequest.of(offset, limit)).getContent(); } // ... rest of the method } else { if (StringUtils.isEmpty(keyword)) { - return userRepository.findFirst20ByEnabled(1); + return userRepository.findByEnabled(1, PageRequest.of(offset, limit)).getContent(); } // ... rest of the method }
56-65: Simplify collection population with Stream APIThe duplicate code for populating the users map can be simplified using Java's Stream API.
- if (!CollectionUtils.isEmpty(byUsername)) { - for (UserPO user : byUsername) { - users.put(user.getId(), user); - } - } - if (!CollectionUtils.isEmpty(byUserDisplayName)) { - for (UserPO user : byUserDisplayName) { - users.put(user.getId(), user); - } - } + Stream.concat( + CollectionUtils.isEmpty(byUsername) ? Stream.empty() : byUsername.stream(), + CollectionUtils.isEmpty(byUserDisplayName) ? Stream.empty() : byUserDisplayName.stream() + ).forEach(user -> users.put(user.getId(), user));
37-38: Add Javadoc for public methodThis public method would benefit from Javadoc to explain its purpose, parameters, and return value.
+/** + * Finds users based on a keyword search and activity status. + * Searches by both username and display name, combining results without duplicates. + * + * @param keyword The search term to match against usernames and display names + * @param includeInactiveUsers Whether to include inactive (disabled) users in the results + * @return A list of unique UserPO objects matching the search criteria + */ public List<UserPO> findUsers(String keyword, boolean includeInactiveUsers) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apollo-common/src/main/java/com/ctrip/framework/apollo/common/constants/VersionUtil.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/util/UserSearchService.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apollo-common/src/main/java/com/ctrip/framework/apollo/common/constants/VersionUtil.java
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build (8)
🔇 Additional comments (1)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/util/UserSearchService.java (1)
29-35: LGTM: Good use of dependency injection and immutabilityThe class is well structured with a clean constructor that takes the necessary repository dependency. The use of
finalfor the repository field ensures immutability which is a good practice.
| byUsername = userRepository.findByUsernameLike("%" + keyword + "%"); | ||
| byUserDisplayName = userRepository.findByUserDisplayNameLike("%" + keyword + "%"); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential SQL injection risk in LIKE queries
Direct string concatenation in LIKE queries can be vulnerable to SQL injection. While Spring Data JPA does provide some protection, it's safer to use prepared statements or escape special characters.
- byUsername = userRepository.findByUsernameLike("%" + keyword + "%");
- byUserDisplayName = userRepository.findByUserDisplayNameLike("%" + keyword + "%");
+ String escapedKeyword = keyword.replace("%", "\\%").replace("_", "\\_");
+ byUsername = userRepository.findByUsernameLike("%" + escapedKeyword + "%");
+ byUserDisplayName = userRepository.findByUserDisplayNameLike("%" + escapedKeyword + "%");Alternatively, consider adding repository methods that take the keyword as a parameter and use @Param annotation with prepared statements.
Also applies to: 52-53
|
I have read the CLA Document and I hereby sign the CLA |
Arsh255
changed the title
| public void close(){ | ||
| // closing span become parent-scope's last span | ||
| if (hangUp != null) { | ||
| hangUp.setLastSpanId(activeSpan().spanId()); | ||
| } | ||
| this.manager.setScope(hangUp); | ||
| public void close() { | ||
| // Closing span becomes parent-scope's last span, managed externally |
There was a problem hiding this comment.
This looks like a big change, @spaceluke would you please help to take a look?
There was a problem hiding this comment.
I'm confused, this change removed the logic of setting a scope's last active span.
But I don't see a change about to add this logic in somewhere else?
There was a problem hiding this comment.
For some aspect, this change do help to reduce the circulate dependencies, that's good.
The comments mentioned to manage it externally, I would like to discuss this for a solution.😉 @Arsh255
There was a problem hiding this comment.
Thank you so much. Okay, I got the issue and have changed it. I am pushing it as a new commit. Please review it, and if possible, can you accept my PR request if you are okay with other changes?
There was a problem hiding this comment.
I had reviewed your commit Changes in ApolloAuditScope for managing last active span..
It is a good method to avoid circular dependencies by adding previousScope in ApolloAuditScope, that's cool and I bet it would work.
But in the audit function related code like ApolloAuditSpanAspect, it called appendAuditLog method in ApolloAuditLogApi and it returned a scope for auto-closing.
In order not to destroy the original code function of the life circle of a scope, some changes also needs to be done here.
It might be a big change when changing the ApolloAuditLogApi implementation, I suggest to separate changes in apollo-audit to a new PR.
Thanks for your contribution!
| /** | ||
| * @author Jason Song([email protected]) | ||
| */ | ||
| public class ApolloServer { | ||
| public final static String VERSION = | ||
| "java-" + ApolloServer.class.getPackage().getImplementationVersion(); | ||
| public static final String VERSION = VersionUtil.getVersion(ApolloServer.class); |
There was a problem hiding this comment.
Since it's only used here, extracting it as a separate utility isn't necessary.
There was a problem hiding this comment.
so you want me to revert it back ? or it will work
…oAuditScope and ApolloAuditScopeManager for Unidirectional Association" This reverts commit adc4273.
|
i have reverted all changes for cyclic dependency from this PR now. And I will create a new PR request for ApolloAudit |
| /** | ||
| * @author Jason Song([email protected]) | ||
| */ | ||
| public class ApolloServer { | ||
| public final static String VERSION = | ||
| "java-" + ApolloServer.class.getPackage().getImplementationVersion(); | ||
| public static final String VERSION = VersionUtil.getVersion(ApolloServer.class); |
| reversedGrayReleaseRuleLabelCache.containsKey( | ||
| assembleReversedGrayReleaseRuleKey(clientAppId, namespaceName, | ||
| GrayReleaseRuleItemDTO.ALL_Label)))) { | ||
|
|
| @@ -202,10 +204,11 @@ private void cleanReleaseHistory(ReleaseHistory cleanRelease) { | |||
| boolean hasMore = true; | |||
| while (hasMore && !Thread.currentThread().isInterrupted()) { | |||
| List<ReleaseHistory> cleanReleaseHistoryList = releaseHistoryRepository.findFirst100ByAppIdAndClusterNameAndNamespaceNameAndBranchNameAndIdLessThanEqualOrderByIdAsc( | |||
| appId, clusterName, namespaceName, branchName, maxId.get()); | |||
| appId, clusterName, namespaceName, branchName, maxId.get()); | |||
There was a problem hiding this comment.
This change appears unnecessary.
| appId, clusterName, namespaceName, branchName, maxId.get()); | |
| appId, clusterName, namespaceName, branchName, maxId.get()); |
| .map(ReleaseHistory::getReleaseId) | ||
| .collect(Collectors.toSet()); |
There was a problem hiding this comment.
This change appears unnecessary.
| .map(ReleaseHistory::getReleaseId) | |
| .collect(Collectors.toSet()); | |
| .map(ReleaseHistory::getReleaseId) | |
| .collect(Collectors.toSet()); |
| import java.util.List; | ||
| import java.util.Map; | ||
|
|
||
| public class UserSearchService { |
There was a problem hiding this comment.
The code style of this class doesn't follow the guidelines. Please refer to the CONTRIBUTING Guide for details.
- What's the purpose of this PR?
This PR focuses on improving the overall code structure, readability, and maintainability by refactoring duplicate logic, simplifying complex methods, and enhancing encapsulation.
- Key improvements include:
Removing duplicate user search logic by extracting it into a dedicated service.
Breaking down long, complex methods into smaller, more readable functions.
Eliminating hardcoded magic numbers by introducing named constants.
Fixing cyclic dependencies by converting bidirectional associations to unidirectional ones.
Strengthening encapsulation by moving version retrieval logic to a utility class.
- Detailed Changelog:
1. Extracted Common User Search Logic
The findUsers method was duplicated in multiple places.
Created a new UserSearchService to centralize this logic.
Updated OidcLocalUserServiceImpl and SpringSecurityUserService to use this new service.
2. Refactored Complex Methods in updateSet
Introduced helper methods for better readability and maintainability:
validateNamespace → Handles namespace validation.
validateItemLimit → Ensures item constraints.
processItemChanges → Manages item creation, updates, and deletions.
auditChanges → Separates audit logging logic.
commitChanges → Handles commit operations separately.
3. Replaced Magic Number 100 with a Constant
Previously, the batch size of 100 was hardcoded in cleanReleaseHistory.
Introduced a BATCH_SIZE constant to make it configurable and improve readability.
4. Fixed Cyclic Dependency in ApolloAuditScope & ApolloAuditScopeManager
Replaced the bidirectional relationship with a unidirectional association.
Removed unnecessary dependencies to simplify interactions and improve maintainability.
Updated unit tests accordingly to align with this change.
5. Improved Encapsulation for Version Retrieval
Moved version retrieval logic from ApolloServer to a new utility class VersionUtil.
Ensured VERSION is now private and final to enforce better encapsulation.
Made VersionUtil a proper utility class to prevent direct instantiation.
- Why These Changes?
Less duplicated code → Easier to maintain in the future.
Better readability → Developers can quickly understand each function.
More flexibility → Constants and utilities make future updates simpler.
Fewer dependencies → Improves code organization and testability.
Ready for Review! Let me know if any changes are needed. 🎯
Summary by CodeRabbit
New Features
Refactor
Tests