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

[miiiinju1]

🚀 개발 사항

• Log DDL 설계

CREATE TABLE logs (
    log_id BIGINT AUTO_INCREMENT PRIMARY KEY,
    application_id BIGINT NOT NULL,
    level ENUM('INFO', 'ERROR') NOT NULL, 
    log_data TEXT NOT NULL,
    timestamp DATETIME NOT NULL
);

• Log를 저장하는 LogRepository를 JdbcTemplate로 개발했습니다.
• LogRepository에서 작동하는 사항을 검증하기 위해 Test를 추가했습니다.

이슈 번호

• close [💡 FEAT] log를 저장한다. #23

특이 사항 🫶

• Repository 테스트는 @ActiveProfile("test")를 추가하여 테스트 프로파일을 사용하도록 만들었습니다.

[github-actions]

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

버그 위험

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. 트랜잭션 관리: 여러 데이터베이스 작업을 수행할 경우 트랜잭션 관리를 고려해야 합니다.

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

[github-actions]

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

1. 버그 리스크:

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

2. 테스트 독립성:

   • saveLog와 findLog 테스트가 같은 데이터베이스를 공유하므로, 하나의 테스트가 다른 테스트에 영향을 미칠 수 있습니다. 테스트를 분리하거나 데이터를 정리하는 방법을 고려해보세요.

3. 대안적인 Assertions 사용:

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

4. 직관적인 변수명:

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

5. 타임스탬프 하드코딩:

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

6. 예외 처리:

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

7. 주석 추가:

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

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

[tidavid1]

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

[tidavid1]

👍🏻

[github-actions]

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

버그 리스크 및 개선 제안

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 메서드를 통해 접근하는 것이 바람직합니다.

결론

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

[LuizyHub]

확인했습니다! 람다로 매퍼 구현한 부분 더 같이 고민해봐요