Wednesday, November 1, 2017

Replacing exceptions with error notifications during input validation in Java

In my previous article I wrote about an input validation design which replaces hard-to-maintain-and-test if-else blocks. However, as some readers pointed out, it has a drawback - if the input data has more than one validation errors, then the user will have to submit the request multiple times to find all of them. From a usability perspective this is not a good design.

An alternative to throwing exceptions when we find a validation error is to return a Notification object containing the error(s). This will enable us to run all the validation rules on the user input, and catch all violations at the same time. Martin Fowler wrote an article detailing the approach. I highly recommend you to go ahead and give it a read, if you haven't done so already.

In this article I will refactor my previous implementation to use Error Notification object to validate user inputs.

As a first step, I will create an ErrorNotification object which encapsulates my application errors -
public class ErrorNotification {
  private List<String> errors = new ArrayList<>();

  public void addError(String message) {
    this.errors.add(message);
  }

  public boolean hasError() {
    return !this.errors.isEmpty();
  }

  public String getAllErrors() {
    return this.errors.stream()
        .collect(joining(", "));
  }
}
I will then change the OrderItemValidator interface to return an ErrorNotification object -
public interface OrderItemValidator {
  ErrorNotification validate(OrderItem orderItem);
}
and then change all the implementations to adapt to the new return type as well.

Initially, I will change all the implementations to return an empty error object, so that I can get rid of the compilation errors. For example, I will change the ItemDescriptionValidator in the following way -
class ItemDescriptionValidator implements OrderItemValidator {

  @Override
  public ErrorNotification validate(OrderItem orderItem) {
    ErrorNotification errorNotification = new ErrorNotification();
    Optional.ofNullable(orderItem)
        .map(OrderItem::getDescription)
        .map(String::trim)
        .filter(description -> !description.isEmpty())
        .orElseThrow(() -> new IllegalArgumentException("Item description should be provided"));
    return errorNotification;
  }
}
After fixing the compilation errors, I will now start replacing the exceptions with notification messages in each validator. To do this, I will first modify the related tests to reflect my intent, and then modify the validators to pass the tests.

Let's start with the ItemDescriptionValidatorTest class -
public class ItemDescriptionValidatorTest {

  @Test
  public void validate_descriptionIsNull_invalid() {
    ItemDescriptionValidator validator = new ItemDescriptionValidator();

    ErrorNotification errorNotification = validator.validate(new OrderItem());

    assertThat(errorNotification.getAllErrors()).isEqualTo("Item description should be provided");
  }

  @Test
  public void validate_descriptionIsBlank_invalid() {
    OrderItem orderItem = new OrderItem();
    orderItem.setDescription("     ");
    ItemDescriptionValidator validator = new ItemDescriptionValidator();

    ErrorNotification errorNotification = validator.validate(new OrderItem());

    assertThat(errorNotification.getAllErrors()).isEqualTo("Item description should be provided");
  }

  @Test
  public void validate_descriptionGiven_valid() {
    OrderItem orderItem = new OrderItem();
    orderItem.setDescription("dummy description");
    ItemDescriptionValidator validator = new ItemDescriptionValidator();

    ErrorNotification errorNotification = validator.validate(orderItem);

    assertThat(errorNotification.getAllErrors()).isEmpty();
  }
}
When I run these tests, only one of them passes, and two of them fail, which is expected. I will now modify the validator code to pass the tests -
class ItemDescriptionValidator implements OrderItemValidator {
  static final String MISSING_ITEM_DESCRIPTION = "Item description should be provided";

  @Override
  public ErrorNotification validate(OrderItem orderItem) {
    ErrorNotification errorNotification = new ErrorNotification();
    Optional.ofNullable(orderItem)
        .map(OrderItem::getDescription)
        .map(String::trim)
        .filter(description -> !description.isEmpty())
        .ifPresentOrElse(
            description -> {},
            () -> errorNotification.addError(MISSING_ITEM_DESCRIPTION)
        );
    return errorNotification;
  }
}
I am a bit uncomfortable with the use of the ifPresentOrElse method above. The main reason I am using it here is because Optionals don't have something like an ifNotPresent method, which would have allowed me to take an action only when the value is not present (request to my readers - if you know a better way to do this, please comment in!).

After this refactoring, all tests in the ItemValidatorTest class pass with flying color. Great!

Let's refactor the tests in the MenuValidatorTest class now -
public class MenuValidatorTest {

  @Test
  public void validate_menuIdInvalid_invalid() {
    OrderItem orderItem = new OrderItem();
    String menuId = "some menu id";
    orderItem.setMenuId(menuId);
    MenuRepository menuRepository = mock(MenuRepository.class);
    when(menuRepository.menuExists(any())).thenReturn(false);
    MenuValidator validator = new MenuValidator(menuRepository);

    ErrorNotification errorNotification = validator.validate(orderItem);

    assertThat(errorNotification.getAllErrors())
        .isEqualTo(String.format(MenuValidator.INVALID_MENU_ERROR_FORMAT, menuId));
  }

  @Test
  public void validate_menuIdNull_invalid() {
    MenuRepository menuRepository = mock(MenuRepository.class);
    when(menuRepository.menuExists(any())).thenReturn(true);
    MenuValidator validator = new MenuValidator(menuRepository);

    ErrorNotification errorNotification = validator.validate(new OrderItem());

    assertThat(errorNotification.getAllErrors())
        .isEqualTo(MenuValidator.MISSING_MENU_ERROR);
  }

  @Test
  public void validate_menuIdIsBlank_invalid() {
    OrderItem orderItem = new OrderItem();
    orderItem.setMenuId("       \t");
    MenuRepository menuRepository = mock(MenuRepository.class);
    when(menuRepository.menuExists(any())).thenReturn(true);
    MenuValidator validator = new MenuValidator(menuRepository);

    ErrorNotification errorNotification = validator.validate(orderItem);

    assertThat(errorNotification.getAllErrors())
        .isEqualTo(MenuValidator.MISSING_MENU_ERROR);
  }

  @Test
  public void validate_menuIdValid_validated() {
    OrderItem orderItem = new OrderItem();
    String menuId = "some menu id";
    orderItem.setMenuId(menuId);
    MenuRepository menuRepository = mock(MenuRepository.class);
    when(menuRepository.menuExists(menuId)).thenReturn(true);
    MenuValidator validator = new MenuValidator(menuRepository);

    ErrorNotification errorNotification = validator.validate(orderItem);

    assertThat(errorNotification.getAllErrors()).isEmpty();
  }
}

and then then MenuValidator class -
@RequiredArgsConstructor
class MenuValidator implements OrderItemValidator {
  private final MenuRepository menuRepository;

  static final String MISSING_MENU_ERROR = "A menu item must be specified.";
  static final String INVALID_MENU_ERROR_FORMAT = "Given menu [%s] does not exist.";

  @Override
  public ErrorNotification validate(OrderItem orderItem) {
    ErrorNotification errorNotification = new ErrorNotification();
    Optional.ofNullable(orderItem.getMenuId())
        .map(String::trim)
        .filter(menuId -> !menuId.isEmpty())
        .ifPresentOrElse(
            validateMenuExists(errorNotification),
            () -> errorNotification.addError(MISSING_MENU_ERROR)
        );
    return errorNotification;
  }

  private Consumer<String> validateMenuExists(ErrorNotification errorNotification) {
    return menuId -> {
      if (!menuRepository.menuExists(menuId)) {
        errorNotification.addError(String.format(INVALID_MENU_ERROR_FORMAT, menuId));
      }
    };
  }
}
and so on.

After modifying each of the individual validators, I will now modify the Composite to collect all errors for a single order item -
@RequiredArgsConstructor
class OrderItemValidatorComposite implements OrderItemValidator {
  private final List<OrderItemValidator> validators;

  @Override
  public ErrorNotification validate(OrderItem orderItem) {
    ErrorNotification errorNotification = new ErrorNotification();
    validators.stream()
        .map(validator -> validator.validate(orderItem))
        .forEach(errorNotification::addAll);
    return errorNotification;
  }
}
In order to do this, I have added a new method in the ErrorNotification class, called addAll, which basically copies all errors from another ErrorNotification object.

Finally, I will now modify the service method to collect all error messages for all the order items of an order -
@Service
@Slf4j
@RequiredArgsConstructor
class OrderService {
  private final OrderItemValidator validator;

  void createOrder(OrderDTO orderDTO) {
    ErrorNotification errorNotification = new ErrorNotification();
    orderDTO.getOrderItems()
        .stream()
        .map(validator::validate)
        .forEach(errorNotification::addAll);
    if (errorNotification.hasError()) {
      throw new IllegalArgumentException(errorNotification.getAllErrors());
    }

    log.info("Order {} saved", orderDTO);
  }
}

Making this change causes one of the tests in OrderServiceIT to fail, as it was specifically looking for an exception with cause set to NumberFormatException when the price is invalid. After our refactoring, we can safely remove this check as it is no longer relevant.

The full source code for this article has been pushed to GitHub (specific commit URL is here).