Avoid 500 response unless necessary (#509)

* Correct error responses

- Move error handling to its own class
- Change order so plain text is a fallback
- Less logging on user errors
- ERROR level logging for INTERNAL_SERVER_ERROR to make these visible
- Better test coverage

* Just return 404 for unknown paths

Author: mmwinther

klass-api/src/main/java/no/ssb/klass/api/controllers/ClassificationController.java

@@ -26,7 +26,6 @@
 import no.ssb.klass.core.util.AlphaNumericalComparator;
 import no.ssb.klass.core.util.ClientException;
 import no.ssb.klass.core.util.DateRange;
-import no.ssb.klass.core.util.KlassResourceNotFoundException;
 
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -40,14 +39,12 @@
 import org.springframework.hateoas.server.mvc.WebMvcLinkBuilder;
 import org.springframework.http.HttpStatus;
 import org.springframework.http.MediaType;
-import org.springframework.http.ProblemDetail;
 import org.springframework.http.ResponseEntity;
 import org.springframework.transaction.annotation.Transactional;
 import org.springframework.transaction.interceptor.TransactionAspectSupport;
 import org.springframework.web.bind.WebDataBinder;
 import org.springframework.web.bind.annotation.*;
 import org.springframework.web.client.RestClientException;
-import org.springframework.web.method.annotation.MethodArgumentTypeMismatchException;
 import org.springframework.web.servlet.support.ServletUriComponentsBuilder;
 import org.springframework.web.servlet.view.RedirectView;
 
@@ -71,8 +68,6 @@ public class ClassificationController {
     private final StatisticsService statisticsService;
     private final CsvFieldsValidator csvFieldsValidator;
 
-    private static final String EXCEPTION_HANDLER_LOG_MESSAGE_TEMPLATE = "{}. For request: {}";
-
     @Autowired
     public ClassificationController(
             ClassificationService classificationService,
@@ -87,96 +82,6 @@ public ClassificationController(
         this.csvFieldsValidator = csvFieldsValidator;
     }
 
-    @ExceptionHandler(
-            exception = KlassResourceNotFoundException.class,
-            produces = {MediaType.TEXT_PLAIN_VALUE, RestConstants.CONTENT_TYPE_CSV})
-    @ResponseStatus(HttpStatus.NOT_FOUND)
-    public String resourceNotFoundTextExceptionHandler(KlassResourceNotFoundException exception) {
-        log.info(
-                EXCEPTION_HANDLER_LOG_MESSAGE_TEMPLATE,
-                exception.getMessage(),
-                getCurrentRequest());
-        return exception.getMessage();
-    }
-
-    @ExceptionHandler(
-            exception = KlassResourceNotFoundException.class,
-            produces = {
-                MediaType.APPLICATION_JSON_VALUE,
-                MediaType.APPLICATION_PROBLEM_JSON_VALUE,
-                MediaType.TEXT_XML_VALUE,
-                MediaType.APPLICATION_XML_VALUE,
-                MediaType.APPLICATION_PROBLEM_XML_VALUE
-            })
-    @ResponseStatus(HttpStatus.NOT_FOUND)
-    public ProblemDetail resourceNotFoundProblemDetailExceptionHandler(
-            KlassResourceNotFoundException exception) {
-        log.info(
-                EXCEPTION_HANDLER_LOG_MESSAGE_TEMPLATE,
-                exception.getMessage(),
-                getCurrentRequest());
-        return ProblemDetail.forStatusAndDetail(HttpStatus.NOT_FOUND, exception.getMessage());
-    }
-
-    @ExceptionHandler(
-            exception = {
-                RestClientException.class,
-                MethodArgumentTypeMismatchException.class,
-                IllegalArgumentException.class
-            },
-            produces = {MediaType.TEXT_PLAIN_VALUE, RestConstants.CONTENT_TYPE_CSV})
-    @ResponseStatus(HttpStatus.BAD_REQUEST)
-    public String badRequestTextExceptionHandler(Exception exception) {
-        return exception.getMessage();
-    }
-
-    @ExceptionHandler(
-            exception = {
-                RestClientException.class,
-                MethodArgumentTypeMismatchException.class,
-                IllegalArgumentException.class
-            },
-            produces = {
-                MediaType.APPLICATION_JSON_VALUE,
-                MediaType.APPLICATION_PROBLEM_JSON_VALUE,
-                MediaType.TEXT_XML_VALUE,
-                MediaType.APPLICATION_XML_VALUE,
-                MediaType.APPLICATION_PROBLEM_XML_VALUE
-            })
-    @ResponseStatus(HttpStatus.BAD_REQUEST)
-    public ProblemDetail badRequestProblemDetailExceptionHandler(Exception exception) {
-        return ProblemDetail.forStatusAndDetail(HttpStatus.BAD_REQUEST, exception.getMessage());
-    }
-
-    @ExceptionHandler
-    @ResponseStatus(HttpStatus.INTERNAL_SERVER_ERROR)
-    public String serverErrorTextExceptionHandler(Exception exception) {
-        log.warn(
-                EXCEPTION_HANDLER_LOG_MESSAGE_TEMPLATE,
-                exception.getMessage(),
-                getCurrentRequest(),
-                exception);
-        return exception.getMessage();
-    }
-
-    @ExceptionHandler(
-            produces = {
-                MediaType.APPLICATION_JSON_VALUE,
-                MediaType.APPLICATION_PROBLEM_JSON_VALUE,
-                MediaType.TEXT_XML_VALUE,
-                MediaType.APPLICATION_XML_VALUE,
-                MediaType.APPLICATION_PROBLEM_XML_VALUE
-            })
-    @ResponseStatus(HttpStatus.INTERNAL_SERVER_ERROR)
-    public ProblemDetail serverErrorProblemDetailExceptionHandler(Exception exception) {
-        log.warn(
-                EXCEPTION_HANDLER_LOG_MESSAGE_TEMPLATE,
-                exception.getMessage(),
-                getCurrentRequest(),
-                exception);
-        return ProblemDetail.forStatusAndDetail(HttpStatus.BAD_REQUEST, exception.getMessage());
-    }
-
     /** Redirect root to docs for convenience */
     @Hidden // We don't want to document this
     @GetMapping("/")
@@ -729,8 +634,7 @@ public CorrespondenceItemList corresponds(
     public CorrespondenceItemList correspondsAt(
             @PathVariable Long id,
             @RequestParam(value = "targetClassificationId") Long targetClassificationId,
-            @RequestParam(value = "date", required = false)
-                    @DateTimeFormat(pattern = RestConstants.DATE_FORMAT)
+            @RequestParam(value = "date") @DateTimeFormat(pattern = RestConstants.DATE_FORMAT)
                     LocalDate date,
             @RequestParam(value = "csvSeparator", defaultValue = ",") String csvSeparator,
             @RequestParam(value = "csvFields", defaultValue = "") String csvFields,

klass-api/src/main/java/no/ssb/klass/api/controllers/handlers/Handlers.java

@@ -0,0 +1,112 @@
+package no.ssb.klass.api.controllers.handlers;
+
+import jakarta.servlet.http.HttpServletRequest;
+
+import no.ssb.klass.core.util.KlassResourceNotFoundException;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import org.springframework.http.HttpStatus;
+import org.springframework.http.MediaType;
+import org.springframework.http.ProblemDetail;
+import org.springframework.web.bind.MissingServletRequestParameterException;
+import org.springframework.web.bind.annotation.*;
+import org.springframework.web.client.RestClientException;
+import org.springframework.web.method.annotation.MethodArgumentTypeMismatchException;
+import org.springframework.web.servlet.NoHandlerFoundException;
+
+@RestControllerAdvice
+public class Handlers {
+    private static final Logger log = LoggerFactory.getLogger(Handlers.class);
+
+    private static final String EXCEPTION_HANDLER_LOG_MESSAGE_TEMPLATE = "{}. For request: {}";
+
+    @ExceptionHandler(
+            exception = {KlassResourceNotFoundException.class, NoHandlerFoundException.class},
+            produces = {
+                MediaType.APPLICATION_JSON_VALUE,
+                MediaType.APPLICATION_PROBLEM_JSON_VALUE,
+                MediaType.TEXT_XML_VALUE,
+                MediaType.APPLICATION_XML_VALUE,
+                MediaType.APPLICATION_PROBLEM_XML_VALUE
+            })
+    @ResponseStatus(HttpStatus.NOT_FOUND)
+    public ProblemDetail resourceNotFoundProblemDetailExceptionHandler(Exception exception) {
+        if (exception.getClass() == KlassResourceNotFoundException.class) {
+            return ProblemDetail.forStatusAndDetail(HttpStatus.NOT_FOUND, exception.getMessage());
+        }
+        return ProblemDetail.forStatus(HttpStatus.NOT_FOUND);
+    }
+
+    @ExceptionHandler(
+            exception = {KlassResourceNotFoundException.class, NoHandlerFoundException.class})
+    @ResponseStatus(HttpStatus.NOT_FOUND)
+    public @ResponseBody String resourceNotFoundTextExceptionHandler(Exception exception) {
+        if (exception.getClass() == KlassResourceNotFoundException.class) {
+            return exception.getMessage();
+        }
+        return "";
+    }
+
+    @ExceptionHandler(
+            exception = {
+                RestClientException.class,
+                MethodArgumentTypeMismatchException.class,
+                IllegalArgumentException.class,
+                MissingServletRequestParameterException.class,
+                java.lang.NumberFormatException.class
+            },
+            produces = {
+                MediaType.APPLICATION_JSON_VALUE,
+                MediaType.APPLICATION_PROBLEM_JSON_VALUE,
+                MediaType.TEXT_XML_VALUE,
+                MediaType.APPLICATION_XML_VALUE,
+                MediaType.APPLICATION_PROBLEM_XML_VALUE
+            })
+    @ResponseStatus(HttpStatus.BAD_REQUEST)
+    public ProblemDetail badRequestProblemDetailExceptionHandler(Exception exception) {
+        return ProblemDetail.forStatusAndDetail(HttpStatus.BAD_REQUEST, exception.getMessage());
+    }
+
+    @ExceptionHandler(
+            exception = {
+                RestClientException.class,
+                MethodArgumentTypeMismatchException.class,
+                IllegalArgumentException.class,
+                MissingServletRequestParameterException.class
+            })
+    @ResponseStatus(HttpStatus.BAD_REQUEST)
+    public @ResponseBody String badRequestTextExceptionHandler(Exception exception) {
+        return exception.getMessage();
+    }
+
+    @ExceptionHandler(
+            produces = {
+                MediaType.APPLICATION_JSON_VALUE,
+                MediaType.APPLICATION_PROBLEM_JSON_VALUE,
+                MediaType.TEXT_XML_VALUE,
+                MediaType.APPLICATION_XML_VALUE,
+                MediaType.APPLICATION_PROBLEM_XML_VALUE
+            })
+    @ResponseStatus(HttpStatus.INTERNAL_SERVER_ERROR)
+    public ProblemDetail serverErrorProblemDetailExceptionHandler(
+            Exception exception, HttpServletRequest request) {
+        log.error(
+                EXCEPTION_HANDLER_LOG_MESSAGE_TEMPLATE,
+                exception.getMessage(),
+                request.getRequestURI(),
+                exception);
+        return ProblemDetail.forStatusAndDetail(HttpStatus.BAD_REQUEST, exception.getMessage());
+    }
+
+    @ExceptionHandler
+    @ResponseStatus(HttpStatus.INTERNAL_SERVER_ERROR)
+    public String serverErrorTextExceptionHandler(Exception exception, HttpServletRequest request) {
+        log.error(
+                EXCEPTION_HANDLER_LOG_MESSAGE_TEMPLATE,
+                exception.getMessage(),
+                request.getRequestURI(),
+                exception);
+        return exception.getMessage();
+    }
+}

klass-api/src/main/resources/application.properties

@@ -44,9 +44,10 @@ management.metrics.tags.application=klass
 klass.env.search.elasticsearch.index=klass
 springdoc.swagger-ui.path=${klass.env.api.base.path}/swagger-ui.html
 springdoc.api-docs.path=${klass.env.api.base.path}/v3/api-docs
+spring.web.resources.add-mappings=false
 # Logging properties
-logging.level.org.springframework.web.servlet=TRACE
-#logging.level.no.ssb.klass=INFO
+logging.level.org.springframework.web.servlet.mvc.method.annotation.RequestMappingHandlerMapping=TRACE
+logging.level.no.ssb.klass=INFO
 # Mail properties
 klass.env.client.klass-mail.url=http://localhost:8082
 # hibernate config

klass-api/src/test/java/no/ssb/klass/api/ApiDocumentation.java

@@ -21,6 +21,7 @@
 
 import no.ssb.klass.api.config.MockConfig;
 import no.ssb.klass.api.config.TestConfig;
+import no.ssb.klass.api.controllers.handlers.Handlers;
 import no.ssb.klass.api.services.OpenSearchResult;
 import no.ssb.klass.api.services.SearchService;
 import no.ssb.klass.api.util.RestConstants;
@@ -45,6 +46,7 @@
 import org.springframework.boot.autoconfigure.elasticsearch.ElasticsearchRestClientAutoConfiguration;
 import org.springframework.boot.test.context.SpringBootTest;
 import org.springframework.boot.test.context.SpringBootTest.WebEnvironment;
+import org.springframework.context.annotation.Import;
 import org.springframework.data.domain.Page;
 import org.springframework.data.domain.PageImpl;
 import org.springframework.data.domain.Pageable;
@@ -94,6 +96,7 @@
 @AutoConfigureEmbeddedDatabase(
         provider = EMBEDDED,
         type = AutoConfigureEmbeddedDatabase.DatabaseType.POSTGRES)
+@Import(Handlers.class)
 public class ApiDocumentation {
     private static final int CLASS_ID_FAMILIEGRUPPERING = 17;
     private static final int CLASS_ID_GREENHOUSE_GASES = 84;

klass-api/src/test/java/no/ssb/klass/api/applicationtest/RestApiErrorHandlingIntegrationTest.java

@@ -2,6 +2,8 @@
 
 import static io.restassured.RestAssured.*;
 
+import io.restassured.http.ContentType;
+
 import no.ssb.klass.api.util.RestConstants;
 
 import org.junit.jupiter.params.ParameterizedTest;
@@ -12,11 +14,6 @@
 
 import java.util.stream.Stream;
 
-/**
- * @author Mads Lundemo, SSB.
- *     <p>Testsuite that test how the Rest Api handles unexpected sutuations (tests with JSON and
- *     XML)
- */
 class RestApiErrorHandlingIntegrationTest extends AbstractRestApiApplicationTest {
 
     private static Stream<Arguments> contentTypes() {
@@ -27,6 +24,25 @@ private static Stream<Arguments> contentTypes() {
                 Arguments.of(RestConstants.CONTENT_TYPE_CSV, RestConstants.CONTENT_TYPE_CSV));
     }
 
+    private static Stream<Arguments> badRequests() {
+        return Stream.of(
+                Arguments.of("/api/klass/v1/classifications/139/changes"),
+                Arguments.of("/api/klass/v1/classifications/2%3Flanguage=en"),
+                Arguments.of("/api/klass/v1/classifications/null"),
+                Arguments.of(
+                        "/api/klass/v1/classifications/1/correspondsAt?targetClassificationId=2"),
+                Arguments.of("/api/klass/v1/classifications/145/variantAt"));
+    }
+
+    private static Stream<Arguments> notFounds() {
+        return Stream.of(
+                Arguments.of("/unknown/path"),
+                Arguments.of("/api/klass/v1/classifications/99999"),
+                Arguments.of("/api/klass/v1/versions/99999"),
+                Arguments.of(
+                        "/api/klass/v1/classifications/1/correspondsAt?targetClassificationId=99999&date=2020-01-01"));
+    }
+
     @ParameterizedTest
     @MethodSource("contentTypes")
     void classificationNotFound(String acceptedContentType, String responseContentType) {
@@ -56,4 +72,28 @@ void invalidPathParameter(String acceptedContentType, String responseContentType
                 .statusCode(HttpStatus.BAD_REQUEST.value())
                 .contentType(responseContentType);
     }
+
+    @ParameterizedTest
+    @MethodSource("badRequests")
+    void badRequests(String path) {
+        given().port(port)
+                .contentType(ContentType.JSON)
+                .when()
+                .get(path)
+                .prettyPeek()
+                .then()
+                .statusCode(HttpStatus.BAD_REQUEST.value());
+    }
+
+    @ParameterizedTest
+    @MethodSource("notFounds")
+    void notFounds(String path) {
+        given().port(port)
+                .contentType(ContentType.JSON)
+                .when()
+                .get(path)
+                .prettyPeek()
+                .then()
+                .statusCode(HttpStatus.NOT_FOUND.value());
+    }
 }