[BE] refactor: 회원 추가로 인한 스키마 변경 #1075

Kimprodp · 2025-01-30T10:25:40Z

resolves [BE] 회원 도입에 따른 스키마 변경 #1074

🚀 어떤 기능을 구현했나요 ?

회원 기능이 추가되면서 일부 엔티티에 memberId가 필요하게 되었습니다.

reviewGroup에 회원을 식별할 수 있는 memberId 추가
review에 작성자를 식별할 수 있는 memberId 추가

🔥 어떻게 해결했나요 ?

참조가 아닌 long을 사용했습니다.
nullable 합니다.
회원이 생성한 ReviewGroup의 경우 GroupAccessCode도 nullable 해야 하기 때문에 같이 변경했습니다.

📝 어떤 부분에 집중해서 리뷰해야 할까요?

컨트롤러에서 커스텀 리졸버를 구현해서 Member를 받아온다고 가정하고 수정했습니다.
처음에는 서비스에서 회원/비회원 메서드를 분리해서 컨트롤러에서 세션 유무에 따라 분기처리하려 했습니다. 하지만 로직이 크게 바뀌는 것이 아니고 단순히 생성자로 전달하는 것만 달라지기 때문에 굳이 분기하지 않아도 된다고 생각해서 하나로 구현했습니다.
이건 회원이 있냐 없냐의 차이여서 회원과 비회원 세션을 모두 사용하는 api에서는 다르게 구현해야 할 수도 있어요.

📚 참고 자료, 할 말

github-actions · 2025-01-30T10:27:15Z

Test Results

157 tests 154 ✅ 4s ⏱️
59 suites 3 💤
59 files 0 ❌

Results for commit 21a6ec3.

♻️ This comment has been updated with latest results.

nayonsoso

컨트롤러에서 커스텀 리졸버를 구현해서 Member를 받아온다고 가정하고 수정했습니다.

동의합니다 👍
리졸버를 사용해야 Session > JWT 로 바뀐다고 하더라도 변경이 적을 것 같네요

테드상의 고민이 많이 느껴진 코드였습니다🥹👏👏
몇가지 부분 코멘트 남겼습니다~
빨리 머지되는게 좋을 것 같아서, 별로이면 시원하게 "별로임 ㅋ" 남겨줘도 됩니다잉

nayonsoso · 2025-01-31T15:19:58Z

backend/src/main/java/reviewme/reviewgroup/domain/GroupAccessCode.java

+    @Column(name = "group_access_code", nullable = true)
    private String code;


코드 리뷰하다가 흠칫... "그룹 엑세스 코드가 뭐지?" 싶었습니다😅
이제는 그룹 엑세스 코드가 {비회원이 리뷰 그룹 만들 때 입력하는 비밀번호}가 되었으니
그에 따라서 이름이 바뀌어야 할 것 같습니다.

다만 이 내용은 현재 PR에서 벗어나는 것 같고, 프론트와의 합의도 필요한 것이라 일단은 코멘트만 남겨놓습니다.
백로그로 생각해봅시다!
이슈 파뒀어요~ #1077

nayonsoso · 2025-01-31T15:23:48Z

backend/src/main/java/reviewme/reviewgroup/service/ReviewGroupService.java

+    public String generateReviewRequestCode() {
+        String reviewRequestCode;


함수 분리 👍👍
그런데 접근 제어자가 private 이어야 할 것 같아요~

실수했네요.. 수정했습니다👍

nayonsoso · 2025-01-31T15:24:46Z

backend/src/main/java/reviewme/reviewgroup/service/exception/GroupAccessCodeNullException.java

+@Slf4j
+public class GroupAccessCodeNullException extends NullPointerException{
+    public GroupAccessCodeNullException() {
+        super("비회원 리뷰 그룹 생성은 그룹 액세스 코드를 필수로 입력해야 해요.");
+        log.info("Non-member review group creation failed: Group access code is required");
+    }
+}


앞에 언급한 'reviewGroupAccessCode' 네이밍 변경의 필요성을 여기에서도 느꼈습니다..
예외 메시지가 한번에 이해되지 않네요😅

nayonsoso · 2025-01-31T15:37:00Z

backend/src/main/resources/db/migration/V6__review_group.sql

+ALTER TABLE review_group ADD COLUMN member_id NULL;
+ALTER TABLE review_group MODIFY COLUMN group_access_code VARCHAR(255) NULL:


member_id가 BIGINT라고 명시를 안해줘도 괜찮나요?

앗 빼먹었군요 수정했습니다👍

nayonsoso · 2025-01-31T16:24:43Z

backend/src/test/java/reviewme/reviewgroup/ReviewGroupTest.java

@@ -11,18 +11,37 @@
 class ReviewGroupTest {

    @Test
-    void 정상_생성된다() {
+    void 회원id로_생성한다() {


테스트 메서드가 직접적인 구현 내용을 포함하기보다,
"회원의 리뷰 그룹을 생성한다"와 같이 테스트 목적을 나타내게 하는건 어떨까요?
리뷰 그룹을 생성할 때 인자로 memberId 가 아니라 Member member 를 받게 한다면 깨질 테스트같네요.

동의합니다~

nayonsoso · 2025-01-31T16:32:26Z

backend/src/main/java/reviewme/review/domain/Review.java

+    public Review(long templateId, long reviewGroupId, List<Answer> answers) {
+        this(null, templateId, reviewGroupId, answers);
+    }
+


이 생성자는 이제 테스트코드에서만 사용되고 있네요🥲
테스트 코드에서만 사용되는 프로덕션 코드는 없는게 좋다 생각해요.

테스트 코드에서 사용되는 부분이 많긴 한데,..
따로 PR을 파기보다 이 PR에서 추가 커밋으로 작업하는건 어떤가요?
그것까지 포함해야 Review 에 member 라는 개념이 추가되었다고 볼 수 있을 것 같아서요!

사실 저도 프로덕션에는 프로덕션 코드만 있는게 맞다고 생각해서 전부 수정하려 했었는데요.
하다보니 null을 계속 사용하거나, 의미 없는 memberId를 명시하자니, 가독성이 너무 떨어지게 되어서 차라리 생성자를 나누면 확실하게 회원/비회원이 작성한 리뷰임을 생성했다고 알 수 있을 것 같아서 그냥 두었어요.

근데 역시 테스트 전용이라는 딱지가 붙는 것 같아 찝찝하네요. 😭👍
차라리 테스트코드에서만 생성자를 나눠서 사용할 수 있도록 ReviewFixture를 두어서 적용해봤습니다!

nayonsoso · 2025-01-31T16:35:36Z

backend/src/main/java/reviewme/review/controller/ReviewController.java

    @PostMapping("/v2/reviews")
-    public ResponseEntity<Void> createReview(@Valid @RequestBody ReviewRegisterRequest request) {
-        // 회원 세션 추후 추가해야 함
-        long savedReviewId = reviewRegisterService.registerReview(request);
+    public ResponseEntity<Void> createReview(
+            @Valid @RequestBody ReviewRegisterRequest request
+            /*
+            TODO: 회원 세션 임시 사용 방식, 이후 리졸버를 통해 객체로 받아와야 함
+            @Nullable @LoginMember Member member
+             */
+    ) {
+        /*
+        TODO: 회원 세션 유무에 따른 분기처리 로직
+        Long memberId = Optional.ofNullable(member).map(Member::getId).orElse(null);
+         */
+        long memberId = 1L; // 임시
+        long savedReviewId = reviewRegisterService.registerReview(request, memberId);


임시로 구현하기보다, Member member 를 컨트롤러에서 인자로 받고,
Long memberId = Optional.ofNullable(member).map(Member::getId).orElse(null);
를 주석 해제하는건 어떤가요?

이 코드대로라면 dev 서버에서 리뷰를 작성했을 때, 나중에 정합성 문제가 생길 것 같아요.
dev 서버긴 하더라도 나중에 귀찮을 일은 안만드는게 좋다 생각합니다 🙋🏻‍♀️

해당 시점에는 Member가 구현되어 있지 않아서 주석 처리 했습니다.
일단은 빠르게 머지하기 위해서 null을 전달하는 것으로 수정 하고 리졸버와 함께 바로 작업예정입니다~

nayonsoso · 2025-01-31T16:36:53Z

backend/src/main/java/reviewme/review/domain/Review.java

+    public Review(Long memberId, long templateId, long reviewGroupId, List<Answer> answers) {
+        this.memberId = memberId;
        this.templateId = templateId;
        this.reviewGroupId = reviewGroupId;
        this.answers = answers;
        this.createdAt = LocalDateTime.now();
    }


사소한 부분이긴 한데, null 이 될 수 있는 memberId 는 생성자에서 인자 순서를 맨 뒤로 하는게 어떨까요?
별로면 "별로임"이라고 남겨주세요!

별로임 😢

실제로 null을 전달하는 경우는 없으니, 필드 순서와 생성자 순서를 맞추는 것이 가독성이 더 좋다고 생각합니다..🙏

추가로, @nullable을 명시해서 null이 올 수 있다는 것을 명시했어요!

nayonsoso · 2025-01-31T16:37:24Z

backend/src/main/java/reviewme/reviewgroup/controller/ReviewGroupController.java

+            /*
+            TODO: 회원 세션 임시 사용 방식, 이후 리졸버를 통해 객체로 받아와야 함
+            @Nullable @LoginMember Member member
+             */
    ) {


저도 디토

nayonsoso · 2025-01-31T16:45:55Z

backend/src/main/java/reviewme/reviewgroup/domain/ReviewGroup.java

+    public ReviewGroup(Long memberId, long templateId, String reviewee, String projectName, String reviewRequestCode) {
+        this(memberId, templateId, reviewee, projectName, reviewRequestCode, null);
+    }
+
+    public ReviewGroup(long templateId, String reviewee, String projectName, String reviewRequestCode,
+                       String groupAccessCode) {
+        this(null, templateId, reviewee, projectName, reviewRequestCode, groupAccessCode);
    }


개인적으로 Review 등록 코드를 감명깊게 봤어요.
nullable 에 대한 특별한 처리 없이 Long memberId 를 넘겨주는게 깔끔하더라고요.
그래서 ReviewGroup 코드에서는 왜 이 방식으로 하지 않았지? 싶었어요.

public ReviewGroup(long templateId, String reviewee, String projectName, String reviewRequestCode, @Nullable Long memberId, @Nullable String groupAccessCode) { validateRevieweeLength(reviewee); validateProjectNameLength(projectName); validateMemberIdOrGroupAccessCode(memberId, groupAccessCode); this.memberId = memberId; this.templateId = templateId; this.reviewee = reviewee; this.projectName = projectName; this.reviewRequestCode = reviewRequestCode; } private void validateMemberIdOrGroupAccessCode(Long memberId, String groupAccessCode) { if ((memberId != null && groupAccessCode != null) || (memberId == null && groupAccessCode == null)) { throw new IllegalArgumentException("Either memberId or groupAccessCode must be null"); } }

이런식으로 코드를 바꾸고, 생성에서 오류가 날 수 있는 정책을 생성자 내부로 옮기는건 어떤가요?
그리고 서비스 코드에서는 깔끔하게 생성자 호출만 하고요!

Review는 단순히 작성자가 있느냐 없느냐 이 차이라서 상관없었지만, ReviewGroup에서는 memberId가 있으면 groupAccessCode가 존재하면 안된다는 것을 검증하는 것이 ReviewGroup 자체 도메인과는 별개로 회원 정책에 관여하다고 느껴서 서비스에서 분기처리 하는 것으로 구현했어요.

리뷰 그룹은 말 그대로 리뷰를 묶어서 관리하는 그룹인데 이 그룹이 회원 전용인지, 비회원 전용인지에 대한 개념 자체가 리뷰 그룹의 책임인가? 라고 볼 수 없을 것 같아요. 즉, 리뷰 그룹이라는 개념은 회원 전용/비회원 전용이라는 개념이 없어도 존재할 수 있는 도메인이예요. 따라서 우리가 리뷰 그룹이 Member를 몰라도 된다고 생각해서 간접 참조를 사용했는데 이를 검증하는 로직이 내부에 있다면 맞지 않다고 생각했어요.

반면 리뷰의 memberId는 말 그대로 작성자가 있냐 없냐의 개념이어서 단순히 데이터를 가지고 있기 때문에 어색함이 없어요.

해당 코멘트로 인해 조금 더 생각해보니, 리뷰 그룹에서 생성자를 나눠놓은 것 자체가 어느정도 책임을 가지고 있다는 생각이 드네요.
산초의 이 리뷰가 아니었다면 의도와 맞지 않는 코드가 될 뻔했네요. 👍👍
일단 정해진 기능 구현이 우선이니, 이 부분은 조금 더 생각해서 리팩터링 해보겠습니다. 🙏

nayonsoso

선프루브! 후팩터링!

cloudflare-workers-and-pages · 2025-02-10T13:51:53Z

Deploying 2024-review-me-release with Cloudflare Pages

Latest commit:	`5a4da23`
Status:	✅ Deploy successful!
Preview URL:	https://e2286523.2024-review-me-release.pages.dev
Branch Preview URL:	https://be-refactor-1074-schema-chan.2024-review-me-release.pages.dev

View logs

cloudflare-workers-and-pages · 2025-02-10T13:53:02Z

Deploying 2024-review-me-develop with Cloudflare Pages

Latest commit:	`5a4da23`
Status:	✅ Deploy successful!
Preview URL:	https://e17f094d.2024-review-me-develop.pages.dev
Branch Preview URL:	https://be-refactor-1074-schema-chan.2024-review-me-develop.pages.dev

View logs

Kimprodp added ✨ BE 우선순위: 중 🛠️ 리팩터링 labels Jan 30, 2025

Kimprodp added this to the 7차 스프린트 milestone Jan 30, 2025

Kimprodp requested a review from nayonsoso January 30, 2025 10:25

Kimprodp self-assigned this Jan 30, 2025

Kimprodp requested review from donghoony and skylar1220 as code owners January 30, 2025 10:25

Kimprodp linked an issue Jan 30, 2025 that may be closed by this pull request

[BE] 회원 도입에 따른 스키마 변경 #1074

Closed

nayonsoso requested changes Jan 31, 2025

View reviewed changes

nayonsoso mentioned this pull request Jan 31, 2025

[BE] 깃허브 인증 구현과 세션 권한 체계 설계 #1060

Merged

Kimprodp requested review from chysis, BadaHertz52, soosoo22 and ImxYJL as code owners February 10, 2025 06:20

nayonsoso approved these changes Feb 10, 2025

View reviewed changes

Kimprodp added 11 commits February 11, 2025 10:43

refactor: 리뷰 그룹에 memberId 추가 및 회원 종류에 따른 생성자 분리

2cf0c06

refactor: 회원 세션 유무에 따른 리뷰 그룹 생성 로직 분기처리

6455829

refactor: 리뷰 그룹의 그룹액세스코드 null 처리

59dab48

refactor: 확정되지 않은 파라미터 주석처리

3769bd2

test: 테스트 코드 수정

0a55d35

refactor: 리뷰 그룹 서비스의 메서드 분리를 다시 병합

8f03604

refactor: 리뷰에 작성자id(memberId)추가

8c8376e

test: 테스트 수정사항 반영

b087e88

refactor: 리뷰의 필드명 변경 (reviewerId -> memberId)

453e1bd

flyway: DB 스키마 변경

4dfbeb6

refactor: 접근 제어자 수정

82f6114

Kimprodp and others added 18 commits February 11, 2025 10:43

flyway: 컬럼 데이터 타입 명시

cca4735

test: 테스트명 변경

09e088e

refactor: 테스트에서만 사용되는 생성자 제거

9b52a5f

test: 회원과 비회원의 리뷰 생성에 따른 리뷰 Fixture 생성 및 적용

375fca1

refactor: 컨트롤러의 파라미터로 임시 id가 아닌 null을 전달하도록 변경

0311a62

refactor: Review 생성자에 @nullable 표시

526e6dc

test: api테스트에서 모킹 서비스 인자 전달 nullable로 변경

e6129d5

chore: ci gradle 캐시 삭제 (임시)

84b3ca6

chore: ci gradle 캐시 삭제 제거

2fb6aa3

chore: junit-vintage-engine 제외

88b7dab

chore: junit-vintage-engine 제외 -> 삭제

65d32cd

chore: 런타임 열거형 오류 방지 의존성 추가

8a5fad6

test: assertAll 임시 분리

071ab2d

chore: sql 문법 수정

bd01efe

chore: mockito extension 제거

f1071c3

chore: 런타임 열거형 오류 방지 의존성 추가 -> 삭제

2f75966

refactor: nullable 추가

56c814e

test: ReviewGroupServiceTest 삭제 (임시)

5a4da23

Kimprodp force-pushed the be/refactor/1074-schema-change branch from 9ce224a to 5a4da23 Compare February 11, 2025 01:44

Kimprodp added 2 commits February 11, 2025 10:51

test: ReviewGroupServiceTest 삭제 (임시) -> 추가

5327a0a

chore: 런타임 열거형 오류 방지 의존성 추가

21a6ec3

Kimprodp merged commit 4248c38 into develop Feb 11, 2025
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BE] refactor: 회원 추가로 인한 스키마 변경 #1075

[BE] refactor: 회원 추가로 인한 스키마 변경 #1075

Kimprodp commented Jan 30, 2025

github-actions bot commented Jan 30, 2025 •

edited

Loading

nayonsoso left a comment •

edited

Loading

nayonsoso Jan 31, 2025

nayonsoso Jan 31, 2025

Kimprodp Feb 10, 2025

nayonsoso Jan 31, 2025

nayonsoso Jan 31, 2025

Kimprodp Feb 10, 2025

nayonsoso Jan 31, 2025

Kimprodp Feb 10, 2025

nayonsoso Jan 31, 2025

Kimprodp Feb 10, 2025 •

edited

Loading

nayonsoso Jan 31, 2025

Kimprodp Feb 10, 2025 •

edited

Loading

nayonsoso Jan 31, 2025

Kimprodp Feb 10, 2025

Kimprodp Feb 10, 2025

nayonsoso Jan 31, 2025

Kimprodp Feb 10, 2025

nayonsoso Jan 31, 2025

Kimprodp Feb 10, 2025 •

edited

Loading

nayonsoso left a comment

cloudflare-workers-and-pages bot commented Feb 10, 2025 •

edited

Loading

cloudflare-workers-and-pages bot commented Feb 10, 2025 •

edited

Loading

		@Column(name = "group_access_code", nullable = true)
		private String code;

		public String generateReviewRequestCode() {
		String reviewRequestCode;

		ALTER TABLE review_group ADD COLUMN member_id NULL;
		ALTER TABLE review_group MODIFY COLUMN group_access_code VARCHAR(255) NULL:

[BE] refactor: 회원 추가로 인한 스키마 변경 #1075

[BE] refactor: 회원 추가로 인한 스키마 변경 #1075

Conversation

Kimprodp commented Jan 30, 2025

🚀 어떤 기능을 구현했나요 ?

🔥 어떻게 해결했나요 ?

📝 어떤 부분에 집중해서 리뷰해야 할까요?

📚 참고 자료, 할 말

github-actions bot commented Jan 30, 2025 • edited Loading

Test Results

nayonsoso left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kimprodp Feb 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kimprodp Feb 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kimprodp Feb 10, 2025 • edited Loading

Choose a reason for hiding this comment

nayonsoso left a comment

Choose a reason for hiding this comment

cloudflare-workers-and-pages bot commented Feb 10, 2025 • edited Loading

Deploying 2024-review-me-release with Cloudflare Pages

cloudflare-workers-and-pages bot commented Feb 10, 2025 • edited Loading

Deploying 2024-review-me-develop with Cloudflare Pages

github-actions bot commented Jan 30, 2025 •

edited

Loading

nayonsoso left a comment •

edited

Loading

Kimprodp Feb 10, 2025 •

edited

Loading

Kimprodp Feb 10, 2025 •

edited

Loading

Kimprodp Feb 10, 2025 •

edited

Loading

cloudflare-workers-and-pages bot commented Feb 10, 2025 •

edited

Loading

cloudflare-workers-and-pages bot commented Feb 10, 2025 •

edited

Loading