Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

import org.springframework.context.annotation.Bean;
import org.springframework.context.annotation.Configuration;
import org.springframework.web.client.RestTemplate;
import org.springframework.web.servlet.config.annotation.CorsRegistry;
import org.springframework.web.servlet.config.annotation.WebMvcConfigurer;

Expand Down Expand Up @@ -31,4 +32,9 @@ public void addCorsMappings(CorsRegistry registry) {
}
};
}

@Bean
public RestTemplate restTemplate() {
return new RestTemplate();
}
Comment on lines +36 to +39

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Creating a RestTemplate with new RestTemplate() is not recommended as it comes with no timeouts configured. This can lead to your application threads being blocked indefinitely if the remote service is unresponsive, potentially causing resource exhaustion.

It's a best practice to configure connection and read timeouts. The recommended way to create RestTemplate beans in Spring Boot is by using RestTemplateBuilder, which allows for easy configuration.

}
Original file line number Diff line number Diff line change
@@ -1,44 +1,42 @@
package com.yen.SpotifyPlayList.controller;

import com.yen.SpotifyPlayList.model.dto.GetRecommendationsDto;
import com.yen.SpotifyPlayList.service.RecommendationsService;
import com.yen.SpotifyPlayList.service.CustomSpotifyRecommendationService;
import lombok.extern.slf4j.Slf4j;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.http.HttpStatus;
import org.springframework.http.ResponseEntity;
import org.springframework.web.bind.annotation.*;
import se.michaelthelin.spotify.model_objects.specification.Recommendations;

@Slf4j
@RestController
@RequestMapping("/recommend")
public class RecommendationsController {

@Autowired
private RecommendationsService recommendationsService;
private CustomSpotifyRecommendationService recommendationsService;

@PostMapping("/")
public ResponseEntity getRecommendation(@RequestBody GetRecommendationsDto getRecommendationsDto) {
public ResponseEntity<?> getRecommendation(@RequestBody GetRecommendationsDto getRecommendationsDto) {
try {
log.info("(getRecommendation) getRecommendationsDto = " + getRecommendationsDto.toString());

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

For logging, it's better to use parameterized messages instead of string concatenation. This improves performance because the string is only formatted if the log level is enabled, avoiding unnecessary toString() calls.

Suggested change
log.info("(getRecommendation) getRecommendationsDto = " + getRecommendationsDto.toString());
log.info("(getRecommendation) getRecommendationsDto = {}", getRecommendationsDto);

Recommendations recommendations = recommendationsService.getRecommendation(getRecommendationsDto);
return ResponseEntity.status(HttpStatus.OK).body(recommendations);
ResponseEntity<String> recommendations = recommendationsService.getRecommendations(getRecommendationsDto);
return ResponseEntity.status(recommendations.getStatusCode()).body(recommendations.getBody());
Comment on lines +24 to +25

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The service method getRecommendations already returns a ResponseEntity<String>. You can return this directly from the controller method, which simplifies the code and avoids creating a new ResponseEntity instance unnecessarily. The return type ResponseEntity<?> is compatible.

Suggested change
ResponseEntity<String> recommendations = recommendationsService.getRecommendations(getRecommendationsDto);
return ResponseEntity.status(recommendations.getStatusCode()).body(recommendations.getBody());
return recommendationsService.getRecommendations(getRecommendationsDto);

} catch (Exception e) {
log.error("getRecommendation error : " + e);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

When logging exceptions, it's crucial to include the full stack trace. Logging just e.getMessage() can hide the root cause and make debugging significantly harder. Pass the exception object itself to the logger.

Suggested change
log.error("getRecommendation error : " + e);
log.error("getRecommendation error", e);

return ResponseEntity.status(HttpStatus.BAD_REQUEST).body(e.getMessage());
}
}

// TODO: Implement custom recommendation logic for playlist-based recommendations
@GetMapping("/playlist/{playListId}")
public ResponseEntity getRecommendationWithPlayList(@PathVariable("playListId") String playListId) {
public ResponseEntity<?> getRecommendationWithPlayList(@PathVariable("playListId") String playListId) {
try {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

This try-catch block is unnecessary. The code within the try block is unlikely to throw an exception that needs to be handled here. The catch block is effectively dead code. Removing the try-catch will make the code cleaner and simpler.

log.info("(getRecommendationWithPlayList) playListId = {}", playListId);

log.info("(getRecommendationWithPlayList) playListId = " + playListId);
Recommendations recommendations = recommendationsService.getRecommendationWithPlayList(playListId);
return ResponseEntity.status(HttpStatus.OK).body(recommendations);
return ResponseEntity.status(HttpStatus.NOT_IMPLEMENTED).body("Feature not yet implemented with custom service");
} catch (Exception e) {
log.error("getRecommendationWithPlayList error : " + e);
return ResponseEntity.status(HttpStatus.BAD_REQUEST).body(e.getMessage());
}
}

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
package com.yen.SpotifyPlayList.service;

import com.yen.SpotifyPlayList.model.dto.GetRecommendationsDto;
import lombok.extern.slf4j.Slf4j;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.beans.factory.annotation.Value;
import org.springframework.http.*;
import org.springframework.stereotype.Service;
import org.springframework.web.client.RestTemplate;
import org.springframework.web.util.UriComponentsBuilder;

@Service
@Slf4j
public class CustomSpotifyRecommendationService {

@Value("${spotify.api.base-url:https://api.spotify.com/v1}")
private String spotifyApiBaseUrl;

@Autowired
private AuthService authService;

@Autowired
private RestTemplate restTemplate;

public ResponseEntity<String> getRecommendations(GetRecommendationsDto request) {
try {
String url = buildRecommendationUrl(request);
HttpHeaders headers = new HttpHeaders();
headers.setBearerAuth(authService.getAccessToken());
headers.setContentType(MediaType.APPLICATION_JSON);

HttpEntity<?> entity = new HttpEntity<>(headers);

log.info("Making recommendation request to URL: {}", url);
ResponseEntity<String> response = restTemplate.exchange(
url,
HttpMethod.GET,
entity,
String.class
);

log.info("Received recommendation response: {}", response.getStatusCode());
return response;

} catch (Exception e) {
log.error("Error getting recommendations: {}", e.getMessage());

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

When logging exceptions, it's crucial to include the full stack trace for effective debugging. Logging only the message via e.getMessage() loses valuable context about where and why the error occurred. Please pass the full exception object to the logger.

Suggested change
log.error("Error getting recommendations: {}", e.getMessage());
log.error("Error getting recommendations", e);

throw new RuntimeException("Failed to get recommendations", e);
}
}

private String buildRecommendationUrl(GetRecommendationsDto request) {
UriComponentsBuilder builder = UriComponentsBuilder
.fromHttpUrl(spotifyApiBaseUrl + "/recommendations");

// Add seed parameters
if (request.getSeedArtistId() != null) {
builder.queryParam("seed_artists", request.getSeedArtistId());
}
if (request.getSeedTrack() != null) {
builder.queryParam("seed_tracks", request.getSeedTrack());
}
if (request.getSeedGenres() != null) {
builder.queryParam("seed_genres", request.getSeedGenres());
}

// Add other parameters
builder.queryParam("limit", request.getAmount());
builder.queryParam("market", request.getMarket());
builder.queryParam("min_popularity", request.getMinPopularity());
builder.queryParam("max_popularity", request.getMaxPopularity());
builder.queryParam("target_popularity", request.getTargetPopularity());
Comment on lines +188 to +192

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

These query parameters are being added to the URL unconditionally. If properties like amount, market, or the popularity fields in the DTO can be null (which they should be if they are optional), this could result in sending parameters with null values, or default primitive values (e.g., 0 for an int) which might be invalid for the Spotify API.

You should add these parameters only if they have a non-null value, similar to how you handle the seed parameters.

if (request.getAmount() != null) {
    builder.queryParam("limit", request.getAmount());
}
if (request.getMarket() != null) {
    builder.queryParam("market", request.getMarket());
}
if (request.getMinPopularity() != null) {
    builder.queryParam("min_popularity", request.getMinPopularity());
}
if (request.getMaxPopularity() != null) {
    builder.queryParam("max_popularity", request.getMaxPopularity());
}
if (request.getTargetPopularity() != null) {
    builder.queryParam("target_popularity", request.getTargetPopularity());
}


return builder.build().encode().toUriString();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -7,4 +7,5 @@ spotify.redirect.url=http://localhost:8080/playlist
#spotify.redirectURL=http://<ec2_url>:8080/playlist
spotify.authorize.scope=playlist-modify-public,playlist-modify-private,user-read-private,user-read-email

spotify.userId=62kytpy7jswykfjtnjn9zv3ou
spotify.userId=62kytpy7jswykfjtnjn9zv3ou
spotify.api.base-url=https://api.spotify.com/v1