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

지도/코스에 핀 추가 API 구현 #67

Merged
merged 17 commits into from
Nov 7, 2024
Merged

지도/코스에 핀 추가 API 구현 #67

merged 17 commits into from
Nov 7, 2024

Conversation

Miensoap
Copy link
Collaborator

@Miensoap Miensoap commented Nov 7, 2024

📄 Summary

지도/코스에 핀 추가 API 구현

🙋🏻 More

지도 상세 조회 버그 수정
지도 오타 수정
목 데이터 추가

orphanremoval , cascade 관련
entity 에 생성자 임의로 정의했을 때 오류 of 로 대체

코스 장소 추가시 응답 스키마 관련

장소 상세 정보를 모두 응답할지 vs id 와 한줄평만 응답할지

🕰️ Actual Time of Completion

3.5H

#64 -> #60 -> 이 PR 순으로 머지할게용
@koomchang 이 PR 위에서 유저 모듈 연동 후 코스 권한 관련 예외처리와 테스트 작성 진행해 주심 됩니다!!

close #65

@Miensoap Miensoap added BE 백엔드 관련 이슈입니다 🌱 기능추가 새로운 기능 요청입니다 🚑 버그 예기치 않은 문제 또는 의도하지 않은 동작입니다 ⚙️ 리팩터링 코드 퀄리티를 높입니다 labels Nov 7, 2024
@Miensoap Miensoap added this to the week2 milestone Nov 7, 2024
@Miensoap Miensoap self-assigned this Nov 7, 2024
@Miensoap Miensoap marked this pull request as ready for review November 7, 2024 04:38
Copy link
Collaborator

@hyohyo12 hyohyo12 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

수고하셨습니다!

backend/src/course/course.repository.ts Outdated Show resolved Hide resolved
@Miensoap Miensoap force-pushed the feature/#65 branch 2 times, most recently from 5f425ad to f086a1a Compare November 7, 2024 09:03
Copy link
Collaborator

@koomchang koomchang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

고생하셨습니다!

@@ -64,7 +64,7 @@ CREATE TABLE MAP_PLACE
(
id INT PRIMARY KEY AUTO_INCREMENT,
place_id INT NOT NULL,
map_id INT NOT NULL,
map_id INT,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

q: 나중에 바뀌는거죠?

Copy link
Collaborator Author

@Miensoap Miensoap Nov 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NOT NULL 로 바뀌냐는 질문인가요?

아니오! ORM 으로 관계 해제했을 때 null 로 업데이트 되는 과정에서
NOT NULL 제약조건으로 오류가 발생해 삭제했습니다.

orphanremoval 옵션을 사용해서 참조가 null 이 되었을 경우 레코드를 삭제하도록
어플리케이션 레벨에서 제어할 것 같아요.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@koomchang 지금보니 이거 다시 추가해도 될 것 같아요!
update set null 쿼리가 안나가게 할 수 있겠네요.

제가 cascade , orphanedRowAction, onDelete 관련 정리해서 개발 일지에 공유할게요!

@@ -0,0 +1,89 @@
-- USER 데이터 삽입
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

p3: csv 만들어둬도 괜찮을 듯합니다! 나중에 몇 백만건 테스트하려면!

async deleteCourse(@Param('id') id: number) {
return await this.courseService.deleteCourse(id);
@Put('/:id/places')
async SetPlacesOfCourse(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

p1: 네이밍 수정바랍니다!

Suggested change
async SetPlacesOfCourse(
async setPlacesOfCourse(

@@ -34,7 +40,7 @@ export class CourseService {
const publicMaps = maps.filter((map) => map.isPublic);

return {
maps: await Promise.all(publicMaps.map(CourseListResponse.from)),
courses: await Promise.all(publicMaps.map(CourseListResponse.from)),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

q: 제가 지식이 부족해서.. from 은 메소드인데 () 없이 쓸 수 있는게 정적 메서드라서 그런건가요?

Copy link
Collaborator Author

@Miensoap Miensoap Nov 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

사실 이거 저도
java 에서 CourseListResponse::from 으로 메서드 참조가 되는 거 생각하고
해보니까 이것도 되네? 하고 알게된건데요,

단일 인자고, static 메서드면 되는 것 같습니다

Comment on lines +119 to +125
await this.courseRepository.save(course); // Todo. Q.바로 장소 조회하면 장소 정보가 없음.. (장소 참조만 객체에 저장했기 때문)
const reloadedCourse = await this.courseRepository.findById(course.id);

return {
places: await getPlacesResponseOfCourseWithOrder(reloadedCourse),
};
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

q: save 하고 다시 findById 하는거에 대해서 설명 부탁드립니다!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

place 를 실제로 조회해서 넣은 것이 아니라, id 만 존재하는지 확인하고
{id : placeId} as place 형태로 참조만 추가해 COURSE_PLACE 테이블에 추가하도록 구현했는데요,

그 이후 place의 온전한 정보를 응답하려니, 위의 참조로 충분하지 않고,
실제 조회가 이루어져야 하더라구요.

이후에
place 상세정보 까진 응답하지 않는다 vs 미리 조회해서 응답한다
결정해서 수정할 것 같습니다!

Comment on lines +21 to +23
defaultMessage() {
return '동일한 장소는 연속된 순서로 추가할 수 없습니다.';
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

q: 이건 어디 쓰이는 건가요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A->B->C->A 코스는 가능하지만
A->A->B 는 허용하지 않는 것을 검증합니다

Comment on lines +8 to +9
@Column()
placeId: number;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

q: place 가 있는데 placeID가 있는 이유는 뭔가요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

place 테이블을 실제로 조회하지 않고 placeId 만 필요한 경우
이미 조회한 MapPlace 레코드의 데이터를 필드로 두어 사용합니다.

Comment on lines +16 to +17
onDelete: 'CASCADE',
orphanedRowAction: 'delete',
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

q: CASCADE 이외에 orphanedRowAction이 필요한 이유가 뭔가요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

관계가 끊어지면 update null 쿼리를 실행하는데요,
그 레코드들은 다시 사용될 일이 없기 때문에 삭제하도록 설정했습니다.

Comment on lines +24 to +27
@OneToMany(() => MapPlace, (mapPlace) => mapPlace.map, {
eager: true,
cascade: true,
})
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

q: 지연로딩과 즉시로딩에 대해서 설명 해주시면 좋을 것 같습니다! 각각 어떤 상황에서 쓰는 게 좋은지 궁금해요

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

우리 서비스에서 사례를 찾아보자면!

메인 페이지에서 지도 목록을 조회할 때,
연관된 테이블 Map - MapPlace - Place 까지 모두 로딩하려면 오래 걸리기 때문에

당장 필요한 핀 개수 ( MapPlace 레코드 ) 만 로딩하고,
MapPlace.place 가 호출되었을 때 장소 테이블을 조회하게 됩니다.

이 때 비동기적으로 조회해 반환하기 때문에, Promise 타입으로 선언합니다!

반대로,
메인 페이지에나 상세 페이지에나 항상 유저 프로필, 닉네임 등의 정보는 표시하기로 하였으니,
유저는 eager 타입으로 항상 함께 조회해 즉시 사용할 수 있게 설정하였습니다.

};
}

private async checkPlaceCanAddToMap(placeId: number, map: Map) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

p3: checkIfPlaceCanAddToMap 이 더 이해하기 좋을 것 같아요!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

검증하고 예외를 던지는 함수인데,
if 가 들어가니 저는 boolean 을 반환해야 할 것 같은 느낌이 들어요

ThrowErrorIfPlaceCannotAddedToMap() 처럼 되어야 자연스러울 것 같은 ??

@Miensoap Miensoap merged commit 142407d into develop Nov 7, 2024
2 checks passed
@Miensoap Miensoap deleted the feature/#65 branch November 13, 2024 07:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BE 백엔드 관련 이슈입니다 ⚙️ 리팩터링 코드 퀄리티를 높입니다 🌱 기능추가 새로운 기능 요청입니다 🚑 버그 예기치 않은 문제 또는 의도하지 않은 동작입니다
Projects
None yet
3 participants