-
-
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
Changes from 9 commits
547af9d
e947e39
0631770
ea19b4e
998459e
597242a
a648d5a
b9fd00b
adba638
c3fe024
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 |
---|---|---|
|
@@ -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("The maximum number of items (" + bizConfig.itemNumLimit() + ") for this namespace has been reached. Current item count is " + itemCount + "."); | ||
} | ||
} | ||
|
||
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,147 @@ | ||
/* | ||
* 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.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.springframework.beans.factory.annotation.Autowired; | ||
import org.springframework.boot.test.mock.mockito.MockBean; | ||
import org.springframework.test.context.jdbc.Sql; | ||
|
||
public class ItemSetServiceTest extends AbstractIntegrationTest { | ||
|
||
@MockBean | ||
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() { | ||
|
||
when(bizConfig.itemKeyLengthLimit()).thenReturn(128); | ||
when(bizConfig.itemValueLengthLimit()).thenReturn(20000); | ||
|
||
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() { | ||
|
||
when(bizConfig.itemKeyLengthLimit()).thenReturn(128); | ||
when(bizConfig.itemValueLengthLimit()).thenReturn(20000); | ||
|
||
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() { | ||
|
||
when(bizConfig.itemKeyLengthLimit()).thenReturn(128); | ||
when(bizConfig.itemValueLengthLimit()).thenReturn(20000); | ||
|
||
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); |
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.
Consider enforcing item count limit in all item creation methods
The item count limit is currently enforced only in the
create
method. Other methods that create items, such ascreateComment
, do not include this check. This could allow the item count limit to be bypassed through other endpoints, leading to inconsistent enforcement.To ensure consistent application of the item count limit across the application, consider adding the item count validation in other item creation methods like
createComment
.