-
-
Notifications
You must be signed in to change notification settings - Fork 10.2k
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
feat: add determine single namespace item num limit logic #5227
base: master
Are you sure you want to change the base?
Changes from 3 commits
547af9d
e947e39
0631770
ea19b4e
998459e
597242a
a648d5a
b9fd00b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -17,6 +17,7 @@ | |||||
package com.ctrip.framework.apollo.adminservice.controller; | ||||||
|
||||||
import com.ctrip.framework.apollo.adminservice.aop.PreAcquireNamespaceLock; | ||||||
import com.ctrip.framework.apollo.biz.config.BizConfig; | ||||||
import com.ctrip.framework.apollo.biz.entity.Commit; | ||||||
import com.ctrip.framework.apollo.biz.entity.Item; | ||||||
import com.ctrip.framework.apollo.biz.entity.Namespace; | ||||||
|
@@ -58,13 +59,14 @@ public class ItemController { | |||||
private final NamespaceService namespaceService; | ||||||
private final CommitService commitService; | ||||||
private final ReleaseService releaseService; | ||||||
private final BizConfig bizConfig; | ||||||
|
||||||
|
||||||
public ItemController(final ItemService itemService, final NamespaceService namespaceService, final CommitService commitService, final ReleaseService releaseService) { | ||||||
public ItemController(final ItemService itemService, final NamespaceService namespaceService, final CommitService commitService, final ReleaseService releaseService, final BizConfig bizConfig) { | ||||||
this.itemService = itemService; | ||||||
this.namespaceService = namespaceService; | ||||||
this.commitService = commitService; | ||||||
this.releaseService = releaseService; | ||||||
this.bizConfig = bizConfig; | ||||||
} | ||||||
|
||||||
@PreAcquireNamespaceLock | ||||||
|
@@ -78,6 +80,14 @@ public ItemDTO create(@PathVariable("appId") String appId, | |||||
if (managedEntity != null) { | ||||||
throw BadRequestException.itemAlreadyExists(entity.getKey()); | ||||||
} | ||||||
|
||||||
if (bizConfig.isItemNumLimitEnabled()) { | ||||||
int itemCount = itemService.findNonEmptyItemCount(entity.getNamespaceId()); | ||||||
if (itemCount >= bizConfig.itemNumLimit()) { | ||||||
throw new BadRequestException("current namespace item count=[" + itemCount + "], single namespace max allow count=[ " + bizConfig.itemNumLimit() + "]"); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Enhance exception message for better user understanding The exception message could be more user-friendly and clearer to end-users. Apply this diff to improve the exception message: -throw new BadRequestException("current namespace item count=[" + itemCount + "], single namespace max allow count=[ " + bizConfig.itemNumLimit() + "]");
+throw new BadRequestException("The maximum number of items (" + bizConfig.itemNumLimit() + ") for this namespace has been reached. Current item count is " + itemCount + "."); 📝 Committable suggestion
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a valid suggestion. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @nobodyiam, I'm glad you found the suggestion helpful! (_/) |
||||||
} | ||||||
} | ||||||
|
||||||
nobodyiam marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Encapsulate item limit logic within To maintain separation of concerns, consider moving the item count limit check into the |
||||||
entity = itemService.save(entity); | ||||||
dto = BeanUtils.transform(ItemDTO.class, entity); | ||||||
commitService.createCommit(appId, clusterName, namespaceName, new ConfigChangeContentBuilder().createItem(entity).build(), | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -135,6 +135,10 @@ public List<Item> findItemsModifiedAfterDate(long namespaceId, Date date) { | |
return itemRepository.findByNamespaceIdAndDataChangeLastModifiedTimeGreaterThan(namespaceId, date); | ||
} | ||
|
||
public int findNonEmptyItemCount(long namespaceId) { | ||
return itemRepository.countByNamespaceIdAndFilterKeyEmpty(namespaceId); | ||
} | ||
Comment on lines
+138
to
+140
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Approve new method, but The new To complete the
Example implementation in the @Transactional
public Item save(Item entity) {
// Existing checks...
long namespaceId = entity.getNamespaceId();
int currentItemCount = findNonEmptyItemCount(namespaceId);
int itemNumLimit = bizConfig.getItemNumLimit(namespaceId);
if (currentItemCount >= itemNumLimit) {
throw new BadRequestException("Item limit reached for this namespace. Limit: " + itemNumLimit);
}
// Existing save logic...
} Similar checks should be added to other relevant methods. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Batch modification will result in a large number of queries to the DB There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @youngzil, I understand your concern about the potential performance impact when batch modifications result in numerous database queries. Thank you for bringing this to my attention. ✏️ Learnings added
|
||
|
||
public Page<Item> findItemsByKey(String key, Pageable pageable) { | ||
return itemRepository.findByKey(key, pageable); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,6 +16,7 @@ | |
*/ | ||
package com.ctrip.framework.apollo.biz.service; | ||
|
||
import com.ctrip.framework.apollo.biz.config.BizConfig; | ||
import com.ctrip.framework.apollo.biz.entity.Audit; | ||
import com.ctrip.framework.apollo.biz.entity.Item; | ||
import com.ctrip.framework.apollo.biz.entity.Namespace; | ||
|
@@ -25,6 +26,7 @@ | |
import com.ctrip.framework.apollo.common.exception.BadRequestException; | ||
import com.ctrip.framework.apollo.common.exception.NotFoundException; | ||
import com.ctrip.framework.apollo.common.utils.BeanUtils; | ||
import com.ctrip.framework.apollo.core.utils.StringUtils; | ||
import java.util.List; | ||
import org.springframework.stereotype.Service; | ||
import org.springframework.transaction.annotation.Transactional; | ||
|
@@ -38,20 +40,23 @@ public class ItemSetService { | |
private final CommitService commitService; | ||
private final ItemService itemService; | ||
private final NamespaceService namespaceService; | ||
private final BizConfig bizConfig; | ||
|
||
public ItemSetService( | ||
final AuditService auditService, | ||
final CommitService commitService, | ||
final ItemService itemService, | ||
final NamespaceService namespaceService) { | ||
final NamespaceService namespaceService, | ||
final BizConfig bizConfig) { | ||
this.auditService = auditService; | ||
this.commitService = commitService; | ||
this.itemService = itemService; | ||
this.namespaceService = namespaceService; | ||
this.bizConfig = bizConfig; | ||
} | ||
|
||
@Transactional | ||
public ItemChangeSets updateSet(Namespace namespace, ItemChangeSets changeSets){ | ||
public ItemChangeSets updateSet(Namespace namespace, ItemChangeSets changeSets) { | ||
return updateSet(namespace.getAppId(), namespace.getClusterName(), namespace.getNamespaceName(), changeSets); | ||
} | ||
|
||
|
@@ -64,6 +69,16 @@ public ItemChangeSets updateSet(String appId, String clusterName, | |
throw NotFoundException.namespaceNotFound(appId, clusterName, namespaceName); | ||
} | ||
|
||
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(); | ||
Comment on lines
+74
to
+75
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Clarify variable names for better readability The variables |
||
itemCount = itemCount + createItemCount - deleteItemCount; | ||
if (itemCount > bizConfig.itemNumLimit()) { | ||
throw new BadRequestException("current namespace item count=[" + itemCount + "], single namespace max allow count=[ " + bizConfig.itemNumLimit() + "]"); | ||
} | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Address potential concurrency issues in item count validation The current logic for enforcing the item number limit may encounter concurrency issues. If multiple requests modify the namespace items simultaneously, the item count retrieved might be outdated by the time the validation occurs, potentially leading to inconsistent states or exceeding the limit unintentionally. To ensure consistency and prevent race conditions, consider implementing synchronization mechanisms such as database-level locks, optimistic locking, or wrapping this operation within a transaction with an appropriate isolation level. |
||
|
||
String operator = changeSet.getDataChangeLastModifiedBy(); | ||
ConfigChangeContentBuilder configChangeContentBuilder = new ConfigChangeContentBuilder(); | ||
|
||
|
@@ -84,7 +99,7 @@ public ItemChangeSets updateSet(String appId, String clusterName, | |
|
||
if (configChangeContentBuilder.hasContent()) { | ||
commitService.createCommit(appId, clusterName, namespaceName, configChangeContentBuilder.build(), | ||
changeSet.getDataChangeLastModifiedBy()); | ||
changeSet.getDataChangeLastModifiedBy()); | ||
} | ||
|
||
return changeSet; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,126 @@ | ||
package com.ctrip.framework.apollo.biz.service; | ||
|
||
import static org.mockito.Mockito.when; | ||
|
||
import com.ctrip.framework.apollo.biz.AbstractIntegrationTest; | ||
import com.ctrip.framework.apollo.biz.config.BizConfig; | ||
import com.ctrip.framework.apollo.biz.entity.Item; | ||
import com.ctrip.framework.apollo.biz.entity.Namespace; | ||
import com.ctrip.framework.apollo.common.dto.ItemChangeSets; | ||
import com.ctrip.framework.apollo.common.dto.ItemDTO; | ||
import com.ctrip.framework.apollo.common.exception.BadRequestException; | ||
import org.junit.Assert; | ||
import org.junit.Test; | ||
import org.mockito.Mock; | ||
import org.springframework.beans.factory.annotation.Autowired; | ||
import org.springframework.test.context.jdbc.Sql; | ||
import org.springframework.test.util.ReflectionTestUtils; | ||
|
||
public class ItemSetServiceTest extends AbstractIntegrationTest { | ||
|
||
@Mock | ||
private BizConfig bizConfig; | ||
|
||
@Autowired | ||
private ItemService itemService; | ||
@Autowired | ||
private NamespaceService namespaceService; | ||
|
||
@Autowired | ||
private ItemSetService itemSetService; | ||
|
||
@Test | ||
@Sql(scripts = "/sql/itemset-test.sql", executionPhase = Sql.ExecutionPhase.BEFORE_TEST_METHOD) | ||
@Sql(scripts = "/sql/clean.sql", executionPhase = Sql.ExecutionPhase.AFTER_TEST_METHOD) | ||
public void testUpdateSetWithoutItemNumLimit() { | ||
|
||
ReflectionTestUtils.setField(itemSetService, "bizConfig", bizConfig); | ||
when(bizConfig.isItemNumLimitEnabled()).thenReturn(false); | ||
when(bizConfig.itemNumLimit()).thenReturn(5); | ||
|
||
Namespace namespace = namespaceService.findOne(1L); | ||
|
||
ItemChangeSets changeSets = new ItemChangeSets(); | ||
changeSets.addCreateItem(buildNormalItem(0L, namespace.getId(), "k6", "v6", "test item num limit", 6)); | ||
changeSets.addCreateItem(buildNormalItem(0L, namespace.getId(), "k7", "v7", "test item num limit", 7)); | ||
|
||
try { | ||
itemSetService.updateSet(namespace, changeSets); | ||
} catch (Exception e) { | ||
Assert.fail(); | ||
} | ||
|
||
int size = itemService.findNonEmptyItemCount(namespace.getId()); | ||
Assert.assertEquals(7, size); | ||
|
||
} | ||
|
||
@Test | ||
@Sql(scripts = "/sql/itemset-test.sql", executionPhase = Sql.ExecutionPhase.BEFORE_TEST_METHOD) | ||
@Sql(scripts = "/sql/clean.sql", executionPhase = Sql.ExecutionPhase.AFTER_TEST_METHOD) | ||
public void testUpdateSetWithItemNumLimit() { | ||
|
||
ReflectionTestUtils.setField(itemSetService, "bizConfig", bizConfig); | ||
when(bizConfig.isItemNumLimitEnabled()).thenReturn(true); | ||
when(bizConfig.itemNumLimit()).thenReturn(5); | ||
|
||
Namespace namespace = namespaceService.findOne(1L); | ||
Item item9901 = itemService.findOne(9901); | ||
Item item9902 = itemService.findOne(9902); | ||
|
||
ItemChangeSets changeSets = new ItemChangeSets(); | ||
changeSets.addUpdateItem(buildNormalItem(item9901.getId(), item9901.getNamespaceId(), item9901.getKey(), item9901.getValue() + " update", item9901.getComment(), item9901.getLineNum())); | ||
changeSets.addDeleteItem(buildNormalItem(item9902.getId(), item9902.getNamespaceId(), item9902.getKey(), item9902.getValue() + " update", item9902.getComment(), item9902.getLineNum())); | ||
changeSets.addCreateItem(buildNormalItem(0L, item9901.getNamespaceId(), "k6", "v6", "test item num limit", 6)); | ||
changeSets.addCreateItem(buildNormalItem(0L, item9901.getNamespaceId(), "k7", "v7", "test item num limit", 7)); | ||
|
||
try { | ||
itemSetService.updateSet(namespace, changeSets); | ||
Assert.fail(); | ||
} catch (Exception e) { | ||
Assert.assertTrue(e instanceof BadRequestException); | ||
} | ||
|
||
int size = itemService.findNonEmptyItemCount(namespace.getId()); | ||
Assert.assertEquals(5, size); | ||
|
||
} | ||
|
||
@Test | ||
@Sql(scripts = "/sql/itemset-test.sql", executionPhase = Sql.ExecutionPhase.BEFORE_TEST_METHOD) | ||
@Sql(scripts = "/sql/clean.sql", executionPhase = Sql.ExecutionPhase.AFTER_TEST_METHOD) | ||
public void testUpdateSetWithItemNumLimit2() { | ||
|
||
ReflectionTestUtils.setField(itemSetService, "bizConfig", bizConfig); | ||
when(bizConfig.isItemNumLimitEnabled()).thenReturn(true); | ||
when(bizConfig.itemNumLimit()).thenReturn(5); | ||
|
||
Namespace namespace = namespaceService.findOne(1L); | ||
Item item9901 = itemService.findOne(9901); | ||
Item item9902 = itemService.findOne(9902); | ||
|
||
ItemChangeSets changeSets = new ItemChangeSets(); | ||
changeSets.addUpdateItem(buildNormalItem(item9901.getId(), item9901.getNamespaceId(), item9901.getKey(), item9901.getValue() + " update", item9901.getComment(), item9901.getLineNum())); | ||
changeSets.addDeleteItem(buildNormalItem(item9902.getId(), item9902.getNamespaceId(), item9902.getKey(), item9902.getValue() + " update", item9902.getComment(), item9902.getLineNum())); | ||
changeSets.addCreateItem(buildNormalItem(0L, item9901.getNamespaceId(), "k6", "v6", "test item num limit", 6)); | ||
|
||
try { | ||
itemSetService.updateSet(namespace, changeSets); | ||
} catch (Exception e) { | ||
Assert.fail(); | ||
} | ||
|
||
int size = itemService.findNonEmptyItemCount(namespace.getId()); | ||
Assert.assertEquals(5, size); | ||
|
||
} | ||
|
||
|
||
private ItemDTO buildNormalItem(Long id, Long namespaceId, String key, String value, String comment, int lineNum) { | ||
ItemDTO item = new ItemDTO(key, value, comment, lineNum); | ||
item.setId(id); | ||
item.setNamespaceId(namespaceId); | ||
return item; | ||
} | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,25 @@ | ||
-- | ||
-- 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. | ||
-- | ||
|
||
INSERT INTO "Namespace" (`Id`, `AppId`, `ClusterName`, `NamespaceName`, `IsDeleted`, `DataChange_CreatedBy`, `DataChange_LastModifiedBy`)VALUES(1,'testApp', 'default', 'application', 0, 'apollo', 'apollo'); | ||
|
||
INSERT INTO "Item" (`Id`, `NamespaceId`, "Key", "Type", "Value", `Comment`, `LineNum`) | ||
VALUES | ||
(9901, 1, 'k1', 0, 'v1', '', 1), | ||
(9902, 1, 'k2', 2, 'v2', '', 2), | ||
(9903, 1, 'k3', 0, 'v3', '', 3), | ||
(9904, 1, 'k4', 0, 'v4', '', 4), | ||
(9905, 1, 'k5', 0, 'v5', '', 5); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -482,6 +482,23 @@ Apollo从1.6.0版本开始增加访问密钥机制,从而只有经过身份验 | |
|
||
![System-parameterization-of-global-search-configuration-items](../images/System-parameterization-of-global-search-configuration-items.png) | ||
|
||
## 6.4 单个命名空间下的配置项数量限制 | ||
从2.4.0版本开始,apollo-portal提供了限制单个命名空间下的配置项数量的功能,此功能默认关闭,需要配置系统 `item.num.limit.enabled` 开启,同时提供了系统参数`item.num.limit`来动态配置单个Namespace下的item数量上限值 | ||
|
||
**设置方法:** | ||
1. 用超级管理员账号登录到Apollo配置中心的界面 | ||
2. 进入`管理员工具 - 系统参数 - ConfigDB 配置管理`页面新增或修改`item.num.limit.enabled`配置项为true/false 即可开启/关闭此功能 | ||
|
||
[//]: # ( ![item-num-limit-enabled](https://cdn.jsdelivr.net/gh/apolloconfig/apollo@master/doc/images/item-num-limit-enabled.png)) | ||
![item-num-limit-enabled](../../../doc/images/item-num-limit-enabled.png) | ||
3. 进入`管理员工具 - 系统参数 - ConfigDB 配置管理`页面新增或修改`item.num.limit`配置项来配置单个Namespace下的item数量上限值 | ||
|
||
[//]: # ( ![item-num-limit](https://cdn.jsdelivr.net/gh/apolloconfig/apollo@master/doc/images/item-num-limit.png)) | ||
![item-num-limit](../../../doc/images/item-num-limit.png) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Inconsistent image references need attention. There are inconsistencies in the image references:
Please standardize the image references. If using relative paths, ensure they are correct and consistent throughout the document. |
||
|
||
|
||
|
||
|
||
# 七、最佳实践 | ||
|
||
## 7.1 安全相关 | ||
|
@@ -512,4 +529,4 @@ Apollo 支持细粒度的权限控制,请务必根据实际情况做好权限 | |
1. `apollo-configservice`和`apollo-adminservice`是基于内网可信网络设计的,所以出于安全考虑,禁止`apollo-configservice`和`apollo-adminservice`直接暴露在公网 | ||
2. 对敏感配置可以考虑开启[访问秘钥](#_62-%e9%85%8d%e7%bd%ae%e8%ae%bf%e9%97%ae%e5%af%86%e9%92%a5),从而只有经过身份验证的客户端才能访问敏感配置 | ||
3. 1.7.1及以上版本可以考虑为`apollo-adminservice`开启[访问控制](zh/deployment/distributed-deployment-guide?id=_326-admin-serviceaccesscontrolenabled-配置apollo-adminservice是否开启访问控制),从而只有[受控的](zh/deployment/distributed-deployment-guide?id=_3112-admin-serviceaccesstokens-设置apollo-portal访问各环境apollo-adminservice所需的access-token)`apollo-portal`才能访问对应接口,增强安全性 | ||
4. 2.1.0及以上版本可以考虑为`eureka`开启[访问控制](zh/deployment/distributed-deployment-guide?id=_329-apolloeurekaserversecurityenabled-配置是否开启eureka-server的登录认证),从而只有受控的`apollo-configservice`和`apollo-adminservice`可以注册到`eureka`,增强安全性 | ||
4. 2.1.0及以上版本可以考虑为`eureka`开启[访问控制](zh/deployment/distributed-deployment-guide?id=_329-apolloeurekaserversecurityenabled-配置是否开启eureka-server的登录认证),从而只有受控的`apollo-configservice`和`apollo-adminservice`可以注册到`eureka`,增强安全性 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.