Skip to content
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.

Sign up for GitHub

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

refactor: removed some code smells from few classes #5284

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

refactor: removed some code smells from few classes #5284

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5907adf
refactor: removed some code smells from few classes
Siddikpatel Nov 23, 2024
21b5eb3
Added description to changes.md
Siddikpatel Nov 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGES.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ Release Notes.
Apollo 2.4.0

------------------
* [Refactor: removed few of the code smells and refactored the code to improve readability](https://github.com/apolloconfig/apollo/pull/5284)
* [Update the server config link in system info page](https://github.com/apolloconfig/apollo/pull/5204)
* [Feature support portal restTemplate Client connection pool config](https://github.com/apolloconfig/apollo/pull/5200)
* [Feature added the ability for administrators to globally search for Value](https://github.com/apolloconfig/apollo/pull/5182)
Expand Down
9 changes: 1 addition & 8 deletions apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/config/BizConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,15 +72,8 @@ public class BizConfig extends RefreshableConfig {
new TypeToken<Map<String, Integer>>() {
}.getType();

private final BizDBPropertySource propertySource;

public BizConfig(final BizDBPropertySource propertySource) {
this.propertySource = propertySource;
}

@Override
protected List<RefreshablePropertySource> getRefreshablePropertySources() {
return Collections.singletonList(propertySource);
super(propertySource);
}

public List<String> eurekaServiceUrls() {
Expand Down
15 changes: 11 additions & 4 deletions .../src/main/java/com/ctrip/framework/apollo/biz/grayReleaseRule/GrayReleaseRulesHolder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,10 +157,17 @@ public Long findReleaseIdFromGrayReleaseRule(String clientAppId, String clientIp
* load gray releases. Because gray release rules actually apply to one more dimension - cluster.
*/
public boolean hasGrayReleaseRule(String clientAppId, String clientIp, String namespaceName) {
return reversedGrayReleaseRuleCache.containsKey(assembleReversedGrayReleaseRuleKey(clientAppId,
namespaceName, clientIp)) || reversedGrayReleaseRuleCache.containsKey
(assembleReversedGrayReleaseRuleKey(clientAppId, namespaceName, GrayReleaseRuleItemDTO
.ALL_IP));

String ruleKeyViaClientIp = assembleReversedGrayReleaseRuleKey(clientAppId,
namespaceName, clientIp);
String ruleKeyViaItemDTO = assembleReversedGrayReleaseRuleKey(clientAppId, namespaceName, GrayReleaseRuleItemDTO
.ALL_IP);

boolean hasScheduleForThisKey = reversedGrayReleaseRuleCache.containsKey(ruleKeyViaClientIp);
boolean hasScheduleForThatKey = reversedGrayReleaseRuleCache.containsKey
(ruleKeyViaItemDTO);

return hasScheduleForThisKey || hasScheduleForThatKey;
}

private void scanGrayReleaseRules() {
Expand Down
1 change: 0 additions & 1 deletion apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/service/ReleaseHistoryService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,6 @@
import org.springframework.stereotype.Service;
import org.springframework.transaction.TransactionStatus;
import org.springframework.transaction.annotation.Transactional;

import java.util.Date;
import java.util.Map;
import java.util.Set;
Expand Down
2 changes: 1 addition & 1 deletion apollo-build-sql-converter/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@
<artifactId>spring-boot-starter-jdbc</artifactId>
<scope>test</scope>
</dependency>
</dependencies>
</dependencies>

<profiles>
<profile>
Expand Down
17 changes: 12 additions & 5 deletions apollo-common/src/main/java/com/ctrip/framework/apollo/common/config/RefreshableConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
import org.springframework.core.env.ConfigurableEnvironment;
import org.springframework.util.CollectionUtils;

import java.util.Collections;
import java.util.List;
import java.util.concurrent.Executors;
import java.util.concurrent.ScheduledExecutorService;
Expand All @@ -36,7 +37,7 @@
import javax.annotation.PostConstruct;


public abstract class RefreshableConfig {
public class RefreshableConfig {

private static final Logger logger = LoggerFactory.getLogger(RefreshableConfig.class);

Expand All @@ -49,18 +50,24 @@ public abstract class RefreshableConfig {
@Autowired
private ConfigurableEnvironment environment;

private List<RefreshablePropertySource> propertySources;

/**
* register refreshable property source.
* Notice: The front property source has higher priority.
*/
protected abstract List<RefreshablePropertySource> getRefreshablePropertySources();
private List<RefreshablePropertySource> propertySources;

public RefreshableConfig(RefreshablePropertySource propertySources) {
this.propertySources = Collections.singletonList(propertySources);
}

private List<RefreshablePropertySource> getPropertySources() {
return propertySources;
}

@PostConstruct
public void setup() {

propertySources = getRefreshablePropertySources();
propertySources = getPropertySources();
if (CollectionUtils.isEmpty(propertySources)) {
throw new IllegalStateException("Property sources can not be empty.");
}
Expand Down
12 changes: 9 additions & 3 deletions .../java/com/ctrip/framework/apollo/configservice/service/config/ConfigServiceWithCache.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,9 +112,9 @@ protected Release findLatestActiveRelease(String appId, String clusterName, Stri

ConfigCacheEntry cacheEntry = configCache.getUnchecked(cacheKey);

//cache is out-dated
if (clientMessages != null && clientMessages.has(messageKey) &&
clientMessages.get(messageKey) > cacheEntry.getNotificationId()) {
// Checking if the cache is out-dated
boolean isCacheOutdated = outdatedCacheCheck(clientMessages, messageKey, cacheEntry);
if (isCacheOutdated) {
//invalidate the cache and try to load from db again
invalidate(cacheKey);
cacheEntry = configCache.getUnchecked(cacheKey);
Expand All @@ -123,6 +123,12 @@ protected Release findLatestActiveRelease(String appId, String clusterName, Stri
return cacheEntry.getRelease();
}

private static boolean outdatedCacheCheck(ApolloNotificationMessages clientMessages, String messageKey, ConfigCacheEntry cacheEntry) {
boolean clientMessagesHasMessageKey = clientMessages != null && clientMessages.has(messageKey);
return clientMessagesHasMessageKey &&
clientMessages.get(messageKey) > cacheEntry.getNotificationId();
}

private void invalidate(String key) {
configCache.invalidate(key);
Tracer.logEvent(TRACER_EVENT_CACHE_INVALIDATE, key);
Expand Down
66 changes: 55 additions & 11 deletions ...al/src/main/java/com/ctrip/framework/apollo/openapi/auth/ConsumerPermissionValidator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,33 +18,49 @@

import static com.ctrip.framework.apollo.portal.service.SystemRoleManagerService.SYSTEM_PERMISSION_TARGET_ID;

import com.ctrip.framework.apollo.openapi.entity.ConsumerRole;
import com.ctrip.framework.apollo.openapi.repository.ConsumerRoleRepository;
import com.ctrip.framework.apollo.openapi.service.ConsumerRolePermissionService;
import com.ctrip.framework.apollo.openapi.util.ConsumerAuthUtil;
import com.ctrip.framework.apollo.portal.constant.PermissionType;
import com.ctrip.framework.apollo.portal.entity.po.Permission;
import com.ctrip.framework.apollo.portal.entity.po.RolePermission;
import com.ctrip.framework.apollo.portal.repository.PermissionRepository;
import com.ctrip.framework.apollo.portal.repository.RolePermissionRepository;
import com.ctrip.framework.apollo.portal.util.RoleUtils;
import com.nimbusds.oauth2.sdk.util.CollectionUtils;
import org.springframework.stereotype.Component;
import javax.servlet.http.HttpServletRequest;
import java.util.List;
import java.util.Set;
import java.util.stream.Collectors;

@Component
public class ConsumerPermissionValidator {

private final ConsumerRolePermissionService permissionService;
private final ConsumerAuthUtil consumerAuthUtil;
private final RolePermissionRepository rolePermissionRepository;
private final ConsumerRoleRepository consumerRoleRepository;
private final PermissionRepository permissionRepository;

public ConsumerPermissionValidator(final ConsumerRolePermissionService permissionService,
final ConsumerAuthUtil consumerAuthUtil) {
this.permissionService = permissionService;
public ConsumerPermissionValidator(final ConsumerAuthUtil consumerAuthUtil,
final RolePermissionRepository rolePermissionRepository
, final ConsumerRoleRepository consumerRoleRepository,
final PermissionRepository permissionRepository) {
this.consumerAuthUtil = consumerAuthUtil;
this.rolePermissionRepository = rolePermissionRepository;
this.consumerRoleRepository = consumerRoleRepository;
this.permissionRepository = permissionRepository;
}

public boolean hasModifyNamespacePermission(HttpServletRequest request, String appId,
String namespaceName, String env) {
if (hasCreateNamespacePermission(request, appId)) {
return true;
}
return permissionService.consumerHasPermission(consumerAuthUtil.retrieveConsumerId(request),
return consumerHasPermission(consumerAuthUtil.retrieveConsumerId(request),
PermissionType.MODIFY_NAMESPACE, RoleUtils.buildNamespaceTargetId(appId, namespaceName))
|| permissionService.consumerHasPermission(consumerAuthUtil.retrieveConsumerId(request),
|| consumerHasPermission(consumerAuthUtil.retrieveConsumerId(request),
PermissionType.MODIFY_NAMESPACE,
RoleUtils.buildNamespaceTargetId(appId, namespaceName, env));

Expand All @@ -55,26 +71,54 @@ public boolean hasReleaseNamespacePermission(HttpServletRequest request, String
if (hasCreateNamespacePermission(request, appId)) {
return true;
}
return permissionService.consumerHasPermission(consumerAuthUtil.retrieveConsumerId(request),
return consumerHasPermission(consumerAuthUtil.retrieveConsumerId(request),
PermissionType.RELEASE_NAMESPACE, RoleUtils.buildNamespaceTargetId(appId, namespaceName))
|| permissionService.consumerHasPermission(consumerAuthUtil.retrieveConsumerId(request),
|| consumerHasPermission(consumerAuthUtil.retrieveConsumerId(request),
PermissionType.RELEASE_NAMESPACE,
RoleUtils.buildNamespaceTargetId(appId, namespaceName, env));

}

public boolean hasCreateNamespacePermission(HttpServletRequest request, String appId) {
return permissionService.consumerHasPermission(consumerAuthUtil.retrieveConsumerId(request),
return consumerHasPermission(consumerAuthUtil.retrieveConsumerId(request),
PermissionType.CREATE_NAMESPACE, appId);
}

public boolean hasCreateClusterPermission(HttpServletRequest request, String appId) {
return permissionService.consumerHasPermission(consumerAuthUtil.retrieveConsumerId(request),
return consumerHasPermission(consumerAuthUtil.retrieveConsumerId(request),
PermissionType.CREATE_CLUSTER, appId);
}

public boolean hasCreateApplicationPermission(HttpServletRequest request) {
long consumerId = consumerAuthUtil.retrieveConsumerId(request);
return permissionService.consumerHasPermission(consumerId, PermissionType.CREATE_APPLICATION, SYSTEM_PERMISSION_TARGET_ID);
return consumerHasPermission(consumerId, PermissionType.CREATE_APPLICATION, SYSTEM_PERMISSION_TARGET_ID);
}

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;
}
}
46 changes: 22 additions & 24 deletions ...tal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/ReleaseController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,9 +75,8 @@ public OpenReleaseDTO createRelease(@PathVariable String appId, @PathVariable St
@PathVariable String namespaceName,
@RequestBody NamespaceReleaseDTO model,
HttpServletRequest request) {
RequestPrecondition.checkArguments(!StringUtils.isContainEmpty(model.getReleasedBy(), model
.getReleaseTitle()),
"Params(releaseTitle and releasedBy) can not be empty");
boolean doesContainEmpty = StringUtils.isContainEmpty(model.getReleasedBy(), model.getReleaseTitle());
RequestPrecondition.checkArguments(!doesContainEmpty, "Params(releaseTitle and releasedBy) can not be empty");

if (userService.findByUserId(model.getReleasedBy()) == null) {
throw BadRequestException.userNotExists(model.getReleasedBy());
Expand All @@ -99,9 +98,8 @@ public OpenReleaseDTO merge(@PathVariable String appId, @PathVariable String env
@PathVariable String clusterName, @PathVariable String namespaceName,
@PathVariable String branchName, @RequestParam(value = "deleteBranch", defaultValue = "true") boolean deleteBranch,
@RequestBody NamespaceReleaseDTO model, HttpServletRequest request) {
RequestPrecondition.checkArguments(!StringUtils.isContainEmpty(model.getReleasedBy(), model
.getReleaseTitle()),
"Params(releaseTitle and releasedBy) can not be empty");
boolean doesContainEmpty = StringUtils.isContainEmpty(model.getReleasedBy(), model.getReleaseTitle());
RequestPrecondition.checkArguments(!doesContainEmpty, "Params(releaseTitle and releasedBy) can not be empty");

if (userService.findByUserId(model.getReleasedBy()) == null) {
throw BadRequestException.userNotExists(model.getReleasedBy());
Expand All @@ -121,20 +119,15 @@ public OpenReleaseDTO createGrayRelease(@PathVariable String appId,
@PathVariable String namespaceName, @PathVariable String branchName,
@RequestBody NamespaceReleaseDTO model,
HttpServletRequest request) {
RequestPrecondition.checkArguments(!StringUtils.isContainEmpty(model.getReleasedBy(), model
.getReleaseTitle()),
"Params(releaseTitle and releasedBy) can not be empty");
boolean doesContainEmpty = StringUtils.isContainEmpty(model.getReleasedBy(), model.getReleaseTitle());
RequestPrecondition.checkArguments(!doesContainEmpty, "Params(releaseTitle and releasedBy) can not be empty");

if (userService.findByUserId(model.getReleasedBy()) == null) {
throw BadRequestException.userNotExists(model.getReleasedBy());
}

NamespaceReleaseModel releaseModel = BeanUtils.transform(NamespaceReleaseModel.class, model);

releaseModel.setAppId(appId);
releaseModel.setEnv(Env.valueOf(env).toString());
releaseModel.setClusterName(branchName);
releaseModel.setNamespaceName(namespaceName);
populateReleaseModel(releaseModel, appId, Env.valueOf(env).toString(), branchName, namespaceName);

return OpenApiBeanUtils.transformFromReleaseDTO(releaseService.publish(releaseModel));
}
Expand All @@ -146,9 +139,9 @@ public OpenReleaseDTO createGrayDelRelease(@PathVariable String appId,
@PathVariable String namespaceName, @PathVariable String branchName,
@RequestBody NamespaceGrayDelReleaseDTO model,
HttpServletRequest request) {
RequestPrecondition.checkArguments(!StringUtils.isContainEmpty(model.getReleasedBy(), model
.getReleaseTitle()),
"Params(releaseTitle and releasedBy) can not be empty");
boolean doesContainEmpty = StringUtils.isContainEmpty(model.getReleasedBy(), model.getReleaseTitle());
RequestPrecondition.checkArguments(!doesContainEmpty, "Params(releaseTitle and releasedBy) can not be empty");

RequestPrecondition.checkArguments(model.getGrayDelKeys() != null,
"Params(grayDelKeys) can not be null");

Expand All @@ -157,19 +150,17 @@ public OpenReleaseDTO createGrayDelRelease(@PathVariable String appId,
}

NamespaceGrayDelReleaseModel releaseModel = BeanUtils.transform(NamespaceGrayDelReleaseModel.class, model);
releaseModel.setAppId(appId);
releaseModel.setEnv(env.toUpperCase());
releaseModel.setClusterName(branchName);
releaseModel.setNamespaceName(namespaceName);
populateReleaseModel(releaseModel, appId, env.toUpperCase(), branchName, namespaceName);

return OpenApiBeanUtils.transformFromReleaseDTO(releaseService.publish(releaseModel, releaseModel.getReleasedBy()));
ReleaseDTO publish = releaseService.publish(releaseModel, releaseModel.getReleasedBy());
return OpenApiBeanUtils.transformFromReleaseDTO(publish);
}

@PutMapping(path = "/releases/{releaseId}/rollback")
public void rollback(@PathVariable String env,
@PathVariable long releaseId, @RequestParam String operator, HttpServletRequest request) {
RequestPrecondition.checkArguments(!StringUtils.isContainEmpty(operator),
"Param operator can not be empty");
boolean doesContainEmpty = StringUtils.isContainEmpty(operator);
RequestPrecondition.checkArguments(!doesContainEmpty, "Param operator can not be empty");

if (userService.findByUserId(operator) == null) {
throw BadRequestException.userNotExists(operator);
Expand All @@ -188,4 +179,11 @@ public void rollback(@PathVariable String env,
this.releaseOpenApiService.rollbackRelease(env, releaseId, operator);
}

private void populateReleaseModel(NamespaceReleaseModel releaseModel, String appId, String env, String branchName, String namespaceName) {
releaseModel.setAppId(appId);
releaseModel.setEnv(env.toUpperCase());
releaseModel.setClusterName(branchName);
releaseModel.setNamespaceName(namespaceName);
}

}
Loading
Loading