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

Feat/#23 Log를 저장할 수 있는 기능 추가 #30

Merged
merged 8 commits into from
Aug 13, 2024
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
package info.logbat.log.domain;
package info.logbat.domain.log.domain;

import info.logbat.domain.log.domain.values.LogData;
import info.logbat.domain.log.domain.enums.Level;

import info.logbat.log.domain.enums.Level;
import info.logbat.log.domain.values.LogData;
import java.time.LocalDateTime;
import lombok.Getter;

Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
package info.logbat.log.domain.enums;
package info.logbat.domain.log.domain.enums;

public enum Level {
ERROR,
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
package info.logbat.log.domain.values;
package info.logbat.domain.log.domain.values;

import lombok.Getter;

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
package info.logbat.domain.log.repository;

import info.logbat.domain.log.domain.Log;
import java.sql.PreparedStatement;
import java.sql.Statement;
import java.sql.Timestamp;
import java.util.Optional;
import lombok.AllArgsConstructor;
import org.springframework.dao.EmptyResultDataAccessException;
import org.springframework.jdbc.core.JdbcTemplate;
import org.springframework.jdbc.core.RowMapper;
import org.springframework.jdbc.support.GeneratedKeyHolder;
import org.springframework.jdbc.support.KeyHolder;
import org.springframework.stereotype.Repository;

@Repository
@AllArgsConstructor
public class LogRepository {

private final JdbcTemplate jdbcTemplate;

public long save(Log log) {
String sql = "INSERT INTO logs (application_id, level, log_data, timestamp) VALUES (?, ?, ?, ?)";

KeyHolder keyHolder = new GeneratedKeyHolder();

jdbcTemplate.update(connection -> {
PreparedStatement ps = connection.prepareStatement(sql, Statement.RETURN_GENERATED_KEYS);
ps.setLong(1, log.getApplicationId());
ps.setString(2, log.getLevel().name());
ps.setString(3, log.getLogData().getValue());
ps.setTimestamp(4, Timestamp.valueOf(log.getTimestamp()));
return ps;
}, keyHolder);

return Optional.ofNullable(keyHolder.getKey())
.map(Number::longValue)
.orElseThrow(() -> new IllegalStateException("로그 저장에 실패했습니다."));
}

public Optional<Log> findById(Long logId) {
String sql = "SELECT * FROM logs WHERE log_id = ?";

try {
return Optional.ofNullable(
jdbcTemplate.queryForObject(
sql,
LOG_ROW_MAPPER,
logId
));
} catch (EmptyResultDataAccessException e) {
return Optional.empty();
}
}

private static final RowMapper<Log> LOG_ROW_MAPPER = (rs, rowNum) -> new Log(
rs.getLong("log_id"),
rs.getLong("application_id"),
rs.getString("level"),
rs.getString("log_data"),
rs.getTimestamp("timestamp").toLocalDateTime()
);
}

Choose a reason for hiding this comment

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

코드 패치에 대한 간단한 리뷰를 제공하겠습니다.

버그 위험

  1. 타임스탬프 변환: log.getTimestamp()가 null일 경우, Timestamp.valueOf(log.getTimestamp())에서 NullPointerException이 발생할 수 있습니다. 이 부분에 대한 검증이 필요합니다.
  2. 로그 레벨 문자열 처리: log.getLevel().name()이 예상하지 못한 값을 반환할 수 있는지 확인해야 합니다. Enum의 값이 데이터베이스의 값과 다를 경우 문제를 일으킬 수 있습니다.
  3. SQL 예외 처리: 데이터베이스 관련 예외가 발생 시 적절한 처리가 필요합니다.

개선 제안

  1. Optional 사용: findById 메서드에서 Optional.ofNullable 대신 jdbcTemplate.queryForObject의 결과가 null인 경우 외부에서 처리하도록 할 수도 있습니다.
  2. 로깅 추가: SQL 성공 및 실패에 대한 로깅을 추가하는 것이 좋습니다. 이는 디버깅 시 유용할 수 있습니다.
  3. 매직 문자열 제거: SQL 쿼리에서 테이블 이름과 컬럼 이름을 상수로 정의하여 하드코딩된 문자열을 줄여 유지보수를 용이하게 할 수 있습니다.
  4. 트랜잭션 관리: 여러 데이터베이스 작업을 수행할 경우 트랜잭션 관리를 고려해야 합니다.

이러한 점들에 유의하면 코드의 안정성과 가독성을 높일 수 있을 것입니다.

Copy link
Member

Choose a reason for hiding this comment

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

실패 케이스도 작성하면 좋을 것 같습니다 :D

Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
package info.logbat.domain.log.repository;

import static org.assertj.core.api.Assertions.assertThat;

import info.logbat.domain.log.domain.Log;
import info.logbat.domain.log.domain.enums.Level;
import jakarta.transaction.Transactional;
import java.time.LocalDateTime;
import java.util.Optional;
import org.junit.jupiter.api.DisplayName;
import org.junit.jupiter.api.Test;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.boot.test.context.SpringBootTest;
import org.springframework.test.context.ActiveProfiles;

@SpringBootTest
@Transactional
@ActiveProfiles("test")
@DisplayName("LogRepository 테스트")
class LogRepositoryTest {

@Autowired
private LogRepository logRepository;

@DisplayName("Log를 저장할 수 있다.")
@Test
void saveLog() {
// given
String 로그_레벨 = "INFO";
String 로그_데이터 = "테스트_로그_데이터";
LocalDateTime 타임스탬프 = LocalDateTime.of(2021, 1, 1, 0, 0, 0);

Log 로그 = new Log(1L, 로그_레벨, 로그_데이터, 타임스탬프);

// when
long 저장된_ID = logRepository.save(로그);

// then
assertThat(저장된_ID)
.isPositive();
}

@DisplayName("저장한 Log를 조회할 수 있다.")
@Test
void findLog() {
// given
String 로그_레벨 = "INFO";
String 로그_데이터 = "테스트_로그_데이터";
LocalDateTime 타임스탬프 = LocalDateTime.of(2021, 1, 1, 0, 0, 0);

long 로그_ID = logRepository.save(new Log(1L, 로그_레벨, 로그_데이터, 타임스탬프));

// when
Optional<Log> 저장된_로그 = logRepository.findById(로그_ID);

// then
assertThat(저장된_로그).isPresent()
.get()
.extracting("logId", "applicationId", "level", "logData.value", "timestamp")
.containsExactly(로그_ID, 1L, Level.INFO, "테스트_로그_데이터",
LocalDateTime.of(2021, 1, 1, 0, 0, 0));
}

@DisplayName("없는 Log를 조회하면 빈 Optional을 반환한다.")
@Test
void findNonexistentLog() {
// given
long 없는_로그_ID = 1L;

// when
Optional<Log> 저장된_로그 = logRepository.findById(없는_로그_ID);

// then
assertThat(저장된_로그).isEmpty();
}
}

Choose a reason for hiding this comment

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

코드 리뷰 결과는 다음과 같습니다:

  1. 버그 리스크:

    • Log 객체의 생성자 호출 시 applicationId가 상수로 1L로 하드코딩되어 있습니다. 데이터베이스에 여러 애플리케이션에서 로그를 저장할 경우, 이 값이 유일성을 보장하지 못해 오류가 발생할 수 있습니다.
  2. 테스트 독립성:

    • saveLogfindLog 테스트가 같은 데이터베이스를 공유하므로, 하나의 테스트가 다른 테스트에 영향을 미칠 수 있습니다. 테스트를 분리하거나 데이터를 정리하는 방법을 고려해보세요.
  3. 대안적인 Assertions 사용:

    • extracting 메서드를 사용하여 각 속성을 추출하고 비교하는 방식은 좋지만, 실수를 방지하기 위해 각 속성에 대해 별도로 검증하는 것도 고려해볼 수 있습니다.
  4. 직관적인 변수명:

    • 한글 변수를 사용하는 것은 좋지만, 영어와 혼용될 경우 코드 가독성이 떨어질 수 있습니다. 일관된 네이밍 컨벤션을 유지하는 것이 좋습니다.
  5. 타임스탬프 하드코딩:

    • 타임스탬프를 하드코딩하는 대신, 현재 시간을 기준으로 생성하는 것을 검토해볼 필요성이 있습니다. 이는 미래의 변경 사항에 유연하게 대응할 수 있게 합니다.
  6. 예외 처리:

    • logRepository.save() 또는 logRepository.findById() 메서드 호출 시 예외 발생 가능성을 염두에 두고 적절한 예외 처리를 고려하세요.
  7. 주석 추가:

    • 테스트 의도를 명확히 하기 위해 주석을 보강하는 것이 좋습니다. 특히 복잡한 로직이 포함될 경우에는 더욱 그러합니다.

이러한 점들을 개선하면 코드의 안정성과 가독성을 높이는 데 도움이 될 것입니다.

Choose a reason for hiding this comment

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

코드 리뷰를 시작하겠습니다.

버그 리스크 및 개선 제안

  1. 로그 ID 오버라이딩:

    • new Log(1L, ...)에서 로그 ID가 하드코딩되어 있습니다. 문제가 발생할 수 있는데, 이는 다른 로그와 충돌할 위험이 있습니다. ID는 자동 생성되도록 조정하는 것이 좋습니다.
  2. 존재하지 않는 로그 조회 테스트:

    • findNonexistentLog() 메서드에서 항상 1L을 조회하고 있습니다. 테스트 데이터베이스에 따라 이 ID가 존재할 수도 있습니다. 더 안전한 방법으로 랜덤이나 최대값 이상의 숫자를 사용하는 것이 좋습니다.
  3. 테스트 데이터 정리:

    • 현재 코드에서는 테스트 후 데이터 클린업이 없습니다. @Transactional 어노테이션으로 인한 롤백을 사용할 수 있지만, 확실히 데이터가 분리되어야 하는 경우 주의해야 합니다.
  4. 예외 처리:

    • 레벨이나 로깅 데이터가 null일 때의 처리가 없습니다. 이는 예외를 발생시킬 수 있으므로, 유효성 검사를 추가하는 것이 좋습니다.
  5. 타임스탬프 매직 넘버 문제:

    • LocalDateTime.of(2021, 1, 1, 0, 0, 0)와 같은 매직 넘버는 상수로 정의하여 의미를 명확히 하는 것이 좋습니다. 예를 들어, DEFAULT_TIMESTAMP와 같이 표현할 수 있습니다.
  6. 로깅 및 디버깅:

    • 실패 시 테스트 결과를 더 명확하게 확인하기 위해 assert문에 커스텀 메시지를 추가하면 좋습니다. 이렇게 하면 향후 문제가 생겼을 때 원인을 찾기 쉬워집니다.
  7. assertThat 사용 시 경고:

    • 여러 프로퍼티를 추출할 때, getter 메서드를 활용하는 것이 명시적이며 좋습니다. 직접적인 필드 접근보다는 public getter 메서드를 통해 접근하는 것이 바람직합니다.

결론

전반적으로 잘 구성된 테스트 코드임에도 불구하고 몇 가지 잠재적인 문제점과 개선 사항이 발견되었습니다. 이러한 점들을 고려하여 코드를 보강하면 더 안정적인 테스트 케이스가 될 것입니다.

Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
package info.logbat.log.domain;
package info.logbat.domain.project.domain.log.domain;

import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatThrownBy;

import info.logbat.log.domain.enums.Level;
import info.logbat.domain.log.domain.Log;
import info.logbat.domain.log.domain.enums.Level;
import java.time.LocalDateTime;
import org.junit.jupiter.api.DisplayName;
import org.junit.jupiter.api.Nested;
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
package info.logbat.log.domain.enums;
package info.logbat.domain.project.domain.log.domain.enums;

import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatThrownBy;
Expand All @@ -10,6 +10,8 @@
import org.junit.jupiter.params.provider.Arguments;
import org.junit.jupiter.params.provider.MethodSource;

import info.logbat.domain.log.domain.enums.Level;

@DisplayName("로그 Level enum 테스트")
class LevelTest {

Expand Down
Original file line number Diff line number Diff line change
@@ -1,11 +1,13 @@
package info.logbat.log.domain.values;
package info.logbat.domain.project.domain.log.domain.values;

import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatThrownBy;

import org.junit.jupiter.api.DisplayName;
import org.junit.jupiter.api.Test;

import info.logbat.domain.log.domain.values.LogData;

@DisplayName("LogData VO 테스트")
class LogDataTest {

Expand Down