Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

init structure, add liquibase #9

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Conversation

teract10s
Copy link

No description provided.

Copy link

@NazarSmith228 NazarSmith228 left a comment

Choose a reason for hiding this comment

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

Please fix the CI build

Copy link

@NazarSmith228 NazarSmith228 left a comment

Choose a reason for hiding this comment

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

Good job! Let's improve your solution

pom.xml Outdated
<dependency>
<groupId>org.liquibase</groupId>
<artifactId>liquibase-core</artifactId>
<version>${liquibase.version}</version>

Choose a reason for hiding this comment

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

there is no need to specify the version - it is taken from spring-boot-starter-parent

@Component
@RequiredArgsConstructor
public class RickAndMortyApiClient {
private static final String BASE_CHARACTER_URL = "https://rickandmortyapi.com/api/character";

Choose a reason for hiding this comment

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

this property should be stored in application properties and referenced via @Value


@GetMapping("/search")
public List<PersonageResponseDto> getPersonageByName(
Pageable pageable, @RequestParam String name

Choose a reason for hiding this comment

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

you may make use of @PageableDefault here in case client did not provide pagination parameters

import lombok.Data;

@Data
public class ExternalResponseDto {

Choose a reason for hiding this comment

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

why not record?

import lombok.Data;

@Data
public class Info {

Choose a reason for hiding this comment

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

why not record?


public List<Personage> getAllPersonages() {
List<Result> resultList = new ArrayList<>();
ExternalResponseDto externalResponseDto = getExternalResponseDto(BASE_CHARACTER_URL);

Choose a reason for hiding this comment

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

there is no need to pass the url as a parameter - you can simply access it inside of that method

request, HttpResponse.BodyHandlers.ofString());
return objectMapper.readValue(response.body(), ExternalResponseDto.class);
} catch (IOException | InterruptedException e) {
throw new RuntimeException("Can't send request to url: " + url);

Choose a reason for hiding this comment

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

too generic :)

consider creating your custom exception (P.S. don't forget to handle that)

@RestController
@RequiredArgsConstructor
@RequestMapping("/rick-and-morty")
public class RickAndMortyController {

Choose a reason for hiding this comment

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

I don't see any Swagger documentation :)

Choose a reason for hiding this comment

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

Let's add Tag annotation


@Component
@RequiredArgsConstructor
public class DbLoader {

Choose a reason for hiding this comment

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

Suggested change
public class DbLoader {
public class PersonageDatabasePopulator {

Comment on lines 16 to 18
if (personageRepository.count() == 0) {
personageRepository.saveAll(rickAndMortyApiClient.getAllPersonages());
}

Choose a reason for hiding this comment

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

just wondering - do you need to keep only up-to-date personages?

if yes - then you might need to clear your db first and only then load the latest info from API:

if (personageRepository.count() != 0) {
  personageRepository.deleteAll();
}
personageRepository.saveAll(rickAndMortyApiClient.getAllPersonages());

Copy link
Author

Choose a reason for hiding this comment

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

I did it because in our task description was it: All data from the public API should be fetched once, and only once, when the Application context is created. But I uncorrected understood it, i thought that i had to do it only once

Choose a reason for hiding this comment

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

@teract10s no, you're right - you have to do that API call only once

I'm asking about DB state. imagine that you ran you application 2 times:

  1. DB is empty - call the API to save the personages
  2. DB is not empty - call the API to save the personages - a lot of duplicates as a result

Copy link

@NazarSmith228 NazarSmith228 left a comment

Choose a reason for hiding this comment

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

Good job! Just a couple of remarks left

import org.springframework.web.bind.annotation.ExceptionHandler;
import org.springframework.web.servlet.mvc.method.annotation.ResponseEntityExceptionHandler;

public class CustomGlobalExceptionHandler extends ResponseEntityExceptionHandler {

Choose a reason for hiding this comment

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

don't you need @ControllerAdvice?

import org.springframework.web.servlet.mvc.method.annotation.ResponseEntityExceptionHandler;

public class CustomGlobalExceptionHandler extends ResponseEntityExceptionHandler {
@ExceptionHandler(SendingRequestException.class)

Choose a reason for hiding this comment

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

where this exception is thrown? I didn't find that place

Copy link
Author

Choose a reason for hiding this comment

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

Ops. I forgot to delete this after reformat RickAndMortyClient


@Component
@RequiredArgsConstructor
public class RickAndMortyApiClient {

Choose a reason for hiding this comment

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

minor

may be simplified to RickAndMortyClient

Comment on lines 8 to 10
public class ThirdPartyApiConfiguration {
@Bean
public RestTemplate getRestTemplate() {

Choose a reason for hiding this comment

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

Suggested change
public class ThirdPartyApiConfiguration {
@Bean
public RestTemplate getRestTemplate() {
public class ApiClientConfiguration {
@Bean
public RestTemplate restTemplate() {

Copy link

@Elena-Bruyako Elena-Bruyako left a comment

Choose a reason for hiding this comment

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

Good job! Left some comments.

@RestController
@RequiredArgsConstructor
@RequestMapping("/rick-and-morty")
public class RickAndMortyController {

Choose a reason for hiding this comment

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

Let's add Tag annotation

import org.springframework.data.domain.Pageable;

public interface PersonageService {

Choose a reason for hiding this comment

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

remove empty line


@ControllerAdvice
public class CustomGlobalExceptionHandler extends ResponseEntityExceptionHandler {
@ExceptionHandler(Exception.class)

Choose a reason for hiding this comment

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

?

Suggested change
@ExceptionHandler(Exception.class)
@ExceptionHandler(EntityNotFoundException.class)

public class CustomGlobalExceptionHandler extends ResponseEntityExceptionHandler {
@ExceptionHandler(Exception.class)
public ResponseEntity<Object> handleEntityNotFound(Exception ex) {
return new ResponseEntity<>(ex.getMessage(), HttpStatus.SERVICE_UNAVAILABLE);

Choose a reason for hiding this comment

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

Suggested change
return new ResponseEntity<>(ex.getMessage(), HttpStatus.SERVICE_UNAVAILABLE);
return new ResponseEntity<>(ex.getMessage(), HttpStatus.NOT_FOUND);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants