feat: 유저가 선호하는 필터를 저장하는 기능을 구현한다 #453

sosow0212 · 2023-08-11T16:11:58Z

📄 Summary

유저가 선호하는 필터를 저장하는 기능을 구현했습니다.
테이블 MemberFilter(id, member_id, filter_id)를 통해서 필터를 관리합니다.

member, filter가 삭제된다면 해당하는 MemberFilter 테이블에 column도 같이 삭제됩니다.

🕰️ Actual Time of Completion

3시간

🙋🏻 More

close #452

drunkenhw

수고하셨습니다 인수테스트가 빠진 것 같아 확인부탁드립니다

drunkenhw · 2023-08-12T04:44:03Z

backend/src/main/java/com/carffeine/carffeine/filter/controller/dto/FiltersResponse.java

+    public static FiltersResponse fromMemberFilters(List<MemberFilter> memberFilters) {
+        List<Filter> filters = memberFilters.stream()
+                .map(MemberFilter::getFilter)
+                .toList();
+
+        return getFiltersResponse(filters);


filter response를 두 api에서 사용하는 것 같아보이는데요 이렇게 하신 특별한 이유가 있으실까요? 만약 하나를 위해 변경하면 둘 다 변경되는 일이 생길 것 같아서요

Filter 등록 후 반환과, 멤버가 선호 Filter를 등록하고 반환하는 형식은 같습니다
멤버가 선호하는 MemberFilter 역시도 Filter 테이블 안에 속하기 때문입니다.

그래서 따로 반환될 일은 없다고 생각했습니다

drunkenhw · 2023-08-12T04:45:23Z

backend/src/main/java/com/carffeine/carffeine/member/domain/MemberFilter.java

+
+    @ManyToOne
+    @JoinColumn(name = "member_id", nullable = false)
+    @OnDelete(action = OnDeleteAction.CASCADE)


on delete를 사용하신 이유가 무엇인가요?

mebmer_filter 테이블은 (id, member_id, filter_id) 컬럼을 가지고, 멤버가 선호하는 필터를 나타내는 테이블입니다.

만약 member_filter테이블에서 member가 제거되거나, filter가 제거된다면 의미가 없는 column이 됩니다.
그래서 참조하는 객체가 삭제되면 해당 객체도 삭제되는 것을 의도했습니다.

제가 질문이 너무 짧았네요 cascade와 orphanremoval 속성을 사용하지 않고 on delete 옵션을 사용하신 이유가 있나요?

아 지금 테이블 관계가 Member <--(1:n)-- MemberFilter --(n:1)--> Filter 이렇게 되어 있습니다.
단방향 관계로 MemberFilter에서만 Member, Filter를 참조하는 구조입니다.

그래서 orphanremoval은 적용할 수 없었고, cacade = CASCADE.REMOVE 혹은 OnDelete를 적용할 수 있었습니다.

제가 알기로는 cascade로 삭제하는 것은 jpa 레벨에서 진행되는 것이고, 참조하는 레코드 수 만큼 쿼리가 나가고, OnDelete로 삭제하는 것은 db단에서 처리하기 때문에 삭제쿼리가 나가지 않고 삭제가 되는 것으로 알고 있습니다!

혹시 틀린 부분이 있다면 말씀해주세요~

drunkenhw · 2023-08-12T04:47:57Z

backend/src/main/java/com/carffeine/carffeine/member/service/MemberService.java

+        return memberFilterRepository.findAllByMember(findMember(loginMember)).stream()
+                .map(MemberFilter::getFilter)
+                .collect(Collectors.toList());


가독성을 위해 find member는 변수로 분리하시는 것은 어떠신가요

drunkenhw · 2023-08-12T04:48:12Z

backend/src/main/java/com/carffeine/carffeine/member/service/MemberService.java

+
+    @Transactional(readOnly = true)
+    public List<Filter> findMemberFilters(Long memberId, Long loginMember) {
+        validateMember(memberId, loginMember);


id로만 비교하시는 이유가 있으신가요

저번에 대화를 통해 해당 API는 RESTful 원칙을 따르기로 했는데요.

그래서 현재 /api/members/{memberId}/filters의 구조를 가지고 있습니다.
또한 header로 로그인한 유저의 token또한 가지고 있습니다.

따라서 서버에 들어오는 데이터는 총 두 가지입니다.

path에서 들어오는 {memberId}

token을 기준으로 받아오는 memberId

그래서 본인인지 검증하는 방식을 path에서 memberId와 token에서 memberId로 먼저 비교해줬습니다.

id가 pk이므로 겹칠 일이 없어서 괜찮다고 생각했기 때문입니다.

제이가 하신 방법으로도 할 수 있을 것 같은데

loginMember를 repository에서 찾아온다

없으면 예외를 던진다

있으면 path variable에 받아온 memberId와 loginMember객체의 id를 검증한다

로직을 수행한다
이러한 플로우로 할 수도 있었을 것 같은데 바로 id를 비교하시는 이유가 무엇인지 궁금했습니다

저는 두 개의 로직을 나눴는데, 현재 모든 메서드가 검증 후 멤버를 받아서 써야하네요!
박스터 리뷰대로 바꾸는게 좋아보입니다 👍

drunkenhw · 2023-08-12T04:50:12Z

backend/src/main/java/com/carffeine/carffeine/member/service/MemberService.java

+                .toList();
+
+        return filters.stream()
+                .map(it -> new MemberFilter(member, Filter.of(it.getName(), it.getFilterType().getName())))


Suggested change

.map(it -> new MemberFilter(member, Filter.of(it.getName(), it.getFilterType().getName())))

.map(it -> new MemberFilter(member, it))

이 부분 정상 동작하나요? 이렇게 바꿔야할 것 같아서요

아이고 이부분 인수테스트 하다가 발견했네요 수정했습니다!

drunkenhw · 2023-08-12T04:50:48Z

backend/src/test/java/com/carffeine/carffeine/member/controller/MemberControllerTest.java

+        when(memberService.findMemberFilters(any(), any())).thenReturn(filters);
+
+        // then
+        mockMvc.perform(RestDocumentationRequestBuilders.get("/members/{memberId}/filters", 1L)


static import 부탁드려요

drunkenhw · 2023-08-12T04:51:53Z

backend/src/test/java/com/carffeine/carffeine/member/domain/FakeMemberFilterRepository.java

+    private Long id = 0L;
+
+    @Override
+    public List<MemberFilter> findAllByMember(final Member member) {


final 지워주세요

drunkenhw · 2023-08-12T04:54:13Z

backend/src/test/java/com/carffeine/carffeine/member/service/MemberServiceTest.java

+        List<Filter> result = memberService.findMemberFilters(member.getId(), member.getId());
+
+        // then
+        assertThat(memberFilters.get(0).getFilter().getName()).isEqualTo(result.get(0).getName());


get name으로 비교하신 이유가 있으신가요?

equals hashcode 재정의를 했는데 name으로 비교를 했네요😅

drunkenhw · 2023-08-12T04:55:48Z

backend/src/test/java/com/carffeine/carffeine/member/service/MemberServiceTest.java

+        assertSoftly(softly -> {
+            softly.assertThat(result.size()).isEqualTo(memberFilters.size());
+        });


하나만 검증하면 assert softly는 제거해도 되지않을까요
그리고 혹시 다른 회원이 필터를 지우면 발생하는 예외케이스도 검증해주실 수 있을까요?

넵 꼼꼼한 리뷰 감사합니다

kiarakim

수고하셧습니다아

drunkenhw

수고하셨습니다

drunkenhw · 2023-08-12T05:49:39Z

backend/src/main/java/com/carffeine/carffeine/member/domain/MemberFilter.java

+
+    @ManyToOne
+    @JoinColumn(name = "member_id", nullable = false)
+    @OnDelete(action = OnDeleteAction.CASCADE)


제가 질문이 너무 짧았네요 cascade와 orphanremoval 속성을 사용하지 않고 on delete 옵션을 사용하신 이유가 있나요?

drunkenhw · 2023-08-12T05:51:40Z

backend/src/main/java/com/carffeine/carffeine/member/service/MemberService.java

+
+    @Transactional(readOnly = true)
+    public List<Filter> findMemberFilters(Long memberId, Long loginMember) {
+        validateMember(memberId, loginMember);


제이가 하신 방법으로도 할 수 있을 것 같은데

loginMember를 repository에서 찾아온다

없으면 예외를 던진다

있으면 path variable에 받아온 memberId와 loginMember객체의 id를 검증한다

로직을 수행한다
이러한 플로우로 할 수도 있었을 것 같은데 바로 id를 비교하시는 이유가 무엇인지 궁금했습니다

drunkenhw · 2023-08-12T05:53:16Z

backend/src/main/java/com/carffeine/carffeine/member/service/MemberService.java

+        memberFilterRepository.deleteAllByMember(member);
+
+        return memberFilterRepository.saveAll(makeMemberFilters(filtersRequest, member));


다 지우고 다 저장하는 것 밖에 방법이 없겠죠?
저는 마땅히 떠오르지 않는데 뭔가 더 좋은 방법이 있을 것 같아서요

비교해서 바꾸는 방법도 있긴 할 것 같은데, 효율적일지는 모르겠어요 ㅠ
고민 조금 더 해볼게요

drunkenhw · 2023-08-12T05:55:00Z

...nd/src/test/java/com/carffeine/carffeine/member/integration/MemberFilterIntegrationTest.java

+        관리자 = memberRepository.save(Member.builder()
+                .id(2L)
+                .memberRole(MemberRole.ADMIN)
+                .build());
+        관리자_토큰 = "Bearer " + provider.create(관리자.getId());
+
+        필터_리스트 = new FiltersRequest(
+                List.of(
+                        new FilterRequest(FilterType.COMPANY.getName(), "광주시"),
+                        new FilterRequest(FilterType.CAPACITY.getName(), "2.00"),
+                        new FilterRequest(FilterType.CONNECTOR_TYPE.getName(), "DC_COMBO")
+                )
+        );
+
+        생성_요청("/filters", 필터_리스트, 관리자_토큰);


다른 부분은 repo에 save를 직접하는데 필터를 저장하는 부분은 요청을 보내 저장하는 이유가 있을까요?
이렇게 되면 시간이 조금 더 걸릴 것 같아서요

be-student

고생 많으셨습니다! 👍

sosow0212 added 2 commits August 11, 2023 23:51

feat: MemberFilter API 구현

6cfc5e5

test: 테스트 작성

ec8847a

sosow0212 added 🌱 기능추가 새로운 기능 요청입니다 BE 백엔드 관련 이슈입니다 labels Aug 11, 2023

sosow0212 self-assigned this Aug 11, 2023

sosow0212 requested review from be-student, drunkenhw and kiarakim as code owners August 11, 2023 16:11

sosow0212 temporarily deployed to test August 11, 2023 16:12 — with GitHub Actions Inactive

drunkenhw requested changes Aug 12, 2023

View reviewed changes

sosow0212 added 2 commits August 12, 2023 13:59

test: 인수 테스트 추가

1a6dae3

fix: 연관 관계가 맺어지지 않는 오류 해결

54a8b74

sosow0212 temporarily deployed to test August 12, 2023 05:00 — with GitHub Actions Inactive

refactor: 코드 리팩토링 및 final 키워드 제거

f3da5c7

sosow0212 temporarily deployed to test August 12, 2023 05:34 — with GitHub Actions Inactive

kiarakim approved these changes Aug 12, 2023

View reviewed changes

drunkenhw approved these changes Aug 12, 2023

View reviewed changes

be-student approved these changes Aug 12, 2023

View reviewed changes

sosow0212 added 2 commits August 12, 2023 16:16

refactor: 메서드 리팩토링

6c32b43

refactor: 테스트 리팩토링

ac2f7ce

sosow0212 temporarily deployed to test August 12, 2023 07:22 — with GitHub Actions Inactive

sosow0212 merged commit e20c7c8 into develop Aug 12, 2023
1 check passed

sosow0212 deleted the feat/452 branch August 12, 2023 07:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: 유저가 선호하는 필터를 저장하는 기능을 구현한다 #453

feat: 유저가 선호하는 필터를 저장하는 기능을 구현한다 #453

sosow0212 commented Aug 11, 2023 •

edited by github-actions bot

drunkenhw left a comment

drunkenhw Aug 12, 2023

sosow0212 Aug 12, 2023

drunkenhw Aug 12, 2023

sosow0212 Aug 12, 2023

drunkenhw Aug 12, 2023

sosow0212 Aug 12, 2023

drunkenhw Aug 12, 2023

drunkenhw Aug 12, 2023

sosow0212 Aug 12, 2023

drunkenhw Aug 12, 2023

sosow0212 Aug 12, 2023

drunkenhw Aug 12, 2023

sosow0212 Aug 12, 2023

drunkenhw Aug 12, 2023

drunkenhw Aug 12, 2023

drunkenhw Aug 12, 2023

sosow0212 Aug 12, 2023

drunkenhw Aug 12, 2023

sosow0212 Aug 12, 2023

kiarakim left a comment

drunkenhw left a comment

drunkenhw Aug 12, 2023

drunkenhw Aug 12, 2023

drunkenhw Aug 12, 2023

sosow0212 Aug 12, 2023

drunkenhw Aug 12, 2023

be-student left a comment

	.map(it -> new MemberFilter(member, Filter.of(it.getName(), it.getFilterType().getName())))
	.map(it -> new MemberFilter(member, it))

		memberFilterRepository.deleteAllByMember(member);

		return memberFilterRepository.saveAll(makeMemberFilters(filtersRequest, member));

feat: 유저가 선호하는 필터를 저장하는 기능을 구현한다 #453

feat: 유저가 선호하는 필터를 저장하는 기능을 구현한다 #453

Conversation

sosow0212 commented Aug 11, 2023 • edited by github-actions bot

📄 Summary

🕰️ Actual Time of Completion

🙋🏻 More

drunkenhw left a 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

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

Choose a reason for hiding this comment

kiarakim left a comment

Choose a reason for hiding this comment

drunkenhw left a 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

be-student left a comment

Choose a reason for hiding this comment

sosow0212 commented Aug 11, 2023 •

edited by github-actions bot