Frachtwerk / essencium-backend

Essencium Backend is a software library built on top of Spring Boot that allows developers to quickly get started on new software projects. Essencium provides, for example, a fully implemented role-rights concept as well as various field-tested solutions for access management and authentication.
GNU Lesser General Public License v3.0
15 stars 3 forks source link

`@NotNull` Jakarta validation is not executed for method parameters #380

Open JelmenGuhlke opened 6 months ago

JelmenGuhlke commented 6 months ago

Describe the bug The jakarta.validation.constraints.NotNull validation constrain is heavily used in the Essencium project on method parameter level. This is true for the controller, service and repository layer. When a null value is passed to a method with a @NotNull annotated parameter, no exception is thrown. Instead the code simple continues with the normal execution flow.

By default the @NotNull is only a hint, like the passed parameter should not be null and the implementation of the method should take care of any null values - e.g. with

public void doSomethingNullSave(@NotNull final String token) {
  Objects.requireNonNull(token, "The token should not be null");
}

As written above, in general the @NotNull does nothing if the passed value is null until a validator is executed against the given value. This happens for example if the @NotNull is set on a JPA entity parameter during the JPA commit and flush process. It also happens for controller methods - in these cases the @NotNull is often redundant, since a @RequestBody or @PathParam / @RequestParam can not be null. @Service and @Repository classes do not execute any validation against method parameter. So for all @NotNull annotated parameters a null value can be passed.

To Reproduce Steps to reproduce the behavior: Since the validation of method parameters are part of interaction two components with each other, a simple integration test can demonstrate the behavior:

  1. Add dummy endpoint to a controller:
    
    @GetMapping(value = "/not-null")
    public final void notNull() {
    userService.publicNullMethod(null);
    }

// in AbstractUserController.java public void publicNullMethod(@NotNull final String token) { var userToUpdate = userRepository .findByPasswordResetToken(token) .orElseThrow(() -> new BadCredentialsException("Invalid reset token")); }

// in BaseUserRepository.java Optional findByPasswordResetToken(@NotNull String passwordResetToken);

2. Make endpoint "public" in `WebSecurityConfig`:
```java
private static final RequestMatcher DEFAULT_PUBLIC_URLS =
  new OrRequestMatcher( 
....
    new AntPathRequestMatcher("/v1/not-null/**"),
....);
  1. Execute simple integration test

    @Test
    void testNotNull() throws Exception {
    RequestBuilder requestBuilder =
          MockMvcRequestBuilders.get("/v1/not-null");
    
    mockMvc.perform(requestBuilder).andExpect(status().isNoContent());
    }

Expected behavior As a user of the API I would assume, that any passed null should throw a validation exception. But in the given example above the thrown exception is "jakarta.servlet.ServletException: Request processing failed: org.springframework.dao.IncorrectResultSizeDataAccessException: Query did not return a unique result: 2 results were returned" and is thrown in findByPasswordResetToken execution of the repository proxy. The passed null value to the service is passed to the repository and the SQL query is executed with a select *** where token is null;, although on the service method and the repository method a @NotNull is present.

To enable the validation execution, a @Validated can be added to the class. Since all the @NotNull annotations lead to a false sense of security during development, all usages on parameter level should be checked and if possible and needed:

cholakov11 commented 3 weeks ago

I would suggest implementing a custom annotation with custom validator which throws a custom NullParameterException. However, the topic with domain specific exceptions is discussed in #593. I would suggest first to design the exception handling and then revisit this issue.