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

polish #1597

Merged
merged 4 commits into from
Dec 25, 2024
Merged

polish #1597

Show file tree
Hide file tree
Changes from all commits
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 @@ -28,6 +28,7 @@ void addRedisKeyValue() throws Exception {
.content(objectMapper.writeValueAsString(addRedisRequest))
.assertThat()
.hasStatus(HttpStatus.CREATED)
.hasContentType(MediaType.APPLICATION_JSON)
.bodyJson()
.convertTo(GenericResponse.class)
.satisfies(response -> assertThat(response.response()).isEqualTo(true));
Expand All @@ -42,6 +43,7 @@ void getFromCache() {
.param("key", "junit")
.assertThat()
.hasStatusOk()
.hasContentType(MediaType.APPLICATION_JSON)
.bodyJson()
.convertTo(GenericResponse.class)
.satisfies(response -> assertThat(response.response()).isEqualTo("JunitValue"));
Expand All @@ -61,6 +63,7 @@ void expireFromCache() {
.param("key", "junit")
.assertThat()
.hasStatusOk()
.hasContentType(MediaType.APPLICATION_JSON)
.bodyJson()
.convertTo(GenericResponse.class)
.satisfies(
Expand Down
6 changes: 3 additions & 3 deletions jpa/boot-read-replica-postgresql/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,9 @@
<relativePath /> <!-- lookup parent from repository -->
</parent>
<groupId>com.example.demo</groupId>
<artifactId>boot-jpa-read-replica-postgresql</artifactId>
<artifactId>boot-read-replica-postgresql</artifactId>
<version>0.0.1-SNAPSHOT</version>
<name>boot-jpa-read-replica-postgresql</name>
<name>boot-read-replica-postgresql</name>
<description>Demo project for Spring Boot Read Replica</description>

<properties>
Expand Down Expand Up @@ -109,7 +109,7 @@
<configuration>
<java>
<googleJavaFormat>
<version>1.24.0</version>
<version>1.25.2</version>
<style>AOSP</style>
</googleJavaFormat>
</java>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,14 @@
import com.example.demo.readreplica.service.ArticleService;
import java.net.URI;
import org.springframework.http.ResponseEntity;
import org.springframework.web.bind.annotation.DeleteMapping;
import org.springframework.web.bind.annotation.GetMapping;
import org.springframework.web.bind.annotation.PathVariable;
import org.springframework.web.bind.annotation.PostMapping;
import org.springframework.web.bind.annotation.RequestBody;
import org.springframework.web.bind.annotation.RequestMapping;
import org.springframework.web.bind.annotation.RestController;
import org.springframework.web.servlet.support.ServletUriComponentsBuilder;

@RestController
@RequestMapping("/articles")
Expand All @@ -22,7 +24,7 @@ class ArticleController {
}

@GetMapping("/{id}")
ResponseEntity<ArticleDTO> findArticleById(@PathVariable Integer id) {
ResponseEntity<ArticleDTO> findArticleById(@PathVariable Long id) {
return this.articleService
.findArticleById(id)
.map(ResponseEntity::ok)
Expand All @@ -32,6 +34,23 @@ ResponseEntity<ArticleDTO> findArticleById(@PathVariable Integer id) {
@PostMapping("/")
ResponseEntity<Object> saveArticle(@RequestBody ArticleDTO articleDTO) {
Long articleId = this.articleService.saveArticle(articleDTO);
return ResponseEntity.created(URI.create("/articles/" + articleId)).build();
URI location =
ServletUriComponentsBuilder.fromCurrentRequest()
.path("{id}")
.buildAndExpand(articleId)
.toUri();
return ResponseEntity.created(location).build();
}

@DeleteMapping("/{id}")
ResponseEntity<Object> deleteArticle(@PathVariable Long id) {
return this.articleService
.findById(id)
.map(
article -> {
articleService.deleteById(article.getId());
return ResponseEntity.accepted().build();
})
.orElseGet(() -> ResponseEntity.notFound().build());
}
}
Original file line number Diff line number Diff line change
@@ -1,10 +1,26 @@
package com.example.demo.readreplica.domain;

import static com.example.demo.readreplica.domain.CommentDTO.convertToComment;

import com.example.demo.readreplica.entities.Article;
import com.example.demo.readreplica.entities.Comment;
import java.time.LocalDateTime;
import java.util.List;

public record ArticleDTO(
String title,
LocalDateTime authored,
LocalDateTime published,
List<CommentDTO> commentDTOs) {}
List<CommentDTO> commentDTOs) {

public Article convertToArticle() {
Article article =
new Article().setAuthored(authored).setTitle(title).setPublished(published);
commentDTOs.forEach(
commentDTO -> {
Comment comment = convertToComment(commentDTO);
article.addComment(comment);
});
return article;
}
}
Original file line number Diff line number Diff line change
@@ -1,3 +1,10 @@
package com.example.demo.readreplica.domain;

public record CommentDTO(String comment) {}
import com.example.demo.readreplica.entities.Comment;

public record CommentDTO(String comment) {

static Comment convertToComment(CommentDTO commentDTO) {
return new Comment().setComment(commentDTO.comment());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -11,5 +11,5 @@ public interface ArticleRepository extends JpaRepository<Article, Long> {

@Transactional(readOnly = true)
@Query("select a from Article a left join fetch a.comments where a.id = :articleId ")
Optional<Article> findByArticleId(@Param("articleId") Integer articleId);
Optional<Article> findByArticleId(@Param("articleId") Long articleId);
}
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,13 @@
import com.example.demo.readreplica.domain.ArticleDTO;
import com.example.demo.readreplica.domain.CommentDTO;
import com.example.demo.readreplica.entities.Article;
import com.example.demo.readreplica.entities.Comment;
import com.example.demo.readreplica.repository.ArticleRepository;
import java.util.List;
import java.util.Optional;
import org.springframework.stereotype.Service;
import org.springframework.transaction.annotation.Transactional;

@Service
@Transactional(readOnly = true)
public class ArticleService {

private final ArticleRepository articleRepository;
Expand All @@ -19,17 +18,26 @@ public class ArticleService {
this.articleRepository = articleRepository;
}

public Optional<ArticleDTO> findArticleById(Integer id) {
public Optional<ArticleDTO> findArticleById(Long id) {
return this.articleRepository.findByArticleId(id).map(this::convertToArticleDTO);
}

@Transactional
public Long saveArticle(ArticleDTO articleDTO) {
Article article = convertToArticle(articleDTO);
Article article = articleDTO.convertToArticle();
Article savedArticle = this.articleRepository.save(article);
return savedArticle.getId();
}

public Optional<Article> findById(Long id) {
return articleRepository.findById(id);
}
Comment on lines +32 to +34
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider returning ArticleDTO instead of Article entity.

Exposing JPA entities directly could lead to lazy loading issues if the entity is accessed outside the transaction boundary. Consider converting to DTO before returning.

-    public Optional<Article> findById(Long id) {
-        return articleRepository.findById(id);
+    public Optional<ArticleDTO> findById(Long id) {
+        return articleRepository.findById(id).map(this::convertToArticleDTO);

Committable suggestion skipped: line range outside the PR's diff.


@Transactional
public void deleteById(Long id) {
articleRepository.deleteById(id);
}

private ArticleDTO convertToArticleDTO(Article articleEntity) {
return new ArticleDTO(
articleEntity.getTitle(),
Expand All @@ -39,24 +47,4 @@ private ArticleDTO convertToArticleDTO(Article articleEntity) {
.map(comment -> new CommentDTO(comment.getComment()))
.toList());
}

private Article convertToArticle(ArticleDTO articleDTO) {
Article article = new Article();
article.setAuthored(articleDTO.authored());
article.setTitle(articleDTO.title());
article.setPublished(articleDTO.published());
convertToComment(articleDTO.commentDTOs()).forEach(article::addComment);
return article;
}

private List<Comment> convertToComment(List<CommentDTO> commentDTOs) {
return commentDTOs.stream()
.map(
commentDTO -> {
Comment comment = new Comment();
comment.setComment(commentDTO.comment());
return comment;
})
.toList();
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,104 @@
package com.example.demo.readreplica.controller;

import static org.assertj.core.api.Assertions.assertThat;

import com.example.demo.readreplica.domain.ArticleDTO;
import com.example.demo.readreplica.domain.CommentDTO;
import com.fasterxml.jackson.core.JsonProcessingException;
import com.fasterxml.jackson.databind.ObjectMapper;
import java.time.LocalDateTime;
import java.util.List;
import java.util.concurrent.atomic.AtomicReference;
import org.junit.jupiter.api.Test;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.boot.test.autoconfigure.web.servlet.AutoConfigureMockMvc;
import org.springframework.boot.test.context.SpringBootTest;
import org.springframework.http.HttpStatus;
import org.springframework.http.MediaType;
import org.springframework.test.web.servlet.assertj.MockMvcTester;

@SpringBootTest
@AutoConfigureMockMvc
class ArticleControllerIntTest {

@Autowired private MockMvcTester mvcTester;

@Autowired private ObjectMapper objectMapper;

@Test
void findArticleById() {

mvcTester
.get()
.uri("/articles/1")
.assertThat()
.hasStatusOk()
.hasContentType(MediaType.APPLICATION_JSON)
.bodyJson()
.convertTo(ArticleDTO.class)
.satisfies(
articleDTO -> {
assertThat(articleDTO.title())
.isNotNull()
.isEqualTo("Waiter! There is a bug in my JSoup!");
assertThat(articleDTO.authored())
.isNotNull()
.isInstanceOf(LocalDateTime.class);
assertThat(articleDTO.published())
.isNotNull()
.isInstanceOf(LocalDateTime.class);
assertThat(articleDTO.commentDTOs())
.isNotNull()
.hasSize(2)
.hasOnlyElementsOfType(CommentDTO.class);
});
}
Comment on lines +28 to +55
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Enhance test coverage with setup and error scenarios.

The test makes assumptions about existing test data and lacks coverage for error scenarios. Consider these improvements:

  1. Add a @BeforeEach method to set up test data consistently
  2. Add tests for error scenarios (e.g., non-existent article)
  3. Move test data to constants or test properties file

Example structure:

private static final String TEST_ARTICLE_TITLE = "Waiter! There is a bug in my JSoup!";

@BeforeEach
void setUp() {
    // Setup test data
}

@Test
void findArticleById_NotFound() {
    mvcTester.get()
            .uri("/articles/999")
            .assertThat()
            .hasStatus(HttpStatus.NOT_FOUND);
}


@Test
void saveArticleRetriveAndDelete() throws JsonProcessingException {
ArticleDTO articleDTO =
new ArticleDTO(
"junitTitle",
LocalDateTime.now().minusDays(1),
LocalDateTime.now(),
List.of(new CommentDTO("junitComment")));
AtomicReference<String> location = new AtomicReference<>();
mvcTester
.post()
.uri("/articles/")
.contentType(MediaType.APPLICATION_JSON)
.content(objectMapper.writeValueAsString(articleDTO))
.assertThat()
.hasStatus(HttpStatus.CREATED)
.matches(
result -> {
location.set(result.getResponse().getHeader("Location"));
assertThat(location.get()).isNotBlank().contains("/articles/");
});

mvcTester
.get()
.uri(location.get())
.assertThat()
.hasStatusOk()
.hasContentType(MediaType.APPLICATION_JSON)
.bodyJson()
.convertTo(ArticleDTO.class)
.satisfies(
response -> {
assertThat(response.title()).isNotNull().isEqualTo("junitTitle");
assertThat(response.authored())
.isNotNull()
.isInstanceOf(LocalDateTime.class);
assertThat(response.published())
.isNotNull()
.isInstanceOf(LocalDateTime.class);
assertThat(response.commentDTOs())
.isNotNull()
.hasSize(1)
.hasOnlyElementsOfType(CommentDTO.class);
});

mvcTester.delete().uri(location.get()).assertThat().hasStatus(HttpStatus.ACCEPTED);
}
Comment on lines +57 to +103
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Split the test method and add validation scenarios.

The current test method violates the Single Responsibility Principle by testing multiple operations. Consider these improvements:

  1. Split into separate test methods for better isolation:

    • testSaveArticle
    • testRetrieveArticle
    • testDeleteArticle
  2. Add validation scenario tests:

    • Invalid article data
    • Missing required fields
    • Invalid date ranges
  3. Fix potential timing issues with LocalDateTime.now()

Example structure:

@Test
void saveArticle_ValidData() {
    ArticleDTO articleDTO = createValidArticleDTO();
    // Test POST only
}

@Test
void saveArticle_InvalidData() {
    ArticleDTO articleDTO = createInvalidArticleDTO();
    mvcTester.post()
            .uri("/articles/")
            .contentType(MediaType.APPLICATION_JSON)
            .content(objectMapper.writeValueAsString(articleDTO))
            .assertThat()
            .hasStatus(HttpStatus.BAD_REQUEST);
}

private ArticleDTO createValidArticleDTO() {
    LocalDateTime now = LocalDateTime.now();
    return new ArticleDTO(
            "junitTitle",
            now.minusDays(1),
            now,
            List.of(new CommentDTO("junitComment")));
}

}
Loading