fix the potential data inconsistency issue

[nobodyiam]

What's the purpose of this PR

fix the potential data inconsistency issue when updating item by text

Which issue(s) this PR fixes:

Fixes #3334

Brief changelog

• add namespace id check before submitting the request to admin service

Follow this checklist to help us incorporate your contribution quickly and easily:

• Read the Contributing Guide before making this pull request.
• Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
• Write necessary unit tests to verify the code.
• Run mvn clean test to make sure this pull request doesn't break anything.
• Update the CHANGES log.

[huayaoyue6]

此方式可以处理 #3334 中提到的bug。但是我看adminservice中添加了对namespaceId的判断：

apollo/apollo-adminservice/src/main/java/com/ctrip/framework/apollo/adminservice/controller/ItemController.java

Line 134 in 0ae4fc7

if (namespace == null || namespace.getId() != managedEntity.getNamespaceId()) {

所以为了保持逻辑的一致性，我不仅在portal模块中添加了和此pr一样的判断逻辑，也在adminservice中判断了一下changeSet中是否存在多个namespaceId的情况：

apollo/apollo-adminservice/src/main/java/com/ctrip/framework/apollo/adminservice/controller/ItemSetController.java

Line 43 in 0ae4fc7

itemSetService.updateSet(appId, clusterName, namespaceName, changeSet);

[nobodyiam]

@huayaoyue6

Thanks for the reminder and I think your solution is better. Would you please help to submit a pr to fix this issue? I will close this pr.

[codecov-commenter]

Codecov Report

Merging #4256 (ce34827) into master (eb78ceb) will increase coverage by 0.00%.
The diff coverage is 30.76%.

@@            Coverage Diff            @@
##             master    #4256   +/-   ##
=========================================
  Coverage     52.55%   52.55%           
- Complexity     2630     2634    +4     
=========================================
  Files           486      486           
  Lines         15232    15244   +12     
  Branches       1572     1577    +5     
=========================================
+ Hits           8005     8012    +7     
- Misses         6671     6677    +6     
+ Partials        556      555    -1     

Impacted Files	Coverage Δ
...p/framework/apollo/biz/service/ItemSetService.java	9.83% <18.18%> (+0.03%)	⬆️
...p/framework/apollo/portal/service/ItemService.java	47.12% <100.00%> (+0.61%)	⬆️
...ervice/service/ReleaseMessageServiceWithCache.java	87.05% <0.00%> (+1.17%)	⬆️
...apollo/portal/entity/model/NamespaceTextModel.java	88.46% <0.00%> (+11.53%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eb78ceb...ce34827. Read the comment docs.

[huayaoyue6]

我是双重判断。就几行代码，您要是方便直接在此pr上追加得了，关闭再提交又多折腾一下。

[nobodyiam]

@huayaoyue6

所以为了保持逻辑的一致性，我不仅在portal模块中添加了和此pr一样的判断逻辑，也在adminservice中判断了一下changeSet中是否存在多个namespaceId的情况：

可以的，是否能贴一下这段的改动，我追加一下~

[huayaoyue6]

ItemSetController

package com.ctrip.framework.apollo.adminservice.controller;

import com.ctrip.framework.apollo.adminservice.aop.PreAcquireNamespaceLock;
import com.ctrip.framework.apollo.biz.service.ItemSetService;
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 java.util.HashSet;
import java.util.Set;
import java.util.stream.Collectors;
import org.springframework.http.HttpStatus;
import org.springframework.http.ResponseEntity;
import org.springframework.web.bind.annotation.PathVariable;
import org.springframework.web.bind.annotation.PostMapping;
import org.springframework.web.bind.annotation.RequestBody;
import org.springframework.web.bind.annotation.RestController;

@RestController
public class ItemSetController {

  private final ItemSetService itemSetService;

  public ItemSetController(final ItemSetService itemSetService) {
    this.itemSetService = itemSetService;
  }

  @PreAcquireNamespaceLock
  @PostMapping("/apps/{appId}/clusters/{clusterName}/namespaces/{namespaceName}/itemset")
  public ResponseEntity<Void> create(@PathVariable String appId, @PathVariable String clusterName,
                                     @PathVariable String namespaceName, @RequestBody ItemChangeSets changeSet) {

    Set<Long> itemNamespaceIds = new HashSet<>(1);
    itemNamespaceIds.addAll(changeSet.getCreateItems().stream().map(ItemDTO::getNamespaceId).collect(Collectors.toSet()));
    itemNamespaceIds.addAll(changeSet.getUpdateItems().stream().map(ItemDTO::getNamespaceId).collect(Collectors.toSet()));
    itemNamespaceIds.addAll(changeSet.getDeleteItems().stream().map(ItemDTO::getNamespaceId).collect(Collectors.toSet()));

    if (itemNamespaceIds.size() > 1){
      throw new BadRequestException("Invalid request,multiple namespaceId found!");
    }

    itemSetService.updateSet(appId, clusterName, namespaceName, changeSet,"ItemSetController.create");

    return ResponseEntity.status(HttpStatus.OK).build();
  }


}

ItemSetControllerTest:

  @Test
  @Sql(scripts = "/controller/test-itemset.sql", executionPhase = ExecutionPhase.BEFORE_TEST_METHOD)
  @Sql(scripts = "/controller/cleanup.sql", executionPhase = ExecutionPhase.AFTER_TEST_METHOD)
  public void testItemSetCreatedWithErrorNamespaceId() {
    String appId = "someAppId";
    AppDTO app =
        restTemplate.getForObject("http://localhost:" + port + "/apps/" + appId, AppDTO.class);

    ClusterDTO cluster = restTemplate.getForObject(
        "http://localhost:" + port + "/apps/" + app.getAppId() + "/clusters/default",
        ClusterDTO.class);

    NamespaceDTO namespace =
        restTemplate.getForObject("http://localhost:" + port + "/apps/" + app.getAppId()
            + "/clusters/" + cluster.getName() + "/namespaces/application", NamespaceDTO.class);

    Assert.assertEquals("someAppId", app.getAppId());
    Assert.assertEquals("default", cluster.getName());
    Assert.assertEquals("application", namespace.getNamespaceName());

    ItemChangeSets itemSet = new ItemChangeSets();
    itemSet.setDataChangeLastModifiedBy("created");
    RestTemplate createdTemplate = (new TestRestTemplate()).getRestTemplate();
    createdTemplate.setMessageConverters(restTemplate.getMessageConverters());

    int createdSize = 3;
    for (int i = 0; i < createdSize; i++) {
      ItemDTO item = new ItemDTO();
      item.setNamespaceId(namespace.getId() + i);
      item.setKey("key_" + i);
      item.setValue("created_value_" + i);
      itemSet.addCreateItem(item);
    }

    ResponseEntity<Map> response =
        createdTemplate.postForEntity(
            "http://localhost:" + port + "/apps/" + app.getAppId() + "/clusters/"
                + cluster.getName() + "/namespaces/" + namespace.getNamespaceName() + "/itemset",
            itemSet, Map.class);
    Assert.assertEquals(HttpStatus.BAD_REQUEST, response.getStatusCode());
    Map body = response.getBody();
    Assert.assertEquals(body.get("exception"), BadRequestException.class.getName());
  }

[huayaoyue6]

补充了测试用例

[nobodyiam]

@huayaoyue6 Thanks for the tip, I implemented in a different way but should also work.

[huayaoyue6]

👍

[lepdou]

LGTM