Enhancement: validates http parameters using javax.validation api (ap…

…olloconfig#1858)

* enhancement: validates http parameters using javax.validation api

* improve code quality according Codacity

* update as requested

* update according to Codacy

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

@@ -7,7 +7,6 @@
 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.common.utils.InputValidator;
 import com.ctrip.framework.apollo.core.utils.StringUtils;
 import org.springframework.data.domain.Pageable;
 import org.springframework.web.bind.annotation.DeleteMapping;
@@ -19,6 +18,7 @@
 import org.springframework.web.bind.annotation.RequestParam;
 import org.springframework.web.bind.annotation.RestController;
 
+import javax.validation.Valid;
 import java.util.List;
 import java.util.Objects;
 
@@ -34,10 +34,7 @@ public AppController(final AppService appService, final AdminService adminServic
   }
 
   @PostMapping("/apps")
-  public AppDTO create(@RequestBody AppDTO dto) {
-    if (!InputValidator.isValidClusterNamespace(dto.getAppId())) {
-      throw new BadRequestException(String.format("AppId格式错误: %s", InputValidator.INVALID_CLUSTER_NAMESPACE_MESSAGE));
-    }
+  public AppDTO create(@Valid @RequestBody AppDTO dto) {
     App entity = BeanUtils.transform(App.class, dto);
     App managedEntity = appService.findOne(entity.getAppId());
     if (managedEntity != null) {
@@ -46,8 +43,7 @@ public AppDTO create(@RequestBody AppDTO dto) {
 
     entity = adminService.createNewApp(entity);
 
-    dto = BeanUtils.transform(AppDTO.class, entity);
-    return dto;
+    return BeanUtils.transform(AppDTO.class, entity);
   }
 
   @DeleteMapping("/apps/{appId:.+}")

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

@@ -6,7 +6,6 @@
 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.common.utils.InputValidator;
 import com.ctrip.framework.apollo.core.ConfigConsts;
 import org.springframework.web.bind.annotation.DeleteMapping;
 import org.springframework.web.bind.annotation.GetMapping;
@@ -16,6 +15,7 @@
 import org.springframework.web.bind.annotation.RequestParam;
 import org.springframework.web.bind.annotation.RestController;
 
+import javax.validation.Valid;
 import java.util.List;
 
 @RestController
@@ -30,11 +30,7 @@ public ClusterController(final ClusterService clusterService) {
   @PostMapping("/apps/{appId}/clusters")
   public ClusterDTO create(@PathVariable("appId") String appId,
                            @RequestParam(value = "autoCreatePrivateNamespace", defaultValue = "true") boolean autoCreatePrivateNamespace,
-                           @RequestBody ClusterDTO dto) {
-    if (!InputValidator.isValidClusterNamespace(dto.getName())) {
-      throw new BadRequestException(String.format("Cluster格式错误: %s", InputValidator.INVALID_CLUSTER_NAMESPACE_MESSAGE));
-    }
-
+                           @Valid @RequestBody ClusterDTO dto) {
     Cluster entity = BeanUtils.transform(Cluster.class, dto);
     Cluster managedEntity = clusterService.findOne(appId, entity.getName());
     if (managedEntity != null) {
@@ -47,8 +43,7 @@ public ClusterDTO create(@PathVariable("appId") String appId,
       entity = clusterService.saveWithoutInstanceOfAppNamespaces(entity);
     }
 
-    dto = BeanUtils.transform(ClusterDTO.class, entity);
-    return dto;
+    return BeanUtils.transform(ClusterDTO.class, entity);
   }
 
   @DeleteMapping("/apps/{appId}/clusters/{clusterName:.+}")

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

@@ -6,7 +6,6 @@
 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.common.utils.InputValidator;
 import org.springframework.web.bind.annotation.DeleteMapping;
 import org.springframework.web.bind.annotation.GetMapping;
 import org.springframework.web.bind.annotation.PathVariable;
@@ -15,6 +14,7 @@
 import org.springframework.web.bind.annotation.RequestParam;
 import org.springframework.web.bind.annotation.RestController;
 
+import javax.validation.Valid;
 import java.util.List;
 import java.util.Map;
 
@@ -29,10 +29,8 @@ public NamespaceController(final NamespaceService namespaceService) {
 
   @PostMapping("/apps/{appId}/clusters/{clusterName}/namespaces")
   public NamespaceDTO create(@PathVariable("appId") String appId,
-                             @PathVariable("clusterName") String clusterName, @RequestBody NamespaceDTO dto) {
-    if (!InputValidator.isValidClusterNamespace(dto.getNamespaceName())) {
-      throw new BadRequestException(String.format("Namespace格式错误: %s", InputValidator.INVALID_CLUSTER_NAMESPACE_MESSAGE));
-    }
+                             @PathVariable("clusterName") String clusterName,
+                             @Valid @RequestBody NamespaceDTO dto) {
     Namespace entity = BeanUtils.transform(Namespace.class, dto);
     Namespace managedEntity = namespaceService.findOne(appId, clusterName, entity.getNamespaceName());
     if (managedEntity != null) {
@@ -41,8 +39,7 @@ public NamespaceDTO create(@PathVariable("appId") String appId,
 
     entity = namespaceService.save(entity);
 
-    dto = BeanUtils.transform(NamespaceDTO.class, entity);
-    return dto;
+    return BeanUtils.transform(NamespaceDTO.class, entity);
   }
 
   @DeleteMapping("/apps/{appId}/clusters/{clusterName}/namespaces/{namespaceName:.+}")

apollo-adminservice/src/test/java/com/ctrip/framework/apollo/adminservice/controller/AbstractControllerTest.java

@@ -21,7 +21,7 @@ public abstract class AbstractControllerTest {
 
   @Autowired
   private HttpMessageConverters httpMessageConverters;
-  
+
   protected RestTemplate restTemplate = (new TestRestTemplate()).getRestTemplate();
 
   @PostConstruct
@@ -32,4 +32,8 @@ private void postConstruct() {
 
   @Value("${local.server.port}")
   int port;
+
+  protected String url(String path) {
+    return "http://localhost:" + port + path;
+  }
 }

apollo-adminservice/src/test/java/com/ctrip/framework/apollo/adminservice/controller/AppControllerTest.java

@@ -4,7 +4,7 @@
 import com.ctrip.framework.apollo.common.dto.AppDTO;
 import com.ctrip.framework.apollo.common.entity.App;
 import com.ctrip.framework.apollo.common.utils.BeanUtils;
-
+import com.ctrip.framework.apollo.common.utils.InputValidator;
 import org.junit.Assert;
 import org.junit.Test;
 import org.springframework.beans.factory.annotation.Autowired;
@@ -13,6 +13,7 @@
 import org.springframework.test.context.jdbc.Sql;
 import org.springframework.test.context.jdbc.Sql.ExecutionPhase;
 import org.springframework.web.client.HttpClientErrorException;
+import static org.hamcrest.Matchers.containsString;
 
 public class AppControllerTest extends AbstractControllerTest {
 
@@ -112,6 +113,20 @@ public void testDelete() {
     Assert.assertNull(deletedApp);
   }
 
+  @Test
+  @Sql(scripts = "/controller/cleanup.sql", executionPhase = ExecutionPhase.AFTER_TEST_METHOD)
+  public void shouldFailedWhenAppIdIsInvalid() {
+    AppDTO dto = generateSampleDTOData();
+    dto.setAppId("invalid app id");
+    try {
+      restTemplate.postForEntity(getBaseAppUrl(), dto, String.class);
+      Assert.fail("Should throw");
+    } catch (HttpClientErrorException e) {
+      Assert.assertEquals(HttpStatus.BAD_REQUEST, e.getStatusCode());
+      Assert.assertThat(new String(e.getResponseBodyAsByteArray()), containsString(InputValidator.INVALID_CLUSTER_NAMESPACE_MESSAGE));
+    }
+  }
+
   private AppDTO generateSampleDTOData() {
     AppDTO dto = new AppDTO();
     dto.setAppId("someAppId");

apollo-adminservice/src/test/java/com/ctrip/framework/apollo/adminservice/controller/ClusterControllerTest.java

@@ -2,17 +2,23 @@
 
 import com.ctrip.framework.apollo.biz.entity.Cluster;
 import com.ctrip.framework.apollo.biz.service.ClusterService;
+import com.ctrip.framework.apollo.common.dto.ClusterDTO;
 import com.ctrip.framework.apollo.common.exception.BadRequestException;
+import com.ctrip.framework.apollo.common.utils.InputValidator;
 import com.ctrip.framework.apollo.core.ConfigConsts;
+import org.junit.Assert;
 import org.junit.Before;
 import org.junit.Test;
 import org.mockito.Mock;
 import org.mockito.MockitoAnnotations;
+import org.springframework.http.ResponseEntity;
+import org.springframework.web.client.HttpClientErrorException;
 
+import static org.hamcrest.Matchers.containsString;
 import static org.mockito.Matchers.any;
 import static org.mockito.Mockito.*;
 
-public class ClusterControllerTest {
+public class ClusterControllerTest extends AbstractControllerTest {
   private ClusterController clusterController;
 
   @Mock
@@ -40,4 +46,29 @@ public void testDeleteSuccess() {
     clusterController.delete("1", "2", "d");
     verify(clusterService, times(1)).findOne("1", "2");
   }
+
+  @Test
+  public void shouldFailWhenRequestBodyInvalid() {
+    ClusterDTO cluster = new ClusterDTO();
+    cluster.setAppId("valid");
+    cluster.setName("notBlank");
+    ResponseEntity<ClusterDTO> response =
+        restTemplate.postForEntity(baseUrl() + "/apps/{appId}/clusters", cluster, ClusterDTO.class, cluster.getAppId());
+    ClusterDTO createdCluster = response.getBody();
+    Assert.assertNotNull(createdCluster);
+    Assert.assertEquals(cluster.getAppId(), createdCluster.getAppId());
+    Assert.assertEquals(cluster.getName(), createdCluster.getName());
+
+    cluster.setName("invalid app name");
+    try {
+      restTemplate.postForEntity(baseUrl() + "/apps/{appId}/clusters", cluster, ClusterDTO.class, cluster.getAppId());
+      Assert.fail("Should throw");
+    } catch (HttpClientErrorException e) {
+      Assert.assertThat(new String(e.getResponseBodyAsByteArray()), containsString(InputValidator.INVALID_CLUSTER_NAMESPACE_MESSAGE));
+    }
+  }
+
+  private String baseUrl() {
+    return "http://localhost:" + port;
+  }
 }

apollo-adminservice/src/test/java/com/ctrip/framework/apollo/adminservice/controller/NamespaceControllerTest.java

@@ -0,0 +1,31 @@
+package com.ctrip.framework.apollo.adminservice.controller;
+
+import com.ctrip.framework.apollo.common.dto.NamespaceDTO;
+import com.ctrip.framework.apollo.common.utils.InputValidator;
+import org.junit.Assert;
+import org.junit.Test;
+import org.springframework.web.client.HttpClientErrorException;
+import static org.hamcrest.Matchers.containsString;
+
+/**
+ * Created by kezhenxu at 2019/1/8 16:27.
+ *
+ * @author kezhenxu ([email protected])
+ */
+public class NamespaceControllerTest extends AbstractControllerTest {
+  @Test
+  public void create() {
+    try {
+      NamespaceDTO namespaceDTO = new NamespaceDTO();
+      namespaceDTO.setClusterName("cluster");
+      namespaceDTO.setNamespaceName("invalid name");
+      namespaceDTO.setAppId("whatever");
+      restTemplate.postForEntity(
+          url("/apps/{appId}/clusters/{clusterName}/namespaces"),
+          namespaceDTO, NamespaceDTO.class, namespaceDTO.getAppId(), namespaceDTO.getClusterName());
+      Assert.fail("Should throw");
+    } catch (HttpClientErrorException e) {
+      Assert.assertThat(new String(e.getResponseBodyAsByteArray()), containsString(InputValidator.INVALID_CLUSTER_NAMESPACE_MESSAGE));
+    }
+  }
+}

apollo-client/src/test/java/com/ctrip/framework/apollo/internals/RemoteConfigRepositoryTest.java

@@ -190,7 +190,7 @@ public Void answer(InvocationOnMock invocation) throws Throwable {
     when(pollResponse.getBody()).thenReturn(Lists.newArrayList(someNotification));
     when(someResponse.getBody()).thenReturn(newApolloConfig);
 
-    longPollFinished.get(500, TimeUnit.MILLISECONDS);
+    longPollFinished.get(5000, TimeUnit.MILLISECONDS);
 
     remoteConfigLongPollService.stopLongPollingRefresh();
 

apollo-common/src/main/java/com/ctrip/framework/apollo/common/controller/GlobalDefaultExceptionHandler.java

@@ -1,34 +1,34 @@
 package com.ctrip.framework.apollo.common.controller;
 
-import com.google.gson.Gson;
-import com.google.gson.reflect.TypeToken;
-
 import com.ctrip.framework.apollo.common.exception.AbstractApolloHttpException;
+import com.ctrip.framework.apollo.common.exception.BadRequestException;
 import com.ctrip.framework.apollo.tracer.Tracer;
-
+import com.google.gson.Gson;
+import com.google.gson.reflect.TypeToken;
+import java.lang.reflect.Type;
+import java.time.LocalDateTime;
+import java.time.format.DateTimeFormatter;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Optional;
+import javax.servlet.ServletException;
+import javax.servlet.http.HttpServletRequest;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 import org.slf4j.event.Level;
 import org.springframework.http.HttpHeaders;
 import org.springframework.http.HttpStatus;
 import org.springframework.http.ResponseEntity;
 import org.springframework.security.access.AccessDeniedException;
+import org.springframework.validation.ObjectError;
 import org.springframework.web.HttpMediaTypeException;
 import org.springframework.web.HttpRequestMethodNotSupportedException;
+import org.springframework.web.bind.MethodArgumentNotValidException;
 import org.springframework.web.bind.annotation.ControllerAdvice;
 import org.springframework.web.bind.annotation.ExceptionHandler;
 import org.springframework.web.client.HttpStatusCodeException;
-
-import java.lang.reflect.Type;
-import java.time.LocalDateTime;
-import java.time.format.DateTimeFormatter;
-import java.util.HashMap;
-import java.util.Map;
-
-import javax.servlet.ServletException;
-import javax.servlet.http.HttpServletRequest;
-
-import static org.slf4j.event.Level.*;
+import static org.slf4j.event.Level.ERROR;
+import static org.slf4j.event.Level.WARN;
 import static org.springframework.http.HttpStatus.BAD_REQUEST;
 import static org.springframework.http.HttpStatus.FORBIDDEN;
 import static org.springframework.http.HttpStatus.INTERNAL_SERVER_ERROR;
@@ -72,6 +72,18 @@ public ResponseEntity<Map<String, Object>> badRequest(HttpServletRequest request
     return handleError(request, ex.getHttpStatus(), ex);
   }
 
+  @ExceptionHandler(MethodArgumentNotValidException.class)
+  public ResponseEntity<Map<String, Object>> handleMethodArgumentNotValidException(
+      HttpServletRequest request, MethodArgumentNotValidException ex
+  ) {
+    final Optional<ObjectError> firstError = ex.getBindingResult().getAllErrors().stream().findFirst();
+    if (firstError.isPresent()) {
+      final String firstErrorMessage = firstError.get().getDefaultMessage();
+      return handleError(request, BAD_REQUEST, new BadRequestException(firstErrorMessage));
+    }
+    return handleError(request, BAD_REQUEST, ex);
+  }
+
   private ResponseEntity<Map<String, Object>> handleError(HttpServletRequest request,
                                                           HttpStatus status, Throwable ex) {
     return handleError(request, status, ex, ERROR);

apollo-common/src/main/java/com/ctrip/framework/apollo/common/dto/AppDTO.java

@@ -1,11 +1,18 @@
 package com.ctrip.framework.apollo.common.dto;
 
+import com.ctrip.framework.apollo.common.utils.InputValidator;
+import javax.validation.constraints.Pattern;
+
 public class AppDTO extends BaseDTO{
 
   private long id;
 
   private String name;
 
+  @Pattern(
+      regexp = InputValidator.CLUSTER_NAMESPACE_VALIDATOR,
+      message = "AppId格式错误: " + InputValidator.INVALID_CLUSTER_NAMESPACE_MESSAGE
+  )
   private String appId;
 
   private String orgId;

apollo-common/src/main/java/com/ctrip/framework/apollo/common/dto/ClusterDTO.java

@@ -1,11 +1,21 @@
 package com.ctrip.framework.apollo.common.dto;
 
+import com.ctrip.framework.apollo.common.utils.InputValidator;
+import javax.validation.constraints.NotBlank;
+import javax.validation.constraints.Pattern;
+
 public class ClusterDTO extends BaseDTO{
 
   private long id;
 
+  @NotBlank(message = "cluster name cannot be blank")
+  @Pattern(
+      regexp = InputValidator.CLUSTER_NAMESPACE_VALIDATOR,
+      message = "Cluster格式错误: " + InputValidator.INVALID_CLUSTER_NAMESPACE_MESSAGE
+  )
   private String name;
 
+  @NotBlank(message = "appId cannot be blank")
   private String appId;
 
   private long parentClusterId;

apollo-common/src/main/java/com/ctrip/framework/apollo/common/dto/NamespaceDTO.java

@@ -1,12 +1,19 @@
 package com.ctrip.framework.apollo.common.dto;
 
+import com.ctrip.framework.apollo.common.utils.InputValidator;
+import javax.validation.constraints.Pattern;
+
 public class NamespaceDTO extends BaseDTO{
   private long id;
 
   private String appId;
-  
+
   private String clusterName;
 
+  @Pattern(
+      regexp = InputValidator.CLUSTER_NAMESPACE_VALIDATOR,
+      message = "Namespace格式错误: " + InputValidator.INVALID_CLUSTER_NAMESPACE_MESSAGE
+  )
   private String namespaceName;
 
   public long getId() {