Feature/10 이슈 넘버 #10 에 대한 게시판 기능 구현하였습니다#22
Conversation
- POST 검색: 특정 글을 제목으로 찾을 수 있도록 검색창을 만듭니다. - POST 목록 조회: 어떤 POST가 작성되었는지 페이지를 넘기며 목록을 조회할 수 있도록 합니다. - POST 조회: 작성된 POST를 조회할 수 있도록 합니다. - POST 수정: 작성자가 작성된 POST를 수정할 수 있도록 합니다. - POST 작성: POST를 작성할 수 있도록 합니다. - POST 삭제: 작성자가 POST 을 삭제할 수 있도록 합니다. - ADMIN 기능: 관리자가 다른 작성자의 POST를 수정하고 삭제할 수 있도록 합니다. +α 작성 시간, 수정 시간 기록 기능 구현하였습니다 +α 잘못된 게시글 접근에 대한 예외처리 기능 구현하였습니다
Duskafka
left a comment
There was a problem hiding this comment.
리뷰
전체적으로 노력한 부분들이 많이 보이지만 미숙한 부분들도 많이 보입니다. 줄바꿈 및 메소드의 역할과 책임에 대한 것을 더 생각하며 코딩하면 더 좋은 결과가 나올 겁니다.
| } | ||
|
|
||
| @PutMapping("/{postId}") | ||
| @PreAuthorize("hasRole('USER') or hasRole('ADMIN')") |
There was a problem hiding this comment.
이 부분에 or을 사용하는 게 아니라 hasAnyRole을 사용해보면 어떨까요
| Pageable pageable = PageRequest.of(page, size); // 페이지 정보 설정 | ||
| List<Post> posts = (search == null) ? postService.getAllPosts(pageable).getContent() : | ||
| postService.searchPosts(search, pageable).getContent(); // 검색어가 없으면 전체 게시글 조회, 있으면 검색된 게시글 조회 | ||
| return posts.stream().map(Post::toResponse).collect(Collectors.toList()); // 게시글을 PostResponse로 변환하여 반환 | ||
| } |
There was a problem hiding this comment.
지금 보면 컨트롤러 계층까지 엔티티가 노출되었습니다. 이는 엔티티의 변경이 서비스 계층을 넘어 컨트롤러 계층까지 영향을 줄 수 있다는 의미입니다.
서비스 계층에서 DTO로 컨트롤러 계층에 반환해주는 것은 어떨까요.
| private boolean isAdmin(String studentId) { | ||
| return postService.isAdmin(studentId); | ||
| } |
There was a problem hiding this comment.
이런 단순 서비스 호출 메소드는 따로 분리할 필요가 있을까요?
There was a problem hiding this comment.
단일 책임 원칙에 따라서 해당 메소드는 삭제 처리하였습니다
| public static final String ROLE_USER = "ROLE_USER"; | ||
| public static final String ROLE_ADMIN = "ROLE_ADMIN"; |
There was a problem hiding this comment.
유지보수를 더 용이하게 하기 위해서 입니다. 이후에 새로운 권한 추가시에 상수만 추가하면 되기 때문입니다.
There was a problem hiding this comment.
그러면 위 클래스는 두 개의 역할을 가지므로 SCP 원칙에 위배됩니다.
| public String getStudentId() { | ||
| return this.student != null ? this.student.getLoginId() : null; | ||
| } |
There was a problem hiding this comment.
이런 null 체크는 엔티티 내부가 아닌 서비스 계층에서 하는 게 맞다고 생각합니다.
위와 같이 연관관계에 student가 제대로 들어가지 않아 발생하는 문제 또는 student가 조회되지 않는 경우는 비즈니스 로직에 문제가 있을 확률이 큽니다.
| } | ||
|
|
||
| public PostResponse updatePost(Long postId, PostUpdateRequest request, String studentId) { | ||
| Post post = postRepository.findById(postId).orElseThrow(() -> new PostNotFoundException("게시글을 찾을 수 없습니다")); |
| if (!post.getStudent().getLoginId().equals(studentId) && !isAdmin(studentId)) { | ||
| throw new UnauthorizedAccessException(); | ||
| } |
There was a problem hiding this comment.
이 조건문 대로라면 로그인아이디가 같지 않고 어드민이 아니어야하는데 조건문이 이상합니다. 확인해주세요.
| } | ||
|
|
||
| public void deletePost(Long postId, String studentId) { | ||
| Post post = postRepository.findById(postId).orElseThrow(() -> new PostNotFoundException("게시글을 찾을 수 없습니다")); |
| if (!post.getStudent().getLoginId().equals(studentId) && !isAdmin(studentId)) { | ||
| throw new UnauthorizedAccessException(); | ||
| } |
There was a problem hiding this comment.
여기서도 마찬가지로 어드민이 아니고 로그인 아이디가 같지 않아야한다는 조건이 있는데 조건문이 이상합니다.
There was a problem hiding this comment.
글 게시자(학생 아이디)나 어드민이 아니라면 (수정, 삭제의)권한없음의 예외처리를 하도록 만든 조건문입니다.
| public boolean isAdmin(String studentId) { | ||
| Student student = studentRepository.findByLoginId(studentId) | ||
| .orElseThrow(() -> new NotFoundStudentException("학생을 찾을 수 없습니다.")); | ||
|
|
||
| return student.getAuthorities().stream() | ||
| .anyMatch(authority -> authority.getAuthorityName().equals("ROLE_ADMIN")); | ||
| } |
There was a problem hiding this comment.
이 메소드가 사용되는 곳을 보니 이미 메소드 내에서 Student를 조회했습니다. 그런데 이렇게 한 번 더 조회할 필요가 있을까요? 물론 1차 캐시에 의해 조회가 되지는 않겠지만 차라리 Student를 매개변수로 받아서 검사해보는 것은 어떨까요.
아니면 Student 객체 내부에 ROLE_ADMIN 권한을 가지고 있는지 검사하는 메소드를 추가하는 것이 더 좋을 것 같습니다.
|
오류가 났다고 말한 부분이 있는데 POST URL을 어떻게 보냈는지도 알려줘야 오류를 잡을 수 있습니다. |
다만 맨 마지막 사항에 대한 Student 객체 내부에 ROLE_ADMIN 권한을 가지고 있는지 검사하는 메소드 추가에 대한 사항에서 문제가 발생했습니다 PostApiController의 54번째 줄부터 참고 부탁드립니다
| } | ||
|
|
||
| @GetMapping | ||
| public Page<PostResponse> getPostList(@RequestParam(required = false) String search, |
There was a problem hiding this comment.
search가 어떤 정보를 담고 있는지 매개변수명만 보고 알 수 없습니다.
더 정확한 변수명이 좋을 것 같습니다.
| public static Authority createRole(String roleName) { | ||
| return new Authority(roleName); | ||
| } |
There was a problem hiding this comment.
이러면 유효하지 않은 Authority까지 생길 수 있는 위험이 있습니다. Enum 클래스로 권한을 관리하고 검사하는 것을 추천합니다.
| public static Post createPost(String title, String content, String description, Student student) { | ||
| Post post = new Post(); | ||
| post.title = title; | ||
| post.content = content; | ||
| post.description = description; | ||
| post.student = student; | ||
| return post; | ||
| } |
There was a problem hiding this comment.
생성자를 사용해서 리턴하는 방식을 사용하는 것은 어떨까요.
| public void assignRole(String role) { | ||
| Authority authority = new Authority(role); // Authority 객체를 엔티티 내부에서 생성 | ||
| this.addAuthority(authority); // 권한을 추가 | ||
| } |
There was a problem hiding this comment.
여전히 Authority 엔티티를 Student 내부에서 생성하네요.
이러면 도메인 주도 설계 원칙에서 벗어납니다. 그리고 Authority의 변경 파급력이 Student까지 미치게 됩니다.
There was a problem hiding this comment.
관련하여 Role을 Authority로 변환하는 책임을 Authority 객체 내부에서 처리하도록 변경하였습니다
| public void addAuthority(Authority authority) { | ||
| this.authorities.add(authority); | ||
| } |
| if ("admin".equals(student.getLoginId())) { | ||
| student.addAuthority(new Authority("ROLE_ADMIN")); | ||
| } else { | ||
| student.addAuthority(new Authority("ROLE_USER")); | ||
| } |
There was a problem hiding this comment.
이런 코딩은 회원가입 시마다 if문을 통과해야 하는 오버헤드가 발생합니다. 불필요합니다.
There was a problem hiding this comment.
확인하였습니다 삭제하겠습니다
| public boolean isAdmin(Student student) { | ||
| return student.hasRoleAdmin(); | ||
| } |
There was a problem hiding this comment.
admin인지 검사하는 것을 따로 메소드로 분리할 필요가 있을까요?
너무 세세하게 분리된 메소드는 오히려 코드의 가독성을 낮춥니다.
There was a problem hiding this comment.
코드 전체의 짜임을 위해서 메소드를 일부 수정하였습니다. 필요할거 같아 조금 수정한 뒤 유지하였습니다. 실력이 부족한터라 이부분에 있어서는 조언 부탁드립니다!
| if ("admin".equals(loginId)) { | ||
| authority = authorityRepository.findByAuthorityName("ROLE_ADMIN") | ||
| .orElseThrow(() -> new RuntimeException("관리자 권한이 존재하지 않습니다.")); | ||
| } else { | ||
| authority = authorityRepository.findByAuthorityName("ROLE_USER") | ||
| .orElseThrow(() -> new RuntimeException("사용자 권한이 존재하지 않습니다.")); | ||
| } |
There was a problem hiding this comment.
loginId가 admin이라면 권한을 부여하는 로직을 계속 사용하는데 이는 올바르지 못한 회원가입 방식입니다.
모든 유저가 회원가입을 할 때마다 이러한 로직을 통과하면 오버헤드가 발생합니다.
단일책임 원칙, 도메인 주도 설계 위반 사항들을 수정하였습니다
단일책임 원칙, 도메인 주도 설계 위반 사항들을 수정하였습니다
2차 수정사항에 대한 수정 완료되었습니다. 수정사항 확인 부탁드립니다
| Pageable pageable = PageRequest.of(page, size); // 페이지 정보 설정 | ||
|
|
||
| Page<Post> postsPage = (search == null) | ||
| Page<PostResponse> postResponsePagePage = (search == null) |
There was a problem hiding this comment.
postResponsePagePage는 변수명으로 올바르지 않은 것 같습니다.
There was a problem hiding this comment.
변수명 postPage으로 변경하였습니다.
| public boolean isAdmin(String studentId) { | ||
| Student student = studentRepository.findByLoginId(studentId) | ||
| .orElseThrow(() -> new NotFoundStudentException("학생을 찾을 수 없습니다.")); | ||
|
|
||
| return !student.hasRoleAdmin(); |
There was a problem hiding this comment.
admin인 것을 알기 위해서 꼭 데이터베이스에 한 번 조회해봐야하나요?
There was a problem hiding this comment.
Student 객체에서 hasRoleAdmin() 메소드를 호출하여 권한을 확인하는 방식으로 변경하였습니다.
| public boolean hasRoleAdmin() { | ||
| return this.getAuthorities().stream() | ||
| .anyMatch(authority -> authority.getAuthorityName().equals("ROLE_ADMIN")); |
There was a problem hiding this comment.
Set인터페이스인데contain메소드로는 검증이 안 되나요?- 그리고 hasRoleAdmin이라는 Admin만 검사하는 메소드보다는 매개변수로 Role을 받고 Role이 있는지 검사하는 메소드로 만들어보면 어떨까요.
There was a problem hiding this comment.
해당 방식으로 코드 수정 완료하였습니다
| public class PostNotFoundException extends RuntimeException { | ||
| public PostNotFoundException(String Post_Not_Found) { | ||
| super("게시글을 찾을 수 없습니다"); | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
자바의 매개변수명 및 필드명은 카멜케이스를 사용합니다.
There was a problem hiding this comment.
카멜케이스 형태로 변경하였습니다
| public class UnauthorizedAccessException extends RuntimeException { | ||
| public UnauthorizedAccessException(String 권한이_없습니다) { | ||
| super("이 게시글에 대한 접근 권한이 없습니다."); | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
매개변수명에 한글은 좋지 않습니다. 영어만 사용하고 카멜케이스를 사용해주세요.
There was a problem hiding this comment.
매개변수명 영어로 변경완료하였습니다.
| private void assignRole(Student entity, String roleName) { | ||
| Authority authority = authorityRepository.findByAuthorityName(roleName) | ||
| .orElseThrow(() -> new RuntimeException(roleName + " 권한이 존재하지 않습니다.")); | ||
| entity.addAuthority(Authority.createRole(authority.getRole())); | ||
| } |
There was a problem hiding this comment.
entity.addAuthority(Authority.createRole(authority.getRole()));부분에서 이미 가져온 Authority가 있는데 왜 새로 Authority를 만들어서 Student에 넣어주나요?
There was a problem hiding this comment.
확인했습니다 해당 assignRole메소드 삭제 처리하였습니다
| public void assignAdminRole(String studentId) { | ||
| Student student = studentRepository.findByLoginId(studentId) | ||
| .orElseThrow(() -> new NotFoundStudentException("학생을 찾을 수 없습니다.")); | ||
|
|
||
| assignRole(student, "ROLE_ADMIN"); | ||
| } |
There was a problem hiding this comment.
이런 메소드라면 차라리 이 메소드가 호출되는 부분에서 assignRole(student, "ROLE_ADMIM")이런식으로 호출하면 되지 않을까요?
There was a problem hiding this comment.
헤당 메소드는 Student 객체에 지정된 role에 해당하는 권한을 Authority 객체를 통해 찾아 부여하는 기능을 기준으로 수정 되었습니다.
현재 PostApiController에 로거 사용했음에도 불구하고 테스트시 로그조차도 안남고 401에러가 뜨고 있습니다
테스트 완료되었습니다 POST기능 구현 완료되었습니다
테스트 완료되었습니다 POST기능 구현 완료되었습니다
테스트 완료되었습니다 POST기능 구현 완료되었습니다
#️⃣ Issue Number
#10
📝 요약(Summary)
#10 에 대한 게시판 기능 구현 완료됨
+α 작성 시간, 수정 시간 기록 기능 구현하였습니다
+α 잘못된 게시글 접근에 대한 예외처리 기능 구현하였습니다
🛠️ PR 유형
어떤 변경 사항이 있나요?
📸스크린샷 (선택)
💬 공유사항 to 리뷰어
게시글 생성(POST) 테스트 중에 401 에러가 있습니다
**오류로그
2025-08-01T15:59:26.531+09:00 INFO 14228 --- [nio-8080-exec-3] com.demo.service.CustomLoginService : Student(id=1, name=string, studentNumber=0, phoneNumber=string, loginId=string, password=$2a$10$TMyXXPukU9DXGZ5vKPh2b.NZbtg5umzOxaWcdsP/l6j.6FXIdpcWe, email=string, authorities=[com.demo.domain.Authority@6b35d5b2])
2025-08-01T15:59:26.606+09:00 INFO 14228 --- [nio-8080-exec-3] c.demo.controller.api.AuthApiController : UsernamePasswordAuthenticationToken [Principal=org.springframework.security.core.userdetails.User [Username=string, Password=[PROTECTED], Enabled=true, AccountNonExpired=true, CredentialsNonExpired=true, AccountNonLocked=true, Granted Authorities=[ROLE_USER]], Credentials=[PROTECTED], Authenticated=true, Details=null, Granted Authorities=[ROLE_USER]]
2025-08-01T16:00:20.601+09:00 INFO 14228 --- [nio-8080-exec-4] com.demo.jwt.TokenValidator : [TokenProvider] Token validation successful.
2025-08-01T16:00:20.603+09:00 INFO 14228 --- [nio-8080-exec-4] com.demo.jwt.TokenProvider : org.springframework.security.core.userdetails.User [Username=string, Password=[PROTECTED], Enabled=true, AccountNonExpired=true, CredentialsNonExpired=true, AccountNonLocked=true, Granted Authorities=[[ROLE_USER]]]
2025-08-01T16:00:20.603+09:00 INFO 14228 --- [nio-8080-exec-4] com.demo.jwt.JwtFilter : UsernamePasswordAuthenticationToken [Principal=org.springframework.security.core.userdetails.User [Username=string, Password=[PROTECTED], Enabled=true, AccountNonExpired=true, CredentialsNonExpired=true, AccountNonLocked=true, Granted Authorities=[[ROLE_USER]]], Credentials=[PROTECTED], Authenticated=true, Details=null, Granted Authorities=[[ROLE_USER]]]
2025-08-01T16:00:20.610+09:00 WARN 14228 --- [nio-8080-exec-4] .w.s.m.s.DefaultHandlerExceptionResolver : Resolved [org.springframework.web.bind.MissingServletRequestParameterException: Required request parameter 'studentId' for method parameter type String is not present]
2025-08-01T16:00:20.616+09:00 WARN 14228 --- [nio-8080-exec-4] c.demo.jwt.JwtAuthenticationEntryPoint : [JwtAuthenticationEntryPoint] Authentication failed for request URI: /error. Error message: Full authentication is required to access this resource
✅ PR Checklist
PR이 다음 요구 사항을 충족하는지 확인하세요.