jhipster / generator-jhipster

JHipster is a development platform to quickly generate, develop, & deploy modern web applications & microservice architectures.
https://www.jhipster.tech
Apache License 2.0
21.56k stars 4.02k forks source link

Either MissingServletRequestPartException and MissingServletRequestParameterException should throw a Bad Request error code #6283

Closed xtremebiker closed 7 years ago

xtremebiker commented 7 years ago
Overview of the issue

The generated ExceptionTranslator catches the MethodArgumentNotValidException and the CustomParameterizedException, but MissingServletRequestPartException and MissingServletRequestParameterException should as well be caught and treated to return bad request codes.

Reproduce the error

Created a new REST endpoint to display entity field suggestions with this signature:

@GetMapping("/installations/suggestions")
@Timed
public Set<String> getSuggestions(
@RequestParam(required = false, defaultValue = "") String pattern,
            @RequestParam String field);

Then created a new integration test that should return 400 since the required field parameter is missing:

@Test
@Transactional
public void suggestionsRequiredType() throws Exception {
    // Get the installation
    restInstallationMockMvc.perform(get("/api/installations/suggestions?pattern=cio", installation.getId()))
            .andExpect(status().isBadRequest());
}

But the test fails as the endpoint returns a 500 response instead of 400. That's because the exception is caught by the standard processor:

@ExceptionHandler(Exception.class)
public ResponseEntity<ErrorVM> processException(Exception ex) {
    log.error(ex.getMessage(), ex);
    BodyBuilder builder;
    ErrorVM errorVM;
    ResponseStatus responseStatus = AnnotationUtils.findAnnotation(ex.getClass(), ResponseStatus.class);
    if (responseStatus != null) {
        builder = ResponseEntity.status(responseStatus.value());
        errorVM = new ErrorVM("error." + responseStatus.value().value(), responseStatus.reason());
    } else {
        builder = ResponseEntity.status(HttpStatus.INTERNAL_SERVER_ERROR);
        errorVM = new ErrorVM(ErrorConstants.ERR_INTERNAL_SERVER_ERROR, "Internal server error");
    }
    return builder.body(errorVM);
}
Suggest a Fix

Adding this in the ExceptionTranslator solved it:

@ExceptionHandler(MissingServletRequestParameterException.class)
@ResponseBody
@ResponseStatus(HttpStatus.BAD_REQUEST)
public ErrorVM processMissingServletRequestParameterException(MissingServletRequestParameterException exception) {
    return new ErrorVM(ErrorConstants.ERR_BAD_REQUEST, exception.getMessage());
}
JHipster Version(s)

4.4.1

jdubois commented 7 years ago

Understood -> this part of JHipster isn't very clear, let me "handle" this :-)

jdubois commented 7 years ago

OK so this part of JHipster is much worse than I thought!!

Anyway: that's bad, it takes some time to fix (as i18n is huge now), and I know what to do -> I should have some time this week (I have a 7 hour flight on Thursday), so I'm assigning this to myself.

cbornet commented 7 years ago

There are other standard Spring exceptions that we override : https://github.com/spring-projects/spring-framework/blob/a0bce618c2f51d8af1fc00ee2c3868ba2c8e0045/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/annotation/ResponseEntityExceptionHandler.java#L106 We should probably do something for them also. Also, I propose to use rfc7807 application/problem+json errors. I believe they should work well with our front-end.

cbornet commented 7 years ago

After some digging, I think we should remove our ExceptionHandler beans and use a global ErrorAttributes instead. I'll try something asap.