zhongjiajie commented on code in PR #11255:
URL: https://github.com/apache/dolphinscheduler/pull/11255#discussion_r937320395
##########
dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/service/impl/UsersServiceImpl.java:
##########
@@ -383,71 +384,33 @@ public Map<String, Object> updateUser(User loginUser, int
userId,
String phone,
String queue,
int state,
- String timeZone) throws IOException {
+ String timeZone) {
Map<String, Object> result = new HashMap<>();
result.put(Constants.STATUS, false);
if (resourcePermissionCheckService.functionDisabled()) {
- putMsg(result, Status.FUNCTION_DISABLED);
- return result;
+ throw new ServiceException("Function Disabled");
}
if (check(result, !canOperator(loginUser, userId),
Status.USER_NO_OPERATION_PERM)) {
- return result;
+ throw new ServiceException("User No Operation Perm");
}
User user = userMapper.selectById(userId);
if (user == null) {
- putMsg(result, Status.USER_NOT_EXIST, userId);
- return result;
- }
- if (StringUtils.isNotEmpty(userName)) {
-
- if (!CheckUtils.checkUserName(userName)) {
- putMsg(result, Status.REQUEST_PARAMS_NOT_VALID_ERROR,
userName);
- return result;
- }
-
- User tempUser = userMapper.queryByUserNameAccurately(userName);
- if (tempUser != null && tempUser.getId() != userId) {
- putMsg(result, Status.USER_NAME_EXIST);
- return result;
- }
- user.setUserName(userName);
+ throw new ServiceException("User Doesn't Exist");
}
- if (StringUtils.isNotEmpty(userPassword)) {
- if (!CheckUtils.checkPasswordLength(userPassword)) {
- putMsg(result, Status.USER_PASSWORD_LENGTH_ERROR);
- return result;
- }
- user.setUserPassword(EncryptionUtils.getMd5(userPassword));
- }
-
- if (StringUtils.isNotEmpty(email)) {
- if (!CheckUtils.checkEmail(email)) {
- putMsg(result, Status.REQUEST_PARAMS_NOT_VALID_ERROR, email);
- return result;
- }
- user.setEmail(email);
- }
-
- if (StringUtils.isNotEmpty(phone) && !CheckUtils.checkPhone(phone)) {
- putMsg(result, Status.REQUEST_PARAMS_NOT_VALID_ERROR, phone);
- return result;
- }
-
- if (state == 0 && user.getState() != state && loginUser.getId() ==
user.getId()) {
- putMsg(result, Status.NOT_ALLOW_TO_DISABLE_OWN_ACCOUNT);
- return result;
- }
+ checkUserParams(userName, userPassword, email, phone, state);
if (StringUtils.isNotEmpty(timeZone)) {
if (!CheckUtils.checkTimeZone(timeZone)) {
- putMsg(result, Status.TIME_ZONE_ILLEGAL, timeZone);
- return result;
+ throw new ServiceException(String.format("timeZone %s doesn't
valid", timeZone));
Review Comment:
same as here, reuse status
```suggestion
throw new
ServiceException(MessageFormat.format(Status.TIME_ZONE_ILLEGAL.getMsg(),
timeZone));
```
##########
dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/service/impl/UsersServiceImpl.java:
##########
@@ -383,71 +384,33 @@ public Map<String, Object> updateUser(User loginUser, int
userId,
String phone,
String queue,
int state,
- String timeZone) throws IOException {
+ String timeZone) {
Map<String, Object> result = new HashMap<>();
result.put(Constants.STATUS, false);
if (resourcePermissionCheckService.functionDisabled()) {
- putMsg(result, Status.FUNCTION_DISABLED);
- return result;
+ throw new ServiceException("Function Disabled");
}
if (check(result, !canOperator(loginUser, userId),
Status.USER_NO_OPERATION_PERM)) {
- return result;
+ throw new ServiceException("User No Operation Perm");
Review Comment:
we can reuse the `status` here
```suggestion
throw new
ServiceException(Status.USER_NO_OPERATION_PERM.getMsg());
```
##########
dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/service/impl/UsersServiceImpl.java:
##########
@@ -383,71 +384,33 @@ public Map<String, Object> updateUser(User loginUser, int
userId,
String phone,
String queue,
int state,
- String timeZone) throws IOException {
+ String timeZone) {
Map<String, Object> result = new HashMap<>();
result.put(Constants.STATUS, false);
if (resourcePermissionCheckService.functionDisabled()) {
- putMsg(result, Status.FUNCTION_DISABLED);
- return result;
+ throw new ServiceException("Function Disabled");
}
if (check(result, !canOperator(loginUser, userId),
Status.USER_NO_OPERATION_PERM)) {
- return result;
+ throw new ServiceException("User No Operation Perm");
}
User user = userMapper.selectById(userId);
if (user == null) {
- putMsg(result, Status.USER_NOT_EXIST, userId);
- return result;
- }
- if (StringUtils.isNotEmpty(userName)) {
-
- if (!CheckUtils.checkUserName(userName)) {
- putMsg(result, Status.REQUEST_PARAMS_NOT_VALID_ERROR,
userName);
- return result;
- }
-
- User tempUser = userMapper.queryByUserNameAccurately(userName);
- if (tempUser != null && tempUser.getId() != userId) {
- putMsg(result, Status.USER_NAME_EXIST);
- return result;
- }
- user.setUserName(userName);
+ throw new ServiceException("User Doesn't Exist");
}
- if (StringUtils.isNotEmpty(userPassword)) {
- if (!CheckUtils.checkPasswordLength(userPassword)) {
- putMsg(result, Status.USER_PASSWORD_LENGTH_ERROR);
- return result;
- }
- user.setUserPassword(EncryptionUtils.getMd5(userPassword));
- }
-
- if (StringUtils.isNotEmpty(email)) {
- if (!CheckUtils.checkEmail(email)) {
- putMsg(result, Status.REQUEST_PARAMS_NOT_VALID_ERROR, email);
- return result;
- }
- user.setEmail(email);
- }
-
- if (StringUtils.isNotEmpty(phone) && !CheckUtils.checkPhone(phone)) {
- putMsg(result, Status.REQUEST_PARAMS_NOT_VALID_ERROR, phone);
- return result;
- }
-
- if (state == 0 && user.getState() != state && loginUser.getId() ==
user.getId()) {
- putMsg(result, Status.NOT_ALLOW_TO_DISABLE_OWN_ACCOUNT);
- return result;
- }
+ checkUserParams(userName, userPassword, email, phone, state);
if (StringUtils.isNotEmpty(timeZone)) {
if (!CheckUtils.checkTimeZone(timeZone)) {
- putMsg(result, Status.TIME_ZONE_ILLEGAL, timeZone);
- return result;
+ throw new ServiceException(String.format("timeZone %s doesn't
valid", timeZone));
}
user.setTimeZone(timeZone);
}
+ user.setUserName(userName);
+ user.setUserPassword(EncryptionUtils.getMd5(userPassword));
+ user.setEmail(email);
user.setPhone(phone);
user.setQueue(queue);
user.setState(state);
Review Comment:
better to add new construct to init new object
##########
dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/service/impl/UsersServiceImpl.java:
##########
@@ -383,71 +384,33 @@ public Map<String, Object> updateUser(User loginUser, int
userId,
String phone,
String queue,
int state,
- String timeZone) throws IOException {
+ String timeZone) {
Map<String, Object> result = new HashMap<>();
result.put(Constants.STATUS, false);
if (resourcePermissionCheckService.functionDisabled()) {
- putMsg(result, Status.FUNCTION_DISABLED);
- return result;
+ throw new ServiceException("Function Disabled");
Review Comment:
```suggestion
throw new ServiceException(Status.FUNCTION_DISABLED.getMsg());
```
##########
dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/service/impl/UsersServiceImpl.java:
##########
@@ -1107,6 +1070,52 @@ private String checkUserParams(String userName, String
password, String email, S
return msg;
}
+ /**
+ * @return if check failed throw error, otherwise return null
+ */
+ private void checkUserParams(String userName, String userPassword, String
email, String phone, Integer state) {
+
+ if (StringUtils.isNotEmpty(userName)) {
+
+ if (!CheckUtils.checkUserName(userName)) {
+ throw new ServiceException(String.format("userName %s doesn't
valid", userName));
Review Comment:
same here, when we already have status, we can reuse the status.getMsg()
##########
dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/service/impl/UsersServiceImpl.java:
##########
@@ -206,11 +206,13 @@ public User createUser(String userName,
User user = new User();
Date now = new Date();
+ checkUserParams(userName, userPassword, email, phone, state);
+
user.setUserName(userName);
user.setUserPassword(EncryptionUtils.getMd5(userPassword));
user.setEmail(email);
- user.setTenantId(tenantId);
user.setPhone(phone);
+ user.setTenantId(tenantId);
user.setState(state);
// create general users, administrator users are currently built-in
user.setUserType(UserType.GENERAL_USER);
Review Comment:
Make we can add a new constructors to class `User`, just like we add it to
https://github.com/apache/dolphinscheduler/blob/a6154220dc9fd78a894f9fc47c78378cba1d03cd/dolphinscheduler-dao/src/main/java/org/apache/dolphinscheduler/dao/entity/Queue.java#L57-L63
##########
dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/service/impl/UsersServiceImpl.java:
##########
@@ -383,71 +384,33 @@ public Map<String, Object> updateUser(User loginUser, int
userId,
String phone,
String queue,
int state,
- String timeZone) throws IOException {
+ String timeZone) {
Map<String, Object> result = new HashMap<>();
result.put(Constants.STATUS, false);
if (resourcePermissionCheckService.functionDisabled()) {
- putMsg(result, Status.FUNCTION_DISABLED);
- return result;
+ throw new ServiceException("Function Disabled");
}
if (check(result, !canOperator(loginUser, userId),
Status.USER_NO_OPERATION_PERM)) {
- return result;
+ throw new ServiceException("User No Operation Perm");
}
User user = userMapper.selectById(userId);
if (user == null) {
- putMsg(result, Status.USER_NOT_EXIST, userId);
- return result;
- }
- if (StringUtils.isNotEmpty(userName)) {
-
- if (!CheckUtils.checkUserName(userName)) {
- putMsg(result, Status.REQUEST_PARAMS_NOT_VALID_ERROR,
userName);
- return result;
- }
-
- User tempUser = userMapper.queryByUserNameAccurately(userName);
- if (tempUser != null && tempUser.getId() != userId) {
- putMsg(result, Status.USER_NAME_EXIST);
- return result;
- }
- user.setUserName(userName);
+ throw new ServiceException("User Doesn't Exist");
Review Comment:
```suggestion
throw new ServiceException(Status.USER_NOT_EXIST.getMsg());
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
