1
\$\begingroup\$

Background

I was working on a coding challenge as part of an interview process. I had to create a REST API where the user can report sensor data and query derived metrics based on several filter criteria.

Ingest

In the sensor data ingest handler I could simply annotate the expected request body entity with validation rules.

@Data
@Builder
public class SensorIngestRequest {
    final static String DATE_FORMAT = "yyyy-MM-dd'T'HH:mm:ss";

    @NotNull(message = "must be provided in the following format: " + DATE_FORMAT)
    @JsonFormat(shape = JsonFormat.Shape.STRING, pattern = DATE_FORMAT, timezone = "GMT")
    Date observedAt;

    @NotNull(message = "must be provided and it should be a hyphened UUID v4")
    UUID deviceId;

    @NotNull(message = "must be provided")
    Integer measuredValue;

    @NotEmpty(message = "must be provided and should not be empty")
    String measureUnit;
}

then I could use @Valid to enforce validation

@PostMapping("/{sensor-name}")
ResponseEntity<?> ingestSensorData(
        @PathVariable(name="sensor-name") String sensorName,
        @Valid @RequestBody SensorIngestRequest ingestRequest) {

    //Compute
    var serviceModel = ingestService.ingest(sensorName, ingestRequest);

    //Generate Response << not important from the question perspective
    //Calculate surrounding days for observed at
    var calendar = Calendar.getInstance();
    calendar.setTime(serviceModel.getObservedAt());
    calendar.set(Calendar.HOUR_OF_DAY, 0);
    var from = calendar.getTime();
    calendar.add(Calendar.DATE, 1);
    var till = calendar.getTime();

    //Generate links << not important from the question perspective
    var linkSingle = createLinkForGetMetric(sensorName, from, till, Optional.of(ingestRequest.getDeviceId()), LinkRelation.of(RELATION_NAME_FOR_SINGLE_DEVICE_METRIC));
    var linkAll = createLinkForGetMetric(sensorName, from, till, Optional.empty(), LinkRelation.of(RELATION_NAME_FOR_ALL_DEVICES_METRIC));
    var responseModel = EntityModel.of(serviceModel, linkSingle, linkAll);

    return ResponseEntity
            .created(responseModel.getRequiredLink(RELATION_NAME_FOR_SINGLE_DEVICE_METRIC).toUri())
            .body(responseModel);
}

If the validation fails then it throws a MethodArgumentNotValidException against which I can define a ControllerAdvice

@ControllerAdvice
public class ValidationExceptionAdvice {
    final static String ERROR_FORMAT = "'%s' parameter %s";

    @ExceptionHandler(MethodArgumentNotValidException.class)
    public ResponseEntity<Problem> handleMANVE(MethodArgumentNotValidException ex) {
        var errors = ex.getBindingResult().getAllErrors().stream()
                .map(error -> String.format(ERROR_FORMAT,((FieldError) error).getField(), error.getDefaultMessage()))
                .sorted() //Make errors' ordering consistent
                .collect(Collectors.toList());

        return getResponseEntity(errors);
    }

    ResponseEntity<Problem> getResponseEntity(List<String> errors) {
        return ResponseEntity
                .status(HttpStatus.BAD_REQUEST)
                .body(Problem.create()
                        .withTitle("Invalid data has been provided, please correct it")
                        .withDetail(errors.toString()));
    }
}

Query

In the query/search handler I anticipate two dates (a from and a to). These could be used to narrow the date interval against which the metric calculation should be performed. A sample request looks like this

GET /api/v1/sensors/windSpeed/metrics/avg?from=2023-05-22&till=2023-05-23
@GetMapping("/{sensor-name}/metrics/{metric-type}")
ResponseEntity<?> getSensorMetricForADateRangeForADevice(
        @PathVariable(name="sensor-name") String sensorName,
        @PathVariable(name="metric-type") String metricType,
        @RequestParam("from") @Valid @DateTimeFormat(pattern="yyyy-MM-dd") Date from,
        @RequestParam("till") @Valid @DateTimeFormat(pattern="yyyy-MM-dd") Date till,
        @RequestParam(name="device-id",required = false) Optional<UUID> deviceId)  {

    //Extra input data validation
    if (!"avg".equalsIgnoreCase(metricType)) {
        throw createCVE("metric-type", 1, metricType, String.class, "currently supports only 'avg'");
    }

    if (from.compareTo(till) > 0) {
        throw createCVE("till", 3, till, Date.class,"should be greater than from");
    }

    //Generate Response
    //... << omitted for the sake of brevity

    return ResponseEntity.ok().body(EntityModel.of(serviceModel, linkSingle, linkAll));
}

As you can see I had to compare from and to to make sure that from is smaller than to. I have found pretty hard to throw a hand-crafted MethodArgumentNotValidException. So, I've decided to use ConstraintViolationException instead. Well, that is also not an easy peasy task.

<T> ConstraintViolationException createCVE(
        String parameterName,
        int parameterIndex,
        T parameter,
        Class<T> parameterType,
        String violationMessage) {

    //NOTE: here the method name does not matter
    final var propertyPath = PathImpl.createPathFromString("");
    propertyPath.addParameterNode(parameterName, parameterIndex);

    var constraintViolation = ConstraintViolationImpl.forParameterValidation(
            violationMessage,
            Collections.emptyMap(),
            Collections.emptyMap(),
            violationMessage,
            parameterType,
            parameter,
            null,
            parameter,
            propertyPath,
            null,
            null,
            null);
    return new ConstraintViolationException(Set.of(constraintViolation));
}

Quite frankly I don't like approach either. I had to write another ExceptionHandler as well to create a proper Problem response.

@ExceptionHandler(ConstraintViolationException.class)
public ResponseEntity<Problem> handleCVE(ConstraintViolationException ex) {
    var errors = ex.getConstraintViolations().stream()
            .map(error -> String.format(ERROR_FORMAT, error.getPropertyPath(), error.getMessage()))
            .collect(Collectors.toList());

    return getResponseEntity(errors);
}

Question

  • Is there any better way to perform custom cross parameter check for GET request?
\$\endgroup\$
6
  • 2
    \$\begingroup\$ I saw you are using the Date that has been deprecated cause a lot of problems in favour of the LocalDate class , is this required from the interview? Because you are using streams so LocalDate is equally available. \$\endgroup\$ May 27, 2023 at 17:42
  • \$\begingroup\$ @dariosicily Nice catch, thanks. I working on multiple projects with different programming languages (C#, Golang, TypeScript and Java) and sometimes I mix up things. ¯_(ツ)_/¯ \$\endgroup\$ May 30, 2023 at 8:42
  • \$\begingroup\$ Glad to have helped :) , to simplify the validation code you could use the BindingResult class like here, its behaviour is still the same in the rest controller or mvc controller. \$\endgroup\$ May 30, 2023 at 10:19
  • \$\begingroup\$ @dariosicily How does BindingResult help to perform cross parameter check (from points to an earlier date than till)? According to my understanding this collects the validation errors. Or shall I use the addError with a FieldError if my custom validation fails? \$\endgroup\$ May 30, 2023 at 10:49
  • \$\begingroup\$ I suggested BindingResult because you can intercept @NotNull and @DateTimeFormat validation errors and then create a new ResponseEntity(errormessage, HttpStatus.BAD_REQUEST)if BindingResult is not empty.In this case you should create a ResponseEntity for comparing at runtime the two dates with the error after checked BindingResults is empty. Alternatively you can create a new custom annotation for comparing dates like here but it seems me more complicated. \$\endgroup\$ May 30, 2023 at 16:02

2 Answers 2

1
\$\begingroup\$

The first thing I noticed is the use of the deprecated Date class instead of the LocalDate class that has been introduced with Java 8. The code presented and the flow of the program are both correct, but some simplification can be introduced with the explicit use of the BindingResult class that is not directly used. Starting from the signature of the search method described in the question:

@GetMapping("/{sensor-name}/metrics/{metric-type}")
ResponseEntity<?> getSensorMetricForADateRangeForADevice(
        @PathVariable(name="sensor-name") String sensorName,
        @PathVariable(name="metric-type") String metricType,
        @RequestParam("from") @Valid @DateTimeFormat(pattern="yyyy-MM-dd") Date from,
        @RequestParam("till") @Valid @DateTimeFormat(pattern="yyyy-MM-dd") Date till) 

With a new helper class containing both the Date from and Date till fields and with the BindingResult class can be rewritten like below:

@Data
public class LocalDateChecker {
    @NotNull
    @DateTimeFormat(pattern="yyyy-MM-dd")
    private LocalDate from;
    @NotNull
    @DateTimeFormat(pattern="yyyy-MM-dd")
    private LocalDate till;
}

@RestController
public class SensorMetricController {

    @GetMapping("/api/v1/sensors/{sensor-name}/metrics/{metric-type}")
    public ResponseEntity getSensorMetricForADateRangeForADevice(
            @PathVariable(name = "sensor-name") String sensorName,
            @PathVariable(name = "metric-type") String metricType,
            @Valid LocalDateChecker localDateChecker,
            BindingResult bindingResult)
}

The advantage is that the bindingResult contains all the validation errors and relative messages erasing the propagation of the validation code outside of the relative controller. A possible way to rewrite the controller is below:

@RestController
public class SensorMetricController {

    @GetMapping("/api/v1/sensors/{sensor-name}/metrics/{metric-type}")
    public ResponseEntity getSensorMetricForADateRangeForADevice(
            @PathVariable(name = "sensor-name") String sensorName,
            @PathVariable(name = "metric-type") String metricType,
            @Valid LocalDateChecker localDateChecker,
            BindingResult bindingResult) {
        
        //I'm writing the first error, but all errors could be reported
        // in one time
        if (bindingResult.hasErrors()) {
            FieldError fieldError = bindingResult.getFieldErrors().get(0);
            throw new MetricException(fieldError.getField() + " " + fieldError.getDefaultMessage());
        }

        if (!"avg".equalsIgnoreCase(metricType)) {
            throw new MetricException("metric type currently supports only 'avg'");
        }

        if (localDateChecker.getFrom().isAfter(localDateChecker.getTill())) {
           throw new MetricException(("till shoud be greater or equal to from"));
        }
        return new ResponseEntity(List.of(
                sensorName,
                metricType,
                localDateChecker.getFrom(),
                localDateChecker.getTill()
        ), HttpStatus.OK);

    }
}

Because the generic user has to understand just which is the absent or malformed field not generating a correct output the only important thing is the propagation of a new generated MetricException exception containing the error message to the ControllerAdvice that will communicate the bad request error to the user:

public class MetricException extends RuntimeException {

    public MetricException(String message) {
        super(message);
    }

}

@ControllerAdvice
public class SensorMetricAdvice {

    @ResponseBody
    @ExceptionHandler(MetricException.class)
    @ResponseStatus(HttpStatus.BAD_REQUEST)
    String metricExceptionHandler(MetricException ex) {
        return ex.getMessage();
    }
}

Substantially all the original code presented in the question is absolutely correct, but with help of spring classes its size can be reduced, the code I wrote it's based on the validating-form-input (adapted to a rest controller) and Building REST services with Spring.

\$\endgroup\$
2
  • 1
    \$\begingroup\$ Thanks for the detailed review and suggestions. I will try it out tomorrow :) \$\endgroup\$ May 31, 2023 at 18:41
  • 1
    \$\begingroup\$ @PeterCsala Glad to have helped :) \$\endgroup\$ Jun 1, 2023 at 11:59
1
\$\begingroup\$

You could wrap the request parameters in a class and then implement a custom validator that checks the dates.

That way, your controller's code would be reduced to:

@GetMapping("/{sensor-name}/metrics/{metric-type}")
ResponseEntity<?> getSensorMetricForADateRangeForADevice(
    @Valid SensorQuery sensorQuery) {
  ...
}
\$\endgroup\$
2
  • \$\begingroup\$ Hey Miguel thanks for the review. Let me check your suggested links. \$\endgroup\$ May 30, 2023 at 8:42
  • \$\begingroup\$ Could you please show me how would you apply your first suggested link to request parameters that are annotated with @PathVariable? \$\endgroup\$ May 30, 2023 at 9:00

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.