Shadi A.

[shmoonwalker]

finished task for week 4

[cometbroom]

You did a solid job covering the assignment Shadi!

The project is clearly split into controller, service, DTO, model, and exception packages, and the service layer contains most of the business logic instead of putting it in the controller. The API design document is also quite complete.

The core behavior is mostly implemented well. Records get a server-generated traceId, are stored in memory, can be listed, fetched, replaced, deleted, filtered, and summarized. The request DTOs use validation annotations such as @NotBlank and @NotNull.

[cometbroom]

Impressive builder pattern usage Shadi!

Indeed DTOs are very commong candidates for the builder pattern.

Couple things though:

1. When using the builder pattern for initialization, you'd want to stick to using that afterwards. Therefore none of the NoArgsConstructors and AllArgsConstructor are needed. And indeed they're not used.
2. We want DTOs still immutable so we can use the idea behind the builder pattern to our advantage. We remove so @Setter so that an object will be final after .build(). However this is currently not possible. Read my comment on CreateAnalyticsRecordRequest why.

[cometbroom]

For consistency with the response DTOs, consider using the Builder pattern for the request DTOs as well.
Making Lombok builders work seamlessly with Spring Boot’s automatic JSON (de)serialization is currently a bit tricky due to Jackson version differences:

In Spring Boot 3 (Jackson 2), you could simply annotate the class with @builder and @Jacksonized.
Spring Boot 4.0.6 ships with Jackson 3, which is not yet supported by Lombok’s @Jacksonized annotation.

Once Lombok adds support for Jackson 3, the request class can be simplified to:

@Builder
@Jacksonized
public class AnalyticsRecordResponse {

    private String traceId;
    private String eventType;
    private String eventSource;
    private String anonymizedSessionId;
    private LocalDateTime timestamp;
}

For now, you need to explicitly configure Jackson to use the Lombok-generated builder:

import jakarta.validation.constraints.NotBlank;
import jakarta.validation.constraints.NotNull;
import lombok.Builder;
import lombok.Getter;
import tools.jackson.databind.annotation.JsonDeserialize;
import tools.jackson.databind.annotation.JsonPOJOBuilder;

import java.time.LocalDateTime;

@Getter
@Builder
@JsonDeserialize(builder = CreateAnalyticsRecordRequest.CreateAnalyticsRecordRequestBuilder.class)
public class CreateAnalyticsRecordRequest {

    @NotBlank private final String eventType;
    @NotBlank private final String eventSource;
    @NotBlank private final String anonymizedSessionId;
    @NotNull  private final LocalDateTime timestamp;

    @JsonPOJOBuilder(withPrefix = "")
    public static class CreateAnalyticsRecordRequestBuilder {
        // Lombok fills this in. The annotation above tells Jackson 3
        // that builder setters have no "with" prefix.
    }
}

[cometbroom]

Very well done on have an error response DTO.

And indeed ErrorResponse classes are very commonly using the builder pattern.

[cometbroom]

For consistency, avoid returning a raw Map here while the other exception handlers return ErrorResponse. Consider extending ErrorResponse with an optional validation-errors field and returning the same response type from all handlers, for example:
You can add an errors Map type in your error response which returns only 1 error in most cases except when there are more errors:

public class ErrorResponse {
    private int status;
    private String message;
    private String path;
    private Map<String, String> errors;
}

Then in your exception handler wrapping with ResponseEntity for a complete error response:

    @ExceptionHandler(MethodArgumentNotValidException.class)
    public ResponseEntity<ErrorResponse> handleValidationException(
        MethodArgumentNotValidException ex,
        HttpServletRequest request
    ) {
        Map<String, String> errors = new HashMap<>();

        ex.getBindingResult()
            .getFieldErrors()
            .forEach(error -> errors.put(error.getField(), error.getDefaultMessage()));

        ErrorResponse response = ErrorResponse.builder()
            .status(HttpStatus.BAD_REQUEST.value())
            .message("Validation failed")
            .path(request.getRequestURI())
            .errors(errors)
            .build();

        return ResponseEntity.status(HttpStatus.BAD_REQUEST).body(response);
    }

[cometbroom]

Great job on having a specific exception for common cases!

[cometbroom]

Since startTime and endTime are LocalDateTime query parameters, it will help Spring to parse more date formats and returning a clear response if you annotate them with @DateTimeFormat, for example:

@DateTimeFormat(iso = DateTimeFormat.ISO.DATE_TIME) @RequestParam(required = false) LocalDateTime startTime,
@DateTimeFormat(iso = DateTimeFormat.ISO.DATE_TIME) @RequestParam(required = false) LocalDateTime endTime

[cometbroom]

One small improvement to consider: if eventType and eventSource have a fixed set of allowed values, they could be modeled as enums instead of plain Strings.

That would make invalid or inconsistent values harder to introduce. If these values are intentionally flexible, keeping them as strings is fine, but it would be good to document the expected naming convention or validate them with something like @Pattern.

[cometbroom]

Well done on correct positional placement! (expected, actual) 👍

[cometbroom]

Great job on the tests. Only thing I'd add is to reduce code duplication:

        CreateAnalyticsRecordRequest request = new CreateAnalyticsRecordRequest();
        request.setEventType("BUTTON_CLICK");
        request.setEventSource("HOME_PAGE");
        request.setAnonymizedSessionId("session-123");
        request.setTimestamp(LocalDateTime.now());

Is repeated 3 times.

Then elsewhere in the test it seems that a CreateAnalyticsRecordRequest object is with different properties. You can reduce code duplication by having a private method for providing different slices of the object:

    private static CreateAnalyticsRecordRequest getCreateAnalyticsRecordRequest(String anonymizedSessionId, boolean nowLocalTimeStamp) {
        CreateAnalyticsRecordRequest request = new CreateAnalyticsRecordRequest();

        request.setEventType("BUTTON_CLICK");
        request.setEventSource("HOME_PAGE");
        request.setAnonymizedSessionId(anonymizedSessionId);

        if (nowLocalTimeStamp) {
            request.setTimestamp(LocalDateTime.now());
        }
        return request;
    }

[cometbroom]

Since this handler produces the final error response sent to the client, consider returning ResponseEntity<ErrorResponse> instead of only ErrorResponse. That way the actual HTTP status code matches the status in the response body.

@ExceptionHandler(AnalyticsRecordNotFoundException.class)
public ResponseEntity<ErrorResponse> handleAnalyticsRecordNotFoundException(
        AnalyticsRecordNotFoundException ex,
        HttpServletRequest request
) {
    ErrorResponse response = ErrorResponse.builder()
            .status(HttpStatus.NOT_FOUND.value())
            .message(ex.getMessage())
            .path(request.getRequestURI())
            .build();

    return ResponseEntity.status(HttpStatus.NOT_FOUND).body(response);
}