Shadi A.

[shmoonwalker]

finished tasks for week 6

[cometbroom]

Submission review (PR#6): Implemented both endpoints and tests. All tests pass. Recommendations: fix potential crash in UserStatsRepository for users with no streams; replace mock-only tests with DB integration tests using the provided test data; provide reason for user 404; handle lyrics client exceptions; verify test data and pom versions. Good job on the requirements!

[cometbroom]

You delivered both required endpoints: GET /users/{id}/stats and GET /tracks/{id}/lyrics. The code is organized by feature package (users and tracks), uses records for response objects, and keeps the lyrics API call behind a separate LyricsClient, which is a good design choice.

The main thing to improve is the testing approach. The tests are good MVC slice tests, but they are not full integration tests since the repositories are mocked. That means the HTTP mapping and JSON responses are tested, but the SQL queries and database integration are not.

Also worth a look is the ApiExceptionHandler.

[cometbroom]

LyricsApiException for non-404 here are not caught by ApiExceptionHandler (only ResponseStatusException is handled). This ends up as a generic 500 INTERNAL_SERVER_ERROR.

You can also handle LyricsApiException in the ApiExceptionHandler and convert it to consistent response for the client.

[cometbroom]

Explicit type parameter <String> is not needed here.

With Optional<T>, T gets inferred by the compiler on return. If your functions' return is String then it gets inferred as Optional<String>.

[cometbroom]

Good use of ResponseStatusException but one important note because this is already a Spring web exception, Spring MVC translates it directly into an HTTP error response. That means your custom ApiExceptionHandler may not always be the code path responsible for the response, even though the endpoint still returns the correct 404. Please see my comment on ApiExceptionHandler.

Another thing to keep in mind: For larger applications: creating specific exception types, such as TrackNotFoundException or LyricsNotFoundException also helps logs and analytics easier to navigate.
That is because you can filter errors by exception type when you investigate production issues. You don't need that for this assignment yet; it is more relevant later when you get into observability and monitoring in Week 13.

[cometbroom]

Awesome job returning an Optional<String> here! Then each service which relies on this client can decide if they need it so much that they throw an error or just continue.

[cometbroom]

This is up to preference but you could use Lombok here to automatically generate the constructor. That'd be using RequiredArgsConstructor

import lombok.RequiredArgsConstructor;
import org.springframework.web.bind.annotation.RestController;

@RestController
@RequiredArgsConstructor
public class TrackLyricsService {
  private final TrackRepository trackRepository;
  private final LyricsApiClient lyricsApiClient;
}

Having the constructor written down is also completely fine. Some people prefer that as the injection mechanism might be clearer.

[cometbroom]

Nice clear flow in this controller. As the application grows, though, this orchestration logic is better placed in a service layer: find the track, fetch the lyrics, decide what happens when either one is missing, and return the final TrackLyrics response.

Then ideally your service would throw a domain-specific exception that would become an HTTP error response in your ExceptionHandler class with @RestControllerAdvice

Keeping controllers thin has two main benefits.

• First, the same use case can be reused from another controller, a scheduled job, a message consumer, or a CLI command without copying controller logic.
• Second, service methods are usually easier to test directly because they do not require setting up the web layer.

[cometbroom]

ResponseStatusException is already a Spring web exception, Spring MVC can translates it directly into an HTTP error response. That means it won't reach this handler.

If you want full control over the error response body, often you'd have to throw your own exception.

[cometbroom]

Very nice null handling here. If a null is passed here error response becomes a generic 500 with no immediately clear reason why.

[cometbroom]

This was not covered in the assignment requirement but for a true integration tests, repository layer shouldn't be mocked either. Only the database itself would then have to be mocked with an h2 database or Testcontainers.
Since the changes for Testcontainers setup won't be fitting in a review comment, I added a branch on my fork for a Testcontainer setup.
Currently there's nothing to test but it does boots up a docker container with mocked testdata which allow data to be retrieved and remove the need for mocking Repository. Please take a look and adapt if needed: https://github.com/HackYourAssignment/c55-backend-week-6/compare/main...cometbroom:example/testcontainers-pgdb-mock?expand=1

[cometbroom]

Same here as TrackRepository mocking in TrackLyricsControllerTests, please read my comment there.