Skip to content

Refactor : Improve Code Maintainability, Readability, and Encapsulation #5364

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Original file line number Diff line number Diff line change
Expand Up @@ -170,15 +170,20 @@ public boolean hasGrayReleaseRule(String clientAppId, String clientIp, String cl
return true;
}
// check label gray rule
if (!Strings.isNullOrEmpty(clientLabel) &&
(reversedGrayReleaseRuleLabelCache.containsKey(
assembleReversedGrayReleaseRuleKey(clientAppId, namespaceName, clientLabel)) ||
reversedGrayReleaseRuleLabelCache.containsKey(
assembleReversedGrayReleaseRuleKey(clientAppId, namespaceName,
GrayReleaseRuleItemDTO.ALL_Label)))) {

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change

if (isClientLabelValid(clientLabel) && isGrayReleaseRulePresent(clientAppId, namespaceName, clientLabel)) {
return true;
}
return false;
}

private boolean isClientLabelValid(String clientLabel) {
return !Strings.isNullOrEmpty(clientLabel);
}

private boolean isGrayReleaseRulePresent(String clientAppId, String namespaceName, String clientLabel) {
return reversedGrayReleaseRuleLabelCache.containsKey(assembleReversedGrayReleaseRuleKey(clientAppId, namespaceName, clientLabel)) ||
reversedGrayReleaseRuleLabelCache.containsKey(assembleReversedGrayReleaseRuleKey(clientAppId, namespaceName, GrayReleaseRuleItemDTO.ALL_Label));
}

private void scanGrayReleaseRules() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -63,46 +63,77 @@ public ItemChangeSets updateSet(Namespace namespace, ItemChangeSets changeSets)
@Transactional
public ItemChangeSets updateSet(String appId, String clusterName,
String namespaceName, ItemChangeSets changeSet) {
Namespace namespace = namespaceService.findOne(appId, clusterName, namespaceName);
Namespace namespace = validateNamespace(appId, clusterName, namespaceName);
validateItemLimit(namespace, changeSet);

String operator = changeSet.getDataChangeLastModifiedBy();
ConfigChangeContentBuilder configChangeContentBuilder = new ConfigChangeContentBuilder();

processItemChanges(changeSet, namespace, operator, configChangeContentBuilder);
auditChanges(changeSet, operator);
commitChanges(appId, clusterName, namespaceName, configChangeContentBuilder, operator);

return changeSet;
}

private Namespace validateNamespace(String appId, String clusterName, String namespaceName) {
Namespace namespace = namespaceService.findOne(appId, clusterName, namespaceName);
if (namespace == null) {
throw NotFoundException.namespaceNotFound(appId, clusterName, namespaceName);
}
return namespace;
}

if (bizConfig.isItemNumLimitEnabled()) {
int itemCount = itemService.findNonEmptyItemCount(namespace.getId());
int createItemCount = (int) changeSet.getCreateItems().stream().filter(item -> !StringUtils.isEmpty(item.getKey())).count();
int deleteItemCount = (int) changeSet.getDeleteItems().stream().filter(item -> !StringUtils.isEmpty(item.getKey())).count();
itemCount = itemCount + createItemCount - deleteItemCount;
if (itemCount > bizConfig.itemNumLimit()) {
throw new BadRequestException("The maximum number of items (" + bizConfig.itemNumLimit() + ") for this namespace has been reached. Current item count is " + itemCount + ".");
}
private void validateItemLimit(Namespace namespace, ItemChangeSets changeSet) {
if (!bizConfig.isItemNumLimitEnabled()) {
return;
}

String operator = changeSet.getDataChangeLastModifiedBy();
ConfigChangeContentBuilder configChangeContentBuilder = new ConfigChangeContentBuilder();
int itemCount = itemService.findNonEmptyItemCount(namespace.getId());
int createItemCount = (int) changeSet.getCreateItems().stream()
.filter(item -> !StringUtils.isEmpty(item.getKey())).count();
int deleteItemCount = (int) changeSet.getDeleteItems().stream()
.filter(item -> !StringUtils.isEmpty(item.getKey())).count();

itemCount = itemCount + createItemCount - deleteItemCount;

if (itemCount > bizConfig.itemNumLimit()) {
throw new BadRequestException("The maximum number of items (" + bizConfig.itemNumLimit() +
") for this namespace has been reached. Current item count is " + itemCount + ".");
}
}

private void processItemChanges(ItemChangeSets changeSet, Namespace namespace,
String operator, ConfigChangeContentBuilder configChangeContentBuilder) {
if (!CollectionUtils.isEmpty(changeSet.getCreateItems())) {
this.doCreateItems(changeSet.getCreateItems(), namespace, operator, configChangeContentBuilder);
auditService.audit("ItemSet", null, Audit.OP.INSERT, operator);
doCreateItems(changeSet.getCreateItems(), namespace, operator, configChangeContentBuilder);
}
if (!CollectionUtils.isEmpty(changeSet.getUpdateItems())) {
doUpdateItems(changeSet.getUpdateItems(), namespace, operator, configChangeContentBuilder);
}
if (!CollectionUtils.isEmpty(changeSet.getDeleteItems())) {
doDeleteItems(changeSet.getDeleteItems(), namespace, operator, configChangeContentBuilder);
}
}

private void auditChanges(ItemChangeSets changeSet, String operator) {
if (!CollectionUtils.isEmpty(changeSet.getCreateItems())) {
auditService.audit("ItemSet", null, Audit.OP.INSERT, operator);
}
if (!CollectionUtils.isEmpty(changeSet.getUpdateItems())) {
this.doUpdateItems(changeSet.getUpdateItems(), namespace, operator, configChangeContentBuilder);
auditService.audit("ItemSet", null, Audit.OP.UPDATE, operator);
}

if (!CollectionUtils.isEmpty(changeSet.getDeleteItems())) {
this.doDeleteItems(changeSet.getDeleteItems(), namespace, operator, configChangeContentBuilder);
auditService.audit("ItemSet", null, Audit.OP.DELETE, operator);
}
}

private void commitChanges(String appId, String clusterName, String namespaceName,
ConfigChangeContentBuilder configChangeContentBuilder, String operator) {
if (configChangeContentBuilder.hasContent()) {
commitService.createCommit(appId, clusterName, namespaceName, configChangeContentBuilder.build(),
changeSet.getDataChangeLastModifiedBy());
commitService.createCommit(appId, clusterName, namespaceName,
configChangeContentBuilder.build(), operator);
}

return changeSet;
}

private void doDeleteItems(List<ItemDTO> toDeleteItems, Namespace namespace, String operator,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,8 @@ public class ReleaseHistoryService {
ApolloThreadFactory.create("ReleaseHistoryService", true));
private final AtomicBoolean cleanStopped = new AtomicBoolean(false);

private static final int BATCH_SIZE = 100; // Number of records processed per batch

private final ReleaseHistoryRepository releaseHistoryRepository;
private final ReleaseRepository releaseRepository;
private final AuditService auditService;
Expand Down Expand Up @@ -189,7 +191,7 @@ private void cleanReleaseHistory(ReleaseHistory cleanRelease) {
String branchName = cleanRelease.getBranchName();

int retentionLimit = this.getReleaseHistoryRetentionLimit(cleanRelease);
//Second check, if retentionLimit is default value, do not clean
// Second check, if retentionLimit is default value, do not clean
if (retentionLimit == DEFAULT_RELEASE_HISTORY_RETENTION_SIZE) {
return;
}
Expand All @@ -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());
Copy link
Member

Choose a reason for hiding this comment

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

This change appears unnecessary.

Suggested change
appId, clusterName, namespaceName, branchName, maxId.get());
appId, clusterName, namespaceName, branchName, maxId.get());


Set<Long> releaseIds = cleanReleaseHistoryList.stream()
.map(ReleaseHistory::getReleaseId)
.collect(Collectors.toSet());
.map(ReleaseHistory::getReleaseId)
.collect(Collectors.toSet());
Comment on lines +210 to +211
Copy link
Member

Choose a reason for hiding this comment

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

This change appears unnecessary.

Suggested change
.map(ReleaseHistory::getReleaseId)
.collect(Collectors.toSet());
.map(ReleaseHistory::getReleaseId)
.collect(Collectors.toSet());


transactionManager.execute(new TransactionCallbackWithoutResult() {
@Override
Expand All @@ -214,7 +217,8 @@ protected void doInTransactionWithoutResult(TransactionStatus status) {
releaseRepository.deleteAllById(releaseIds);
}
});
hasMore = cleanReleaseHistoryList.size() == 100;

hasMore = cleanReleaseHistoryList.size() == BATCH_SIZE;
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,11 @@
*/
package com.ctrip.framework.apollo.common.constants;

import com.ctrip.framework.apollo.common.constants.VersionUtil;

/**
* @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);
Copy link
Member

Choose a reason for hiding this comment

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

Since it's only used here, extracting it as a separate utility isn't necessary.

Copy link
Author

Choose a reason for hiding this comment

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

so you want me to revert it back ? or it will work

Copy link
Member

Choose a reason for hiding this comment

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

I’d prefer to revert it.

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
/*
* Copyright 2024 Apollo Authors
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*
*/

package com.ctrip.framework.apollo.common.constants;

/**
* Utility class for retrieving version information.
*/
public class VersionUtil {
private VersionUtil() {
// Prevent instantiation
}

public static String getVersion(Class<?> clazz) {
String implementationVersion = clazz.getPackage().getImplementationVersion();
return "java-" + (implementationVersion != null ? implementationVersion : "unknown");
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@
import java.util.List;
import java.util.Map;
import java.util.stream.Collectors;

import com.ctrip.framework.apollo.portal.util.UserSearchService;
import org.springframework.security.core.GrantedAuthority;
import org.springframework.security.core.authority.SimpleGrantedAuthority;
import org.springframework.security.core.userdetails.User;
Expand All @@ -53,11 +55,14 @@ public class OidcLocalUserServiceImpl implements OidcLocalUserService {

private final UserRepository userRepository;

private final UserSearchService userSearchService;

public OidcLocalUserServiceImpl(
JdbcUserDetailsManager userDetailsManager,
UserRepository userRepository) {
this.userDetailsManager = userDetailsManager;
this.userRepository = userRepository;
this.userSearchService = new UserSearchService(userRepository);
}

@Transactional(rollbackFor = Exception.class)
Expand Down Expand Up @@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

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.

Suggested change
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());
}

if (CollectionUtils.isEmpty(users)) {
return Collections.emptyList();
}
return users.stream().map(UserPO::toUserInfo)
.collect(Collectors.toList());
}

private 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();
}
byUsername = userRepository.findByUsernameLike("%" + keyword + "%");
byUserDisplayName = userRepository.findByUserDisplayNameLike("%" + keyword + "%");
} else {
if (StringUtils.isEmpty(keyword)) {
return userRepository.findFirst20ByEnabled(1);
}
byUsername = userRepository
.findByUsernameLikeAndEnabled("%" + keyword + "%", 1);
byUserDisplayName = userRepository
.findByUserDisplayNameLikeAndEnabled("%" + keyword + "%", 1);
}
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);
}
}
return new ArrayList<>(users.values());
}

@Override
public UserInfo findByUserId(String userId) {
UserPO userPO = userRepository.findByUsername(userId);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@

import java.util.HashMap;
import java.util.Map;

import com.ctrip.framework.apollo.portal.util.UserSearchService;
import org.springframework.security.crypto.password.PasswordEncoder;
import org.springframework.transaction.annotation.Transactional;
import org.springframework.util.CollectionUtils;
Expand All @@ -47,13 +49,16 @@ public class SpringSecurityUserService implements UserService {

private final AuthorityRepository authorityRepository;

private final UserSearchService userSearchService;

public SpringSecurityUserService(
PasswordEncoder passwordEncoder,
UserRepository userRepository,
AuthorityRepository authorityRepository) {
this.passwordEncoder = passwordEncoder;
this.userRepository = userRepository;
this.authorityRepository = authorityRepository;
this.userSearchService = new UserSearchService(userRepository);
}

@Transactional
Expand Down Expand Up @@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

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.

Suggested change
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());
}

if (CollectionUtils.isEmpty(users)) {
return Collections.emptyList();
}
return users.stream().map(UserPO::toUserInfo)
.collect(Collectors.toList());
}

private 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();
}
byUsername = userRepository.findByUsernameLike("%" + keyword + "%");
byUserDisplayName = userRepository.findByUserDisplayNameLike("%" + keyword + "%");
} else {
if (StringUtils.isEmpty(keyword)) {
return userRepository.findFirst20ByEnabled(1);
}
byUsername = userRepository.findByUsernameLikeAndEnabled("%" + keyword + "%", 1);
byUserDisplayName = userRepository
.findByUserDisplayNameLikeAndEnabled("%" + keyword + "%", 1);
}
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);
}
}
return new ArrayList<>(users.values());
}

@Override
public UserInfo findByUserId(String userId) {
UserPO userPO = userRepository.findByUsername(userId);
Expand Down
Loading