-
Notifications
You must be signed in to change notification settings - Fork 1
Refactor : 단일 게시글 조회 시 스크랩 및 좋아요 확인 가능한 코드로 수정 #87
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
Changes from all commits
1c0f669
b34a91d
cd7fb8b
f8ba6ca
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,10 +1,11 @@ | ||
| package naughty.tuzamate.domain.post.service.query; | ||
|
|
||
| import naughty.tuzamate.auth.principal.PrincipalDetails; | ||
| import naughty.tuzamate.domain.post.dto.PostResDTO; | ||
| import naughty.tuzamate.domain.post.enums.BoardType; | ||
|
|
||
| public interface PostQueryService { | ||
|
|
||
| PostResDTO.PostPreviewDTO getPost(Long postId); | ||
| PostResDTO.PostDTO getPost(Long postId, PrincipalDetails principalDetails); | ||
| PostResDTO.PostPreviewListDTO getPostList(BoardType boardType, Long cursor, int size); | ||
| } | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,11 +1,16 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| package naughty.tuzamate.domain.post.service.query; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import lombok.RequiredArgsConstructor; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import naughty.tuzamate.auth.principal.PrincipalDetails; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import naughty.tuzamate.domain.post.converter.PostConverter; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import naughty.tuzamate.domain.post.dto.PostResDTO; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import naughty.tuzamate.domain.post.entity.Post; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import naughty.tuzamate.domain.post.enums.BoardType; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import naughty.tuzamate.domain.post.repository.PostRepository; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import naughty.tuzamate.domain.postLike.repository.PostLikeRepository; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import naughty.tuzamate.domain.postScrap.repository.PostScrapRepository; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import naughty.tuzamate.domain.user.entity.User; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import naughty.tuzamate.domain.user.repository.UserRepository; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import naughty.tuzamate.global.error.GeneralErrorCode; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import naughty.tuzamate.global.error.exception.CustomException; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import org.springframework.data.domain.PageRequest; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -14,24 +19,30 @@ | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import org.springframework.stereotype.Service; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import org.springframework.transaction.annotation.Transactional; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import java.security.Principal; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import java.util.List; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| @Service | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| @RequiredArgsConstructor | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| public class PostQueryServiceImpl implements PostQueryService { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| private final PostRepository postRepository; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| private final PostLikeRepository postLikeRepository; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| private final PostScrapRepository postScrapRepository; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| @Override | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| public PostResDTO.PostPreviewDTO getPost(Long postId) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| public PostResDTO.PostDTO getPost(Long postId, PrincipalDetails principalDetails) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Post post = postRepository.findById(postId) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| .orElseThrow(() -> new CustomException(GeneralErrorCode.NOT_FOUND_404)); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (!post.isRead()) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| post.setIsRead(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return PostConverter.toPostPreviewDTO(post); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| boolean liked = postLikeRepository.existsByPostIdAndUserId(postId, principalDetails.getId()); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| boolean scraped = postScrapRepository.existsByPostIdAndUserId(postId, principalDetails.getId()); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return PostConverter.toPostDTO(post, liked, scraped); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+34
to
46
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Verification agent 🧩 Analysis chainHandle null PrincipalDetails to avoid NPEs and decide on transactional semantics for isRead
Suggested changes: @@
- public PostResDTO.PostDTO getPost(Long postId, PrincipalDetails principalDetails) {
+ @Transactional
+ public PostResDTO.PostDTO getPost(Long postId, PrincipalDetails principalDetails) {
@@
- if (!post.isRead()) {
- post.setIsRead();
- }
+ if (!post.isRead()) {
+ post.setIsRead(); // persists due to @Transactional
+ }
@@
- boolean liked = postLikeRepository.existsByPostIdAndUserId(postId, principalDetails.getId());
- boolean scraped = postScrapRepository.existsByPostIdAndUserId(postId, principalDetails.getId());
+ Long userId = (principalDetails != null) ? principalDetails.getId() : null;
+ boolean liked = (userId != null) && postLikeRepository.existsByPostIdAndUserId(postId, userId);
+ boolean scraped = (userId != null) && postScrapRepository.existsByPostIdAndUserId(postId, userId);Verification/questions:
🏁 Script executed: #!/bin/bash
# 1. Check if getPost is already annotated with @Transactional
rg -n "@Transactional" -C2 src/main/java/naughty/tuzamate/domain/post/service/query/PostQueryServiceImpl.java
# 2. Locate all usages of getPost to find its controller and mapping
rg -nP --type java "getPost\s*\(" -C3
# 3. Inspect Spring Security configuration for anonymous access rules
rg -nP --type java "authorizeHttpRequests|authorizeRequests|permitAll|authenticated" -C5 src/main/javaLength of output: 10085 🏁 Script executed: #!/bin/bash
# Locate the definition of allowUrls to see if GET /boards/**/posts/** is publicly accessible
rg -n "allowUrls" -C5 src/main/java/naughty/tuzamate/global/config/SecurityConfig.javaLength of output: 1118 Add @transactional to getPost and rely on enforced authentication for PrincipalDetails
Suggested diff (and import): --- a/src/main/java/naughty/tuzamate/domain/post/service/query/PostQueryServiceImpl.java
+++ b/src/main/java/naughty/tuzamate/domain/post/service/query/PostQueryServiceImpl.java
@@
+import org.springframework.transaction.annotation.Transactional;
@@
- public PostResDTO.PostDTO getPost(Long postId, PrincipalDetails principalDetails) {
+ @Transactional
+ public PostResDTO.PostDTO getPost(Long postId, PrincipalDetails principalDetails) {
@@
if (!post.isRead()) {
post.setIsRead();
}Also confirm whether mutating the read flag belongs in a GET endpoint; if not, consider moving that logic to a dedicated command/API. 📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| @Override | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -20,32 +20,34 @@ | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| @Configuration | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| public class FireBaseConfig { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // String 대신 Resource 타입으로 주입받음 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| @Value("${firebase.service-account.path}") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| private Resource serviceAccountResource; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| private String serviceAccountPath; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| @Bean | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| public FirebaseApp firebaseApp() { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // 주입받은 Resource에서 바로 InputStream을 얻음 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| InputStream serviceAccount = serviceAccountResource.getInputStream(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| try (InputStream serviceAccount = getServiceAccountStream()) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| FirebaseOptions options = FirebaseOptions.builder() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| .setCredentials(GoogleCredentials.fromStream(serviceAccount)) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| .build(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // 앱이 이미 초기화되었는지 확인 (중복 초기화 방지) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (FirebaseApp.getApps().isEmpty()) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| log.info("Successfully initialized firebase app"); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return FirebaseApp.initializeApp(options); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } else { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return FirebaseApp.getInstance(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| log.info(" Successfully initialized firebase app"); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return FirebaseApp.initializeApp(options); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } catch (IOException exception) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| log.error("Fail to initialize firebase app: {}", exception.getMessage(), exception); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // 초기화 실패 시 null 대신 예외를 던져서 애플리케이션이 문제를 인지하게 하는 것이 더 좋습니다. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| throw new RuntimeException("Failed to initialize Firebase app.", exception); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| log.error(" Fail to initialize firebase app: {}", exception.getMessage(), exception); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return null; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+28
to
+41
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Critical: Possible duplicate app initialization and null-returning @bean.
Reintroduce the existence check and throw a runtime exception on failure instead of returning null. Apply this diff: - try (InputStream serviceAccount = getServiceAccountStream()) {
-
- FirebaseOptions options = FirebaseOptions.builder()
- .setCredentials(GoogleCredentials.fromStream(serviceAccount))
- .build();
-
- log.info(" Successfully initialized firebase app");
- return FirebaseApp.initializeApp(options);
-
- } catch (IOException exception) {
- log.error(" Fail to initialize firebase app: {}", exception.getMessage(), exception);
- return null;
- }
+ try (InputStream serviceAccount = getServiceAccountStream()) {
+ FirebaseOptions options = FirebaseOptions.builder()
+ .setCredentials(GoogleCredentials.fromStream(serviceAccount))
+ .build();
+
+ FirebaseApp app;
+ if (FirebaseApp.getApps().isEmpty()) {
+ app = FirebaseApp.initializeApp(options);
+ log.info("Successfully initialized Firebase app");
+ } else {
+ app = FirebaseApp.getInstance();
+ log.info("Reusing existing Firebase app");
+ }
+ return app;
+ } catch (IOException exception) {
+ log.error("Fail to initialize Firebase app: {}", exception.getMessage(), exception);
+ throw new IllegalStateException("Failed to initialize Firebase app", exception);
+ }📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| private InputStream getServiceAccountStream() throws IOException { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // 절대경로면 FileInputStream, 아니면 classpath | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (serviceAccountPath.startsWith("/") || serviceAccountPath.contains(":")) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| log.info("Using absolute path for Firebase service account: {}", serviceAccountPath); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return new FileInputStream(serviceAccountPath); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } else { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| log.info("Using classpath resource for Firebase service account: {}", serviceAccountPath); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return new ClassPathResource(serviceAccountPath).getInputStream(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Verification agent
🧩 Analysis chain
Decouple query interface from security type and plan for null principals
Binding the query interface to PrincipalDetails tightens coupling and makes anonymous access error-prone. Prefer passing a nullable userId (or Optional) so the query layer doesn’t depend on auth internals. Also ensure null is handled gracefully for public reads.
Apply this minimal refactor at the interface level:
To verify call sites were all updated after the signature change, run:
🏁 Script executed:
Length of output: 8156
Decouple PostQueryService from PrincipalDetails
Bind the query layer only to a simple, nullable userId rather than the auth principal. This avoids leaking security internals into your service API and makes anonymous reads explicit.
Please update the following:
• PostQueryService.java
– Remove the
PrincipalDetailsimport– Change
to
• PostQueryServiceImpl.java
– Update the method signature to
getPost(Long postId, Long userId)– Replace all uses of
principalDetailswithuserId(e.g.principalDetails.getMember().getId())– Ensure you handle
userId == nullgracefully for public reads• PostController.java
– In the
getPosthandler, pass the ID (or null) instead of the principal:🤖 Prompt for AI Agents