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 2 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,6 +4,7 @@
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;
Expand Down Expand Up @@ -34,4 +35,16 @@ ResponseEntity<Object> saveArticle(@RequestBody ArticleDTO articleDTO) {
Long articleId = this.articleService.saveArticle(articleDTO);
return ResponseEntity.created(URI.create("/articles/" + articleId)).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 @@ -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 @@ -25,11 +24,20 @@ public Optional<ArticleDTO> findArticleById(Integer id) {

@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 saveArticle() 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);
}
}
Loading