R. Yusup

[Yusuprozimemet]

No description provided.

[cometbroom]

Great job Yusup!

You delivered both required endpoints: GET /users/{id}/stats and GET /tracks/{id}/lyrics. The API is properly layered with controller, service, repository/client layers that are separated by concern. The lyrics API call is wrapped in its own LyricsApiClient, which is a very good design choice for this assignment and makes testing much easier.

Please do take a look at the comments I left, especially the comment on having @ResponseStatus on an exception. Implementing the fixes of course are optional :)

[cometbroom]

I really like the short-circuit conditions here, they were immediately readable. Also well done on not having an unnecessary else block.

However, since you are returning an Optional, have you considered using the Optional API?

return Optional.ofNullable(response).map(LyricsResponse::lyrics).filter(s -> !s.isBlank());

For Spring projects you can also use hasText() method in org.springframework.util.StringUtils to make it still simpler:

return Optional.ofNullable(response).map(LyricsResponse::lyrics).filter(StringUtils::hasText);

What you currently have though is perfectly fine and some prefer it for readability.

[cometbroom]

You could add the shape in Javadoc comments that start with /**. In IntelliJ if you write that and press enter, you'll get a Javadoc block ready to write in.

Then in the Javadoc segment you can either use <pre> or {@snippet lang="JSON": for Java 18+ to render code:
Java 18+:

/**
 * Represents the response structure from the lyrics.ovh API.
 * Shape of the lyrics.ovh response:
 * {@snippet lang="JSON":
 * {
 *     "lyrics": "..."
 * }
 * }
 */

All Java versions:

/**
 * Represents the response structure from the lyrics.ovh API.
 * Shape of the lyrics.ovh response:
 * <pre>
 * {@code
 *     "lyrics": "..."
 * }
 * </pre>
 */

An advantage of this is that if you hover over LyricsResponse elsewhere in the code, intellisense will show you these comments.

[cometbroom]

Very nice usage of @Value!

[cometbroom]

Incredibly thin controller, well-done!

[cometbroom]

Nicely done on building the message with artistName and trackTitle rather than taking the whole message!

If you'd like to do this without concatenation:

super("No lyrics found for \"%s\" by %s.".formatted(trackTitle, artistName));

Another possible improvement is to take the original exception too and passing it as a cause:

public LyricsNotFoundException(String trackTitle, String artistName, Throwable cause) {
  super("No lyrics found for \"%s\" by %s.".formatted(trackTitle, artistName), cause);
}

This way you won't lose the stacktrace. However, it's good to have both constructors as sometimes a stacktrace wouldn't help much when you're sure an exception had to do with bad user input:

public LyricsNotFoundException(String trackTitle, String artistName, Throwable cause) {
    super("No lyrics found for \"%s\" by %s.".formatted(trackTitle, artistName), cause);
}

public LyricsNotFoundException(String trackTitle, String artistName) {
    this(trackTitle, artistName, null);
}

[cometbroom]

I would recommend avoiding @ResponseStatus directly on the exception class here.

Annotating an exception with @ResponseStatus tightly couples that exception to a specific HTTP response. That can be convenient for small examples, but in a real API it is usually better to handle exceptions centrally, for example with @ControllerAdvice and @ExceptionHandler.

There are two main reasons for that:

1. Logging and investigation
   Before an exception becomes an HTTP response, we often want a central place to log it with useful context. This is important when investigating production issues, for example when a customer reports a failed request.

2. Consistent API error responses
   If this exception is handled by Spring’s default error handling, the response format may differ from the custom error format used for other exceptions. APIs should usually return errors in one consistent structure so clients can depend on it.

Spring’s documentation also warns about this annotation:

Warning: when using this annotation on an exception class, or when setting the reason attribute of this annotation, the HttpServletResponse.sendError method will be used.

With HttpServletResponse.sendError, the response is considered complete and should not be written to any further. Furthermore, the Servlet container will typically write an HTML error page therefore making the use of a reason unsuitable for REST APIs. For such cases it is preferable to use a org.springframework.http.ResponseEntity as a return type and avoid the use of @ResponseStatus altogether.

[cometbroom]

Since you're already using Lombok (In the DTOs, @Builder etc...), you can use Lombok here too to generate the constructor:

import lombok.RequiredArgsConstructor;
import net.hackyourfuture.backend.week6.postify.client.LyricsApiClient;
import net.hackyourfuture.backend.week6.postify.repository.TrackRepository;
import org.springframework.stereotype.Service;

@Service
@RequiredArgsConstructor
public class TrackLyricsService {
  private final TrackRepository trackRepository;
  private final LyricsApiClient lyricsApiClient;
  
  // ... getLyrics() method and rest of the class
}

[cometbroom]

Consider using jdbcTemplate.queryForObject here instead of query(...).stream().findFirst().

This method is expected to return at most one track because track_id should be unique, so using queryForObject makes that expectation explicit in the JDBC layer. It will throw EmptyResultDataAccessException when no row is found, which you can map to Optional.empty(), and it will fail if multiple rows are returned, which is useful because that would point to a data integrity problem sooner than later.

Example:

public Optional<TrackLyrics> findTrackWithArtist(Long trackId) {
  try {
    String sql = """
        SELECT tracks.track_id, tracks.track_title, artists.artist_name
        FROM tracks
        JOIN albums  ON albums.album_id   = tracks.album_id
        JOIN artists ON artists.artist_id = albums.artist_id
        WHERE tracks.track_id = ?
        """;
    TrackLyrics trackLyrics = jdbcTemplate.queryForObject(sql, (rs, rowNum) -> TrackLyrics.builder()
            .trackId(rs.getLong("track_id"))
            .trackTitle(rs.getString("track_title"))
            .artistName(rs.getString("artist_name"))
            .build(),
        trackId);
    return Optional.of(trackLyrics);
  } catch (EmptyResultDataAccessException e) {
    return Optional.empty();
  }
}

I would avoid adding LIMIT 1 here. If a primary-key lookup ever returns multiple rows, that is a data integrity issue we probably want to notice rather than silently hide.

[cometbroom]

Same comment here as in TrackRepository. Please consider using jdbcTemplate.queryForObject here instead of query(...).stream().findFirst().

[cometbroom]

Very nice mocking here!

Also good job moving the external API call into LyricsApiClient. It keeps this test independent from the real API and makes the controller/service behavior much easier to verify.