[Improvement][API] Change the way exceptions are handled in the API

[lyleshaw]

Search before asking

• I had searched in the issues and found no similar feature requirement.

Description

Now we put the exception into the Result class and parse it.
This is not an elegant approach, so we would like to change the original

this.putMsg(result, Status.XXX, XXX);
return result;

to

throw new ServiceException(String.format("xxx %s doesn't valid", xxx));

Are you willing to submit a PR?

• Yes I am willing to submit a PR!

Code of Conduct

• I agree to follow this project's Code of Conduct

[github-actions]

Thank you for your feedback, we have received your issue, Please wait patiently for a reply.

• In order for us to understand your request as soon as possible, please provide detailed information、version or pictures.
• If you haven't received a reply for a long time, you can join our slack and send your question to channel #troubleshooting

[SbloodyS]

I think putMsg and Exception are two different behaviors. We use putMsg to return valid error messages through API, while exceptions will not be returned through API.

[lyleshaw]

I think putMsg and Exception are two different behaviors. We use putMsg to return valid error messages through API, while exceptions will not be returned through API.

@zhongjiajie Hi Jiajie, What's your opinion about this issue?

[zhongjiajie]

I think putMsg and Exception are two different behaviors. We use putMsg to return valid error messages through API, while exceptions will not be returned through API.

I have discuss with @caishunfeng , we think putMsg it is redundant and we should throw exception directly in serviceImpl, So we add a new ExceptionHandler in

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

Lines 39 to 43 in ae6aa53

	@ExceptionHandler(ServiceException.class)
	public Result exceptionHandler(ServiceException e, HandlerMethod hm) {
	logger.error("ServiceException: ", e);
	return new Result(e.getCode(), e.getMessage());
	}

from api side, which will covert the ServiceException to Result object, and return in controller. And we already have some existing code in

dolphinscheduler/dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/service/impl/TenantServiceImpl.java

Lines 94 to 105 in ae6aa53

	private void tenantCodeValid(String tenantCode) throws ServiceException {
	Map<String, Object> result = new HashMap<>();
	if (StringUtils.isEmpty(tenantCode)) {
	throw new ServiceException(Status.REQUEST_PARAMS_NOT_VALID_ERROR, tenantCode);
	} else if (StringUtils.length(tenantCode) > TENANT_FULL_NAME_MAX_LENGTH) {
	throw new ServiceException(Status.TENANT_FULL_NAME_TOO_LONG_ERROR);
	} else if (!RegexUtils.isValidLinuxUserName(tenantCode)) {
	throw new ServiceException(Status.CHECK_OS_TENANT_CODE_ERROR);
	} else if (checkTenantExists(tenantCode)) {
	throw new ServiceException(Status.OS_TENANT_CODE_EXIST, tenantCode);
	}
	}

And @SbloodyS do you think it is a good idea about this change? @SbloodyS

[SbloodyS]

That make sense to me. And it's a big project to modify it. I suggest mark it as a DSIP and make a todo list just as we do in #10257. WDYT? @zhongjiajie

[CalvinKirs]

I think putMsg and Exception are two different behaviors. We use putMsg to return valid error messages through API, while exceptions will not be returned through API.

I have discuss with @caishunfeng , we think putMsg it is redundant and we should throw exception directly in serviceImpl, So we add a new ExceptionHandler in

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

Lines 39 to 43 in ae6aa53

	@ExceptionHandler(ServiceException.class)
	public Result exceptionHandler(ServiceException e, HandlerMethod hm) {
	logger.error("ServiceException: ", e);
	return new Result(e.getCode(), e.getMessage());
	}

from api side, which will covert the ServiceException to Result object, and return in controller. And we already have some existing code in

dolphinscheduler/dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/service/impl/TenantServiceImpl.java

Lines 94 to 105 in ae6aa53

	private void tenantCodeValid(String tenantCode) throws ServiceException {
	Map<String, Object> result = new HashMap<>();
	if (StringUtils.isEmpty(tenantCode)) {
	throw new ServiceException(Status.REQUEST_PARAMS_NOT_VALID_ERROR, tenantCode);
	} else if (StringUtils.length(tenantCode) > TENANT_FULL_NAME_MAX_LENGTH) {
	throw new ServiceException(Status.TENANT_FULL_NAME_TOO_LONG_ERROR);
	} else if (!RegexUtils.isValidLinuxUserName(tenantCode)) {
	throw new ServiceException(Status.CHECK_OS_TENANT_CODE_ERROR);
	} else if (checkTenantExists(tenantCode)) {
	throw new ServiceException(Status.OS_TENANT_CODE_EXIST, tenantCode);
	}
	}

And @SbloodyS do you think it is a good idea about this change? @SbloodyS

+1， make sense to me, Result is usually only used as a standard data format for external system interaction.

[github-actions]

This issue has been automatically marked as stale because it has not had recent activity for 30 days. It will be closed in next 7 days if no further activity occurs.