-
Notifications
You must be signed in to change notification settings - Fork 6
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
[BE] Refactor/#430 핀 복사 시 JdbcTemplate을 이용하여 bulk insert하도록 변경 #617
Changes from all commits
ca47a30
11f64ff
331db76
2486358
79dcbce
12ab047
974de2b
5186cc8
e6a30ba
60cab90
dc9a5b7
f907795
26db14d
585e135
7512b63
0073ed2
a57a16c
de403b5
58fc4ea
ab519fb
eabc420
bd7f38c
6976452
bda1f24
986ffb1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
package com.mapbefine.mapbefine.pin.infrastructure; | ||
|
||
import com.mapbefine.mapbefine.pin.domain.Pin; | ||
import com.mapbefine.mapbefine.topic.domain.Topic; | ||
import java.util.List; | ||
|
||
public interface PinBatchRepositoryCustom { | ||
|
||
int[] saveAllToTopic(Topic topicForCopy, List<Pin> originalPins); | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,142 @@ | ||
package com.mapbefine.mapbefine.pin.infrastructure; | ||
|
||
import static java.sql.Statement.EXECUTE_FAILED; | ||
|
||
import com.mapbefine.mapbefine.pin.domain.Pin; | ||
import com.mapbefine.mapbefine.pin.domain.PinImage; | ||
import com.mapbefine.mapbefine.topic.domain.Topic; | ||
import java.sql.PreparedStatement; | ||
import java.sql.SQLException; | ||
import java.sql.Timestamp; | ||
import java.time.LocalDateTime; | ||
import java.util.Collection; | ||
import java.util.List; | ||
import java.util.Objects; | ||
import java.util.stream.IntStream; | ||
import lombok.extern.slf4j.Slf4j; | ||
import org.springframework.jdbc.core.BatchPreparedStatementSetter; | ||
import org.springframework.jdbc.core.JdbcTemplate; | ||
import org.springframework.stereotype.Repository; | ||
|
||
@Slf4j | ||
@Repository | ||
public class PinBatchRepositoryCustomImpl implements PinBatchRepositoryCustom { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Impl 로 만들지 않는다면 작동을 안하나요? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @kpeel5839 https://cs-ssupport.tistory.com/511 다른 이름도 설정할 수 있는 방법이 있긴한데, 널리 쓰이진 않는 것 같아요 ㅋㅋ There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Impl 이 짱이긴하죠 껄껄 |
||
|
||
private final JdbcTemplate jdbcTemplate; | ||
|
||
public PinBatchRepositoryCustomImpl(JdbcTemplate jdbcTemplate) { | ||
this.jdbcTemplate = jdbcTemplate; | ||
} | ||
|
||
public int[] saveAllToTopic(Topic topicForCopy, List<Pin> originalPins) { | ||
int[] rowCount = bulkInsertPins(topicForCopy, originalPins); | ||
List<PinImageInsertDto> pinImageInsertDtos = createPinImageInsertDtos(originalPins, rowCount); | ||
|
||
if (pinImageInsertDtos.isEmpty()) { | ||
return rowCount; | ||
} | ||
return bulkInsertPinImages(pinImageInsertDtos); | ||
} | ||
|
||
private int[] bulkInsertPins(Topic topicForCopy, List<Pin> originalPins) { | ||
String bulkInsertSql = "INSERT INTO pin " | ||
+ "(name, description, member_id, topic_id, location_id, " | ||
+ "created_at, updated_at) " | ||
+ "VALUES " | ||
+ "(?, ?, ?, ?, ?, " | ||
+ "?, ?)"; | ||
Comment on lines
+42
to
+47
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 깔꼼스 하군요! |
||
LocalDateTime createdAt = topicForCopy.getLastPinUpdatedAt(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 아 이거를 위해서 근데 그렇다기에는 잘 모르겠네요 허허허허 |
||
Long topicId = topicForCopy.getId(); | ||
Long creatorId = topicForCopy.getCreator().getId(); | ||
log.debug("[Query] bulk insert size {} : {}", originalPins.size(), bulkInsertSql); | ||
|
||
return jdbcTemplate.batchUpdate(bulkInsertSql, new BatchPreparedStatementSetter() { | ||
@Override | ||
public void setValues(PreparedStatement ps, int i) throws SQLException { | ||
Pin pin = originalPins.get(i); | ||
ps.setString(1, pin.getName()); | ||
ps.setString(2, pin.getDescription()); | ||
ps.setLong(3, creatorId); | ||
ps.setLong(4, topicId); | ||
ps.setLong(5, pin.getLocation().getId()); | ||
ps.setTimestamp(6, Timestamp.valueOf(createdAt)); | ||
ps.setTimestamp(7, Timestamp.valueOf(createdAt)); | ||
log.trace("[Parameter Binding] {} : " | ||
+ "name={}, description={}, member_id={}, topic_id={}, location_id={}, " | ||
+ "created_at={}, updated_at={}", | ||
i, pin.getName(), pin.getDescription(), creatorId, topicId, pin.getLocation().getId(), | ||
createdAt, createdAt); | ||
} | ||
|
||
@Override | ||
public int getBatchSize() { | ||
return originalPins.size(); | ||
} | ||
}); | ||
} | ||
|
||
private List<PinImageInsertDto> createPinImageInsertDtos(List<Pin> originalPins, int[] rowCount) { | ||
Long firstIdFromBatch = jdbcTemplate.queryForObject("SELECT last_insert_id()", Long.class); | ||
validateId(firstIdFromBatch); | ||
Comment on lines
+78
to
+80
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 해당 쿼리문에 대한 DB 자체의 문제로 발생하는 거면, 내부적으로 다른 예외를 던져주지 않을까 싶은데.. 아닌가요 ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 맞네요. null일 가능성은 없네요! |
||
|
||
return IntStream.range(0, originalPins.size()) | ||
.filter(index -> rowCount[index] != EXECUTE_FAILED) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 |
||
.mapToObj(index -> { | ||
Pin pin = originalPins.get(index); | ||
return PinImageInsertDto.of(pin.getPinImages(), firstIdFromBatch + index); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 이 부분 아주 깔끔하게 해결해주셨네요 멋집니다 도이 |
||
}).flatMap(Collection::stream) | ||
.toList(); | ||
} | ||
|
||
private void validateId(Long firstIdFromBatch) { | ||
if (Objects.isNull(firstIdFromBatch)) { | ||
throw new IllegalStateException("fail to batch update pins"); | ||
} | ||
} | ||
|
||
private int[] bulkInsertPinImages(List<PinImageInsertDto> pinImages) { | ||
String bulkInsertSql = "INSERT INTO pin_image " | ||
+ "(image_url, pin_id) " | ||
+ "VALUES " | ||
+ "(?, ?)"; | ||
log.debug("[Query] bulk insert size {} : {}", pinImages.size(), bulkInsertSql); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 여기서 왜 역시 도이짱! |
||
|
||
return jdbcTemplate.batchUpdate(bulkInsertSql, new BatchPreparedStatementSetter() { | ||
@Override | ||
public void setValues(PreparedStatement ps, int i) throws SQLException { | ||
PinImageInsertDto pinImage = pinImages.get(i); | ||
ps.setString(1, pinImage.imageUrl); | ||
ps.setLong(2, pinImage.pinId); | ||
log.trace("[Parameter Binding] {} : imageUrl={}, pinImage={} ", | ||
i, pinImage.imageUrl, pinImage.pinId); | ||
} | ||
|
||
@Override | ||
public int getBatchSize() { | ||
return pinImages.size(); | ||
} | ||
}); | ||
} | ||
|
||
private record PinImageInsertDto( | ||
String imageUrl, | ||
Long pinId, | ||
boolean isDeleted | ||
) { | ||
Comment on lines
+121
to
+125
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 반영해주셔서 감사합니다~! |
||
|
||
public static PinImageInsertDto of(PinImage pinImage, Long pinId) { | ||
return new PinImageInsertDto( | ||
pinImage.getImageUrl(), | ||
pinId, | ||
pinImage.isDeleted() | ||
); | ||
} | ||
|
||
private static List<PinImageInsertDto> of(List<PinImage> pinImages, Long pinId) { | ||
return pinImages.stream() | ||
.map(pinImage -> PinImageInsertDto.of(pinImage, pinId)) | ||
.toList(); | ||
} | ||
} | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -55,12 +55,11 @@ public Long saveTopic(AuthMember member, TopicCreateRequest request) { | |
Topic topic = convertToTopic(member, request); | ||
List<Long> pinIds = request.pins(); | ||
|
||
topicRepository.save(topic); | ||
if (!pinIds.isEmpty()) { | ||
copyPinsToTopic(member, topic, pinIds); | ||
} | ||
|
||
topicRepository.save(topic); | ||
|
||
return topic.getId(); | ||
} | ||
|
||
|
@@ -105,14 +104,14 @@ private void copyPinsToTopic( | |
) { | ||
List<Pin> originalPins = findAllPins(pinIds); | ||
validateCopyablePins(member, originalPins); | ||
topic.increasePinCount(pinIds.size()); | ||
pinRepository.flush(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 여기도 flush 가 있는 것을 보니 batch update 를 진행하기 위해서는 명시적인 flush 호출이 필요한가보군요 흠흠 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. update할때는 flush를 해주어야 하는 게 맞아요! |
||
|
||
Member creator = findCreatorByAuthMember(member); | ||
|
||
originalPins.forEach(pin -> pin.copyToTopic(creator, topic)); | ||
pinRepository.saveAllToTopic(topic, originalPins); | ||
} | ||
|
||
private List<Pin> findAllPins(List<Long> pinIds) { | ||
List<Pin> findPins = pinRepository.findAllById(pinIds); | ||
List<Pin> findPins = pinRepository.findAllByIdIn(pinIds); | ||
|
||
if (pinIds.size() != findPins.size()) { | ||
throw new PinBadRequestException(ILLEGAL_PIN_ID); | ||
|
@@ -134,15 +133,13 @@ private void validateCopyablePins(AuthMember member, List<Pin> originalPins) { | |
public Long merge(AuthMember member, TopicMergeRequest request) { | ||
Topic topic = convertToTopic(member, request); | ||
List<Topic> originalTopics = findAllTopics(request.topics()); | ||
|
||
validateCopyableTopics(member, originalTopics); | ||
|
||
Member creator = findCreatorByAuthMember(member); | ||
List<Pin> originalPins = getAllPinsFromTopics(originalTopics); | ||
originalPins.forEach(pin -> pin.copyToTopic(creator, topic)); | ||
|
||
topicRepository.save(topic); | ||
topic.increasePinCount(originalPins.size()); | ||
|
||
topicRepository.save(topic); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 어라! 여기는 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 엥 그러네 ... |
||
pinRepository.saveAllToTopic(topic, originalPins); | ||
return topic.getId(); | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,27 +1,27 @@ | ||
package com.mapbefine.mapbefine.pin.domain; | ||
|
||
import static com.mapbefine.mapbefine.member.domain.Role.USER; | ||
import static org.assertj.core.api.Assertions.assertThat; | ||
import static org.assertj.core.api.SoftAssertions.assertSoftly; | ||
|
||
import com.mapbefine.mapbefine.TestDatabaseContainer; | ||
import com.mapbefine.mapbefine.common.annotation.RepositoryTest; | ||
import com.mapbefine.mapbefine.common.config.JpaConfig; | ||
import com.mapbefine.mapbefine.location.LocationFixture; | ||
import com.mapbefine.mapbefine.location.domain.Location; | ||
import com.mapbefine.mapbefine.location.domain.LocationRepository; | ||
import com.mapbefine.mapbefine.member.MemberFixture; | ||
import com.mapbefine.mapbefine.member.domain.Member; | ||
import com.mapbefine.mapbefine.member.domain.MemberRepository; | ||
import com.mapbefine.mapbefine.member.domain.Role; | ||
import com.mapbefine.mapbefine.pin.PinFixture; | ||
import com.mapbefine.mapbefine.pin.PinImageFixture; | ||
import com.mapbefine.mapbefine.topic.TopicFixture; | ||
import com.mapbefine.mapbefine.topic.domain.Topic; | ||
import com.mapbefine.mapbefine.topic.domain.TopicRepository; | ||
import java.util.List; | ||
import org.junit.jupiter.api.BeforeEach; | ||
import org.junit.jupiter.api.DisplayName; | ||
import org.junit.jupiter.api.Test; | ||
import org.springframework.beans.factory.annotation.Autowired; | ||
import org.springframework.boot.test.autoconfigure.orm.jpa.DataJpaTest; | ||
import org.springframework.context.annotation.Import; | ||
|
||
@RepositoryTest | ||
class PinRepositoryTest extends TestDatabaseContainer { | ||
|
@@ -31,6 +31,8 @@ class PinRepositoryTest extends TestDatabaseContainer { | |
@Autowired | ||
private PinRepository pinRepository; | ||
@Autowired | ||
private PinImageRepository pinImageRepository; | ||
@Autowired | ||
private LocationRepository locationRepository; | ||
@Autowired | ||
private MemberRepository memberRepository; | ||
|
@@ -41,7 +43,7 @@ class PinRepositoryTest extends TestDatabaseContainer { | |
|
||
@BeforeEach | ||
void setUp() { | ||
member = memberRepository.save(MemberFixture.create("member", "[email protected]", Role.USER)); | ||
member = memberRepository.save(MemberFixture.create("member", "[email protected]", USER)); | ||
topic = topicRepository.save(TopicFixture.createByName("topic", member)); | ||
location = locationRepository.save(LocationFixture.create()); | ||
} | ||
|
@@ -123,4 +125,39 @@ void deleteAllByMemberIdInOtherTopics_Success() { | |
assertThat(pinRepository.findAllByCreatorId(MemberId)).isEmpty(); | ||
} | ||
|
||
@Test | ||
@DisplayName("기존에 존재하는 핀들을 토픽에 한 번에 복사할 수 있다. (bulk insert)") | ||
void saveAllToTopic() { | ||
// given | ||
for (int i = 0; i < 10; i++) { | ||
Pin pin = pinRepository.save(PinFixture.create(location, topic, member)); | ||
pinRepository.flush(); | ||
pinImageRepository.save(PinImageFixture.create(pin)); | ||
} | ||
Member copier = memberRepository.save(MemberFixture.create("copier", "[email protected]", USER)); | ||
Topic topicForCopy = topicRepository.save(TopicFixture.createByName("otherTopic", copier)); | ||
|
||
// when | ||
List<Pin> originalPins = topic.getPins(); | ||
pinRepository.saveAllToTopic(topicForCopy, originalPins); | ||
|
||
// then | ||
List<Pin> copiedPins = pinRepository.findAllByTopicId(topicForCopy.getId()); | ||
List<PinInfo> originalPinInfos = originalPins.stream() | ||
.map(Pin::getPinInfo) | ||
.toList(); | ||
|
||
assertSoftly(softly -> { | ||
softly.assertThat(copiedPins).extracting("pinInfo") | ||
.usingRecursiveComparison() | ||
.isEqualTo(originalPinInfos); | ||
softly.assertThat(copiedPins.get(0).getCreator()) | ||
.isEqualTo(copier); | ||
softly.assertThat(copiedPins).hasSize(originalPins.size()) | ||
.flatMap(Pin::getPinImages) | ||
.allSatisfy(pinImage -> { | ||
assertThat(pinImage.getImageUrl()).isEqualTo(PinImageFixture.IMAGE_URL); | ||
}); | ||
}); | ||
} | ||
Comment on lines
+150
to
+162
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 전 몰랐어요 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 허걱스 몰랐어요. 근데 성능이 현저히 떨어진다는 건 테스트 시간이 많이 길어진다는 뜻인가요?! assertSoftly로 하면 assertAll과 달리 각 테스트가 실패한 코드 라인 위치까지 보여줘서 편하다구 생각했는데 이 점은 어떻게 생각하시나요..?! |
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
그리고 사용자 정의로 구현한 내용이 기본 구현이나 Repository aspect(공식문서 워딩인데 Repository에 대해 전역적으로 적용되는 기능들을 말하는 게 아닐까 싶어요)보다 우선시 된다고 하네용