From bdc53195f08c78f7c221985efbaf8ddd27b4d047 Mon Sep 17 00:00:00 2001 From: Arpit Mohan Date: Wed, 28 Aug 2019 09:35:06 +0000 Subject: [PATCH] Sending standardized HTTP error codes to the client application. This structure ensures that we can define the http error code, app error code and (in future) localized string when we want to. Now when we want to send any error message to the client, we should add it to the enum `AppsmithError` and throw an `AppsmithException` by passing this enum value to the constructor. In the future, we can also localize these error messages by defining the template message in a `messages_en.properties` file. --- .../com/appsmith/server/dtos/ResponseDto.java | 9 ++++ .../server/exceptions/AppsmithError.java | 29 +++++++++++ .../server/exceptions/AppsmithException.java | 27 ++++++++++ .../appsmith/server/exceptions/ErrorDTO.java | 19 +++++++ .../exceptions/GlobalExceptionHandler.java | 49 +++++++++++++++++++ .../appsmith/server/filters/MDCFilter.java | 25 +++++++++- .../server/filters/RequestIdFilter.java | 34 ------------- .../appsmith/server/services/BaseService.java | 4 +- .../server/services/PluginService.java | 2 - .../server/services/PluginServiceImpl.java | 3 +- .../server/services/QueryServiceImpl.java | 3 +- .../server/services/ResourceServiceImpl.java | 3 +- 12 files changed, 166 insertions(+), 41 deletions(-) create mode 100644 app/server/src/main/java/com/appsmith/server/exceptions/AppsmithError.java create mode 100644 app/server/src/main/java/com/appsmith/server/exceptions/ErrorDTO.java create mode 100644 app/server/src/main/java/com/appsmith/server/exceptions/GlobalExceptionHandler.java delete mode 100644 app/server/src/main/java/com/appsmith/server/filters/RequestIdFilter.java diff --git a/app/server/src/main/java/com/appsmith/server/dtos/ResponseDto.java b/app/server/src/main/java/com/appsmith/server/dtos/ResponseDto.java index f4c12b1f0d..6699234b32 100644 --- a/app/server/src/main/java/com/appsmith/server/dtos/ResponseDto.java +++ b/app/server/src/main/java/com/appsmith/server/dtos/ResponseDto.java @@ -18,4 +18,13 @@ public class ResponseDto implements Serializable { private T data; private String message; + + private boolean success = true; + + public ResponseDto(int status, T data, String message) { + this.status = status; + this.data = data; + this.message = message; + } + } diff --git a/app/server/src/main/java/com/appsmith/server/exceptions/AppsmithError.java b/app/server/src/main/java/com/appsmith/server/exceptions/AppsmithError.java new file mode 100644 index 0000000000..0dab1465f1 --- /dev/null +++ b/app/server/src/main/java/com/appsmith/server/exceptions/AppsmithError.java @@ -0,0 +1,29 @@ +package com.appsmith.server.exceptions; + +import lombok.Getter; + +import java.text.MessageFormat; + +@Getter +public enum AppsmithError { + + NO_RESOURCE_FOUND(404, 1000, "Unable to find {0} with id {1}"), + INVALID_PARAMETER(400, 4000, "Invalid parameter {0} provided in the input"), + INTERNAL_SERVER_ERROR(500, 5000, "Internal server error while processing request"); + + private Integer httpErrorCode; + private Integer appErrorCode; + private String message; + + private AppsmithError(Integer httpErrorCode, Integer appErrorCode, String message, Object... args) { + this.httpErrorCode = httpErrorCode; + this.appErrorCode = appErrorCode; + MessageFormat fmt = new MessageFormat(message); + this.message = fmt.format(args); + } + + public String getMessage(Object... args) { + return new MessageFormat(this.message).format(args); + } + +} diff --git a/app/server/src/main/java/com/appsmith/server/exceptions/AppsmithException.java b/app/server/src/main/java/com/appsmith/server/exceptions/AppsmithException.java index 28787d8086..876fd11f7e 100644 --- a/app/server/src/main/java/com/appsmith/server/exceptions/AppsmithException.java +++ b/app/server/src/main/java/com/appsmith/server/exceptions/AppsmithException.java @@ -1,8 +1,35 @@ package com.appsmith.server.exceptions; +import lombok.Getter; +import lombok.Setter; + +@Getter +@Setter public class AppsmithException extends Exception { + private AppsmithError error; + private Object[] args; + public AppsmithException(String msg) { super(msg); } + + public AppsmithException(AppsmithError error, Object... args) { + super(error.getMessage(args)); + this.error = error; + this.args = args; + } + + public Integer getHttpStatus() { + return this.error.getHttpErrorCode(); + } + + public String getMessage(Object... args) { + return this.error.getMessage(this.args); + } + + public Integer getAppErrorCode() { + return this.error.getAppErrorCode(); + } + } diff --git a/app/server/src/main/java/com/appsmith/server/exceptions/ErrorDTO.java b/app/server/src/main/java/com/appsmith/server/exceptions/ErrorDTO.java new file mode 100644 index 0000000000..3399703740 --- /dev/null +++ b/app/server/src/main/java/com/appsmith/server/exceptions/ErrorDTO.java @@ -0,0 +1,19 @@ +package com.appsmith.server.exceptions; + +import lombok.AllArgsConstructor; +import lombok.Builder; +import lombok.Getter; +import lombok.Setter; + +import java.io.Serializable; + +@Getter +@Setter +@Builder +@AllArgsConstructor +public class ErrorDTO implements Serializable { + + private int errorCode; + + private String errorMessage; +} diff --git a/app/server/src/main/java/com/appsmith/server/exceptions/GlobalExceptionHandler.java b/app/server/src/main/java/com/appsmith/server/exceptions/GlobalExceptionHandler.java new file mode 100644 index 0000000000..113e1db21f --- /dev/null +++ b/app/server/src/main/java/com/appsmith/server/exceptions/GlobalExceptionHandler.java @@ -0,0 +1,49 @@ +package com.appsmith.server.exceptions; + +import com.appsmith.server.dtos.ResponseDto; +import org.springframework.http.HttpStatus; +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.server.ServerWebExchange; +import reactor.core.publisher.Mono; + +/** + * This class catches all the Exceptions and formats them into a proper ResponseDTO object before + * sending it to the client. + */ +@ControllerAdvice +public class GlobalExceptionHandler { + + /** + * This function only catches the AppsmithException type and formats it into ResponseEntity object + * Ideally, we should only be throwing AppsmithException from our code. This ensures that we can standardize + * and set proper error messages and codes. + * @param e AppsmithException that will be caught by the function + * @param exchange ServerWebExchange contract in order to extract the response and set the http status code + * @return Mono> + */ + @ExceptionHandler + @ResponseBody + public Mono> catchAppsmithException(AppsmithException e, ServerWebExchange exchange) { + exchange.getResponse().setStatusCode(HttpStatus.resolve(e.getHttpStatus())); + return Mono.just(new ResponseDto<>(e.getHttpStatus(), new ErrorDTO(e.getAppErrorCode(), e.getMessage()), null, false)); + } + + /** + * This function catches the generic Exception class and is meant to be a catch all to ensure that we don't leak + * any information to the client. Ideally, the function #catchAppsmithException should be used + * + * @param e Exception that will be caught by the function + * @param exchange ServerWebExchange contract in order to extract the response and set the http status code + * @return Mono> + */ + @ExceptionHandler + @ResponseBody + public Mono> catchException(Exception e, ServerWebExchange exchange) { + exchange.getResponse().setStatusCode(HttpStatus.INTERNAL_SERVER_ERROR); + return Mono.just(new ResponseDto<>(HttpStatus.INTERNAL_SERVER_ERROR.value(), new ErrorDTO(AppsmithError.INTERNAL_SERVER_ERROR.getHttpErrorCode(), + AppsmithError.INTERNAL_SERVER_ERROR.getMessage()), null, false)); + } + +} diff --git a/app/server/src/main/java/com/appsmith/server/filters/MDCFilter.java b/app/server/src/main/java/com/appsmith/server/filters/MDCFilter.java index 5cbacee57a..a233c8c602 100644 --- a/app/server/src/main/java/com/appsmith/server/filters/MDCFilter.java +++ b/app/server/src/main/java/com/appsmith/server/filters/MDCFilter.java @@ -1,6 +1,7 @@ package com.appsmith.server.filters; import com.appsmith.server.helpers.LogHelper; +import lombok.extern.slf4j.Slf4j; import org.slf4j.MDC; import org.springframework.http.HttpHeaders; import org.springframework.http.server.reactive.ServerHttpRequest; @@ -13,6 +14,7 @@ import reactor.core.publisher.Mono; import reactor.util.context.Context; import java.util.Map; +import java.util.UUID; import static java.util.stream.Collectors.toMap; @@ -21,9 +23,12 @@ import static java.util.stream.Collectors.toMap; * These MDC parameters are also set in the response object before being sent to the user. */ @Component +@Slf4j public class MDCFilter implements WebFilter { private static final String MDC_HEADER_PREFIX = "X-MDC-"; + private static final String REQUEST_ID_HEADER = "X-REQUEST-ID"; + private static final String REQUEST_ID_LOG = "requestId"; @Override public Mono filter(ServerWebExchange exchange, WebFilterChain chain) { @@ -43,6 +48,8 @@ public class MDCFilter implements WebFilter { .filter(x -> x.getKey().startsWith(MDC_HEADER_PREFIX)) .collect(toMap(v -> v.getKey().substring((MDC_HEADER_PREFIX.length())), Map.Entry::getValue)); + contextMap.put(REQUEST_ID_LOG, getOrCreateRequestId(request)); + // Set the MDC context here for regular non-reactive logs MDC.setContextMap(contextMap); @@ -59,8 +66,24 @@ public class MDCFilter implements WebFilter { final HttpHeaders httpHeaders = response.getHeaders(); // Add all the request MDC keys to the response object ctx.>get(LogHelper.CONTEXT_MAP) - .forEach((key, value) -> httpHeaders.add(MDC_HEADER_PREFIX + key, value)); + .forEach((key, value) -> { + if(!key.contains(REQUEST_ID_LOG)) { + httpHeaders.add(MDC_HEADER_PREFIX + key, value); + } else { + httpHeaders.add(REQUEST_ID_HEADER, value); + } + }); + }).then(); } + + private String getOrCreateRequestId(final ServerHttpRequest request) { + if (!request.getHeaders().containsKey(REQUEST_ID_HEADER)) { + request.mutate().header(REQUEST_ID_HEADER, UUID.randomUUID().toString()).build(); + } + + String header = request.getHeaders().get(REQUEST_ID_HEADER).get(0); + return header; + } } diff --git a/app/server/src/main/java/com/appsmith/server/filters/RequestIdFilter.java b/app/server/src/main/java/com/appsmith/server/filters/RequestIdFilter.java deleted file mode 100644 index 903bab3b13..0000000000 --- a/app/server/src/main/java/com/appsmith/server/filters/RequestIdFilter.java +++ /dev/null @@ -1,34 +0,0 @@ -package com.appsmith.server.filters; - -import com.appsmith.server.helpers.LogHelper; -import lombok.extern.slf4j.Slf4j; -import org.springframework.stereotype.Component; -import org.springframework.web.server.ServerWebExchange; -import org.springframework.web.server.WebFilter; -import org.springframework.web.server.WebFilterChain; -import reactor.core.publisher.Mono; - -import java.util.UUID; - -/** - * This class specifically parses the requestId key from the headers and sets it in the logger MDC - */ -@Slf4j -@Component -public class RequestIdFilter implements WebFilter { - - private static final String REQUEST_ID_HEADER = "X-REQUEST-ID"; - private static final String REQUEST_ID_LOG = "requestId"; - - @Override - public Mono filter(ServerWebExchange exchange, WebFilterChain chain) { - - if (!exchange.getRequest().getHeaders().containsKey(REQUEST_ID_HEADER)) { - exchange.getRequest().mutate().header(REQUEST_ID_HEADER, UUID.randomUUID().toString()).build(); - } - - String header = exchange.getRequest().getHeaders().get(REQUEST_ID_HEADER).get(0); - log.debug("Setting the requestId header to {}", header); - return chain.filter(exchange).subscriberContext(LogHelper.putLogContext(REQUEST_ID_LOG, header)); - } -} diff --git a/app/server/src/main/java/com/appsmith/server/services/BaseService.java b/app/server/src/main/java/com/appsmith/server/services/BaseService.java index 1132ff3c85..3ec6e0dcb9 100644 --- a/app/server/src/main/java/com/appsmith/server/services/BaseService.java +++ b/app/server/src/main/java/com/appsmith/server/services/BaseService.java @@ -1,8 +1,10 @@ package com.appsmith.server.services; import com.appsmith.server.domains.BaseDomain; +import com.appsmith.server.exceptions.AppsmithError; import com.appsmith.server.exceptions.AppsmithException; import com.appsmith.server.repositories.BaseRepository; + import com.mongodb.BasicDBObject; import com.mongodb.DBObject; import org.springframework.data.mongodb.core.ReactiveMongoTemplate; @@ -63,7 +65,7 @@ public abstract class BaseService getById(ID id) { return repository.findById(id) - .switchIfEmpty(Mono.error(new AppsmithException("Unable to find resource with id: " + id.toString()))); + .switchIfEmpty(Mono.error(new AppsmithException(AppsmithError.NO_RESOURCE_FOUND, "resource", id))); } @Override diff --git a/app/server/src/main/java/com/appsmith/server/services/PluginService.java b/app/server/src/main/java/com/appsmith/server/services/PluginService.java index cb9a43b053..9153dbeb4e 100644 --- a/app/server/src/main/java/com/appsmith/server/services/PluginService.java +++ b/app/server/src/main/java/com/appsmith/server/services/PluginService.java @@ -19,8 +19,6 @@ public interface PluginService extends CrudService { */ PluginExecutor getPluginExecutor(PluginType pluginType, String className); - public Mono create(Plugin plugin) throws AppsmithException; - public Mono installPlugin(PluginTenantDTO plugin); public Mono uninstallPlugin(PluginTenantDTO plugin); diff --git a/app/server/src/main/java/com/appsmith/server/services/PluginServiceImpl.java b/app/server/src/main/java/com/appsmith/server/services/PluginServiceImpl.java index 1b5fd8dbf4..33248736a5 100644 --- a/app/server/src/main/java/com/appsmith/server/services/PluginServiceImpl.java +++ b/app/server/src/main/java/com/appsmith/server/services/PluginServiceImpl.java @@ -7,6 +7,7 @@ import com.appsmith.server.domains.Tenant; import com.appsmith.server.domains.TenantPlugin; import com.appsmith.server.dtos.PluginTenantDTO; import com.appsmith.server.dtos.TenantPluginStatus; +import com.appsmith.server.exceptions.AppsmithError; import com.appsmith.server.exceptions.AppsmithException; import com.appsmith.server.repositories.PluginRepository; import com.appsmith.server.repositories.UserRepository; @@ -71,7 +72,7 @@ public class PluginServiceImpl extends BaseService create(Plugin plugin) throws AppsmithException { if (plugin.getId() != null) { - throw new AppsmithException("During create plugin, Id is not null. Can't create new plugin."); + throw new AppsmithException(AppsmithError.INVALID_PARAMETER, "id"); } plugin.setDeleted(false); return pluginRepository.save(plugin); diff --git a/app/server/src/main/java/com/appsmith/server/services/QueryServiceImpl.java b/app/server/src/main/java/com/appsmith/server/services/QueryServiceImpl.java index d1b0421cfe..c7241db460 100644 --- a/app/server/src/main/java/com/appsmith/server/services/QueryServiceImpl.java +++ b/app/server/src/main/java/com/appsmith/server/services/QueryServiceImpl.java @@ -2,6 +2,7 @@ package com.appsmith.server.services; import com.appsmith.server.domains.Query; import com.appsmith.server.dtos.CommandQueryParams; +import com.appsmith.server.exceptions.AppsmithError; import com.appsmith.server.exceptions.AppsmithException; import com.appsmith.server.repositories.QueryRepository; import lombok.extern.slf4j.Slf4j; @@ -36,7 +37,7 @@ public class QueryServiceImpl extends BaseService queryMono = repository.findByName(name) - .switchIfEmpty(Mono.defer(() -> Mono.error(new AppsmithException("Unable to find query by id: " + name)))); + .switchIfEmpty(Mono.defer(() -> Mono.error(new AppsmithException(AppsmithError.NO_RESOURCE_FOUND, "query", name)))); // 2. Instantiate the implementation class based on the query type Mono pluginExecutorMono = queryMono.map(queryObj -> diff --git a/app/server/src/main/java/com/appsmith/server/services/ResourceServiceImpl.java b/app/server/src/main/java/com/appsmith/server/services/ResourceServiceImpl.java index 418a93fd8f..fa3dfec22b 100644 --- a/app/server/src/main/java/com/appsmith/server/services/ResourceServiceImpl.java +++ b/app/server/src/main/java/com/appsmith/server/services/ResourceServiceImpl.java @@ -4,6 +4,7 @@ import com.appsmith.server.domains.Plugin; import com.appsmith.server.domains.Resource; import com.appsmith.server.domains.Tenant; import com.appsmith.server.domains.TenantPlugin; +import com.appsmith.server.exceptions.AppsmithError; import com.appsmith.server.exceptions.AppsmithException; import com.appsmith.server.repositories.ResourceRepository; import lombok.extern.slf4j.Slf4j; @@ -41,7 +42,7 @@ public class ResourceServiceImpl extends BaseService create(@NotNull Resource resource) throws AppsmithException { if (resource.getId() != null) { - throw new AppsmithException("During create resource, Id is not null. Can't create new resource."); + throw new AppsmithException(AppsmithError.INVALID_PARAMETER, "id"); } Mono tenantMono = tenantService.findById(tenantId);