api server exception management and code optimization (#397) (#2397)

* api server exception management and code optimization (#397)

* add unit test for exception handler.

* fix some style and naming issues

Co-authored-by: dailidong <dailidong66@gmail.com>

dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/controller/AccessTokenController.java

@@ -18,6 +18,7 @@ package org.apache.dolphinscheduler.api.controller;
 
 
 import org.apache.dolphinscheduler.api.enums.Status;
+import org.apache.dolphinscheduler.api.exceptions.ApiException;
 import org.apache.dolphinscheduler.api.service.AccessTokenService;
 import org.apache.dolphinscheduler.api.utils.Result;
 import org.apache.dolphinscheduler.common.Constants;
@@ -37,13 +38,14 @@ import springfox.documentation.annotations.ApiIgnore;
 import java.util.Map;
 
 import static org.apache.dolphinscheduler.api.enums.Status.*;
+
 /**
  * access token controller
  */
 @Api(tags = "ACCESS_TOKEN_TAG", position = 1)
 @RestController
 @RequestMapping("/access-token")
-public class AccessTokenController extends BaseController{
+public class AccessTokenController extends BaseController {
 
 
     private static final Logger logger = LoggerFactory.getLogger(AccessTokenController.class);
@@ -54,140 +56,125 @@ public class AccessTokenController extends BaseController{
 
     /**
      * create token
-     * @param loginUser login user
-     * @param userId token for user id
+     *
+     * @param loginUser  login user
+     * @param userId     token for user id
      * @param expireTime expire time for the token
-     * @param token token
+     * @param token      token
      * @return create result state code
      */
     @ApiIgnore
     @PostMapping(value = "/create")
     @ResponseStatus(HttpStatus.CREATED)
+    @ApiException(CREATE_ACCESS_TOKEN_ERROR)
     public Result createToken(@ApiIgnore @RequestAttribute(value = Constants.SESSION_USER) User loginUser,
-                                                  @RequestParam(value = "userId") int userId,
-                                                  @RequestParam(value = "expireTime") String expireTime,
-                                                  @RequestParam(value = "token") String token){
+                              @RequestParam(value = "userId") int userId,
+                              @RequestParam(value = "expireTime") String expireTime,
+                              @RequestParam(value = "token") String token) {
         logger.info("login user {}, create token , userId : {} , token expire time : {} , token : {}", loginUser.getUserName(),
-                userId,expireTime,token);
-
-        try {
-            Map<String, Object> result = accessTokenService.createToken(userId, expireTime, token);
-            return returnDataList(result);
-        }catch (Exception e){
-            logger.error(CREATE_ACCESS_TOKEN_ERROR.getMsg(),e);
-            return error(CREATE_ACCESS_TOKEN_ERROR.getCode(), CREATE_ACCESS_TOKEN_ERROR.getMsg());
-        }
+                userId, expireTime, token);
+
+        Map<String, Object> result = accessTokenService.createToken(userId, expireTime, token);
+        return returnDataList(result);
     }
 
     /**
      * generate token string
-     * @param loginUser login user
-     * @param userId token for user
+     *
+     * @param loginUser  login user
+     * @param userId     token for user
      * @param expireTime expire time
      * @return token string
      */
     @ApiIgnore
     @PostMapping(value = "/generate")
     @ResponseStatus(HttpStatus.CREATED)
+    @ApiException(GENERATE_TOKEN_ERROR)
     public Result generateToken(@RequestAttribute(value = Constants.SESSION_USER) User loginUser,
-                              @RequestParam(value = "userId") int userId,
-                              @RequestParam(value = "expireTime") String expireTime){
-        logger.info("login user {}, generate token , userId : {} , token expire time : {}",loginUser,userId,expireTime);
-        try {
-            Map<String, Object> result = accessTokenService.generateToken(userId, expireTime);
-            return returnDataList(result);
-        }catch (Exception e){
-            logger.error(GENERATE_TOKEN_ERROR.getMsg(),e);
-            return error(GENERATE_TOKEN_ERROR.getCode(), GENERATE_TOKEN_ERROR.getMsg());
-        }
+                                @RequestParam(value = "userId") int userId,
+                                @RequestParam(value = "expireTime") String expireTime) {
+        logger.info("login user {}, generate token , userId : {} , token expire time : {}", loginUser, userId, expireTime);
+        Map<String, Object> result = accessTokenService.generateToken(userId, expireTime);
+        return returnDataList(result);
     }
 
     /**
      * query access token list paging
      *
      * @param loginUser login user
-     * @param pageNo page number
+     * @param pageNo    page number
      * @param searchVal search value
-     * @param pageSize page size
+     * @param pageSize  page size
      * @return token list of page number and page size
      */
-    @ApiOperation(value = "queryAccessTokenList", notes= "QUERY_ACCESS_TOKEN_LIST_NOTES")
+    @ApiOperation(value = "queryAccessTokenList", notes = "QUERY_ACCESS_TOKEN_LIST_NOTES")
     @ApiImplicitParams({
-            @ApiImplicitParam(name = "searchVal", value = "SEARCH_VAL", dataType ="String"),
+            @ApiImplicitParam(name = "searchVal", value = "SEARCH_VAL", dataType = "String"),
             @ApiImplicitParam(name = "pageNo", value = "PAGE_NO", dataType = "Int", example = "1"),
-            @ApiImplicitParam(name = "pageSize", value = "PAGE_SIZE", dataType ="Int",example = "20")
+            @ApiImplicitParam(name = "pageSize", value = "PAGE_SIZE", dataType = "Int", example = "20")
     })
-    @GetMapping(value="/list-paging")
+    @GetMapping(value = "/list-paging")
     @ResponseStatus(HttpStatus.OK)
+    @ApiException(QUERY_ACCESSTOKEN_LIST_PAGING_ERROR)
     public Result queryAccessTokenList(@ApiIgnore @RequestAttribute(value = Constants.SESSION_USER) User loginUser,
-                                @RequestParam("pageNo") Integer pageNo,
-                                @RequestParam(value = "searchVal", required = false) String searchVal,
-                                @RequestParam("pageSize") Integer pageSize){
+                                       @RequestParam("pageNo") Integer pageNo,
+                                       @RequestParam(value = "searchVal", required = false) String searchVal,
+                                       @RequestParam("pageSize") Integer pageSize) {
         logger.info("login user {}, list access token paging, pageNo: {}, searchVal: {}, pageSize: {}",
-                loginUser.getUserName(),pageNo,searchVal,pageSize);
-        try{
-            Map<String, Object> result = checkPageParams(pageNo, pageSize);
-            if(result.get(Constants.STATUS) != Status.SUCCESS){
-                return returnDataListPaging(result);
-            }
-            searchVal = ParameterUtils.handleEscapes(searchVal);
-            result = accessTokenService.queryAccessTokenList(loginUser, searchVal, pageNo, pageSize);
+                loginUser.getUserName(), pageNo, searchVal, pageSize);
+
+        Map<String, Object> result = checkPageParams(pageNo, pageSize);
+        if (result.get(Constants.STATUS) != Status.SUCCESS) {
             return returnDataListPaging(result);
-        }catch (Exception e){
-            logger.error(QUERY_ACCESSTOKEN_LIST_PAGING_ERROR.getMsg(),e);
-            return error(QUERY_ACCESSTOKEN_LIST_PAGING_ERROR.getCode(),QUERY_ACCESSTOKEN_LIST_PAGING_ERROR.getMsg());
         }
+        searchVal = ParameterUtils.handleEscapes(searchVal);
+        result = accessTokenService.queryAccessTokenList(loginUser, searchVal, pageNo, pageSize);
+        return returnDataListPaging(result);
     }
 
     /**
      * delete access token by id
+     *
      * @param loginUser login user
-     * @param id token id
+     * @param id        token id
      * @return delete result code
      */
     @ApiIgnore
     @PostMapping(value = "/delete")
     @ResponseStatus(HttpStatus.OK)
+    @ApiException(DELETE_ACCESS_TOKEN_ERROR)
     public Result delAccessTokenById(@RequestAttribute(value = Constants.SESSION_USER) User loginUser,
-                              @RequestParam(value = "id") int  id) {
+                                     @RequestParam(value = "id") int id) {
         logger.info("login user {}, delete access token, id: {},", loginUser.getUserName(), id);
-        try {
-            Map<String, Object> result = accessTokenService.delAccessTokenById(loginUser, id);
-            return returnDataList(result);
-        }catch (Exception e){
-            logger.error(DELETE_ACCESS_TOKEN_ERROR.getMsg(),e);
-            return error(Status.DELETE_ACCESS_TOKEN_ERROR.getCode(), Status.DELETE_ACCESS_TOKEN_ERROR.getMsg());
-        }
+        Map<String, Object> result = accessTokenService.delAccessTokenById(loginUser, id);
+        return returnDataList(result);
     }
 
 
     /**
      * update token
-     * @param loginUser login user
-     * @param id token id
-     * @param userId token for user
+     *
+     * @param loginUser  login user
+     * @param id         token id
+     * @param userId     token for user
      * @param expireTime token expire time
-     * @param token token string
+     * @param token      token string
      * @return update result code
      */
     @ApiIgnore
     @PostMapping(value = "/update")
     @ResponseStatus(HttpStatus.OK)
+    @ApiException(UPDATE_ACCESS_TOKEN_ERROR)
     public Result updateToken(@RequestAttribute(value = Constants.SESSION_USER) User loginUser,
                               @RequestParam(value = "id") int id,
                               @RequestParam(value = "userId") int userId,
                               @RequestParam(value = "expireTime") String expireTime,
-                              @RequestParam(value = "token") String token){
+                              @RequestParam(value = "token") String token) {
         logger.info("login user {}, update token , userId : {} , token expire time : {} , token : {}", loginUser.getUserName(),
-                userId,expireTime,token);
-
-        try {
-            Map<String, Object> result = accessTokenService.updateToken(id,userId, expireTime, token);
-            return returnDataList(result);
-        }catch (Exception e){
-            logger.error(UPDATE_ACCESS_TOKEN_ERROR.getMsg(),e);
-            return error(UPDATE_ACCESS_TOKEN_ERROR.getCode(), UPDATE_ACCESS_TOKEN_ERROR.getMsg());
-        }
+                userId, expireTime, token);
+
+        Map<String, Object> result = accessTokenService.updateToken(id, userId, expireTime, token);
+        return returnDataList(result);
     }
 
 }

dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/enums/Status.java

@@ -27,6 +27,8 @@ public enum Status {
 
     SUCCESS(0, "success", "成功"),
 
+    INTERNAL_SERVER_ERROR_ARGS(10000, "Internal Server Error: {0}", "服务端异常: {0}"),
+
     REQUEST_PARAMS_NOT_VALID_ERROR(10001, "request parameter {0} is not valid", "请求参数[{0}]无效"),
     TASK_TIMEOUT_PARAMS_ERROR(10002, "task timeout parameter is not valid", "任务超时参数无效"),
     USER_NAME_EXIST(10003, "user name already exists", "用户名已存在"),

dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/exceptions/ApiException.java

@@ -0,0 +1,34 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You 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 org.apache.dolphinscheduler.api.exceptions;
+
+import org.apache.dolphinscheduler.api.enums.Status;
+
+import java.lang.annotation.Retention;
+import java.lang.annotation.Target;
+
+import static java.lang.annotation.ElementType.METHOD;
+import static java.lang.annotation.RetentionPolicy.RUNTIME;
+
+/**
+ * controller exception annotation
+ */
+@Retention(RUNTIME)
+@Target(METHOD)
+public @interface ApiException {
+    Status value();
+}

dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/exceptions/ApiExceptionHandler.java

@@ -0,0 +1,48 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You 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 org.apache.dolphinscheduler.api.exceptions;
+
+import org.apache.dolphinscheduler.api.enums.Status;
+import org.apache.dolphinscheduler.api.utils.Result;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import org.springframework.web.bind.annotation.ControllerAdvice;
+import org.springframework.web.bind.annotation.ExceptionHandler;
+import org.springframework.web.bind.annotation.ResponseBody;
+import org.springframework.web.method.HandlerMethod;
+
+/**
+ * Exception Handler
+ */
+@ControllerAdvice
+@ResponseBody
+public class ApiExceptionHandler {
+
+    private static final Logger logger = LoggerFactory.getLogger(ApiExceptionHandler.class);
+
+    @ExceptionHandler(Exception.class)
+    public Result exceptionHandler(Exception e, HandlerMethod hm) {
+        logger.error(e.getMessage(), e);
+        ApiException ce = hm.getMethodAnnotation(ApiException.class);
+        if (ce == null) {
+            return Result.errorWithArgs(Status.INTERNAL_SERVER_ERROR_ARGS, e.getMessage());
+        }
+        Status st = ce.value();
+        return Result.error(st);
+    }
+
+}

dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/service/AccessTokenService.java

@@ -83,6 +83,9 @@ public class AccessTokenService extends BaseService {
     public Map<String, Object> createToken(int userId, String expireTime, String token) {
         Map<String, Object> result = new HashMap<>(5);
 
+        if (userId <= 0) {
+            throw new IllegalArgumentException("User id should not less than or equals to 0.");
+        }
         AccessToken accessToken = new AccessToken();
         accessToken.setUserId(userId);
         accessToken.setExpireTime(DateUtils.stringToDate(expireTime));

dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/utils/Result.java

@@ -16,6 +16,10 @@
  */
 package org.apache.dolphinscheduler.api.utils;
 
+import org.apache.dolphinscheduler.api.enums.Status;
+
+import java.text.MessageFormat;
+
 /**
  * result
  *
@@ -37,13 +41,58 @@ public class Result<T> {
      */
     private T data;
 
-    public Result(){}
+    public Result() {
+    }
 
-    public Result(Integer code , String msg){
+    public Result(Integer code, String msg) {
         this.code = code;
         this.msg = msg;
     }
 
+    private Result(T data) {
+        this.code  = 0;
+        this.data = data;
+    }
+
+    private Result(Status status) {
+        if (status != null) {
+            this.code = status.getCode();
+            this.msg = status.getMsg();
+        }
+    }
+
+    /**
+     * Call this function if there is success
+     *
+     * @param data data
+     * @param <T> type
+     * @return resule
+     */
+    public static <T> Result<T> success(T data) {
+        return new Result<>(data);
+    }
+
+    /**
+     * Call this function if there is any error
+     *
+     * @param status status
+     * @return result
+     */
+    public static Result error(Status status) {
+        return new Result(status);
+    }
+
+    /**
+     * Call this function if there is any error
+     *
+     * @param status status
+     * @param args args
+     * @return result
+     */
+    public static Result errorWithArgs(Status status, Object... args) {
+        return new Result(status.getCode(), MessageFormat.format(status.getMsg(), args));
+    }
+
     public Integer getCode() {
         return code;
     }

dolphinscheduler-api/src/test/java/org/apache/dolphinscheduler/api/controller/AccessTokenControllerTest.java

@@ -56,6 +56,23 @@ public class AccessTokenControllerTest extends AbstractControllerTest{
         logger.info(mvcResult.getResponse().getContentAsString());
     }
 
+    @Test
+    public void testExceptionHandler() throws Exception {
+        MultiValueMap<String, String> paramsMap = new LinkedMultiValueMap<>();
+        paramsMap.add("userId","-1");
+        paramsMap.add("expireTime","2019-12-18 00:00:00");
+        paramsMap.add("token","507f5aeaaa2093dbdff5d5522ce00510");
+        MvcResult mvcResult = mockMvc.perform(post("/access-token/create")
+                .header("sessionId", sessionId)
+                .params(paramsMap))
+                .andExpect(status().isOk())
+                .andExpect(content().contentType(MediaType.APPLICATION_JSON_UTF8))
+                .andReturn();
+        Result result = JSONUtils.parseObject(mvcResult.getResponse().getContentAsString(), Result.class);
+        Assert.assertEquals(Status.CREATE_ACCESS_TOKEN_ERROR.getCode(), result.getCode().intValue());
+        logger.info(mvcResult.getResponse().getContentAsString());
+    }
+
     @Test
     public void testGenerateToken() throws Exception {
         MultiValueMap<String, String> paramsMap = new LinkedMultiValueMap<>();

dolphinscheduler-api/src/test/java/org/apache/dolphinscheduler/api/exceptions/ApiExceptionHandlerTest.java

@@ -0,0 +1,42 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You 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 org.apache.dolphinscheduler.api.exceptions;
+
+import org.apache.dolphinscheduler.api.controller.AccessTokenController;
+import org.apache.dolphinscheduler.api.enums.Status;
+import org.apache.dolphinscheduler.api.utils.Result;
+import org.apache.dolphinscheduler.dao.entity.User;
+import org.junit.Assert;
+import org.junit.Test;
+import org.springframework.web.method.HandlerMethod;
+
+import java.lang.reflect.Method;
+
+import static org.junit.Assert.*;
+
+public class ApiExceptionHandlerTest {
+
+    @Test
+    public void exceptionHandler() throws NoSuchMethodException {
+        ApiExceptionHandler handler = new ApiExceptionHandler();
+        AccessTokenController controller = new AccessTokenController();
+        Method method = controller.getClass().getMethod("createToken", User.class, int.class, String.class, String.class);
+        HandlerMethod hm = new HandlerMethod(controller, method);
+        Result result = handler.exceptionHandler(new RuntimeException("test exception"), hm);
+        Assert.assertEquals(Status.CREATE_ACCESS_TOKEN_ERROR.getCode(),result.getCode().intValue());
+    }
+}

dolphinscheduler-api/src/test/java/org/apache/dolphinscheduler/api/utils/ResultTest.java

@@ -0,0 +1,48 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You 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 org.apache.dolphinscheduler.api.utils;
+
+import org.apache.dolphinscheduler.api.enums.Status;
+import org.junit.Assert;
+import org.junit.Test;
+
+import java.util.HashMap;
+
+import static org.junit.Assert.*;
+
+public class ResultTest {
+
+    @Test
+    public void success() {
+        HashMap<String, String> map = new HashMap<>();
+        map.put("testdata", "test");
+        Result ret = Result.success(map);
+        Assert.assertEquals(Status.SUCCESS.getCode(), ret.getCode().intValue());
+    }
+
+    @Test
+    public void error() {
+        Result ret = Result.error(Status.ACCESS_TOKEN_NOT_EXIST);
+        Assert.assertEquals(Status.ACCESS_TOKEN_NOT_EXIST.getCode(), ret.getCode().intValue());
+    }
+
+    @Test
+    public void errorWithArgs() {
+        Result ret = Result.errorWithArgs(Status.INTERNAL_SERVER_ERROR_ARGS, "test internal server error");
+        Assert.assertEquals(Status.INTERNAL_SERVER_ERROR_ARGS.getCode(), ret.getCode().intValue());
+    }
+}

pom.xml

@@ -694,6 +694,7 @@
                         <include>**/api/dto/resources/visitor/ResourceTreeVisitorTest.java</include>
                         <include>**/api/enums/testGetEnum.java</include>
                         <include>**/api/enums/StatusTest.java</include>
+                        <include>**/api/exceptions/ApiExceptionHandlerTest.java</include>
                         <include>**/api/interceptor/LoginHandlerInterceptorTest.java</include>
                         <include>**/api/security/PasswordAuthenticatorTest.java</include>
                         <include>**/api/security/SecurityConfigTest.java</include>
@@ -726,6 +727,7 @@
                         <include>**/api/utils/FileUtilsTest.java</include>
                         <include>**/api/utils/CheckUtilsTest.java</include>
                         <include>**/api/utils/CheckUtilsTest.java</include>
+                        <include>**/api/utils/ResultTest.java</include>
                         <include>**/common/graph/DAGTest.java</include>
                         <include>**/common/os/OshiTest.java</include>
                         <include>**/common/os/OSUtilsTest.java</include>