Friday, May 12, 2017

Clean Code from the trenches - Validation

Let's directly start with an example. Consider a simple web service which allows clients to place order to a shop. A very simplified version of the order controller could look something like below -

@RestController
@RequestMapping(value = "/",
    consumes = MediaType.APPLICATION_JSON_VALUE,
    produces = MediaType.APPLICATION_JSON_VALUE)
public class OrderController {
  private final OrderService orderService;

  public OrderController(OrderService orderService) {
    this.orderService = orderService;
  }

  @PostMapping
  public void doSomething(@Valid @RequestBody OrderDTO order) {
    orderService.createOrder(order);
  }
}

And the corresponding DTO class -

@Getter
@Setter
@ToString
public class OrderDTO {

  @NotNull
  private String customerId;

  @NotNull
  @Size(min = 1)
  private List<OrderItem> orderItems;

  @Getter
  @Setter
  @ToString
  public static class OrderItem {
    private String menuId;
    private String description;
    private String price;
    private Integer quantity;
  }
}


The most common approach for creating an order from this DTO is to pass it to a service, validate it as necessary, and then persist it in the database -


@Service
@Slf4j
class OrderService {
  private final MenuRepository menuRepository;

  OrderService(MenuRepository menuRepository) {
    this.menuRepository = menuRepository;
  }

  void createOrder(OrderDTO orderDTO) {
    orderDTO.getOrderItems()
        .forEach(this::validate);

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

  private void validate(OrderItem orderItem) {
    String menuId = orderItem.getMenuId();
    if (menuId == null || menuId.trim().isEmpty()) {
      throw new IllegalArgumentException("A menu item must be specified.");
    }
    if (!menuRepository.menuExists(menuId.trim())) {
      throw new IllegalArgumentException("Given menu " + menuId + " does not exist.");
    }

    String description = orderItem.getDescription();
    if (description == null || description.trim().isEmpty()) {
      throw new IllegalArgumentException("Item description should be provided");
    }

    String price = orderItem.getPrice();
    if (price == null || price.trim().isEmpty()) {
      throw new IllegalArgumentException("Price cannot be empty.");
    }
    try {
      new BigDecimal(price);
    } catch (NumberFormatException ex) {
      throw new IllegalArgumentException("Given price is not in valid format", ex);
    }
    if (orderItem.getQuantity() == null) {
      throw new IllegalArgumentException("Quantity must be given");
    }
    if (orderItem.getQuantity() <= 0) {
      throw new IllegalArgumentException("Given quantity "
          + orderItem.getQuantity()
          + " is not valid.");
    }
  }
}


The validate method is not well written. It is very hard to test. Introducing new validation rule in the future is also hard, and so is removing/modifying any of the existing ones. From my experience I have seen that most people write a few generic assertions for this type of validation check, typically in an integration test class, touching only one or two (or more, but not all) of the validation rules. As a result, refactoring in the future can only be done in Edit and Pray mode.

We can improve the code structure if we use Polymorphism to replace these conditionals. Let's create a common super type for representing a single validation rule -

public interface OrderItemValidator {
  void validate(OrderItem orderItem);
}


Next step is to create validation rule implementations which will focus on separate validation areas for the DTO. Let's start with the menu validator -

public class MenuValidator implements OrderItemValidator {
  private final MenuRepository menuRepository;

  public MenuValidator(MenuRepository menuRepository) {
    this.menuRepository = menuRepository;
  }

  @Override
  public void validate(OrderItem orderItem) {
    String menuId = Optional.ofNullable(orderItem.getMenuId())
        .map(String::trim)
        .filter(id -> !id.isEmpty())
        .orElseThrow(() -> new IllegalArgumentException("A menu item must be specified."));

    if (!menuRepository.menuExists(menuId)) {
      throw new IllegalArgumentException("Given menu [" + menuId + "] does not exist.");
    }
  }
}


Then the item description validator -

public class ItemDescriptionValidator implements OrderItemValidator {

  @Override
  public void validate(OrderItem orderItem) {
    Optional.ofNullable(orderItem)
        .map(OrderItem::getDescription)
        .map(String::trim)
        .filter(description -> !description.isEmpty())
        .orElseThrow(() -> new IllegalArgumentException("Item description should be provided"));
  }
}


Price validator -

public class PriceValidator implements OrderItemValidator {

  @Override
  public void validate(OrderItem orderItem) {
    String price = Optional.ofNullable(orderItem)
        .map(OrderItem::getPrice)
        .map(String::trim)
        .filter(itemPrice -> !itemPrice.isEmpty())
        .orElseThrow(() -> new IllegalArgumentException("Price cannot be empty."));

    try {
      new BigDecimal(price);
    } catch (NumberFormatException ex) {
      throw new IllegalArgumentException("Given price [" + price + "] is not in valid format", ex);
    }
  }
}


And finally, the quantity validator -

public class QuantityValidator implements OrderItemValidator {

  @Override
  public void validate(OrderItem orderItem) {
    Integer quantity = Optional.ofNullable(orderItem)
        .map(OrderItem::getQuantity)
        .orElseThrow(() -> new IllegalArgumentException("Quantity must be given"));
    if (quantity <= 0) {
      throw new IllegalArgumentException("Given quantity " + quantity + " is not valid.");
    }
  }
}


Each of these validator implementations can now be easily tested, independently from each other. Reasoning about each one of them also becomes easier. and so are future addition/modification/removal.

Now the wiring part. How can we integrate these validators with the order service?

One way would be to directly create a list in the OrderService constructor, and populate it with the validators. Or we could use Spring to inject a List into the OrderService -

@Service
@Slf4j
class OrderService {
  private final List<OrderItemValidator> validators;

  OrderService(List<OrderItemValidator> validators) {
    this.validators = validators;
  }

  void createOrder(OrderDTO orderDTO) {
    orderDTO.getOrderItems()
        .forEach(this::validate);

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

  private void validate(OrderItem orderItem) {
    validators.forEach(validator -> validator.validate(orderItem));
  }
}


In order for this to work we will have to declare each of the validator implementations as a Spring Bean.

We could improve our abstraction even further. The OrderService is now accepting a List of the validators. However, we can change it to be only aware of the OrderItemValidator type, and nothing else. This gives us the flexibility of injecting either a single validator or any composition of validators in the future.

So now our goal is to change the order service to treat a composition of order item validators in the same way as a single validator. There is a well-known design pattern called Composite which lets us do exactly that.

Let's create a new implementation of the validator interface, which will be the composite -

class OrderItemValidatorComposite implements OrderItemValidator {
  private final List<OrderItemValidator> validators;

  OrderItemValidatorComposite(List<OrderItemValidator> validators) {
    this.validators = validators;
  }

  @Override
  public void validate(OrderItem orderItem) {
    validators.forEach(validator -> validator.validate(orderItem));
  }
}


We then create a new Spring configuration class, which will instantiate and initialize this composite, and then expose it as a bean -

@Configuration
class ValidatorConfiguration {

  @Bean
  OrderItemValidator orderItemValidator(MenuRepository menuRepository) {
    return new OrderItemValidatorComposite(Arrays.asList(
        new MenuValidator(menuRepository),
        new ItemDescriptionValidator(),
        new PriceValidator(),
        new QuantityValidator()
    ));
  }
}


We then change the OrderService class in the following way -

@Service
@Slf4j
class OrderService {
  private final OrderItemValidator validator;

  OrderService(OrderItemValidator orderItemValidator) {
    this.validator = orderItemValidator;
  }

  void createOrder(OrderDTO orderDTO) {
    orderDTO.getOrderItems()
        .forEach(validator::validate);

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


And we are done!

The benefits of this approach are many. The whole validation logic has completely been abstracted away from the ordering service. Testing is easier. Future maintenance is easier. Clients only know about one validator type, and nothing else.

However, all of the above come with some problems too. Sometimes people are not comfortable with this design. They may feel like this is just too much abstraction, or that they will not be needing this much flexibility or testability for future maintenance. I'd suggest to adopt this approach based on the team culture. After all, there is no single right way of doing things in Software Development.

Note that for the sake of this article I have taken some short cuts here as well. These includes throwing a generic IllegalArgumentException when validation fails. You'd probably want a more specific/custom exception in a production-grade application to identify between different scenarios. The decimal parsing is also done naively, you might want to fix on a specific format, and then use DecimalFormat to parse it.

The full code has been uploaded to Github.

If you find any typo/other errors please feel free to comment in!

Sunday, March 5, 2017

Dealing with Java's LocalDateTime in JPA

A few days ago I ran into a problem while dealing with a LocalDateTime attribute in JPA. In this blog post I will try to create a sample problem to explain the issue, along with the solution that I used.

Consider the following entity, which models an Employee of a certain company -

@Entity
@Getter
@Setter
public class Employee {

  @Id
  @GeneratedValue
  private Long id;
  private String name;
  private String department;
  private LocalDateTime joiningDate;
}

I was using Spring Data JPA, so created the following repository -
@Repository
public interface EmployeeRepository 
    extends JpaRepository<Employee, Long> {

}

I wanted to find all employees who have joined the company at a particular date. To do that I extended my repository from JpaSpecificationExecutor -
@Repository
public interface EmployeeRepository 
    extends JpaRepository<Employee, Long>,
    JpaSpecificationExecutor<Employee> {

}

and wrote a query like below -
@SpringBootTest
@RunWith(SpringRunner.class)
@Transactional
public class EmployeeRepositoryIT {

  @Autowired
  private EmployeeRepository employeeRepository;

  @Test
  public void findingEmployees_joiningDateIsZeroHour_found() {
    DateTimeFormatter formatter = DateTimeFormatter.ofPattern("yyyy-MM-dd HH:mm:ss");
    LocalDateTime joiningDate = LocalDateTime.parse("2014-04-01 00:00:00", formatter);

    Employee employee = new Employee();
    employee.setName("Test Employee");
    employee.setDepartment("Test Department");
    employee.setJoiningDate(joiningDate);
    employeeRepository.save(employee);

    // Query to find employees
    List<Employee> employees = employeeRepository.findAll((root, query, cb) ->
        cb.and(
            cb.greaterThanOrEqualTo(root.get(Employee_.joiningDate), joiningDate),
            cb.lessThan(root.get(Employee_.joiningDate), joiningDate.plusDays(1)))
    );

    assertThat(employees).hasSize(1);
  }
}

The above test passed without any problem. However, the following test failed (which was supposed to pass) -
@Test
public void findingEmployees_joiningDateIsNotZeroHour_found() {
  DateTimeFormatter formatter = DateTimeFormatter.ofPattern("yyyy-MM-dd HH:mm:ss");
  LocalDateTime joiningDate = LocalDateTime.parse("2014-04-01 08:00:00", formatter);
  LocalDateTime zeroHour = LocalDateTime.parse("2014-04-01 00:00:00", formatter);

  Employee employee = new Employee();
  employee.setName("Test Employee");
  employee.setDepartment("Test Department");
  employee.setJoiningDate(joiningDate);
  employeeRepository.save(employee);

  List<Employee> employees = employeeRepository.findAll((root, query, cb) ->
      cb.and(
          cb.greaterThanOrEqualTo(root.get(Employee_.joiningDate), zeroHour),
          cb.lessThan(root.get(Employee_.joiningDate), zeroHour.plusDays(1))
      )
  );

  assertThat(employees).hasSize(1);
}

The only thing that is different from the previous test is that in the previous test I used the zero hour as the joining date, and here I used 8 AM.

At first it seemed weird to me. The tests seemed to pass whenever the joining date of an employee was set to a zero hour of a day, but failed whenever it was set to any other time.

In order to investigate the problem I turned on the hibernate logging to see the actual query and the values being sent to the database, and noticed something like this in the log -
2017-03-05 22:26:20.804 DEBUG 8098 --- [           main] org.hibernate.SQL:
    select
        employee0_.id as id1_0_,
        employee0_.department as departme2_0_,
        employee0_.joining_date as joining_3_0_,
        employee0_.name as name4_0_
    from
        employee employee0_
    where
        employee0_.joining_date>=?
        and employee0_.joining_dateHibernate:
    select
        employee0_.id as id1_0_,
        employee0_.department as departme2_0_,
        employee0_.joining_date as joining_3_0_,
        employee0_.name as name4_0_
    from
        employee employee0_
    where
        employee0_.joining_date>=?
        and employee0_.joining_date2017-03-05 22:26:20.806 TRACE 8098 --- [           main] o.h.type.descriptor.sql.BasicBinder      : binding parameter [1] as [VARBINARY] - [2014-04-01T00:00]
2017-03-05 22:26:20.807 TRACE 8098 --- [           main] o.h.type.descriptor.sql.BasicBinder      : binding parameter [2] as [VARBINARY] - [2014-04-02T00:00]
It was evident that JPA was NOT treating the joiningDate attribute as a date or time, but as a VARBINARY type. This is why the comparison to an actual date was failing.

In my opinion this is not a very good design. Rather than throwing something like UnsupportedAttributeException or whatever, it was silently trying to convert the value to something else, and thus failing the comparison at random (well, not exactly random). This type of bugs are hard to find in the application unless you have a strong suit of automated tests, which was fortunately my case.

Back to the problem now. The reason JPA was failing to convert LocalDateTime appropriately was very simple. The last version of the JPA specification (which is 2.1) was released before Java 8, and as a result it cannot handle the new Date and Time API.

To solve the problem, I created a custom converter implementation which converts the LocalDateTime to java.sql.Timestamp before saving it to the database, and vice versa. That solved the problem -
@Converter(autoApply = true)
public class LocalDateTimeConverter implements AttributeConverter<LocalDateTime, Timestamp> {

  @Override
  public Timestamp convertToDatabaseColumn(LocalDateTime localDateTime) {
    return Optional.ofNullable(localDateTime)
        .map(Timestamp::valueOf)
        .orElse(null);
  }

  @Override
  public LocalDateTime convertToEntityAttribute(Timestamp timestamp) {
    return Optional.ofNullable(timestamp)
        .map(Timestamp::toLocalDateTime)
        .orElse(null);
  }
}

The above converter will be automatically applied whenever I try to save a LocalDateTime attribute. I could also explicitly mark the attributes that I wanted to convert explicitly, using the javax.persistence.Convert annotation -
@Convert(converter = LocalDateTimeConverter.class)
private LocalDateTime joiningDate;

The full code is available at Github.


Tuesday, February 14, 2017

Subtyping in Java Generics

Consider the following block of Java code, which we all know as valid -

We can do this because Long is a subtype of Number. However, the following will fail to compile -

Allowing such assignments would have easily let programmers break the type safety guarantee provided by the Generics. One would then be able to do -

From the above example, it is clear that Subtyping in Java Generics works differently than the usual class based Subtyping. A list of numbers cannot point directly to a list of longs even though Long is a subtype of Number. In order to get around this restriction, we will have to use an upper bounded wildcard -

which will also allow us to refer to a list of floats as well.

A List<? extends Number>  is then treated as something like a super type of both List<Long> and List<Number>. In fact, as long as a type X is a subtype of Number, List<? extends Number> will be able to refer to List<X> without any compilation errors.

Using an upper bounded wildcard makes our code much more flexible to future changes. Consider the following method which tries to find the sum of the longs -

If we change the method signature to use upper bounded wildcard, then we can also pass a list of integers to it -

Without the wildcard we would have to first convert the integers to long, and then pass it to the method.

An upper bounded wildcard brings its own set of restrictions though. We cannot add any new value to the list we are pointing to (except null). Allowing such assignments would have again let us break the type safety (see the first example). Also, retrieved values can only be treated as of type upper bound. Using an upper bounded wildcard thus results in a read-only list from which we can only read, but cannot store any meaningful values into it.

If we want the opposite, that is, a write-only list, then we would use a lower bounded wildcard -

The above list will allow as to store any type which is a subtype of Number into it. However, we can only retrieve items from it as Object. Allowing the retrieval of any other type would have resulted in a ClassCastException at runtime as we would have no way of knowing exactly which subtype of Number was stored in the list.

Reference resolution also works the opposite way of the upper bound. A List<? super Number> can reference any list of type X, where X is a super type of Number.

To summarize, then, a List<? extends X> means -
  1. We can use this reference to point to a list of type Y, where Y is a subtype of X.
  2. We cannot store anything into the list other than null.
  3. We can only refer to the retrieved items from this list as X.
whereas a List<? super X> means -
  1. We can use this reference to point to a list of type Y, where Y is a super type of X.
  2. We can store any value into it which is a subtype of X.
  3. We can only refer to the retrieved items from this list as Object.
When I am  trying to read/store values into these lists, I find it useful to read List<? extends X> as -
1. A list of items from where we get values of type X (when operating on it)
2. A variable which can point to a list of subtype of X (during reference assignment)
Similarly, I read List<? super X> as -
1. A list of item where we might add values of type X (when operating on it)
2. A variable which can point to a list of supertype of X (during reference assignment)
This is the reason the upper bounded wildcard references are sometimes called as Producers, since we can only read from them in order to do something effective. Similarly, the lower bounded wildcards are called Consumers. People sometimes use a small mnemonic for it, PECS, which basically translates to -
Producer Extends, Consumer Super
This same producer-consumer concept has been heavily used by the Java 8 API, as can be seen from these default method implementations of Function.