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

Trip과 DayLog 영속 관계 수정 #137

Merged
merged 6 commits into from
Jul 27, 2023
Merged

Trip과 DayLog 영속 관계 수정 #137

merged 6 commits into from
Jul 27, 2023

Conversation

mcodnjs
Copy link
Collaborator

@mcodnjs mcodnjs commented Jul 25, 2023

📄 Summary

#131

🙋🏻 More

영속화 왜안돼!
안되는줄 알았지만? 나의 능지? 였고 ?
Trip을 처음 저장하거나 완전히 삭제할 경우 DayLog에 Cascade 적용

@mcodnjs mcodnjs changed the title Trip과 DayLog 영속 관계 삭제 Trip과 DayLog 영속 관계 수정 Jul 25, 2023
Comment on lines 109 to 129
private void addEmptyDayLogs(Trip trip, int currentPeriod, int requestPeriod) {
final List<DayLog> emptyDayLogs = new ArrayList<>();
for (int i = currentPeriod; i < requestPeriod; i++) {
final DayLog emptyDayLog = DayLog.generateEmpty(i + 1, trip);
emptyDayLogs.add(emptyDayLog);
}
trip.getDayLogs().addAll(emptyDayLogs);
dayLogRepository.saveAll(emptyDayLogs);
}

private void removeRemainingDayLogs(Trip trip, int currentPeriod, int requestPeriod) {
final List<DayLog> dayLogsToRemove = new ArrayList<>();
trip.getDayLogs().removeIf(dayLog -> {
final boolean isRemovable = dayLog.getOrdinal() >= requestPeriod + 1 && dayLog.getOrdinal() <= currentPeriod;
if (isRemovable) {
dayLogsToRemove.add(dayLog);
}
return isRemovable;
});
dayLogRepository.deleteAll(dayLogsToRemove);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

파라미터 final 실종사건

Copy link
Member

Choose a reason for hiding this comment

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

😱😱😱😱😱😱😱😱😱

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

진짜 충격 그 자체네 ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@@ -35,7 +41,7 @@ public class DayLog extends BaseEntity {
@JoinColumn(name = "trip_id", nullable = false)
private Trip trip;

@OneToMany(mappedBy = "dayLog")
@OneToMany(mappedBy = "dayLog", cascade = REMOVE)
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

Copy link
Collaborator

Choose a reason for hiding this comment

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

얘는 PERSIST 필요없나요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

DayLog 영속화할 때 Item 저장할 일은 없을 것 같아서 안넣었는데 혹시 필요할까요 ?! ?
orphanRemoval = true 옵션은 추가했스빈다 ~

Comment on lines 54 to 56
@Mock
private DayLogRepository dayLogRepository;

Copy link
Collaborator

Choose a reason for hiding this comment

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

이거 테스트에서 안쓰여도 TripService에 있어서 선언해야하는건가요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

넵 .. 메소드 테스트할 때 호출되는 레포라 목 안해놓으면 NPE가 발생합니더

saveAllTripCities(cites, newTrip);
saveDayLogs(newTrip);
final Trip trip = tripRepository.save(newTrip);
return trip.getId();
}

private void saveDayLogs(final Trip savedTrip) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

진짜 별거아닌디 인자 이름 그냥 trip이어도 되지않을까요?.?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

final로 강제하고 싶어서 .. 그대로 유지했습니다 .. 🫠

@@ -35,7 +41,7 @@ public class DayLog extends BaseEntity {
@JoinColumn(name = "trip_id", nullable = false)
private Trip trip;

@OneToMany(mappedBy = "dayLog")
@OneToMany(mappedBy = "dayLog", cascade = REMOVE)
Copy link
Collaborator

Choose a reason for hiding this comment

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

얘는 PERSIST 필요없나요?

emptyDayLogs.add(emptyDayLog);
}
trip.getDayLogs().addAll(emptyDayLogs);
dayLogRepository.saveAll(emptyDayLogs);
Copy link
Collaborator

Choose a reason for hiding this comment

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

아하 dayLogRepo로 직접 save해서 cascadeType PERSIST 지우신 거 였군요. 왜 영속성 전이를 이용한 방법에서 repo로 save하는 방법으로 바꾸셨나요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

👍👍👍

Trip이 생성될 때 바뀌는 DayLog들이 아니라, Trip 수정 시 발생하는 DayLog 영속화를 위해 직접 save 했는데요!
아래와 같이 변경하였습니다 ~~

  • Merge 옵션으로 일부 DayLog 추가
  • orphanRemoval = true 옵션으로 일부 DayLog 삭제

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@@ -41,7 +47,7 @@ public class Trip extends BaseEntity {
@ColumnDefault("''")
private String description;

@OneToMany(mappedBy = "trip", cascade = CascadeType.PERSIST)
@OneToMany(mappedBy = "trip", cascade = {PERSIST, REMOVE, MERGE}, orphanRemoval = true)
Copy link
Member

Choose a reason for hiding this comment

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

이게 뭔가요? 설명좀 (진짜 몰라서 그럼)

Copy link
Member

Choose a reason for hiding this comment

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

아니다 직접 들어야겠다

Comment on lines 41 to 42
final Trip newTrip = new Trip(getInitTitle(cites), tripCreateRequest.getStartDate(), tripCreateRequest.getEndDate());
saveAllTripCities(cites, newTrip);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
final Trip newTrip = new Trip(getInitTitle(cites), tripCreateRequest.getStartDate(), tripCreateRequest.getEndDate());
saveAllTripCities(cites, newTrip);
final Trip newTrip = new Trip(
getInitTitle(cites),
tripCreateRequest.getStartDate(),
tripCreateRequest.getEndDate()
);

image

라인 넘어가서 바꿔주는게 좋을듯 합니다.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Comment on lines +49 to +50
final Period period = Period.between(savedTrip.getStartDate(), savedTrip.getEndDate().plusDays(1));
final List<DayLog> dayLogs = IntStream.rangeClosed(1, period.getDays() + 1)
Copy link
Member

Choose a reason for hiding this comment

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

네이밍이 멋지네요

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

알아요~

Comment on lines 107 to 108
.mapToObj(i -> DayLog.generateEmpty(i + 1, trip))
.toList();
Copy link
Member

Choose a reason for hiding this comment

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

네이밍 잘하는거 같은데 i도 다른걸로 바꿔주실 수 있나요? (의견임 그냥 이대로 가도 됨,,)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@github-actions
Copy link

📝 Jacoco Test Coverage

Total Project Coverage 81.73% 🍏
File Coverage [87.87%]
Trip.java 100% 🍏
DayLog.java 100% 🍏
TripService.java 85.76% 🍏
GlobalExceptionHandler.java 48.65%

@mcodnjs mcodnjs merged commit ea00d12 into develop Jul 27, 2023
1 check passed
@jjongwa jjongwa deleted the feature/#131-trip-bug branch August 1, 2023 01:58
@mcodnjs mcodnjs linked an issue Aug 2, 2023 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Trip 생성, 수정 API 버그 수정
4 participants